| 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: <CAMknhBFzsRnroXrrb84ruZctdMnMn3g3h6RavhFPhFN_JonDJw@mail.gmail.com>
Date: Sun, 17 Dec 2023 18:30:34 -0600
From: David Lechner <dlechner@...libre.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>
Cc: apw@...onical.com, joe@...ches.com, dwaipayanray1@...il.com,
lukas.bulwahn@...il.com, paul.cercueil@...log.com,
Michael.Hennerich@...log.com, lars@...afoo.de, jic23@...nel.org,
robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
dan.carpenter@...aro.org, marcelo.schmitt1@...il.com,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 15/15] iio: adc: ad7091r: Allow users to configure
device events
On Sat, Dec 16, 2023 at 11:52 AM Marcelo Schmitt
<marcelo.schmitt@...log.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@...log.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
>
Powered by blists - more mailing lists