Re: [PATCH 1/3] staging: iio: ad7780: Add is_ad778x flag chip info

[Ardelean, Alexandru]

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@xxxxxx>
> ---
>  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,
>  	},
>  };
>