All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mark Brown <broonie@kernel.org>
To: Jerome NEANNE <jneanne@baylibre.com>
Cc: lgirdwood@gmail.com, robh+dt@kernel.org, nm@ti.com,
	kristo@kernel.org, will@kernel.org, lee.jones@linaro.org,
	khilman@baylibre.com, narmstrong@baylibre.com, msp@baylibre.com,
	j-keerthy@ti.com, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [RFC PATCH 2/5] mfd: drivers: Add TI TPS65219 PMIC support
Date: Mon, 13 Jun 2022 13:57:25 +0100	[thread overview]
Message-ID: <Yqc0NQSLO2j2IHI3@sirena.org.uk> (raw)
In-Reply-To: <20220613090604.9975-3-jneanne@baylibre.com>


[-- Attachment #1.1: Type: text/plain, Size: 1619 bytes --]

On Mon, Jun 13, 2022 at 11:06:01AM +0200, Jerome NEANNE wrote:

> +/**
> + * tps65219_reg_read: Read a single tps65219 register.
> + *
> + * @tps: Device to read from.
> + * @reg: Register to read.
> + * @val: Contians the value
> + */
> +int tps65219_reg_read(struct tps65219 *tps, unsigned int reg,
> +			unsigned int *val)
> +{
> +	return regmap_read(tps->regmap, reg, val);
> +}
> +EXPORT_SYMBOL_GPL(tps65219_reg_read);

It is better practice to just expose the regmap and let the function
drivers use it, that means the function drivers can just use standard
helper functions.

> +static int tps65219_update_bits(struct tps65219 *tps, unsigned int reg,
> +		unsigned int mask, unsigned int val)
> +{
> +	int ret;
> +	unsigned int data;
> +
> +	ret = regmap_read(tps->regmap, reg, &data);
> +	if (ret) {
> +		dev_err(tps->dev, "Read from reg 0x%x failed\n", reg);
> +		return ret;
> +	}
> +
> +	data &= ~mask;
> +	data |= val & mask;
> +
> +	mutex_lock(&tps->tps_lock);
> +	ret = tps65219_reg_write(tps, reg, data);
> +	if (ret)
> +		dev_err(tps->dev, "Write for reg 0x%x failed\n", reg);
> +	mutex_unlock(&tps->tps_lock);

It's not clear what this locking is intended to protect.  It looks like
this should just be using regmap_update_bits().

> +static const struct regmap_range tps65219_yes_ranges[] = {
> +	regmap_reg_range(TPS65219_REG_INT_SOURCE, TPS65219_REG_POWER_UP_STATUS),
> +};
> +
> +static const struct regmap_access_table tps65219_volatile_table = {
> +	.yes_ranges = tps65219_yes_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(tps65219_yes_ranges),
> +};

tps65219_yes_ranges probably needs a clearer name.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID (diff)
From: Mark Brown <broonie@kernel.org>
To: Jerome NEANNE <jneanne@baylibre.com>
Cc: lgirdwood@gmail.com, robh+dt@kernel.org, nm@ti.com,
	kristo@kernel.org, will@kernel.org, lee.jones@linaro.org,
	khilman@baylibre.com, narmstrong@baylibre.com, msp@baylibre.com,
	j-keerthy@ti.com, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [RFC PATCH 2/5] mfd: drivers: Add TI TPS65219 PMIC support
Date: Mon, 13 Jun 2022 13:57:25 +0100	[thread overview]
Message-ID: <Yqc0NQSLO2j2IHI3@sirena.org.uk> (raw)
In-Reply-To: <20220613090604.9975-3-jneanne@baylibre.com>

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

On Mon, Jun 13, 2022 at 11:06:01AM +0200, Jerome NEANNE wrote:

> +/**
> + * tps65219_reg_read: Read a single tps65219 register.
> + *
> + * @tps: Device to read from.
> + * @reg: Register to read.
> + * @val: Contians the value
> + */
> +int tps65219_reg_read(struct tps65219 *tps, unsigned int reg,
> +			unsigned int *val)
> +{
> +	return regmap_read(tps->regmap, reg, val);
> +}
> +EXPORT_SYMBOL_GPL(tps65219_reg_read);

It is better practice to just expose the regmap and let the function
drivers use it, that means the function drivers can just use standard
helper functions.

> +static int tps65219_update_bits(struct tps65219 *tps, unsigned int reg,
> +		unsigned int mask, unsigned int val)
> +{
> +	int ret;
> +	unsigned int data;
> +
> +	ret = regmap_read(tps->regmap, reg, &data);
> +	if (ret) {
> +		dev_err(tps->dev, "Read from reg 0x%x failed\n", reg);
> +		return ret;
> +	}
> +
> +	data &= ~mask;
> +	data |= val & mask;
> +
> +	mutex_lock(&tps->tps_lock);
> +	ret = tps65219_reg_write(tps, reg, data);
> +	if (ret)
> +		dev_err(tps->dev, "Write for reg 0x%x failed\n", reg);
> +	mutex_unlock(&tps->tps_lock);

It's not clear what this locking is intended to protect.  It looks like
this should just be using regmap_update_bits().

> +static const struct regmap_range tps65219_yes_ranges[] = {
> +	regmap_reg_range(TPS65219_REG_INT_SOURCE, TPS65219_REG_POWER_UP_STATUS),
> +};
> +
> +static const struct regmap_access_table tps65219_volatile_table = {
> +	.yes_ranges = tps65219_yes_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(tps65219_yes_ranges),
> +};

tps65219_yes_ranges probably needs a clearer name.

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

  reply	other threads:[~2022-06-13 12:58 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-13  9:05 [RFC PATCH 0/5] Add TI TPS65219 PMIC support for AM642 SK board Jerome NEANNE
2022-06-13  9:05 ` Jerome NEANNE
2022-06-13  9:06 ` [RFC PATCH 1/5] regulator: dt-bindings: Add TI TPS65219 PMIC bindings Jerome NEANNE
2022-06-13  9:06   ` Jerome NEANNE
2022-06-16 17:52   ` Rob Herring
2022-06-16 17:52     ` Rob Herring
2022-06-13  9:06 ` [RFC PATCH 2/5] mfd: drivers: Add TI TPS65219 PMIC support Jerome NEANNE
2022-06-13  9:06   ` Jerome NEANNE
2022-06-13 12:57   ` Mark Brown [this message]
2022-06-13 12:57     ` Mark Brown
2022-06-14 21:03   ` Kevin Hilman
2022-06-14 21:03     ` Kevin Hilman
2022-06-13  9:06 ` [RFC PATCH 3/5] regulator: drivers: Add TI TPS65219 PMIC regulators support Jerome NEANNE
2022-06-13  9:06   ` Jerome NEANNE
2022-06-13 13:04   ` Mark Brown
2022-06-13 13:04     ` Mark Brown
2022-06-13  9:06 ` [RFC PATCH 4/5] arm64: Kconfig: Introduce CONFIG_MFD_TPS65219 and CONFIG_REGULATOR_TPS65219 Jerome NEANNE
2022-06-13  9:06   ` Jerome NEANNE
2022-06-13  9:06 ` [RFC PATCH 5/5] arm64: dts: ti: Add TI TPS65219 PMIC support for AM642 SK board Jerome NEANNE
2022-06-13  9:06   ` Jerome NEANNE
2022-06-13 13:06   ` Mark Brown
2022-06-13 13:06     ` 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=Yqc0NQSLO2j2IHI3@sirena.org.uk \
    --to=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=j-keerthy@ti.com \
    --cc=jneanne@baylibre.com \
    --cc=khilman@baylibre.com \
    --cc=kristo@kernel.org \
    --cc=lee.jones@linaro.org \
    --cc=lgirdwood@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=msp@baylibre.com \
    --cc=narmstrong@baylibre.com \
    --cc=nm@ti.com \
    --cc=robh+dt@kernel.org \
    --cc=will@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 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.