Re: [PATCH] iio: light: veml6030: add support for triggered buffer

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 9 Nov 2024 15:32:45 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> Hi Jonathan, thanks for your feedback.
> 
> On 09/11/2024 14:27, Jonathan Cameron wrote:
> > On Thu, 07 Nov 2024 21:22:45 +0100
> > Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
> >   
> >> All devices supported by this driver (currently veml6030, veml6035
> >> and veml7700) have two 16-bit channels, and can profit for the same
> >> configuration to support data access via triggered buffers.
> >>
> >> The measurements are stored in two 16-bit consecutive registers
> >> (addresses 0x04 and 0x05) as little endian, unsigned data.
> >>
> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>  
> > Hi Javier,
> > 
> > Some comments inline below.
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> >> ---
> >>  drivers/iio/light/Kconfig    |  2 ++
> >>  drivers/iio/light/veml6030.c | 84 ++++++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 86 insertions(+)
> >>  
> 
> ...
> 
> >> +static irqreturn_t veml6030_trigger_handler(int irq, void *p)
> >> +{
> >> +	struct iio_poll_func *pf = p;
> >> +	struct iio_dev *iio = pf->indio_dev;
> >> +	struct veml6030_data *data = iio_priv(iio);
> >> +	int i, ret, reg;
> >> +	int j = 0;
> >> +
> >> +	iio_for_each_active_channel(iio, i) {  
> > Given you've set the available_scan_masks such that all channels are on
> > or off, you should be able to read them unconditionally.
> > The IIO core demux code will break them up if the user requested a subset.
> >   
> 
> What I wanted to model is that both channels (ALS and WH) should be
> accessible, but allowing the user to request a single one, as the ALS
> channel is usually more relevant. It seems that in that case I should
> leave available_scan_masks as it is, right? I don't want to keep any
> channel from being accessible.
Ah. No that's not what it is for.  If you don't set it at all, any
combination of channels may be enabled.  If you set it you are restricting
the choice of what is enabled to particular combinations.  In the simple
case this is used when you want to enforce (at the driver level) that
'all' channels are always captured - usually because there is an efficient
bulk channel read.

The IIO core will then pull out the data for the channels the user actually
requested.

In a case where the reads are independent just don't provide available_scan_masks
at all. That matches providing  0x3, 0x2, 0x1, 0x0 here  So any combination of
channels.

> 
> > If it makes sense to allow individual channels (looks like it here)
> > then don't provide available_scan_masks.
> > 
> > A bulk read may make sense (I've not checked register values).
> >   
> 
> In my interpretation, I thought that I could read a single register if
> there is only a single active channel, instead of using regmap_read_bulk
> unconditionally. the data registers have consecutive addresses, and a
> bulk read is possible, though.
> 
> What approach is preferred in this case? Reading and pushing both
> channels at once without any loop, letting the IIO core demux do the
> rest, or reading only the active channels as it is done here?

It depends a bit on the device. People tend not to want one axis of an 
accelerometer, but here a single channel is a likely case to optimize
for, so just don't provide available_scan_masks and it will all work I think.

If it's is a significant saving you can always figure out if a bulk
read makes sense in your driver and do it if both channels are enabled.
Probably not worth the complexity here.


Jonathan

> 
> >> +		ret = regmap_read(data->regmap, VEML6030_REG_DATA(i), &reg);
> >> +		if (ret)
> >> +			goto done;
> >> +
> >> +		data->scan.chans[j++] = reg;
> >> +	}
> >> +
> >> +	iio_push_to_buffers_with_timestamp(iio, &data->scan, pf->timestamp);
> >> +
> >> +done:
> >> +	iio_trigger_notify_done(iio->trig);
> >> +
> >> +	return IRQ_HANDLED;
> >> +}  
> 
> Thanks again and best regards,
> Javier Carrasco
>