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

[Guenter Roeck]

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