| Message ID | 20220222223610.23098-1-linux@roeck-us.net |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v7,1/2] dt-bindings: hwmon: add tmp464.yaml | expand |
| Context | Check | Description |
|---|---|---|
| robh/patch-applied | success | |
| robh/checkpatch | success | |
| robh/dtbs-check | success | |
| robh/dt-meta-schema | success |
On Tue, 22 Feb 2022 14:36:09 -0800, Guenter Roeck wrote: > From: Agathe Porte <agathe.porte@nokia.com> > > Add basic description of the tmp464 driver DT bindings. > > Signed-off-by: Agathe Porte <agathe.porte@nokia.com> > Cc: Krzysztof Adamski <krzysztof.adamski@nokia.com> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > v7: > - No change > > v6: > - Model ti,n-factor as int32 instead of int8. > > v5: > - Dropped ti,n-factor from channel@0 example. Added additional > channel to examples to show positive ti,n-factor property. > > v4: > - No changes > > v3: > - Addedd support for TMP468. > - Changed number of channels from 0..3 (which was wrong anyway) to 0..8. > - Changed value range for ti,n-factor to int8, with an example for > a negative value. > - Added myself as driver maintainer. > > .../devicetree/bindings/hwmon/ti,tmp464.yaml | 114 ++++++++++++++++++ > MAINTAINERS | 7 ++ > 2 files changed, 121 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml > Reviewed-by: Rob Herring <robh@kernel.org>
On Tue, Feb 22, 2022 at 02:36:10PM -0800, Guenter Roeck wrote: > Add support for Texas Instruments TMP464 and TMP468 temperature sensor > ICs. > > TI's TMP464 is an I2C temperature sensor chip. This chip is similar > to TI's TMP421 chip, but with 16bit-wide registers (instead of > 8bit-wide registers). The chip has one local sensor and four remote > sensors. TMP468 is similar to TMP464 but has one local and eight > remote sensors. > > Originally-from: Agathe Porte <agathe.porte@nokia.com> > Cc: Agathe Porte <agathe.porte@nokia.com> > Cc: Krzysztof Adamski <krzysztof.adamski@nokia.com> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> Any review / test feedback on this patch ? I would like to apply it before the commit window opens, but the time is getting short. Thanks, Guenter
Hi Guenter, Le 02/03/2022 à 18:59, Guenter Roeck a écrit : > Any review / test feedback on this patch ? I would like to apply it > before the commit window opens, but the time is getting short. I thought that you did receive the TMP464 samples and had the opportunity to test on it. I will test v7 on our hardware equipped with TMP464, verify that DT support works fine, and will reply to this email with my findings. Bests. Agathe.
On 3/3/22 00:57, Agathe Porte wrote: > Hi Guenter, > > Le 02/03/2022 à 18:59, Guenter Roeck a écrit : >> Any review / test feedback on this patch ? I would like to apply it >> before the commit window opens, but the time is getting short. > > I thought that you did receive the TMP464 samples and had the opportunity to test on it. I will test v7 on our hardware equipped with TMP464, verify that DT support works fine, and will reply to this email with my findings. > Yes, I did, and thanks a lot for it! I even wrote a qemu emulation for the chip to be able to test the devicetree code. Still, I need to have someone else confirm that I didn't mess up. Thanks, Guenter
Le 3/3/2022 à 4:00 PM, Guenter Roeck a écrit : > On 3/3/22 00:57, Agathe Porte wrote: >> Hi Guenter, >> >> Le 02/03/2022 à 18:59, Guenter Roeck a écrit : >>> Any review / test feedback on this patch ? I would like to apply it >>> before the commit window opens, but the time is getting short. >> >> I thought that you did receive the TMP464 samples and had the >> opportunity to test on it. I will test v7 on our hardware equipped >> with TMP464, verify that DT support works fine, and will reply to >> this email with my findings. >> > > Yes, I did, and thanks a lot for it! I even wrote a qemu emulation > for the chip to be able to test the devicetree code. Great! > Still, I need to have someone else confirm that I didn't mess up. I tested v7 on our hardware and the behavior seems to be the same as our previous, in-house driver, if that gives you a point of comparison. We only use temp*_input sysfs though. No compilation warnings. I can disable and enable sensors fine at runtime: > # cat temp*_input > 43063 > 35813 > 34938 > 39313 > 29125 > # echo 0 | tee temp*_enable > 0 > # cat temp*_input > cat: temp1_input: No data available > cat: temp2_input: No data available > cat: temp3_input: No data available > cat: temp4_input: No data available > cat: temp5_input: No data available > # echo 1 | tee temp*_enable > 1 > # cat temp*_input > 43063 > 35750 > 34875 > 39313 > 29188 For what it's worth: Tested-by: Agathe Porte <agathe.porte@nokia.com> Bests.
On Thu, Mar 03, 2022 at 04:31:39PM +0100, Agathe Porte wrote: > Le 3/3/2022 à 4:00 PM, Guenter Roeck a écrit : > > On 3/3/22 00:57, Agathe Porte wrote: > > > Hi Guenter, > > > > > > Le 02/03/2022 à 18:59, Guenter Roeck a écrit : > > > > Any review / test feedback on this patch ? I would like to apply it > > > > before the commit window opens, but the time is getting short. > > > > > > I thought that you did receive the TMP464 samples and had the > > > opportunity to test on it. I will test v7 on our hardware equipped > > > with TMP464, verify that DT support works fine, and will reply to > > > this email with my findings. > > > > > > > Yes, I did, and thanks a lot for it! I even wrote a qemu emulation > > for the chip to be able to test the devicetree code. > > Great! > > > Still, I need to have someone else confirm that I didn't mess up. > > I tested v7 on our hardware and the behavior seems to be the same as our > previous, in-house driver, if that gives you a point of comparison. We only > use temp*_input sysfs though. > > No compilation warnings. > > I can disable and enable sensors fine at runtime: > > > # cat temp*_input > > 43063 > > 35813 > > 34938 > > 39313 > > 29125 > > # echo 0 | tee temp*_enable > > 0 > > # cat temp*_input > > cat: temp1_input: No data available > > cat: temp2_input: No data available > > cat: temp3_input: No data available > > cat: temp4_input: No data available > > cat: temp5_input: No data available > > # echo 1 | tee temp*_enable > > 1 > > # cat temp*_input > > 43063 > > 35750 > > 34875 > > 39313 > > 29188 > > For what it's worth: > > Tested-by: Agathe Porte <agathe.porte@nokia.com> > Thanks a lot! I applied the series to hwmon-next. Guenter
Hi Agathe, On 3/14/22 08:46, Agathe Porte wrote: > Hi, > > Le 2/22/2022 à 11:36 PM, Guenter Roeck a écrit : >> of_property_read_string(child,"label", &data->channel[channel].label); > > Upon trying to merge v7 in our codebase, our static analyzer tool detected that the return code of this function was not checked. > > As I guess putting a label is optional, maybe we should add a `(void)` on the same line just before the function call to clearly indicate that not checking the return value is intentional and that it is not a coding mistake? > > EDIT: As I was reading the implementation of of_property_read_string [1], it will not touch the destination string in case of error. Which means that labels may sit uninitialized and contain garbage data? > Thanks for the feedback. If of_property_read_string() returns an error, it will not set the pointer to &data->channel[channel].label, which by default is NULL because the data structure was allocated with devm_kzalloc(). That means tmp464_is_visible() will disable the label attribute. I don't see a problem with the current code. There are lots of examples in the kernel where the return value from of_property_read_string() is silently ignored. Not a single one of those uses a (void) typecast. I don't really want to start making such changes just to make static analyzers happy. Thanks, Guenter
Hi Guenter, Le 3/15/2022 à 2:22 AM, Guenter Roeck a écrit : > If of_property_read_string() returns an error, it will not set the > pointer > to &data->channel[channel].label, which by default is NULL because the > data structure was allocated with devm_kzalloc(). That means > tmp464_is_visible() > will disable the label attribute. I don't see a problem with the current > code. Thanks for the explanation. I agree that there is no problem on this point. > There are lots of examples in the kernel where the return value from > of_property_read_string() is silently ignored. Not a single one of > those uses a (void) typecast. I don't really want to start making > such changes just to make static analyzers happy. I have to disagree here. Because something has always (not) be done in the past should not be a reason to (not) do it in the future out of pure habit. I did not suggest to add the (void) casts in existing code: I agree it would be a burden with no real added value. But making static analyzers happy seems justified *for new code*. It also makes *other developers* more confident, because with the cast we are sure that not checking the return value is very intentional. Please enlighten me if there are any downsides that I did not think of and that would block this one-line change. Best regards, Agathe.
Hi Agathe, On 3/15/22 06:03, Agathe Porte wrote: > Hi Guenter, > > Le 3/15/2022 à 2:22 AM, Guenter Roeck a écrit : >> If of_property_read_string() returns an error, it will not set the pointer >> to &data->channel[channel].label, which by default is NULL because the >> data structure was allocated with devm_kzalloc(). That means tmp464_is_visible() >> will disable the label attribute. I don't see a problem with the current >> code. > > Thanks for the explanation. I agree that there is no problem on this point. > >> There are lots of examples in the kernel where the return value from >> of_property_read_string() is silently ignored. Not a single one of >> those uses a (void) typecast. I don't really want to start making >> such changes just to make static analyzers happy. > > I have to disagree here. Because something has always (not) be done in the past should not be a reason to (not) do it in the future out of pure habit. I did not suggest to add the (void) casts in existing code: I agree it would be a burden with no real added value. > > But making static analyzers happy seems justified *for new code*. It also makes *other developers* more confident, because with the cast we are sure that not checking the return value is very intentional. > > Please enlighten me if there are any downsides that I did not think of and that would block this one-line change. > Changing the code now would require either a separate patch or a rebase of the hwmon-next tree. Rebasing the hwmon-next tree at this point of the release cycle (a few days before the commit window opens) is something I really don't want to do, leaving the option to add a separate patch for the change. That makes it identical to changing existing code to add the (void). In addition to that, I do not agree that adding (void) really adds value here; it just says "this is done on purpose" because the static analyzer doesn't know better. 0-day stopped reporting this kind of perceived problem, presumably for good reason. Since the result of the function call is implied in setting or not setting the passed pointer, a return value check or adding (void) is not warranted. This would be different if the property was mandatory, but that is not the case here. There are lots of other functions in the kernel where return values are not checked, for a variety of reasons. Functions where checking the return value is necessary/mandatory are tagged with __must_check. For others it is left to the caller to decide if a return value should be checked, and if it makes sense / adds value to add (void). I'll give you another example: cancel_work_sync() and related functions. I am sure your static analyzer will complain about the failure to check its return value in almost all cases. A counter-example is, say, platform_driver_register(), where the return value should really be checked and a (void) typecast should be used if it is not checked on purpose. The problem is that static analyzers can not determine if the return value check is necessary, and should either leave it alone or make reports conditional on some command line option. Overall we'll have to agree to disagree. Thanks, Guenter
diff --git a/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml b/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml new file mode 100644 index 000000000000..801ca9ba7d34 --- /dev/null +++ b/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml @@ -0,0 +1,114 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/hwmon/ti,tmp464.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: TMP464 and TMP468 temperature sensors + +maintainers: + - Agathe Porte <agathe.porte@nokia.com> + +description: | + ±0.0625°C Remote and Local temperature sensor + https://www.ti.com/lit/ds/symlink/tmp464.pdf + https://www.ti.com/lit/ds/symlink/tmp468.pdf + +properties: + compatible: + enum: + - ti,tmp464 + - ti,tmp468 + + reg: + maxItems: 1 + + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + +required: + - compatible + - reg + +additionalProperties: false + +patternProperties: + "^channel@([0-8])$": + type: object + description: | + Represents channels of the device and their specific configuration. + + properties: + reg: + description: | + The channel number. 0 is local channel, 1-8 are remote channels. + items: + minimum: 0 + maximum: 8 + + label: + description: | + A descriptive name for this channel, like "ambient" or "psu". + + ti,n-factor: + description: | + The value (two's complement) to be programmed in the channel specific N correction register. + For remote channels only. + $ref: /schemas/types.yaml#/definitions/int32 + items: + minimum: -128 + maximum: 127 + + required: + - reg + + additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + sensor@4b { + compatible = "ti,tmp464"; + reg = <0x4b>; + }; + }; + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + sensor@4b { + compatible = "ti,tmp464"; + reg = <0x4b>; + #address-cells = <1>; + #size-cells = <0>; + + channel@0 { + reg = <0x0>; + label = "local"; + }; + + channel@1 { + reg = <0x1>; + ti,n-factor = <(-10)>; + label = "external"; + }; + + channel@2 { + reg = <0x2>; + ti,n-factor = <0x10>; + label = "somelabel"; + }; + + channel@3 { + reg = <0x3>; + status = "disabled"; + }; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index fca970a46e77..f51bc7c7e439 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -19489,6 +19489,13 @@ S: Maintained F: Documentation/hwmon/tmp401.rst F: drivers/hwmon/tmp401.c +TMP464 HARDWARE MONITOR DRIVER +M: Agathe Porte <agathe.porte@nokia.com> +M: Guenter Roeck <linux@roeck-us.net> +L: linux-hwmon@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml + TMP513 HARDWARE MONITOR DRIVER M: Eric Tremblay <etremblay@distech-controls.com> L: linux-hwmon@vger.kernel.org