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
