Re: [PATCH v3 1/6] drm/bridge: ti-sn65dsi86: Export bridge GPIOs to Linux

[Linus Walleij <linus.walleij@linaro.org>]

On Thu, Apr 23, 2020 at 6:26 PM Douglas Anderson <dianders@chromium.org> wrote:

> The ti-sn65dsi86 MIPI DSI to eDP bridge chip has 4 pins on it that can
> be used as GPIOs in a system.  Each pin can be configured as input,
> output, or a special function for the bridge chip.  These are:
> - GPIO1: SUSPEND Input
> - GPIO2: DSIA VSYNC
> - GPIO3: DSIA HSYNC or VSYNC
> - GPIO4: PWM
>
> Let's expose these pins as GPIOs.  A few notes:
> - Access to ti-sn65dsi86 is via i2c so we set "can_sleep".
> - These pins can't be configured for IRQ.
> - There are no programmable pulls or other fancy features.
> - Keeping the bridge chip powered might be expensive.  The driver is
>   setup such that if all used GPIOs are only inputs we'll power the
>   bridge chip on just long enough to read the GPIO and then power it
>   off again.  Setting a GPIO as output will keep the bridge powered.
> - If someone releases a GPIO we'll implicitly switch it to an input so
>   we no longer need to keep the bridge powered for it.
>
> Because of all of the above limitations we just need to implement a
> bare-bones GPIO driver.  The device tree bindings already account for
> this device being a GPIO controller so we only need the driver changes
> for it.
>
> NOTE: Despite the fact that these pins are nominally muxable I don't
> believe it makes sense to expose them through the pinctrl interface as
> well as the GPIO interface.  The special functions are things that the
> bridge chip driver itself would care about and it can just configure
> the pins as needed.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Pretty cool.

I wonder if this chip could use the generic regmap GPIO helpers
that we are working on when they come around?
https://lore.kernel.org/linux-gpio/20200423174543.17161-11-michael@walle.cc/

> +#include <linux/gpio/driver.h>
> +#include <linux/gpio.h>

Only <linux/gpio/driver.h> should be needed else you are doing
something wrong.

> + * @gchip:        If we expose our GPIOs, this is used.
> + * @gchip_output: A cache of whether we've set GPIOs to output.  This
> + *                serves double-duty of keeping track of the direction and
> + *                also keeping track of whether we've incremented the
> + *                pm_runtime reference count for this pin, which we do
> + *                whenever a pin is configured as an output.

That sounds a bit hairy but I guess it's fine.

> + */
>  struct ti_sn_bridge {
>         struct device                   *dev;
>         struct regmap                   *regmap;
> @@ -102,6 +136,9 @@ struct ti_sn_bridge {
>         struct gpio_desc                *enable_gpio;
>         struct regulator_bulk_data      supplies[SN_REGULATOR_SUPPLY_NUM];
>         int                             dp_lanes;
> +
> +       struct gpio_chip                gchip;
> +       DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);

Do you really need a bitmap for 4 bits? Can't you just have something
like an u8 and check bit 0,1,2,3 ... well I suppose it has some elegance to
it as well but... hm.

> +static struct ti_sn_bridge *gchip_to_pdata(struct gpio_chip *chip)
> +{
> +       return container_of(chip, struct ti_sn_bridge, gchip);
> +}
> +
> +static int ti_sn_bridge_gpio_get_direction(struct gpio_chip *chip,
> +                                          unsigned int offset)
> +{
> +       struct ti_sn_bridge *pdata = gchip_to_pdata(chip);

Is there some specific reason why you don't just use
gpiochip_get_data()?

> +       /*
> +        * We already have to keep track of the direction because we use
> +        * that to figure out whether we've powered the device.  We can
> +        * just return that rather than (maybe) powering up the device
> +        * to ask its direction.
> +        */
> +       return test_bit(offset, pdata->gchip_output) ?
> +               GPIOF_DIR_OUT : GPIOF_DIR_IN;
> +}

Don't use these legacy defines, they are for consumers.
Use GPIO_LINE_DIRECTION_IN  and GPIO_LINE_DIRECTION_OUT.
from <linux/gpio/driver.h>

> +       ret = regmap_read(pdata->regmap, SN_GPIO_IO_REG, &val);
> +       pm_runtime_put(pdata->dev);
> +
> +       if (ret)
> +               return ret;
> +
> +       return (val >> (SN_GPIO_INPUT_SHIFT + offset)) & 1;

My preferred way to do this is:

#include <linux/bits.h>

return !!(val & BIT(SN_GPIO_INPUT_SHIFT + offset));

> +static void ti_sn_bridge_gpio_set(struct gpio_chip *chip, unsigned int offset,
> +                                 int val)
> +{
> +       struct ti_sn_bridge *pdata = gchip_to_pdata(chip);
> +       int ret;
> +
> +       if (!test_bit(offset, pdata->gchip_output)) {
> +               dev_err(pdata->dev, "Ignoring GPIO set while input\n");
> +               return;
> +       }
> +
> +       val &= 1;
> +       ret = regmap_update_bits(pdata->regmap, SN_GPIO_IO_REG,
> +                                BIT(SN_GPIO_OUTPUT_SHIFT + offset),
> +                                val << (SN_GPIO_OUTPUT_SHIFT + offset));

Looks like a job for the generic helper library.

> +static int ti_sn_bridge_gpio_direction_input(struct gpio_chip *chip,
> +                                            unsigned int offset)
> +{
> +       struct ti_sn_bridge *pdata = gchip_to_pdata(chip);
> +       int shift = offset * 2;
> +       int ret;
> +
> +       if (!test_and_clear_bit(offset, pdata->gchip_output))
> +               return 0;
> +
> +       ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
> +                                0x3 << shift, SN_GPIO_MUX_INPUT << shift);

But this 0x03 does not look very generic, it's not just 1 bit but 2.

Overall it looks good, just the minor things above need fixing or
looking into.

Yours,
Linus Walleij