linux-kernel - Re: [PATCH v3] hwmon: adm1272: Enable temperature sampling for adm1272 adm1278

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 15 Sep 2020 20:09:18 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Chu Lin <linchuyuan@...gle.com>
Cc:     jdelvare@...e.com, belgaied@...gle.com, jasonling@...gle.com,
        linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
        zhongqil@...gle.com
Subject: Re: [PATCH v3] hwmon: adm1272: Enable temperature sampling for
 adm1272 adm1278

On Tue, Jul 21, 2020 at 03:49:00AM +0000, Chu Lin wrote:
> Problem:
> 	adm1272 and adm1278 supports temperature sampling. The
> current way of enabling it requires the user manually unbind the device
> from the driver, flip the temperature sampling control bit and then bind
> the device back to the driver. It would be nice if we can control this in a
> better way by reading the dt.
> 
> Solution:
> 	Introducing device tree binding analog,temp1-enable. If the
> flag is set, flip the temp1_en control bit on probing.
> 
> Testing:
> 1). iotools smbus_write16 35 0x10 0xd4 0x0037 // disable the temp1_en
> 2). recompile the dt to have  analog,temp1-enable set
> 3). Probe the driver and make sure tempX shows up in hwmon
> 
> Signed-off-by: Chu Lin <linchuyuan@...gle.com>

For my reference:

Reviewed-by: Guenter Roeck <linux@...ck-us.net>

We'll need to wait for DT review before I can apply the patch.

Thanks,
Guenter

> ---
> ChangeLog v1->v2:
>  - Rename adm1272-adm1278-temp1-en to analog-temperature1-enable
> 
> ChangeLog v2->v3:
>  - Rename analog-temperature1-enable analog,temp1-enable
> 
>  drivers/hwmon/pmbus/adm1275.c | 36 +++++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
> index 19317575d1c6..0482670281bd 100644
> --- a/drivers/hwmon/pmbus/adm1275.c
> +++ b/drivers/hwmon/pmbus/adm1275.c
> @@ -475,6 +475,7 @@ static int adm1275_probe(struct i2c_client *client,
>  	const struct coefficients *coefficients;
>  	int vindex = -1, voindex = -1, cindex = -1, pindex = -1;
>  	int tindex = -1;
> +	bool temp1_en;
>  	u32 shunt;
>  
>  	if (!i2c_check_functionality(client->adapter,
> @@ -536,6 +537,9 @@ static int adm1275_probe(struct i2c_client *client,
>  	if (shunt == 0)
>  		return -EINVAL;
>  
> +	temp1_en = of_property_read_bool(client->dev.of_node, "analog,temp1-enable") &&
> +		(mid->driver_data == adm1272 || mid->driver_data == adm1278);
> +
>  	data->id = mid->driver_data;
>  
>  	info = &data->info;
> @@ -614,16 +618,18 @@ static int adm1275_probe(struct i2c_client *client,
>  		info->func[0] |= PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT |
>  			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;
>  
> +		ret = config;
> +		/* Enable temp if it is instructed by dt (it is disabled by default) */
> +		if (temp1_en && !(config & ADM1278_TEMP1_EN))
> +			config |= ADM1278_TEMP1_EN;
>  		/* Enable VOUT if not enabled (it is disabled by default) */
> -		if (!(config & ADM1278_VOUT_EN)) {
> +		if (!(config & ADM1278_VOUT_EN))
>  			config |= ADM1278_VOUT_EN;
> -			ret = i2c_smbus_write_byte_data(client,
> -							ADM1275_PMON_CONFIG,
> -							config);
> +		if (ret != config) {
> +			ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG, config);
>  			if (ret < 0) {
> -				dev_err(&client->dev,
> -					"Failed to enable VOUT monitoring\n");
> -				return -ENODEV;
> +				dev_err(&client->dev, "Failed to enable config control bits");
> +				return ret;
>  			}
>  		}
>  
> @@ -685,16 +691,18 @@ static int adm1275_probe(struct i2c_client *client,
>  		info->func[0] |= PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT |
>  			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;
>  
> +		ret = config;
> +		/* Enable temp if it is instructed by dt (it is disabled by default) */
> +		if (temp1_en && !(config & ADM1278_TEMP1_EN))
> +			config |= ADM1278_TEMP1_EN;
>  		/* Enable VOUT if not enabled (it is disabled by default) */
> -		if (!(config & ADM1278_VOUT_EN)) {
> +		if (!(config & ADM1278_VOUT_EN))
>  			config |= ADM1278_VOUT_EN;
> -			ret = i2c_smbus_write_byte_data(client,
> -							ADM1275_PMON_CONFIG,
> -							config);
> +		if (ret != config) {
> +			ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG, config);
>  			if (ret < 0) {
> -				dev_err(&client->dev,
> -					"Failed to enable VOUT monitoring\n");
> -				return -ENODEV;
> +				dev_err(&client->dev, "Failed to enable config control bits");
> +				return ret;
>  			}
>  		}
>