Messages in this thread |  | | Date | Sat, 16 Dec 2023 22:06:03 -0800 | | Subject | Re: [PATCH 2/2] hwmon: Add AMS AS6200 temperature sensor | | From | 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, ®val); > 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
|  |