Received-SPF: pass (google.com: domain of linux-kernel+bounces-183123-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Sun, 19 May 2024 15:18:09 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: Gustavo Silva <gustavograzs@gmail.com>
Cc: robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org,
 lars@metafoo.de, christophe.jaillet@wanadoo.fr,
 gerald.loacker@wolfvision.net, devicetree@vger.kernel.org,
 linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/6] iio: chemical: ens160: add triggered buffer support
Message-ID: <20240519151809.192448d1@jic23-huawei>
In-Reply-To: <20240512210444.30824-5-gustavograzs@gmail.com>
References: <20240512210444.30824-1-gustavograzs@gmail.com>
	<20240512210444.30824-5-gustavograzs@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Sun, 12 May 2024 18:04:40 -0300
Gustavo Silva <gustavograzs@gmail.com> wrote:

> ENS160 supports a data ready interrupt. Use it in combination with
> triggered buffer for continuous data readings.
> 
> Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>
Hi Gustavo,

Various comments inline. Mostly simplifications you can probably make.

Jonathan

> ---
>  drivers/iio/chemical/ens160.h      |   2 +-
>  drivers/iio/chemical/ens160_core.c | 155 ++++++++++++++++++++++++++++-
>  drivers/iio/chemical/ens160_i2c.c  |   2 +-
>  drivers/iio/chemical/ens160_spi.c  |   2 +-
>  4 files changed, 156 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/chemical/ens160.h b/drivers/iio/chemical/ens160.h
> index 3fd079bc4..a8a2f1263 100644
> --- a/drivers/iio/chemical/ens160.h
> +++ b/drivers/iio/chemical/ens160.h
> @@ -2,7 +2,7 @@
>  #ifndef ENS160_H_
>  #define ENS160_H_
>  
> -int ens160_core_probe(struct device *dev, struct regmap *regmap,
> +int ens160_core_probe(struct device *dev, struct regmap *regmap, int irq,
>  		      const char *name);
>  void ens160_core_remove(struct device *dev);
>  
> diff --git a/drivers/iio/chemical/ens160_core.c b/drivers/iio/chemical/ens160_core.c
> index 25593420d..4b960ef00 100644
> --- a/drivers/iio/chemical/ens160_core.c
> +++ b/drivers/iio/chemical/ens160_core.c
> @@ -11,6 +11,9 @@
>  
>  #include <linux/bitfield.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
>  
> @@ -24,6 +27,11 @@
>  
>  #define ENS160_REG_OPMODE	0x10
>  
> +#define ENS160_REG_CONFIG		0x11
> +#define ENS160_REG_CONFIG_INTEN		BIT(0)
> +#define ENS160_REG_CONFIG_INTDAT	BIT(1)
> +#define ENS160_REG_CONFIG_INT_CFG	BIT(5)
> +
>  #define ENS160_REG_MODE_DEEP_SLEEP	0x00
>  #define ENS160_REG_MODE_IDLE		0x01
>  #define ENS160_REG_MODE_STANDARD	0x02
> @@ -48,6 +56,12 @@
>  
>  struct ens160_data {
>  	struct regmap *regmap;
> +	struct mutex mutex;
> +	struct {
> +		u16 chans[2];

As per the bot reply. This should be __le16.
> +		s64 timestamp __aligned(8);
> +	} scan;

You can do spi read directly into here but if you do
move it to the end of the structure and align it to IIO_DMA_MINALIGN.

> +	int irq;
As below - not sure there is any advantage in keeping a copy
of this after probe. I'd just pass it into the functions that need it.
>  };

>  
>  static int ens160_read_raw(struct iio_dev *indio_dev,
> @@ -79,10 +108,19 @@ static int ens160_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> +		ret = iio_device_claim_direct_mode(indio_dev);

Use iio_device_claim_direct_scoped() and guard() for the mutex
as will automate the unwinding of the two types of lock and avoid
you having to do it by hand.


> +		if (ret)
> +			return ret;
> +		mutex_lock(&data->mutex);
>  		ret = regmap_bulk_read(data->regmap, chan->address,
>  					&buf, sizeof(buf));
> -		if (ret)
> +		if (ret) {
> +			mutex_unlock(&data->mutex);
> +			iio_device_release_direct_mode(indio_dev);
>  			return ret;
> +		}
> +		mutex_unlock(&data->mutex);
> +		iio_device_release_direct_mode(indio_dev);
>  		*val = le16_to_cpu(buf);
>  		return IIO_VAL_INT;
>  
> @@ -182,7 +220,104 @@ static const struct iio_info ens160_info = {
>  	.read_raw = ens160_read_raw,
>  };
>  
> -int ens160_core_probe(struct device *dev, struct regmap *regmap,
> +static irqreturn_t ens160_irq_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +
> +	if (iio_buffer_enabled(indio_dev))

How else did you get here?  Either you should use a threaded interrupt
to check the status registers on the device, or you should assume
there is no other way of getting here (and hence no sharing of interrupt
etc) in which case this check is unnecessary and you can use
iio_trigger_generic_data_rdy_poll().


> +		iio_trigger_poll(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t ens160_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ens160_data *data = iio_priv(indio_dev);
> +	__le16 val;
> +	int ret, i, j = 0;
> +
> +	mutex_lock(&data->mutex);
> +
> +	for_each_set_bit(i, indio_dev->active_scan_mask,
> +			 indio_dev->masklength) {
> +		ret = regmap_bulk_read(data->regmap,
> +				       ENS160_REG_DATA_TVOC + 2 * i, &val, 2U);

sizeof(val) instead of hardcoded 2. Though better still to just bulk
read the lot ever time and loose this loop in favour of the demux in the IIO
core handling the rare occasion of people wanting one channel.

> +		if (ret)
> +			goto err;
> +
> +		data->scan.chans[j++] = val;

Read directly into the data->scan.chans[]

Also, I'd assume that 90% of the time, people want all the channels.  A such
can you just bulk read them all?  Then you can use available_scan_masks
to let the IIO core handle the 10% of the time when only one channel is requested.


> +	}
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
> +					   pf->timestamp);
> +err:
> +	mutex_unlock(&data->mutex);
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ens160_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct ens160_data *data = iio_priv(indio_dev);
> +	unsigned int int_bits = ENS160_REG_CONFIG_INTEN |
> +				ENS160_REG_CONFIG_INTDAT |
> +				ENS160_REG_CONFIG_INT_CFG;
> +	int ret;
> +
> +	if (state)
> +		ret = regmap_set_bits(data->regmap, ENS160_REG_CONFIG,
> +				      int_bits);
		return ...
> +	else
> +		ret = regmap_clear_bits(data->regmap, ENS160_REG_CONFIG,
> +					int_bits);
		return ...

> +
> +	return ret;
> +}
> +
> +static const struct iio_trigger_ops ens160_trigger_ops = {
> +	.set_trigger_state = ens160_set_trigger_state,
> +	.validate_device = iio_trigger_validate_own_device,
> +};
> +
> +static int ens160_setup_trigger(struct iio_dev *indio_dev)
> +{
> +	struct ens160_data *data = iio_priv(indio_dev);
> +	struct device *dev = indio_dev->dev.parent;
> +	struct iio_trigger *trig;
> +	int ret;
> +
> +	trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
> +				      iio_device_id(indio_dev));
> +	if (!trig)
> +		return dev_err_probe(dev, -ENOMEM,
> +				     "failed to allocate trigger\n");
> +
> +	trig->ops = &ens160_trigger_ops;
> +	iio_trigger_set_drvdata(trig, indio_dev);
> +
> +	ret = devm_iio_trigger_register(dev, trig);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->trig = iio_trigger_get(trig);
> +
> +	ret = devm_request_threaded_irq(dev, data->irq,
> +					ens160_irq_handler,
> +					NULL,
> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
Generally, for new drivers we leave the direction control up to firmware.
A nasty, but common trick is to use an inverter to do level conversion.
That results in the polarity being switched but is not explicitly described
in the firmware. So we rely in those cases on the firmware settings for
the interrupt not being modified by the driver.

IRQF_ONESHOT, only here.

> +					indio_dev->name,
> +					indio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to request irq\n");
> +
> +	return 0;
> +}
> +
> +int ens160_core_probe(struct device *dev, struct regmap *regmap, int irq,
>  		      const char *name)
>  {
>  	struct ens160_data *data;
> @@ -196,6 +331,7 @@ int ens160_core_probe(struct device *dev, struct regmap *regmap,
>  	data = iio_priv(indio_dev);
>  	dev_set_drvdata(dev, indio_dev);
>  	data->regmap = regmap;
> +	data->irq = irq;

As below. This stashing of a copy of irq is an unnecessary complication.

>  
>  	indio_dev->name = name;
>  	indio_dev->info = &ens160_info;
> @@ -203,12 +339,27 @@ int ens160_core_probe(struct device *dev, struct regmap *regmap,
>  	indio_dev->channels = ens160_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(ens160_channels);
>  
> +	if (data->irq > 0) {

Pass the irq into the setup_trigger call. You don't need it other than for
registration so no point in keeping it in the data structure.

> +		ret = ens160_setup_trigger(indio_dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to setup trigger\n");
> +	}
> +
>  	ret = ens160_chip_init(data);
>  	if (ret) {
>  		dev_err_probe(dev, ret, "chip initialization failed\n");
>  		return ret;
>  	}
>  
> +	mutex_init(&data->mutex);
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      iio_pollfunc_store_time,
> +					      ens160_trigger_handler, NULL);
> +	if (ret)
> +		return ret;
> +
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  EXPORT_SYMBOL_NS(ens160_core_probe, IIO_ENS160);