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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 25 Jun 2024 18:59:58 +0200
Jerome Brunet <jbrunet@baylibre.com> wrote:

> On Tue 25 Jun 2024 at 09:52, David Lechner <dlechner@baylibre.com> wrote:
> 
> > On 6/25/24 3:31 AM, Jerome Brunet wrote:  
> >> On Mon 24 Jun 2024 at 17:51, David Lechner <dlechner@baylibre.com> wrote:
> >>   
> >>> On 6/24/24 12:31 PM, Jerome Brunet wrote:  
> >>>> Add support for the HW found in most Amlogic SoC dedicated to measure
> >>>> system clocks.
> >>>>  
> >>>
> >>>
> >>>  
> >>>> +static int cmsr_read_raw(struct iio_dev *indio_dev,
> >>>> +			 struct iio_chan_spec const *chan,
> >>>> +			 int *val, int *val2, long mask)
> >>>> +{
> >>>> +	struct amlogic_cmsr *cm = iio_priv(indio_dev);
> >>>> +
> >>>> +	guard(mutex)(&cm->lock);
> >>>> +
> >>>> +	switch (mask) {
> >>>> +	case IIO_CHAN_INFO_RAW:
> >>>> +		*val = cmsr_measure_unlocked(cm, chan->channel);  
> >>>
> >>> Is this actually returning an alternating voltage magnitutde?
> >>> Most frequency drivers don't have a raw value, only frequency.  
> >> 
> >> No it is not the magnitude, it is the clock rate (frequency) indeed.
> >> Maybe altvoltage was not the right pick for that but nothing obvious
> >> stands out for Hz measurements  
> >
> > I'm certainly not an expert on the subject, but looking at the other
> > frequency drivers, using altvoltage looks correct.
> >
> > But, we in those drivers, nearly all only have a "frequency" attribute
> > but don't have a "raw" attribute. The ones that do have a "raw" attribute
> > are frequency generators that use the raw attribute determine the output
> > voltage.
> >  
> >>   
> >>>  
> >>>> +		if (*val < 0)
> >>>> +			return *val;
> >>>> +		return IIO_VAL_INT;
> >>>> +
> >>>> +	case IIO_CHAN_INFO_PROCESSED: /* Result in Hz */  
> >>>
> >>> Shouldn't this be IIO_CHAN_INFO_FREQUENCY?  
> >> 
> >> How would I get raw / processed / scale with IIO_CHAN_INFO_FREQUENCY ?
> >>   
> >>>
> >>> Processed is just (raw + offset) * scale which would be a voltage
> >>> in this case since the channel type is IIO_ALTVOLTAGE.  
> >> 
> >> This is was Processed does here, along with selecting the most
> >> appropriate scale to perform the measurement.
> >>   
> >>>  
> >>>> +		*val = cmsr_measure_processed_unlocked(cm, chan->channel, val2);
> >>>> +		if (*val < 0)
> >>>> +			return *val;
> >>>> +		return IIO_VAL_INT_64;
> >>>> +
> >>>> +	case IIO_CHAN_INFO_SCALE:  
> >>>
> >>> What is this attribute being used for?  
> >> 
> >> Hz
> >>   
> >>>
> >>> (clearly not used to convert the raw value to millivolts :-) )
> >>>
> >>> Maybe IIO_CHAN_INFO_INT_TIME is the right one for this? Although
> >>> so far, that has only been used with light sensors.  
> >> 
> >> I think you are mixing up channel info and type here.
> >> I do want the info
> >>  * IIO_CHAN_INFO_RAW
> >>  * IIO_CHAN_INFO_PROCESSED
> >>  * IIO_CHAN_INFO_SCALE
> >> 
> >> I want those info to represent an alternate voltage frequency in Hz.
> >> I thought type 'IIO_ALTVOLTAGE' was the right pick for that. Apparently
> >> it is not. What is the appropriate type then ? Should I add a new one ?  
> >
> >
> > The documentation at Documentation/ABI/testing/sysfs-bus-iio explains
> > what the combination of a channel type and info means.  
> 
> Oh missed that, Thx
> 
> >
> > For example, out_altvoltageY_raw is defined as it used for the frequency
> > generator case that I mentioned above. in_altvoltageY_raw is not defined
> > which means probably no one has needed it yet. But it would still be the
> > voltage value, not the frequency.  
> 
> Got it. So the type I picked is wrong for sure.
> So, maybe I need something new to measure a frequency ?

Yes. Seems likely we need a new channel type to me.
In theory we could abuse an angular rate channel but that's nasty so
lets not do that. :)

Jonathan