| 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
| ||
|
Message-ID: <CAMknhBE0a-nmMEt4XG9YskUdcP0+6yz1mZnvSo3QpimAkQzFsg@mail.gmail.com>
Date: Mon, 2 Oct 2023 12:06:36 -0500
From: David Lechner <dlechner@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-staging@...ts.linux.dev,
David Lechner <david@...hnology.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Michael Hennerich <Michael.Hennerich@...log.com>,
Nuno Sá <nuno.sa@...log.com>,
Axel Haslam <ahaslam@...libre.com>,
Philip Molloy <pmolloy@...libre.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 25/27] staging: iio: resolver: ad2s1210: rename DOS
reset min/max attrs
On Sat, Sep 30, 2023 at 10:47 AM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Fri, 29 Sep 2023 12:23:30 -0500
> David Lechner <dlechner@...libre.com> wrote:
>
> > From: David Lechner <david@...hnology.com>
> >
> > From: David Lechner <dlechner@...libre.com>
> >
> > The AD2S1210 has a programmable threshold for the degradation of signal
> > (DOS) mismatch fault. This fault is triggered when the difference in
> > amplitude between the sine and cosine inputs exceeds the threshold.
> >
> > The DOS reset min/max registers on the chip provide initial values
> > for internal tracking of the min/max of the monitor signal after the
> > fault register is cleared.
> >
> > This patch converts the custom device DOS reset min/max threshold
> > attributes custom event attributes on the monitor signal channel.
> >
> > The attributes now use millivolts instead of the raw register value in
> > accordance with the IIO ABI.
> >
> > Emitting the event will be implemented in a later patch.
> >
> > Signed-off-by: David Lechner <dlechner@...libre.com>
> > ---
> >
> > v3 changes: This is a new patch in v3
> >
> > .../Documentation/sysfs-bus-iio-resolver-ad2s1210 | 27 ++++++
> > drivers/staging/iio/resolver/ad2s1210.c | 99 ++++++++++++----------
> > 2 files changed, 82 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-resolver-ad2s1210 b/drivers/staging/iio/Documentation/sysfs-bus-iio-resolver-ad2s1210
> > new file mode 100644
> > index 000000000000..ea75881b0c77
> > --- /dev/null
> > +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-resolver-ad2s1210
> > @@ -0,0 +1,27 @@
> > +What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_max
> Ah. So these are differential. But the mismatch channel value isn't?
>
> I also got the format wrong for differential channels. Oops. Should
> be the in_altvoltage0-altvoltage1 format for the previous suggestion
> to change that channel type to differential.
>
> This looks fine to me as new ABI.
>
> Jonathan
>
As discussed in
<https://lore.kernel.org/linux-iio/CAMknhBFKSqXvgOeRjGAOfURzndmxmCffdU6MUirEmfzKqwM_Kg@mail.gmail.com/>,
technically no they are not differential. I started to implement it
that way but changed my mind after understanding the datasheet more in
depth. It looks like I forgot to update the documentation in this
patch to match the final implementation that was submitted.
In any case, I will update the documentation to match the
implementation or vice versa depending on what we decide on for which
channel the events should be associated with.
>
>
> > +KernelVersion: 6.7
> > +Contact: linux-iio@...r.kernel.org
> > +Description:
> > + Reading returns the current Degradation of Signal Reset Maximum
> > + Threshold value in millivolts. Writing sets the value.
> > +
> > +What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_max_available
> > +KernelVersion: 6.7
> > +Contact: linux-iio@...r.kernel.org
> > +Description:
> > + Reading returns the allowable voltage range for
> > + in_altvoltage0-altvoltage1_thresh_rising_reset_max.
> > +
> > +What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_min
> > +KernelVersion: 6.7
> > +Contact: linux-iio@...r.kernel.org
> > +Description:
> > + Reading returns the current Degradation of Signal Reset Minimum
> > + Threshold value in millivolts. Writing sets the value.
> > +
> > +What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_min_available
> > +KernelVersion: 6.7
> > +Contact: linux-iio@...r.kernel.org
> > +Description:
> > + Reading returns the allowable voltage range for
> > + in_altvoltage0-altvoltage1_thresh_rising_reset_min.
> > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> > index aa14edbe8a77..e1c95ec73545 100644
> > --- a/drivers/staging/iio/resolver/ad2s1210.c
> > +++ b/drivers/staging/iio/resolver/ad2s1210.c
> > @@ -283,41 +283,6 @@ static ssize_t ad2s1210_clear_fault(struct device *dev,
> > return ret < 0 ? ret : len;
> > }
> >
> > -static ssize_t ad2s1210_show_reg(struct device *dev,
> > - struct device_attribute *attr,
> > - char *buf)
> > -{
> > - struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> > - struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
> > - unsigned int value;
> > - int ret;
> > -
> > - mutex_lock(&st->lock);
> > - ret = regmap_read(st->regmap, iattr->address, &value);
> > - mutex_unlock(&st->lock);
> > -
> > - return ret < 0 ? ret : sprintf(buf, "%d\n", value);
> > -}
> > -
> > -static ssize_t ad2s1210_store_reg(struct device *dev,
> > - struct device_attribute *attr,
> > - const char *buf, size_t len)
> > -{
> > - struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> > - unsigned char data;
> > - int ret;
> > - struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
> > -
> > - ret = kstrtou8(buf, 10, &data);
> > - if (ret)
> > - return -EINVAL;
> > -
> > - mutex_lock(&st->lock);
> > - ret = regmap_write(st->regmap, iattr->address, data);
> > - mutex_unlock(&st->lock);
> > - return ret < 0 ? ret : len;
> > -}
> > -
> > static int ad2s1210_single_conversion(struct ad2s1210_state *st,
> > struct iio_chan_spec const *chan,
> > int *val)
> > @@ -743,13 +708,6 @@ static int ad2s1210_write_raw(struct iio_dev *indio_dev,
> > static IIO_DEVICE_ATTR(fault, 0644,
> > ad2s1210_show_fault, ad2s1210_clear_fault, 0);
> >
> > -static IIO_DEVICE_ATTR(dos_rst_max_thrd, 0644,
> > - ad2s1210_show_reg, ad2s1210_store_reg,
> > - AD2S1210_REG_DOS_RST_MAX_THRD);
> > -static IIO_DEVICE_ATTR(dos_rst_min_thrd, 0644,
> > - ad2s1210_show_reg, ad2s1210_store_reg,
> > - AD2S1210_REG_DOS_RST_MIN_THRD);
> > -
> > static const struct iio_event_spec ad2s1210_position_event_spec[] = {
> > {
> > /* Tracking error exceeds LOT threshold fault. */
> > @@ -867,8 +825,6 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
> >
> > static struct attribute *ad2s1210_attributes[] = {
> > &iio_dev_attr_fault.dev_attr.attr,
> > - &iio_dev_attr_dos_rst_max_thrd.dev_attr.attr,
> > - &iio_dev_attr_dos_rst_min_thrd.dev_attr.attr,
> > NULL,
> > };
> >
> > @@ -876,6 +832,49 @@ static const struct attribute_group ad2s1210_attribute_group = {
> > .attrs = ad2s1210_attributes,
> > };
> >
> > +static ssize_t event_attr_voltage_reg_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> > + struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
> > + unsigned int value;
> > + int ret;
> > +
> > + mutex_lock(&st->lock);
> > + ret = regmap_read(st->regmap, iattr->address, &value);
> > + mutex_unlock(&st->lock);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + return sprintf(buf, "%d\n", value * THRESHOLD_MILLIVOLT_PER_LSB);
> > +}
> > +
> > +static ssize_t event_attr_voltage_reg_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t len)
> > +{
> > + struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> > + struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
> > + u16 data;
> > + int ret;
> > +
> > + ret = kstrtou16(buf, 10, &data);
> > + if (ret)
> > + return -EINVAL;
> > +
> > + mutex_lock(&st->lock);
> > + ret = regmap_write(st->regmap, iattr->address,
> > + data / THRESHOLD_MILLIVOLT_PER_LSB);
> > + mutex_unlock(&st->lock);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + return len;
> > +}
> > +
> > static ssize_t
> > in_angl1_thresh_rising_value_available_show(struct device *dev,
> > struct device_attribute *attr,
> > @@ -906,6 +905,14 @@ IIO_CONST_ATTR(in_phase0_mag_value_available,
> > IIO_CONST_ATTR(in_altvoltage0_thresh_falling_value_available, THRESHOLD_RANGE_STR);
> > IIO_CONST_ATTR(in_altvoltage0_thresh_rising_value_available, THRESHOLD_RANGE_STR);
> > IIO_CONST_ATTR(in_altvoltage0_mag_value_available, THRESHOLD_RANGE_STR);
> > +IIO_DEVICE_ATTR(in_altvoltage0_mag_reset_max, 0644,
> > + event_attr_voltage_reg_show, event_attr_voltage_reg_store,
> > + AD2S1210_REG_DOS_RST_MAX_THRD);
> > +IIO_CONST_ATTR(in_altvoltage0_mag_reset_max_available, THRESHOLD_RANGE_STR);
> > +IIO_DEVICE_ATTR(in_altvoltage0_mag_reset_min, 0644,
> > + event_attr_voltage_reg_show, event_attr_voltage_reg_store,
> > + AD2S1210_REG_DOS_RST_MIN_THRD);
> > +IIO_CONST_ATTR(in_altvoltage0_mag_reset_min_available, THRESHOLD_RANGE_STR);
> > IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0);
> > IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0);
These attribute names don't match the documentation above. :-/
> >
> > @@ -914,6 +921,10 @@ static struct attribute *ad2s1210_event_attributes[] = {
> > &iio_const_attr_in_altvoltage0_thresh_falling_value_available.dev_attr.attr,
> > &iio_const_attr_in_altvoltage0_thresh_rising_value_available.dev_attr.attr,
> > &iio_const_attr_in_altvoltage0_mag_value_available.dev_attr.attr,
> > + &iio_dev_attr_in_altvoltage0_mag_reset_max.dev_attr.attr,
> > + &iio_const_attr_in_altvoltage0_mag_reset_max_available.dev_attr.attr,
> > + &iio_dev_attr_in_altvoltage0_mag_reset_min.dev_attr.attr,
> > + &iio_const_attr_in_altvoltage0_mag_reset_min_available.dev_attr.attr,
> > &iio_dev_attr_in_angl1_thresh_rising_value_available.dev_attr.attr,
> > &iio_dev_attr_in_angl1_thresh_rising_hysteresis_available.dev_attr.attr,
> > NULL,
> >
>
Powered by blists - more mailing lists