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

[Jerome Brunet <jbrunet@baylibre.com>]

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

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

>
>> +		*val2 = cmsr_get_time_unlocked(cm);
>> +		*val = 1000000;
>> +		return IIO_VAL_FRACTIONAL;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +

-- 
Jerome