linux-kernel - Re: [PATCH v3 2/2] iio: adc: Add ti-ads1018 driver

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: <18fbf486-c1cc-4cd2-af12-ffa093fa9ce7@baylibre.com>
Date: Mon, 1 Dec 2025 10:07:38 -0600
From: David Lechner <dlechner@...libre.com>
To: Kurt Borja <kuurtb@...il.com>,
 Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Tobias Sperling <tobias.sperling@...ting.com>,
 Nuno Sá <nuno.sa@...log.com>,
 Andy Shevchenko <andy@...nel.org>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH v3 2/2] iio: adc: Add ti-ads1018 driver

On 11/29/25 9:31 PM, Kurt Borja wrote:
> On Sat Nov 29, 2025 at 9:21 AM -05, Andy Shevchenko wrote:
> 

...

> 
>>> +static int ads1018_read_unlocked(struct ads1018 *ads1018, __be16 *cnv, bool hold_cs)
>>
>> Hmm... Don't we want to return value in CPU order? I don't know the answer
>> here, and IIRC IIO triggers might be actually good with endianess conversion
>> done, if required, in user space.
> 
> I specified IIO_BE endianness in each channel's .scan_type, so this
> works. However, I don't have issue especifying IIO_CPU and just
> returning CPU order values.
> 

Usually, we want to change the data as little as possible, so leaving
it as IIO_BE is fine.

> ...
> 
>>> + * Context: Expects iio_device_claim_direct() is held.
>>
>> Jonathan et al., do we have lockdep assert available for this?
>> I really prefer to see the code for it, while comment is good,
>> it is not good enough.
> 
> This would be nice.
> 
> ...
> 
>>> +	if (iio_device_claim_buffer_mode(indio_dev))
>>> +		goto out_notify_done;
>>> +
>>> +	if (iio_trigger_using_own(indio_dev)) {
>>> +		disable_irq(ads1018->drdy_irq);
>>> +		ret = ads1018_read_unlocked(ads1018, &scan.conv, true);
>>> +		enable_irq(ads1018->drdy_irq);
>>> +	} else {
>>> +		ret = spi_read(ads1018->spi, ads1018->rx_buf, sizeof(ads1018->rx_buf));
>>> +		scan.conv = ads1018->rx_buf[0];
>>> +	}
>>> +
>>> +	iio_device_release_buffer_mode(indio_dev);
>>> +
>>> +	if (ret)
>>> +		goto out_notify_done;
>>> +
>>> +	iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan), pf->timestamp);
>>> +
>>> +out_notify_done:
>>> +	iio_trigger_notify_done(ads1018->indio_trig);
>>
>> Jonathan et al., maybe we need an ACQUIRE() class for this? It will solve
>> the conditional scoped guard case, no?

No, ACQUIRE() is not scoped, just conditional. I don't think it
will improve anything here.

> 
> ...
> 
> If no one prefers to do it, I can submit a patch implementing this. Same
> for the lockdep issue above.
>