linux-kernel - Re: [PATCH v6 2/2] iio: light: ROHM BH1745 colour sensor

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: <f9211cc1-7392-4b5e-83fe-e971b274f348@gmail.com>
Date: Mon, 1 Jul 2024 20:34:24 +0100
From: Mudit Sharma <muditsharma.info@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: lars@...afoo.de, krzk+dt@...nel.org, conor+dt@...nel.org,
 robh@...nel.org, linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, Ivan Orlov <ivan.orlov0322@...il.com>,
 Javier Carrasco <javier.carrasco.cruz@...il.com>
Subject: Re: [PATCH v6 2/2] iio: light: ROHM BH1745 colour sensor

On 29/06/2024 18:50, Jonathan Cameron wrote:
> On Sat, 29 Jun 2024 10:10:19 +0100
> Mudit Sharma <muditsharma.info@...il.com> wrote:
> 
>> On 28/06/2024 20:37, Jonathan Cameron wrote:
>>> Hi Mudit,
>>>
>>> I'd failed on previous reviews to notice the odd trigger in here.
>>> What is it, because it doesn't seem to be a dataready trigger as the device
>>> doesn't seem to provide such an interrupt?
>>
>> Hi Jonathan,
>>
>> Thank you for your review on this.
>>
>> I've incorrect called it as a dataready trigger, I missed this as part
>> of my initial cleanup - apologies for the confusion caused by this. I
>> should potentially call it 'threshold' or 'dev'. Please suggest what you
>> think would be appropriate here.
>>
>> The sensor has an active low interrupt pin which is connected to a GPIO
>> (input, pullup). When the sensor reading crosses value set in threshold
>> high or threshold low resisters, interrupt signal is generated and the
>> interrupt gets handled in 'bh1745_interrupt_handler()' (interrupt also
>> depends on number of consecutive judgements set in BH1745_PERSISTENCE
>> register)
> 
> This isn't really a trigger. Just report the event and don't register a trigger at
> all.
> 
> In theory we could have a trigger with these properties (and we did discuss
> many years ago how to do this in a generic fashion) but today it isn't
> something any standard userspace will understand how to use.
> 
>>
>>>
>>> Various other comments inline.
>>
>> Will address all for v7
>>>   
>> ...
>>>> +static irqreturn_t bh1745_interrupt_handler(int interrupt, void *p)
>>>> +{
>>>> +	struct iio_dev *indio_dev = p;
>>>> +	struct bh1745_data *data = iio_priv(indio_dev);
>>>> +	int ret;
>>>> +	int value;
>>>> +
>>>> +	ret = regmap_read(data->regmap, BH1745_INTR, &value);
>>>> +	if (ret)
>>>> +		return IRQ_NONE;
>>>> +
>>>> +	if (value & BH1745_INTR_STATUS) {
>>>> +		guard(mutex)(&data->lock);
>>>> +		iio_push_event(indio_dev,
>>>> +			       IIO_UNMOD_EVENT_CODE(IIO_INTENSITY, data->int_src,
>>>> +						    IIO_EV_TYPE_THRESH,
>>>> +						    IIO_EV_DIR_EITHER),
>>>> +			       iio_get_time_ns(indio_dev));
>>>
>>> What is happening here.  You always push out the event and use that as
>>> a trigger?  This is an unusual trigger if it's appropriate to use it for
>>> one at all.  You've called it a dataready trigger but it is not obvious
>>> that this device provides any such signal.
>>
>> When an interrupt occurs, BH1745_INTR_STATUS bit is set in the
>> BH1745_INTR register. Event is only pushed out when the
>> BH1745_INTR_STATUS bit is set.
> That bit is fine. My confusion is more about the trigger.  I think
> the short answer is drop the next line and indeed all the code registering
> a trigger as this device doesn't provide appropriate hardware.
> 

Noted - thank you for your guidance.

Will include the changes for v7.

Best regards,
Mudit Sharma