LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: Oskar Senft <osk@google.com>,
	linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: Jean Delvare <jdelvare@suse.com>
Subject: Re: [PATCH] hwmon: (nct7802) Make RTD modes configurable.
Date: Fri, 10 Sep 2021 09:21:55 -0700	[thread overview]
Message-ID: <99a84071-f737-9a8b-73fa-bcc3e0ff6a3f@roeck-us.net> (raw)
In-Reply-To: <20210910130755.2027995-1-osk@google.com>

On 9/10/21 6:07 AM, Oskar Senft wrote:
> This change allows the RTD modes to be configurable via device tree
> bindings. When the setting is not present via device tree, the driver
> still defaults to the previous behavior where the RTD modes are left
> alone.
> 
> Signed-off-by: Oskar Senft <osk@google.com>
> ---
>   drivers/hwmon/nct7802.c | 94 ++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 92 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/nct7802.c b/drivers/hwmon/nct7802.c
> index 604af2f6103a..6a6ab529bdd3 100644
> --- a/drivers/hwmon/nct7802.c
> +++ b/drivers/hwmon/nct7802.c
> @@ -51,6 +51,24 @@ static const u8 REG_VOLTAGE_LIMIT_MSB_SHIFT[2][5] = {
>   #define REG_CHIP_ID		0xfe
>   #define REG_VERSION_ID		0xff
>   
> +/*
> + * Sensor modes according to 7.2.32 Mode Selection Register
> + */
> +#define RTD_MODE_CLOSED		0x0
> +#define RTD_MODE_CURRENT	0x1
> +#define RTD_MODE_THERMISTOR	0x2
> +#define RTD_MODE_VOLTAGE	0x3
> +#define RTD_MODE_UNDEFINED	0xf
> +
> +#define MODE_BITS_MASK		0x3
> +
> +/*
> + * Bit offsets for sensors modes in REG_MODE
> + */
> +#define MODE_OFFSET_RTD1	0
> +#define MODE_OFFSET_RTD2	2
> +#define MODE_OFFSET_RTD3	4
> +

I think the access can be defined in a macro, with the index as parameter.
to be able to use it in the temp_type_{read,store} functions.


>   /*
>    * Data structures and manipulation thereof
>    */
> @@ -1038,7 +1056,9 @@ static const struct regmap_config nct7802_regmap_config = {
>   	.volatile_reg = nct7802_regmap_is_volatile,
>   };
>   
> -static int nct7802_init_chip(struct nct7802_data *data)
> +static int nct7802_init_chip(struct nct7802_data *data,
> +	unsigned char rtd1_mode, unsigned char rtd2_mode,
> +	unsigned char rtd3_mode)
>   {
>   	int err;
>   
> @@ -1052,15 +1072,57 @@ static int nct7802_init_chip(struct nct7802_data *data)
>   	if (err)
>   		return err;
>   
> +	/* Configure sensor modes */
> +	if ((rtd1_mode & MODE_BITS_MASK) == rtd1_mode) {

This is an odd way of checking if the mode is set. Why not just
"if (rtd1_mode != RTD_MODE_UNDEFINED)" ?

> +		err = regmap_update_bits(data->regmap, REG_MODE,
> +			MODE_BITS_MASK << MODE_OFFSET_RTD1,
> +			rtd1_mode << MODE_OFFSET_RTD1);
> +		if (err)
> +			return err;
> +	}
> +	if ((rtd2_mode & MODE_BITS_MASK) == rtd2_mode) {
> +		err = regmap_update_bits(data->regmap, REG_MODE,
> +			MODE_BITS_MASK << MODE_OFFSET_RTD2,
> +			rtd2_mode << MODE_OFFSET_RTD2);
> +		if (err)
> +			return err;
> +	}
> +	if ((rtd3_mode & MODE_BITS_MASK) == rtd3_mode) {
> +		err = regmap_update_bits(data->regmap, REG_MODE,
> +			MODE_BITS_MASK << MODE_OFFSET_RTD3,
> +			rtd3_mode << MODE_OFFSET_RTD3);
> +		if (err)
> +			return err;
> +	}

This should all be handled in a single register update. Read the mode register,
do all the bit updates, then write it back as complete register if it changed.

> +
>   	/* Enable Vcore and VCC voltage monitoring */
>   	return regmap_update_bits(data->regmap, REG_VMON_ENABLE, 0x03, 0x03);
>   }
>   
> +static unsigned char rtd_mode_from_string(const char *value)
> +{
> +	if (!strcmp(value, "closed"))
> +		return RTD_MODE_CLOSED;
> +	if (!strcmp(value, "current"))
> +		return RTD_MODE_CURRENT;
> +	if (!strcmp(value, "thermistor"))
> +		return RTD_MODE_THERMISTOR;
> +	if (!strcmp(value, "voltage"))
> +		return RTD_MODE_VOLTAGE;
> +
> +	return RTD_MODE_UNDEFINED;
> +}
> +
>   static int nct7802_probe(struct i2c_client *client)
>   {
>   	struct device *dev = &client->dev;
>   	struct nct7802_data *data;
>   	struct device *hwmon_dev;
> +	int rtd_mode_count;
> +	unsigned char rtd1_mode = RTD_MODE_UNDEFINED;
> +	unsigned char rtd2_mode = RTD_MODE_UNDEFINED;
> +	unsigned char rtd3_mode = RTD_MODE_UNDEFINED;
> +	const char *prop_value;
>   	int ret;
>   
>   	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> @@ -1074,7 +1136,25 @@ static int nct7802_probe(struct i2c_client *client)
>   	mutex_init(&data->access_lock);
>   	mutex_init(&data->in_alarm_lock);
>   
> -	ret = nct7802_init_chip(data);
> +	if (dev->of_node) {
> +		rtd_mode_count = of_property_count_strings(dev->of_node,
> +			"nuvoton,rtd-modes");
> +
> +		if (rtd_mode_count > 0)
> +			if (!of_property_read_string_index(dev->of_node,
> +				"nuvoton,rtd-modes", 0, &prop_value))
> +				rtd1_mode = rtd_mode_from_string(prop_value);
> +		if (rtd_mode_count > 1)
> +			if (!of_property_read_string_index(dev->of_node,
> +				"nuvoton,rtd-modes", 1, &prop_value))
> +				rtd2_mode = rtd_mode_from_string(prop_value);
> +		if (rtd_mode_count > 2)
> +			if (!of_property_read_string_index(dev->of_node,
> +				"nuvoton,rtd-modes", 2, &prop_value))
> +				rtd3_mode = rtd_mode_from_string(prop_value);
> +	}
> +

Better do this in nct7802_init_chip(), and pass dev as parameter.

> +	ret = nct7802_init_chip(data, rtd1_mode, rtd2_mode, rtd3_mode); >   	if (ret < 0)
>   		return ret;
>   
> @@ -1094,10 +1174,20 @@ static const struct i2c_device_id nct7802_idtable[] = {
>   };
>   MODULE_DEVICE_TABLE(i2c, nct7802_idtable);
>   
> +static const struct of_device_id __maybe_unused nct7802_of_match[] = {
> +	{
> +		.compatible = "nuvoton,nct7802",
> +		.data = 0

Unnecessary.

> +	},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, nct7802_of_match);
> +
>   static struct i2c_driver nct7802_driver = {
>   	.class = I2C_CLASS_HWMON,
>   	.driver = {
>   		.name = DRVNAME,
> +		.of_match_table = of_match_ptr(nct7802_of_match),
>   	},
>   	.detect = nct7802_detect,
>   	.probe_new = nct7802_probe,
> 


  reply	other threads:[~2021-09-10 16:22 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-10 13:07 Oskar Senft
2021-09-10 16:21 ` Guenter Roeck [this message]
2021-09-10 21:42 ` kernel test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=99a84071-f737-9a8b-73fa-bcc3e0ff6a3f@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=jdelvare@suse.com \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=osk@google.com \
    --subject='Re: [PATCH] hwmon: (nct7802) Make RTD modes configurable.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).