| 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: <492e8f347d4682ff672f81151ab4b1fe7f5b3059.camel@gmail.com>
Date: Tue, 17 Oct 2023 10:26:43 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: David Lechner <dlechner@...libre.com>, linux-iio@...r.kernel.org
Cc: Jonathan Cameron <jic23@...nel.org>,
Michael Hennerich <Michael.Hennerich@...log.com>,
nuno.sa@...log.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] iio: resolver: ad2s1210: add support for
adi,fixed-mode
On Mon, 2023-10-16 at 08:54 -0500, David Lechner wrote:
> It is possible to use the AD2S1210 with hardwired mode pins (A0 and A1).
> According to the devicetree bindings, in this case the adi,fixed-mode
> property will specify which of the 3 possible modes the mode pins are
> hardwired for and the gpio-modes property is not allowed.
>
> This adds support for the case where the mode pins are hardwired for
> config mode. In this configuration, the position and value must be read
> from the config register.
>
> The case of hardwired position or velocity mode is not supported as
> there would be no way to configure the device.
>
> Signed-off-by: David Lechner <dlechner@...libre.com>
> ---
>
LGTM
Reviewed-by: Nuno Sa <nuno.sa@...log.com>
> v2 changes:
> * Use regmap_bulk_read() instead of new local function.
> * Simplify adi,fixed-mode property error checking.
>
> drivers/iio/resolver/ad2s1210.c | 150 +++++++++++++++++++++++++-------
> 1 file changed, 119 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/iio/resolver/ad2s1210.c b/drivers/iio/resolver/ad2s1210.c
> index 1bd1b950e7cc..7f688bfe2172 100644
> --- a/drivers/iio/resolver/ad2s1210.c
> +++ b/drivers/iio/resolver/ad2s1210.c
> @@ -141,7 +141,7 @@ struct ad2s1210_state {
> struct spi_device *sdev;
> /** GPIO pin connected to SAMPLE line. */
> struct gpio_desc *sample_gpio;
> - /** GPIO pins connected to A0 and A1 lines. */
> + /** GPIO pins connected to A0 and A1 lines (optional). */
> struct gpio_descs *mode_gpios;
> /** Used to access config registers. */
> struct regmap *regmap;
> @@ -149,6 +149,8 @@ struct ad2s1210_state {
> unsigned long clkin_hz;
> /** Available raw hysteresis values based on resolution. */
> int hysteresis_available[2];
> + /* adi,fixed-mode property - only valid when mode_gpios == NULL. */
> + enum ad2s1210_mode fixed_mode;
> /** The selected resolution */
> enum ad2s1210_resolution resolution;
> /** Copy of fault register from the previous read. */
> @@ -175,6 +177,9 @@ static int ad2s1210_set_mode(struct ad2s1210_state *st,
> enum ad2s1210_mode mode)
> struct gpio_descs *gpios = st->mode_gpios;
> DECLARE_BITMAP(bitmap, 2);
>
> + if (!gpios)
> + return mode == st->fixed_mode ? 0 : -EOPNOTSUPP;
> +
> bitmap[0] = mode;
>
> return gpiod_set_array_value(gpios->ndescs, gpios->desc, gpios->info,
> @@ -276,7 +281,8 @@ static int ad2s1210_regmap_reg_read(void *context,
> unsigned int reg,
> * parity error. The fault register is read-only and the D7 bit means
> * something else there.
> */
> - if (reg != AD2S1210_REG_FAULT && st->rx[1] & AD2S1210_ADDRESS_DATA)
> + if ((reg > AD2S1210_REG_VELOCITY_LSB && reg != AD2S1210_REG_FAULT)
> + && st->rx[1] & AD2S1210_ADDRESS_DATA)
> return -EBADMSG;
>
> *val = st->rx[1];
> @@ -450,21 +456,53 @@ static int ad2s1210_single_conversion(struct iio_dev
> *indio_dev,
> ad2s1210_toggle_sample_line(st);
> timestamp = iio_get_time_ns(indio_dev);
>
> - switch (chan->type) {
> - case IIO_ANGL:
> - ret = ad2s1210_set_mode(st, MOD_POS);
> - break;
> - case IIO_ANGL_VEL:
> - ret = ad2s1210_set_mode(st, MOD_VEL);
> - break;
> - default:
> - return -EINVAL;
> + if (st->fixed_mode == MOD_CONFIG) {
> + unsigned int reg_val;
> +
> + switch (chan->type) {
> + case IIO_ANGL:
> + ret = regmap_bulk_read(st->regmap,
> + AD2S1210_REG_POSITION_MSB,
> + &st->sample.raw, 2);
> + if (ret < 0)
> + return ret;
> +
> + break;
> + case IIO_ANGL_VEL:
> + ret = regmap_bulk_read(st->regmap,
> + AD2S1210_REG_VELOCITY_MSB,
> + &st->sample.raw, 2);
> + if (ret < 0)
> + return ret;
> +
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + ret = regmap_read(st->regmap, AD2S1210_REG_FAULT, ®_val);
> + if (ret < 0)
> + return ret;
> +
> + st->sample.fault = reg_val;
> + } else {
> + switch (chan->type) {
> + case IIO_ANGL:
> + ret = ad2s1210_set_mode(st, MOD_POS);
> + break;
> + case IIO_ANGL_VEL:
> + ret = ad2s1210_set_mode(st, MOD_VEL);
> + break;
> + default:
> + return -EINVAL;
> + }
> + if (ret < 0)
> + return ret;
> +
> + ret = spi_read(st->sdev, &st->sample, 3);
> + if (ret < 0)
> + return ret;
> }
> - if (ret < 0)
> - return ret;
> - ret = spi_read(st->sdev, &st->sample, 3);
> - if (ret < 0)
> - return ret;
>
> switch (chan->type) {
> case IIO_ANGL:
> @@ -1252,27 +1290,53 @@ static irqreturn_t ad2s1210_trigger_handler(int irq,
> void *p)
> ad2s1210_toggle_sample_line(st);
>
> if (test_bit(0, indio_dev->active_scan_mask)) {
> - ret = ad2s1210_set_mode(st, MOD_POS);
> - if (ret < 0)
> - goto error_ret;
> -
> - ret = spi_read(st->sdev, &st->sample, 3);
> - if (ret < 0)
> - goto error_ret;
> + if (st->fixed_mode == MOD_CONFIG) {
> + ret = regmap_bulk_read(st->regmap,
> + AD2S1210_REG_POSITION_MSB,
> + &st->sample.raw, 2);
> + if (ret < 0)
> + goto error_ret;
> + } else {
> + ret = ad2s1210_set_mode(st, MOD_POS);
> + if (ret < 0)
> + goto error_ret;
> +
> + ret = spi_read(st->sdev, &st->sample, 3);
> + if (ret < 0)
> + goto error_ret;
> + }
>
> memcpy(&st->scan.chan[chan++], &st->sample.raw, 2);
> }
>
> if (test_bit(1, indio_dev->active_scan_mask)) {
> - ret = ad2s1210_set_mode(st, MOD_VEL);
> - if (ret < 0)
> - goto error_ret;
> + if (st->fixed_mode == MOD_CONFIG) {
> + ret = regmap_bulk_read(st->regmap,
> + AD2S1210_REG_VELOCITY_MSB,
> + &st->sample.raw, 2);
> + if (ret < 0)
> + goto error_ret;
> + } else {
> + ret = ad2s1210_set_mode(st, MOD_VEL);
> + if (ret < 0)
> + goto error_ret;
> +
> + ret = spi_read(st->sdev, &st->sample, 3);
> + if (ret < 0)
> + goto error_ret;
> + }
>
> - ret = spi_read(st->sdev, &st->sample, 3);
> + memcpy(&st->scan.chan[chan++], &st->sample.raw, 2);
> + }
> +
> + if (st->fixed_mode == MOD_CONFIG) {
> + unsigned int reg_val;
> +
> + ret = regmap_read(st->regmap, AD2S1210_REG_FAULT, ®_val);
> if (ret < 0)
> - goto error_ret;
> + return ret;
>
> - memcpy(&st->scan.chan[chan++], &st->sample.raw, 2);
> + st->sample.fault = reg_val;
> }
>
> ad2s1210_push_events(indio_dev, st->sample.fault, pf->timestamp);
> @@ -1299,9 +1363,24 @@ static const struct iio_info ad2s1210_info = {
> static int ad2s1210_setup_properties(struct ad2s1210_state *st)
> {
> struct device *dev = &st->sdev->dev;
> + const char *str_val;
> u32 val;
> int ret;
>
> + ret = device_property_read_string(dev, "adi,fixed-mode", &str_val);
> + if (ret == -EINVAL)
> + st->fixed_mode = -1;
> + else if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "failed to read adi,fixed-mode property\n");
> + else {
> + if (strcmp(str_val, "config"))
> + return dev_err_probe(dev, -EINVAL,
> + "only adi,fixed-mode=\"config\" is
> supported\n");
> +
> + st->fixed_mode = MOD_CONFIG;
> + }
> +
> ret = device_property_read_u32(dev, "assigned-resolution-bits",
> &val);
> if (ret < 0)
> return dev_err_probe(dev, ret,
> @@ -1357,12 +1436,21 @@ static int ad2s1210_setup_gpios(struct ad2s1210_state
> *st)
> "failed to request sample GPIO\n");
>
> /* both pins high means that we start in config mode */
> - st->mode_gpios = devm_gpiod_get_array(dev, "mode", GPIOD_OUT_HIGH);
> + st->mode_gpios = devm_gpiod_get_array_optional(dev, "mode",
> + GPIOD_OUT_HIGH);
> if (IS_ERR(st->mode_gpios))
> return dev_err_probe(dev, PTR_ERR(st->mode_gpios),
> "failed to request mode GPIOs\n");
>
> - if (st->mode_gpios->ndescs != 2)
> + if (!st->mode_gpios && st->fixed_mode == -1)
> + return dev_err_probe(dev, -EINVAL,
> + "must specify either adi,fixed-mode or mode-
> gpios\n");
> +
> + if (st->mode_gpios && st->fixed_mode != -1)
> + return dev_err_probe(dev, -EINVAL,
> + "must specify only one of adi,fixed-mode or mode-
> gpios\n");
> +
> + if (st->mode_gpios && st->mode_gpios->ndescs != 2)
> return dev_err_probe(dev, -EINVAL,
> "requires exactly 2 mode-gpios\n");
>
Powered by blists - more mailing lists