Re: [PATCH v3 1/6] drm/bridge: ti-sn65dsi86: Export bridge GPIOs to Linux
From: Doug Anderson
Date: Tue Apr 28 2020 - 20:45:40 EST
Hi,
On Tue, Apr 28, 2020 at 5:44 AM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> On Thu, Apr 23, 2020 at 6:26 PM Douglas Anderson <dianders@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > Cc: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
>
> 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@xxxxxxxx/
An important part of my patch is the handling of power management.
Specifically:
* If the GPIO is an input we don't need to keep the device powered,
just power it temporarily to read the pin.
* If the GPIO is an output we do need to keep the device powered.
I suppose that could be common for other similar devices so as long as
the generic interfaces can handle this concept we can try to use it.
> > +#include <linux/gpio/driver.h>
> > +#include <linux/gpio.h>
>
> Only <linux/gpio/driver.h> should be needed else you are doing
> something wrong.
It's because I needed GPIOF_DIR_OUT / GPIOF_DIR_IN which was
apparently wrong. See below.
> > + * @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.
Doing so requires adding a lock to this driver to handle concurrent
users of the different GPIOs. I can go back and do that but I'd
rather not.
Some prior discussion:
https://lore.kernel.org/r/CAD=FV=WJONhm4ukwZa2vGtozrz_SmLuTCLxVimnGba7wRPPzgQ@xxxxxxxxxxxxxx
...if you want me to change this to a u8 + a mutex then please let me
know, otherwise I'll assume keeping it a bitmap is fine.
> > +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()?
I guess I'm used to interfaces that don't have a data pointer. I'll
change it to gpiochip_get_data() at your suggestion, though (I think)
it might be slightly less efficient (a function call and a pointer
dereference compared to a subtract operation).
> > + /*
> > + * 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>
That's what I get for reading the comments. I'll change this in the
next version. I've also sent the following patch to help keep other
people from falling into my trap:
20200428172322.1.I396f351e364f3c09df7c7606e79abefb8682c092@changeid/">https://lore.kernel.org/r/20200428172322.1.I396f351e364f3c09df7c7606e79abefb8682c092@changeid/
> > + 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));
Somehow I think of "!!" as being a bool and this function as returning
something that's logically an int. It really doesn't matter a whole
lot and I'm happy to change it, so I'll change it in the next version.
> > +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.
I think that (for now) this comment is a no-op since the generic
helper library isn't landed yet, right? ...and it wouldn't handle the
power management I need? If I'm confused and I need to act on this
comment, please let me know.
> > +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.
Sure, I can add #define SN_GPIO_MUX_MASK 0x3. Basically the mux is:
* 0: input
* 1: output
* 2: special function
As talked about in the patch comments, I don't define this as an
official pinmux driver because that seems overkill. I'll assume it's
OK to just do the #define and use it. If you want something more, let
me know.
> Overall it looks good, just the minor things above need fixing or
> looking into.
Thank you very much for the review! I'll plan to send a new patch out
in the next day or two with minor comments addressed and making the
assumptions I've documented above. If I got something wrong then
please yell. ...or yell after I send the next version and I'll send
yet another version after that! :-)
-Doug