[PATCH v2 0/4] Add TI TMP116 Support

[PATCH v2 0/4] Add TI TMP116 Support

[Marco Felsch]

Hi,

this small series adds the support for the TI TMP116 temperature sensor
which is predecessor of the TMP117 but still in production.

Regards,
  Marco

Marco Felsch (4):
  dt-bindings: iio: ti,tmp117: fix documentation link
  dt-bindings: iio: ti,tmp117: add binding for the TMP116
  iio: temperature: tmp117: add TI TMP116 support
  iio: temperature: tmp117: cosmetic alignment cleanup

 .../bindings/iio/temperature/ti,tmp117.yaml   | 16 ++++--
 drivers/iio/temperature/tmp117.c              | 57 ++++++++++++-------
 2 files changed, 48 insertions(+), 25 deletions(-)

-- 
2.30.2

[PATCH v2 1/4] dt-bindings: iio: ti,tmp117: fix documentation link

[Marco Felsch]

Fix the broken link to point to the correct homepage.

Fixes: 5e713b25d137 ("dt-bindings: iio: temperature: Add DT bindings for TMP117")
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
v2:
- added Krzysztof ack

 .../devicetree/bindings/iio/temperature/ti,tmp117.yaml          | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
index 347bc16a4671..8d1ec4d39b28 100644
--- a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
+++ b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
@@ -9,7 +9,7 @@ title: "TI TMP117 - Digital temperature sensor with integrated NV memory"
 description: |
     TI TMP117 - Digital temperature sensor with integrated NV memory that supports
     I2C interface.
-      https://www.ti.com/lit/gpn/tmp1
+      https://www.ti.com/lit/gpn/tmp117
 
 maintainers:
   - Puranjay Mohan <puranjay12@gmail.com>
-- 
2.30.2

[PATCH v2 2/4] dt-bindings: iio: ti,tmp117: add binding for the TMP116

[Marco Felsch]

The TMP116 is the predecessor of the TMP117.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- drop items from single enum

 .../bindings/iio/temperature/ti,tmp117.yaml        | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
index 8d1ec4d39b28..9b78357d6a79 100644
--- a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
+++ b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
@@ -7,8 +7,9 @@ $schema: "http://devicetree.org/meta-schemas/core.yaml#"
 title: "TI TMP117 - Digital temperature sensor with integrated NV memory"
 
 description: |
-    TI TMP117 - Digital temperature sensor with integrated NV memory that supports
-    I2C interface.
+    TI TMP116/117 - Digital temperature sensor with integrated NV memory that
+    supports I2C interface.
+      https://www.ti.com/lit/gpn/tmp116
       https://www.ti.com/lit/gpn/tmp117
 
 maintainers:
@@ -16,8 +17,13 @@ maintainers:
 
 properties:
   compatible:
-    enum:
-      - ti,tmp117
+    oneOf:
+      - enum:
+          - ti,tmp117
+      - items:
+          - enum:
+              - ti,tmp116
+          - const: ti,tmp117
 
   reg:
     maxItems: 1
-- 
2.30.2

Re: [PATCH v2 2/4] dt-bindings: iio: ti,tmp117: add binding for the TMP116

[Krzysztof Kozlowski]

On 21/12/2022 10:27, Marco Felsch wrote:
> The TMP116 is the predecessor of the TMP117.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> v2:
> - drop items from single enum


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 2/4] dt-bindings: iio: ti,tmp117: add binding for the TMP116

[Jonathan Cameron]

On Wed, 21 Dec 2022 10:27:59 +0100
Marco Felsch <m.felsch@pengutronix.de> wrote:

> The TMP116 is the predecessor of the TMP117.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
I'm not sure this is introducing a valid fallback. The driver changes
imply some things the tmp117 driver supports, that this device
does not. A fallback compatible would mean that a new DT
with an old kernel would load the tmp117 against a tmp116 and
expect it to fully work.

An example is calibbias which you've dropped from the channels
array entry.

Jonathan


> ---
> v2:
> - drop items from single enum
> 
>  .../bindings/iio/temperature/ti,tmp117.yaml        | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
> index 8d1ec4d39b28..9b78357d6a79 100644
> --- a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
> +++ b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
> @@ -7,8 +7,9 @@ $schema: "http://devicetree.org/meta-schemas/core.yaml#"
>  title: "TI TMP117 - Digital temperature sensor with integrated NV memory"
>  
>  description: |
> -    TI TMP117 - Digital temperature sensor with integrated NV memory that supports
> -    I2C interface.
> +    TI TMP116/117 - Digital temperature sensor with integrated NV memory that
> +    supports I2C interface.
> +      https://www.ti.com/lit/gpn/tmp116
>        https://www.ti.com/lit/gpn/tmp117
>  
>  maintainers:
> @@ -16,8 +17,13 @@ maintainers:
>  
>  properties:
>    compatible:
> -    enum:
> -      - ti,tmp117
> +    oneOf:
> +      - enum:
> +          - ti,tmp117
> +      - items:
> +          - enum:
> +              - ti,tmp116
> +          - const: ti,tmp117
>  
>    reg:
>      maxItems: 1

Re: [PATCH v2 2/4] dt-bindings: iio: ti,tmp117: add binding for the TMP116

[Marco Felsch]

On 22-12-23, Jonathan Cameron wrote:
> On Wed, 21 Dec 2022 10:27:59 +0100
> Marco Felsch <m.felsch@pengutronix.de> wrote:
> 
> > The TMP116 is the predecessor of the TMP117.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> I'm not sure this is introducing a valid fallback. The driver changes
> imply some things the tmp117 driver supports, that this device
> does not. A fallback compatible would mean that a new DT
> with an old kernel would load the tmp117 against a tmp116 and
> expect it to fully work.

Since driver does all the detection an update of the bindings isn't
really necessary. It is just to have a compatible already in place in
case there a things we can't detected during runtime. This flow is
common for a lot of SoC drivers. The fallback will be used as long as
possible and once a specific feature can't be detected only via the
binding, the driver adds the new binding to it of_compatible.

Regards,
  Marco

> An example is calibbias which you've dropped from the channels
> array entry.
> 
> Jonathan
> 
> 
> > ---
> > v2:
> > - drop items from single enum
> > 
> >  .../bindings/iio/temperature/ti,tmp117.yaml        | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
> > index 8d1ec4d39b28..9b78357d6a79 100644
> > --- a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
> > +++ b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
> > @@ -7,8 +7,9 @@ $schema: "http://devicetree.org/meta-schemas/core.yaml#"
> >  title: "TI TMP117 - Digital temperature sensor with integrated NV memory"
> >  
> >  description: |
> > -    TI TMP117 - Digital temperature sensor with integrated NV memory that supports
> > -    I2C interface.
> > +    TI TMP116/117 - Digital temperature sensor with integrated NV memory that
> > +    supports I2C interface.
> > +      https://www.ti.com/lit/gpn/tmp116
> >        https://www.ti.com/lit/gpn/tmp117
> >  
> >  maintainers:
> > @@ -16,8 +17,13 @@ maintainers:
> >  
> >  properties:
> >    compatible:
> > -    enum:
> > -      - ti,tmp117
> > +    oneOf:
> > +      - enum:
> > +          - ti,tmp117
> > +      - items:
> > +          - enum:
> > +              - ti,tmp116
> > +          - const: ti,tmp117
> >  
> >    reg:
> >      maxItems: 1
> 
> 

Re: [PATCH v2 2/4] dt-bindings: iio: ti,tmp117: add binding for the TMP116

[Jonathan Cameron]

On Fri, 23 Dec 2022 16:03:38 +0100
Marco Felsch <m.felsch@pengutronix.de> wrote:

> On 22-12-23, Jonathan Cameron wrote:
> > On Wed, 21 Dec 2022 10:27:59 +0100
> > Marco Felsch <m.felsch@pengutronix.de> wrote:
> >   
> > > The TMP116 is the predecessor of the TMP117.
> > > 
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>  
> > I'm not sure this is introducing a valid fallback. The driver changes
> > imply some things the tmp117 driver supports, that this device
> > does not. A fallback compatible would mean that a new DT
> > with an old kernel would load the tmp117 against a tmp116 and
> > expect it to fully work.  
> 
> Since driver does all the detection an update of the bindings isn't
> really necessary. It is just to have a compatible already in place in
> case there a things we can't detected during runtime. This flow is
> common for a lot of SoC drivers. The fallback will be used as long as
> possible and once a specific feature can't be detected only via the
> binding, the driver adds the new binding to it of_compatible.

That's true going forwards and for drivers that introduce a shared
generic compatible alongside the initial binding. It can't be easily
retrofit.

Fallback compatible is also to allow this to work with old kernels
- which it doesn't because the kernel driver
a) rejects non tmp117 ids (we should fix that by just warning instead 
if the ID isn't what we expect. It would be good to factor that out
as a separate patch that we can backport)

b) assumes the tmp116 (after above fixed) supports things it doesn't.

So it's not a valid use of a fallback compatible.  A driver can't
rely on matching device IDs it didn't previously know about. It sees
tmp116 compatible and thinks it knows how to handle the device, which
it doesn't. This might lead to some very grumpy people not understanding
why their old kernel doesn't work.

Jonathan

> 
> Regards,
>   Marco
> 
> > An example is calibbias which you've dropped from the channels
> > array entry.
> > 
> > Jonathan
> > 
> >   
> > > ---
> > > v2:
> > > - drop items from single enum
> > > 
> > >  .../bindings/iio/temperature/ti,tmp117.yaml        | 14 ++++++++++----
> > >  1 file changed, 10 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
> > > index 8d1ec4d39b28..9b78357d6a79 100644
> > > --- a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
> > > +++ b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
> > > @@ -7,8 +7,9 @@ $schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > >  title: "TI TMP117 - Digital temperature sensor with integrated NV memory"
> > >  
> > >  description: |
> > > -    TI TMP117 - Digital temperature sensor with integrated NV memory that supports
> > > -    I2C interface.
> > > +    TI TMP116/117 - Digital temperature sensor with integrated NV memory that
> > > +    supports I2C interface.
> > > +      https://www.ti.com/lit/gpn/tmp116
> > >        https://www.ti.com/lit/gpn/tmp117
> > >  
> > >  maintainers:
> > > @@ -16,8 +17,13 @@ maintainers:
> > >  
> > >  properties:
> > >    compatible:
> > > -    enum:
> > > -      - ti,tmp117
> > > +    oneOf:
> > > +      - enum:
> > > +          - ti,tmp117
> > > +      - items:
> > > +          - enum:
> > > +              - ti,tmp116
> > > +          - const: ti,tmp117
> > >  
> > >    reg:
> > >      maxItems: 1  
> > 
> >   

Re: [PATCH v2 2/4] dt-bindings: iio: ti,tmp117: add binding for the TMP116

[Marco Felsch]

On 22-12-23, Jonathan Cameron wrote:
> On Fri, 23 Dec 2022 16:03:38 +0100
> Marco Felsch <m.felsch@pengutronix.de> wrote:
> 
> > On 22-12-23, Jonathan Cameron wrote:
> > > On Wed, 21 Dec 2022 10:27:59 +0100
> > > Marco Felsch <m.felsch@pengutronix.de> wrote:
> > >   
> > > > The TMP116 is the predecessor of the TMP117.
> > > > 
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>  
> > > I'm not sure this is introducing a valid fallback. The driver changes
> > > imply some things the tmp117 driver supports, that this device
> > > does not. A fallback compatible would mean that a new DT
> > > with an old kernel would load the tmp117 against a tmp116 and
> > > expect it to fully work.  
> > 
> > Since driver does all the detection an update of the bindings isn't
> > really necessary. It is just to have a compatible already in place in
> > case there a things we can't detected during runtime. This flow is
> > common for a lot of SoC drivers. The fallback will be used as long as
> > possible and once a specific feature can't be detected only via the
> > binding, the driver adds the new binding to it of_compatible.
> 
> That's true going forwards and for drivers that introduce a shared
> generic compatible alongside the initial binding. It can't be easily
> retrofit.
> 
> Fallback compatible is also to allow this to work with old kernels

What this small series does is adding the support for the chip. So the
support starts with the kernel version which includes these patches. Why
do you assume that one expect to have a proper support with an older
kernel? I fully get the point that driver needs to deal with older
device-tree's but having using a newer device-tree's (fw) on older
kernels and expecting that older kernels does support the chip is a bit
odd to me.

> - which it doesn't because the kernel driver
> a) rejects non tmp117 ids (we should fix that by just warning instead 
> if the ID isn't what we expect. It would be good to factor that out
> as a separate patch that we can backport)
> 
> b) assumes the tmp116 (after above fixed) supports things it doesn't.
> 
> So it's not a valid use of a fallback compatible. 

I added the compatible sring only to have it in place for the future. I
really didn't have to do that at all since the detection can be done
during runtime and the only compatible would be tmp117. So sorry but I
don't get your point here.

> A driver can't rely on matching device IDs it didn't previously know
> about. It sees tmp116 compatible and thinks it knows how to handle the
> device, which it doesn't. This might lead to some very grumpy people
> not understanding why their old kernel doesn't work.

Why do you think that people think that the driver have to support a
device based on the compatible? I saw device-trees with nodes not
matching any kernel driver since the device-tree is just the firmware
descibing the hardware. If I got Krzysztof correctly people have to
check the driver implementation for the according support since the
bindings can be used by other projects as well and are not limited to
the linux-kernel.

Regards,
  Marco

> Jonathan
> 
> > 
> > Regards,
> >   Marco
> > 
> > > An example is calibbias which you've dropped from the channels
> > > array entry.
> > > 
> > > Jonathan
> > > 
> > >   
> > > > ---
> > > > v2:
> > > > - drop items from single enum
> > > > 
> > > >  .../bindings/iio/temperature/ti,tmp117.yaml        | 14 ++++++++++----
> > > >  1 file changed, 10 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
> > > > index 8d1ec4d39b28..9b78357d6a79 100644
> > > > --- a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
> > > > +++ b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
> > > > @@ -7,8 +7,9 @@ $schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > >  title: "TI TMP117 - Digital temperature sensor with integrated NV memory"
> > > >  
> > > >  description: |
> > > > -    TI TMP117 - Digital temperature sensor with integrated NV memory that supports
> > > > -    I2C interface.
> > > > +    TI TMP116/117 - Digital temperature sensor with integrated NV memory that
> > > > +    supports I2C interface.
> > > > +      https://www.ti.com/lit/gpn/tmp116
> > > >        https://www.ti.com/lit/gpn/tmp117
> > > >  
> > > >  maintainers:
> > > > @@ -16,8 +17,13 @@ maintainers:
> > > >  
> > > >  properties:
> > > >    compatible:
> > > > -    enum:
> > > > -      - ti,tmp117
> > > > +    oneOf:
> > > > +      - enum:
> > > > +          - ti,tmp117
> > > > +      - items:
> > > > +          - enum:
> > > > +              - ti,tmp116
> > > > +          - const: ti,tmp117
> > > >  
> > > >    reg:
> > > >      maxItems: 1  
> > > 
> > >   
> 
> 

Re: [PATCH v2 2/4] dt-bindings: iio: ti,tmp117: add binding for the TMP116

[Jonathan Cameron]

On Fri, 23 Dec 2022 17:10:51 +0100
Marco Felsch <m.felsch@pengutronix.de> wrote:

> On 22-12-23, Jonathan Cameron wrote:
> > On Fri, 23 Dec 2022 16:03:38 +0100
> > Marco Felsch <m.felsch@pengutronix.de> wrote:
> >   
> > > On 22-12-23, Jonathan Cameron wrote:  
> > > > On Wed, 21 Dec 2022 10:27:59 +0100
> > > > Marco Felsch <m.felsch@pengutronix.de> wrote:
> > > >     
> > > > > The TMP116 is the predecessor of the TMP117.
> > > > > 
> > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>    
> > > > I'm not sure this is introducing a valid fallback. The driver changes
> > > > imply some things the tmp117 driver supports, that this device
> > > > does not. A fallback compatible would mean that a new DT
> > > > with an old kernel would load the tmp117 against a tmp116 and
> > > > expect it to fully work.    
> > > 
> > > Since driver does all the detection an update of the bindings isn't
> > > really necessary. It is just to have a compatible already in place in
> > > case there a things we can't detected during runtime. This flow is
> > > common for a lot of SoC drivers. The fallback will be used as long as
> > > possible and once a specific feature can't be detected only via the
> > > binding, the driver adds the new binding to it of_compatible.  
> > 
> > That's true going forwards and for drivers that introduce a shared
> > generic compatible alongside the initial binding. It can't be easily
> > retrofit.
> > 
> > Fallback compatible is also to allow this to work with old kernels  
> 
> What this small series does is adding the support for the chip. So the
> support starts with the kernel version which includes these patches. Why
> do you assume that one expect to have a proper support with an older
> kernel? I fully get the point that driver needs to deal with older
> device-tree's but having using a newer device-tree's (fw) on older
> kernels and expecting that older kernels does support the chip is a bit
> odd to me.

Probably need the DT maintainers to offer the opinion on this as we
disagree on how fallback compatibles are supposed to work.
I'll accept whatever they say on this point (I've been persuaded
into a more relaxed stance in the past on this).

Allowing a new DTS file to work with old kernels is one of the advantages of
fallback compatibles. On many devices there is no coupling between the two
and there shouldn't need to be one. So in this case the device is not
compatible and we should not imply that it is.

> 
> > - which it doesn't because the kernel driver
> > a) rejects non tmp117 ids (we should fix that by just warning instead 
> > if the ID isn't what we expect. It would be good to factor that out
> > as a separate patch that we can backport)
> > 
> > b) assumes the tmp116 (after above fixed) supports things it doesn't.
> > 
> > So it's not a valid use of a fallback compatible.   
> 
> I added the compatible sring only to have it in place for the future. I
> really didn't have to do that at all since the detection can be done
> during runtime and the only compatible would be tmp117. So sorry but I
> don't get your point here.

Fallback compatibles have multiple purposes. One of them, which should work
is to allow a kernel that only support tmp117 to work with a DTS that says
it is a tmp116 with fallback to tmp117. That's not true here, so there
should not be a fallback.

> 
> > A driver can't rely on matching device IDs it didn't previously know
> > about. It sees tmp116 compatible and thinks it knows how to handle the
> > device, which it doesn't. This might lead to some very grumpy people
> > not understanding why their old kernel doesn't work.  
> 
> Why do you think that people think that the driver have to support a
> device based on the compatible? I saw device-trees with nodes not
> matching any kernel driver since the device-tree is just the firmware
> descibing the hardware. If I got Krzysztof correctly people have to
> check the driver implementation for the according support since the
> bindings can be used by other projects as well and are not limited to
> the linux-kernel.

My understanding is that supporting new compatible parts on old kernels
is one of the reasons fall back compatibles exist (as well as the case
you mention of not modifying a driver until we hit something that requires
it) If that doesn't work, then the driver is broken - which is why I suggest
we fix that.  However in this case the part is not compatible
so the fallback should not be there. You should just add the ID to the
driver given you are modifying it to support the part anyway.

Jonathan


> 
> Regards,
>   Marco
> 
> > Jonathan
> >   
> > > 
> > > Regards,
> > >   Marco
> > >   
> > > > An example is calibbias which you've dropped from the channels
> > > > array entry.
> > > > 
> > > > Jonathan
> > > > 
> > > >     
> > > > > ---
> > > > > v2:
> > > > > - drop items from single enum
> > > > > 
> > > > >  .../bindings/iio/temperature/ti,tmp117.yaml        | 14 ++++++++++----
> > > > >  1 file changed, 10 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
> > > > > index 8d1ec4d39b28..9b78357d6a79 100644
> > > > > --- a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
> > > > > +++ b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
> > > > > @@ -7,8 +7,9 @@ $schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > > >  title: "TI TMP117 - Digital temperature sensor with integrated NV memory"
> > > > >  
> > > > >  description: |
> > > > > -    TI TMP117 - Digital temperature sensor with integrated NV memory that supports
> > > > > -    I2C interface.
> > > > > +    TI TMP116/117 - Digital temperature sensor with integrated NV memory that
> > > > > +    supports I2C interface.
> > > > > +      https://www.ti.com/lit/gpn/tmp116
> > > > >        https://www.ti.com/lit/gpn/tmp117
> > > > >  
> > > > >  maintainers:
> > > > > @@ -16,8 +17,13 @@ maintainers:
> > > > >  
> > > > >  properties:
> > > > >    compatible:
> > > > > -    enum:
> > > > > -      - ti,tmp117
> > > > > +    oneOf:
> > > > > +      - enum:
> > > > > +          - ti,tmp117
> > > > > +      - items:
> > > > > +          - enum:
> > > > > +              - ti,tmp116
> > > > > +          - const: ti,tmp117
> > > > >  
> > > > >    reg:
> > > > >      maxItems: 1    
> > > > 
> > > >     
> > 
> >   

Re: [PATCH v2 2/4] dt-bindings: iio: ti,tmp117: add binding for the TMP116

[Marco Felsch]

Hi Jonathan,

On 22-12-23, Jonathan Cameron wrote:
> On Fri, 23 Dec 2022 17:10:51 +0100
> Marco Felsch <m.felsch@pengutronix.de> wrote:
> 
> > On 22-12-23, Jonathan Cameron wrote:
> > > On Fri, 23 Dec 2022 16:03:38 +0100
> > > Marco Felsch <m.felsch@pengutronix.de> wrote:
> > >   
> > > > On 22-12-23, Jonathan Cameron wrote:  
> > > > > On Wed, 21 Dec 2022 10:27:59 +0100
> > > > > Marco Felsch <m.felsch@pengutronix.de> wrote:
> > > > >     
> > > > > > The TMP116 is the predecessor of the TMP117.
> > > > > > 
> > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>    
> > > > > I'm not sure this is introducing a valid fallback. The driver changes
> > > > > imply some things the tmp117 driver supports, that this device
> > > > > does not. A fallback compatible would mean that a new DT
> > > > > with an old kernel would load the tmp117 against a tmp116 and
> > > > > expect it to fully work.    
> > > > 
> > > > Since driver does all the detection an update of the bindings isn't
> > > > really necessary. It is just to have a compatible already in place in
> > > > case there a things we can't detected during runtime. This flow is
> > > > common for a lot of SoC drivers. The fallback will be used as long as
> > > > possible and once a specific feature can't be detected only via the
> > > > binding, the driver adds the new binding to it of_compatible.  
> > > 
> > > That's true going forwards and for drivers that introduce a shared
> > > generic compatible alongside the initial binding. It can't be easily
> > > retrofit.
> > > 
> > > Fallback compatible is also to allow this to work with old kernels  
> > 
> > What this small series does is adding the support for the chip. So the
> > support starts with the kernel version which includes these patches. Why
> > do you assume that one expect to have a proper support with an older
> > kernel? I fully get the point that driver needs to deal with older
> > device-tree's but having using a newer device-tree's (fw) on older
> > kernels and expecting that older kernels does support the chip is a bit
> > odd to me.
> 
> Probably need the DT maintainers to offer the opinion on this as we
> disagree on how fallback compatibles are supposed to work.
> I'll accept whatever they say on this point (I've been persuaded
> into a more relaxed stance in the past on this).

Me too :) and if it is your way I can go with it by adding a new ID. I
just wanna know how to handle this.

> Allowing a new DTS file to work with old kernels is one of the advantages of
> fallback compatibles.

I never used and thought about it that way.

> On many devices there is no coupling between the two and there
> shouldn't need to be one. So in this case the device is not compatible
> and we should not imply that it is.

No there should be no coupling, this is my understandig as well.

> > > - which it doesn't because the kernel driver
> > > a) rejects non tmp117 ids (we should fix that by just warning instead 
> > > if the ID isn't what we expect. It would be good to factor that out
> > > as a separate patch that we can backport)
> > > 
> > > b) assumes the tmp116 (after above fixed) supports things it doesn't.
> > > 
> > > So it's not a valid use of a fallback compatible.   
> > 
> > I added the compatible sring only to have it in place for the future. I
> > really didn't have to do that at all since the detection can be done
> > during runtime and the only compatible would be tmp117. So sorry but I
> > don't get your point here.
> 
> Fallback compatibles have multiple purposes. One of them, which should work
> is to allow a kernel that only support tmp117 to work with a DTS that says
> it is a tmp116 with fallback to tmp117. That's not true here, so there
> should not be a fallback.
> 
> > 
> > > A driver can't rely on matching device IDs it didn't previously know
> > > about. It sees tmp116 compatible and thinks it knows how to handle the
> > > device, which it doesn't. This might lead to some very grumpy people
> > > not understanding why their old kernel doesn't work.  
> > 
> > Why do you think that people think that the driver have to support a
> > device based on the compatible? I saw device-trees with nodes not
> > matching any kernel driver since the device-tree is just the firmware
> > descibing the hardware. If I got Krzysztof correctly people have to
> > check the driver implementation for the according support since the
> > bindings can be used by other projects as well and are not limited to
> > the linux-kernel.
> 
> My understanding is that supporting new compatible parts on old kernels
> is one of the reasons fall back compatibles exist (as well as the case
> you mention of not modifying a driver until we hit something that requires
> it) If that doesn't work, then the driver is broken - which is why I suggest
> we fix that.  However in this case the part is not compatible
> so the fallback should not be there. You should just add the ID to the
> driver given you are modifying it to support the part anyway.

Let's see what the dt-maintainers say :)

Regards,
  Marco

Re: [PATCH v2 2/4] dt-bindings: iio: ti,tmp117: add binding for the TMP116

[Krzysztof Kozlowski]

On 23/12/2022 18:13, Marco Felsch wrote:
> Hi Jonathan,
> 
> On 22-12-23, Jonathan Cameron wrote:
>> On Fri, 23 Dec 2022 17:10:51 +0100
>> Marco Felsch <m.felsch@pengutronix.de> wrote:
>>
>>> On 22-12-23, Jonathan Cameron wrote:
>>>> On Fri, 23 Dec 2022 16:03:38 +0100
>>>> Marco Felsch <m.felsch@pengutronix.de> wrote:
>>>>   
>>>>> On 22-12-23, Jonathan Cameron wrote:  
>>>>>> On Wed, 21 Dec 2022 10:27:59 +0100
>>>>>> Marco Felsch <m.felsch@pengutronix.de> wrote:
>>>>>>     
>>>>>>> The TMP116 is the predecessor of the TMP117.
>>>>>>>
>>>>>>> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>    
>>>>>> I'm not sure this is introducing a valid fallback. The driver changes
>>>>>> imply some things the tmp117 driver supports, that this device
>>>>>> does not. A fallback compatible would mean that a new DT
>>>>>> with an old kernel would load the tmp117 against a tmp116 and
>>>>>> expect it to fully work.    
>>>>>
>>>>> Since driver does all the detection an update of the bindings isn't
>>>>> really necessary. It is just to have a compatible already in place in
>>>>> case there a things we can't detected during runtime. This flow is
>>>>> common for a lot of SoC drivers. The fallback will be used as long as
>>>>> possible and once a specific feature can't be detected only via the
>>>>> binding, the driver adds the new binding to it of_compatible.  
>>>>
>>>> That's true going forwards and for drivers that introduce a shared
>>>> generic compatible alongside the initial binding. It can't be easily
>>>> retrofit.
>>>>
>>>> Fallback compatible is also to allow this to work with old kernels  

Yes, if the devices are compatible, e.g. there is no need to change in
the driver to support new device.

If the devices need auto-detection and are compatible in an auto-detect
way, then I don't think we have such goal.

>>>
>>> What this small series does is adding the support for the chip. So the
>>> support starts with the kernel version which includes these patches. Why
>>> do you assume that one expect to have a proper support with an older
>>> kernel? I fully get the point that driver needs to deal with older
>>> device-tree's but having using a newer device-tree's (fw) on older
>>> kernels and expecting that older kernels does support the chip is a bit
>>> odd to me.
>>
>> Probably need the DT maintainers to offer the opinion on this as we
>> disagree on how fallback compatibles are supposed to work.
>> I'll accept whatever they say on this point (I've been persuaded
>> into a more relaxed stance in the past on this).

DTS can be used outside of kernel - other projects or new DTS with old
kernel - and the way of working is bound by bindings. Therefore it is
really good if you use new DTS with older kernel and it works.

As I said above, for devices that are fully compatible, this should be
the goal. Many SoC components are like this and we describe them that
way. However they do not have mostly auto-detection.

Now for devices which are both:
 - compatible according to the binding (so the interface is the same,
stable and handled by Linux),
 - AND actually significantly different, where the difference is
recognized by auto-detection,
the Linux should be reasonable and it might freely choose not to support
unknown devices.

You can compare it to the world without DT where everything is
auto-detectable. The Linux kernel performs auto-detection and based on
this either works or does not work with the device. But the kernel has
full discretion to decide about it.

Users would be happy if kernel would work with unknown, new devices. But
also users would be unhappy if this damages their system because of e.g.
wrong voltage.

Best regards,
Krzysztof

Re: [PATCH v2 2/4] dt-bindings: iio: ti,tmp117: add binding for the TMP116

[Jonathan Cameron]

On Tue, 27 Dec 2022 09:40:13 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 23/12/2022 18:13, Marco Felsch wrote:
> > Hi Jonathan,
> > 
> > On 22-12-23, Jonathan Cameron wrote:  
> >> On Fri, 23 Dec 2022 17:10:51 +0100
> >> Marco Felsch <m.felsch@pengutronix.de> wrote:
> >>  
> >>> On 22-12-23, Jonathan Cameron wrote:  
> >>>> On Fri, 23 Dec 2022 16:03:38 +0100
> >>>> Marco Felsch <m.felsch@pengutronix.de> wrote:
> >>>>     
> >>>>> On 22-12-23, Jonathan Cameron wrote:    
> >>>>>> On Wed, 21 Dec 2022 10:27:59 +0100
> >>>>>> Marco Felsch <m.felsch@pengutronix.de> wrote:
> >>>>>>       
> >>>>>>> The TMP116 is the predecessor of the TMP117.
> >>>>>>>
> >>>>>>> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>      
> >>>>>> I'm not sure this is introducing a valid fallback. The driver changes
> >>>>>> imply some things the tmp117 driver supports, that this device
> >>>>>> does not. A fallback compatible would mean that a new DT
> >>>>>> with an old kernel would load the tmp117 against a tmp116 and
> >>>>>> expect it to fully work.      
> >>>>>
> >>>>> Since driver does all the detection an update of the bindings isn't
> >>>>> really necessary. It is just to have a compatible already in place in
> >>>>> case there a things we can't detected during runtime. This flow is
> >>>>> common for a lot of SoC drivers. The fallback will be used as long as
> >>>>> possible and once a specific feature can't be detected only via the
> >>>>> binding, the driver adds the new binding to it of_compatible.    
> >>>>
> >>>> That's true going forwards and for drivers that introduce a shared
> >>>> generic compatible alongside the initial binding. It can't be easily
> >>>> retrofit.
> >>>>
> >>>> Fallback compatible is also to allow this to work with old kernels    
> 
> Yes, if the devices are compatible, e.g. there is no need to change in
> the driver to support new device.
> 
> If the devices need auto-detection and are compatible in an auto-detect
> way, then I don't think we have such goal.
> 
> >>>
> >>> What this small series does is adding the support for the chip. So the
> >>> support starts with the kernel version which includes these patches. Why
> >>> do you assume that one expect to have a proper support with an older
> >>> kernel? I fully get the point that driver needs to deal with older
> >>> device-tree's but having using a newer device-tree's (fw) on older
> >>> kernels and expecting that older kernels does support the chip is a bit
> >>> odd to me.  
> >>
> >> Probably need the DT maintainers to offer the opinion on this as we
> >> disagree on how fallback compatibles are supposed to work.
> >> I'll accept whatever they say on this point (I've been persuaded
> >> into a more relaxed stance in the past on this).  
> 
> DTS can be used outside of kernel - other projects or new DTS with old
> kernel - and the way of working is bound by bindings. Therefore it is
> really good if you use new DTS with older kernel and it works.
> 
> As I said above, for devices that are fully compatible, this should be
> the goal. Many SoC components are like this and we describe them that
> way. However they do not have mostly auto-detection.
> 
> Now for devices which are both:
>  - compatible according to the binding (so the interface is the same,
> stable and handled by Linux),
>  - AND actually significantly different, where the difference is
> recognized by auto-detection,
> the Linux should be reasonable and it might freely choose not to support
> unknown devices.

Ok. In this case my gut feeling would be that a new ID and no fallback
is the best balance.  Ironically if we'd had a binding for the tmp116 first
and fell back to that from the tmp117 we'd probably be fine (just
have fewer features).  I guess nothing stops us documenting that binding
even though the tmp117 is already used to match in Linux.

> 
> You can compare it to the world without DT where everything is
> auto-detectable. The Linux kernel performs auto-detection and based on
> this either works or does not work with the device. But the kernel has
> full discretion to decide about it.
> 
> Users would be happy if kernel would work with unknown, new devices. But
> also users would be unhappy if this damages their system because of e.g.
> wrong voltage.

Agreed - using old code is a nice to have, but not always the best choice.

Jonathan

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 2/4] dt-bindings: iio: ti,tmp117: add binding for the TMP116

[Marco Felsch]

On 22-12-30, Jonathan Cameron wrote:
> On Tue, 27 Dec 2022 09:40:13 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
> > On 23/12/2022 18:13, Marco Felsch wrote:
> > > Hi Jonathan,
> > > 
> > > On 22-12-23, Jonathan Cameron wrote:  
> > >> On Fri, 23 Dec 2022 17:10:51 +0100
> > >> Marco Felsch <m.felsch@pengutronix.de> wrote:
> > >>  
> > >>> On 22-12-23, Jonathan Cameron wrote:  
> > >>>> On Fri, 23 Dec 2022 16:03:38 +0100
> > >>>> Marco Felsch <m.felsch@pengutronix.de> wrote:
> > >>>>     
> > >>>>> On 22-12-23, Jonathan Cameron wrote:    
> > >>>>>> On Wed, 21 Dec 2022 10:27:59 +0100
> > >>>>>> Marco Felsch <m.felsch@pengutronix.de> wrote:
> > >>>>>>       
> > >>>>>>> The TMP116 is the predecessor of the TMP117.
> > >>>>>>>
> > >>>>>>> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>      
> > >>>>>> I'm not sure this is introducing a valid fallback. The driver changes
> > >>>>>> imply some things the tmp117 driver supports, that this device
> > >>>>>> does not. A fallback compatible would mean that a new DT
> > >>>>>> with an old kernel would load the tmp117 against a tmp116 and
> > >>>>>> expect it to fully work.      
> > >>>>>
> > >>>>> Since driver does all the detection an update of the bindings isn't
> > >>>>> really necessary. It is just to have a compatible already in place in
> > >>>>> case there a things we can't detected during runtime. This flow is
> > >>>>> common for a lot of SoC drivers. The fallback will be used as long as
> > >>>>> possible and once a specific feature can't be detected only via the
> > >>>>> binding, the driver adds the new binding to it of_compatible.    
> > >>>>
> > >>>> That's true going forwards and for drivers that introduce a shared
> > >>>> generic compatible alongside the initial binding. It can't be easily
> > >>>> retrofit.
> > >>>>
> > >>>> Fallback compatible is also to allow this to work with old kernels    
> > 
> > Yes, if the devices are compatible, e.g. there is no need to change in
> > the driver to support new device.
> > 
> > If the devices need auto-detection and are compatible in an auto-detect
> > way, then I don't think we have such goal.
> > 
> > >>>
> > >>> What this small series does is adding the support for the chip. So the
> > >>> support starts with the kernel version which includes these patches. Why
> > >>> do you assume that one expect to have a proper support with an older
> > >>> kernel? I fully get the point that driver needs to deal with older
> > >>> device-tree's but having using a newer device-tree's (fw) on older
> > >>> kernels and expecting that older kernels does support the chip is a bit
> > >>> odd to me.  
> > >>
> > >> Probably need the DT maintainers to offer the opinion on this as we
> > >> disagree on how fallback compatibles are supposed to work.
> > >> I'll accept whatever they say on this point (I've been persuaded
> > >> into a more relaxed stance in the past on this).  
> > 
> > DTS can be used outside of kernel - other projects or new DTS with old
> > kernel - and the way of working is bound by bindings. Therefore it is
> > really good if you use new DTS with older kernel and it works.
> > 
> > As I said above, for devices that are fully compatible, this should be
> > the goal. Many SoC components are like this and we describe them that
> > way. However they do not have mostly auto-detection.
> > 
> > Now for devices which are both:
> >  - compatible according to the binding (so the interface is the same,
> > stable and handled by Linux),
> >  - AND actually significantly different, where the difference is
> > recognized by auto-detection,
> > the Linux should be reasonable and it might freely choose not to support
> > unknown devices.
> 
> Ok. In this case my gut feeling would be that a new ID and no fallback
> is the best balance.  Ironically if we'd had a binding for the tmp116 first
> and fell back to that from the tmp117 we'd probably be fine (just
> have fewer features).  I guess nothing stops us documenting that binding
> even though the tmp117 is already used to match in Linux.

Hi Jonathan, Krzysztof,

thanks for the detailed explanation. IMHO the fallback mechanism is
still fine but if you are worried about it I will change this to have a
separate compatible.

Regards,
  Marco

> > You can compare it to the world without DT where everything is
> > auto-detectable. The Linux kernel performs auto-detection and based on
> > this either works or does not work with the device. But the kernel has
> > full discretion to decide about it.
> > 
> > Users would be happy if kernel would work with unknown, new devices. But
> > also users would be unhappy if this damages their system because of e.g.
> > wrong voltage.
> 
> Agreed - using old code is a nice to have, but not always the best choice.
> 
> Jonathan
> 
> > 
> > Best regards,
> > Krzysztof
> > 
> 
> 

[PATCH v2 3/4] iio: temperature: tmp117: add TI TMP116 support

[Marco Felsch]

The TMP116 is the predecessor of the TMP117. The TMP116 don't support
custom offset calibration data, instead this register is used as generic
EEPROM storage as well.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- no changes

 drivers/iio/temperature/tmp117.c | 40 ++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/temperature/tmp117.c b/drivers/iio/temperature/tmp117.c
index f9b8f2b570f6..468dafa6fa8e 100644
--- a/drivers/iio/temperature/tmp117.c
+++ b/drivers/iio/temperature/tmp117.c
@@ -31,9 +31,11 @@
 #define TMP117_REG_DEVICE_ID		0xF
 
 #define TMP117_RESOLUTION_10UC		78125
-#define TMP117_DEVICE_ID		0x0117
 #define MICRODEGREE_PER_10MILLIDEGREE	10000
 
+#define TMP116_DEVICE_ID		0x1116
+#define TMP117_DEVICE_ID		0x0117
+
 struct tmp117_data {
 	struct i2c_client *client;
 	s16 calibbias;
@@ -105,6 +107,13 @@ static const struct iio_chan_spec tmp117_channels[] = {
 		.type = IIO_TEMP,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_SCALE),
+};
+
+static const struct iio_chan_spec tmp116_channels[] = {
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
 	},
 };
 
@@ -118,27 +127,28 @@ static int tmp117_identify(struct i2c_client *client)
 	int dev_id;
 
 	dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID);
-	if (dev_id < 0)
+	switch (dev_id) {
+	case TMP116_DEVICE_ID:
+	case TMP117_DEVICE_ID:
 		return dev_id;
-	if (dev_id != TMP117_DEVICE_ID) {
-		dev_err(&client->dev, "TMP117 not found\n");
+	default:
+		dev_err(&client->dev, "TMP116/117 not found\n");
 		return -ENODEV;
 	}
-	return 0;
 }
 
 static int tmp117_probe(struct i2c_client *client)
 {
 	struct tmp117_data *data;
 	struct iio_dev *indio_dev;
-	int ret;
+	int dev_id;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
 		return -EOPNOTSUPP;
 
-	ret = tmp117_identify(client);
-	if (ret < 0)
-		return ret;
+	dev_id = tmp117_identify(client);
+	if (dev_id < 0)
+		return dev_id;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 	if (!indio_dev)
@@ -148,12 +158,18 @@ static int tmp117_probe(struct i2c_client *client)
 	data->client = client;
 	data->calibbias = 0;
 
-	indio_dev->name = "tmp117";
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &tmp117_info;
 
-	indio_dev->channels = tmp117_channels;
-	indio_dev->num_channels = ARRAY_SIZE(tmp117_channels);
+	if (dev_id == TMP117_DEVICE_ID) {
+		indio_dev->channels = tmp117_channels;
+		indio_dev->num_channels = ARRAY_SIZE(tmp117_channels);
+		indio_dev->name = "tmp117";
+	} else {
+		indio_dev->channels = tmp116_channels;
+		indio_dev->num_channels = ARRAY_SIZE(tmp116_channels);
+		indio_dev->name = "tmp116";
+	}
 
 	return devm_iio_device_register(&client->dev, indio_dev);
 }
-- 
2.30.2

Re: [PATCH v2 3/4] iio: temperature: tmp117: add TI TMP116 support

[Jonathan Cameron]

On Wed, 21 Dec 2022 10:28:00 +0100
Marco Felsch <m.felsch@pengutronix.de> wrote:

> The TMP116 is the predecessor of the TMP117. The TMP116 don't support
> custom offset calibration data, instead this register is used as generic
> EEPROM storage as well.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
A few comments inline.
Thanks,

Jonathan

> ---
> v2:
> - no changes
> 
>  drivers/iio/temperature/tmp117.c | 40 ++++++++++++++++++++++----------
>  1 file changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/temperature/tmp117.c b/drivers/iio/temperature/tmp117.c
> index f9b8f2b570f6..468dafa6fa8e 100644
> --- a/drivers/iio/temperature/tmp117.c
> +++ b/drivers/iio/temperature/tmp117.c
> @@ -31,9 +31,11 @@
>  #define TMP117_REG_DEVICE_ID		0xF
>  
>  #define TMP117_RESOLUTION_10UC		78125
> -#define TMP117_DEVICE_ID		0x0117
>  #define MICRODEGREE_PER_10MILLIDEGREE	10000
>  
> +#define TMP116_DEVICE_ID		0x1116
> +#define TMP117_DEVICE_ID		0x0117
> +
>  struct tmp117_data {
>  	struct i2c_client *client;
>  	s16 calibbias;
> @@ -105,6 +107,13 @@ static const struct iio_chan_spec tmp117_channels[] = {
>  		.type = IIO_TEMP,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_SCALE),
> +};
> +
> +static const struct iio_chan_spec tmp116_channels[] = {
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
>  	},
>  };
>  
> @@ -118,27 +127,28 @@ static int tmp117_identify(struct i2c_client *client)
>  	int dev_id;
>  
>  	dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID);
> -	if (dev_id < 0)

Keep this handling of the smbus read returning an error.
Otherwise, you end up replacing the error code with -ENODEV rather than
returning what actually happened.

	if (dev_id < 0)
		return dev_id;

	switch (dev_id) {
...

> +	switch (dev_id) {
> +	case TMP116_DEVICE_ID:
> +	case TMP117_DEVICE_ID:
>  		return dev_id;
> -	if (dev_id != TMP117_DEVICE_ID) {
> -		dev_err(&client->dev, "TMP117 not found\n");
> +	default:
> +		dev_err(&client->dev, "TMP116/117 not found\n");
>  		return -ENODEV;
>  	}
> -	return 0;
>  }
>  
>  static int tmp117_probe(struct i2c_client *client)
>  {
>  	struct tmp117_data *data;
>  	struct iio_dev *indio_dev;
> -	int ret;
> +	int dev_id;
>  
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
>  		return -EOPNOTSUPP;
>  
> -	ret = tmp117_identify(client);
> -	if (ret < 0)
> -		return ret;
> +	dev_id = tmp117_identify(client);
> +	if (dev_id < 0)
> +		return dev_id;
>  
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>  	if (!indio_dev)
> @@ -148,12 +158,18 @@ static int tmp117_probe(struct i2c_client *client)
>  	data->client = client;
>  	data->calibbias = 0;
>  
> -	indio_dev->name = "tmp117";
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &tmp117_info;
>  
> -	indio_dev->channels = tmp117_channels;
> -	indio_dev->num_channels = ARRAY_SIZE(tmp117_channels);
> +	if (dev_id == TMP117_DEVICE_ID) {

Probably better to assume we may get more parts in future and use a
switch statement here to explicitly match each value.

> +		indio_dev->channels = tmp117_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(tmp117_channels);
> +		indio_dev->name = "tmp117";
> +	} else {
> +		indio_dev->channels = tmp116_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(tmp116_channels);
> +		indio_dev->name = "tmp116";
> +	}
>  
>  	return devm_iio_device_register(&client->dev, indio_dev);
>  }

Re: [PATCH v2 3/4] iio: temperature: tmp117: add TI TMP116 support

[Marco Felsch]

Hi Jonathan,

On 22-12-23, Jonathan Cameron wrote:
> On Wed, 21 Dec 2022 10:28:00 +0100
> Marco Felsch <m.felsch@pengutronix.de> wrote:
> 
> > The TMP116 is the predecessor of the TMP117. The TMP116 don't support
> > custom offset calibration data, instead this register is used as generic
> > EEPROM storage as well.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> A few comments inline.
> Thanks,
> 
> Jonathan
> 
> > ---
> > v2:
> > - no changes
> > 
> >  drivers/iio/temperature/tmp117.c | 40 ++++++++++++++++++++++----------
> >  1 file changed, 28 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/iio/temperature/tmp117.c b/drivers/iio/temperature/tmp117.c
> > index f9b8f2b570f6..468dafa6fa8e 100644
> > --- a/drivers/iio/temperature/tmp117.c
> > +++ b/drivers/iio/temperature/tmp117.c
> > @@ -31,9 +31,11 @@
> >  #define TMP117_REG_DEVICE_ID		0xF
> >  
> >  #define TMP117_RESOLUTION_10UC		78125
> > -#define TMP117_DEVICE_ID		0x0117
> >  #define MICRODEGREE_PER_10MILLIDEGREE	10000
> >  
> > +#define TMP116_DEVICE_ID		0x1116
> > +#define TMP117_DEVICE_ID		0x0117
> > +
> >  struct tmp117_data {
> >  	struct i2c_client *client;
> >  	s16 calibbias;
> > @@ -105,6 +107,13 @@ static const struct iio_chan_spec tmp117_channels[] = {
> >  		.type = IIO_TEMP,
> >  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >  			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_SCALE),
> > +};
> > +
> > +static const struct iio_chan_spec tmp116_channels[] = {
> > +	{
> > +		.type = IIO_TEMP,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +				      BIT(IIO_CHAN_INFO_SCALE),
> >  	},
> >  };
> >  
> > @@ -118,27 +127,28 @@ static int tmp117_identify(struct i2c_client *client)
> >  	int dev_id;
> >  
> >  	dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID);
> > -	if (dev_id < 0)
> 
> Keep this handling of the smbus read returning an error.
> Otherwise, you end up replacing the error code with -ENODEV rather than
> returning what actually happened.
> 
> 	if (dev_id < 0)
> 		return dev_id;

You're right, I will change this thanks.

> 	switch (dev_id) {
> ...
> 
> > +	switch (dev_id) {
> > +	case TMP116_DEVICE_ID:
> > +	case TMP117_DEVICE_ID:
> >  		return dev_id;
> > -	if (dev_id != TMP117_DEVICE_ID) {
> > -		dev_err(&client->dev, "TMP117 not found\n");
> > +	default:
> > +		dev_err(&client->dev, "TMP116/117 not found\n");
> >  		return -ENODEV;
> >  	}
> > -	return 0;
> >  }
> >  
> >  static int tmp117_probe(struct i2c_client *client)
> >  {
> >  	struct tmp117_data *data;
> >  	struct iio_dev *indio_dev;
> > -	int ret;
> > +	int dev_id;
> >  
> >  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
> >  		return -EOPNOTSUPP;
> >  
> > -	ret = tmp117_identify(client);
> > -	if (ret < 0)
> > -		return ret;
> > +	dev_id = tmp117_identify(client);
> > +	if (dev_id < 0)
> > +		return dev_id;
> >  
> >  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> >  	if (!indio_dev)
> > @@ -148,12 +158,18 @@ static int tmp117_probe(struct i2c_client *client)
> >  	data->client = client;
> >  	data->calibbias = 0;
> >  
> > -	indio_dev->name = "tmp117";
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> >  	indio_dev->info = &tmp117_info;
> >  
> > -	indio_dev->channels = tmp117_channels;
> > -	indio_dev->num_channels = ARRAY_SIZE(tmp117_channels);
> > +	if (dev_id == TMP117_DEVICE_ID) {
> 
> Probably better to assume we may get more parts in future and use a
> switch statement here to explicitly match each value.

As you want, I will change it.

Regards,
  Marco

> > +		indio_dev->channels = tmp117_channels;
> > +		indio_dev->num_channels = ARRAY_SIZE(tmp117_channels);
> > +		indio_dev->name = "tmp117";
> > +	} else {
> > +		indio_dev->channels = tmp116_channels;
> > +		indio_dev->num_channels = ARRAY_SIZE(tmp116_channels);
> > +		indio_dev->name = "tmp116";
> > +	}
> >  
> >  	return devm_iio_device_register(&client->dev, indio_dev);
> >  }
> 
> 

Re: [PATCH v2 3/4] iio: temperature: tmp117: add TI TMP116 support

[Jonathan Cameron]

On Fri, 23 Dec 2022 16:07:28 +0100
Marco Felsch <m.felsch@pengutronix.de> wrote:

> Hi Jonathan,
> 
> On 22-12-23, Jonathan Cameron wrote:
> > On Wed, 21 Dec 2022 10:28:00 +0100
> > Marco Felsch <m.felsch@pengutronix.de> wrote:
> >   
> > > The TMP116 is the predecessor of the TMP117. The TMP116 don't support
> > > custom offset calibration data, instead this register is used as generic
> > > EEPROM storage as well.
> > > 
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>  
> > A few comments inline.
> > Thanks,
> > 
> > Jonathan
> >   
> > > ---
> > > v2:
> > > - no changes
> > > 
> > >  drivers/iio/temperature/tmp117.c | 40 ++++++++++++++++++++++----------
> > >  1 file changed, 28 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/iio/temperature/tmp117.c b/drivers/iio/temperature/tmp117.c
> > > index f9b8f2b570f6..468dafa6fa8e 100644
> > > --- a/drivers/iio/temperature/tmp117.c
> > > +++ b/drivers/iio/temperature/tmp117.c
> > > @@ -31,9 +31,11 @@
> > >  #define TMP117_REG_DEVICE_ID		0xF
> > >  
> > >  #define TMP117_RESOLUTION_10UC		78125
> > > -#define TMP117_DEVICE_ID		0x0117
> > >  #define MICRODEGREE_PER_10MILLIDEGREE	10000
> > >  
> > > +#define TMP116_DEVICE_ID		0x1116
> > > +#define TMP117_DEVICE_ID		0x0117
> > > +
> > >  struct tmp117_data {
> > >  	struct i2c_client *client;
> > >  	s16 calibbias;
> > > @@ -105,6 +107,13 @@ static const struct iio_chan_spec tmp117_channels[] = {
> > >  		.type = IIO_TEMP,
> > >  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > >  			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_SCALE),
> > > +};
> > > +
> > > +static const struct iio_chan_spec tmp116_channels[] = {
> > > +	{
> > > +		.type = IIO_TEMP,
> > > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > > +				      BIT(IIO_CHAN_INFO_SCALE),
> > >  	},
> > >  };
> > >  
> > > @@ -118,27 +127,28 @@ static int tmp117_identify(struct i2c_client *client)
> > >  	int dev_id;
> > >  
> > >  	dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID);
> > > -	if (dev_id < 0)  
> > 
> > Keep this handling of the smbus read returning an error.
> > Otherwise, you end up replacing the error code with -ENODEV rather than
> > returning what actually happened.
> > 
> > 	if (dev_id < 0)
> > 		return dev_id;  
> 
> You're right, I will change this thanks.
> 
> > 	switch (dev_id) {
> > ...
> >   
> > > +	switch (dev_id) {
> > > +	case TMP116_DEVICE_ID:
> > > +	case TMP117_DEVICE_ID:
> > >  		return dev_id;
> > > -	if (dev_id != TMP117_DEVICE_ID) {
> > > -		dev_err(&client->dev, "TMP117 not found\n");
> > > +	default:
> > > +		dev_err(&client->dev, "TMP116/117 not found\n");
> > >  		return -ENODEV;
As per the other branch of this thread.  This isn't an error.
If we want fallback compatibles to work in their role of allowing
for newer devices that are actually compatible, the most we should
do here is warn.

Say a new tmp117b device is released. It's fully backwards compatible
with the exception of an ID - or supports only new features + backwards
compatibility then that would have a fallback to tmp117 and we need
it to work.

> > >  	}
> > > -	return 0;
> > >  }
> > >  
> > >  static int tmp117_probe(struct i2c_client *client)
> > >  {
> > >  	struct tmp117_data *data;
> > >  	struct iio_dev *indio_dev;
> > > -	int ret;
> > > +	int dev_id;
> > >  
> > >  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
> > >  		return -EOPNOTSUPP;
> > >  
> > > -	ret = tmp117_identify(client);
> > > -	if (ret < 0)
> > > -		return ret;
> > > +	dev_id = tmp117_identify(client);
> > > +	if (dev_id < 0)
> > > +		return dev_id;
> > >  
> > >  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > >  	if (!indio_dev)
> > > @@ -148,12 +158,18 @@ static int tmp117_probe(struct i2c_client *client)
> > >  	data->client = client;
> > >  	data->calibbias = 0;
> > >  
> > > -	indio_dev->name = "tmp117";
> > >  	indio_dev->modes = INDIO_DIRECT_MODE;
> > >  	indio_dev->info = &tmp117_info;
> > >  
> > > -	indio_dev->channels = tmp117_channels;
> > > -	indio_dev->num_channels = ARRAY_SIZE(tmp117_channels);
> > > +	if (dev_id == TMP117_DEVICE_ID) {  
> > 
> > Probably better to assume we may get more parts in future and use a
> > switch statement here to explicitly match each value.  
> 
> As you want, I will change it.
> 
> Regards,
>   Marco
> 
> > > +		indio_dev->channels = tmp117_channels;
> > > +		indio_dev->num_channels = ARRAY_SIZE(tmp117_channels);
> > > +		indio_dev->name = "tmp117";
> > > +	} else {
> > > +		indio_dev->channels = tmp116_channels;
> > > +		indio_dev->num_channels = ARRAY_SIZE(tmp116_channels);
> > > +		indio_dev->name = "tmp116";
> > > +	}
> > >  
> > >  	return devm_iio_device_register(&client->dev, indio_dev);
> > >  }  
> > 
> >   

Re: [PATCH v2 3/4] iio: temperature: tmp117: add TI TMP116 support

[Marco Felsch]

On 22-12-23, Jonathan Cameron wrote:

...

> > > > @@ -118,27 +127,28 @@ static int tmp117_identify(struct i2c_client *client)
> > > >  	int dev_id;
> > > >  
> > > >  	dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID);
> > > > -	if (dev_id < 0)  
> > > 
> > > Keep this handling of the smbus read returning an error.
> > > Otherwise, you end up replacing the error code with -ENODEV rather than
> > > returning what actually happened.
> > > 
> > > 	if (dev_id < 0)
> > > 		return dev_id;  
> > 
> > You're right, I will change this thanks.
> > 
> > > 	switch (dev_id) {
> > > ...
> > >   
> > > > +	switch (dev_id) {
> > > > +	case TMP116_DEVICE_ID:
> > > > +	case TMP117_DEVICE_ID:
> > > >  		return dev_id;
> > > > -	if (dev_id != TMP117_DEVICE_ID) {
> > > > -		dev_err(&client->dev, "TMP117 not found\n");
> > > > +	default:
> > > > +		dev_err(&client->dev, "TMP116/117 not found\n");
> > > >  		return -ENODEV;
>
> As per the other branch of this thread.  This isn't an error.
> If we want fallback compatibles to work in their role of allowing
> for newer devices that are actually compatible, the most we should
> do here is warn.
> 
> Say a new tmp117b device is released. It's fully backwards compatible
> with the exception of an ID - or supports only new features + backwards
> compatibility then that would have a fallback to tmp117 and we need
> it to work.

This isn't part of this patchset and IMHO implementing something which
may happen in the future is not the way we should go.

Regards,
  Marco

Re: [PATCH v2 3/4] iio: temperature: tmp117: add TI TMP116 support

[Jonathan Cameron]

On Fri, 23 Dec 2022 17:13:59 +0100
Marco Felsch <m.felsch@pengutronix.de> wrote:

> On 22-12-23, Jonathan Cameron wrote:
> 
> ...
> 
> > > > > @@ -118,27 +127,28 @@ static int tmp117_identify(struct i2c_client *client)
> > > > >  	int dev_id;
> > > > >  
> > > > >  	dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID);
> > > > > -	if (dev_id < 0)    
> > > > 
> > > > Keep this handling of the smbus read returning an error.
> > > > Otherwise, you end up replacing the error code with -ENODEV rather than
> > > > returning what actually happened.
> > > > 
> > > > 	if (dev_id < 0)
> > > > 		return dev_id;    
> > > 
> > > You're right, I will change this thanks.
> > >   
> > > > 	switch (dev_id) {
> > > > ...
> > > >     
> > > > > +	switch (dev_id) {
> > > > > +	case TMP116_DEVICE_ID:
> > > > > +	case TMP117_DEVICE_ID:
> > > > >  		return dev_id;
> > > > > -	if (dev_id != TMP117_DEVICE_ID) {
> > > > > -		dev_err(&client->dev, "TMP117 not found\n");
> > > > > +	default:
> > > > > +		dev_err(&client->dev, "TMP116/117 not found\n");
> > > > >  		return -ENODEV;  
> >
> > As per the other branch of this thread.  This isn't an error.
> > If we want fallback compatibles to work in their role of allowing
> > for newer devices that are actually compatible, the most we should
> > do here is warn.
> > 
> > Say a new tmp117b device is released. It's fully backwards compatible
> > with the exception of an ID - or supports only new features + backwards
> > compatibility then that would have a fallback to tmp117 and we need
> > it to work.  
> 
> This isn't part of this patchset and IMHO implementing something which
> may happen in the future is not the way we should go.

I held a similar view, but the response I got from the DT maintainers was
that a driver should not reject a DTS that says it is compatible based
on an unknown ID - because it prevents that case of an old kernel working
absolutely fine with a completely compatible newer part.

Jonathan


> 
> Regards,
>   Marco

Re: [PATCH v2 3/4] iio: temperature: tmp117: add TI TMP116 support

[Krzysztof Kozlowski]

On 23/12/2022 18:16, Jonathan Cameron wrote:
> On Fri, 23 Dec 2022 17:13:59 +0100
> Marco Felsch <m.felsch@pengutronix.de> wrote:
> 
>> On 22-12-23, Jonathan Cameron wrote:
>>
>> ...
>>
>>>>>> @@ -118,27 +127,28 @@ static int tmp117_identify(struct i2c_client *client)
>>>>>>  	int dev_id;
>>>>>>  
>>>>>>  	dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID);
>>>>>> -	if (dev_id < 0)    
>>>>>
>>>>> Keep this handling of the smbus read returning an error.
>>>>> Otherwise, you end up replacing the error code with -ENODEV rather than
>>>>> returning what actually happened.
>>>>>
>>>>> 	if (dev_id < 0)
>>>>> 		return dev_id;    
>>>>
>>>> You're right, I will change this thanks.
>>>>   
>>>>> 	switch (dev_id) {
>>>>> ...
>>>>>     
>>>>>> +	switch (dev_id) {
>>>>>> +	case TMP116_DEVICE_ID:
>>>>>> +	case TMP117_DEVICE_ID:
>>>>>>  		return dev_id;
>>>>>> -	if (dev_id != TMP117_DEVICE_ID) {
>>>>>> -		dev_err(&client->dev, "TMP117 not found\n");
>>>>>> +	default:
>>>>>> +		dev_err(&client->dev, "TMP116/117 not found\n");
>>>>>>  		return -ENODEV;  
>>>
>>> As per the other branch of this thread.  This isn't an error.
>>> If we want fallback compatibles to work in their role of allowing
>>> for newer devices that are actually compatible, the most we should
>>> do here is warn.
>>>
>>> Say a new tmp117b device is released. It's fully backwards compatible
>>> with the exception of an ID - or supports only new features + backwards
>>> compatibility then that would have a fallback to tmp117 and we need
>>> it to work.  
>>
>> This isn't part of this patchset and IMHO implementing something which
>> may happen in the future is not the way we should go.
> 
> I held a similar view, but the response I got from the DT maintainers was
> that a driver should not reject a DTS that says it is compatible based
> on an unknown ID - because it prevents that case of an old kernel working
> absolutely fine with a completely compatible newer part.

I don't think that there was such generic recommendation. Accepting
unknown devices (unknown register IDs) is a risk - device might behave
correct or not. If it is a critical device, like regulator, misbehave
might damage something.

What's more, how Linux driver behaves on device IDs (not compatibles) is
also a bit outside of DT scope.

If a driver claims it handles compatibles tmp117, then indeed it should
work fine with any DTS node claiming to be compatible with tmp117.
However driver is free to make further checks (if possible) whether the
device (e.g. tmp116 or tmp11X) is really compatible and reject unknown
devices for safety reason.

The same as x86 kernel is fine to reject to work on newest (unknown) x86
processors for safety reasons... which is terrible from user-experience
point of view unless it is real safety case.

Best regards,
Krzysztof

Re: [PATCH v2 3/4] iio: temperature: tmp117: add TI TMP116 support

[Jonathan Cameron]

On Tue, 27 Dec 2022 09:30:08 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 23/12/2022 18:16, Jonathan Cameron wrote:
> > On Fri, 23 Dec 2022 17:13:59 +0100
> > Marco Felsch <m.felsch@pengutronix.de> wrote:
> >   
> >> On 22-12-23, Jonathan Cameron wrote:
> >>
> >> ...
> >>  
> >>>>>> @@ -118,27 +127,28 @@ static int tmp117_identify(struct i2c_client *client)
> >>>>>>  	int dev_id;
> >>>>>>  
> >>>>>>  	dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID);
> >>>>>> -	if (dev_id < 0)      
> >>>>>
> >>>>> Keep this handling of the smbus read returning an error.
> >>>>> Otherwise, you end up replacing the error code with -ENODEV rather than
> >>>>> returning what actually happened.
> >>>>>
> >>>>> 	if (dev_id < 0)
> >>>>> 		return dev_id;      
> >>>>
> >>>> You're right, I will change this thanks.
> >>>>     
> >>>>> 	switch (dev_id) {
> >>>>> ...
> >>>>>       
> >>>>>> +	switch (dev_id) {
> >>>>>> +	case TMP116_DEVICE_ID:
> >>>>>> +	case TMP117_DEVICE_ID:
> >>>>>>  		return dev_id;
> >>>>>> -	if (dev_id != TMP117_DEVICE_ID) {
> >>>>>> -		dev_err(&client->dev, "TMP117 not found\n");
> >>>>>> +	default:
> >>>>>> +		dev_err(&client->dev, "TMP116/117 not found\n");
> >>>>>>  		return -ENODEV;    
> >>>
> >>> As per the other branch of this thread.  This isn't an error.
> >>> If we want fallback compatibles to work in their role of allowing
> >>> for newer devices that are actually compatible, the most we should
> >>> do here is warn.
> >>>
> >>> Say a new tmp117b device is released. It's fully backwards compatible
> >>> with the exception of an ID - or supports only new features + backwards
> >>> compatibility then that would have a fallback to tmp117 and we need
> >>> it to work.    
> >>
> >> This isn't part of this patchset and IMHO implementing something which
> >> may happen in the future is not the way we should go.  
> > 
> > I held a similar view, but the response I got from the DT maintainers was
> > that a driver should not reject a DTS that says it is compatible based
> > on an unknown ID - because it prevents that case of an old kernel working
> > absolutely fine with a completely compatible newer part.  
> 
> I don't think that there was such generic recommendation. Accepting
> unknown devices (unknown register IDs) is a risk - device might behave
> correct or not. If it is a critical device, like regulator, misbehave
> might damage something.

Agreed - I didn't express that there are limits to such a requirement.
Indeed not a good idea with regulators etc!  However, for input devices
like this one things are a little simpler - in theory they could be used
for something that ends up damaging hardware if done wrong, but it's much
less likely.

> 
> What's more, how Linux driver behaves on device IDs (not compatibles) is
> also a bit outside of DT scope.
> 
> If a driver claims it handles compatibles tmp117, then indeed it should
> work fine with any DTS node claiming to be compatible with tmp117.
> However driver is free to make further checks (if possible) whether the
> device (e.g. tmp116 or tmp11X) is really compatible and reject unknown
> devices for safety reason.

Ok. For input devices at least in IIO we went around this a few times and
ended up with deciding that a dev_info() type message was the best balance.
We will need to be more careful for output devices.

> 
> The same as x86 kernel is fine to reject to work on newest (unknown) x86
> processors for safety reasons... which is terrible from user-experience
> point of view unless it is real safety case.

Hopefully that never happens :)

Jonathan

> 
> Best regards,
> Krzysztof
> 

[PATCH v2 4/4] iio: temperature: tmp117: cosmetic alignment cleanup

[Marco Felsch]

Align the code correctly if possible and align the channel bit mask to
make it easier to read.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v2:
- no changes

 drivers/iio/temperature/tmp117.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/temperature/tmp117.c b/drivers/iio/temperature/tmp117.c
index 468dafa6fa8e..df0ee1231975 100644
--- a/drivers/iio/temperature/tmp117.c
+++ b/drivers/iio/temperature/tmp117.c
@@ -42,8 +42,8 @@ struct tmp117_data {
 };
 
 static int tmp117_read_raw(struct iio_dev *indio_dev,
-		struct iio_chan_spec const *channel, int *val,
-		int *val2, long mask)
+			   struct iio_chan_spec const *channel, int *val,
+			   int *val2, long mask)
 {
 	struct tmp117_data *data = iio_priv(indio_dev);
 	s32 ret;
@@ -51,7 +51,7 @@ static int tmp117_read_raw(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 		ret = i2c_smbus_read_word_swapped(data->client,
-						TMP117_REG_TEMP);
+						  TMP117_REG_TEMP);
 		if (ret < 0)
 			return ret;
 		*val = sign_extend32(ret, 15);
@@ -59,7 +59,7 @@ static int tmp117_read_raw(struct iio_dev *indio_dev,
 
 	case IIO_CHAN_INFO_CALIBBIAS:
 		ret = i2c_smbus_read_word_swapped(data->client,
-					TMP117_REG_TEMP_OFFSET);
+						  TMP117_REG_TEMP_OFFSET);
 		if (ret < 0)
 			return ret;
 		*val = sign_extend32(ret, 15);
@@ -81,9 +81,8 @@ static int tmp117_read_raw(struct iio_dev *indio_dev,
 	}
 }
 
-static int tmp117_write_raw(struct iio_dev *indio_dev,
-		struct iio_chan_spec const *channel, int val,
-		int val2, long mask)
+static int tmp117_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec
+			    const *channel, int val, int val2, long mask)
 {
 	struct tmp117_data *data = iio_priv(indio_dev);
 	s16 off;
@@ -106,7 +105,9 @@ static const struct iio_chan_spec tmp117_channels[] = {
 	{
 		.type = IIO_TEMP,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
-			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_SCALE),
+				      BIT(IIO_CHAN_INFO_CALIBBIAS) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+	},
 };
 
 static const struct iio_chan_spec tmp116_channels[] = {
-- 
2.30.2