| 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: <283d2478-8788-44fb-bb22-48968250d71f@baylibre.com>
Date: Tue, 7 Jan 2025 11:11:24 -0600
From: David Lechner <dlechner@...libre.com>
To: Julien Stephan <jstephan@...libre.com>,
Lars-Peter Clausen <lars@...afoo.de>,
Michael Hennerich <Michael.Hennerich@...log.com>,
Nuno Sá <nuno.sa@...log.com>,
Jonathan Cameron <jic23@...nel.org>, Jonathan Corbet <corbet@....net>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 4/5] iio: adc: ad7380: add alert support
On 1/7/25 2:48 AM, Julien Stephan wrote:
> The alert functionality is an out of range indicator and can be used as
> an early indicator of an out of bounds conversion result.
>
> ALERT_LOW_THRESHOLD and ALERT_HIGH_THRESHOLD registers are common to all
> channels.
>
> When using 1 SDO line (only mode supported by the driver right now), i.e
> data outputs only on SDOA, SDOB (or SDOD for 4 channels variants) is
> used as an alert pin. The alert pin is updated at the end of the
> conversion (set to low if an alert occurs) and is cleared on a falling
> edge of CS.
>
> The ALERT register contains information about the exact alert status:
> channel and direction. ALERT register can be accessed using debugfs if
> enabled.
>
> User can set high/low thresholds and enable alert detection using the
> regular iio events attributes:
>
> events/in_thresh_falling_value events/in_thresh_rising_value
> events/thresh_either_en
>
> In most use cases, user will hardwire the alert pin to trigger a shutdown.
>
> In theory, we could generate userspace IIO events for alerts, but this
> is not implemented yet for several reasons [1]. This can be implemented
> later if a real use case actually requires it.
>
> Signed-off-by: Julien Stephan <jstephan@...libre.com>
>
> [1] https://lore.kernel.org/all/4be16272-5197-4fa1-918c-c4cdfcaee02e@baylibre.com/
> ---
...
> +static int ad7380_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 ad7380_state *st = iio_priv(indio_dev);
> + int alert_en, tmp, ret;
> +
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD7380_REG_ADDR_CONFIG1, &tmp);
> +
> + iio_device_release_direct_mode(indio_dev);
> +
> + if (ret)
> + return ret;
> +
> + alert_en = FIELD_GET(AD7380_CONFIG1_ALERTEN, tmp);
nit: return directly and drop alter_en.
> +
> + return alert_en;
> +}
> +
> +static int ad7380_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + bool state)
> +{
> + struct ad7380_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(st->regmap,
> + AD7380_REG_ADDR_CONFIG1,
> + AD7380_CONFIG1_ALERTEN,
> + FIELD_PREP(AD7380_CONFIG1_ALERTEN, state));
> +
> + iio_device_release_direct_mode(indio_dev);
> +
> + if (ret)
> + return ret;
> +
> + return 0;
return ret;
> +}
> +
> +static int ad7380_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 ad7380_state *st = iio_priv(indio_dev);
> + int ret, tmp;
> +
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + ret = iio_device_claim_direct_mode(indio_dev);
> +
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + ret = regmap_read(st->regmap,
> + AD7380_REG_ADDR_ALERT_HIGH_TH,
> + &tmp);
> + if (ret)
> + return ret;
Can't return directly here without releasing direct mode.
Suggest to move everything between claim and release to a helper function to
simplify things (avoiding goto and break).
> +
> + *val = FIELD_GET(AD7380_ALERT_HIGH_TH, tmp);
> + ret = IIO_VAL_INT;
> + break;
> + case IIO_EV_DIR_FALLING:
> + ret = regmap_read(st->regmap,
> + AD7380_REG_ADDR_ALERT_LOW_TH,
> + &tmp);
> + if (ret)
> + return ret;
> +
> + FIELD_GET(AD7380_ALERT_LOW_TH, tmp);
> + ret = IIO_VAL_INT;
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + iio_device_release_direct_mode(indio_dev);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ad7380_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 ad7380_state *st = iio_priv(indio_dev);
> + const struct iio_scan_type *scan_type;
> + int ret;
> + u16 th;
> +
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + return ret;
> +
> + /*
> + * According to the datasheet,
> + * AD7380_REG_ADDR_ALERT_HIGH_TH[11:0] are the 12 MSB of the
> + * 16-bits internal alert high register. LSB are set to 0xf.
> + * AD7380_REG_ADDR_ALERT_LOW_TH[11:0] are the 12 MSB of the
> + * 16 bits internal alert low register. LSB are set to 0x0.
> + *
> + * When alert is enabled the conversion from the adc is compared
> + * immediately to the alert high/low thresholds, before any
> + * oversampling. This means that the thresholds are the same for
> + * normal mode and oversampling mode.
> + */
> +
> + /* Extract the 12 MSB of val */
> + scan_type = iio_get_current_scan_type(indio_dev, chan);
> + if (IS_ERR(scan_type))
> + return PTR_ERR(scan_type);
Same with this function, it isn't releasing on error, so we can use a helper
here too to solve it.
> +
> + th = val >> (scan_type->realbits - 12);
> +
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + ret = regmap_write(st->regmap,
> + AD7380_REG_ADDR_ALERT_HIGH_TH,
> + th);
> + if (ret)
> + return ret;
> +
> + break;
> + case IIO_EV_DIR_FALLING:
> + ret = regmap_write(st->regmap,
> + AD7380_REG_ADDR_ALERT_LOW_TH,
> + th);
> + if (ret)
> + return ret;
> +
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + iio_device_release_direct_mode(indio_dev);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> +}
> +
Powered by blists - more mailing lists