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

From: Abdel Alkuor <alkuor@gmail.com>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Jean Delvare <jdelvare@suse.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Jonathan Corbet <corbet@lwn.net>,
	linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org
Subject: Re: [PATCH 2/2] hwmon: Add AMS AS6200 temperature sensor
Date: Sat, 16 Dec 2023 23:59:46 -0500	[thread overview]
Message-ID: <ZX6AQg1vz/Zz6JeG@abdel> (raw)
In-Reply-To: <c606c40b-8571-4618-827a-555ceab3ae74@roeck-us.net>

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,
  	...
  }

Thanks,
Abdel