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, > > >
