Re: [PATCH v4 5/5] gpio: syscon: Add support for the Xylon LogiCVC GPIOs

[Paul Kocialkowski]

Hi Linus,

On Fri 29 Nov 19, 10:24, Linus Walleij wrote:
> Hi Paul,
> 
> thanks for your patch!
> 
> On Thu, Nov 28, 2019 at 4:54 PM Paul Kocialkowski
> <paul.kocialkowski@xxxxxxxxxxx> wrote:
> 
> > The LogiCVC display hardware block comes with GPIO capabilities
> > that must be exposed separately from the main driver (as GPIOs) for
> > use with regulators and panels. A syscon is used to share the same
> > regmap across the two drivers.
> >
> > Since the GPIO capabilities are pretty simple, add them to the syscon
> > GPIO driver.
> >
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> (...)
> > +#define LOGICVC_CTRL_REG               0x40
> > +#define LOGICVC_CTRL_GPIO_SHIFT                11
> > +#define LOGICVC_CTRL_GPIO_BITS         5
> > +
> > +#define LOGICVC_POWER_CTRL_REG         0x78
> > +#define LOGICVC_POWER_CTRL_GPIO_SHIFT  0
> > +#define LOGICVC_POWER_CTRL_GPIO_BITS   4
> > +
> > +static void logicvc_gpio_offset(struct syscon_gpio_priv *priv,
> > +                               unsigned offset, unsigned int *reg,
> > +                               unsigned int *bit)
> > +{
> > +       if (offset >= LOGICVC_CTRL_GPIO_BITS) {
> > +               *reg = LOGICVC_POWER_CTRL_REG;
> > +
> > +               /* To the (virtual) power ctrl offset. */
> > +               offset -= LOGICVC_CTRL_GPIO_BITS;
> > +               /* To the actual bit offset in reg. */
> > +               offset += LOGICVC_POWER_CTRL_GPIO_SHIFT;
> > +       } else {
> > +               *reg = LOGICVC_CTRL_REG;
> > +
> > +               /* To the actual bit offset in reg. */
> > +               offset += LOGICVC_CTRL_GPIO_SHIFT;
> > +       }
> > +
> > +       *bit = BIT(offset);
> > +}
> 
> The gpio-syscon.c is for simple syscons where the lines
> you want to affect are nicely ordered in the registers.
> It is intended to be generic.
> 
> This is kind of shoehorning a special case into the generic
> code.
> 
> Isn't it more appropriate to create a specific driver for this
> hardware?

Yes I'm fine with that too. Indeed the driver has custom set/get operations
that don't really fit well into generic code.

> Special get/set quirks for any possible quirky offset is
> certainly not the way to go, if this should be supported
> we need generic properties in struct syscon_gpio_data
> to indicate the valid bits and offsets.

I guess the rationale would be to define multiple possible bit offsets for
different ranges of GPIO offsets, but I don't think it would be very useful
outside of this case.

I'll probably craft a new version with a dedicated driver then.

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Attachment: signature.asc
Description: PGP signature