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.
> 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.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
