Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 4 Sep 2022 16:50:03 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Ramona Bolboaca <ramona.bolboaca@analog.com>
Cc:     <robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <linux-iio@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 2/2] iio: adc: add max11205 adc driver
Message-ID: <20220904165003.192d5030@jic23-huawei>
In-Reply-To: <20220831133021.215625-2-ramona.bolboaca@analog.com>
References: <20220831133021.215625-1-ramona.bolboaca@analog.com>
        <20220831133021.215625-2-ramona.bolboaca@analog.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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),
> +	},
> +};