| Message ID | 20190927100407.1863293-1-paul.kocialkowski@bootlin.com |
|---|---|
| Headers | show |
| Series | LogiCVC mfd and GPIO support | expand |
pt., 27 wrz 2019 o 12:04 Paul Kocialkowski <paul.kocialkowski@bootlin.com> napisał(a): > > Some drivers might need a custom get operation to match custom > behavior implemented in the set operation. > > Add plumbing for supporting that. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > --- > drivers/gpio/gpio-syscon.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c > index 31f332074d7d..05c537ed73f1 100644 > --- a/drivers/gpio/gpio-syscon.c > +++ b/drivers/gpio/gpio-syscon.c > @@ -43,8 +43,9 @@ struct syscon_gpio_data { > unsigned int bit_count; > unsigned int dat_bit_offset; > unsigned int dir_bit_offset; > - void (*set)(struct gpio_chip *chip, > - unsigned offset, int value); > + int (*get)(struct gpio_chip *chip, unsigned offset); > + void (*set)(struct gpio_chip *chip, unsigned offset, > + int value); Why did you change this line? Doesn't seem necessary and pollutes the history. Bart > }; > > struct syscon_gpio_priv { > @@ -252,7 +253,7 @@ static int syscon_gpio_probe(struct platform_device *pdev) > priv->chip.label = dev_name(dev); > priv->chip.base = -1; > priv->chip.ngpio = priv->data->bit_count; > - priv->chip.get = syscon_gpio_get; > + priv->chip.get = priv->data->get ? : syscon_gpio_get; > if (priv->data->flags & GPIO_SYSCON_FEAT_IN) > priv->chip.direction_input = syscon_gpio_dir_in; > if (priv->data->flags & GPIO_SYSCON_FEAT_OUT) { > -- > 2.23.0 >
Hi Paul, just two nits: pt., 27 wrz 2019 o 12:04 Paul Kocialkowski <paul.kocialkowski@bootlin.com> napisał(a): > > 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(+) > > diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c > index 05c537ed73f1..cdcb913b8f0c 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 = 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; > + unsigned int bit; > + unsigned int value; Can you put these on a single line? > + 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; Same here. Bart > + > + 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 +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_gpio, > + }, > { } > }; > MODULE_DEVICE_TABLE(of, syscon_gpio_ids); > -- > 2.23.0 >
Hi, On Thu 03 Oct 19, 10:24, Bartosz Golaszewski wrote: > pt., 27 wrz 2019 o 12:04 Paul Kocialkowski > <paul.kocialkowski@bootlin.com> napisał(a): > > > > Some drivers might need a custom get operation to match custom > > behavior implemented in the set operation. > > > > Add plumbing for supporting that. > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > --- > > drivers/gpio/gpio-syscon.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c > > index 31f332074d7d..05c537ed73f1 100644 > > --- a/drivers/gpio/gpio-syscon.c > > +++ b/drivers/gpio/gpio-syscon.c > > @@ -43,8 +43,9 @@ struct syscon_gpio_data { > > unsigned int bit_count; > > unsigned int dat_bit_offset; > > unsigned int dir_bit_offset; > > - void (*set)(struct gpio_chip *chip, > > - unsigned offset, int value); > > + int (*get)(struct gpio_chip *chip, unsigned offset); > > + void (*set)(struct gpio_chip *chip, unsigned offset, > > + int value); > > Why did you change this line? Doesn't seem necessary and pollutes the history. This is for consistency since both the "chip" and "offset" arguments can fit in a single line. Since I want the "get" addition to fit in a single line, bringing back "offset" on the previous line of "set" makes things consistent. There's probably no particular reason for the split in the first place. Do you think it needs a separate cosmetic commit only for that? I'd rather add a note in the commit message and keep the change as-is. Cheers, Paul > Bart > > > }; > > > > struct syscon_gpio_priv { > > @@ -252,7 +253,7 @@ static int syscon_gpio_probe(struct platform_device *pdev) > > priv->chip.label = dev_name(dev); > > priv->chip.base = -1; > > priv->chip.ngpio = priv->data->bit_count; > > - priv->chip.get = syscon_gpio_get; > > + priv->chip.get = priv->data->get ? : syscon_gpio_get; > > if (priv->data->flags & GPIO_SYSCON_FEAT_IN) > > priv->chip.direction_input = syscon_gpio_dir_in; > > if (priv->data->flags & GPIO_SYSCON_FEAT_OUT) { > > -- > > 2.23.0 > >
Hi and thanks for the review! On Thu 03 Oct 19, 10:26, Bartosz Golaszewski wrote: > Hi Paul, > > just two nits: > > pt., 27 wrz 2019 o 12:04 Paul Kocialkowski > <paul.kocialkowski@bootlin.com> napisał(a): > > > > 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(+) > > > > diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c > > index 05c537ed73f1..cdcb913b8f0c 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 = 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; > > + unsigned int bit; > > + unsigned int value; > > Can you put these on a single line? Sure thing. > > + 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; > > Same here. Will do! Cheers, Paul > > + > > + 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 +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_gpio, > > + }, > > { } > > }; > > MODULE_DEVICE_TABLE(of, syscon_gpio_ids); > > -- > > 2.23.0 > >
czw., 3 paź 2019 o 13:26 Paul Kocialkowski <paul.kocialkowski@bootlin.com> napisał(a): > > Hi, > > On Thu 03 Oct 19, 10:24, Bartosz Golaszewski wrote: > > pt., 27 wrz 2019 o 12:04 Paul Kocialkowski > > <paul.kocialkowski@bootlin.com> napisał(a): > > > > > > Some drivers might need a custom get operation to match custom > > > behavior implemented in the set operation. > > > > > > Add plumbing for supporting that. > > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > > --- > > > drivers/gpio/gpio-syscon.c | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c > > > index 31f332074d7d..05c537ed73f1 100644 > > > --- a/drivers/gpio/gpio-syscon.c > > > +++ b/drivers/gpio/gpio-syscon.c > > > @@ -43,8 +43,9 @@ struct syscon_gpio_data { > > > unsigned int bit_count; > > > unsigned int dat_bit_offset; > > > unsigned int dir_bit_offset; > > > - void (*set)(struct gpio_chip *chip, > > > - unsigned offset, int value); > > > + int (*get)(struct gpio_chip *chip, unsigned offset); > > > + void (*set)(struct gpio_chip *chip, unsigned offset, > > > + int value); > > > > Why did you change this line? Doesn't seem necessary and pollutes the history. > > This is for consistency since both the "chip" and "offset" arguments can fit > in a single line. Since I want the "get" addition to fit in a single line, > bringing back "offset" on the previous line of "set" makes things consistent. > There's probably no particular reason for the split in the first place. > > Do you think it needs a separate cosmetic commit only for that? > I'd rather add a note in the commit message and keep the change as-is. > The line is still broken - just in a different place. I'd prefer to leave it as it is frankly, there's nothing wrong with it. Bart > Cheers, > > Paul > > > Bart > > > > > }; > > > > > > struct syscon_gpio_priv { > > > @@ -252,7 +253,7 @@ static int syscon_gpio_probe(struct platform_device *pdev) > > > priv->chip.label = dev_name(dev); > > > priv->chip.base = -1; > > > priv->chip.ngpio = priv->data->bit_count; > > > - priv->chip.get = syscon_gpio_get; > > > + priv->chip.get = priv->data->get ? : syscon_gpio_get; > > > if (priv->data->flags & GPIO_SYSCON_FEAT_IN) > > > priv->chip.direction_input = syscon_gpio_dir_in; > > > if (priv->data->flags & GPIO_SYSCON_FEAT_OUT) { > > > -- > > > 2.23.0 > > > > > -- > Paul Kocialkowski, Bootlin > Embedded Linux and kernel engineering > https://bootlin.com
Hi, On Thu 03 Oct 19, 16:05, Bartosz Golaszewski wrote: > czw., 3 paź 2019 o 13:26 Paul Kocialkowski > <paul.kocialkowski@bootlin.com> napisał(a): > > > > Hi, > > > > On Thu 03 Oct 19, 10:24, Bartosz Golaszewski wrote: > > > pt., 27 wrz 2019 o 12:04 Paul Kocialkowski > > > <paul.kocialkowski@bootlin.com> napisał(a): > > > > > > > > Some drivers might need a custom get operation to match custom > > > > behavior implemented in the set operation. > > > > > > > > Add plumbing for supporting that. > > > > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > > > --- > > > > drivers/gpio/gpio-syscon.c | 7 ++++--- > > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c > > > > index 31f332074d7d..05c537ed73f1 100644 > > > > --- a/drivers/gpio/gpio-syscon.c > > > > +++ b/drivers/gpio/gpio-syscon.c > > > > @@ -43,8 +43,9 @@ struct syscon_gpio_data { > > > > unsigned int bit_count; > > > > unsigned int dat_bit_offset; > > > > unsigned int dir_bit_offset; > > > > - void (*set)(struct gpio_chip *chip, > > > > - unsigned offset, int value); > > > > + int (*get)(struct gpio_chip *chip, unsigned offset); > > > > + void (*set)(struct gpio_chip *chip, unsigned offset, > > > > + int value); > > > > > > Why did you change this line? Doesn't seem necessary and pollutes the history. > > > > This is for consistency since both the "chip" and "offset" arguments can fit > > in a single line. Since I want the "get" addition to fit in a single line, > > bringing back "offset" on the previous line of "set" makes things consistent. > > There's probably no particular reason for the split in the first place. > > > > Do you think it needs a separate cosmetic commit only for that? > > I'd rather add a note in the commit message and keep the change as-is. > > > > The line is still broken - just in a different place. I'd prefer to > leave it as it is frankly, there's nothing wrong with it. The point is rather that this introduces inconsistency between the two lines. It's definitely not a major issue, but I still believe it is a coding style issue. It surely doesn't hurt to fix it. Cheers, Paul > Bart > > > Cheers, > > > > Paul > > > > > Bart > > > > > > > }; > > > > > > > > struct syscon_gpio_priv { > > > > @@ -252,7 +253,7 @@ static int syscon_gpio_probe(struct platform_device *pdev) > > > > priv->chip.label = dev_name(dev); > > > > priv->chip.base = -1; > > > > priv->chip.ngpio = priv->data->bit_count; > > > > - priv->chip.get = syscon_gpio_get; > > > > + priv->chip.get = priv->data->get ? : syscon_gpio_get; > > > > if (priv->data->flags & GPIO_SYSCON_FEAT_IN) > > > > priv->chip.direction_input = syscon_gpio_dir_in; > > > > if (priv->data->flags & GPIO_SYSCON_FEAT_OUT) { > > > > -- > > > > 2.23.0 > > > > > > > > -- > > Paul Kocialkowski, Bootlin > > Embedded Linux and kernel engineering > > https://bootlin.com