| Message ID | 24a9f1bb721e66df65e36797b0c3fd2ca1f95227.1702746240.git.marcelo.schmitt1@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for AD7091R-2/-4/-8 | expand |
On Sat, 16 Dec 2023 14:51:50 -0300 Marcelo Schmitt <marcelo.schmitt@analog.com> wrote: > Implement event configuration callbacks allowing users to read/write > event thresholds and enable/disable event generation. > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > --- > This is from a review suggestion David made on v3 [1]. > > Is this the case for a Suggested-by tag? > > [1]: https://lore.kernel.org/linux-iio/CAMknhBFPbAqp4-AQdmbp+VRW-Ksk1PxaLCG+3n=Zk4gyStqhgw@mail.gmail.com/#t > > drivers/iio/adc/ad7091r-base.c | 117 +++++++++++++++++++++++++++++++-- > 1 file changed, 113 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c > index 57355ca157a1..64e8baeff258 100644 > --- a/drivers/iio/adc/ad7091r-base.c > +++ b/drivers/iio/adc/ad7091r-base.c > @@ -20,19 +20,18 @@ const struct iio_event_spec ad7091r_events[] = { > { > .type = IIO_EV_TYPE_THRESH, > .dir = IIO_EV_DIR_RISING, > - .mask_separate = BIT(IIO_EV_INFO_VALUE) | > - BIT(IIO_EV_INFO_ENABLE), > + .mask_separate = BIT(IIO_EV_INFO_VALUE), This is an ABI change. So would need a really strong reason to make it... mind you - it seems like this has been broken until now anyway so this change may be fine. > }, > { > .type = IIO_EV_TYPE_THRESH, > .dir = IIO_EV_DIR_FALLING, > - .mask_separate = BIT(IIO_EV_INFO_VALUE) | > - BIT(IIO_EV_INFO_ENABLE), > + .mask_separate = BIT(IIO_EV_INFO_VALUE), > }, > { > .type = IIO_EV_TYPE_THRESH, > .dir = IIO_EV_DIR_EITHER, > .mask_separate = BIT(IIO_EV_INFO_HYSTERESIS), > + .mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE), It's relatively unusual that you can't separate the two event directions with careful control of the thresholds. So I think you can implement the existing ABI by just setting the thresholds to the either 0 or 2^12 - 1 as appropriate. The docs seem to say it must exceed the value, or fall below the min so these values should ensure it can't do either. You can then enable the event generate if one of them is set.
On Sat, Dec 16, 2023 at 11:52 AM Marcelo Schmitt <marcelo.schmitt@analog.com> wrote: > > Implement event configuration callbacks allowing users to read/write > event thresholds and enable/disable event generation. > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > --- > This is from a review suggestion David made on v3 [1]. > > Is this the case for a Suggested-by tag? > > [1]: https://lore.kernel.org/linux-iio/CAMknhBFPbAqp4-AQdmbp+VRW-Ksk1PxaLCG+3n=Zk4gyStqhgw@mail.gmail.com/#t > I suppose it fits the definition of Suggested-by well enough and would be appreciated. Even more so on [PATCH v4 02/15] "iio: adc: ad7091r: Pass iio_dev to event handler".
On Sun, Dec 17, 2023 at 5:58 PM David Lechner <dlechner@baylibre.com> wrote: > > On Sat, Dec 16, 2023 at 11:52 AM Marcelo Schmitt > <marcelo.schmitt@analog.com> wrote: > > > > Implement event configuration callbacks allowing users to read/write > > event thresholds and enable/disable event generation. > > > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > > --- > > This is from a review suggestion David made on v3 [1]. > > > > Is this the case for a Suggested-by tag? > > > > [1]: https://lore.kernel.org/linux-iio/CAMknhBFPbAqp4-AQdmbp+VRW-Ksk1PxaLCG+3n=Zk4gyStqhgw@mail.gmail.com/#t > > > > I suppose it fits the definition of Suggested-by well enough and would > be appreciated. Even more so on [PATCH v4 02/15] "iio: adc: ad7091r: > Pass iio_dev to event handler". And it seems like this should have the Fixes tag since this is fixing a null pointer dereference. And the commit message should describe the problem and that this as a fix, otherwise it sounds like we are just adding a new feature here.
On Sat, Dec 16, 2023 at 11:52 AM Marcelo Schmitt <marcelo.schmitt@analog.com> wrote: > > Implement event configuration callbacks allowing users to read/write > event thresholds and enable/disable event generation. > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > --- > This is from a review suggestion David made on v3 [1]. > > Is this the case for a Suggested-by tag? > > [1]: https://lore.kernel.org/linux-iio/CAMknhBFPbAqp4-AQdmbp+VRW-Ksk1PxaLCG+3n=Zk4gyStqhgw@mail.gmail.com/#t > > drivers/iio/adc/ad7091r-base.c | 117 +++++++++++++++++++++++++++++++-- > 1 file changed, 113 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c > index 57355ca157a1..64e8baeff258 100644 > --- a/drivers/iio/adc/ad7091r-base.c > +++ b/drivers/iio/adc/ad7091r-base.c > @@ -20,19 +20,18 @@ const struct iio_event_spec ad7091r_events[] = { > { > .type = IIO_EV_TYPE_THRESH, > .dir = IIO_EV_DIR_RISING, > - .mask_separate = BIT(IIO_EV_INFO_VALUE) | > - BIT(IIO_EV_INFO_ENABLE), > + .mask_separate = BIT(IIO_EV_INFO_VALUE), > }, > { > .type = IIO_EV_TYPE_THRESH, > .dir = IIO_EV_DIR_FALLING, > - .mask_separate = BIT(IIO_EV_INFO_VALUE) | > - BIT(IIO_EV_INFO_ENABLE), > + .mask_separate = BIT(IIO_EV_INFO_VALUE), > }, > { > .type = IIO_EV_TYPE_THRESH, > .dir = IIO_EV_DIR_EITHER, > .mask_separate = BIT(IIO_EV_INFO_HYSTERESIS), > + .mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE), > }, > }; > EXPORT_SYMBOL_NS_GPL(ad7091r_events, IIO_AD7091R); > @@ -128,8 +127,118 @@ static int ad7091r_read_raw(struct iio_dev *iio_dev, > return ret; > } > > +static int ad7091r_read_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir) > +{ > + struct ad7091r_state *st = iio_priv(indio_dev); > + unsigned int alert; > + int ret; > + > + ret = regmap_read(st->map, AD7091R_REG_CONF, &alert); > + if (ret) > + return ret; > + > + return !!(FIELD_GET(AD7091R_REG_CONF_ALERT_EN, alert)); > +} According to the datasheet, bit 4 of the config register is for selecting the function of the ALERT/BUSY/GPO0 pin as either ALERT/BUSY or GPIO, so this sounds like a pinmux function rather than an event enable function. context: `#define AD7091R_REG_CONF_ALERT_EN BIT(4)` in a previous patch > + > +static int ad7091r_write_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, int state) > +{ > + struct ad7091r_state *st = iio_priv(indio_dev); > + > + /* > + * Whenever alerts are enabled, every ADC conversion will generate > + * an alert if the conversion result for a particular channel falls > + * bellow the respective low limit register or above the respective > + * high limit register. > + * We can enable or disable all alerts by writing to the config reg. > + */ > + return regmap_update_bits(st->map, AD7091R_REG_CONF, > + AD7091R_REG_CONF_ALERT_EN, state << 4); > +} > + > +static int ad7091r_read_event_value(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + enum iio_event_info info, int *val, int *val2) > +{ > + struct ad7091r_state *st = iio_priv(indio_dev); > + int ret; > + > + switch (info) { > + case IIO_EV_INFO_VALUE: > + switch (dir) { > + case IIO_EV_DIR_RISING: > + ret = regmap_read(st->map, > + AD7091R_REG_CH_HIGH_LIMIT(chan->channel), > + val); > + if (ret) > + return ret; > + return IIO_VAL_INT; > + case IIO_EV_DIR_FALLING: > + ret = regmap_read(st->map, > + AD7091R_REG_CH_LOW_LIMIT(chan->channel), > + val); > + if (ret) > + return ret; > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > + case IIO_EV_INFO_HYSTERESIS: > + ret = regmap_read(st->map, > + AD7091R_REG_CH_HYSTERESIS(chan->channel), > + val); > + if (ret) > + return ret; > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > +} > + > +static int ad7091r_write_event_value(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + enum iio_event_info info, int val, int val2) > +{ > + struct ad7091r_state *st = iio_priv(indio_dev); > + > + switch (info) { > + case IIO_EV_INFO_VALUE: > + switch (dir) { > + case IIO_EV_DIR_RISING: > + return regmap_write(st->map, > + AD7091R_REG_CH_HIGH_LIMIT(chan->channel), > + val); > + case IIO_EV_DIR_FALLING: > + return regmap_write(st->map, > + AD7091R_REG_CH_LOW_LIMIT(chan->channel), > + val); These registers are limited to 12-bit values. Do we need to check val first and return -EINVAL if out of range? > + default: > + return -EINVAL; > + } > + case IIO_EV_INFO_HYSTERESIS: > + return regmap_write(st->map, > + AD7091R_REG_CH_HYSTERESIS(chan->channel), > + val); > + default: > + return -EINVAL; > + } > +} > + > static const struct iio_info ad7091r_info = { > .read_raw = ad7091r_read_raw, > + .read_event_config = &ad7091r_read_event_config, > + .write_event_config = &ad7091r_write_event_config, > + .read_event_value = &ad7091r_read_event_value, > + .write_event_value = &ad7091r_write_event_value, > }; > > static irqreturn_t ad7091r_event_handler(int irq, void *private) > -- > 2.42.0 >
diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c index 57355ca157a1..64e8baeff258 100644 --- a/drivers/iio/adc/ad7091r-base.c +++ b/drivers/iio/adc/ad7091r-base.c @@ -20,19 +20,18 @@ const struct iio_event_spec ad7091r_events[] = { { .type = IIO_EV_TYPE_THRESH, .dir = IIO_EV_DIR_RISING, - .mask_separate = BIT(IIO_EV_INFO_VALUE) | - BIT(IIO_EV_INFO_ENABLE), + .mask_separate = BIT(IIO_EV_INFO_VALUE), }, { .type = IIO_EV_TYPE_THRESH, .dir = IIO_EV_DIR_FALLING, - .mask_separate = BIT(IIO_EV_INFO_VALUE) | - BIT(IIO_EV_INFO_ENABLE), + .mask_separate = BIT(IIO_EV_INFO_VALUE), }, { .type = IIO_EV_TYPE_THRESH, .dir = IIO_EV_DIR_EITHER, .mask_separate = BIT(IIO_EV_INFO_HYSTERESIS), + .mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE), }, }; EXPORT_SYMBOL_NS_GPL(ad7091r_events, IIO_AD7091R); @@ -128,8 +127,118 @@ static int ad7091r_read_raw(struct iio_dev *iio_dev, return ret; } +static int ad7091r_read_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir) +{ + struct ad7091r_state *st = iio_priv(indio_dev); + unsigned int alert; + int ret; + + ret = regmap_read(st->map, AD7091R_REG_CONF, &alert); + if (ret) + return ret; + + return !!(FIELD_GET(AD7091R_REG_CONF_ALERT_EN, alert)); +} + +static int ad7091r_write_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, int state) +{ + struct ad7091r_state *st = iio_priv(indio_dev); + + /* + * Whenever alerts are enabled, every ADC conversion will generate + * an alert if the conversion result for a particular channel falls + * bellow the respective low limit register or above the respective + * high limit register. + * We can enable or disable all alerts by writing to the config reg. + */ + return regmap_update_bits(st->map, AD7091R_REG_CONF, + AD7091R_REG_CONF_ALERT_EN, state << 4); +} + +static int ad7091r_read_event_value(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info, int *val, int *val2) +{ + struct ad7091r_state *st = iio_priv(indio_dev); + int ret; + + switch (info) { + case IIO_EV_INFO_VALUE: + switch (dir) { + case IIO_EV_DIR_RISING: + ret = regmap_read(st->map, + AD7091R_REG_CH_HIGH_LIMIT(chan->channel), + val); + if (ret) + return ret; + return IIO_VAL_INT; + case IIO_EV_DIR_FALLING: + ret = regmap_read(st->map, + AD7091R_REG_CH_LOW_LIMIT(chan->channel), + val); + if (ret) + return ret; + return IIO_VAL_INT; + default: + return -EINVAL; + } + case IIO_EV_INFO_HYSTERESIS: + ret = regmap_read(st->map, + AD7091R_REG_CH_HYSTERESIS(chan->channel), + val); + if (ret) + return ret; + return IIO_VAL_INT; + default: + return -EINVAL; + } +} + +static int ad7091r_write_event_value(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info, int val, int val2) +{ + struct ad7091r_state *st = iio_priv(indio_dev); + + switch (info) { + case IIO_EV_INFO_VALUE: + switch (dir) { + case IIO_EV_DIR_RISING: + return regmap_write(st->map, + AD7091R_REG_CH_HIGH_LIMIT(chan->channel), + val); + case IIO_EV_DIR_FALLING: + return regmap_write(st->map, + AD7091R_REG_CH_LOW_LIMIT(chan->channel), + val); + default: + return -EINVAL; + } + case IIO_EV_INFO_HYSTERESIS: + return regmap_write(st->map, + AD7091R_REG_CH_HYSTERESIS(chan->channel), + val); + default: + return -EINVAL; + } +} + static const struct iio_info ad7091r_info = { .read_raw = ad7091r_read_raw, + .read_event_config = &ad7091r_read_event_config, + .write_event_config = &ad7091r_write_event_config, + .read_event_value = &ad7091r_read_event_value, + .write_event_value = &ad7091r_write_event_value, }; static irqreturn_t ad7091r_event_handler(int irq, void *private)
Implement event configuration callbacks allowing users to read/write event thresholds and enable/disable event generation. Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> --- This is from a review suggestion David made on v3 [1]. Is this the case for a Suggested-by tag? [1]: https://lore.kernel.org/linux-iio/CAMknhBFPbAqp4-AQdmbp+VRW-Ksk1PxaLCG+3n=Zk4gyStqhgw@mail.gmail.com/#t drivers/iio/adc/ad7091r-base.c | 117 +++++++++++++++++++++++++++++++-- 1 file changed, 113 insertions(+), 4 deletions(-)