[PATCH v4 0/2] Maxim MAX1241 driver

[PATCH v4 0/2] Maxim MAX1241 driver

[Alexandru Lazar]

Hello again,

Here's version 4 of a patch series which adds support for the Maxim
MAX1241, a 12-bit, single-channel, SPI-connected ADC.

Changelog so far:

v4:

* Dropped explicit documentation of SPI reg property
* Reordered patch series so that dt bindings come first

v3:

* Fixed silly copy-paste error in Kconfig description

v2:

* Removed useeless header includes
* Dropped needlessly verbose stuff in _read and _probe functions
* Dropped useless GPL notice
* Lowered log level of shdn pin status in probe function, now it's
  dev_dbg
* Added proper error checking for the GPIO shutdown pin
* remove now always returns zero (man, I've been wrong about this for
  *years* now...)
* Added regulator disable action, cleanup is now handled via devm
* Drop delay_usecs, use delay.value, delay.unit
* Drop config_of, of_match_ptr call
* Dropped IIO_BUFFER, IIO_TRIGGERED_BUFFER dependencies, set SPI_MASTER
  as dependency, fix indenting.
* DT binding: use correct id, add reg description (looks pretty
  standard), dropped spi-max-frequency, fixed dt_binding_check
  complaints (oops!)

Apologies for the last botched message -- my machine died at the
wrongest possible time.

All the best,
Alex

Alexandru Lazar (2):
  dt-bindings: iio: adc: Add MAX1241 device tree bindings in
    documentation
  iio: adc: Add MAX1241 driver

 .../bindings/iio/adc/maxim,max1241.yaml       |  61 ++++++
 drivers/iio/adc/Kconfig                       |  10 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/max1241.c                     | 206 ++++++++++++++++++
 4 files changed, 278 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
 create mode 100644 drivers/iio/adc/max1241.c

-- 
2.25.2

[PATCH v4 1/2] dt-bindings: iio: adc: Add MAX1241 device tree bindings in documentation

[Alexandru Lazar]

Add device-tree bindings documentation for the MAX1241 device driver.

Signed-off-by: Alexandru Lazar <alazar@startmail.com>
---
 .../bindings/iio/adc/maxim,max1241.yaml       | 61 +++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
new file mode 100644
index 000000000000..de41d422ce3b
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
@@ -0,0 +1,61 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright 2020 Ioan-Alexandru Lazar
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/maxim,max1241.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Maxim MAX1241 12-bit, single-channel analog to digital converter
+
+maintainers:
+  - Ioan-Alexandru Lazar <alazar@startmail.com>
+
+description: |
+  Bindings for the max1241 12-bit, single-channel ADC device. This
+  driver supports voltage reading and can optionally be configured for
+  power-down mode operation. The datasheet can be found at:
+    https://datasheets.maximintegrated.com/en/ds/MAX1240-MAX1241.pdf
+
+properties:
+  compatible:
+    enum:
+      - maxim,max1241
+
+  reg:
+    maxItems: 1
+
+  vref-supply:
+    description:
+      Device tree identifier of the regulator that provides the external
+      reference voltage.
+    maxItems: 1
+
+  shdn-gpios:
+    description:
+      GPIO spec for the GPIO pin connected to the ADC's /SHDN pin. If
+      specified, the /SHDN pin will be asserted between conversions,
+      thus enabling power-down mode.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - vref-supply
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    spi0 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+        adc@0 {
+            compatible = "maxim,max1241";
+            reg = <0>;
+            vref-supply = <&vdd_3v3_reg>;
+            spi-max-frequency = <1000000>;
+            shdn-gpios = <&gpio 26 1>;
+        };
+    };
+
+
-- 
2.25.2

Re: [PATCH v4 1/2] dt-bindings: iio: adc: Add MAX1241 device tree bindings in documentation

[Jonathan Cameron]

On Fri, 20 Mar 2020 17:01:14 +0200
Alexandru Lazar <alazar@startmail.com> wrote:

> Add device-tree bindings documentation for the MAX1241 device driver.
> 
> Signed-off-by: Alexandru Lazar <alazar@startmail.com>

Please consider also adding the vdd-supply.
It's not really required, but if you don't add it from the start
chances are high that at some point someone else will need to
add it.

One trivial thing inline.  Otherwise looks good to me.

> ---
>  .../bindings/iio/adc/maxim,max1241.yaml       | 61 +++++++++++++++++++
>  1 file changed, 61 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> new file mode 100644
> index 000000000000..de41d422ce3b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> @@ -0,0 +1,61 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright 2020 Ioan-Alexandru Lazar
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/maxim,max1241.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Maxim MAX1241 12-bit, single-channel analog to digital converter
> +
> +maintainers:
> +  - Ioan-Alexandru Lazar <alazar@startmail.com>
> +
> +description: |
> +  Bindings for the max1241 12-bit, single-channel ADC device. This
> +  driver supports voltage reading and can optionally be configured for

Driver shouldn't be mentioned in the binding. It's a description of the
hardware only.

> +  power-down mode operation. The datasheet can be found at:
> +    https://datasheets.maximintegrated.com/en/ds/MAX1240-MAX1241.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - maxim,max1241
> +
> +  reg:
> +    maxItems: 1
> +
> +  vref-supply:
> +    description:
> +      Device tree identifier of the regulator that provides the external
> +      reference voltage.
> +    maxItems: 1
> +
> +  shdn-gpios:
> +    description:
> +      GPIO spec for the GPIO pin connected to the ADC's /SHDN pin. If
> +      specified, the /SHDN pin will be asserted between conversions,
> +      thus enabling power-down mode.
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - vref-supply
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    spi0 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +        adc@0 {
> +            compatible = "maxim,max1241";
> +            reg = <0>;
> +            vref-supply = <&vdd_3v3_reg>;
> +            spi-max-frequency = <1000000>;
> +            shdn-gpios = <&gpio 26 1>;
> +        };
> +    };
> +
> +

Re: [PATCH v4 1/2] dt-bindings: iio: adc: Add MAX1241 device tree bindings in documentation

[Alexandru Lazar]

Hi Jonathan,

> Please consider also adding the vdd-supply.
> It's not really required, but if you don't add it from the start
> chances are high that at some point someone else will need to
> add it.

Sorry if I'm missing something obvious here -- what vdd-supply is
that? Are you thinking of the regulator used for the ADC's reference
voltage? That's already there (vref-supply).

Or did you mean I should add a definition for the regulator output used
for the device's Vdd input (i.e. the positive supply voltage)? Needless
to say, I'm happy to add it if you think it's a good idea. It's just I
don't think I've seen it in other drivers (except maybe ad7192?) -- so I
figured I'd ask before sending a botched v5.

Best regards,
Alex

> 
> One trivial thing inline.  Otherwise looks good to me.
> 
> > ---
> >  .../bindings/iio/adc/maxim,max1241.yaml       | 61 +++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> > new file mode 100644
> > index 000000000000..de41d422ce3b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> > @@ -0,0 +1,61 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright 2020 Ioan-Alexandru Lazar
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/maxim,max1241.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Maxim MAX1241 12-bit, single-channel analog to digital converter
> > +
> > +maintainers:
> > +  - Ioan-Alexandru Lazar <alazar@startmail.com>
> > +
> > +description: |
> > +  Bindings for the max1241 12-bit, single-channel ADC device. This
> > +  driver supports voltage reading and can optionally be configured for
> 
> Driver shouldn't be mentioned in the binding. It's a description of the
> hardware only.
> 
> > +  power-down mode operation. The datasheet can be found at:
> > +    https://datasheets.maximintegrated.com/en/ds/MAX1240-MAX1241.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - maxim,max1241
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vref-supply:
> > +    description:
> > +      Device tree identifier of the regulator that provides the external
> > +      reference voltage.
> > +    maxItems: 1
> > +
> > +  shdn-gpios:
> > +    description:
> > +      GPIO spec for the GPIO pin connected to the ADC's /SHDN pin. If
> > +      specified, the /SHDN pin will be asserted between conversions,
> > +      thus enabling power-down mode.
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - vref-supply
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    spi0 {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +        adc@0 {
> > +            compatible = "maxim,max1241";
> > +            reg = <0>;
> > +            vref-supply = <&vdd_3v3_reg>;
> > +            spi-max-frequency = <1000000>;
> > +            shdn-gpios = <&gpio 26 1>;
> > +        };
> > +    };
> > +
> > +
> 

Re: [PATCH v4 1/2] dt-bindings: iio: adc: Add MAX1241 device tree bindings in documentation

[Ardelean, Alexandru]

On Sat, 2020-03-21 at 21:35 +0200, Alexandru Lazar wrote:
> Hi Jonathan,
> 
> > Please consider also adding the vdd-supply.
> > It's not really required, but if you don't add it from the start
> > chances are high that at some point someone else will need to
> > add it.
> 
> Sorry if I'm missing something obvious here -- what vdd-supply is
> that? Are you thinking of the regulator used for the ADC's reference
> voltage? That's already there (vref-supply).
> 
> Or did you mean I should add a definition for the regulator output used
> for the device's Vdd input (i.e. the positive supply voltage)? Needless
> to say, I'm happy to add it if you think it's a good idea. It's just I
> don't think I've seen it in other drivers (except maybe ad7192?) -- so I
> figured I'd ask before sending a botched v5.
> 

Yep.
Jonathan refers to Vdd input/pin [on the chip] which is different from Vref [REF
pin].
Not all drivers define Vdd.
Some call it AVdd.

You can check via:  git grep -i vdd | cut -d: -f1 | sort | uniq -c
in the drivers/iio folder 

It's an idea to add it, and that can give control to the driver to power-up the
ADC, by defining a regulator [vdd-supply] in the device-tree.

Maybe it could be interesting to move this to the IIO core as an option-flag.
[But that's another discussion]

> Best regards,
> Alex
> 
> > One trivial thing inline.  Otherwise looks good to me.
> > 
> > > ---
> > >  .../bindings/iio/adc/maxim,max1241.yaml       | 61 +++++++++++++++++++
> > >  1 file changed, 61 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> > > b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> > > new file mode 100644
> > > index 000000000000..de41d422ce3b
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> > > @@ -0,0 +1,61 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright 2020 Ioan-Alexandru Lazar
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iio/adc/maxim,max1241.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Maxim MAX1241 12-bit, single-channel analog to digital converter
> > > +
> > > +maintainers:
> > > +  - Ioan-Alexandru Lazar <alazar@startmail.com>
> > > +
> > > +description: |
> > > +  Bindings for the max1241 12-bit, single-channel ADC device. This
> > > +  driver supports voltage reading and can optionally be configured for
> > 
> > Driver shouldn't be mentioned in the binding. It's a description of the
> > hardware only.
> > 
> > > +  power-down mode operation. The datasheet can be found at:
> > > +    https://datasheets.maximintegrated.com/en/ds/MAX1240-MAX1241.pdf
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - maxim,max1241
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  vref-supply:
> > > +    description:
> > > +      Device tree identifier of the regulator that provides the external
> > > +      reference voltage.
> > > +    maxItems: 1
> > > +
> > > +  shdn-gpios:
> > > +    description:
> > > +      GPIO spec for the GPIO pin connected to the ADC's /SHDN pin. If
> > > +      specified, the /SHDN pin will be asserted between conversions,
> > > +      thus enabling power-down mode.
> > > +    maxItems: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - vref-supply
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/gpio/gpio.h>
> > > +    spi0 {
> > > +      #address-cells = <1>;
> > > +      #size-cells = <0>;
> > > +
> > > +        adc@0 {
> > > +            compatible = "maxim,max1241";
> > > +            reg = <0>;
> > > +            vref-supply = <&vdd_3v3_reg>;
> > > +            spi-max-frequency = <1000000>;
> > > +            shdn-gpios = <&gpio 26 1>;
> > > +        };
> > > +    };
> > > +
> > > +

Re: [PATCH v4 1/2] dt-bindings: iio: adc: Add MAX1241 device tree bindings in documentation

[Alexandru Lazar]

> Yep.
> Jonathan refers to Vdd input/pin [on the chip] which is different from Vref [REF
> pin].
> Not all drivers define Vdd.
> Some call it AVdd.
> 
> [...]
> 
> It's an idea to add it, and that can give control to the driver to power-up the
> ADC, by defining a regulator [vdd-supply] in the device-tree.

Hmm... I don't know how useful this would be for the 124x family (I
doubt anyone who needs one of these will power it from its own,
independent supply), but it's a pretty harmless change. I can't think of
any reason to say no :-).

Thanks,
Alex

Re: [PATCH v4 1/2] dt-bindings: iio: adc: Add MAX1241 device tree bindings in documentation

[Jonathan Cameron]

On Sun, 22 Mar 2020 11:53:17 +0200
Alexandru Lazar <alazar@startmail.com> wrote:

> > Yep.
> > Jonathan refers to Vdd input/pin [on the chip] which is different from Vref [REF
> > pin].
> > Not all drivers define Vdd.
> > Some call it AVdd.
> > 
> > [...]
> > 
> > It's an idea to add it, and that can give control to the driver to power-up the
> > ADC, by defining a regulator [vdd-supply] in the device-tree.  
> 
> Hmm... I don't know how useful this would be for the 124x family (I
> doubt anyone who needs one of these will power it from its own,
> independent supply), 

You'd be surprised how often this gets added to drivers precisely because
people will put it on a controllable supply.  It may well not have it's own
supply but it may share one with a bunch of other external chips and
all of them need to use the regulator framework controls to make sure it's
only disabled when they are all suspended etc.

See the number of times Linus Walleij has added this for various sensors
and ADCs because he has boards where the control is needed.

Jonathan


> but it's a pretty harmless change. I can't think of
> any reason to say no :-).
> 
> Thanks,
> Alex

Re: [PATCH v4 1/2] dt-bindings: iio: adc: Add MAX1241 device tree bindings in documentation

[Alexandru Lazar]

> You'd be surprised how often this gets added to drivers precisely because
> people will put it on a controllable supply.  It may well not have it's own
> supply but it may share one with a bunch of other external chips and
> all of them need to use the regulator framework controls to make sure it's
> only disabled when they are all suspended etc.

I figured it might be something like this :-). I've added the vdd-supply
binding in v5.

If this isn't something that can be easily handled in the core, do you
think we can document it somewhere as a convention/common idiom?
(Assuming it's not already documented, of course). It seems like it's
something that all IIO devices would need. I can do the writing part.

Thanks,
Alex

Re: [PATCH v4 1/2] dt-bindings: iio: adc: Add MAX1241 device tree bindings in documentation

[Jonathan Cameron]

On Sun, 22 Mar 2020 18:06:04 +0200
Alexandru Lazar <alazar@startmail.com> wrote:

> > You'd be surprised how often this gets added to drivers precisely because
> > people will put it on a controllable supply.  It may well not have it's own
> > supply but it may share one with a bunch of other external chips and
> > all of them need to use the regulator framework controls to make sure it's
> > only disabled when they are all suspended etc.  
> 
> I figured it might be something like this :-). I've added the vdd-supply
> binding in v5.
> 
> If this isn't something that can be easily handled in the core, do you
> think we can document it somewhere as a convention/common idiom?
> (Assuming it's not already documented, of course). It seems like it's
> something that all IIO devices would need. I can do the writing part.

Hmm. We could do with a sort of 'things you'd normally find in a driver'
document.  We don't have such a document, but interesting to think about
what would be in it...  Perhaps a 'best practice' document would
be a better way of putting it.  I don't really want to see a huge
number of patches adding regulators to drivers that don't have them already
for example.  Clearly no one needed them yet :)

If you want to take a stab at such a document that would be great.

Jonathan


> 
> Thanks,
> Alex

Re: [PATCH v4 1/2] dt-bindings: iio: adc: Add MAX1241 device tree bindings in documentation

[Jonathan Cameron]

On Sun, 22 Mar 2020 09:02:15 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Sat, 2020-03-21 at 21:35 +0200, Alexandru Lazar wrote:
> > Hi Jonathan,
> >   
> > > Please consider also adding the vdd-supply.
> > > It's not really required, but if you don't add it from the start
> > > chances are high that at some point someone else will need to
> > > add it.  
> > 
> > Sorry if I'm missing something obvious here -- what vdd-supply is
> > that? Are you thinking of the regulator used for the ADC's reference
> > voltage? That's already there (vref-supply).
> > 
> > Or did you mean I should add a definition for the regulator output used
> > for the device's Vdd input (i.e. the positive supply voltage)? Needless
> > to say, I'm happy to add it if you think it's a good idea. It's just I
> > don't think I've seen it in other drivers (except maybe ad7192?) -- so I
> > figured I'd ask before sending a botched v5.
> >   
> 
> Yep.
> Jonathan refers to Vdd input/pin [on the chip] which is different from Vref [REF
> pin].
> Not all drivers define Vdd.
> Some call it AVdd.
> 
> You can check via:  git grep -i vdd | cut -d: -f1 | sort | uniq -c
> in the drivers/iio folder 
> 
> It's an idea to add it, and that can give control to the driver to power-up the
> ADC, by defining a regulator [vdd-supply] in the device-tree.
> 
> Maybe it could be interesting to move this to the IIO core as an option-flag.
> [But that's another discussion]

Can't easily move it to the core because how you actually handle it is very
much driver dependent.  A devices that has state will keep it on most of
the time, but other drivers will handle it in runtime pm if the device
starts up quickly enough.

We want this to be visible in individual drivers, even if it would be
less code perhaps in the core.

Jonathan

> 
> > Best regards,
> > Alex
> >   
> > > One trivial thing inline.  Otherwise looks good to me.
> > >   
> > > > ---
> > > >  .../bindings/iio/adc/maxim,max1241.yaml       | 61 +++++++++++++++++++
> > > >  1 file changed, 61 insertions(+)
> > > >  create mode 100644
> > > > Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> > > > b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> > > > new file mode 100644
> > > > index 000000000000..de41d422ce3b
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> > > > @@ -0,0 +1,61 @@
> > > > +# SPDX-License-Identifier: GPL-2.0
> > > > +# Copyright 2020 Ioan-Alexandru Lazar
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/iio/adc/maxim,max1241.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Maxim MAX1241 12-bit, single-channel analog to digital converter
> > > > +
> > > > +maintainers:
> > > > +  - Ioan-Alexandru Lazar <alazar@startmail.com>
> > > > +
> > > > +description: |
> > > > +  Bindings for the max1241 12-bit, single-channel ADC device. This
> > > > +  driver supports voltage reading and can optionally be configured for  
> > > 
> > > Driver shouldn't be mentioned in the binding. It's a description of the
> > > hardware only.
> > >   
> > > > +  power-down mode operation. The datasheet can be found at:
> > > > +    https://datasheets.maximintegrated.com/en/ds/MAX1240-MAX1241.pdf
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - maxim,max1241
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  vref-supply:
> > > > +    description:
> > > > +      Device tree identifier of the regulator that provides the external
> > > > +      reference voltage.
> > > > +    maxItems: 1
> > > > +
> > > > +  shdn-gpios:
> > > > +    description:
> > > > +      GPIO spec for the GPIO pin connected to the ADC's /SHDN pin. If
> > > > +      specified, the /SHDN pin will be asserted between conversions,
> > > > +      thus enabling power-down mode.
> > > > +    maxItems: 1
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - vref-supply
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    #include <dt-bindings/gpio/gpio.h>
> > > > +    spi0 {
> > > > +      #address-cells = <1>;
> > > > +      #size-cells = <0>;
> > > > +
> > > > +        adc@0 {
> > > > +            compatible = "maxim,max1241";
> > > > +            reg = <0>;
> > > > +            vref-supply = <&vdd_3v3_reg>;
> > > > +            spi-max-frequency = <1000000>;
> > > > +            shdn-gpios = <&gpio 26 1>;
> > > > +        };
> > > > +    };
> > > > +
> > > > +  

Re: [PATCH v4 1/2] dt-bindings: iio: adc: Add MAX1241 device tree bindings in documentation

[Rob Herring]

On Fri, Mar 20, 2020 at 05:01:14PM +0200, Alexandru Lazar wrote:
> Add device-tree bindings documentation for the MAX1241 device driver.
> 
> Signed-off-by: Alexandru Lazar <alazar@startmail.com>
> ---
>  .../bindings/iio/adc/maxim,max1241.yaml       | 61 +++++++++++++++++++
>  1 file changed, 61 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> new file mode 100644
> index 000000000000..de41d422ce3b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max1241.yaml
> @@ -0,0 +1,61 @@
> +# SPDX-License-Identifier: GPL-2.0

Dual license new bindings please:

(GPL-2.0-only OR BSD-2-Clause)

> +# Copyright 2020 Ioan-Alexandru Lazar
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/maxim,max1241.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Maxim MAX1241 12-bit, single-channel analog to digital converter
> +
> +maintainers:
> +  - Ioan-Alexandru Lazar <alazar@startmail.com>
> +
> +description: |
> +  Bindings for the max1241 12-bit, single-channel ADC device. This
> +  driver supports voltage reading and can optionally be configured for
> +  power-down mode operation. The datasheet can be found at:
> +    https://datasheets.maximintegrated.com/en/ds/MAX1240-MAX1241.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - maxim,max1241
> +
> +  reg:
> +    maxItems: 1
> +
> +  vref-supply:
> +    description:
> +      Device tree identifier of the regulator that provides the external
> +      reference voltage.
> +    maxItems: 1

Drop this. Supplies are always 1 item.

> +
> +  shdn-gpios:

shutdown-gpios is the semi standard name.

> +    description:
> +      GPIO spec for the GPIO pin connected to the ADC's /SHDN pin. If
> +      specified, the /SHDN pin will be asserted between conversions,
> +      thus enabling power-down mode.
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - vref-supply
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    spi0 {

Just 'spi'

> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +        adc@0 {
> +            compatible = "maxim,max1241";
> +            reg = <0>;
> +            vref-supply = <&vdd_3v3_reg>;
> +            spi-max-frequency = <1000000>;
> +            shdn-gpios = <&gpio 26 1>;
> +        };
> +    };
> +
> +
> -- 
> 2.25.2
> 

[PATCH v4 2/2] iio: adc: Add MAX1241 driver

[Alexandru Lazar]

Add driver for the Maxim MAX1241 12-bit, single-channel ADC. The driver
includes support for this device's low-power operation mode.

Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Signed-off-by: Alexandru Lazar <alazar@startmail.com>
---
 drivers/iio/adc/Kconfig   |  10 ++
 drivers/iio/adc/Makefile  |   1 +
 drivers/iio/adc/max1241.c | 206 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 217 insertions(+)
 create mode 100644 drivers/iio/adc/max1241.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 5d8540b7b427..55f6462cd93f 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -566,6 +566,16 @@ config MAX1118
 	  To compile this driver as a module, choose M here: the module will be
 	  called max1118.
 
+config MAX1241
+	tristate "Maxim max1241 ADC driver"
+	depends on SPI_MASTER
+	help
+	  Say yes here to build support for Maxim max1241 12-bit, single-channel
+	  ADC.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called max1241.
+
 config MAX1363
 	tristate "Maxim max1363 ADC driver"
 	depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index a1f1fbec0f87..37d6f17559dc 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -54,6 +54,7 @@ obj-$(CONFIG_LTC2497) += ltc2497.o
 obj-$(CONFIG_MAX1027) += max1027.o
 obj-$(CONFIG_MAX11100) += max11100.o
 obj-$(CONFIG_MAX1118) += max1118.o
+obj-$(CONFIG_MAX1241) += max1241.o
 obj-$(CONFIG_MAX1363) += max1363.o
 obj-$(CONFIG_MAX9611) += max9611.o
 obj-$(CONFIG_MCP320X) += mcp320x.o
diff --git a/drivers/iio/adc/max1241.c b/drivers/iio/adc/max1241.c
new file mode 100644
index 000000000000..0278510ec346
--- /dev/null
+++ b/drivers/iio/adc/max1241.c
@@ -0,0 +1,206 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MAX1241 low-power, 12-bit serial ADC
+ *
+ * Datasheet: https://datasheets.maximintegrated.com/en/ds/MAX1240-MAX1241.pdf
+ */
+
+#include <linux/regulator/consumer.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+
+#define MAX1241_VAL_MASK 0xFFF
+#define MAX1241_SHDN_DELAY_USEC 4
+
+enum max1241_id {
+	max1241,
+};
+
+struct max1241 {
+	struct spi_device *spi;
+	struct mutex lock;
+	struct regulator *reg;
+	struct gpio_desc *shdn;
+
+	__be16 data ____cacheline_aligned;
+};
+
+static const struct iio_chan_spec max1241_channels[] = {
+	{
+		.type = IIO_VOLTAGE,
+		.indexed = 1,
+		.channel = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 12,
+			.storagebits = 16,
+		},
+	},
+};
+
+static int max1241_read(struct max1241 *adc)
+{
+	struct spi_transfer xfers[] = {
+		/*
+		 * Begin conversion by bringing /CS low for at least
+		 * tconv us.
+		 */
+		{
+			.len = 0,
+			.delay.value = 8,
+			.delay.unit = SPI_DELAY_UNIT_USECS,
+		},
+		/*
+		 * Then read two bytes of data in our RX buffer.
+		 */
+		{
+			.rx_buf = &adc->data,
+			.len = 2,
+		},
+	};
+
+	return spi_sync_transfer(adc->spi, xfers, ARRAY_SIZE(xfers));
+}
+
+static int max1241_read_raw(struct iio_dev *indio_dev,
+			struct iio_chan_spec const *chan,
+			int *val, int *val2, long mask)
+{
+	int ret, vref_uV;
+	struct max1241 *adc = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&adc->lock);
+
+		if (adc->shdn) {
+			gpiod_set_value(adc->shdn, 0);
+			udelay(MAX1241_SHDN_DELAY_USEC);
+		}
+
+		ret = max1241_read(adc);
+
+		if (adc->shdn)
+			gpiod_set_value(adc->shdn, 1);
+
+		if (ret) {
+			mutex_unlock(&adc->lock);
+			return ret;
+		}
+
+		*val = (be16_to_cpu(adc->data) >> 3) & MAX1241_VAL_MASK;
+
+		mutex_unlock(&adc->lock);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		vref_uV = regulator_get_voltage(adc->reg);
+
+		if (vref_uV < 0)
+			return vref_uV;
+
+		*val = vref_uV / 1000;
+		*val2 = 12;
+
+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info max1241_info = {
+	.read_raw = max1241_read_raw,
+};
+
+static void max1241_disable_reg_action(void *data)
+{
+	struct max1241 *adc = data;
+	int err;
+
+	err = regulator_disable(adc->reg);
+	if (err)
+		dev_err(&adc->spi->dev, "could not disable reference regulator.\n");
+}
+
+static int max1241_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct max1241 *adc;
+	int ret = 0;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	adc = iio_priv(indio_dev);
+	adc->spi = spi;
+	mutex_init(&adc->lock);
+
+	spi_set_drvdata(spi, indio_dev);
+
+	adc->reg = devm_regulator_get(&spi->dev, "vref");
+	if (IS_ERR(adc->reg)) {
+		dev_err(&spi->dev, "failed to get vref regulator\n");
+		return PTR_ERR(adc->reg);
+	}
+
+	ret = regulator_enable(adc->reg);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(&spi->dev, max1241_disable_reg_action,
+					adc);
+	if (ret) {
+		dev_err(&spi->dev, "could not set up regulator cleanup action!\n");
+		return ret;
+	}
+
+	adc->shdn = devm_gpiod_get_optional(&spi->dev, "shdn", GPIOD_OUT_HIGH);
+
+	if (IS_ERR(adc->shdn))
+		return PTR_ERR(adc->shdn);
+
+	if (!adc->shdn)
+		dev_dbg(&spi->dev, "no shdn pin passed, low-power mode disabled");
+	else
+		dev_dbg(&spi->dev, "shdn pin passed, low-power mode enabled");
+
+	indio_dev->name = spi_get_device_id(spi)->name;
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->info = &max1241_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = max1241_channels;
+	indio_dev->num_channels = ARRAY_SIZE(max1241_channels);
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct spi_device_id max1241_id[] = {
+	{ "max1241", max1241 },
+	{},
+};
+
+static const struct of_device_id max1241_dt_ids[] = {
+	{ .compatible = "maxim,max1241" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, max1241_dt_ids);
+
+static struct spi_driver max1241_spi_driver = {
+	.driver = {
+		.name = "max1241",
+		.of_match_table = max1241_dt_ids,
+	},
+	.probe = max1241_probe,
+	.id_table = max1241_id,
+};
+module_spi_driver(max1241_spi_driver);
+
+MODULE_AUTHOR("Alexandru Lazar <alazar@startmail.com>");
+MODULE_DESCRIPTION("MAX1241 ADC driver");
+MODULE_LICENSE("GPL v2");
-- 
2.25.2

Re: [PATCH v4 2/2] iio: adc: Add MAX1241 driver

[Jonathan Cameron]

On Fri, 20 Mar 2020 17:01:15 +0200
Alexandru Lazar <alazar@startmail.com> wrote:

> Add driver for the Maxim MAX1241 12-bit, single-channel ADC. The driver
> includes support for this device's low-power operation mode.
> 
> Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> Signed-off-by: Alexandru Lazar <alazar@startmail.com>

Few minor things inline, but looks good in general.

> ---
>  drivers/iio/adc/Kconfig   |  10 ++
>  drivers/iio/adc/Makefile  |   1 +
>  drivers/iio/adc/max1241.c | 206 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 217 insertions(+)
>  create mode 100644 drivers/iio/adc/max1241.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 5d8540b7b427..55f6462cd93f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -566,6 +566,16 @@ config MAX1118
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called max1118.
>  
> +config MAX1241
> +	tristate "Maxim max1241 ADC driver"
> +	depends on SPI_MASTER
> +	help
> +	  Say yes here to build support for Maxim max1241 12-bit, single-channel
> +	  ADC.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called max1241.
> +
>  config MAX1363
>  	tristate "Maxim max1363 ADC driver"
>  	depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index a1f1fbec0f87..37d6f17559dc 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -54,6 +54,7 @@ obj-$(CONFIG_LTC2497) += ltc2497.o
>  obj-$(CONFIG_MAX1027) += max1027.o
>  obj-$(CONFIG_MAX11100) += max11100.o
>  obj-$(CONFIG_MAX1118) += max1118.o
> +obj-$(CONFIG_MAX1241) += max1241.o
>  obj-$(CONFIG_MAX1363) += max1363.o
>  obj-$(CONFIG_MAX9611) += max9611.o
>  obj-$(CONFIG_MCP320X) += mcp320x.o
> diff --git a/drivers/iio/adc/max1241.c b/drivers/iio/adc/max1241.c
> new file mode 100644
> index 000000000000..0278510ec346
> --- /dev/null
> +++ b/drivers/iio/adc/max1241.c
> @@ -0,0 +1,206 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * MAX1241 low-power, 12-bit serial ADC
> + *
> + * Datasheet: https://datasheets.maximintegrated.com/en/ds/MAX1240-MAX1241.pdf
> + */
> +
> +#include <linux/regulator/consumer.h>

Nitpick; Tidy up ordering of headers.  If no other reason to have a particular
order go with alphabetical.

> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +
> +#define MAX1241_VAL_MASK 0xFFF
> +#define MAX1241_SHDN_DELAY_USEC 4
> +
> +enum max1241_id {
> +	max1241,
> +};
> +
> +struct max1241 {
> +	struct spi_device *spi;
> +	struct mutex lock;
> +	struct regulator *reg;
> +	struct gpio_desc *shdn;
> +
> +	__be16 data ____cacheline_aligned;
> +};
> +
> +static const struct iio_chan_spec max1241_channels[] = {
> +	{
> +		.type = IIO_VOLTAGE,
> +		.indexed = 1,
> +		.channel = 0,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 12,
> +			.storagebits = 16,
> +		},

No need to specify scan_index or scan_type unless the driver supports
buffered modes.  Nothing will use them until then.

> +	},
> +};
> +
> +static int max1241_read(struct max1241 *adc)
> +{
> +	struct spi_transfer xfers[] = {
> +		/*
> +		 * Begin conversion by bringing /CS low for at least
> +		 * tconv us.
> +		 */
> +		{
> +			.len = 0,
> +			.delay.value = 8,
> +			.delay.unit = SPI_DELAY_UNIT_USECS,
> +		},
> +		/*
> +		 * Then read two bytes of data in our RX buffer.
> +		 */
> +		{
> +			.rx_buf = &adc->data,
> +			.len = 2,
> +		},
> +	};
> +
> +	return spi_sync_transfer(adc->spi, xfers, ARRAY_SIZE(xfers));
> +}
> +
> +static int max1241_read_raw(struct iio_dev *indio_dev,
> +			struct iio_chan_spec const *chan,
> +			int *val, int *val2, long mask)
> +{
> +	int ret, vref_uV;
> +	struct max1241 *adc = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&adc->lock);
> +
> +		if (adc->shdn) {
> +			gpiod_set_value(adc->shdn, 0);
> +			udelay(MAX1241_SHDN_DELAY_USEC);
> +		}
> +
> +		ret = max1241_read(adc);
> +
> +		if (adc->shdn)
> +			gpiod_set_value(adc->shdn, 1);
> +
> +		if (ret) {
> +			mutex_unlock(&adc->lock);
> +			return ret;
> +		}
> +
> +		*val = (be16_to_cpu(adc->data) >> 3) & MAX1241_VAL_MASK;
> +
> +		mutex_unlock(&adc->lock);
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		vref_uV = regulator_get_voltage(adc->reg);
> +
> +		if (vref_uV < 0)
> +			return vref_uV;
> +
> +		*val = vref_uV / 1000;
> +		*val2 = 12;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info max1241_info = {
> +	.read_raw = max1241_read_raw,
> +};
> +
> +static void max1241_disable_reg_action(void *data)
> +{
> +	struct max1241 *adc = data;
> +	int err;
> +
> +	err = regulator_disable(adc->reg);
> +	if (err)
> +		dev_err(&adc->spi->dev, "could not disable reference regulator.\n");
> +}
> +
> +static int max1241_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct max1241 *adc;
> +	int ret = 0;

Looks like ret is always set in all the paths that can use it.
Hence no need to initialize here.

> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	adc = iio_priv(indio_dev);
> +	adc->spi = spi;
> +	mutex_init(&adc->lock);
> +
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	adc->reg = devm_regulator_get(&spi->dev, "vref");
> +	if (IS_ERR(adc->reg)) {
> +		dev_err(&spi->dev, "failed to get vref regulator\n");
> +		return PTR_ERR(adc->reg);
> +	}
> +
> +	ret = regulator_enable(adc->reg);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(&spi->dev, max1241_disable_reg_action,
> +					adc);
> +	if (ret) {
> +		dev_err(&spi->dev, "could not set up regulator cleanup action!\n");
> +		return ret;
> +	}
> +
> +	adc->shdn = devm_gpiod_get_optional(&spi->dev, "shdn", GPIOD_OUT_HIGH);
> +
> +	if (IS_ERR(adc->shdn))
> +		return PTR_ERR(adc->shdn);
> +
> +	if (!adc->shdn)
> +		dev_dbg(&spi->dev, "no shdn pin passed, low-power mode disabled");
> +	else
> +		dev_dbg(&spi->dev, "shdn pin passed, low-power mode enabled");
> +
> +	indio_dev->name = spi_get_device_id(spi)->name;
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->info = &max1241_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = max1241_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(max1241_channels);
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static const struct spi_device_id max1241_id[] = {
> +	{ "max1241", max1241 },
> +	{},
> +};
> +
> +static const struct of_device_id max1241_dt_ids[] = {
> +	{ .compatible = "maxim,max1241" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, max1241_dt_ids);
> +
> +static struct spi_driver max1241_spi_driver = {
> +	.driver = {
> +		.name = "max1241",
> +		.of_match_table = max1241_dt_ids,
> +	},
> +	.probe = max1241_probe,
> +	.id_table = max1241_id,
> +};
> +module_spi_driver(max1241_spi_driver);
> +
> +MODULE_AUTHOR("Alexandru Lazar <alazar@startmail.com>");
> +MODULE_DESCRIPTION("MAX1241 ADC driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v4 2/2] iio: adc: Add MAX1241 driver

[Andy Shevchenko]

On Fri, Mar 20, 2020 at 05:01:15PM +0200, Alexandru Lazar wrote:
> Add driver for the Maxim MAX1241 12-bit, single-channel ADC. The driver
> includes support for this device's low-power operation mode.

...

> +#include <linux/regulator/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>

I think you can keep them sorted.

...

> +#define MAX1241_VAL_MASK 0xFFF

GENMASK() ?

...

> +		if (adc->shdn) {
> +			gpiod_set_value(adc->shdn, 0);
> +			udelay(MAX1241_SHDN_DELAY_USEC);
> +		}
> +
> +		ret = max1241_read(adc);
> +
> +		if (adc->shdn)
> +			gpiod_set_value(adc->shdn, 1);

I guess easier to read in a way

		if () {
			gpio...
			read();
			gpio...
		} else {
			read();
		}

Or actually introduce runtime PM and move these gpio calls there.


...

> +static int max1241_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct max1241 *adc;

> +	int ret = 0;

Redundant assignment.

> +	ret = regulator_enable(adc->reg);
> +	if (ret)
> +		return ret;
> +

> +	ret = devm_add_action_or_reset(&spi->dev, max1241_disable_reg_action,
> +					adc);

Introducing
	struct device *dev = &spi->dev;
will simplifies such lines like above by making them on one line.

> +	if (ret) {
> +		dev_err(&spi->dev, "could not set up regulator cleanup action!\n");
> +		return ret;
> +	}

> +	adc->shdn = devm_gpiod_get_optional(&spi->dev, "shdn", GPIOD_OUT_HIGH);

> +

Redundant blank line.

> +	if (IS_ERR(adc->shdn))
> +		return PTR_ERR(adc->shdn);

> +	if (!adc->shdn)

Why not to use positive conditional?

> +		dev_dbg(&spi->dev, "no shdn pin passed, low-power mode disabled");
> +	else
> +		dev_dbg(&spi->dev, "shdn pin passed, low-power mode enabled");

> +}

...

> +static const struct spi_device_id max1241_id[] = {
> +	{ "max1241", max1241 },

> +	{},

Terminators better w/o comma.

> +};
> +
> +static const struct of_device_id max1241_dt_ids[] = {
> +	{ .compatible = "maxim,max1241" },

> +	{},

Ditto.

> +};

-- 
With Best Regards,
Andy Shevchenko