| Message ID | 20190910152855.111588-3-paul.kocialkowski@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] gpio: syscon: Add support for a custom get operation | expand |
On Tue, Sep 10, 2019 at 4:29 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> I'm fine with this for now, but the gpio-syscon driver is now growing big and when you use it you are getting support for a whole bunch of systems you're not running on included in your binary. We need to think about possibly creating drivers/gpio/syscon and split subdrivers into separate files and config options so that people can slim down to what they actually need. > + *bit = 1 << offset; Please do this: #include <linux/bits.h> *bit = BIT(offset); Yours, Linus Walleij
Hi, On Thu 12 Sep 19, 10:17, Linus Walleij wrote: > On Tue, Sep 10, 2019 at 4:29 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> > > I'm fine with this for now, but the gpio-syscon driver is now growing > big and when you use it you are getting support for a whole bunch > of systems you're not running on included in your binary. > > We need to think about possibly creating drivers/gpio/syscon > and split subdrivers into separate files and config options > so that people can slim down to what they actually need. Thanks for the review! I understand your concern about more devices getting pulled-in and built unconditionally. Though I do like the idea of having a single driver for only exposing the GPIO part of bigger components instead of specific drivers with < 100 lines of useful code. Maybe a first step would be to introduce Kconfig options for each device and ifdef around in the code, as to solve the "built unconditionally" aspect? Either way, I'll send v2 still adding my driver to gpio-syscon but feel free to have me in the loop when that driver needs to be changed. > > + *bit = 1 << offset; > > Please do this: > > #include <linux/bits.h> > > *bit = BIT(offset); Sure thing and sorry I missed that, thanks! Cheers, Paul
On Mon, Sep 23, 2019 at 3:33 PM Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote: > Maybe a first step would be to introduce Kconfig options for each device and > ifdef around in the code, as to solve the "built unconditionally" aspect? ifdefs is something we try to avoid using too much, better for things to have their own files and use a library, usually, it's cleaner. Yours, Linus Walleij
diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c index 05c537ed73f1..3d435187940b 100644 --- a/drivers/gpio/gpio-syscon.c +++ b/drivers/gpio/gpio-syscon.c @@ -190,6 +190,70 @@ 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 = 1 << offset; +} + +static int logicvc_gpio_get(struct gpio_chip *chip, unsigned offset) +{ + struct syscon_gpio_priv *priv = gpiochip_get_data(chip); + unsigned int reg; + unsigned int bit; + unsigned int 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; + unsigned int bit; + + logicvc_gpio_offset(priv, offset, ®, &bit); + + regmap_update_bits(priv->syscon, reg, bit, val ? bit : 0); +} + +static const struct syscon_gpio_data logicvc_3_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 +267,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_3_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 | 68 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+)