Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 20 Oct 2019 14:43:35 +0200
From:   Sebastian Reichel <sre@kernel.org>
To:     Dan Murphy <dmurphy@ti.com>
Cc:     linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/3] power: supply: bq2515x: Introduce the bq2515x family
Message-ID: <20191020124335.gssoiyjmdtpygwqy@earth.universe>
References: <20190930143137.21624-1-dmurphy@ti.com>
 <20190930143137.21624-3-dmurphy@ti.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="pug62zvjsqlqwju4"
Content-Disposition: inline
In-Reply-To: <20190930143137.21624-3-dmurphy@ti.com>
User-Agent: NeoMutt/20180716
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--pug62zvjsqlqwju4
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hi,

Here are some more notes.

On Mon, Sep 30, 2019 at 09:31:37AM -0500, Dan Murphy wrote:
> Introduce the bq25150 and bq25155 supply chargers.
>=20
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---

[...]

> +/* initial field values, converted to register values */
> +struct bq2515x_init_data {
> +	u8 ichg;	/* charge current		*/
> +	u8 vreg;	/* regulation voltage		*/

The following 4 entries seem to be unused.

> +	u8 iterm;	/* termination current		*/
> +	u8 iprechg;	/* precharge current		*/
> +	u8 sysvmin;	/* minimum system voltage limit */
> +	u8 ilim;	/* ILIM current contol		*/
> +};

[...]

> +static int get_const_charge_current(struct bq2515x_device *bq2515x)
> +{
> +	int ret;
> +	int intval;
> +	int iin_msb;
> +	int iin_lsb;
> +	u16 ichg_measurement;
> +	int ilim_val, ichg_multiplier;
> +
> +	if (!bq2515x_is_ps_online(bq2515x))
> +		return -ENODATA;
> +
> +	ret =3D regmap_read(bq2515x->regmap, BQ2515X_ADC_IIN_M, &iin_msb);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret =3D regmap_read(bq2515x->regmap, BQ2515X_ADC_IIN_L, &iin_lsb);
> +	if (ret < 0)
> +		return ret;
> +
> +	ichg_measurement =3D (iin_msb << 8) | iin_lsb;
> +	ret =3D regmap_read(bq2515x->regmap, BQ2515X_ILIMCTRL, &ilim_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ilim_val >=3D BQ2515X_ILIM_150MA)
> +		ichg_multiplier =3D 350;
> +	else
> +		ichg_multiplier =3D 750;
> +
> +	intval =3D (ichg_measurement * 100 / BQ2515X_DIVISOR) * ichg_multiplier;
> +	return intval / 100;
> +}
> [...]

I just checked the calculation and you return mA. This is incorrect,
since the power-supply ABI uses =B5A. Please also re-check the other
calculations. You can find some information about the power-supply
subsystem here:

Documentation/power/power_supply_class.rst

-- Sebastian

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

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

iQIzBAEBCgAdFiEE72YNB0Y/i3JqeVQT2O7X88g7+poFAl2sVnQACgkQ2O7X88g7
+poKkQ//Ul4FyYqJe7GL08pax4RATZxfc9jZlRJqwfOfzHzwZlDSG9VAIP/IqySU
DMZ3uD1qbjNpJYzGzvDJ4Wtr6n0nwoaiGRMnzgEzWNXRjF+mZMTZX/PyZZ2Kvnww
G4xdqz5QgcdVnvdy52qYTlM0f7YKKFdvHKSmEtQCsGTEU0/A9K3bibpAttyCYQGU
Z/LbuuWJfIYCPBi3oYktvO0mtRmD8rK991EeihVuThJ8urtg60/UjQPfgVtBJ9Nz
LNf8dNe/cOS5pKyzrKCr7/5yalLlt5ekAX1INwS3roML6agn+EunkZ/gtE2gxotx
6xG132LE/0o5S/3P10ghlAdb0gVNxSMvA48FA96A4slKgO5W8potHCiR2n56Sedk
MiM6jTyXSjK1c3c7SxJM1B7lsFA+Sr2TDGBdoT6QIzisTo1LlpIbYa6dDavEPLbZ
HrST846Pw7LBPfQzbUI9g+OGJx1n6GHsQLN/MSs/MsVxufl5S5xAbLehwMdae+Ir
3Z2M3uSgtC8v5vpBccHCutfgnd/pYhPeX1hvOQIv2CHQfPBTk6jwb3BMR/bO4GU/
7+x1a/Bsak2jOis2hXQi0QoWP4jjBh1venLqt2QtWxKGCR1zejd3/9wU52rS9x7/
KO7JXiraWHwxCPGdSLL7tJkppc4c8zN4CoYNgR3eFpnrwXKQjsE=
=8M8k
-----END PGP SIGNATURE-----

--pug62zvjsqlqwju4--