| Message ID | 20191128155438.325738-6-paul.kocialkowski@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | LogiCVC mfd and GPIO support | expand |
Hi Paul, thanks for your patch! On Thu, Nov 28, 2019 at 4:54 PM Paul Kocialkowski <paul.kocialkowski@bootlin.com> 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@bootlin.com> (...) > +#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? 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. Yours, Linus Walleij
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@bootlin.com> 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@bootlin.com> > (...) > > +#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
diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c index 36136b7f3a3b..cf1127f81824 100644 --- a/drivers/gpio/gpio-syscon.c +++ b/drivers/gpio/gpio-syscon.c @@ -190,6 +190,67 @@ static const struct syscon_gpio_data keystone_dsp_gpio = { .set = keystone_gpio_set, }; +#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); +} + +static int logicvc_gpio_get(struct gpio_chip *chip, unsigned offset) +{ + struct syscon_gpio_priv *priv = gpiochip_get_data(chip); + unsigned int reg, bit, value; + int ret; + + logicvc_gpio_offset(priv, offset, ®, &bit); + + ret = regmap_read(priv->syscon, reg, &value); + if (ret) + return ret; + + return !!(value & bit); +} + +static void logicvc_gpio_set(struct gpio_chip *chip, unsigned offset, int val) +{ + struct syscon_gpio_priv *priv = gpiochip_get_data(chip); + unsigned int reg, bit; + + logicvc_gpio_offset(priv, offset, ®, &bit); + + regmap_update_bits(priv->syscon, reg, bit, val ? bit : 0); +} + +static const struct syscon_gpio_data logicvc_gpio = { + .flags = GPIO_SYSCON_FEAT_OUT, + .bit_count = LOGICVC_CTRL_GPIO_BITS + LOGICVC_POWER_CTRL_GPIO_BITS, + .get = logicvc_gpio_get, + .set = logicvc_gpio_set, +}; + static const struct of_device_id syscon_gpio_ids[] = { { .compatible = "cirrus,ep7209-mctrl-gpio", @@ -203,6 +264,10 @@ static const struct of_device_id syscon_gpio_ids[] = { .compatible = "rockchip,rk3328-grf-gpio", .data = &rockchip_rk3328_gpio_mute, }, + { + .compatible = "xylon,logicvc-3.02.a-gpio", + .data = &logicvc_gpio, + }, { } }; MODULE_DEVICE_TABLE(of, syscon_gpio_ids);
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@bootlin.com> --- drivers/gpio/gpio-syscon.c | 65 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+)