linux-kernel - Re: [PATCH v4 2/2] power: supply: add support for MAX1720x standalone fuel gauge

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 19 Jun 2024 17:30:27 +0200
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Dimitri Fedrau <dima.fedrau@...il.com>
Cc: Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-pm@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Thomas Weißschuh <thomas@...ch.de>
Subject: Re: [PATCH v4 2/2] power: supply: add support for MAX1720x
 standalone fuel gauge

Hi,

On Wed, Jun 19, 2024 at 08:14:07AM GMT, Dimitri Fedrau wrote:
> Am Wed, Jun 19, 2024 at 01:19:32AM +0200 schrieb Sebastian Reichel:
> > On Mon, Jun 17, 2024 at 08:45:04PM GMT, Dimitri Fedrau wrote:
> > > The MAX17201 monitors a single cell pack. The MAX17205 monitors and
> > > balances a 2S or 3S pack or monitors a multiple-series cell pack. Both
> > > devices use a I2C interface.
> > > 
> > > Signed-off-by: Dimitri Fedrau <dima.fedrau@...il.com>
> > > ---
> > 
> > [...]
> > 
> > > +static int max1720x_read_word_data_nvmem(struct i2c_client *ancillary, u8 addr)
> > > +{
> > > +	u8 rx[2];
> > > +	struct i2c_msg msgs[] = {
> > > +		{
> > > +			.addr = ancillary->addr,
> > > +			.flags = 0,
> > > +			.len = 1,
> > > +			.buf = &addr,
> > > +		},
> > > +		{
> > > +			.addr = ancillary->addr,
> > > +			.flags = I2C_M_RD,
> > > +			.len = 2,
> > > +			.buf = &rx[0],
> > > +		},
> > > +	};
> > > +	int ret;
> > > +
> > > +	ret = i2c_transfer(ancillary->adapter, msgs, ARRAY_SIZE(msgs));
> > > +	if (ret != ARRAY_SIZE(msgs))
> > > +		return ret < 0 ? ret : -EIO;
> > > +
> > > +	return get_unaligned_le16(&rx[0]);
> > > +}
> > 
> > Have you tried using i2c_smbus_read_word_data(ancillary, addr)
> > instead of the above?
> >
> Yes, I did and it worked. Used it in V3 and changed it due to review
> comments by Thomas which make sense to me. I don't have any preference on
> this. Should I change it back ?

Using the smbus function makes the driver work with a pure SMBus
adapter and keeps it working with I2C adapters through the SMBus
"emulation". By using the open coded version we have more code and
the driver does not work with pure SMBus adapters.

So I see no reason why the open coded version should be used. Note,
that regmap also uses i2c_smbus_read_word_data() internally for your
configuration of the non-ancillary device.

Greetings,

-- Sebastian

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)