Message-ID: <20240907172040.7cca5430@jic23-huawei>
Date: Sat, 7 Sep 2024 17:20:40 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Antoni Pokusinski <apokusinski01@...il.com>
Cc: lars@...afoo.de, robh@...nel.org, krzk+dt@...nel.org, pmeerw@...erw.net,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] iio: temperature: tmp006: add triggered buffer
 support

On Mon,  2 Sep 2024 14:59:47 +0200
Antoni Pokusinski <apokusinski01@...il.com> wrote:

> Add support for continuous data capture using triggered buffers for the
> tmp006 sensor. The device features a "data ready" interrupt line which
> is pulled down once a new measurement is ready to be read.
> 
> Signed-off-by: Antoni Pokusinski <apokusinski01@...il.com>
> ---
> Note:
> For some reason, the checkpatch.pl returns a warning for this patch:

Don't worry - it's a well known checkpatch false positive but the case is obscure
enough no one ever bothered 'fixing' it.

Ignore it.

However, I just merged a patch from Andy providing
aligned_s64 as a type so feel free to use that which should
get rid of this message.


> 
>   WARNING: Missing a blank line after declarations
>   #156: FILE: drivers/iio/temperature/tmp006.c:253:
>   +               s16 channels[2];
>   +               s64 ts __aligned(8);
> 
> It also suggests that the following code with a blank line is correct:
> 
> 	struct {
> 		s16 channels[2];
> 
> 		s64 ts __aligned(8);
> 	} scan;
> 
> I left the code as it was (with the WARNING) since it feels obviously
> more natural.

Various comments inline.

Jonathan

> ---
>  drivers/iio/temperature/Kconfig  |   2 +
>  drivers/iio/temperature/tmp006.c | 116 +++++++++++++++++++++++++++++--
>  2 files changed, 111 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> index ed0e4963362f..1244d8e17d50 100644
> --- a/drivers/iio/temperature/Kconfig
> +++ b/drivers/iio/temperature/Kconfig
> @@ -91,6 +91,8 @@ config MLX90635
>  config TMP006
>  	tristate "TMP006 infrared thermopile sensor"
>  	depends on I2C
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
>  	help
>  	  If you say yes here you get support for the Texas Instruments
>  	  TMP006 infrared thermopile sensor.
> diff --git a/drivers/iio/temperature/tmp006.c b/drivers/iio/temperature/tmp006.c
> index 6d8d661f0c82..183b30a41573 100644
> --- a/drivers/iio/temperature/tmp006.c

>  #define TMP006_VOBJECT 0x00
>  #define TMP006_TAMBIENT 0x01
> @@ -45,6 +46,7 @@
>  struct tmp006_data {
>  	struct i2c_client *client;
>  	u16 config;
> +	struct iio_trigger *drdy_trig;
>  };
>  
>  static int tmp006_read_measurement(struct tmp006_data *data, u8 reg)
> @@ -81,15 +83,21 @@ static int tmp006_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> +		ret = iio_device_claim_direct_mode(indio_dev);

I'd suggest the scoped variant of this as you
are effectively taking and releasing locks in different scopes.
The code is write, but not nice to read.

> +		if (ret)
> +			return ret;
> +
>  		if (channel->type == IIO_VOLTAGE) {
>  			/* LSB is 156.25 nV */
>  			ret = tmp006_read_measurement(data, TMP006_VOBJECT);
> +			iio_device_release_direct_mode(indio_dev);
>  			if (ret < 0)
>  				return ret;
>  			*val = sign_extend32(ret, 15);
>  		} else if (channel->type == IIO_TEMP) {
>  			/* LSB is 0.03125 degrees Celsius */
>  			ret = tmp006_read_measurement(data, TMP006_TAMBIENT);
> +			iio_device_release_direct_mode(indio_dev);
>  			if (ret < 0)
>  				return ret;
>  			*val = sign_extend32(ret, 15) >> TMP006_TAMBIENT_SHIFT;

> @@ -164,13 +178,29 @@ static const struct iio_chan_spec tmp006_channels[] = {
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_SCALE),
>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_BE
trailing comma wanted here.

> +		}
>  	},
>  	{
>  		.type = IIO_TEMP,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_SCALE),
>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> -	}
> +		.scan_index = 1,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 14,
> +			.storagebits = 16,
> +			.shift = TMP006_TAMBIENT_SHIFT,
> +			.endianness = IIO_BE
add trailing comma. New fields may be added in future and need setting
so having ht comma will make that easier to do.

> +		}
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(2)

Add a comma.  In is possible that other channels will follow this.
Doesn't often happen but there have been cases.

>  };
>  
>  static const struct iio_info tmp006_info = {
> @@ -213,6 +243,49 @@ static void tmp006_powerdown_cleanup(void *dev)
>  	tmp006_power(dev, false);
>  }
>  
> +static irqreturn_t tmp006_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct tmp006_data *data = iio_priv(indio_dev);
> +	struct {
> +		s16 channels[2];
> +		s64 ts __aligned(8);
> +	} scan;
> +
> +	scan.channels[0] = i2c_smbus_read_word_data(data->client, TMP006_VOBJECT);

read word_data uses an s32 return type to leave space for errors.  
What you have here will cause problems if the top bit happens to be set in
the register (i.e. it's a negative value).

So use a local s32 first then assign it to the buffer
data after checking for negative values.

Note you currently don't ensure both channels are enabled, so if the
user requests only the second channel they will get the wrong data.

Either make this code handle or set available_scan_masks appropriately
to let the core IIO code reorganize the data to what the user asked for.

> +	if (scan.channels[0] < 0)
> +		goto err;
> +
> +	scan.channels[1] = i2c_smbus_read_word_data(data->client, TMP006_TAMBIENT);
> +	if (scan.channels[1] < 0)
> +		goto err;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &scan,
> +					   iio_get_time_ns(indio_dev));
> +err:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
> +
> +static int tmp006_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct tmp006_data *data = iio_priv(indio_dev);
> +
> +	if (state)
> +		data->config |= TMP006_CONFIG_DRDY_EN;
> +	else
> +		data->config &= ~TMP006_CONFIG_DRDY_EN;
> +
> +	return i2c_smbus_write_word_swapped(data->client, TMP006_CONFIG,
> +		data->config);

Align with data on the line above, so just after the (

That's the preferred kernel style except when we are up against
really long lines or similar.

> +}
> +
> +static const struct iio_trigger_ops tmp006_trigger_ops = {
> +	.set_trigger_state = tmp006_set_trigger_state,
> +};
> +
>  static int tmp006_probe(struct i2c_client *client)
>  {
>  	struct iio_dev *indio_dev;
> @@ -258,6 +331,35 @@ static int tmp006_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		return ret;
>  
> +	if (client->irq > 0) {
> +		data->drdy_trig = devm_iio_trigger_alloc(&client->dev,
> +							 "%s-dev%d",
> +							 indio_dev->name,
> +							 iio_device_id(indio_dev));
> +		if (!data->drdy_trig)
> +			return -ENOMEM;
> +
> +		data->drdy_trig->ops = &tmp006_trigger_ops;
> +		iio_trigger_set_drvdata(data->drdy_trig, indio_dev);
> +		ret = iio_trigger_register(data->drdy_trig);
> +		if (ret)
> +			return ret;
> +
> +		indio_dev->trig = iio_trigger_get(data->drdy_trig);
> +
> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +						iio_trigger_generic_data_rdy_poll,
> +						NULL,
> +						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
Type of interrupt should come from firmware.   We wrongly used to set it
in drivers (and are now stuck with that) but that overly constrains
things for no useful purpose.  So IRQF_ONESHOT only here.

> +						"tmp006_irq",
> +						data->drdy_trig);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> +					      tmp006_trigger_handler, NULL);
> +
check ret.

>  	return devm_iio_device_register(&client->dev, indio_dev);
>  }
>  

Powered by blists - more mailing lists