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

[Stephen Boyd]

Quoting Doug Anderson (2020-04-22 09:09:17)
> Hi,
> 
> On Wed, Apr 22, 2020 at 3:23 AM Stephen Boyd <swboyd at chromium.org> wrote:
> >
> > Quoting Douglas Anderson (2020-04-20 22:06:17)
> >
> > > Changes in v2:
> > > - ("Export...GPIOs") is 1/2 of replacement for ("Allow...bridge GPIOs")
> > >
> > >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 165 ++++++++++++++++++++++++++
> > >  1 file changed, 165 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > index 6ad688b320ae..d04c2b83d699 100644
> > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > @@ -874,6 +886,153 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
> > >         return 0;
> > >  }
> > >
> > > +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);
> > > +
> > > +       return (atomic_read(&pdata->gchip_output) & BIT(offset)) ?
> >
> > Any reason this isn't a bitmap?
> 
> Don't bitmaps need an external lock to protect against concurrent
> access? 

Bitmaps are sometimes atomic. See Documentation/atomic_bitops.txt for
more info. From what I see here it looks like we can have a bitmap for
this and then use set_bit(), test_and_set_bit(), etc. and it will be the
same and easier to read.

> When I looked I wasn't convinced that the GPIO subsystem
> prevented two callers from changing two GPIOs at the same time.  See
> below for a bigger discussion.
> 
> 
> > > +               GPIOF_DIR_OUT : GPIOF_DIR_IN;
> >
> > And why can't we read the hardware to figure out if it's in output or
> > input mode?
> 
[...]
> 
> In the next version of the patch I'll plan to add a kerneldoc comment
> to "struct ti_sn_bridge" and add a summary of the above for
> "gchip_output".

Yeah it deserves a comment in the code indicating why it doesn't read
the hardware.

> 
> 
> > > +}
> > > +
> > [...]
> > > +static int ti_sn_bridge_gpio_direction_output(struct gpio_chip *chip,
> > > +                                             unsigned int offset, int val)
> > > +{
> > > +       struct ti_sn_bridge *pdata = gchip_to_pdata(chip);
> > > +       int shift = offset * 2;
> > > +       int old_gchip_output;
> > > +       int ret;
> > > +
> > > +       old_gchip_output = atomic_fetch_or(BIT(offset), &pdata->gchip_output);
> >
> > I presume gpiolib is already preventing a gpio from being modified twice
> > at the same time. So is this atomic stuff really necessary?
[...]
> None of these appear to do any locking.  There's sorta an implicit
> lock in that only one client can "request" a given GPIO at the same
> time so the assumption that we're somewhat protected against two
> concurrent accesses of the exact same GPIO is a bit justified.  ...but
> nothing appears to protect us from concurrent accesses of different
> GPIOs.
> 
> I also notice that other GPIO drivers seem to grab their own locks.
> If it makes the patch more palatable, I can get rid of all the atomic
> stuff and put in a big mutex?

No. I'd rather see the bitmap APIs used instead of a custom bitmap
overlaid on an atomic_t. Otherwise it seems fine to assume a GPIO can't
be touched by two entities at once and thus nothing to worry about for
locking at the driver level for that.