[v3,3/5] gpio: syscon: Add support for a custom get operation
diff mbox series

Message ID 20190927100407.1863293-4-paul.kocialkowski@bootlin.com
State New
Headers show
Series
  • LogiCVC mfd and GPIO support
Related show

Commit Message

Paul Kocialkowski Sept. 27, 2019, 10:04 a.m. UTC
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(-)

Comments

Bartosz Golaszewski Oct. 3, 2019, 8:24 a.m. UTC | #1
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
>
Paul Kocialkowski Oct. 3, 2019, 11:26 a.m. UTC | #2
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
> >
Bartosz Golaszewski Oct. 3, 2019, 2:05 p.m. UTC | #3
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
Paul Kocialkowski Oct. 3, 2019, 2:15 p.m. UTC | #4
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

Patch
diff mbox series

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);
 };
 
 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) {