Message-ID: <20240630111737.276c949a@jic23-huawei>
Date: Sun, 30 Jun 2024 11:17:37 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Jerome Brunet <jbrunet@...libre.com>
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 Sun, 30 Jun 2024 09:21:03 +0200
Jerome Brunet <jbrunet@...libre.com> wrote:

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

So we have had a few cases of software driving autorange finding for sensors
in the past (typically light sensors). An example is the gp2ap020a00f driver but
those have been been driven by interrupts to detect range issue and are much
more complex than what you have here.  Note that driver only provides _RAW,
I think because we have no documentation of how to convert to an illuminance
reading.  It's old though and probably not a good example to follow.

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

I'll ask the obvious question - do you have a usecase for _RAW + _SCALE
if not we can take the conservative approach of not providing it 'yet'
and revisit once that use case surfaces.

Otherwise, I tend to resist mixing processed and raw + scale just because
it makes it hard for userspace to understand the difference. This combination
has only happened in the past due to driver evolution (we got it wrong
initially for a given driver). 

So I'm not strongly against providing both (and amending my standard
'don't do this reply' to include this as a special case) but I want to
know someone actually cares.


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

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

Your explanation for why we should keep them is fine.
Add some suitable comments so we don't forget it.

> 
> >> +	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.
> 
You are welcome
J

Powered by blists - more mailing lists