From: Mark Brown <broonie@kernel.org>
To: Adam Ward <adam.ward@diasemi.com>
Cc: Rob Herring <robh+dt@kernel.org>,
Liam Girdwood <lgirdwood@gmail.com>,
Vincent Whitchurch <vincent.whitchurch@axis.com>,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH 4/9] regulator: da9121: Add device variant details and respective regmaps
Date: Fri, 20 Nov 2020 12:45:52 +0000 [thread overview]
Message-ID: <20201120124552.GA6751@sirena.org.uk> (raw)
In-Reply-To: <e3f0e227c099a2dc560af5aae92f13e4d2a112cb.1605868780.git.Adam.Ward.opensource@diasemi.com>
[-- Attachment #1: Type: text/plain, Size: 907 bytes --]
On Fri, Nov 20, 2020 at 12:14:54PM +0000, Adam Ward wrote:
> Add ability to probe device and validate configuration, then apply a regmap
> configuration for a single or dual buck device accordingly.
This looks like it might benefit from being multiple commits - "X then
Y" type commit logs are often a warning sign of this, it's quite
difficult to review as it's doing several different things.
> +static int da9121_i2c_reg_read(struct i2c_client *client, u8 addr,
> + u8 *buf, int count)
> +{
> + struct i2c_msg xfer[2];
> + int ret;
Why is this open coding register I/O?
> + name = of_get_property(chip->dev->of_node, "compatible", NULL);
> + if (!name) {
> + dev_err(chip->dev, "Cannot get device not compatible string.\n");
> + goto error;
> + }
You shouldn't need to query the compatible string as a property, why is
the code doing this? You know what compatible was used from probe().
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2020-11-20 12:46 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-20 12:14 [PATCH 0/9] regulator: da9121: extend support to variants, add features Adam Ward
2020-11-20 12:14 ` [PATCH 1/9] regulator: Update DA9121 dt-bindings Adam Ward
2020-11-20 13:47 ` Vincent Whitchurch
2020-11-25 9:21 ` Vincent Whitchurch
2020-11-27 13:01 ` Adam Ward
2020-11-27 14:59 ` Mark Brown
2020-11-20 12:14 ` [PATCH 2/9] regulator: da9121: Add header file Adam Ward
2020-11-20 12:14 ` [PATCH 3/9] regulator: da9121: Add device variants Adam Ward
2020-11-20 12:14 ` [PATCH 4/9] regulator: da9121: Add device variant details and respective regmaps Adam Ward
2020-11-20 12:45 ` Mark Brown [this message]
2020-11-20 12:14 ` [PATCH 5/9] regulator: da9121: Add support for device variants via devicetree Adam Ward
2020-11-20 12:51 ` Mark Brown
2020-11-20 12:14 ` [PATCH 6/9] regulator: da9121: Update registration to support multiple buck variants Adam Ward
2020-11-20 13:06 ` Mark Brown
2020-11-20 12:14 ` [PATCH 7/9] regulator: da9121: add current support Adam Ward
2020-11-20 13:17 ` Mark Brown
2020-11-20 12:14 ` [PATCH 8/9] regulator: da9121: add mode support Adam Ward
2020-11-20 12:14 ` [PATCH 9/9] regulator: da9121: add interrupt support Adam Ward
2020-11-20 13:45 ` Mark Brown
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=20201120124552.GA6751@sirena.org.uk \
--to=broonie@kernel.org \
--cc=adam.ward@diasemi.com \
--cc=devicetree@vger.kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=vincent.whitchurch@axis.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.