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]
Message-ID: <1j4j9hift4.fsf@starbuckisacylon.baylibre.com>
Date: Tue, 25 Jun 2024 10:31:51 +0200
From: Jerome Brunet <jbrunet@...libre.com>
To: David Lechner <dlechner@...libre.com>
Cc: Jonathan Cameron <jic23@...nel.org>,  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 Mon 24 Jun 2024 at 17:51, David Lechner <dlechner@...libre.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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ