From: "Ardelean, Alexandru" <alexandru.Ardelean@analog.com>
To: "lars@metafoo.de" <lars@metafoo.de>,
"knaack.h@gmx.de" <knaack.h@gmx.de>,
"jic23@kernel.org" <jic23@kernel.org>,
"Hennerich, Michael" <Michael.Hennerich@analog.com>,
"renatogeh@gmail.com" <renatogeh@gmail.com>,
"giuliano.belinassi@gmail.com" <giuliano.belinassi@gmail.com>,
"pmeerw@pmeerw.net" <pmeerw@pmeerw.net>,
"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
"devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
"kernel-usp@googlegroups.com" <kernel-usp@googlegroups.com>
Subject: Re: [PATCH v2 2/2] staging: iio: ad7780: generates pattern_mask from PAT bits
Date: Thu, 8 Nov 2018 13:51:15 +0000 [thread overview]
Message-ID: <43efc182fc7ab9aa1d2f1ca798e27d85c7132e1f.camel@analog.com> (raw)
In-Reply-To: <ce8ebadd1638810eaaa3b0b30a961a7b3f88ae34.1541681371.git.giuliano.belinassi@usp.br>
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@usp.br>
> ---
> 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,
> },
> };
next prev parent reply other threads:[~2018-11-08 13:51 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-08 13:03 [PATCH v2 0/2] pattern generation and gain update Giuliano Belinassi
2018-11-08 13:03 ` [PATCH v2 1/2] staging: iio: ad7780: check if ad778x before " Giuliano Belinassi
2018-11-08 13:44 ` Ardelean, Alexandru
2018-11-11 12:58 ` Jonathan Cameron
2018-11-12 7:57 ` Ardelean, Alexandru
2018-11-16 18:32 ` Jonathan Cameron
2018-11-08 18:26 ` Tomasz Duszynski
2018-11-09 22:15 ` Giuliano Augusto Faulin Belinassi
2018-11-11 13:00 ` Jonathan Cameron
2018-11-08 13:03 ` [PATCH v2 2/2] staging: iio: ad7780: generates pattern_mask from PAT bits Giuliano Belinassi
2018-11-08 13:51 ` Ardelean, Alexandru [this message]
2018-11-09 22:18 ` Giuliano Augusto Faulin Belinassi
2018-11-11 13:04 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=43efc182fc7ab9aa1d2f1ca798e27d85c7132e1f.camel@analog.com \
--to=alexandru.ardelean@analog.com \
--cc=Michael.Hennerich@analog.com \
--cc=devel@driverdev.osuosl.org \
--cc=giuliano.belinassi@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=jic23@kernel.org \
--cc=kernel-usp@googlegroups.com \
--cc=knaack.h@gmx.de \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pmeerw@pmeerw.net \
--cc=renatogeh@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).