[PATCH 1/2] [PATCH] gpio: Replace usage of bare 'unsigned' with 'unsigned int'

[PATCH 1/2] [PATCH] gpio: Replace usage of bare 'unsigned' with 'unsigned int'

[Hennie Muller]

Fixes a couple of warnings by checkpatch and sparse.

Signed-off-by: Hennie Muller <hm@bitlabs.co.za>
---
 drivers/gpio/gpio-viperboard.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpio-viperboard.c b/drivers/gpio/gpio-viperboard.c
index 9b604f13e302..c301c1d56dd2 100644
--- a/drivers/gpio/gpio-viperboard.c
+++ b/drivers/gpio/gpio-viperboard.c
@@ -79,7 +79,7 @@ MODULE_PARM_DESC(gpioa_freq,
 /* ----- begin of gipo a chip -------------------------------------------- */
 
 static int vprbrd_gpioa_get(struct gpio_chip *chip,
-		unsigned offset)
+		unsigned int offset)
 {
 	int ret, answer, error = 0;
 	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
@@ -129,7 +129,7 @@ static int vprbrd_gpioa_get(struct gpio_chip *chip,
 }
 
 static void vprbrd_gpioa_set(struct gpio_chip *chip,
-		unsigned offset, int value)
+		unsigned int offset, int value)
 {
 	int ret;
 	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
@@ -170,7 +170,7 @@ static void vprbrd_gpioa_set(struct gpio_chip *chip,
 }
 
 static int vprbrd_gpioa_direction_input(struct gpio_chip *chip,
-			unsigned offset)
+			unsigned int offset)
 {
 	int ret;
 	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
@@ -207,7 +207,7 @@ static int vprbrd_gpioa_direction_input(struct gpio_chip *chip,
 }
 
 static int vprbrd_gpioa_direction_output(struct gpio_chip *chip,
-			unsigned offset, int value)
+			unsigned int offset, int value)
 {
 	int ret;
 	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
@@ -251,8 +251,8 @@ static int vprbrd_gpioa_direction_output(struct gpio_chip *chip,
 
 /* ----- begin of gipo b chip -------------------------------------------- */
 
-static int vprbrd_gpiob_setdir(struct vprbrd *vb, unsigned offset,
-	unsigned dir)
+static int vprbrd_gpiob_setdir(struct vprbrd *vb, unsigned int offset,
+	unsigned int dir)
 {
 	struct vprbrd_gpiob_msg *gbmsg = (struct vprbrd_gpiob_msg *)vb->buf;
 	int ret;
@@ -273,7 +273,7 @@ static int vprbrd_gpiob_setdir(struct vprbrd *vb, unsigned offset,
 }
 
 static int vprbrd_gpiob_get(struct gpio_chip *chip,
-		unsigned offset)
+		unsigned int offset)
 {
 	int ret;
 	u16 val;
@@ -305,7 +305,7 @@ static int vprbrd_gpiob_get(struct gpio_chip *chip,
 }
 
 static void vprbrd_gpiob_set(struct gpio_chip *chip,
-		unsigned offset, int value)
+		unsigned int offset, int value)
 {
 	int ret;
 	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
@@ -338,7 +338,7 @@ static void vprbrd_gpiob_set(struct gpio_chip *chip,
 }
 
 static int vprbrd_gpiob_direction_input(struct gpio_chip *chip,
-			unsigned offset)
+			unsigned int offset)
 {
 	int ret;
 	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
@@ -359,7 +359,7 @@ static int vprbrd_gpiob_direction_input(struct gpio_chip *chip,
 }
 
 static int vprbrd_gpiob_direction_output(struct gpio_chip *chip,
-			unsigned offset, int value)
+			unsigned int offset, int value)
 {
 	int ret;
 	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
-- 
2.22.0

[PATCH 2/2] [PATCH] gpio: Explicitly cast from __be16 to unsigned short

[Hennie Muller]

cpu_to_be16 returns a __be16 value. This does not break anything
but does cause sparse to generate unnecessary warnings.

Signed-off-by: Hennie Muller <hm@bitlabs.co.za>
---
 drivers/gpio/gpio-viperboard.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-viperboard.c b/drivers/gpio/gpio-viperboard.c
index c301c1d56dd2..7ff80d18bb70 100644
--- a/drivers/gpio/gpio-viperboard.c
+++ b/drivers/gpio/gpio-viperboard.c
@@ -258,8 +258,8 @@ static int vprbrd_gpiob_setdir(struct vprbrd *vb, unsigned int offset,
 	int ret;
 
 	gbmsg->cmd = VPRBRD_GPIOB_CMD_SETDIR;
-	gbmsg->val = cpu_to_be16(dir << offset);
-	gbmsg->mask = cpu_to_be16(0x0001 << offset);
+	gbmsg->val = (__force u16)cpu_to_be16(dir << offset);
+	gbmsg->mask = (__force u16)cpu_to_be16(0x0001 << offset);
 
 	ret = usb_control_msg(vb->usb_dev, usb_sndctrlpipe(vb->usb_dev, 0),
 		VPRBRD_USB_REQUEST_GPIOB, VPRBRD_USB_TYPE_OUT, 0x0000,
@@ -321,8 +321,8 @@ static void vprbrd_gpiob_set(struct gpio_chip *chip,
 		mutex_lock(&vb->lock);
 
 		gbmsg->cmd = VPRBRD_GPIOB_CMD_SETVAL;
-		gbmsg->val = cpu_to_be16(value << offset);
-		gbmsg->mask = cpu_to_be16(0x0001 << offset);
+		gbmsg->val = (__force u16)cpu_to_be16(value << offset);
+		gbmsg->mask = (__force u16)cpu_to_be16(0x0001 << offset);
 
 		ret = usb_control_msg(vb->usb_dev,
 			usb_sndctrlpipe(vb->usb_dev, 0),
-- 
2.22.0

Re: [PATCH 1/2] [PATCH] gpio: Replace usage of bare 'unsigned' with 'unsigned int'

[Phil Reid]

G'day Hennie,

patch title should be:
gpio: viperboard: Replace usage of bare 'unsigned' with 'unsigned int'

On 21/07/2019 20:52, Hennie Muller wrote:
> Fixes a couple of warnings by checkpatch and sparse.
> 
> Signed-off-by: Hennie Muller <hm@bitlabs.co.za>
> ---
>   drivers/gpio/gpio-viperboard.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-viperboard.c b/drivers/gpio/gpio-viperboard.c
> index 9b604f13e302..c301c1d56dd2 100644
> --- a/drivers/gpio/gpio-viperboard.c
> +++ b/drivers/gpio/gpio-viperboard.c
> @@ -79,7 +79,7 @@ MODULE_PARM_DESC(gpioa_freq,
>   /* ----- begin of gipo a chip -------------------------------------------- */
>   
>   static int vprbrd_gpioa_get(struct gpio_chip *chip,
> -		unsigned offset)
> +		unsigned int offset)

I've encountered these checkpatch warnings as well.

However 'struct gpio_chip' callbacks define the function signatures
as 'unsigned', not 'unsigned int'. So I've also left them as is, to explicitly
match the struct definition.

Be interested to know what the official take on this is.


>   {
>   	int ret, answer, error = 0;
>   	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
> @@ -129,7 +129,7 @@ static int vprbrd_gpioa_get(struct gpio_chip *chip,
>   }
>   
>   static void vprbrd_gpioa_set(struct gpio_chip *chip,
> -		unsigned offset, int value)
> +		unsigned int offset, int value)
>   {
>   	int ret;
>   	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
> @@ -170,7 +170,7 @@ static void vprbrd_gpioa_set(struct gpio_chip *chip,
>   }
>   
>   static int vprbrd_gpioa_direction_input(struct gpio_chip *chip,
> -			unsigned offset)
> +			unsigned int offset)
>   {
>   	int ret;
>   	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
> @@ -207,7 +207,7 @@ static int vprbrd_gpioa_direction_input(struct gpio_chip *chip,
>   }
>   
>   static int vprbrd_gpioa_direction_output(struct gpio_chip *chip,
> -			unsigned offset, int value)
> +			unsigned int offset, int value)
>   {
>   	int ret;
>   	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
> @@ -251,8 +251,8 @@ static int vprbrd_gpioa_direction_output(struct gpio_chip *chip,
>   
>   /* ----- begin of gipo b chip -------------------------------------------- */
>   
> -static int vprbrd_gpiob_setdir(struct vprbrd *vb, unsigned offset,
> -	unsigned dir)
> +static int vprbrd_gpiob_setdir(struct vprbrd *vb, unsigned int offset,
> +	unsigned int dir)
>   {
>   	struct vprbrd_gpiob_msg *gbmsg = (struct vprbrd_gpiob_msg *)vb->buf;
>   	int ret;
> @@ -273,7 +273,7 @@ static int vprbrd_gpiob_setdir(struct vprbrd *vb, unsigned offset,
>   }
>   
>   static int vprbrd_gpiob_get(struct gpio_chip *chip,
> -		unsigned offset)
> +		unsigned int offset)
>   {
>   	int ret;
>   	u16 val;
> @@ -305,7 +305,7 @@ static int vprbrd_gpiob_get(struct gpio_chip *chip,
>   }
>   
>   static void vprbrd_gpiob_set(struct gpio_chip *chip,
> -		unsigned offset, int value)
> +		unsigned int offset, int value)
>   {
>   	int ret;
>   	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
> @@ -338,7 +338,7 @@ static void vprbrd_gpiob_set(struct gpio_chip *chip,
>   }
>   
>   static int vprbrd_gpiob_direction_input(struct gpio_chip *chip,
> -			unsigned offset)
> +			unsigned int offset)
>   {
>   	int ret;
>   	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
> @@ -359,7 +359,7 @@ static int vprbrd_gpiob_direction_input(struct gpio_chip *chip,
>   }
>   
>   static int vprbrd_gpiob_direction_output(struct gpio_chip *chip,
> -			unsigned offset, int value)
> +			unsigned int offset, int value)
>   {
>   	int ret;
>   	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
> 


-- 
Regards
Phil Reid

Re: [PATCH 1/2] [PATCH] gpio: Replace usage of bare 'unsigned' with 'unsigned int'

[Hennie Muller]

On Mon, Jul 22, 2019 at 10:05:00AM +0800, Phil Reid wrote:
> G'day Hennie,
> 
> patch title should be:
> gpio: viperboard: Replace usage of bare 'unsigned' with 'unsigned int'
Thanks Phil.

I'll go read up a bit on amending commit messages for review.

> 
> On 21/07/2019 20:52, Hennie Muller wrote:
> > Fixes a couple of warnings by checkpatch and sparse.
> > 
> > Signed-off-by: Hennie Muller <hm@bitlabs.co.za>
> > ---
> >   drivers/gpio/gpio-viperboard.c | 20 ++++++++++----------
> >   1 file changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpio-viperboard.c b/drivers/gpio/gpio-viperboard.c
> > index 9b604f13e302..c301c1d56dd2 100644
> > --- a/drivers/gpio/gpio-viperboard.c
> > +++ b/drivers/gpio/gpio-viperboard.c
> > @@ -79,7 +79,7 @@ MODULE_PARM_DESC(gpioa_freq,
> >   /* ----- begin of gipo a chip -------------------------------------------- */
> >   static int vprbrd_gpioa_get(struct gpio_chip *chip,
> > -		unsigned offset)
> > +		unsigned int offset)
> 
> I've encountered these checkpatch warnings as well.
> 
> However 'struct gpio_chip' callbacks define the function signatures
> as 'unsigned', not 'unsigned int'. So I've also left them as is, to explicitly
> match the struct definition.
> 
> Be interested to know what the official take on this is.
In hindsight, I saw most of the other gpio drivers follow the same
convention as the viperboard driver. which means
a) my changes add no value and just creates inconsistency.
or
b) there's an opportunity to fix up the rest of the gpio drivers as
well? Which I'll be happy to do.

I'll be eagerly awaiting feedback.
> 
> 
> >   {
> >   	int ret, answer, error = 0;
> >   	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
> > @@ -129,7 +129,7 @@ static int vprbrd_gpioa_get(struct gpio_chip *chip,
> >   }
> >   static void vprbrd_gpioa_set(struct gpio_chip *chip,
> > -		unsigned offset, int value)
> > +		unsigned int offset, int value)
> >   {
> >   	int ret;
> >   	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
> > @@ -170,7 +170,7 @@ static void vprbrd_gpioa_set(struct gpio_chip *chip,
> >   }
> >   static int vprbrd_gpioa_direction_input(struct gpio_chip *chip,
> > -			unsigned offset)
> > +			unsigned int offset)
> >   {
> >   	int ret;
> >   	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
> > @@ -207,7 +207,7 @@ static int vprbrd_gpioa_direction_input(struct gpio_chip *chip,
> >   }
> >   static int vprbrd_gpioa_direction_output(struct gpio_chip *chip,
> > -			unsigned offset, int value)
> > +			unsigned int offset, int value)
> >   {
> >   	int ret;
> >   	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
> > @@ -251,8 +251,8 @@ static int vprbrd_gpioa_direction_output(struct gpio_chip *chip,
> >   /* ----- begin of gipo b chip -------------------------------------------- */
> > -static int vprbrd_gpiob_setdir(struct vprbrd *vb, unsigned offset,
> > -	unsigned dir)
> > +static int vprbrd_gpiob_setdir(struct vprbrd *vb, unsigned int offset,
> > +	unsigned int dir)
> >   {
> >   	struct vprbrd_gpiob_msg *gbmsg = (struct vprbrd_gpiob_msg *)vb->buf;
> >   	int ret;
> > @@ -273,7 +273,7 @@ static int vprbrd_gpiob_setdir(struct vprbrd *vb, unsigned offset,
> >   }
> >   static int vprbrd_gpiob_get(struct gpio_chip *chip,
> > -		unsigned offset)
> > +		unsigned int offset)
> >   {
> >   	int ret;
> >   	u16 val;
> > @@ -305,7 +305,7 @@ static int vprbrd_gpiob_get(struct gpio_chip *chip,
> >   }
> >   static void vprbrd_gpiob_set(struct gpio_chip *chip,
> > -		unsigned offset, int value)
> > +		unsigned int offset, int value)
> >   {
> >   	int ret;
> >   	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
> > @@ -338,7 +338,7 @@ static void vprbrd_gpiob_set(struct gpio_chip *chip,
> >   }
> >   static int vprbrd_gpiob_direction_input(struct gpio_chip *chip,
> > -			unsigned offset)
> > +			unsigned int offset)
> >   {
> >   	int ret;
> >   	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
> > @@ -359,7 +359,7 @@ static int vprbrd_gpiob_direction_input(struct gpio_chip *chip,
> >   }
> >   static int vprbrd_gpiob_direction_output(struct gpio_chip *chip,
> > -			unsigned offset, int value)
> > +			unsigned int offset, int value)
> >   {
> >   	int ret;
> >   	struct vprbrd_gpio *gpio = gpiochip_get_data(chip);
> > 
> 
> 
> -- 
> Regards
> Phil Reid

Re: [PATCH 2/2] [PATCH] gpio: Explicitly cast from __be16 to unsigned short

[Linus Walleij]

On Sun, Jul 21, 2019 at 2:53 PM Hennie Muller <hm@bitlabs.co.za> wrote:

> cpu_to_be16 returns a __be16 value. This does not break anything
> but does cause sparse to generate unnecessary warnings.
>
> Signed-off-by: Hennie Muller <hm@bitlabs.co.za>
(...)

> -       gbmsg->val = cpu_to_be16(dir << offset);
> -       gbmsg->mask = cpu_to_be16(0x0001 << offset);
> +       gbmsg->val = (__force u16)cpu_to_be16(dir << offset);
> +       gbmsg->mask = (__force u16)cpu_to_be16(0x0001 << offset);

Ugh I don't understand this.

Arnd: you know this better than me: is this the right thing to do
to get rid of sparse warnings from the code?

Yours,
Linus Walleij

Re: [PATCH 1/2] [PATCH] gpio: Replace usage of bare 'unsigned' with 'unsigned int'

[Linus Walleij]

On Mon, Jul 22, 2019 at 6:52 AM Hennie Muller <hm@bitlabs.co.za> wrote:
> On Mon, Jul 22, 2019 at 10:05:00AM +0800, Phil Reid wrote:

> > I've encountered these checkpatch warnings as well.
> >
> > However 'struct gpio_chip' callbacks define the function signatures
> > as 'unsigned', not 'unsigned int'. So I've also left them as is, to explicitly
> > match the struct definition.
> >
> > Be interested to know what the official take on this is.
> In hindsight, I saw most of the other gpio drivers follow the same
> convention as the viperboard driver. which means
> a) my changes add no value and just creates inconsistency.
> or
> b) there's an opportunity to fix up the rest of the gpio drivers as
> well? Which I'll be happy to do.

I think it is fine to fix this in drivers and we can fix the prototypes
as well.

Yours,
Linus Walleij

Re: [PATCH 1/2] [PATCH] gpio: Replace usage of bare 'unsigned' with 'unsigned int'

[Linus Walleij]

On Sun, Jul 21, 2019 at 2:53 PM Hennie Muller <hm@bitlabs.co.za> wrote:

> Fixes a couple of warnings by checkpatch and sparse.
>
> Signed-off-by: Hennie Muller <hm@bitlabs.co.za>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH 2/2] [PATCH] gpio: Explicitly cast from __be16 to unsigned short

[Arnd Bergmann]

On Mon, Aug 5, 2019 at 11:11 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Sun, Jul 21, 2019 at 2:53 PM Hennie Muller <hm@bitlabs.co.za> wrote:
>
> > cpu_to_be16 returns a __be16 value. This does not break anything
> > but does cause sparse to generate unnecessary warnings.
> >
> > Signed-off-by: Hennie Muller <hm@bitlabs.co.za>
> (...)
>
> > -       gbmsg->val = cpu_to_be16(dir << offset);
> > -       gbmsg->mask = cpu_to_be16(0x0001 << offset);
> > +       gbmsg->val = (__force u16)cpu_to_be16(dir << offset);
> > +       gbmsg->mask = (__force u16)cpu_to_be16(0x0001 << offset);
>
> Ugh I don't understand this.
>
> Arnd: you know this better than me: is this the right thing to do
> to get rid of sparse warnings from the code?

No, the structure should be modified to use __be16 fields instead of u16.

        Arnd