linux-kernel - Re: [PATCH] iio: dac: ad559*: fixed coding style issue (shorthand unsigned)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aBY0wHEzjbBex-Hn@debian-BULLSEYE-live-builder-AMD64>
Date: Sat, 3 May 2025 12:22:40 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: akshay bansod <akshaybansod997@...il.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>,
	Michael Hennerich <Michael.Hennerich@...log.com>,
	Jonathan Cameron <jic23@...nel.org>,
	David Lechner <dlechner@...libre.com>,
	Nuno Sá <nuno.sa@...log.com>,
	Andy Shevchenko <andy@...nel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH] iio: dac: ad559*: fixed coding style issue (shorthand
 unsigned)

Hello akshay,

Overall, your patch looks good to me.
Though, I think it may get slightly better if you consider my suggestions below.

Regards,
Marcelo

On 05/03, akshay bansod wrote:
> checkpatch.pl flagged use of shorthand 'unsigned' as warning
> trivial fix: replaced use of shorthand 'unsigned' with 'unsigned int'
> 
> Signed-off-by: akshay bansod <akshaybansod997@...il.com>
> ---
...
> @@ -55,7 +55,7 @@ static int ad5592r_gpio_set(struct gpio_chip *chip, unsigned int offset,
>  	return st->ops->reg_write(st, AD5592R_REG_GPIO_SET, st->gpio_val);
>  }
>  
> -static int ad5592r_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> +static int ad5592r_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)

Hmm, the driver code was mostly contained within 80 columns before.
While it doesn't break any code style rule having it like that, I'd break the
line to keep the code under 80 columns.

static int ad5592r_gpio_direction_input(struct gpio_chip *chip,
					unsigned int offset)

Though, since that's just my personal preference, I think it should be fine if
you decide to keep the function declaration in one line.

>  {
>  	struct ad5592r_state *st = gpiochip_get_data(chip);
>  	int ret;
...
> @@ -30,7 +30,7 @@ static int ad5592r_spi_wnop_r16(struct ad5592r_state *st, __be16 *buf)
>  	return spi_sync_transfer(spi, &t, 1);
>  }
>  
> -static int ad5592r_write_dac(struct ad5592r_state *st, unsigned chan, u16 value)
> +static int ad5592r_write_dac(struct ad5592r_state *st, unsigned int chan, u16 value)

I'd also break this one, but you may chose not to if you think it's better to
keep it in one line.

>  {
>  	struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
>  
> @@ -39,7 +39,7 @@ static int ad5592r_write_dac(struct ad5592r_state *st, unsigned chan, u16 value)
>  	return spi_write(spi, &st->spi_msg, sizeof(st->spi_msg));
>  }
>  
> -static int ad5592r_read_adc(struct ad5592r_state *st, unsigned chan, u16 *value)
> +static int ad5592r_read_adc(struct ad5592r_state *st, unsigned int chan, u16 *value)

Same here.

>  {
>  	struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
>  	int ret;
...
> @@ -102,6 +102,7 @@ static const struct ad5592r_rw_ops ad5593r_rw_ops = {
>  static int ad5593r_i2c_probe(struct i2c_client *i2c)
>  {
>  	const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
> +
The addition of a blank line here is desirable, but it doesn't fit this patch's
scope. Drop that from this patch. You may propose another patch adding that if
you want.