On Tue, 2024-08-27 at 02:45 +0000, Billy Tsai wrote:
> Hi Andrew,
>
> Thanks for your suggestion. As I understand it, you’re suggesting that this driver should share the
> common parts with aspeed-gpio.c, correct?
> However, I don’t think that’s necessary. You can treat it as a new GPIO controller because the
> register layout is quite different from aspeed-gpio.c.
Well, we could, but both share a lot of the same capabilities. aspeed-
gpio.c already has to abstract over the register layout because it's so
haphazard. What I was suggesting was to formalise this a bit more by
converting some of the inline functions and macros to callbacks that
can be implemented for each controller.
I haven't tried it myself, but it feels feasible?
> If I try to make it common, the driver could become too complex, potentially requiring a separate
> gpio-aspeed-common.c and necessitating changes to the existing aspeed-gpio.c
I felt the trade-off between the volume of copy/paste and the
complexity of adding a few callbacks weighed in favour of the latter.
Also, given the volume of copy/paste, I think it would be best to
retain the copyright information from aspeed-gpio.c if the outcome is
these must be separate drivers.
Many of the changes seemed to be dealing with the difference between
`struct aspeed_gpio` and `struct aspeed_gpio_g7`, which might be
addressed by some careful struct design and use of container_of().
> Maybe the discussion of merging aspeed-gpio.c and this driver can be postponed until after this one
> is accepted?
Yeah, but I suspect the discussion just won't happen if this is merged.
Now's the time to get consensus on a way forward, while the driver is
yet to be merged.
> > > +
> > > +static const int debounce_timers[4] = { 0x00, 0x04, 0x00, 0x08 };
>
> > This is all largely copy/pasted from gpio-aspeed.c. Can we split it out
> > and share the definitions?
>
> Do you mean moving them into the common header file?
> The structure is fine, but I’m unsure about the debounce_timers.
> It’s a static array, so I don’t think it needs to be shared with gpio-aspeed.c.
That can be changed though? An appropriate pointer can be point into
the driver struct.
> > > +static int aspeed_gpio_g7_get(struct gpio_chip *gc, unsigned int offset)
> > > +{
> > > + struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > > + void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > > +
> > > + return !!(field_get(GPIO_G7_IN_DATA, ioread32(addr)));
> > > +}
> > > +
> > > +static void __aspeed_gpio_g7_set(struct gpio_chip *gc, unsigned int offset, int val)
> > > +{
> > > + struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > > + void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
>
> > The rest of the implementation of this function is broadly the same as
> > in gpio-aspeed.c. The main difference is accounting for the address to
> > access and the bit to whack. If we define some callbacks that replace
> > GPIO_BANK()/to_bank() and GPIO_BIT() that can account for the
> > differences in register layout, perhaps this could be common?
>
> > The trade-off is some complexity vs copy-paste, but there does seem to
> > be an awful lot of the latter with only minor changes so far.
>
> Do you mean I need to create a gpio-aspeed-common.c, define the necessary common APIs,
> and have gpio-aspeed.c and this driver hook into those APIs?
Well, you may not have to do that if we can put it all in gpio-
aspeed.c?
My suspicion is the g7 support could largely become some well-chosen
callbacks.
Andrew
