[v2,1/2] bindings: iio: adc: Add max11205 documentation file

Add bindings documentation file and MAINTAINERS entry for MAX11205.

Signed-off-by: Ramona Bolboaca <ramona.bolboaca@analog.com>
---
changes in v2:
 - reference /schemas/spi/spi-peripheral-props.yaml in allOf 
 - use unevaluatedProperties:false and remove additionalProperties:false
 - use generic node name and fix indentation in device-tree example
 - wrap lines nearer to 75-80 chars in documentation
 - add extra information to the description of the regulator
 .../bindings/iio/adc/maxim,max11205.yaml      | 69 +++++++++++++++++++
 MAINTAINERS                                   |  8 +++
 2 files changed, 77 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max11205.yaml

> >   
> > > +       if (!st->chip_info)
> > > +               st->chip_info = (const struct chip_info *)spi_get_device_id(spi)->driver_data;
> > > +
> > > +       indio_dev->name = st->chip_info->name;
> > > +       indio_dev->modes = INDIO_DIRECT_MODE;
> > > +       indio_dev->channels = max11205_channels;
> > > +       indio_dev->num_channels = 1;
> > > +       indio_dev->info = &max11205_iio_info;
> > > +
> > > +       st->vref = devm_regulator_get(&spi->dev, "vref");  
> > 

> > devm_regulator_get_enable() ?  

> I found the patches which implement devm_regulator_get_enable.
> However, I need to get the voltage of the regulator using
> regulator_get_voltage(struct regulator *regulator) and if I use
> devm_regulator_get_enable I do not have access to the regulator
> pointer. What should I do in this case? Is there an API which works
> like devm_regulator_get_enable but also gives access to the
> regulator's pointer? Thank you for you review!

Ah.  You've run into a fun long running 'discussion'.  There was
great resistance to adding devm_regulator_get_enable() because it
was felt that it was too easy to get the handling wrong and end up
with underflowing reference counters etc.  So the 'solution' was
to make it less useful than it would otherwise have been by making
sure it could not be combined with other accesses to the regulator.

Upshot is you are correct that it cannot be applied in this case.

Also relevant is that the patch in question is going through the
regulator tree so won't be generally available to the rest of the
kernel until next cycle.  As such we'd have had to make such a
change as a follow up patch if it were possible.

Jonathan

p.s. Wrap your replies at 80 chars as well as the code.
Kernel developers have an annoying habit of reading their email in
one small window even when they have very large monitors :)
Also, where sensible crop unnecessary parts of the email to only
include the bit you are responding to. Saves on lots of scrolling..

> Kind Regards, Ramona
> Bolboaca

On Wed, 31 Aug 2022 16:36:25 +0300
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 31/08/2022 16:30, Ramona Bolboaca wrote:
> > Add bindings documentation file and MAINTAINERS entry for MAX11205.
> > 
> > Signed-off-by: Ramona Bolboaca <ramona.bolboaca@analog.com>  
> 
> 
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - maxim,max11205a
> > +      - maxim,max11205b
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  spi-max-frequency:
> > +    maximum: 5000000
> > +
> > +  spi-cpha: true
> > +
> > +  vref-supply:
> > +    description:
> > +      The regulator supply for the ADC reference voltage. This is a differential
> > +      reference. It is equal to the V_REFP - V_REFN. The maximum value is 3.6V.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - spi-max-frequency
> > +  - spi-cpha
> > +  - interrupts
> > +  - vref-supply  
> 
> If you are going to send a new version, please put the properties here
> in the same order as they appear in top-level "properties:".
Tweaked whilst applying.

J
> 
> In any case:
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> Best regards,
> Krzysztof

On Wed, 31 Aug 2022 16:30:21 +0300
Ramona Bolboaca <ramona.bolboaca@analog.com> wrote:

> Adding support for max11205 16-bit single-channel ultra-low power
> delta-sigma adc.
> The MAX11205 is compatible with the 2-wire interface and uses
> SCLK and RDY/DOUT for serial communications. In this mode, all
> controls are implemented by timing the high or low phase of the SCLK.
> The 2-wire serial interface only allows for data to be read out through
> the RDY/DOUT output.
> 
> Datasheet: https://datasheets.maximintegrated.com/en/ds/MAX11205.pdf
> Signed-off-by: Ramona Bolboaca <ramona.bolboaca@analog.com>

Given the requested changes below and those from Andy and Kryzstof are minor, I'll just
tweak them whilst applying.

Diff for this patch was below.  The long line for chip_info is a bit ugly but
not too bad...

diff --git a/drivers/iio/adc/max11205.c b/drivers/iio/adc/max11205.c
index 68e6082e70e5..fc90fed81eb6 100644
--- a/drivers/iio/adc/max11205.c
+++ b/drivers/iio/adc/max11205.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * max11205 16-Bit Delta-Sigma ADC
+ * Maxim MAX11205 16-Bit Delta-Sigma ADC
  *
  * Datasheet: https://datasheets.maximintegrated.com/en/ds/MAX1240-max11205.pdf
  * Copyright (C) 2022 Analog Devices, Inc.
@@ -19,20 +19,20 @@
 #define MAX11205A_OUT_DATA_RATE        116
 #define MAX11205B_OUT_DATA_RATE        13
 
-enum chip_type {
+enum max11205_chip_type {
        TYPE_MAX11205A,
        TYPE_MAX11205B,
 };
 
-struct chip_info {
+struct max11205_chip_info {
        unsigned int    out_data_rate;
        const char      *name;
 };
 
 struct max11205_state {
-       const struct chip_info  *chip_info;
-       struct regulator        *vref;
-       struct ad_sigma_delta   sd;
+       const struct max11205_chip_info *chip_info;
+       struct regulator                *vref;
+       struct ad_sigma_delta           sd;
 };
 
 static const struct ad_sigma_delta_info max11205_sigma_delta_info = {
@@ -81,12 +81,12 @@ static const struct iio_chan_spec max11205_channels[] = {
                        .endianness = IIO_BE
                },
                .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
-               BIT(IIO_CHAN_INFO_SAMP_FREQ) |
-               BIT(IIO_CHAN_INFO_SCALE),
+                                     BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+                                     BIT(IIO_CHAN_INFO_SCALE),
        },
 };
 
-static const struct chip_info max11205_chip_info[] = {
+static const struct max11205_chip_info max11205_chip_info[] = {
        [TYPE_MAX11205A] = {
                .out_data_rate = MAX11205A_OUT_DATA_RATE,
                .name = "max11205a",
@@ -117,9 +117,9 @@ static int max11205_probe(struct spi_device *spi)
        ad_sd_init(&st->sd, indio_dev, spi, &max11205_sigma_delta_info);
 
        st->chip_info = device_get_match_data(&spi->dev);
-
        if (!st->chip_info)
-               st->chip_info = (const struct chip_info *)spi_get_device_id(spi)->driver_data;
+               st->chip_info =
+                       (const struct max11205_chip_info *)spi_get_device_id(spi)->driver_data;
 
        indio_dev->name = st->chip_info->name;
        indio_dev->modes = INDIO_DIRECT_MODE;

> ---
> changes in v2:
>  - add chip_info null pointer check
>  - add support for probing with ACPI table
>  - remove function for module removal
>  - remove irq flag from max11205_sigma_delta_info
>  - add missing commas and missing spaces
>  - remove redundant blank line
>  - wrap text to 75-80 chars
>  - removed typos in commit message
>  drivers/iio/adc/Kconfig    |  14 +++
>  drivers/iio/adc/Makefile   |   1 +
>  drivers/iio/adc/max11205.c | 183 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 198 insertions(+)
>  create mode 100644 drivers/iio/adc/max11205.c
> 

> +enum chip_type {
> +	TYPE_MAX11205A,
> +	TYPE_MAX11205B,
> +};
> +
> +struct chip_info {
> +	unsigned int	out_data_rate;
> +	const char	*name;
> +};
Prefix these enums and structures with max11205.

They have very generic names, so it's definitely possible that
something with the same name might be added to a header included
by this driver
> +
> +struct max11205_state {
> +	const struct chip_info	*chip_info;
> +	struct regulator	*vref;
> +	struct ad_sigma_delta	sd;
> +};
> +


> +static const struct iio_chan_spec max11205_channels[] = {
> +	{
> +		.type = IIO_VOLTAGE,
> +		.indexed = 1,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_BE
> +		},
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +		BIT(IIO_CHAN_INFO_SAMP_FREQ) |

Code editors always get confused on these, but please add an
indent to this line and the next one. Either align it with the BIT()
above, or just push it in one tab.

> +		BIT(IIO_CHAN_INFO_SCALE),
> +	},
> +};

On Sun, Sep 4, 2022 at 7:31 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Sun, 4 Sep 2022 16:50:03 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> > On Wed, 31 Aug 2022 16:30:21 +0300
> > Ramona Bolboaca <ramona.bolboaca@analog.com> wrote:

...

> > Given the requested changes below and those from Andy and Kryzstof are minor, I'll just
> > tweak them whilst applying.
>
> On that note, applied to the togreg branch of iio.git (with changes as noted)
> and pushed out as testing for 0-day to see if it can find anything we missed.

If you are not too lazy and have time to tweak one completely minor
thingy, see below.

...

> > @@ -81,12 +81,12 @@ static const struct iio_chan_spec max11205_channels[] = {
> >                         .endianness = IIO_BE

+ A comma here :-)


> >                 },
> >                 .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > -               BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> > -               BIT(IIO_CHAN_INFO_SCALE),
> > +                                     BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> > +                                     BIT(IIO_CHAN_INFO_SCALE),
> >         },