Received-SPF: pass (google.com: domain of linux-kernel+bounces-2671-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Sun, 17 Dec 2023 15:46:36 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: Marcelo Schmitt <marcelo.schmitt@analog.com>
Cc: <apw@canonical.com>, <joe@perches.com>, <dwaipayanray1@gmail.com>,
 <lukas.bulwahn@gmail.com>, <paul.cercueil@analog.com>,
 <Michael.Hennerich@analog.com>, <lars@metafoo.de>, <robh+dt@kernel.org>,
 <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
 <dan.carpenter@linaro.org>, <dlechner@baylibre.com>,
 <marcelo.schmitt1@gmail.com>, <linux-iio@vger.kernel.org>,
 <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 13/15] iio: adc: Add support for AD7091R-8
Message-ID: <20231217154636.07ed31b4@jic23-huawei>
In-Reply-To: <9a0267bc8d444f2c196d1cbc4e9a22debf5d8e4e.1702746240.git.marcelo.schmitt1@gmail.com>
References: <cover.1702746240.git.marcelo.schmitt1@gmail.com>
	<9a0267bc8d444f2c196d1cbc4e9a22debf5d8e4e.1702746240.git.marcelo.schmitt1@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Sat, 16 Dec 2023 14:50:44 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:

> Add support for Analog Devices AD7091R-2, AD7091R-4, and AD7091R-8
> low power 12-Bit SAR ADCs with SPI interface.
> Extend ad7091r-base driver so it can be used by the AD7091R-8 driver.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
Hi Marcelo

A few trivial things from taking another look.

Jonathan

> diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
> index a30dc587ce45..57355ca157a1 100644
> --- a/drivers/iio/adc/ad7091r-base.c
> +++ b/drivers/iio/adc/ad7091r-base.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/bitops.h>
> +#include <linux/bitfield.h>

Not obvious what connection of this header to the code below is...
Wrong patch perhaps?
>  #include <linux/iio/events.h>
>  #include <linux/iio/iio.h>
>  #include <linux/interrupt.h>
> @@ -187,6 +188,12 @@ int ad7091r_probe(struct device *dev, const struct ad7091r_init_info *init_info,
>  	iio_dev->info = &ad7091r_info;
>  	iio_dev->modes = INDIO_DIRECT_MODE;
>  
> +	if (init_info->setup) {
> +		ret = init_info->setup(st);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	if (irq) {
>  		st->chip_info = &init_info->irq_info;
>  		ret = regmap_update_bits(st->map, AD7091R_REG_CONF,
> diff --git a/drivers/iio/adc/ad7091r-base.h b/drivers/iio/adc/ad7091r-base.h
> index 2b4e25e766c8..994505a740b3 100644
> --- a/drivers/iio/adc/ad7091r-base.h
> +++ b/drivers/iio/adc/ad7091r-base.h
> @@ -45,6 +45,8 @@
>  	.scan_type.realbits = bits,					\
>  }
>  
> +#include <linux/gpio/consumer.h>
> +

struct gpio_desc;

and drop the include which only provides an opaque definition anyway.

>  struct device;
>  
>  enum ad7091r_mode {
> @@ -56,10 +58,14 @@ enum ad7091r_mode {
>  struct ad7091r_state {
>  	struct device *dev;
>  	struct regmap *map;
> +	struct gpio_desc *convst_gpio;
> +	struct gpio_desc *reset_gpio;
>  	struct regulator *vref;
>  	const struct ad7091r_chip_info *chip_info;
>  	enum ad7091r_mode mode;
>  	struct mutex lock; /*lock to prevent concurent reads */
> +	__be16 tx_buf __aligned(IIO_DMA_MINALIGN);
> +	__be16 rx_buf;
>  };
>  
>  struct ad7091r_chip_info {
> @@ -77,6 +83,7 @@ struct ad7091r_init_info {
>  	const struct regmap_config *regmap_config;
>  	void (*init_adc_regmap)(struct ad7091r_state *st,
>  				const struct regmap_config *regmap_conf);
> +	int (*setup)(struct ad7091r_state *st);
>  };
>  
>  extern const struct iio_event_spec ad7091r_events[3];
> diff --git a/drivers/iio/adc/ad7091r8.c b/drivers/iio/adc/ad7091r8.c
> new file mode 100644
> index 000000000000..0a6da47d89c0
> --- /dev/null
> +++ b/drivers/iio/adc/ad7091r8.c

> +static int ad7091r8_gpio_setup(struct ad7091r_state *st)
> +{
> +	st->convst_gpio = devm_gpiod_get(st->dev, "adi,conversion-start",
> +					 GPIOD_OUT_LOW);
> +	if (IS_ERR(st->convst_gpio))
> +		return dev_err_probe(st->dev, PTR_ERR(st->convst_gpio),
> +				     "Error getting convst GPIO\n");
> +
> +	st->reset_gpio =  devm_gpiod_get_optional(st->dev, "reset",

Trivial but looks like a bonus space after the =

> +						  GPIOD_OUT_HIGH);
> +	if (IS_ERR(st->reset_gpio))
> +		return dev_err_probe(st->dev, PTR_ERR(st->convst_gpio),
> +				     "Error on requesting reset GPIO\n");
> +
> +	if (st->reset_gpio) {
> +		fsleep(20);
> +		gpiod_set_value_cansleep(st->reset_gpio, 0);
> +	}
> +
> +	return 0;
> +}