linux-kernel - Re: [PATCH 2/2] hwmon: Add AMS AS6200 temperature sensor

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] [thread-next>] [day] [month] [year] [list]

Date: Sat, 16 Dec 2023 22:06:03 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Abdel Alkuor <alkuor@...il.com>
Cc: Jean Delvare <jdelvare@...e.com>, Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>,
 linux-hwmon@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH 2/2] hwmon: Add AMS AS6200 temperature sensor

On 12/16/23 20:59, Abdel Alkuor wrote:
> On Sat, Dec 16, 2023 at 05:40:35PM -0800, Guenter Roeck wrote:
>> On 12/16/23 14:07, Abdel Alkuor wrote:
>>> On Sat, Dec 16, 2023 at 10:46:53AM -0800, Guenter Roeck wrote:
>>>> On 12/16/23 08:39, Abdel Alkuor wrote:
>>> Should I use tmp112 params for as6200?
>>>
>>
>> Sure, or just add a separate entry for as6200.
>>
> I think some modifications need to be done regarding setting the default
> configuration for chips with config reg of 16 bits.
> 
> Currently, tmp112 set_mask and clr_mask look like this
> 
>     [tmp112] = {
>     	 .set_mask = 3 << 5,	/* 8 samples / second */
>     	 .clr_mask = 1 << 7,	/* no one-shot mode*/
>     	 ...
>     }
> 
> and in probe function, we are using i2c_smbus_read_byte_data which
> basically reads byte 1 of tmp112 config reg and in lm75_write_config
> it writes byte 1 of tmp112 config reg. Now based on tmp112 set_mask,
> we want to set the sample rate but we actually setting R0 and R1 instead.
> According to tmp112 datasheet on pg. 16, byte 1 is written first then
> byte 2, where byte 2 has the conversion rate at bit 6 and 7 (CR0/CR1).
> 
> tmp112 datasheet: https://www.ti.com/lit/ds/symlink/tmp112.pdf?ts=1702713491401&ref_url=https%253A%252F%252Fwww.google.com%252F
> 
> Now, to accommodate 16 bit config register read/write, something along these lines can
> be done:
> - In struct lm75_params,
>    - change set_mask and clr_mask from u8 to u16
>    - Add config reg two bytes size flag
> - Use the proper function to read the config reg based on config reg size i.e
>    For one byte config reg, use i2c_smbus_read_byte_data, and for 2 bytes
>    config reg, use regmap_read.
> 
>    static int lm75_probe(struct i2c_client *client)
>    {
>     	...
> 	if (data->params->config_reg_16bits)
>    		status = regmap_read(client, LM75_REG_CONF, &regval);
> 		if (status < 0) {
> 			dev_dbg(dev, "Can't read config? %d\n", status);
> 			return status;
> 		}
> 		data->orig_conf = regval;
> 		data->current_conf = regval;
> 	} else {
>    		status = i2c_smbus_read_byte_data(client, LM75_REG_CONF);
> 		if (status < 0) {
> 			dev_dbg(dev, "Can't read config? %d\n", status);
> 			return status;
> 		}
> 		data->orig_conf = status;
> 		data->current_conf = status;
> 	}
> 	...
>     }
> 
>     static int lm75_write_config(struct lm75_data *data, u16 set_mask,
>     			     u16 clr_mask)
>     {
>       
>       if (data->params->config_reg_16bits)
>       	clr_mask |= LM75_SHUTDOWN << 8;
>       else
>       	clr_mask |= LM75_SHUTDOWN;
>       ...
>       	if (data->params->config_reg_16bits)
>       		err = regmap_write(data->regmap, LM75_REG_CONF, value);
>       	else
>       		err = i2c_smbus_write_byte_data(data->client,
>       			       			LM75_REG_CONF,
>       						value);
>       ...
>     }
> 
> Based on that, the new tmp112 set_mask and clr_mask would look like this instead,
>    [tmp112] = {
>    	.set_mask = 3 << 6,	/* 8 samples / second */
>    	.clr_mask = 1 << 15,	/* no one-shot mode*/
> 	.config_reg_16bits = 1,
>    	...
>    }
> 

Yes, you are correct, we'll need something like that. lm75_update_interval()
tries to solve the problem for tmp112, but that doesn't work with
set_mask/clear_mask. We should have a separate function lm75_read_config(),
though, to hide the complexity.

Thanks,
Guenter