Re: [PATCH v3 25/27] staging: iio: resolver: ad2s1210: rename DOS reset min/max attrs

[David Lechner]

On Sat, Sep 30, 2023 at 10:47 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Fri, 29 Sep 2023 12:23:30 -0500
> David Lechner <dlechner@xxxxxxxxxxxx> wrote:
>
> > From: David Lechner <david@xxxxxxxxxxxxxx>
> >
> > From: David Lechner <dlechner@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > ---
> >
> > 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@xxxxxxxxxxxxxx/>,
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@xxxxxxxxxxxxxxx
> > +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@xxxxxxxxxxxxxxx
> > +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@xxxxxxxxxxxxxxx
> > +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@xxxxxxxxxxxxxxx
> > +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,
> >
>