LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Mark Brown <broonie@kernel.org>
To: Icenowy Zheng <icenowy@aosc.io>
Cc: Liam Girdwood <lgirdwood@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Maxime Ripard <maxime.ripard@bootlin.com>,
	Chen-Yu Tsai <wens@csie.org>,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-sunxi@googlegroups.com, Ondrej Jirman <megous@megous.com>
Subject: Re: [PATCH v3 2/3] regulator: add support for SY8106A regulator
Date: Tue, 24 Apr 2018 18:07:33 +0100	[thread overview]
Message-ID: <20180424170733.GD22073@sirena.org.uk> (raw)
In-Reply-To: <20180423144657.63264-3-icenowy@aosc.io>

[-- Attachment #1: Type: text/plain, Size: 1793 bytes --]

On Mon, Apr 23, 2018 at 10:46:56PM +0800, Icenowy Zheng wrote:

> --- /dev/null
> +++ b/drivers/regulator/sy8106a-regulator.c
> @@ -0,0 +1,176 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * sy8106a-regulator.c - Regulator device driver for SY8106A

Just make the entire thing a C++ comment so it looks consistent and
joined up.

> +static int sy8106a_set_voltage_sel(struct regulator_dev *rdev, unsigned int sel)
> +{
> +	/* We use our set_voltage_sel in order to avoid unnecessary I2C
> +	 * chatter, because the regulator_get_voltage_sel_regmap using
> +	 * apply_bit would perform 4 unnecessary transfers instead of one,
> +	 * increasing the chance of error.
> +	 */
> +	return regmap_write(rdev->regmap, rdev->desc->vsel_reg,
> +			    sel | SY8106A_GO_BIT);

Why would it do these extra transfers?  Is this just the fact that you
didn't set up a register cache (though the r/m/w cycle should only add
the read)?  We could put some logic in the core regmap code to detect
that an _update_bits() call is going to write to the whole register,
though it'd be even easier to just let this register be cached.

Generally if we can usefully optimize things we should do it at the
framework level.

> +	if (reg & SY8106A_GO_BIT)
> +		return reg & rdev->desc->vsel_mask;
> +	else
> +		return (chip->fixed_voltage - rdev->desc->min_uV) /
> +		       rdev->desc->uV_step;

You could use the standard get_voltage_sel() if you provide a mapping
operation that set everything with _GO_BIT set to return the fixed
voltage.  Though looking at this it seems that the fixed voltage will
always be one that could be set via the register anyway so I'm wondering
if the easiest thing here isn't to just have the driver turn off _GO_BIT
during probe() and not worry about the special case at runtime.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply	other threads:[~2018-04-24 17:07 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-23 14:46 [PATCH v3 0/3] SY8106 regulator support and enable it on Orange Pi PC Icenowy Zheng
2018-04-23 14:46 ` [PATCH v3 1/3] dt-bindings: add binding for the SY8106A voltage regulator Icenowy Zheng
2018-05-09  9:32   ` Applied "regulator: add binding for the SY8106A voltage regulator" to the regulator tree Mark Brown
2018-05-09  9:48   ` Mark Brown
2018-04-23 14:46 ` [PATCH v3 2/3] regulator: add support for SY8106A regulator Icenowy Zheng
2018-04-24 17:07   ` Mark Brown [this message]
2018-04-24 23:41     ` Icenowy Zheng
2018-04-25 10:53       ` Mark Brown
2018-04-25 10:55         ` Icenowy Zheng
2018-04-25 11:13           ` Mark Brown
2018-04-23 14:46 ` [PATCH v3 3/3] ARM: dts: sun8i: h3: Add SY8106A regulator to Orange Pi PC Icenowy Zheng
2018-04-23 15:03   ` Maxime Ripard
2018-04-23 15:04     ` Icenowy Zheng

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=20180424170733.GD22073@sirena.org.uk \
    --to=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=icenowy@aosc.io \
    --cc=lgirdwood@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sunxi@googlegroups.com \
    --cc=maxime.ripard@bootlin.com \
    --cc=megous@megous.com \
    --cc=robh+dt@kernel.org \
    --cc=wens@csie.org \
    --subject='Re: [PATCH v3 2/3] regulator: add support for SY8106A regulator' \
    /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

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).