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

[Doug Anderson]

Hi,

On Wed, Apr 22, 2020 at 2:07 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> Quoting Doug Anderson (2020-04-22 09:09:17)
> > Hi,
> >
> > On Wed, Apr 22, 2020 at 3:23 AM Stephen Boyd <swboyd@xxxxxxxxxxxx> 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.

Hrm.  Somehow I found the wrong place when trying to search for this
previously.  Thanks for pointing me in the right directions.  Much
nicer.


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

OK, added a whole bunch more comments.

Barring something coming up, I'll plan to send v3 tomorrow morning
with all your feedback addressed.  If anyone wants me to wait because
they think I need some more major change, please yell.


-Doug