Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 13 Jun 2022 13:57:25 +0100
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
Message-ID: <Yqc0NQSLO2j2IHI3@sirena.org.uk>
References: <20220613090604.9975-1-jneanne@baylibre.com>
 <20220613090604.9975-3-jneanne@baylibre.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="U2vy66mGJShhsnPN"
Content-Disposition: inline
In-Reply-To: <20220613090604.9975-3-jneanne@baylibre.com>
Precedence: bulk


--U2vy66mGJShhsnPN
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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.

--U2vy66mGJShhsnPN
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmKnNDQACgkQJNaLcl1U
h9Bqegf/anATfCIwadSoj3K2+Sm8a/mvOi3cqdrwn+9BBtO5fPW8QB0iEMMVyX9W
5+AhfxMMfQ9icgu5/ciWCWFuhQ5s0jI5xCbkCdKI9z9wXrIahFJLd0p7UN8Qszen
pGvT/TIF3PTfcQoLt7YNDxLvwNchi9A26stCHUpvZXDInzbeqSvA6JZraL8BE2bw
1uoRmHRfb28iPl8J6v9Jbhb1GOrjpzZKosWB1Awlet0COCWYSGdnRWepEGt0RCfi
JB9PmDmt9Hzx+JaDTu1+2pMiLkzCShs3Sd/oOWAiSB9PKHI73tVll1G1tJ26lWFv
aPxmboun/NkcNpq7jgX0z3UN/AKG7w==
=Q4Fu
-----END PGP SIGNATURE-----

--U2vy66mGJShhsnPN--