Messages in this thread | | | From | Doug Anderson <> | Date | Wed, 22 Apr 2020 16:31:24 -0700 | Subject | Re: [PATCH v2 1/6] drm/bridge: ti-sn65dsi86: Export bridge GPIOs to Linux |
| |
Hi,
On Wed, Apr 22, 2020 at 2:07 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Doug Anderson (2020-04-22 09:09:17) > > Hi, > > > > On Wed, Apr 22, 2020 at 3:23 AM Stephen Boyd <swboyd@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.
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
| |