From: Jonathan Cameron <jic23@kernel.org>
To: Billy Tsai <billy_tsai@aspeedtech.com>
Cc: <knaack.h@gmx.de>, <lars@metafoo.de>, <pmeerw@pmeerw.net>,
<robh+dt@kernel.org>, <joel@jms.id.au>, <andrew@aj.id.au>,
<p.zabel@pengutronix.de>, <alexandru.ardelean@analog.com>,
<linux-iio@vger.kernel.org>, <devicetree@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
<linux-aspeed@lists.ozlabs.org>, <linux-kernel@vger.kernel.org>,
<BMC-SW@aspeedtech.com>
Subject: Re: [PATCH 1/3] iio: adc: aspeed: Orgnaize and add the define of adc
Date: Sun, 18 Oct 2020 11:22:49 +0100 [thread overview]
Message-ID: <20201018112249.44dd3bde@archlinux> (raw)
In-Reply-To: <20201013103245.16723-2-billy_tsai@aspeedtech.com>
On Tue, 13 Oct 2020 18:32:43 +0800
Billy Tsai <billy_tsai@aspeedtech.com> wrote:
> This patch organizes the define of adc to multiple partitions
> and adds the new bit field define for ast2600 driver.
Should be 2 patch patches. If you need to do a reorg,
do it first, then add new bits in a second patch. That way
we are reviewing one non functional change, and one that actually
does something.
As a general rule, I'd also prefer to just see the additional defines
added as and when they are used (in the patch that first uses them).
>
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
> drivers/iio/adc/aspeed_adc.c | 42 ++++++++++++++++++++++++++++++++----
> 1 file changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
> index 1e5375235cfe..ae400c4d6d40 100644
> --- a/drivers/iio/adc/aspeed_adc.c
> +++ b/drivers/iio/adc/aspeed_adc.c
> @@ -21,23 +21,57 @@
> #include <linux/iio/driver.h>
> #include <linux/iopoll.h>
>
> +/**********************************************************
> + * ADC feature define
> + *********************************************************/
I'm generally of the view that block comments like this
normally imply the defines are not sufficiently self
identifying. It should be possible to know what sort of thing
they are at the point of use without having to go find a comment
in the header.
So if you think these are needed, perhaps reconsider the naming
of of the defines? Personally I just don't seem them as necessary.
Like all comments, they tend to 'rot' over time so if they
aren't adding information, better not to have them!
> #define ASPEED_RESOLUTION_BITS 10
> #define ASPEED_CLOCKS_PER_SAMPLE 12
>
> +/**********************************************************
> + * ADC HW register offset define
> + *********************************************************/
> #define ASPEED_REG_ENGINE_CONTROL 0x00
> #define ASPEED_REG_INTERRUPT_CONTROL 0x04
> #define ASPEED_REG_VGA_DETECT_CONTROL 0x08
> #define ASPEED_REG_CLOCK_CONTROL 0x0C
> +#define ASPEED_REG_COMPENSATION_TRIM 0xC4
> #define ASPEED_REG_MAX 0xC0
>
> +/**********************************************************
> + * ADC register Bit field
> + *********************************************************/
> +/*ENGINE_CONTROL */
Inconsistent spacing after /*
> +/* [0] */
> +#define ASPEED_ENGINE_ENABLE BIT(0)
> +/* [3:1] */
If the docs are needed, then better to use FIELD_PREP and GENMASK as that is
going to be self documenting at the actual point of the defines.
> #define ASPEED_OPERATION_MODE_POWER_DOWN (0x0 << 1)
> #define ASPEED_OPERATION_MODE_STANDBY (0x1 << 1)
> #define ASPEED_OPERATION_MODE_NORMAL (0x7 << 1)
> -
> -#define ASPEED_ENGINE_ENABLE BIT(0)
> -
> +/* [4] */
> +#define ASPEED_CTRL_COMPENSATION BIT(4)
> +/* [5] */
> +#define ASPEED_AUTOPENSATING BIT(5)
> +/* [7:6] */
> +#define ASPEED_REF_VOLTAGE_2500mV (0 << 6)
> +#define ASPEED_REF_VOLTAGE_1200mV (1 << 6)
> +#define ASPEED_REF_VOLTAGE_EXT_HIGH (2 << 6)
> +#define ASPEED_REF_VOLTAGE_EXT_LOW (3 << 6)
> +#define ASPEED_BATTERY_SENSING_VOL_DIVIDE_2_3 (0 << 6)
> +#define ASPEED_BATTERY_SENSING_VOL_DIVIDE_1_3 (1 << 6)
> +/* [8] */
> #define ASPEED_ADC_CTRL_INIT_RDY BIT(8)
> -
> +/* [12] */
> +#define ASPEED_ADC_CH7_VOLTAGE_NORMAL (0 << 12)
> +#define ASPEED_ADC_CH7_VOLTAGE_BATTERY (1 << 12)
> +/* [13] */
> +#define ASPEED_ADC_EN_BATTERY_SENSING BIT(13)
> +/* [31:16] */
> +#define ASPEED_ADC_CTRL_CH_EN(n) (1 << (16 + n))
> +#define ASPEED_ADC_CTRL_CH_EN_ALL GENMASK(31, 16)
> +
> +/**********************************************************
> + * Software setting
> + *********************************************************/
> #define ASPEED_ADC_INIT_POLLING_TIME 500
> #define ASPEED_ADC_INIT_TIMEOUT 500000
>
next prev parent reply other threads:[~2020-10-18 10:22 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-13 10:32 [PATCH 0/3] Make driver compatible with ast2600 Billy Tsai
2020-10-13 10:32 ` [PATCH 1/3] iio: adc: aspeed: Orgnaize and add the define of adc Billy Tsai
2020-10-18 10:22 ` Jonathan Cameron [this message]
2020-10-18 18:26 ` Andy Shevchenko
2020-10-13 10:32 ` [PATCH 2/3] iio: adc: aspeed: Modify driver for ast2600 Billy Tsai
2020-10-18 10:46 ` Jonathan Cameron
2020-10-13 10:32 ` [PATCH 3/3] iio: adc: aspeed: Setting ref_voltage in probe Billy Tsai
2020-10-16 17:16 ` Rob Herring
2020-10-18 10:55 ` 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=20201018112249.44dd3bde@archlinux \
--to=jic23@kernel.org \
--cc=BMC-SW@aspeedtech.com \
--cc=alexandru.ardelean@analog.com \
--cc=andrew@aj.id.au \
--cc=billy_tsai@aspeedtech.com \
--cc=devicetree@vger.kernel.org \
--cc=joel@jms.id.au \
--cc=knaack.h@gmx.de \
--cc=lars@metafoo.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-aspeed@lists.ozlabs.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=p.zabel@pengutronix.de \
--cc=pmeerw@pmeerw.net \
--cc=robh+dt@kernel.org \
/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).