linux-kernel - Re: [PATCH 2/2] iio: frequency: add amlogic clock measure support

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]

Date: Sun, 30 Jun 2024 09:21:03 +0200
From: Jerome Brunet <jbrunet@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Lars-Peter Clausen <lars@...afoo.de>,  Neil Armstrong
 <neil.armstrong@...aro.org>,  Kevin Hilman <khilman@...libre.com>,
  linux-kernel@...r.kernel.org,  linux-amlogic@...ts.infradead.org,
  linux-iio@...r.kernel.org,  Rob Herring <robh@...nel.org>,  Krzysztof
 Kozlowski <krzk+dt@...nel.org>,  Conor Dooley <conor+dt@...nel.org>
Subject: Re: [PATCH 2/2] iio: frequency: add amlogic clock measure support

On Sat 29 Jun 2024 at 20:40, Jonathan Cameron <jic23@...nel.org> wrote:

> On Mon, 24 Jun 2024 19:31:03 +0200
> Jerome Brunet <jbrunet@...libre.com> wrote:
>
>> Add support for the HW found in most Amlogic SoC dedicated to measure
>> system clocks.
>> 
>> This drivers aims to replace the one found in
>> drivers/soc/amlogic/meson-clk-measure.c with following improvements:
>> 
>> * Access to the measurements through the IIO API:
>>   Easier re-use of the results in userspace and other drivers
>> * Controllable scale with raw measurements
>> * Higher precision with processed measurements
>> 
>> Signed-off-by: Jerome Brunet <jbrunet@...libre.com>
> Interesting device and the driver is pretty clean.
>
> Needs a new channel type though as altvoltage is in volts not hz.
>
> Various minor comments inline.
>
> Thanks,

Thanks for the feedback Jonathan.

Just a couple of things,

David expressed concerns with having both IIO_CHAN_INFO_RAW and
IIO_CHAN_INFO_PROCESSED for a channel and we did not reach a conclusion
on this.

My idea here is to:
 * Give full control over the scale to the consumer with
   IIO_CHAN_INFO_RAW
 * Give an easy/convenient way to get an Hz result with
   IIO_CHAN_INFO_PROCESSED. There is a bit more than just 'raw * scale'
   in the implementation of this info to figure out the most appropriate
   scale for the measurement. They idea is also to avoid code
   duplication in consumers.

David is definitely more familiar than me with IIO but we did not really
know how to move forward on this.

Is it OK to have both IIO_CHAN_INFO_RAW and IIO_CHAN_INFO_PROCESSED, or
should I ditch IIO_CHAN_INFO_RAW ?

>
> Jonathan

[...]

>> +	indio_dev->name = "amlogic-clk-msr";
>> +	indio_dev->info = &cmsr_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->num_channels = CLK_MSR_MAX;
>
> Superficially looks like the number of channels depends on the compatible.
> Ideally we shouldn't provide channels to userspace that aren't useful.

Not exactly. All SoCs have 128 inputs, Some may not be connected indeed
but some are, and the name is just not documented (yet).

>
> You could search the name arrays to see how far they go, but that is bit ugly.
> Probably wrap those in a structure with a num_channels parameter as well.
>

I've been doodling with this idea while writing this
driver. Technically, there is no problem.

The legacy driver this one will be replacing used to expose all 128
inputs. I thought it was more important to have continuity with the
legacy driver than filtering out possibly useless channels.

Another benefit of keeping all 128 is that the channel index (both in
sysfs and more crucially in DT) matches the one in the SoC documentation.
That makes things easier.

Would it be acceptable to keep all 128 channels then or do you still
prefer that I filter out the undocumented ones ?

>> +	indio_dev->channels = cmsr_populate_channels(dev, conf);
>> +	if (IS_ERR(indio_dev->channels))
>> +		return PTR_ERR(indio_dev->channels);
>> +
>> +	return devm_iio_device_register(dev, indio_dev);
>> +}

Thanks for you help.

-- 
Jerome