| Message ID | 149032e99136a9fe47c3533b57a71092646e497d.1702744180.git.alkuor@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] dt-bindings: hwmon: Add AMS AS6200 temperature sensor | expand |
On 12/16/23 08:39, Abdel Alkuor wrote: > as6200 is a temperature sensor with 0.0625°C resolution and a range between > -40°C to 125°C. > > By default, the driver configures as6200 as following: > - Converstion rate: 8 Hz > - Conversion mode: continuous > - Consecutive fault counts: 6 samples > - Alert state: high polarity > - Alert mode: comparator mode > > Interrupt is supported for the alert pin. > > Datasheet: https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf > Signed-off-by: Abdel Alkuor <alkuor@gmail.com> Please explain why the lm75 driver would not work for this chip. I don't immediately see the problem, especially with TMP112 using almost the same configuration register layout. Thanks, Guenter
On Sat, Dec 16, 2023 at 10:46:53AM -0800, Guenter Roeck wrote: > On 12/16/23 08:39, Abdel Alkuor wrote: > Please explain why the lm75 driver would not work for this chip. > I don't immediately see the problem, especially with TMP112 using almost > the same configuration register layout. > Hi Guenter, That's a good point, tmp112 is very similar to as6200 except R0/R1 and EM bits don't exist in as6200. That being said, the current config for tmp112 in lm75 driver can be used for as6200 as the default R0/R1 is set to 12bits which is the only resolution supported in as6200. Should I use tmp112 params for as6200? Also, can we add support for hwmon_temp_alarm and alert interrupt? Thanks, Abdel
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: >> Please explain why the lm75 driver would not work for this chip. >> I don't immediately see the problem, especially with TMP112 using almost >> the same configuration register layout. >> > Hi Guenter, > > That's a good point, tmp112 is very similar to as6200 except R0/R1 and > EM bits don't exist in as6200. That being said, the current config for > tmp112 in lm75 driver can be used for as6200 as the default R0/R1 is > set to 12bits which is the only resolution supported in as6200. > > Should I use tmp112 params for as6200? > Sure, or just add a separate entry for as6200. > Also, can we add support for hwmon_temp_alarm and alert interrupt? > Yes, of course. Just make it conditional on chip types supporting it (not necessarily all of them, just the ones you know about). Thanks, Guenter
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, ... } Thanks, Abdel
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
On Sat, Dec 16, 2023 at 11:39:29AM -0500, Abdel Alkuor wrote: > as6200 is a temperature sensor with a range between -40°C to > 125°C degrees and an accuracy of ±0.4°C degree between 0 > and 65°C and ±1°C for the other ranges. > > Signed-off-by: Abdel Alkuor <alkuor@gmail.com> Is this not v3? Either way, Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Cheers, Conor. > --- > .../devicetree/bindings/hwmon/ams,as6200.yaml | 52 +++++++++++++++++++ > 1 file changed, 52 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwmon/ams,as6200.yaml > > diff --git a/Documentation/devicetree/bindings/hwmon/ams,as6200.yaml b/Documentation/devicetree/bindings/hwmon/ams,as6200.yaml > new file mode 100644 > index 000000000000..01476c1a4c98 > --- /dev/null > +++ b/Documentation/devicetree/bindings/hwmon/ams,as6200.yaml > @@ -0,0 +1,52 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/hwmon/ams,as6200.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: AMS AS6200 Temperature Sensor > + > +maintainers: > + - Abdel Alkuor <alkuor@gmail.com> > + > +description: | > + as6200 is a temperature sensor with a range between -40°C to > + 125°C degrees and an accuracy of ±0.4°C degree between 0 > + and 65°C and ±1°C for the other ranges. > + https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf > + > +properties: > + compatible: > + const: ams,as6200 > + > + reg: > + maxItems: 1 > + > + vdd-supply: true > + > + interrupts: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - vdd-supply > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + temperature-sensor@48 { > + compatible = "ams,as6200"; > + reg = <0x48>; > + vdd-supply = <&vdd>; > + interrupt-parent = <&gpio1>; > + interrupts = <17 IRQ_TYPE_EDGE_BOTH>; > + }; > + }; > +... > -- > 2.34.1 >
On 12/17/23 12:58, Conor Dooley wrote: > On Sat, Dec 16, 2023 at 11:39:29AM -0500, Abdel Alkuor wrote: >> as6200 is a temperature sensor with a range between -40°C to >> 125°C degrees and an accuracy of ±0.4°C degree between 0 >> and 65°C and ±1°C for the other ranges. >> >> Signed-off-by: Abdel Alkuor <alkuor@gmail.com> > > Is this not v3? FWIW, I don't recall seeing it. > Either way, > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > As I pointed out in my reply to the other patch, this chip is mostly compatible to lm75 and almost register-compatible to tmp112, the only difference being some configuration register bits. Bindings for this chip should be added to Documentation/devicetree/bindings/hwmon/lm75.yaml. Thanks, Guenter
On Sun, Dec 17, 2023 at 01:39:37PM -0800, Guenter Roeck wrote: > On 12/17/23 12:58, Conor Dooley wrote: > > On Sat, Dec 16, 2023 at 11:39:29AM -0500, Abdel Alkuor wrote: > > > as6200 is a temperature sensor with a range between -40°C to > > > 125°C degrees and an accuracy of ±0.4°C degree between 0 > > > and 65°C and ±1°C for the other ranges. > > > > > > Signed-off-by: Abdel Alkuor <alkuor@gmail.com> > > > > Is this not v3? > > FWIW, I don't recall seeing it. Ah, I think this was originally submitted to iio, went through 2 iterations and then ended up here on Jonathan's advice. > > > Either way, > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > > > > As I pointed out in my reply to the other patch, this chip is mostly > compatible to lm75 and almost register-compatible to tmp112, the only > difference being some configuration register bits. > > Bindings for this chip should be added to > Documentation/devicetree/bindings/hwmon/lm75.yaml. Ah. In that case, I would expect my tag not to be picked up by Abdel.
On 12/17/23 13:44, Conor Dooley wrote: > On Sun, Dec 17, 2023 at 01:39:37PM -0800, Guenter Roeck wrote: >> On 12/17/23 12:58, Conor Dooley wrote: >>> On Sat, Dec 16, 2023 at 11:39:29AM -0500, Abdel Alkuor wrote: >>>> as6200 is a temperature sensor with a range between -40°C to >>>> 125°C degrees and an accuracy of ±0.4°C degree between 0 >>>> and 65°C and ±1°C for the other ranges. >>>> >>>> Signed-off-by: Abdel Alkuor <alkuor@gmail.com> >>> >>> Is this not v3? >> >> FWIW, I don't recall seeing it. > > Ah, I think this was originally submitted to iio, went through 2 > iterations and then ended up here on Jonathan's advice. > Hmm, yes, that would have been a duplicate. If iio support for lm75 compatible sensors is needed for some reason, a hwmon -> iio bridge would make more sense. Thanks, Guenter >> >>> Either way, >>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com> >>> >> >> As I pointed out in my reply to the other patch, this chip is mostly >> compatible to lm75 and almost register-compatible to tmp112, the only >> difference being some configuration register bits. >> >> Bindings for this chip should be added to >> Documentation/devicetree/bindings/hwmon/lm75.yaml. > > Ah. In that case, I would expect my tag not to be picked up by Abdel.
On Sat, Dec 16, 2023 at 10:06:03PM -0800, Guenter Roeck wrote: > 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: > > ... > > } > > > > 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. > I'll fix tmp112 parameters in another patch as as6200 patch 2 in v2 implements 2bytes read/write for the configure reg. https://marc.info/?l=linux-hwmon&m=170287522119545&w=2 On another note, I checked all the supported chips in lm75 to see which ones support an alert bit, only 3 chips support it; tmp112(bit 5 in the second byte), tmp100, and tmp101 (bit 7 in the first byte). Thanks, Abdel
diff --git a/Documentation/devicetree/bindings/hwmon/ams,as6200.yaml b/Documentation/devicetree/bindings/hwmon/ams,as6200.yaml new file mode 100644 index 000000000000..01476c1a4c98 --- /dev/null +++ b/Documentation/devicetree/bindings/hwmon/ams,as6200.yaml @@ -0,0 +1,52 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/hwmon/ams,as6200.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: AMS AS6200 Temperature Sensor + +maintainers: + - Abdel Alkuor <alkuor@gmail.com> + +description: | + as6200 is a temperature sensor with a range between -40°C to + 125°C degrees and an accuracy of ±0.4°C degree between 0 + and 65°C and ±1°C for the other ranges. + https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf + +properties: + compatible: + const: ams,as6200 + + reg: + maxItems: 1 + + vdd-supply: true + + interrupts: + maxItems: 1 + +required: + - compatible + - reg + - vdd-supply + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + i2c { + #address-cells = <1>; + #size-cells = <0>; + + temperature-sensor@48 { + compatible = "ams,as6200"; + reg = <0x48>; + vdd-supply = <&vdd>; + interrupt-parent = <&gpio1>; + interrupts = <17 IRQ_TYPE_EDGE_BOTH>; + }; + }; +...
as6200 is a temperature sensor with a range between -40°C to 125°C degrees and an accuracy of ±0.4°C degree between 0 and 65°C and ±1°C for the other ranges. Signed-off-by: Abdel Alkuor <alkuor@gmail.com> --- .../devicetree/bindings/hwmon/ams,as6200.yaml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/ams,as6200.yaml