On Thu, 2018-11-08 at 11:03 -0200, Giuliano Belinassi wrote:
> Previously, all pattern_masks and patterns in the chip_info table were
> hardcoded. Now they are generated using the PAT macros, as described in
> the datasheets.
One comment about indentation/whitespace.
Rest looks good.
Alex
>
> Signed-off-by: Giuliano Belinassi <giuliano.belinassi@xxxxxx>
> ---
> Changes in v2:
> - Added the PATTERN and PATTERN_MASK macros
> - Update BIT macros alignment to match with PATTERN
> - Generate pattern mask with PATTERN_MASK macros.
>
> drivers/staging/iio/adc/ad7780.c | 40 +++++++++++++++++++-------------
> 1 file changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 9ec2b002539e..ff8e3b2d0efc 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -22,14 +22,22 @@
> #include <linux/iio/sysfs.h>
> #include <linux/iio/adc/ad_sigma_delta.h>
>
> -#define AD7780_RDY BIT(7)
> -#define AD7780_FILTER BIT(6)
> -#define AD7780_ERR BIT(5)
> -#define AD7780_ID1 BIT(4)
> -#define AD7780_ID0 BIT(3)
> -#define AD7780_GAIN BIT(2)
> -#define AD7780_PAT1 BIT(1)
> -#define AD7780_PAT0 BIT(0)
> +#define AD7780_RDY BIT(7)
> +#define AD7780_FILTER BIT(6)
> +#define AD7780_ERR BIT(5)
> +#define AD7780_ID1 BIT(4)
> +#define AD7780_ID0 BIT(3)
> +#define AD7780_GAIN BIT(2)
> +#define AD7780_PAT1 BIT(1)
> +#define AD7780_PAT0 BIT(0)
Changing indentation here doesn't add much value; it's mostly
patch/whitespace noise.
While I agree that it looks nicer to indent all these to the same level,
you also need to think about the fact that the kernel git repo is already
pretty big as-is, so it's a good idea if a patch adds as much code/semantic
value [as possible] with as little change [as possible] and with as good of
readability [as possible].
[ Kind of sounds like a balance act between the 3 things ].
> +
> +#define AD7780_PATTERN (AD7780_PAT0)
> +#define AD7780_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1)
> +
> +#define AD7170_PAT2 BIT(2)
> +
> +#define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
> +#define AD7170_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
>
> struct ad7780_chip_info {
> struct iio_chan_spec channel;
> @@ -136,26 +144,26 @@ static const struct ad_sigma_delta_info
> ad7780_sigma_delta_info = {
> static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> [ID_AD7170] = {
> .channel = AD7780_CHANNEL(12, 24),
> - .pattern = 0x5,
> - .pattern_mask = 0x7,
> + .pattern = AD7170_PATTERN,
> + .pattern_mask = AD7170_PATTERN_MASK,
> .is_ad778x = false,
> },
> [ID_AD7171] = {
> .channel = AD7780_CHANNEL(16, 24),
> - .pattern = 0x5,
> - .pattern_mask = 0x7,
> + .pattern = AD7170_PATTERN,
> + .pattern_mask = AD7170_PATTERN_MASK,
> .is_ad778x = false,
> },
> [ID_AD7780] = {
> .channel = AD7780_CHANNEL(24, 32),
> - .pattern = 0x1,
> - .pattern_mask = 0x3,
> + .pattern = AD7780_PATTERN,
> + .pattern_mask = AD7780_PATTERN_MASK,
> .is_ad778x = true,
> },
> [ID_AD7781] = {
> .channel = AD7780_CHANNEL(20, 32),
> - .pattern = 0x1,
> - .pattern_mask = 0x3,
> + .pattern = AD7780_PATTERN,
> + .pattern_mask = AD7780_PATTERN_MASK,
> .is_ad778x = true,
> },
> };
