linux-kernel - Re: [PATCH 1/3] staging: iio: ad7780: Add is

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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 8 Nov 2018 07:36:42 +0000
From:   "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
To:     "lars@...afoo.de" <lars@...afoo.de>,
        "knaack.h@....de" <knaack.h@....de>,
        "jic23@...nel.org" <jic23@...nel.org>,
        "Hennerich, Michael" <Michael.Hennerich@...log.com>,
        "renatogeh@...il.com" <renatogeh@...il.com>,
        "giuliano.belinassi@...il.com" <giuliano.belinassi@...il.com>,
        "pmeerw@...erw.net" <pmeerw@...erw.net>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
        "kernel-usp@...glegroups.com" <kernel-usp@...glegroups.com>
Subject: Re: [PATCH 1/3] staging: iio: ad7780: Add is_ad778x flag chip info

On Wed, 2018-11-07 at 16:49 -0200, Giuliano Belinassi wrote:
> This patch allows further checking of whatever the chip is (ad778x or
> ad717x).

Hey,

The patch looks good overall.
I only have one nitpick for this patch. See inline.

And you can squash this patch with patch `[PATCH 2/3] staging: iio: ad7780:
check if ad778x before gain update`.
In fact, the title of the squashed patch can just be `staging: iio: ad7780:
check if ad778x before gain update` ; because the code in this patch
implies that it's used to check if the device is an ad778x chip.

This patch doesn't have much value on it's own without the 2nd patch, and
you can do them in a single go.

/*
 * Note: yes, I know that these subtle semantics between patch 
 * splitting & squashing can be a bit annoying ; I don't have a general
 * recommendation for them, other than just to keep sending patches
 */

Thanks
Alex

> 
> Signed-off-by: Giuliano Belinassi <giuliano.belinassi@....br>
> ---
>  drivers/staging/iio/adc/ad7780.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 91e016d534ed..6e51bfdb076a 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -35,6 +35,7 @@ struct ad7780_chip_info {
>  	struct iio_chan_spec	channel;
>  	unsigned int		pattern_mask;
>  	unsigned int		pattern;
> +	u8			is_ad778x;
You could make this `bool` type since you are assigning `true/false` values
to this field. It's generally good to be consistent between type names &
type values when using them [even though in C, these are pretty much the
same].

>  };
>  
>  struct ad7780_state {
> @@ -135,21 +136,25 @@ static const struct ad7780_chip_info
> ad7780_chip_info_tbl[] = {
>  		.channel = AD7780_CHANNEL(12, 24),
>  		.pattern = 0x5,
>  		.pattern_mask = 0x7,
> +		.is_ad778x = false,
>  	},
>  	[ID_AD7171] = {
>  		.channel = AD7780_CHANNEL(16, 24),
>  		.pattern = 0x5,
>  		.pattern_mask = 0x7,
> +		.is_ad778x = false,
>  	},
>  	[ID_AD7780] = {
>  		.channel = AD7780_CHANNEL(24, 32),
>  		.pattern = 0x1,
>  		.pattern_mask = 0x3,
> +		.is_ad778x = true,
>  	},
>  	[ID_AD7781] = {
>  		.channel = AD7780_CHANNEL(20, 32),
>  		.pattern = 0x1,
>  		.pattern_mask = 0x3,
> +		.is_ad778x = true,
>  	},
>  };
>