* [PATCH v1 0/2] add support for MCP998X
@ 2025-04-15 13:26 victor.duicu
2025-04-15 13:26 ` [PATCH v1 1/2] dt-bindings: iio: temperature: " victor.duicu
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: victor.duicu @ 2025-04-15 13:26 UTC (permalink / raw)
To: jic23
Cc: andy, dlechner, nuno.sa, victor.duicu, marius.cristea, linux-iio,
linux-kernel
From: Victor Duicu <victor.duicu@microchip.com>
Add support for Microchip MCP998X/33 and MCP998XD/33D
Multichannel Automotive Temperature Monitor Family.
The chip is capable of monitoring temperatures on four
external channels and one internal.
Victor Duicu (2):
dt-bindings: iio: temperature: add support for MCP998X
iio: temperature: add support for MCP998X
.../testing/sysfs-bus-iio-temperature-mcp9982 | 17 +
.../iio/temperature/microchip,mcp9982.yaml | 182 ++++
MAINTAINERS | 7 +
drivers/iio/temperature/Kconfig | 10 +
drivers/iio/temperature/Makefile | 1 +
drivers/iio/temperature/mcp9982.c | 794 ++++++++++++++++++
6 files changed, 1011 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982
create mode 100644 Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
create mode 100644 drivers/iio/temperature/mcp9982.c
base-commit: 31c52fe3b2efeebfc72cc5336653baaa9889b41e
--
2.45.2
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v1 1/2] dt-bindings: iio: temperature: add support for MCP998X 2025-04-15 13:26 [PATCH v1 0/2] add support for MCP998X victor.duicu @ 2025-04-15 13:26 ` victor.duicu 2025-04-15 17:52 ` Jonathan Cameron 2025-04-16 6:45 ` Krzysztof Kozlowski 2025-04-15 13:26 ` [PATCH v1 2/2] " victor.duicu 2025-04-15 17:43 ` [PATCH v1 0/2] " Jonathan Cameron 2 siblings, 2 replies; 15+ messages in thread From: victor.duicu @ 2025-04-15 13:26 UTC (permalink / raw) To: jic23 Cc: andy, dlechner, nuno.sa, victor.duicu, marius.cristea, linux-iio, linux-kernel From: Victor Duicu <victor.duicu@microchip.com> This is the devicetree schema for Microchip MCP998X/33 and MCP998XD/33D Multichannel Automotive Temperature Monitor Family. Signed-off-by: Victor Duicu <victor.duicu@microchip.com> --- .../iio/temperature/microchip,mcp9982.yaml | 182 ++++++++++++++++++ 1 file changed, 182 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml diff --git a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml new file mode 100644 index 000000000000..8cbf897d1278 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml @@ -0,0 +1,182 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/temperature/microchip,mcp9982.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Microchip MCP998X/33 and MCP998XD/33D Multichannel Automotive Temperature Monitor Family + +maintainers: + - Victor Duicu <victor.duicu@microchip.com> + +description: | + The MCP998X/33 and MCP998XD/33D family is a high-accuracy 2-wire multichannel + automotive temperature monitor. + The datasheet can be found here: + https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf + +properties: + compatible: + enum: + - microchip,mcp9933 + - microchip,mcp9933D + - microchip,mcp9982 + - microchip,mcp9982D + - microchip,mcp9983 + - microchip,mcp9983D + - microchip,mcp9984 + - microchip,mcp9984D + - microchip,mcp9985 + - microchip,mcp9985D + + reg: + maxItems: 1 + + interrupts: + maxItems: 2 + + interrupt-names: + description: | + ALERT1 indicates a HIGH or LOW limit was exceeded. + ALERT2 indicates a THERM limit was exceeded. + items: + - const: ALERT1 + - const: ALERT2 + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + + microchip,temp-hysteresis: + description: | + Value of temperature limit hysteresis. + Omit this tag to set the default value. + $ref: /schemas/types.yaml#/definitions/uint32 + + microchip,extended-temp-range: + description: | + Set the chip to work in the extended temperature range -64 degrees C to 191.875 degrees C. + Omit this tag to set the default range 0 degrees C to 127.875 degrees C + type: boolean + + microchip,beta-channel1: + description: | + The beta compensation factor for external channel 1 can be set + by the user, or can be set automatically by the chip. + If one wants to enable beta autodetection, omit this tag. + Please consult the documentation if one wants to set a specific beta. + If anti-parallel diode operation is enabled, the default value is set + and can't be changed. + type: boolean + + microchip,beta-channel2: + description: | + The beta compensation factor for external channel 2 can be set + by the user, or can be set automatically by the chip. + If one wants to enable beta autodetection, omit this tag. + Please consult the documentation if one wants to set a specific beta. + If anti-parallel diode operation is enabled, the default value is set + and can't be changed. + type: boolean + + microchip,apdd-state: + description: | + Enable anti-parallel diode mode operation. + Omit this tag to disable anti-parallel diode mode by default. + type: boolean + + microchip,recd12: + description: | + Enable resistance error correction for external channels 1 and 2. + Not all chips support resistance error correction on external + channels 1 and 2, please consult the documentation. + Omit this tag to disable REC for channels 1 and 2 by default. + type: boolean + + microchip,recd34: + description: | + Enable resistance error correction for external channels 3 and 4. + Not all chips support resistance error correction on external + channels 3 and 4, please consult the documentation. + Omit this tag to disable REC for channels 3 and 4 by default. + type: boolean + + label: + description: Unique name to identify which device this is. + + vdd-supply: true + +patternProperties: + "^channel@[1-4]+$": + description: | + Represents the external temperature channels to which a remote diode is + connected. + type: object + + properties: + reg: + items: + minimum: 1 + maximum: 4 + + microchip,ideality-factor: + description: | + Each channel has an ideality factor. + Beta compensation and resistance error correction automatically correct + for most ideality error. So ideality factor does not need to be adjusted in general. + Omit this tag in order to set the default value. + Please consult the documentation if one wants to set a specific ideality value. + $ref: /schemas/types.yaml#/definitions/uint32 + + label: + description: Unique name to identify which channel this is. + + required: + - reg + + unevaluatedProperties: false + +required: + - compatible + - reg + - vdd-supply + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + temperature-sensor@4c { + compatible = "microchip,mcp9985"; + reg = <0x4c>; + + #address-cells = <1>; + #size-cells = <0>; + + label = "temperature-sensor"; + + microchip,temp-hysteresis = <10>; + microchip,extended-temp-range; + microchip,apdd-state; + microchip,recd12; + microchip,recd34; + vdd-supply = <&vdd>; + + channel@1{ + reg = <0x1>; + label = "CPU Temperature"; + }; + + channel@2{ + reg = <0x2>; + label = "GPU Temperature"; + }; + }; + }; + +... -- 2.45.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: iio: temperature: add support for MCP998X 2025-04-15 13:26 ` [PATCH v1 1/2] dt-bindings: iio: temperature: " victor.duicu @ 2025-04-15 17:52 ` Jonathan Cameron 2025-04-17 11:40 ` Victor.Duicu 2025-04-16 6:45 ` Krzysztof Kozlowski 1 sibling, 1 reply; 15+ messages in thread From: Jonathan Cameron @ 2025-04-15 17:52 UTC (permalink / raw) To: victor.duicu Cc: andy, dlechner, nuno.sa, marius.cristea, linux-iio, linux-kernel On Tue, 15 Apr 2025 16:26:22 +0300 <victor.duicu@microchip.com> wrote: > From: Victor Duicu <victor.duicu@microchip.com> > > This is the devicetree schema for Microchip MCP998X/33 and > MCP998XD/33D Multichannel Automotive Temperature Monitor Family. Hi Victor, Please state briefly here in what way the parts are incompatible as a justification for no fallback compatibles. Quite a bit of that will become apparent when you enforce validity of parameters as suggested below. Various comments inline. > > Signed-off-by: Victor Duicu <victor.duicu@microchip.com> > --- > .../iio/temperature/microchip,mcp9982.yaml | 182 ++++++++++++++++++ > 1 file changed, 182 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml > > diff --git a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml > new file mode 100644 > index 000000000000..8cbf897d1278 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml > @@ -0,0 +1,182 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/temperature/microchip,mcp9982.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Microchip MCP998X/33 and MCP998XD/33D Multichannel Automotive Temperature Monitor Family > + > +maintainers: > + - Victor Duicu <victor.duicu@microchip.com> > + > +description: | > + The MCP998X/33 and MCP998XD/33D family is a high-accuracy 2-wire multichannel > + automotive temperature monitor. > + The datasheet can be found here: > + https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf > + > +properties: > + compatible: > + enum: > + - microchip,mcp9933 > + - microchip,mcp9933D > + - microchip,mcp9982 > + - microchip,mcp9982D > + - microchip,mcp9983 > + - microchip,mcp9983D > + - microchip,mcp9984 > + - microchip,mcp9984D > + - microchip,mcp9985 > + - microchip,mcp9985D > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 2 > + > + interrupt-names: > + description: | > + ALERT1 indicates a HIGH or LOW limit was exceeded. > + ALERT2 indicates a THERM limit was exceeded. > + items: > + - const: ALERT1 > + - const: ALERT2 > + > + "#address-cells": > + const: 1 > + > + "#size-cells": > + const: 0 > + > + microchip,temp-hysteresis: > + description: | > + Value of temperature limit hysteresis. > + Omit this tag to set the default value. > + $ref: /schemas/types.yaml#/definitions/uint32 Can we just make this a userspace thing using appropriate _hysteresis ABI element? > + > + microchip,extended-temp-range: > + description: | > + Set the chip to work in the extended temperature range -64 degrees C to 191.875 degrees C. > + Omit this tag to set the default range 0 degrees C to 127.875 degrees C > + type: boolean I'm curious. Why does this belong in the DT binding? > + > + microchip,beta-channel1: > + description: | > + The beta compensation factor for external channel 1 can be set > + by the user, or can be set automatically by the chip. > + If one wants to enable beta autodetection, omit this tag. > + Please consult the documentation if one wants to set a specific beta. > + If anti-parallel diode operation is enabled, the default value is set > + and can't be changed. > + type: boolean Why is this a hardware thing that belongs in dt? Enforce the constraint in the schema rather than text. > + > + microchip,beta-channel2: > + description: | > + The beta compensation factor for external channel 2 can be set > + by the user, or can be set automatically by the chip. > + If one wants to enable beta autodetection, omit this tag. > + Please consult the documentation if one wants to set a specific beta. > + If anti-parallel diode operation is enabled, the default value is set > + and can't be changed. > + type: boolean > + > + microchip,apdd-state: > + description: | > + Enable anti-parallel diode mode operation. > + Omit this tag to disable anti-parallel diode mode by default. This one is unusual. Maybe a little more description (I looked it up and am fine with why this is in DT) > + type: boolean > + > + microchip,recd12: > + description: | No need for | on paragraphs where formatting doesn't need to be maintained. > + Enable resistance error correction for external channels 1 and 2. > + Not all chips support resistance error correction on external > + channels 1 and 2, please consult the documentation. Enforce it in the schema, no need to say that chips don't support it in text. Look at the various allOf statements with compatible matches in other bindings for how to do that. > + Omit this tag to disable REC for channels 1 and 2 by default. > + type: boolean > + > + microchip,recd34: > + description: | > + Enable resistance error correction for external channels 3 and 4. > + Not all chips support resistance error correction on external > + channels 3 and 4, please consult the documentation. > + Omit this tag to disable REC for channels 3 and 4 by default. > + type: boolean > + > + label: > + description: Unique name to identify which device this is. > + > + vdd-supply: true > + > +patternProperties: > + "^channel@[1-4]+$": > + description: | > + Represents the external temperature channels to which a remote diode is > + connected. > + type: object > + > + properties: > + reg: > + items: > + minimum: 1 > + maximum: 4 > + > + microchip,ideality-factor: > + description: | > + Each channel has an ideality factor. > + Beta compensation and resistance error correction automatically correct > + for most ideality error. So ideality factor does not need to be adjusted in general. wrap at 80 chars. Also try to avoid explicit formatting where it isn't needed. > + Omit this tag in order to set the default value. > + Please consult the documentation if one wants to set a specific ideality value. > + $ref: /schemas/types.yaml#/definitions/uint32 > + > + label: > + description: Unique name to identify which channel this is. > + > + required: > + - reg > + > + unevaluatedProperties: false > + > +required: > + - compatible > + - reg > + - vdd-supply > + > +additionalProperties: false > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + temperature-sensor@4c { > + compatible = "microchip,mcp9985"; > + reg = <0x4c>; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + label = "temperature-sensor"; > + > + microchip,temp-hysteresis = <10>; > + microchip,extended-temp-range; > + microchip,apdd-state; > + microchip,recd12; > + microchip,recd34; > + vdd-supply = <&vdd>; > + > + channel@1{ > + reg = <0x1>; > + label = "CPU Temperature"; > + }; > + > + channel@2{ > + reg = <0x2>; > + label = "GPU Temperature"; > + }; > + }; > + }; > + > +... ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: iio: temperature: add support for MCP998X 2025-04-15 17:52 ` Jonathan Cameron @ 2025-04-17 11:40 ` Victor.Duicu 2025-04-18 16:01 ` Jonathan Cameron 0 siblings, 1 reply; 15+ messages in thread From: Victor.Duicu @ 2025-04-17 11:40 UTC (permalink / raw) To: jic23; +Cc: Marius.Cristea, andy, dlechner, linux-iio, nuno.sa, linux-kernel On Tue, 2025-04-15 at 18:52 +0100, Jonathan Cameron wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Tue, 15 Apr 2025 16:26:22 +0300 > <victor.duicu@microchip.com> wrote: > > > From: Victor Duicu <victor.duicu@microchip.com> > > > > This is the devicetree schema for Microchip MCP998X/33 and > > MCP998XD/33D Multichannel Automotive Temperature Monitor Family. > Hi Victor, > Hi Jonathan, > Please state briefly here in what way the parts are incompatible > as a justification for no fallback compatibles. Quite a bit > of that will become apparent when you enforce validity of parameters > as suggested below. > I am a bit confused, could you elaborate a bit on this point? Are you asking if the chips mcp9982, 83, 84 etc. are compatible among each other? > Various comments inline. > > > > Signed-off-by: Victor Duicu <victor.duicu@microchip.com> > > --- > ... > > > + > > + microchip,extended-temp-range: > > + description: | > > + Set the chip to work in the extended temperature range -64 > > degrees C to 191.875 degrees C. > > + Omit this tag to set the default range 0 degrees C to > > 127.875 degrees C > > + type: boolean > > I'm curious. Why does this belong in the DT binding? > Regarding microchip,extended-temp-range, my perspective is that the user knows beforehand which specific range of temperatures he needs. For example, if the device to be measured is a freezer, the user would be interested in temperatures below 0 degrees C. If we monitor a CPU, the user would be interested in temperatures above 0 degrees C. > > + > > + microchip,beta-channel1: > > + description: | > > + The beta compensation factor for external channel 1 can be > > set > > + by the user, or can be set automatically by the chip. > > + If one wants to enable beta autodetection, omit this tag. > > + Please consult the documentation if one wants to set a > > specific beta. > > + If anti-parallel diode operation is enabled, the default > > value is set > > + and can't be changed. > > + type: boolean > > Why is this a hardware thing that belongs in dt? Enforce the > constraint > in the schema rather than text. > With respect to the beta parameter, it is directly affected by the hardware part used. For example a CPU diode would require a different beta (that could be known by the manufacturer of the device and not know by the final user) as opposed to a diode connected transistor (that could be easily measured by the end user). However, I remain open to the idea of moving temperature range and channel betas to user space if you think it is better that way. Kind regards, Victor Duicu ... > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: iio: temperature: add support for MCP998X 2025-04-17 11:40 ` Victor.Duicu @ 2025-04-18 16:01 ` Jonathan Cameron 0 siblings, 0 replies; 15+ messages in thread From: Jonathan Cameron @ 2025-04-18 16:01 UTC (permalink / raw) To: Victor.Duicu Cc: Marius.Cristea, andy, dlechner, linux-iio, nuno.sa, linux-kernel On Thu, 17 Apr 2025 11:40:53 +0000 <Victor.Duicu@microchip.com> wrote: > On Tue, 2025-04-15 at 18:52 +0100, Jonathan Cameron wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > know the content is safe > > > > On Tue, 15 Apr 2025 16:26:22 +0300 > > <victor.duicu@microchip.com> wrote: > > > > > From: Victor Duicu <victor.duicu@microchip.com> > > > > > > This is the devicetree schema for Microchip MCP998X/33 and > > > MCP998XD/33D Multichannel Automotive Temperature Monitor Family. > > Hi Victor, > > > Hi Jonathan, > > > Please state briefly here in what way the parts are incompatible > > as a justification for no fallback compatibles. Quite a bit > > of that will become apparent when you enforce validity of parameters > > as suggested below. > > > I am a bit confused, could you elaborate a bit on this point? Are you > asking if the chips mcp9982, 83, 84 etc. are compatible among each > other? yes. It makes it easier for binding review to just have statement in the patch description of how they are different in ways the driver needs to know about. Can be simple things like 'only some devices have X' or they have differing numbers of channels. > > > > Various comments inline. > > > > > > Signed-off-by: Victor Duicu <victor.duicu@microchip.com> > > > --- > > > ... > > > > > + > > > + microchip,extended-temp-range: > > > + description: | > > > + Set the chip to work in the extended temperature range -64 > > > degrees C to 191.875 degrees C. > > > + Omit this tag to set the default range 0 degrees C to > > > 127.875 degrees C > > > + type: boolean > > > > I'm curious. Why does this belong in the DT binding? > > > > Regarding microchip,extended-temp-range, my perspective is that the > user knows beforehand which specific range of temperatures he needs. > For example, if the device to be measured is a freezer, the user would > be interested in temperatures below 0 degrees C. If we monitor a CPU, > the user would be interested in temperatures above 0 degrees C. Maybe - though also easy to control from userspace by making the offset writeable. > > > > + > > > + microchip,beta-channel1: > > > + description: | > > > + The beta compensation factor for external channel 1 can be > > > set > > > + by the user, or can be set automatically by the chip. > > > + If one wants to enable beta autodetection, omit this tag. > > > + Please consult the documentation if one wants to set a > > > specific beta. > > > + If anti-parallel diode operation is enabled, the default > > > value is set > > > + and can't be changed. > > > + type: boolean > > > > Why is this a hardware thing that belongs in dt? Enforce the > > constraint > > in the schema rather than text. > > > > With respect to the beta parameter, it is directly affected by the > hardware part used. For example a CPU diode would require a different > beta (that could be known by the manufacturer of the device and not > know by the final user) as opposed to a diode connected transistor > (that could be easily measured by the end user). > > However, I remain open to the idea of moving temperature range and > channel betas to user space if you think it is better that way. For the betas I was more curious about why the dt needs to distinguish between a manual setting and autodetection. When is autodetection a bad idea for example? Jonathan > > Kind regards, > Victor Duicu > ... > > > > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: iio: temperature: add support for MCP998X 2025-04-15 13:26 ` [PATCH v1 1/2] dt-bindings: iio: temperature: " victor.duicu 2025-04-15 17:52 ` Jonathan Cameron @ 2025-04-16 6:45 ` Krzysztof Kozlowski 1 sibling, 0 replies; 15+ messages in thread From: Krzysztof Kozlowski @ 2025-04-16 6:45 UTC (permalink / raw) To: victor.duicu, jic23 Cc: andy, dlechner, nuno.sa, marius.cristea, linux-iio, linux-kernel On 15/04/2025 15:26, victor.duicu@microchip.com wrote: > From: Victor Duicu <victor.duicu@microchip.com> > > This is the devicetree schema for Microchip MCP998X/33 and > MCP998XD/33D Multichannel Automotive Temperature Monitor Family. > > Signed-off-by: Victor Duicu <victor.duicu@microchip.com> <form letter> Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel. Tools like b4 or scripts/get_maintainer.pl provide you proper list of people, so fix your workflow. Tools might also fail if you work on some ancient tree (don't, instead use mainline) or work on fork of kernel (don't, instead use mainline). Just use b4 and everything should be fine, although remember about `b4 prep --auto-to-cc` if you added new patches to the patchset. You missed at least devicetree list (maybe more), so this won't be tested by automated tooling. Performing review on untested code might be a waste of time. Please kindly resend and include all necessary To/Cc entries. </form letter> > --- > .../iio/temperature/microchip,mcp9982.yaml | 182 ++++++++++++++++++ > 1 file changed, 182 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml > > diff --git a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml > new file mode 100644 > index 000000000000..8cbf897d1278 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml > @@ -0,0 +1,182 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/temperature/microchip,mcp9982.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Microchip MCP998X/33 and MCP998XD/33D Multichannel Automotive Temperature Monitor Family Wrap according to coding style document. > + > +maintainers: > + - Victor Duicu <victor.duicu@microchip.com> > + > +description: | > + The MCP998X/33 and MCP998XD/33D family is a high-accuracy 2-wire multichannel > + automotive temperature monitor. > + The datasheet can be found here: > + https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf > + > +properties: > + compatible: > + enum: > + - microchip,mcp9933 > + - microchip,mcp9933D > + - microchip,mcp9982 > + - microchip,mcp9982D > + - microchip,mcp9983 > + - microchip,mcp9983D > + - microchip,mcp9984 > + - microchip,mcp9984D > + - microchip,mcp9985 > + - microchip,mcp9985D > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 2 > + > + interrupt-names: > + description: | > + ALERT1 indicates a HIGH or LOW limit was exceeded. > + ALERT2 indicates a THERM limit was exceeded. > + items: > + - const: ALERT1 alert1 > + - const: ALERT2 alert2 > + > + "#address-cells": > + const: 1 > + > + "#size-cells": > + const: 0 > + > + microchip,temp-hysteresis: > + description: | > + Value of temperature limit hysteresis. > + Omit this tag to set the default value. > + $ref: /schemas/types.yaml#/definitions/uint32 temperature is in specific units, see property units in dtschema. > + > + microchip,extended-temp-range: > + description: | Do not need '|' unless you need to preserve formatting. > + Set the chip to work in the extended temperature range -64 degrees C to 191.875 degrees C. does not look wrapped. > + Omit this tag to set the default range 0 degrees C to 127.875 degrees C > + type: boolean Why is this a property of the board? I imagine someone might want different accuracy during runtime. > + > + microchip,beta-channel1: > + description: | > + The beta compensation factor for external channel 1 can be set > + by the user, or can be set automatically by the chip. > + If one wants to enable beta autodetection, omit this tag. > + Please consult the documentation if one wants to set a specific beta. > + If anti-parallel diode operation is enabled, the default value is set > + and can't be changed. > + type: boolean > + > + microchip,beta-channel2: > + description: | > + The beta compensation factor for external channel 2 can be set > + by the user, or can be set automatically by the chip. > + If one wants to enable beta autodetection, omit this tag. > + Please consult the documentation if one wants to set a specific beta. > + If anti-parallel diode operation is enabled, the default value is set > + and can't be changed. > + type: boolean > + > + microchip,apdd-state: > + description: | > + Enable anti-parallel diode mode operation. > + Omit this tag to disable anti-parallel diode mode by default. > + type: boolean > + > + microchip,recd12: > + description: | > + Enable resistance error correction for external channels 1 and 2. > + Not all chips support resistance error correction on external > + channels 1 and 2, please consult the documentation. > + Omit this tag to disable REC for channels 1 and 2 by default. > + type: boolean > + > + microchip,recd34: > + description: | > + Enable resistance error correction for external channels 3 and 4. > + Not all chips support resistance error correction on external Then this should be restricted per compatible in allOf:if:then:. > + channels 3 and 4, please consult the documentation. > + Omit this tag to disable REC for channels 3 and 4 by default. > + type: boolean > + > + label: > + description: Unique name to identify which device this is. > + > + vdd-supply: true > + > +patternProperties: > + "^channel@[1-4]+$": 4 or 44? Drop + > + description: | Do not need '|' unless you need to preserve formatting. > + Represents the external temperature channels to which a remote diode is > + connected. > + type: object > + > + properties: > + reg: > + items: > + minimum: 1 > + maximum: 4 > + > + microchip,ideality-factor: > + description: | > + Each channel has an ideality factor. > + Beta compensation and resistance error correction automatically correct > + for most ideality error. So ideality factor does not need to be adjusted in general. > + Omit this tag in order to set the default value. default: X > + Please consult the documentation if one wants to set a specific ideality value. > + $ref: /schemas/types.yaml#/definitions/uint32 > + > + label: > + description: Unique name to identify which channel this is. > + > + required: > + - reg > + > + unevaluatedProperties: false additionalProperties or reference appropriate schema for subnode. > + > +required: > + - compatible > + - reg > + - vdd-supply > + > +additionalProperties: false > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + temperature-sensor@4c { > + compatible = "microchip,mcp9985"; > + reg = <0x4c>; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + label = "temperature-sensor"; > + > + microchip,temp-hysteresis = <10>; > + microchip,extended-temp-range; > + microchip,apdd-state; > + microchip,recd12; > + microchip,recd34; > + vdd-supply = <&vdd>; > + > + channel@1{ Missing spaces. Please see DTS coding style or any DTS style. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v1 2/2] iio: temperature: add support for MCP998X 2025-04-15 13:26 [PATCH v1 0/2] add support for MCP998X victor.duicu 2025-04-15 13:26 ` [PATCH v1 1/2] dt-bindings: iio: temperature: " victor.duicu @ 2025-04-15 13:26 ` victor.duicu 2025-04-15 19:05 ` Andy Shevchenko 2025-04-18 18:07 ` Jonathan Cameron 2025-04-15 17:43 ` [PATCH v1 0/2] " Jonathan Cameron 2 siblings, 2 replies; 15+ messages in thread From: victor.duicu @ 2025-04-15 13:26 UTC (permalink / raw) To: jic23 Cc: andy, dlechner, nuno.sa, victor.duicu, marius.cristea, linux-iio, linux-kernel From: Victor Duicu <victor.duicu@microchip.com> This is the driver for Microchip MCP998X/33 and MCP998XD/33D Multichannel Automotive Monitor Family. Signed-off-by: Victor Duicu <victor.duicu@microchip.com> --- .../testing/sysfs-bus-iio-temperature-mcp9982 | 17 + MAINTAINERS | 7 + drivers/iio/temperature/Kconfig | 10 + drivers/iio/temperature/Makefile | 1 + drivers/iio/temperature/mcp9982.c | 794 ++++++++++++++++++ 5 files changed, 829 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982 create mode 100644 drivers/iio/temperature/mcp9982.c diff --git a/Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982 b/Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982 new file mode 100644 index 000000000000..de3360fb05be --- /dev/null +++ b/Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982 @@ -0,0 +1,17 @@ +What: /sys/bus/iio/devices/iio:deviceX/running_average_window +KernelVersion: 6.14 +Contact: linux-iio@vger.kernel.org +Description: + This attribute controls the number of samples for the + running average window applied to External Channel 1. + Using this method the temperature spikes are reduced. + X is the IIO index of the device. + +What: /sys/bus/iio/devices/iio:deviceX/running_average_window_available +KernelVersion: 6.14 +Contact: linux-iio@vger.kernel.org +Description: + Reading returns a list with the possible number of samples used + in the running average window. The window can be composed of 1, + 4 or 8 previous samples. X is the IIO index of the device. + diff --git a/MAINTAINERS b/MAINTAINERS index 01079a189c93..2cbad9beb182 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -15863,6 +15863,13 @@ S: Maintained F: Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml F: drivers/iio/adc/mcp3911.c +MICROCHIP MCP9982 TEMPERATURE DRIVER +M: Victor Duicu <victor.duicu@microchip.com> +L: linux-iio@vger.kernel.org +S: Supported +F: Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml +F: drivers/iio/temperature/mcp9982.c + MICROCHIP MMC/SD/SDIO MCI DRIVER M: Aubin Constans <aubin.constans@microchip.com> S: Maintained diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig index 1244d8e17d50..10b0967c1749 100644 --- a/drivers/iio/temperature/Kconfig +++ b/drivers/iio/temperature/Kconfig @@ -182,4 +182,14 @@ config MCP9600 This driver can also be built as a module. If so, the module will be called mcp9600. +config MCP9982 + tristate "Microchip Technology MCP9982 driver" + depends on I2C + help + Say yes here to build support for Microchip Technology's MCP998X/33 + and MCP998XD/33D Multichannel Automotive Temperature Monitor Family. + + This driver can also be built as a module. If so, the module + will be called mcp9982. + endmenu diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile index 07d6e65709f7..83f5f4bb4ff3 100644 --- a/drivers/iio/temperature/Makefile +++ b/drivers/iio/temperature/Makefile @@ -11,6 +11,7 @@ obj-$(CONFIG_MAX30208) += max30208.o obj-$(CONFIG_MAX31856) += max31856.o obj-$(CONFIG_MAX31865) += max31865.o obj-$(CONFIG_MCP9600) += mcp9600.o +obj-$(CONFIG_MCP9982) += mcp9982.o obj-$(CONFIG_MLX90614) += mlx90614.o obj-$(CONFIG_MLX90632) += mlx90632.o obj-$(CONFIG_MLX90632) += mlx90635.o diff --git a/drivers/iio/temperature/mcp9982.c b/drivers/iio/temperature/mcp9982.c new file mode 100644 index 000000000000..f4edc7b6a437 --- /dev/null +++ b/drivers/iio/temperature/mcp9982.c @@ -0,0 +1,794 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * IIO driver for MCP998X/33 and MCP998XD/33D Multichannel Automotive Temperature Monitor Family + * + * Copyright (C) 2025 Microchip Technology Inc. and its subsidiaries + * + * Author: Victor Duicu <victor.duicu@microchip.com> + * + * Datasheet can be found here: + * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf + * + */ + +#include <linux/bitfield.h> +#include <linux/i2c.h> +#include <linux/iio/events.h> +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> +#include <linux/irq.h> +#include <linux/limits.h> +#include <linux/property.h> +#include <linux/regmap.h> +#include <linux/string.h> +#include <linux/unaligned.h> +#include <linux/units.h> + +/* MCP9982 Registers */ +#define MCP9982_INT_HIGH_BYTE_ADDR(index) (2 * (index)) +#define MCP9982_INT_LOW_BYTE_ADDR(index) (2 * (index) + 1) +#define MCP9982_ONE_SHOT_ADDR 0x0A +#define MCP9982_INT_HIGH_LIMIT_ADDR 0x0B +#define MCP9982_INT_LOW_LIMIT_ADDR 0x0C +#define MCP9982_EXT1_HIGH_LIMIT_HIGH_BYTE_ADDR 0x0D +#define MCP9982_EXT1_HIGH_LIMIT_LOW_BYTE_ADDR 0x0E +#define MCP9982_EXT1_LOW_LIMIT_HIGH_BYTE_ADDR 0x0F +#define MCP9982_EXT1_LOW_LIMIT_LOW_BYTE_ADDR 0x10 +#define MCP9982_INT_THERM_LIMIT_ADDR 0x1D +#define MCP9982_EXT1_THERM_LIMIT_ADDR 0x1E +#define MCP9982_CFG_ADDR 0x22 +#define MCP9982_CONV_ADDR 0x24 +#define MCP9982_HYS_ADDR 0x25 +#define MCP9982_CONSEC_ALRT_ADDR 0x26 +#define MCP9982_ALRT_CFG_ADDR 0x27 +#define MCP9982_RUNNING_AVG_ADDR 0x28 +#define MCP9982_HOTTEST_CFG_ADDR 0x29 +#define MCP9982_STATUS_ADDR 0x2A +#define MCP9982_EXT_FAULT_STATUS_ADDR 0x2B +#define MCP9982_HIGH_LIMIT_STATUS_ADDR 0x2C +#define MCP9982_LOW_LIMIT_STATUS_ADDR 0x2D +#define MCP9982_THERM_LIMIT_STATUS_ADDR 0x2E +#define MCP9982_HOTTEST_HIGH_BYTE_ADDR 0x2F +#define MCP9982_HOTTEST_LOW_BYTE_ADDR 0x30 +#define MCP9982_HOTTEST_STATUS_ADDR 0x31 +#define MCP9982_THERM_SHTDWN_CFG_ADDR 0x32 +#define MCP9982_HRDW_THERM_SHTDWN_LIMIT_ADDR 0x33 +#define MCP9982_EXT_BETA_CFG_ADDR(index) ((index) + 52) +#define MCP9982_EXT_IDEAL_ADDR(index) ((index) + 54) + +/* MCP9982 Bits */ +#define MCP9982_CFG_MSKAL BIT(7) +#define MCP9982_CFG_RS BIT(6) +#define MCP9982_CFG_ATTHM BIT(5) +#define MCP9982_CFG_RECD12 BIT(4) +#define MCP9982_CFG_RECD34 BIT(3) +#define MCP9982_CFG_RANGE BIT(2) +#define MCP9982_CFG_DA_ENA BIT(1) +#define MCP9982_CFG_APDD BIT(0) + +/* MCP9982 Default Values */ +#define MCP9982_TEMP_OFFSET 64 +#define MCP9982_CFG_MSKAL_DEFAULT 1 +#define MCP9982_CFG_RS_DEFAULT 1 +#define MCP9982_CFG_ATTHM_DEFAULT 1 +#define MCP9982_CFG_DA_ENA_DEFAULT 0 +#define MCP9982_CONSEC_ALRT_DEFAULT 112 +#define MCP9982_SAMPLING_FREQ_CODE_DEFAULT 6 +#define MCP9982_BETA_AUTODETECT_ENABLE 16 +#define MCP9982_BETA_MAX_VALUE 15 +#define MCP9982_IDEALITY_MAX_VALUE 63 +#define MCP9982_HYSTERESIS_MAX_VALUE 255 +#define MCP9982_IDEALITY_FACTOR_DEFAULT 18 +#define MCP9982_HYSTERESIS_DEFAULT 10 +#define MCP9982_TEMP_RANGE_DEFAULT 0 +#define MCP9982_APDD_DEFAULT 1 +#define MCP9982_RECD_DEFAULT 1 + +#define MCP9982_NR_CUSTOM_ATTR 2 +#define MCP9982_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr) + +/* Running average window can be set as: + * - 1 sample + * - 4 samples + * - 8 samples + */ +#define MCP9982_RUNNING_AVERAGE_WINDOW_AVAILABLE "1 4 8" + +/* The maximum number of channels a member of the family can have */ +#define MCP9982_MAX_NUM_CHANNELS 5 + +#define MCP9982_CHAN(index, si, __address) ({ \ + struct iio_chan_spec __chan = { \ + .type = IIO_TEMP, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ + .channel = index, \ + .address = __address, \ + .scan_index = si, \ + .scan_type = { \ + .sign = 'u', \ + .realbits = 8, \ + .storagebits = 8, \ + .endianness = IIO_CPU \ + }, \ + .indexed = 1, \ + }; \ + __chan; \ +}) + +enum mcp9982_ids { + MCP9933, + MCP9933D, + MCP9982, + MCP9982D, + MCP9983, + MCP9983D, + MCP9984, + MCP9984D, + MCP9985, + MCP9985D, +}; + +/* + * struct mcp9982_features - features of a mcp9982 instance + * @phys_channels: number of physical channels supported by the chip + * @name: chip's name + */ +struct mcp9982_features { + u8 phys_channels; + const char *name; +}; + +static const struct mcp9982_features mcp9982_chip_config[] = { + [MCP9933] = { + .name = "mcp9933", + .phys_channels = 3, + }, + [MCP9933D] = { + .name = "mcp9933D", + .phys_channels = 3, + }, + [MCP9982] = { + .name = "mcp9982", + .phys_channels = 2, + }, + [MCP9982D] = { + .name = "mcp9982D", + .phys_channels = 2, + }, + [MCP9983] = { + .name = "mcp9983", + .phys_channels = 3, + }, + [MCP9983D] = { + .name = "mcp9983D", + .phys_channels = 3, + }, + [MCP9984] = { + .name = "mcp9984", + .phys_channels = 4, + }, + [MCP9984D] = { + .name = "mcp9984D", + .phys_channels = 4, + }, + [MCP9985] = { + .name = "mcp9985", + .phys_channels = 5, + }, + [MCP9985D] = { + .name = "mcp9985D", + .phys_channels = 5, + }, +}; + +static const int mcp9982_fractional_values[] = { + 0, + 125000, + 250000, + 375000, + 500000, + 625000, + 750000, + 875000, +}; + +static const int mcp9982_conv_rate[][2] = { + {0, 62500}, + {0, 125000}, + {0, 250000}, + {0, 500000}, + {1, 0}, + {2, 0}, + {4, 0}, + {8, 0}, + {16, 0}, + {32, 0}, + {64, 0}, +}; + +/* mcp9982 regmap configuration */ +static const struct regmap_range mcp9982_regmap_wr_ranges[] = { + regmap_reg_range(MCP9982_ONE_SHOT_ADDR, MCP9982_EXT1_LOW_LIMIT_LOW_BYTE_ADDR), + regmap_reg_range(MCP9982_INT_THERM_LIMIT_ADDR, MCP9982_EXT1_THERM_LIMIT_ADDR), + regmap_reg_range(MCP9982_CFG_ADDR, MCP9982_CFG_ADDR), + regmap_reg_range(MCP9982_CONV_ADDR, MCP9982_HOTTEST_CFG_ADDR), + regmap_reg_range(MCP9982_THERM_SHTDWN_CFG_ADDR, MCP9982_THERM_SHTDWN_CFG_ADDR), + regmap_reg_range(MCP9982_EXT_BETA_CFG_ADDR(0), MCP9982_EXT_IDEAL_ADDR(3)), +}; + +static const struct regmap_access_table mcp9982_regmap_wr_table = { + .yes_ranges = mcp9982_regmap_wr_ranges, + .n_yes_ranges = ARRAY_SIZE(mcp9982_regmap_wr_ranges), +}; + +static const struct regmap_range mcp9982_regmap_rd_ranges[] = { + regmap_reg_range(MCP9982_INT_HIGH_BYTE_ADDR(0), MCP9982_EXT1_LOW_LIMIT_LOW_BYTE_ADDR), + regmap_reg_range(MCP9982_INT_THERM_LIMIT_ADDR, MCP9982_EXT1_THERM_LIMIT_ADDR), + regmap_reg_range(MCP9982_CFG_ADDR, MCP9982_CFG_ADDR), + regmap_reg_range(MCP9982_CONV_ADDR, MCP9982_EXT_IDEAL_ADDR(3)), +}; + +static const struct regmap_access_table mcp9982_regmap_rd_table = { + .yes_ranges = mcp9982_regmap_rd_ranges, + .n_yes_ranges = ARRAY_SIZE(mcp9982_regmap_rd_ranges), +}; + +static const struct regmap_config mcp9982_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .rd_table = &mcp9982_regmap_rd_table, + .wr_table = &mcp9982_regmap_wr_table, +}; + +enum mcp9982_channels { + MCP9982_CHAN_INT = 0, + MCP9982_CHAN_EXT1 = 1, + MCP9982_CHAN_EXT2 = 2, + MCP9982_CHAN_EXT3 = 3, + MCP9982_CHAN_EXT4 = 4, +}; + +/** + * struct mcp9992_priv - information about chip parameters + * @client: the i2c-client attached to the device + * @regmap: device register map + * @iio_info iio_info + * @iio_chan specifications of channels + * @num_channels number of physical channels + * @lock synchronize access to driver's state members + * @running_avg number of samples in the running average window + * @hysteresis value of temperature hysteresis + * @temp_range_code coded value representing the set temperature range + * @labels labels of the channels + * @chip_name name of the chip present + * @beta_user_value value given by the user for beta on channel 1 and 2 + * @apdd state of anti-parallel diode mode + * @recd12 state of REC on channels 1 and 2 + * @recd34 state of REC on channels 3 and 4 + * @ideality_user_value values given by user to ideality factor for all channels + */ + +struct mcp9982_priv { + struct i2c_client *client; + struct regmap *regmap; + struct iio_info iio_info; + + struct iio_chan_spec *iio_chan; + u8 num_channels; + + /* + * Synchronize access to private members, and ensure + * atomicity of consecutive regmap operations. + */ + struct mutex lock; + + int running_avg; + int hysteresis; + int temp_range_code; + char *labels[MCP9982_MAX_NUM_CHANNELS]; + char *chip_name; + int beta_user_value[2]; + int apdd; + int recd12; + int recd34; + int ideality_user_value[4]; +}; + +static int mcp9982_read_avail(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + const int **vals, int *type, int *length, long mask) +{ + switch (mask) { + case IIO_CHAN_INFO_SAMP_FREQ: + *type = IIO_VAL_INT_PLUS_MICRO; + *vals = mcp9982_conv_rate[0]; + *length = ARRAY_SIZE(mcp9982_conv_rate) * 2; + return IIO_AVAIL_LIST; + default: + return -EINVAL; + } +} + +static int mcp9982_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct mcp9982_priv *priv = iio_priv(indio_dev); + int ret, val3, HIGH_BYTE, LOW_BYTE; + + /* Write in ONESHOT register to take a new reading */ + ret = regmap_write(priv->regmap, MCP9982_ONE_SHOT_ADDR, 1); + if (ret) + return ret; + + guard(mutex)(&priv->lock); + + switch (mask) { + case IIO_CHAN_INFO_RAW: + HIGH_BYTE = MCP9982_INT_HIGH_BYTE_ADDR(chan->channel); + LOW_BYTE = MCP9982_INT_LOW_BYTE_ADDR(chan->channel); + + ret = regmap_read(priv->regmap, HIGH_BYTE, val); + if (ret) + return ret; + + if (priv->temp_range_code) + *val -= MCP9982_TEMP_OFFSET; + + ret = regmap_read(priv->regmap, LOW_BYTE, val2); + if (ret) + return ret; + + *val2 = mcp9982_fractional_values[*val2 >> 5]; + + return IIO_VAL_INT_PLUS_MICRO; + case IIO_CHAN_INFO_SAMP_FREQ: + ret = regmap_read(priv->regmap, MCP9982_CONV_ADDR, &val3); + if (ret) + return ret; + + *val = mcp9982_conv_rate[val3][0]; + *val2 = mcp9982_conv_rate[val3][1]; + + return IIO_VAL_INT_PLUS_MICRO; + default: + return -EINVAL; + } +} + +static int mcp9982_read_label(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, + char *label) +{ + struct mcp9982_priv *priv = iio_priv(indio_dev); + + if (chan->channel < 0 || chan->channel > 4) + return -EINVAL; + + return sprintf(label, "%s\n", priv->labels[chan->channel]); +} + +static int mcp9982_write_raw_get_fmt(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, + long info) +{ + switch (info) { + case IIO_CHAN_INFO_SAMP_FREQ: + return IIO_VAL_INT_PLUS_MICRO; + default: + return -EINVAL; + } +} + +static int mcp9982_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, + int val, int val2, long mask) +{ + struct mcp9982_priv *priv = iio_priv(indio_dev); + struct device *dev = &priv->client->dev; + int i; + int status = 0; + + guard(mutex)(&priv->lock); + switch (mask) { + case IIO_CHAN_INFO_SAMP_FREQ: + for (i = 0; i < ARRAY_SIZE(mcp9982_conv_rate); i++) { + if (val == mcp9982_conv_rate[i][0] && + val2 == mcp9982_conv_rate[i][1]){ + status = 1; + break; + } + } + if (!status) + return dev_err_probe(dev, -EINVAL, "Sampling Frequency is invalid\n"); + + return regmap_write(priv->regmap, MCP9982_CONV_ADDR, i); + default: + return -EINVAL; + } +} + +static ssize_t mcp9982_running_average_window_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct mcp9982_priv *priv = iio_priv(indio_dev); + + return sprintf(buf, "%u sample(s)\n", priv->running_avg); +} + +static ssize_t mcp9982_running_average_window_avail_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + return sprintf(buf, "%s\n", MCP9982_RUNNING_AVERAGE_WINDOW_AVAILABLE); +} + +static ssize_t mcp9982_running_average_window_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct mcp9982_priv *priv = iio_priv(indio_dev); + int val, ret, reg_val; + + if (kstrtouint(buf, 10, &val)) { + dev_err(dev, "Value is not a number\n"); + return -EINVAL; + } + + switch (val) { + case 1: + reg_val = 0; + break; + case 4: + reg_val = 1; + break; + case 8: + reg_val = 3; + break; + default: + dev_err(dev, "Value is invalid\n"); + return -EINVAL; + } + + guard(mutex)(&priv->lock); + + ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, reg_val); + priv->running_avg = val; + + return count; +} + +static IIO_DEVICE_ATTR(running_average_window, 0644, mcp9982_running_average_window_show, + mcp9982_running_average_window_store, 0); + +static IIO_DEVICE_ATTR(running_average_window_available, 0444, + mcp9982_running_average_window_avail_show, NULL, 1); + +static int mcp9982_prep_custom_attributes(struct mcp9982_priv *priv, struct iio_dev *indio_dev) +{ + struct attribute **mcp9982_custom_attr; + struct attribute_group *mcp9982_group; + struct device *dev = &priv->client->dev; + + mcp9982_group = devm_kzalloc(dev, sizeof(*mcp9982_group), GFP_KERNEL); + if (!mcp9982_group) + return -ENOMEM; + + mcp9982_custom_attr = devm_kzalloc(dev, MCP9982_NR_CUSTOM_ATTR * + sizeof(*mcp9982_group) + 1, GFP_KERNEL); + if (!mcp9982_custom_attr) + return -ENOMEM; + + mcp9982_custom_attr[0] = MCP9982_DEV_ATTR(running_average_window); + mcp9982_custom_attr[1] = MCP9982_DEV_ATTR(running_average_window_available); + + mcp9982_group->attrs = mcp9982_custom_attr; + priv->iio_info.attrs = mcp9982_group; + + return 0; +} + +static const struct iio_info mcp9982_info = { + .read_raw = mcp9982_read_raw, + .read_label = mcp9982_read_label, + .read_avail = mcp9982_read_avail, + .write_raw_get_fmt = mcp9982_write_raw_get_fmt, + .write_raw = mcp9982_write_raw, +}; + +static int mcp9982_init(struct mcp9982_priv *priv) +{ + int ret, i; + u8 val; + + val = FIELD_PREP(MCP9982_CFG_MSKAL, MCP9982_CFG_MSKAL_DEFAULT) | + FIELD_PREP(MCP9982_CFG_RS, MCP9982_CFG_RS_DEFAULT) | + FIELD_PREP(MCP9982_CFG_ATTHM, MCP9982_CFG_ATTHM_DEFAULT) | + FIELD_PREP(MCP9982_CFG_RECD12, priv->recd12) | + FIELD_PREP(MCP9982_CFG_RECD34, priv->recd34) | + FIELD_PREP(MCP9982_CFG_RANGE, priv->temp_range_code) | + FIELD_PREP(MCP9982_CFG_DA_ENA, MCP9982_CFG_DA_ENA_DEFAULT) | + FIELD_PREP(MCP9982_CFG_APDD, priv->apdd); + ret = regmap_write(priv->regmap, MCP9982_CFG_ADDR, val); + if (ret) + return ret; + + ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, MCP9982_SAMPLING_FREQ_CODE_DEFAULT); + if (ret) + return ret; + + ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, priv->hysteresis); + if (ret) + return ret; + + ret = regmap_write(priv->regmap, MCP9982_CONSEC_ALRT_ADDR, MCP9982_CONSEC_ALRT_DEFAULT); + if (ret) + return ret; + + /* RUNNING AVG DEFAULT is 0 */ + ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, 0); + if (ret) + return ret; + priv->running_avg = 1; + + /* HOTTEST CFG DEFAULT is 0 */ + ret = regmap_write(priv->regmap, MCP9982_HOTTEST_CFG_ADDR, 0); + if (ret) + return ret; + + /* Set beta compensation for all channels */ + for (i = 0; i < 2; i++) { + ret = regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(i), + priv->beta_user_value[i]); + if (ret) + return ret; + } + + /* Set ideality factor for all channels */ + for (i = 0; i < 4; i++) { + ret = regmap_write(priv->regmap, MCP9982_EXT_IDEAL_ADDR(i), + priv->ideality_user_value[i]); + if (ret) + return ret; + } + + return 0; +} + +static int mcp9982_parse_of_fw(struct iio_dev *indio_dev) +{ + struct mcp9982_priv *priv = iio_priv(indio_dev); + struct device *dev = &priv->client->dev; + int ret, reg_nr, iio_idx, i; + + if (device_property_present(dev, "microchip,temp-hysteresis")) { + ret = device_property_read_u32(dev, "microchip,temp-hysteresis", &priv->hysteresis); + if (ret) + return dev_err_probe(dev, ret, "Cannot read hysteresis property\n"); + + if (priv->hysteresis > MCP9982_HYSTERESIS_MAX_VALUE) + return dev_err_probe(dev, ret, + "Hysteresis value is higher than maximum\n"); + } else { + priv->hysteresis = MCP9982_HYSTERESIS_DEFAULT; + } + + if (device_property_present(dev, "microchip,extended-temp-range")) + priv->temp_range_code = 1; + else + priv->temp_range_code = MCP9982_TEMP_RANGE_DEFAULT; + + if (device_property_present(dev, "microchip,beta-channel1")) { + ret = device_property_read_u32(dev, "microchip,beta-channel1", + &priv->beta_user_value[0]); + if (ret) + return ret; + + if (priv->beta_user_value[0] > MCP9982_BETA_MAX_VALUE) + return dev_err_probe(dev, -EINVAL, "Beta 1 value is higher than max\n"); + } else { + priv->beta_user_value[0] = MCP9982_BETA_AUTODETECT_ENABLE; + } + + if (device_property_present(dev, "microchip,beta-channel2")) { + ret = device_property_read_u32(dev, "microchip,beta-channel2", + &priv->beta_user_value[1]); + if (ret) + return ret; + + if (priv->beta_user_value[1] > MCP9982_BETA_MAX_VALUE) + return dev_err_probe(dev, -EINVAL, "Beta 2 value is higher than max\n"); + } else { + priv->beta_user_value[1] = MCP9982_BETA_AUTODETECT_ENABLE; + } + + if (device_property_present(dev, "microchip,apdd-state")) + priv->apdd = 0; + else + priv->apdd = MCP9982_APDD_DEFAULT; + + if (device_property_present(dev, "microchip,recd12")) + priv->recd12 = 0; + else + priv->recd12 = MCP9982_RECD_DEFAULT; + + if (device_property_present(dev, "microchip,recd34")) + priv->recd34 = 0; + else + priv->recd34 = MCP9982_RECD_DEFAULT; + + ret = device_property_read_string(dev, "compatible", (const char **)&priv->chip_name); + if (ret) + return dev_err_probe(dev, ret, "Cannot read compatible property\n"); + + priv->chip_name = &priv->chip_name[10]; + priv->num_channels = device_get_child_node_count(dev) + 1; + + for (i = 0; i < 10; i++) + if (strcmp(priv->chip_name, mcp9982_chip_config[i].name) == 0) + break; + if (priv->num_channels > mcp9982_chip_config[i].phys_channels) + return dev_err_probe(dev, -EINVAL, + "The number of channels does not match the chip\n"); + + priv->iio_chan = devm_kzalloc(dev, priv->num_channels * sizeof(*priv->iio_chan), + GFP_KERNEL); + if (!priv->iio_chan) + return -ENOMEM; + + /* first channel is internal and always present */ + priv->iio_chan[0] = MCP9982_CHAN(MCP9982_CHAN_INT, MCP9982_CHAN_INT, + MCP9982_INT_HIGH_BYTE_ADDR(0)); + priv->labels[0] = "internal diode"; + iio_idx++; + device_for_each_child_node_scoped(dev, child) { + ret = fwnode_property_read_u32(child, "reg", ®_nr); + if (reg_nr >= mcp9982_chip_config[i].phys_channels) + return dev_err_probe(dev, -EINVAL, + "The index of the channels does not match the chip\n"); + + if (fwnode_property_present(child, "microchip,ideality-factor")) { + ret = fwnode_property_read_u32(child, "microchip,ideality-factor", + &priv->ideality_user_value[reg_nr - 1]); + if (priv->ideality_user_value[reg_nr - 1] > MCP9982_IDEALITY_MAX_VALUE) + return dev_err_probe(dev, -EINVAL, + "The ideality value is higher than maximum\n"); + } else { + priv->ideality_user_value[reg_nr - 1] = MCP9982_IDEALITY_FACTOR_DEFAULT; + } + + ret = fwnode_property_read_string(child, "label", + (const char **)&priv->labels[reg_nr]); + + priv->iio_chan[iio_idx++] = MCP9982_CHAN(reg_nr, reg_nr, + MCP9982_INT_HIGH_BYTE_ADDR(reg_nr)); + } + + return 0; +} + +static int mcp9982_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct mcp9982_priv *priv; + struct iio_dev *indio_dev; + int ret; + + indio_dev = devm_iio_device_alloc(dev, sizeof(*priv)); + if (!indio_dev) + return -ENOMEM; + + priv = iio_priv(indio_dev); + priv->client = client; + i2c_set_clientdata(client, indio_dev); + + priv->regmap = devm_regmap_init_i2c(client, &mcp9982_regmap_config); + if (IS_ERR(priv->regmap)) + return dev_err_probe(dev, PTR_ERR(priv->regmap), + "Cannot initialize register map\n"); + + ret = devm_mutex_init(dev, &priv->lock); + if (ret) + return ret; + + ret = mcp9982_parse_of_fw(indio_dev); + if (ret) + return dev_err_probe(dev, ret, "Parameter parsing error\n"); + + ret = mcp9982_init(priv); + if (ret) + return dev_err_probe(dev, ret, "Cannot initialize device\n"); + + priv->iio_info = mcp9982_info; + + indio_dev->name = priv->chip_name; + indio_dev->info = &priv->iio_info; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->channels = priv->iio_chan; + indio_dev->num_channels = priv->num_channels; + + ret = mcp9982_prep_custom_attributes(priv, indio_dev); + if (ret < 0) + return dev_err_probe(&client->dev, ret, + "Can't configure custom attributes for MCP9982 device\n"); + + ret = devm_iio_device_register(dev, indio_dev); + if (ret) + return dev_err_probe(dev, ret, "Cannot register IIO device\n"); + + return 0; +} + +static const struct i2c_device_id mcp9982_id[] = { + { .name = "mcp9933", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9933] }, + { .name = "mcp9933D", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9933D] }, + { .name = "mcp9982", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9982] }, + { .name = "mcp9982D", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9982D] }, + { .name = "mcp9983", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9983] }, + { .name = "mcp9983D", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9983D] }, + { .name = "mcp9984", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9984] }, + { .name = "mcp9984D", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9984D] }, + { .name = "mcp9985", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9985] }, + { .name = "mcp9985D", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9985D] }, + { } +}; +MODULE_DEVICE_TABLE(i2c, mcp9982_id); + +static const struct of_device_id mcp9982_of_match[] = { + { + .compatible = "microchip,mcp9933", + .data = &mcp9982_chip_config[MCP9933] + }, + { + .compatible = "microchip,mcp9933D", + .data = &mcp9982_chip_config[MCP9933D] + }, + { + .compatible = "microchip,mcp9982", + .data = &mcp9982_chip_config[MCP9982] + }, + { + .compatible = "microchip,mcp9982D", + .data = &mcp9982_chip_config[MCP9982D] + }, + { + .compatible = "microchip,mcp9983", + .data = &mcp9982_chip_config[MCP9983] + }, + { + .compatible = "microchip,mcp9983D", + .data = &mcp9982_chip_config[MCP9983D] + }, + { + .compatible = "microchip,mcp9984", + .data = &mcp9982_chip_config[MCP9984] + }, + { + .compatible = "microchip,mcp9984D", + .data = &mcp9982_chip_config[MCP9984D] + }, + { + .compatible = "microchip,mcp9985", + .data = &mcp9982_chip_config[MCP9985] + }, + { + .compatible = "microchip,mcp9985D", + .data = &mcp9982_chip_config[MCP9985D] + }, + { } +}; +MODULE_DEVICE_TABLE(of, mcp9982_of_match); + +static struct i2c_driver mcp9982_driver = { + .driver = { + .name = "mcp9982", + .of_match_table = mcp9982_of_match, + }, + .probe = mcp9982_probe, + .id_table = mcp9982_id, +}; + +module_i2c_driver(mcp9982_driver); + +MODULE_AUTHOR("Victor Duicu <victor.duicu@microchip.com>"); +MODULE_DESCRIPTION("MCP998X/33 and MCP998XD/33D Multichannel Automotive Temperature Monitor Driver"); +MODULE_LICENSE("GPL"); -- 2.45.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v1 2/2] iio: temperature: add support for MCP998X 2025-04-15 13:26 ` [PATCH v1 2/2] " victor.duicu @ 2025-04-15 19:05 ` Andy Shevchenko 2025-05-22 9:18 ` Victor.Duicu 2025-04-18 18:07 ` Jonathan Cameron 1 sibling, 1 reply; 15+ messages in thread From: Andy Shevchenko @ 2025-04-15 19:05 UTC (permalink / raw) To: victor.duicu Cc: jic23, andy, dlechner, nuno.sa, marius.cristea, linux-iio, linux-kernel On Tue, Apr 15, 2025 at 4:27 PM <victor.duicu@microchip.com> wrote: > > This is the driver for Microchip MCP998X/33 and MCP998XD/33D > Multichannel Automotive Monitor Family. ... > +KernelVersion: 6.14 This boat is already sailing, it should be v6.16 at bare minimum. > +Contact: linux-iio@vger.kernel.org > +Description: > + This attribute controls the number of samples for the > + running average window applied to External Channel 1. > + Using this method the temperature spikes are reduced. > + X is the IIO index of the device. ... > +KernelVersion: 6.14 Ditto. > +Contact: linux-iio@vger.kernel.org > +Description: > + Reading returns a list with the possible number of samples used > + in the running average window. The window can be composed of 1, > + 4 or 8 previous samples. X is the IIO index of the device. ... > +/* > + * IIO driver for MCP998X/33 and MCP998XD/33D Multichannel Automotive Temperature Monitor Family > + * > + * Copyright (C) 2025 Microchip Technology Inc. and its subsidiaries > + * > + * Author: Victor Duicu <victor.duicu@microchip.com> > + * > + * Datasheet can be found here: > + * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf > + * Redundant blank line > + */ ... > +#include <linux/bitfield.h> + bits.h + err.h > +#include <linux/i2c.h> > +#include <linux/iio/events.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > +#include <linux/irq.h> > +#include <linux/limits.h> > +#include <linux/property.h> > +#include <linux/regmap.h> > +#include <linux/string.h> > +#include <linux/unaligned.h> > +#include <linux/units.h> Please, make sure you follow the IWYU principle. ... > +#define MCP9982_INT_HIGH_BYTE_ADDR(index) (2 * (index)) > +#define MCP9982_INT_LOW_BYTE_ADDR(index) (2 * (index) + 1) Why? Can't you use __be16 everywhere and bulk IO? ... > +#define MCP9982_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr) Useless macro that makes code harder to read. ... > +#define MCP9982_CHAN(index, si, __address) ({ \ > + struct iio_chan_spec __chan = { \ > + .type = IIO_TEMP, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > + .channel = index, \ > + .address = __address, \ > + .scan_index = si, \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = 8, \ > + .storagebits = 8, \ > + .endianness = IIO_CPU \ > + }, \ > + .indexed = 1, \ > + }; \ > + __chan; \ Why in this form and not as a compound literal? > +}) ... > +/* > + * struct mcp9982_features - features of a mcp9982 instance > + * @phys_channels: number of physical channels supported by the chip > + * @name: chip's name > + */ > +struct mcp9982_features { > + u8 phys_channels; > + const char *name; Have you run `pahole`? Has it found a room to improve? > +}; ... > +/** > + * struct mcp9992_priv - information about chip parameters > + * @client: the i2c-client attached to the device > + * @regmap: device register map > + * @iio_info iio_info > + * @iio_chan specifications of channels > + * @num_channels number of physical channels > + * @lock synchronize access to driver's state members > + * @running_avg number of samples in the running average window > + * @hysteresis value of temperature hysteresis > + * @temp_range_code coded value representing the set temperature range > + * @labels labels of the channels > + * @chip_name name of the chip present > + * @beta_user_value value given by the user for beta on channel 1 and 2 > + * @apdd state of anti-parallel diode mode > + * @recd12 state of REC on channels 1 and 2 > + * @recd34 state of REC on channels 3 and 4 > + * @ideality_user_value values given by user to ideality factor for all channels > + */ > + Redundant blank line. ... > +struct mcp9982_priv { > + struct i2c_client *client; For what do you need a client? Wouldn't struct device *dev suffice? And if so, can't it be retrieved from regmap when needed? > + struct regmap *regmap; > + struct iio_info iio_info; > + > + struct iio_chan_spec *iio_chan; > + u8 num_channels; > + > + /* > + * Synchronize access to private members, and ensure > + * atomicity of consecutive regmap operations. > + */ > + struct mutex lock; > + > + int running_avg; > + int hysteresis; > + int temp_range_code; > + char *labels[MCP9982_MAX_NUM_CHANNELS]; > + char *chip_name; > + int beta_user_value[2]; > + int apdd; > + int recd12; > + int recd34; > + int ideality_user_value[4]; > +}; ... > +static int mcp9982_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct mcp9982_priv *priv = iio_priv(indio_dev); > + int ret, val3, HIGH_BYTE, LOW_BYTE; No way we name variables in the capital letters. And use __be16. > + > + /* Write in ONESHOT register to take a new reading */ > + ret = regmap_write(priv->regmap, MCP9982_ONE_SHOT_ADDR, 1); > + if (ret) > + return ret; > + > + guard(mutex)(&priv->lock); > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + HIGH_BYTE = MCP9982_INT_HIGH_BYTE_ADDR(chan->channel); > + LOW_BYTE = MCP9982_INT_LOW_BYTE_ADDR(chan->channel); > + > + ret = regmap_read(priv->regmap, HIGH_BYTE, val); > + if (ret) > + return ret; > + > + if (priv->temp_range_code) > + *val -= MCP9982_TEMP_OFFSET; > + > + ret = regmap_read(priv->regmap, LOW_BYTE, val2); > + if (ret) > + return ret; > + > + *val2 = mcp9982_fractional_values[*val2 >> 5]; > + > + return IIO_VAL_INT_PLUS_MICRO; > + case IIO_CHAN_INFO_SAMP_FREQ: > + ret = regmap_read(priv->regmap, MCP9982_CONV_ADDR, &val3); > + if (ret) > + return ret; > + > + *val = mcp9982_conv_rate[val3][0]; > + *val2 = mcp9982_conv_rate[val3][1]; > + > + return IIO_VAL_INT_PLUS_MICRO; > + default: > + return -EINVAL; > + } > +} ... > + return sprintf(label, "%s\n", priv->labels[chan->channel]); + sprintf.h ... > +static int mcp9982_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + struct mcp9982_priv *priv = iio_priv(indio_dev); > + struct device *dev = &priv->client->dev; > + int i; Why signed? > + int status = 0; Why not boolean? > + guard(mutex)(&priv->lock); > + switch (mask) { > + case IIO_CHAN_INFO_SAMP_FREQ: > + for (i = 0; i < ARRAY_SIZE(mcp9982_conv_rate); i++) { + array_size.h > + if (val == mcp9982_conv_rate[i][0] && > + val2 == mcp9982_conv_rate[i][1]){ > + status = 1; > + break; > + } > + } > + if (!status) > + return dev_err_probe(dev, -EINVAL, "Sampling Frequency is invalid\n"); > + > + return regmap_write(priv->regmap, MCP9982_CONV_ADDR, i); > + default: > + return -EINVAL; > + } > +} ... > +static ssize_t mcp9982_running_average_window_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct mcp9982_priv *priv = iio_priv(indio_dev); > + > + return sprintf(buf, "%u sample(s)\n", priv->running_avg); Please, read the documentation about this. s*printf() must not be used here (in ->show() callbacks). There are special APIs. ... > +static ssize_t mcp9982_running_average_window_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct mcp9982_priv *priv = iio_priv(indio_dev); > + int val, ret, reg_val; > + > + if (kstrtouint(buf, 10, &val)) { Do not shadow the actuall error code. > + dev_err(dev, "Value is not a number\n"); > + return -EINVAL; > + } > + > + switch (val) { > + case 1: > + reg_val = 0; > + break; > + case 4: > + reg_val = 1; > + break; > + case 8: > + reg_val = 3; > + break; > + default: > + dev_err(dev, "Value is invalid\n"); > + return -EINVAL; > + } > + > + guard(mutex)(&priv->lock); > + > + ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, reg_val); How ret is being used? I think I have a déjà vu about this code... I think I commented this or something like this already and there seems to be no reaction... > + priv->running_avg = val; > + > + return count; > +} ... > +static int mcp9982_prep_custom_attributes(struct mcp9982_priv *priv, struct iio_dev *indio_dev) > +{ > + struct attribute **mcp9982_custom_attr; > + struct attribute_group *mcp9982_group; > + struct device *dev = &priv->client->dev; > + > + mcp9982_group = devm_kzalloc(dev, sizeof(*mcp9982_group), GFP_KERNEL); + device/devres.h > + if (!mcp9982_group) > + return -ENOMEM; > + > + mcp9982_custom_attr = devm_kzalloc(dev, MCP9982_NR_CUSTOM_ATTR * > + sizeof(*mcp9982_group) + 1, GFP_KERNEL); No, use devm_kcalloc() and I'm not sure you have got the size correctly here. > + if (!mcp9982_custom_attr) > + return -ENOMEM; > + > + mcp9982_custom_attr[0] = MCP9982_DEV_ATTR(running_average_window); > + mcp9982_custom_attr[1] = MCP9982_DEV_ATTR(running_average_window_available); > + > + mcp9982_group->attrs = mcp9982_custom_attr; > + priv->iio_info.attrs = mcp9982_group; > + > + return 0; > +} > + if (device_property_present(dev, "microchip,beta-channel1")) { ... > + return dev_err_probe(dev, -EINVAL, "Beta 1 value is higher than max\n"); + dev_printk.h ... > + priv->iio_chan = devm_kzalloc(dev, priv->num_channels * sizeof(*priv->iio_chan), > + GFP_KERNEL); kcalloc() > + if (!priv->iio_chan) > + return -ENOMEM; ... > + device_for_each_child_node_scoped(dev, child) { > + ret = fwnode_property_read_u32(child, "reg", ®_nr); How is ret being used? > + if (reg_nr >= mcp9982_chip_config[i].phys_channels) > + return dev_err_probe(dev, -EINVAL, > + "The index of the channels does not match the chip\n"); > + > + if (fwnode_property_present(child, "microchip,ideality-factor")) { > + ret = fwnode_property_read_u32(child, "microchip,ideality-factor", > + &priv->ideality_user_value[reg_nr - 1]); > + if (priv->ideality_user_value[reg_nr - 1] > MCP9982_IDEALITY_MAX_VALUE) > + return dev_err_probe(dev, -EINVAL, > + "The ideality value is higher than maximum\n"); > + } else { > + priv->ideality_user_value[reg_nr - 1] = MCP9982_IDEALITY_FACTOR_DEFAULT; > + } > + > + ret = fwnode_property_read_string(child, "label", > + (const char **)&priv->labels[reg_nr]); Ditto. Why casting? > + priv->iio_chan[iio_idx++] = MCP9982_CHAN(reg_nr, reg_nr, > + MCP9982_INT_HIGH_BYTE_ADDR(reg_nr)); > + } > + > + return 0; > +} ... > + i2c_set_clientdata(client, indio_dev); How is this being used? ... > +static struct i2c_driver mcp9982_driver = { > + .driver = { > + .name = "mcp9982", > + .of_match_table = mcp9982_of_match, > + }, > + .probe = mcp9982_probe, > + .id_table = mcp9982_id, > +}; > + Redundant blank line. > +module_i2c_driver(mcp9982_driver); -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 2/2] iio: temperature: add support for MCP998X 2025-04-15 19:05 ` Andy Shevchenko @ 2025-05-22 9:18 ` Victor.Duicu 2025-05-23 16:21 ` Andy Shevchenko 0 siblings, 1 reply; 15+ messages in thread From: Victor.Duicu @ 2025-05-22 9:18 UTC (permalink / raw) To: andy.shevchenko Cc: Marius.Cristea, jic23, andy, dlechner, linux-iio, nuno.sa, linux-kernel On Tue, 2025-04-15 at 22:05 +0300, Andy Shevchenko wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > Hi Andy, > On Tue, Apr 15, 2025 at 4:27 PM <victor.duicu@microchip.com> wrote: > > > > This is the driver for Microchip MCP998X/33 and MCP998XD/33D > > Multichannel Automotive Monitor Family. > > ... > > > +#define MCP9982_CHAN(index, si, __address) ({ \ > > + struct iio_chan_spec __chan = { \ > > + .type = IIO_TEMP, \ > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > + .info_mask_shared_by_all_available = > > BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > > + .info_mask_shared_by_all = > > BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > > + .channel = index, \ > > + .address = __address, \ > > + .scan_index = si, \ > > + .scan_type = { \ > > + .sign = 'u', \ > > + .realbits = 8, \ > > + .storagebits = 8, \ > > + .endianness = IIO_CPU \ > > + }, \ > > + .indexed = 1, \ > > + }; \ > > + __chan; \ > > Why in this form and not as a compound literal? > I can have up to 5 channels, which have very similar specifications. I use this define to simplify definition of channels and avoid repeating code. Is it now preferable to use compound literal? I could implement something like this: #define put_channel_defaults \ .type = IIO_TEMP \ ... priv->iio_chan[0] = ((struct iio_chan_spec){put_channel_defaults, .channel = x, ... This way when initializing the channels I don't have to repeat the common properties. Do you find this approach agreeable? Kind regards, Duicu Victor ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 2/2] iio: temperature: add support for MCP998X 2025-05-22 9:18 ` Victor.Duicu @ 2025-05-23 16:21 ` Andy Shevchenko 0 siblings, 0 replies; 15+ messages in thread From: Andy Shevchenko @ 2025-05-23 16:21 UTC (permalink / raw) To: Victor.Duicu Cc: Marius.Cristea, jic23, dlechner, linux-iio, nuno.sa, linux-kernel On Thu, May 22, 2025 at 09:18:06AM +0000, Victor.Duicu@microchip.com wrote: > On Tue, 2025-04-15 at 22:05 +0300, Andy Shevchenko wrote: > > On Tue, Apr 15, 2025 at 4:27 PM <victor.duicu@microchip.com> wrote: ... > > > +#define MCP9982_CHAN(index, si, __address) ({ \ > > > + struct iio_chan_spec __chan = { \ > > > + .type = IIO_TEMP, \ > > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > > + .info_mask_shared_by_all_available = > > > BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > > > + .info_mask_shared_by_all = > > > BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > > > + .channel = index, \ > > > + .address = __address, \ > > > + .scan_index = si, \ > > > + .scan_type = { \ > > > + .sign = 'u', \ > > > + .realbits = 8, \ > > > + .storagebits = 8, \ > > > + .endianness = IIO_CPU \ > > > + }, \ > > > + .indexed = 1, \ > > > + }; \ > > > + __chan; \ > > > > Why in this form and not as a compound literal? > > I can have up to 5 channels, which have very similar specifications. > I use this define to simplify definition of channels and avoid > repeating code. > Is it now preferable to use compound literal? > I could implement something like this: > > #define put_channel_defaults \ > .type = IIO_TEMP \ > ... > > priv->iio_chan[0] = ((struct iio_chan_spec){put_channel_defaults, > .channel = x, > ... > > This way when initializing the channels I don't have > to repeat the common properties. > Do you find this approach agreeable? No, just find how the compound literal macros are written in the kernel, e.g., PINCTRL_PIN_FUNCTION(). -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 2/2] iio: temperature: add support for MCP998X 2025-04-15 13:26 ` [PATCH v1 2/2] " victor.duicu 2025-04-15 19:05 ` Andy Shevchenko @ 2025-04-18 18:07 ` Jonathan Cameron 2025-04-28 12:57 ` Victor.Duicu 1 sibling, 1 reply; 15+ messages in thread From: Jonathan Cameron @ 2025-04-18 18:07 UTC (permalink / raw) To: victor.duicu Cc: andy, dlechner, nuno.sa, marius.cristea, linux-iio, linux-kernel On Tue, 15 Apr 2025 16:26:23 +0300 <victor.duicu@microchip.com> wrote: > From: Victor Duicu <victor.duicu@microchip.com> > > This is the driver for Microchip MCP998X/33 and MCP998XD/33D > Multichannel Automotive Monitor Family. > > Signed-off-by: Victor Duicu <victor.duicu@microchip.com> Hi Victor, Various comments inline, Thanks, Jonathan > --- > .../testing/sysfs-bus-iio-temperature-mcp9982 | 17 + > MAINTAINERS | 7 + > drivers/iio/temperature/Kconfig | 10 + > drivers/iio/temperature/Makefile | 1 + > drivers/iio/temperature/mcp9982.c | 794 ++++++++++++++++++ > 5 files changed, 829 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982 > create mode 100644 drivers/iio/temperature/mcp9982.c > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982 b/Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982 > new file mode 100644 > index 000000000000..de3360fb05be > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982 > @@ -0,0 +1,17 @@ > +What: /sys/bus/iio/devices/iio:deviceX/running_average_window As later in review, I think we can control this via the low pass filter 3dB point and use standard ABI. > +KernelVersion: 6.14 > +Contact: linux-iio@vger.kernel.org > +Description: > + This attribute controls the number of samples for the > + running average window applied to External Channel 1. > + Using this method the temperature spikes are reduced. > + X is the IIO index of the device. > + > +What: /sys/bus/iio/devices/iio:deviceX/running_average_window_available > +KernelVersion: 6.14 > +Contact: linux-iio@vger.kernel.org > +Description: > + Reading returns a list with the possible number of samples used > + in the running average window. The window can be composed of 1, > + 4 or 8 previous samples. X is the IIO index of the device. > + > diff --git a/drivers/iio/temperature/mcp9982.c b/drivers/iio/temperature/mcp9982.c > new file mode 100644 > index 000000000000..f4edc7b6a437 > --- /dev/null > +++ b/drivers/iio/temperature/mcp9982.c > @@ -0,0 +1,794 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * IIO driver for MCP998X/33 and MCP998XD/33D Multichannel Automotive Temperature Monitor Family > + * > + * Copyright (C) 2025 Microchip Technology Inc. and its subsidiaries > + * > + * Author: Victor Duicu <victor.duicu@microchip.com> > + * > + * Datasheet can be found here: > + * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf > + * This line doesn't add anything. (one of my more trivial common comments :) > + */ > + > +/* Running average window can be set as: /* * Running average window... for IIO multiline comments. > + * - 1 sample > + * - 4 samples > + * - 8 samples > + */ > +#define MCP9982_RUNNING_AVERAGE_WINDOW_AVAILABLE "1 4 8" > + > +/* The maximum number of channels a member of the family can have */ > +#define MCP9982_MAX_NUM_CHANNELS 5 > + > +#define MCP9982_CHAN(index, si, __address) ({ \ > + struct iio_chan_spec __chan = { \ > + .type = IIO_TEMP, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > + .channel = index, \ > + .address = __address, \ > + .scan_index = si, \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = 8, \ > + .storagebits = 8, \ > + .endianness = IIO_CPU \ Odd concept for 1 byte ;) Anyhow it defaults to IIO_CPU anyway so I'd not set it explicitly. > + }, \ > + .indexed = 1, \ > + }; \ > + __chan; \ > +}) > + > +enum mcp9982_ids { This should go away after changed suggested later. > + MCP9933, > + MCP9933D, > + MCP9982, > + MCP9982D, > + MCP9983, > + MCP9983D, > + MCP9984, > + MCP9984D, > + MCP9985, > + MCP9985D, > +}; > + > +/* /** as it's kernel doc. > + * struct mcp9982_features - features of a mcp9982 instance > + * @phys_channels: number of physical channels supported by the chip > + * @name: chip's name > + */ > +struct mcp9982_features { > + u8 phys_channels; > + const char *name; > +}; > + > +static const struct mcp9982_features mcp9982_chip_config[] = { As later, use separate structures for these. > + [MCP9933] = { > + .name = "mcp9933", > + .phys_channels = 3, > + }, > + [MCP9933D] = { > + .name = "mcp9933D", > + .phys_channels = 3, > + }, > + [MCP9982] = { > + .name = "mcp9982", > + .phys_channels = 2, > + }, > + [MCP9982D] = { > + .name = "mcp9982D", > + .phys_channels = 2, > + }, > + [MCP9983] = { > + .name = "mcp9983", > + .phys_channels = 3, > + }, > + [MCP9983D] = { > + .name = "mcp9983D", > + .phys_channels = 3, > + }, > + [MCP9984] = { > + .name = "mcp9984", > + .phys_channels = 4, > + }, > + [MCP9984D] = { > + .name = "mcp9984D", > + .phys_channels = 4, > + }, > + [MCP9985] = { > + .name = "mcp9985", > + .phys_channels = 5, > + }, > + [MCP9985D] = { > + .name = "mcp9985D", > + .phys_channels = 5, > + }, > +}; > + > +static const int mcp9982_fractional_values[] = { > + 0, > + 125000, > + 250000, > + 375000, > + 500000, > + 625000, > + 750000, > + 875000, > +}; > + > +static const int mcp9982_conv_rate[][2] = { > + {0, 62500}, > + {0, 125000}, > + {0, 250000}, > + {0, 500000}, > + {1, 0}, > + {2, 0}, > + {4, 0}, > + {8, 0}, > + {16, 0}, > + {32, 0}, > + {64, 0}, > +}; { 64, 0 }, etc > + > +/** > + * struct mcp9992_priv - information about chip parameters > + * @client: the i2c-client attached to the device > + * @regmap: device register map > + * @iio_info iio_info > + * @iio_chan specifications of channels > + * @num_channels number of physical channels > + * @lock synchronize access to driver's state members > + * @running_avg number of samples in the running average window > + * @hysteresis value of temperature hysteresis > + * @temp_range_code coded value representing the set temperature range > + * @labels labels of the channels > + * @chip_name name of the chip present > + * @beta_user_value value given by the user for beta on channel 1 and 2 > + * @apdd state of anti-parallel diode mode > + * @recd12 state of REC on channels 1 and 2 > + * @recd34 state of REC on channels 3 and 4 > + * @ideality_user_value values given by user to ideality factor for all channels > + */ > + No blank line here. > +struct mcp9982_priv { > + struct i2c_client *client; Why do we need this after probe? Either get the dev from regmap or store that directly rather than the i2c_client. > + struct regmap *regmap; > + struct iio_info iio_info; As below. I'm not sure why we need this. > + > + struct iio_chan_spec *iio_chan; > + u8 num_channels; > + > + /* > + * Synchronize access to private members, and ensure > + * atomicity of consecutive regmap operations. > + */ > + struct mutex lock; > + > + int running_avg; > + int hysteresis; > + int temp_range_code; > + char *labels[MCP9982_MAX_NUM_CHANNELS]; > + char *chip_name; > + int beta_user_value[2]; > + int apdd; > + int recd12; > + int recd34; > + int ideality_user_value[4]; > +}; > +} > + > +static int mcp9982_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct mcp9982_priv *priv = iio_priv(indio_dev); > + int ret, val3, HIGH_BYTE, LOW_BYTE; Capitals not compliant with kernel coding style. > + > + /* Write in ONESHOT register to take a new reading */ Drop and comments that are fairly obvious. Maybe that 1 needs a define though. > + ret = regmap_write(priv->regmap, MCP9982_ONE_SHOT_ADDR, 1); > + if (ret) > + return ret; > + > + guard(mutex)(&priv->lock); > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + HIGH_BYTE = MCP9982_INT_HIGH_BYTE_ADDR(chan->channel); > + LOW_BYTE = MCP9982_INT_LOW_BYTE_ADDR(chan->channel); Drop the local variables and put the macros inline. > + > + ret = regmap_read(priv->regmap, HIGH_BYTE, val); > + if (ret) > + return ret; > + > + if (priv->temp_range_code) > + *val -= MCP9982_TEMP_OFFSET; > + > + ret = regmap_read(priv->regmap, LOW_BYTE, val2); > + if (ret) > + return ret; > + > + *val2 = mcp9982_fractional_values[*val2 >> 5]; Why 5? Maybe needs a define or at minimum a comment. > + > + return IIO_VAL_INT_PLUS_MICRO; > + case IIO_CHAN_INFO_SAMP_FREQ: > + ret = regmap_read(priv->regmap, MCP9982_CONV_ADDR, &val3); why val3? > + if (ret) > + return ret; > + > + *val = mcp9982_conv_rate[val3][0]; > + *val2 = mcp9982_conv_rate[val3][1]; > + > + return IIO_VAL_INT_PLUS_MICRO; > + default: > + return -EINVAL; > + } > +} > + > +static int mcp9982_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + struct mcp9982_priv *priv = iio_priv(indio_dev); > + struct device *dev = &priv->client->dev; > + int i; > + int status = 0; > + > + guard(mutex)(&priv->lock); > + switch (mask) { > + case IIO_CHAN_INFO_SAMP_FREQ: > + for (i = 0; i < ARRAY_SIZE(mcp9982_conv_rate); i++) { > + if (val == mcp9982_conv_rate[i][0] && > + val2 == mcp9982_conv_rate[i][1]){ > + status = 1; > + break; > + } > + } > + if (!status) if (i == ARRAY_SIZE(mcp9982_conv_rate); and get rid of status. Simply breaking early is sufficient. > + return dev_err_probe(dev, -EINVAL, "Sampling Frequency is invalid\n"); > + > + return regmap_write(priv->regmap, MCP9982_CONV_ADDR, i); > + default: > + return -EINVAL; > + } > +} > + > +static ssize_t mcp9982_running_average_window_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct mcp9982_priv *priv = iio_priv(indio_dev); > + > + return sprintf(buf, "%u sample(s)\n", priv->running_avg); Should read and write in same format. So no units - for that people have to go look at the ABI docs. > +} > + > +static ssize_t mcp9982_running_average_window_avail_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + return sprintf(buf, "%s\n", MCP9982_RUNNING_AVERAGE_WINDOW_AVAILABLE); > +} > + > +static ssize_t mcp9982_running_average_window_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct mcp9982_priv *priv = iio_priv(indio_dev); > + int val, ret, reg_val; > + > + if (kstrtouint(buf, 10, &val)) { > + dev_err(dev, "Value is not a number\n"); Just return -EINVAL. As a general rule - error message that can be triggered directly by userspace based on it's input are bad because they can be used to flush out a log in an attack on a system. > + return -EINVAL; > + } > + > + switch (val) { > + case 1: > + reg_val = 0; > + break; > + case 4: > + reg_val = 1; > + break; > + case 8: > + reg_val = 3; > + break; > + default: > + dev_err(dev, "Value is invalid\n"); no need for an error message here. Returning -EINVAL should be enough > + return -EINVAL; > + } > + > + guard(mutex)(&priv->lock); > + > + ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, reg_val); > + priv->running_avg = val; > + > + return count; > +} > + > +static IIO_DEVICE_ATTR(running_average_window, 0644, mcp9982_running_average_window_show, > + mcp9982_running_average_window_store, 0); Hmm. A running average is a low pass filter. Can we control this instead via standard ABI and the 3dB point? Take a look at the filter ABI in Documentation/ABI/testing/sysfs-bus-iio Custom ABI is rarely used in real cases because the tools tend not to know about it so we avoid it if we possibly can. > + > +static IIO_DEVICE_ATTR(running_average_window_available, 0444, > + mcp9982_running_average_window_avail_show, NULL, 1); > + > +static int mcp9982_prep_custom_attributes(struct mcp9982_priv *priv, struct iio_dev *indio_dev) > +{ > + struct attribute **mcp9982_custom_attr; > + struct attribute_group *mcp9982_group; > + struct device *dev = &priv->client->dev; > + > + mcp9982_group = devm_kzalloc(dev, sizeof(*mcp9982_group), GFP_KERNEL); > + if (!mcp9982_group) > + return -ENOMEM; > + > + mcp9982_custom_attr = devm_kzalloc(dev, MCP9982_NR_CUSTOM_ATTR * > + sizeof(*mcp9982_group) + 1, GFP_KERNEL); > + if (!mcp9982_custom_attr) > + return -ENOMEM; > + > + mcp9982_custom_attr[0] = MCP9982_DEV_ATTR(running_average_window); > + mcp9982_custom_attr[1] = MCP9982_DEV_ATTR(running_average_window_available); These seem fixed, why the allocation rather than a static const array etc? > + > + mcp9982_group->attrs = mcp9982_custom_attr; > + priv->iio_info.attrs = mcp9982_group; Isn't priv->iio_info pointing to something static const? You'll need to duplicate it if you want to modify like this. > + > + return 0; > +} > + > +static const struct iio_info mcp9982_info = { > + .read_raw = mcp9982_read_raw, > + .read_label = mcp9982_read_label, > + .read_avail = mcp9982_read_avail, > + .write_raw_get_fmt = mcp9982_write_raw_get_fmt, > + .write_raw = mcp9982_write_raw, > +}; > + > +static int mcp9982_init(struct mcp9982_priv *priv) > +{ > + int ret, i; > + u8 val; > + > + val = FIELD_PREP(MCP9982_CFG_MSKAL, MCP9982_CFG_MSKAL_DEFAULT) | > + FIELD_PREP(MCP9982_CFG_RS, MCP9982_CFG_RS_DEFAULT) | > + FIELD_PREP(MCP9982_CFG_ATTHM, MCP9982_CFG_ATTHM_DEFAULT) | > + FIELD_PREP(MCP9982_CFG_RECD12, priv->recd12) | > + FIELD_PREP(MCP9982_CFG_RECD34, priv->recd34) | > + FIELD_PREP(MCP9982_CFG_RANGE, priv->temp_range_code) | > + FIELD_PREP(MCP9982_CFG_DA_ENA, MCP9982_CFG_DA_ENA_DEFAULT) | > + FIELD_PREP(MCP9982_CFG_APDD, priv->apdd); > + ret = regmap_write(priv->regmap, MCP9982_CFG_ADDR, val); > + if (ret) > + return ret; > + > + ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, MCP9982_SAMPLING_FREQ_CODE_DEFAULT); > + if (ret) > + return ret; > + > + ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, priv->hysteresis); > + if (ret) > + return ret; > + > + ret = regmap_write(priv->regmap, MCP9982_CONSEC_ALRT_ADDR, MCP9982_CONSEC_ALRT_DEFAULT); > + if (ret) > + return ret; > + > + /* RUNNING AVG DEFAULT is 0 */ I don't think the comment adds anything that isn't obvious from the code. > + ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, 0); > + if (ret) > + return ret; > + priv->running_avg = 1; > + > + /* HOTTEST CFG DEFAULT is 0 */ same here > + ret = regmap_write(priv->regmap, MCP9982_HOTTEST_CFG_ADDR, 0); > + if (ret) > + return ret; > + > + /* Set beta compensation for all channels */ and here > + for (i = 0; i < 2; i++) { Why 2? Can we get that from size of something? > + ret = regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(i), > + priv->beta_user_value[i]); > + if (ret) > + return ret; > + } > + > + /* Set ideality factor for all channels */ > + for (i = 0; i < 4; i++) { Why 4? Can we get that from size of something? > + ret = regmap_write(priv->regmap, MCP9982_EXT_IDEAL_ADDR(i), > + priv->ideality_user_value[i]); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +static int mcp9982_parse_of_fw(struct iio_dev *indio_dev) > +{ > + struct mcp9982_priv *priv = iio_priv(indio_dev); > + struct device *dev = &priv->client->dev; > + int ret, reg_nr, iio_idx, i; > + > + if (device_property_present(dev, "microchip,temp-hysteresis")) { > + ret = device_property_read_u32(dev, "microchip,temp-hysteresis", &priv->hysteresis); > + if (ret) > + return dev_err_probe(dev, ret, "Cannot read hysteresis property\n"); > + > + if (priv->hysteresis > MCP9982_HYSTERESIS_MAX_VALUE) > + return dev_err_probe(dev, ret, > + "Hysteresis value is higher than maximum\n"); > + } else { > + priv->hysteresis = MCP9982_HYSTERESIS_DEFAULT; > + } Usual trick for these is to assume any error means fallback to default. Allowing simpler option of: priv->hysteresis = MCP9... device_property_read_u32(dev, "microchip,....); //don't check error. if (priv->hysteresis > MCP_...) return dev_err_probe(); Which is fine if pointless on the default value as well. The slight loss is this doesn't catch missformatted properties. Advantage is simpler and shorter code. > + > + if (device_property_present(dev, "microchip,extended-temp-range")) Bool rather than present check. > + priv->temp_range_code = 1; > + else > + priv->temp_range_code = MCP9982_TEMP_RANGE_DEFAULT; > + > + if (device_property_present(dev, "microchip,beta-channel1")) { > + ret = device_property_read_u32(dev, "microchip,beta-channel1", > + &priv->beta_user_value[0]); > + if (ret) > + return ret; > + > + if (priv->beta_user_value[0] > MCP9982_BETA_MAX_VALUE) > + return dev_err_probe(dev, -EINVAL, "Beta 1 value is higher than max\n"); > + } else { > + priv->beta_user_value[0] = MCP9982_BETA_AUTODETECT_ENABLE; As above - can set default and not check error on the property read to give simpler code. > + } > + > + if (device_property_present(dev, "microchip,beta-channel2")) { > + ret = device_property_read_u32(dev, "microchip,beta-channel2", > + &priv->beta_user_value[1]); > + if (ret) > + return ret; > + > + if (priv->beta_user_value[1] > MCP9982_BETA_MAX_VALUE) > + return dev_err_probe(dev, -EINVAL, "Beta 2 value is higher than max\n"); > + } else { > + priv->beta_user_value[1] = MCP9982_BETA_AUTODETECT_ENABLE; > + } > + > + if (device_property_present(dev, "microchip,apdd-state")) > + priv->apdd = 0; > + else > + priv->apdd = MCP9982_APDD_DEFAULT; > + > + if (device_property_present(dev, "microchip,recd12")) > + priv->recd12 = 0; > + else > + priv->recd12 = MCP9982_RECD_DEFAULT; > + > + if (device_property_present(dev, "microchip,recd34")) > + priv->recd34 = 0; > + else > + priv->recd34 = MCP9982_RECD_DEFAULT; > + > + ret = device_property_read_string(dev, "compatible", (const char **)&priv->chip_name); This is doing a bad patch to the stuff that we do by getting the match data directly from the tables provided i2c_get_match_data() > + if (ret) > + return dev_err_probe(dev, ret, "Cannot read compatible property\n"); > + > + priv->chip_name = &priv->chip_name[10]; Why 10? > + priv->num_channels = device_get_child_node_count(dev) + 1; > + > + for (i = 0; i < 10; i++) > + if (strcmp(priv->chip_name, mcp9982_chip_config[i].name) == 0) Why the search, just get it directly from what the firmware provided? > + break; > + if (priv->num_channels > mcp9982_chip_config[i].phys_channels) > + return dev_err_probe(dev, -EINVAL, > + "The number of channels does not match the chip\n"); > + > + priv->iio_chan = devm_kzalloc(dev, priv->num_channels * sizeof(*priv->iio_chan), > + GFP_KERNEL); > + if (!priv->iio_chan) > + return -ENOMEM; > + > + /* first channel is internal and always present */ > + priv->iio_chan[0] = MCP9982_CHAN(MCP9982_CHAN_INT, MCP9982_CHAN_INT, > + MCP9982_INT_HIGH_BYTE_ADDR(0)); > + priv->labels[0] = "internal diode"; > + iio_idx++; > + device_for_each_child_node_scoped(dev, child) { > + ret = fwnode_property_read_u32(child, "reg", ®_nr); > + if (reg_nr >= mcp9982_chip_config[i].phys_channels) > + return dev_err_probe(dev, -EINVAL, > + "The index of the channels does not match the chip\n"); > + > + if (fwnode_property_present(child, "microchip,ideality-factor")) { > + ret = fwnode_property_read_u32(child, "microchip,ideality-factor", > + &priv->ideality_user_value[reg_nr - 1]); > + if (priv->ideality_user_value[reg_nr - 1] > MCP9982_IDEALITY_MAX_VALUE) > + return dev_err_probe(dev, -EINVAL, > + "The ideality value is higher than maximum\n"); > + } else { > + priv->ideality_user_value[reg_nr - 1] = MCP9982_IDEALITY_FACTOR_DEFAULT; > + } > + > + ret = fwnode_property_read_string(child, "label", > + (const char **)&priv->labels[reg_nr]); > + > + priv->iio_chan[iio_idx++] = MCP9982_CHAN(reg_nr, reg_nr, > + MCP9982_INT_HIGH_BYTE_ADDR(reg_nr)); > + } > + > + return 0; > +} > + > +static int mcp9982_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct mcp9982_priv *priv; > + struct iio_dev *indio_dev; > + int ret; > + > + indio_dev = devm_iio_device_alloc(dev, sizeof(*priv)); > + if (!indio_dev) > + return -ENOMEM; > + > + priv = iio_priv(indio_dev); > + priv->client = client; > + i2c_set_clientdata(client, indio_dev); Used? > + > + priv->regmap = devm_regmap_init_i2c(client, &mcp9982_regmap_config); > + if (IS_ERR(priv->regmap)) > + return dev_err_probe(dev, PTR_ERR(priv->regmap), > + "Cannot initialize register map\n"); > + > + ret = devm_mutex_init(dev, &priv->lock); > + if (ret) > + return ret; > + > + ret = mcp9982_parse_of_fw(indio_dev); > + if (ret) > + return dev_err_probe(dev, ret, "Parameter parsing error\n"); > + > + ret = mcp9982_init(priv); > + if (ret) > + return dev_err_probe(dev, ret, "Cannot initialize device\n"); > + > + priv->iio_info = mcp9982_info; Why do you need this copy? > + > + indio_dev->name = priv->chip_name; > + indio_dev->info = &priv->iio_info; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = priv->iio_chan; > + indio_dev->num_channels = priv->num_channels; > + > + ret = mcp9982_prep_custom_attributes(priv, indio_dev); > + if (ret < 0) > + return dev_err_probe(&client->dev, ret, > + "Can't configure custom attributes for MCP9982 device\n"); > + > + ret = devm_iio_device_register(dev, indio_dev); > + if (ret) > + return dev_err_probe(dev, ret, "Cannot register IIO device\n"); > + > + return 0; > +} > + > +static const struct i2c_device_id mcp9982_id[] = { > + { .name = "mcp9933", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9933] }, > + { .name = "mcp9933D", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9933D] }, > + { .name = "mcp9982", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9982] }, > + { .name = "mcp9982D", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9982D] }, > + { .name = "mcp9983", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9983] }, > + { .name = "mcp9983D", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9983D] }, > + { .name = "mcp9984", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9984] }, > + { .name = "mcp9984D", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9984D] }, > + { .name = "mcp9985", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9985] }, > + { .name = "mcp9985D", .driver_data = (kernel_ulong_t)&mcp9982_chip_config[MCP9985D] }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, mcp9982_id); > + > +static const struct of_device_id mcp9982_of_match[] = { > + { > + .compatible = "microchip,mcp9933", > + .data = &mcp9982_chip_config[MCP9933] For these, named structures now preferred over an array. It tends to end up ever so slightly simpler and also gets rid of need for the enum. Also you set .data but I'm not spotting where you read it. > + }, > + { kernel style still let you do }, { and save a bunch of lines. Jonathan ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 2/2] iio: temperature: add support for MCP998X 2025-04-18 18:07 ` Jonathan Cameron @ 2025-04-28 12:57 ` Victor.Duicu 2025-04-30 16:17 ` Jonathan Cameron 0 siblings, 1 reply; 15+ messages in thread From: Victor.Duicu @ 2025-04-28 12:57 UTC (permalink / raw) To: jic23; +Cc: Marius.Cristea, andy, dlechner, linux-iio, nuno.sa, linux-kernel On Fri, 2025-04-18 at 19:07 +0100, Jonathan Cameron wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Tue, 15 Apr 2025 16:26:23 +0300 > <victor.duicu@microchip.com> wrote: > > > From: Victor Duicu <victor.duicu@microchip.com> > > > > This is the driver for Microchip MCP998X/33 and MCP998XD/33D > > Multichannel Automotive Monitor Family. > > > > Signed-off-by: Victor Duicu <victor.duicu@microchip.com> > Hi Jonathan, > Hi Victor, > > Various comments inline, > > Thanks, > > Jonathan > > > --- > > .../testing/sysfs-bus-iio-temperature-mcp9982 | 17 + > > MAINTAINERS | 7 + > > drivers/iio/temperature/Kconfig | 10 + > > drivers/iio/temperature/Makefile | 1 + > > drivers/iio/temperature/mcp9982.c | 794 > > ++++++++++++++++++ > > 5 files changed, 829 insertions(+) > > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio- > > temperature-mcp9982 > > create mode 100644 drivers/iio/temperature/mcp9982.c > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-temperature- > > mcp9982 b/Documentation/ABI/testing/sysfs-bus-iio-temperature- > > mcp9982 > > new file mode 100644 > > index 000000000000..de3360fb05be > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982 > > @@ -0,0 +1,17 @@ > > +What: > > /sys/bus/iio/devices/iio:deviceX/running_average_window > > As later in review, I think we can control this via the low pass > filter 3dB point > and use standard ABI. > > > > ... > Hmm. A running average is a low pass filter. Can we control this > instead via > standard ABI and the 3dB point? Take a look at the filter ABI in > Documentation/ABI/testing/sysfs-bus-iio > > Custom ABI is rarely used in real cases because the tools tend not to > know about it > so we avoid it if we possibly can. > > > + > > The moving average filter is used to smooth the temperature spikes. The user should be able to set the size of the window to a few values: 1(disable the filter), 4 and 8. The user does not have access to the frequency properties. Best Regards, Victor > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 2/2] iio: temperature: add support for MCP998X 2025-04-28 12:57 ` Victor.Duicu @ 2025-04-30 16:17 ` Jonathan Cameron 0 siblings, 0 replies; 15+ messages in thread From: Jonathan Cameron @ 2025-04-30 16:17 UTC (permalink / raw) To: Victor.Duicu Cc: jic23, Marius.Cristea, andy, dlechner, linux-iio, nuno.sa, linux-kernel On Mon, 28 Apr 2025 12:57:05 +0000 <Victor.Duicu@microchip.com> wrote: > On Fri, 2025-04-18 at 19:07 +0100, Jonathan Cameron wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > know the content is safe > > > > On Tue, 15 Apr 2025 16:26:23 +0300 > > <victor.duicu@microchip.com> wrote: > > > > > From: Victor Duicu <victor.duicu@microchip.com> > > > > > > This is the driver for Microchip MCP998X/33 and MCP998XD/33D > > > Multichannel Automotive Monitor Family. > > > > > > Signed-off-by: Victor Duicu <victor.duicu@microchip.com> > > > > Hi Jonathan, > > > Hi Victor, > > > > Various comments inline, > > > > Thanks, > > > > Jonathan > > > > > --- > > > .../testing/sysfs-bus-iio-temperature-mcp9982 | 17 + > > > MAINTAINERS | 7 + > > > drivers/iio/temperature/Kconfig | 10 + > > > drivers/iio/temperature/Makefile | 1 + > > > drivers/iio/temperature/mcp9982.c | 794 > > > ++++++++++++++++++ > > > 5 files changed, 829 insertions(+) > > > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio- > > > temperature-mcp9982 > > > create mode 100644 drivers/iio/temperature/mcp9982.c > > > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-temperature- > > > mcp9982 b/Documentation/ABI/testing/sysfs-bus-iio-temperature- > > > mcp9982 > > > new file mode 100644 > > > index 000000000000..de3360fb05be > > > --- /dev/null > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982 > > > @@ -0,0 +1,17 @@ > > > +What: > > > /sys/bus/iio/devices/iio:deviceX/running_average_window > > > > As later in review, I think we can control this via the low pass > > filter 3dB point > > and use standard ABI. > > > > > > > > ... > > > Hmm. A running average is a low pass filter. Can we control this > > instead via > > standard ABI and the 3dB point? Take a look at the filter ABI in > > Documentation/ABI/testing/sysfs-bus-iio > > > > Custom ABI is rarely used in real cases because the tools tend not to > > know about it > > so we avoid it if we possibly can. > > > > > + > > > > > > The moving average filter is used to smooth the temperature spikes. > The user should be able to set the size of the window to > a few values: 1(disable the filter), 4 and 8. > The user does not have access to the frequency properties. Assuming the device is self clocking, then we know the frequency and hence can consider an averaging filter as a type of low pass filter (which is what it effectively is) and control via the 3dB point. Whether it is documented that way is just a question of how they decided to describe it in the datasheet. A moving average is also known as a box car filter. https://www.wavewalkerdsp.com/2022/08/03/bandwidth-of-a-moving-average-filter/ Approximately (pi / N)*sampling frequency (I think anyway, I only took a quick look). Doing that allows you to map it to standard ABI. > > Best Regards, > Victor > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 0/2] add support for MCP998X 2025-04-15 13:26 [PATCH v1 0/2] add support for MCP998X victor.duicu 2025-04-15 13:26 ` [PATCH v1 1/2] dt-bindings: iio: temperature: " victor.duicu 2025-04-15 13:26 ` [PATCH v1 2/2] " victor.duicu @ 2025-04-15 17:43 ` Jonathan Cameron 2025-04-17 7:30 ` Victor.Duicu 2 siblings, 1 reply; 15+ messages in thread From: Jonathan Cameron @ 2025-04-15 17:43 UTC (permalink / raw) To: victor.duicu Cc: andy, dlechner, nuno.sa, marius.cristea, linux-iio, linux-kernel On Tue, 15 Apr 2025 16:26:21 +0300 <victor.duicu@microchip.com> wrote: > From: Victor Duicu <victor.duicu@microchip.com> > > Add support for Microchip MCP998X/33 and MCP998XD/33D > Multichannel Automotive Temperature Monitor Family. > > The chip is capable of monitoring temperatures on four > external channels and one internal. Hi Victor, This gets the standard question for a temperature sensor... Why IIO and not hwmon? Good to have that info in the cover letter or patch description. I'd normally moan about the wild cards even though they are only in patch titles, but meh, the datasheet uses the wild card so we are probably safe for a while at least! Jonathan > > Victor Duicu (2): > dt-bindings: iio: temperature: add support for MCP998X > iio: temperature: add support for MCP998X > > .../testing/sysfs-bus-iio-temperature-mcp9982 | 17 + > .../iio/temperature/microchip,mcp9982.yaml | 182 ++++ > MAINTAINERS | 7 + > drivers/iio/temperature/Kconfig | 10 + > drivers/iio/temperature/Makefile | 1 + > drivers/iio/temperature/mcp9982.c | 794 ++++++++++++++++++ > 6 files changed, 1011 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982 > create mode 100644 Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml > create mode 100644 drivers/iio/temperature/mcp9982.c > > > base-commit: 31c52fe3b2efeebfc72cc5336653baaa9889b41e ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 0/2] add support for MCP998X 2025-04-15 17:43 ` [PATCH v1 0/2] " Jonathan Cameron @ 2025-04-17 7:30 ` Victor.Duicu 0 siblings, 0 replies; 15+ messages in thread From: Victor.Duicu @ 2025-04-17 7:30 UTC (permalink / raw) To: jic23; +Cc: Marius.Cristea, andy, dlechner, linux-iio, nuno.sa, linux-kernel On Tue, 2025-04-15 at 18:43 +0100, Jonathan Cameron wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Tue, 15 Apr 2025 16:26:21 +0300 > <victor.duicu@microchip.com> wrote: > > > From: Victor Duicu <victor.duicu@microchip.com> > > > > Add support for Microchip MCP998X/33 and MCP998XD/33D > > Multichannel Automotive Temperature Monitor Family. > > > > The chip is capable of monitoring temperatures on four > > external channels and one internal. > Hi Victor, > Hi Jonathan, > This gets the standard question for a temperature sensor... > Why IIO and not hwmon? Good to have that info in the cover letter or > patch description. > This particular version of the driver is not yet feature complete. I intend to add some functionality later, such as interrupts and data buffering. > I'd normally moan about the wild cards even though they are only > in patch titles, but meh, the datasheet uses the wild card > so we are probably safe for a while at least! > > Jonathan > > > > > > Victor Duicu (2): > > dt-bindings: iio: temperature: add support for MCP998X > > iio: temperature: add support for MCP998X > > > > .../testing/sysfs-bus-iio-temperature-mcp9982 | 17 + > > .../iio/temperature/microchip,mcp9982.yaml | 182 ++++ > > MAINTAINERS | 7 + > > drivers/iio/temperature/Kconfig | 10 + > > drivers/iio/temperature/Makefile | 1 + > > drivers/iio/temperature/mcp9982.c | 794 > > ++++++++++++++++++ > > 6 files changed, 1011 insertions(+) > > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio- > > temperature-mcp9982 > > create mode 100644 > > Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982 > > .yaml > > create mode 100644 drivers/iio/temperature/mcp9982.c > > > > > > base-commit: 31c52fe3b2efeebfc72cc5336653baaa9889b41e > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2025-05-23 16:21 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-04-15 13:26 [PATCH v1 0/2] add support for MCP998X victor.duicu 2025-04-15 13:26 ` [PATCH v1 1/2] dt-bindings: iio: temperature: " victor.duicu 2025-04-15 17:52 ` Jonathan Cameron 2025-04-17 11:40 ` Victor.Duicu 2025-04-18 16:01 ` Jonathan Cameron 2025-04-16 6:45 ` Krzysztof Kozlowski 2025-04-15 13:26 ` [PATCH v1 2/2] " victor.duicu 2025-04-15 19:05 ` Andy Shevchenko 2025-05-22 9:18 ` Victor.Duicu 2025-05-23 16:21 ` Andy Shevchenko 2025-04-18 18:07 ` Jonathan Cameron 2025-04-28 12:57 ` Victor.Duicu 2025-04-30 16:17 ` Jonathan Cameron 2025-04-15 17:43 ` [PATCH v1 0/2] " Jonathan Cameron 2025-04-17 7:30 ` Victor.Duicu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).