Date:   Tue, 28 Apr 2020 17:45:00 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Dave Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Rob Herring <robh+dt@...nel.org>,
        Neil Armstrong <narmstrong@...libre.com>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Sandeep Panda <spanda@...eaurora.org>,
        Stephen Boyd <swboyd@...omium.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Jeffrey Hugo <jeffrey.l.hugo@...il.com>,
        "open list:DRM PANEL DRIVERS" <dri-devel@...ts.freedesktop.org>,
        MSM <linux-arm-msm@...r.kernel.org>,
        Rob Clark <robdclark@...omium.org>,
        Jernej Skrabec <jernej.skrabec@...l.net>,
        Jonas Karlman <jonas@...boo.se>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/6] drm/bridge: ti-sn65dsi86: Export bridge GPIOs to Linux

Hi,

On Tue, Apr 28, 2020 at 5:44 AM Linus Walleij <linus.walleij@...aro.org> wrote:
>
> On Thu, Apr 23, 2020 at 6:26 PM Douglas Anderson <dianders@...omium.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@...omium.org>
> > Cc: Linus Walleij <linus.walleij@...aro.org>
> > Cc: Bartosz Golaszewski <bgolaszewski@...libre.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/

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@mail.gmail.com

...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:

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

Powered by blists - more mailing lists