linux-kernel - Re: [PATCH v1 5/7] iio: adc: ad4170: Add GPIO controller support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VfyZ-f+KMVjyFVMitmQA65enJwHV+nBdC=XqE_Pz5cP1g@mail.gmail.com>
Date: Mon, 14 Apr 2025 17:24:56 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, jic23@...nel.org, lars@...afoo.de, 
	Michael.Hennerich@...log.com, dlechner@...libre.com, nuno.sa@...log.com, 
	andy@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, 
	marcelo.schmitt1@...il.com
Subject: Re: [PATCH v1 5/7] iio: adc: ad4170: Add GPIO controller support

On Wed, Apr 9, 2025 at 3:26 PM Marcelo Schmitt
<marcelo.schmitt@...log.com> wrote:
>
> The AD4170 has four multifunctional pins that can be used as GPIOs. The
> GPIO functionality can be accessed when the AD4170 chip is not busy
> performing continuous data capture or handling any other register
> read/write request. Also, the AD4170 does not provide any interrupt based
> on GPIO pin states so AD4170 GPIOs can't be used as interrupt sources.
>
> Implement gpio_chip callbacks so to make AD4170 GPIO pins controllable

callbacks to

> through the gpiochip interface.

...

> +static int ad4170_gpio_direction_output(struct gpio_chip *gc,
> +                                       unsigned int offset, int value)
> +{
> +       struct iio_dev *indio_dev = gpiochip_get_data(gc);
> +       struct ad4170_state *st = iio_priv(indio_dev);
> +       int ret;
> +
> +       if (!iio_device_claim_direct(indio_dev))
> +               return -EBUSY;
> +
> +       ret = regmap_clear_bits(st->regmap16, AD4170_GPIO_MODE_REG,
> +                               BIT(offset * 2));
> +       if (ret)
> +               goto err_release;
> +
> +       ret = regmap_set_bits(st->regmap16, AD4170_GPIO_MODE_REG,
> +                             BIT(offset * 2 + 1));
> +
> +err_release:
> +       iio_device_release_direct(indio_dev);
> +
> +       ad4170_gpio_set(gc, offset, value);

This is incorrect ordering, you will have glitches. Can you set the
value beforehands? Or is it broken hardware?

> +       return ret;
> +}

...

> +static int ad4170_gpio_init(struct iio_dev *indio_dev)
> +{
> +       struct ad4170_state *st = iio_priv(indio_dev);

> +       st->gpiochip = (struct gpio_chip) {
> +               .label = "ad4170_gpios",
> +               .base = -1,
> +               .ngpio = 4,
> +               .parent = &st->spi->dev,
> +               .can_sleep = true,
> +               .get_direction = ad4170_gpio_get_direction,
> +               .direction_input = ad4170_gpio_direction_input,
> +               .direction_output = ad4170_gpio_direction_output,
> +               .get = ad4170_gpio_get,
> +               .set_rv = ad4170_gpio_set,
> +               .owner = THIS_MODULE,
> +       };

I think it would be better to have it field by field initialised.

> +       return devm_gpiochip_add_data(&st->spi->dev, &st->gpiochip, indio_dev);
> +}

...

> +       /* Only create a GPIO chip if flagged for it */
> +       if (device_property_read_bool(&st->spi->dev, "gpio-controller")) {
> +               ret = ad4170_gpio_init(indio_dev);
> +               if (ret < 0)

< 0 ? What is the meaning of the positive values that you expect from
this function?

> +                       return ret;
> +       }

-- 
With Best Regards,
Andy Shevchenko