On Tue, 25 Jun 2024 18:59:58 +0200
Jerome Brunet <jbrunet@xxxxxxxxxxxx> wrote:
> On Tue 25 Jun 2024 at 09:52, David Lechner <dlechner@xxxxxxxxxxxx> wrote:
>
> > On 6/25/24 3:31 AM, Jerome Brunet wrote:
> >> On Mon 24 Jun 2024 at 17:51, David Lechner <dlechner@xxxxxxxxxxxx> 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
