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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8c9b1e43-f47e-46a5-9189-fbe73b16ff7b@gmail.com>
Date: Sat, 9 Nov 2024 15:32:45 +0100
From: Javier Carrasco <javier.carrasco.cruz@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Lars-Peter Clausen <lars@...afoo.de>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: light: veml6030: add support for triggered buffer

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@...il.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@...il.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.

> 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?

>> +		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