Re: [PATCH i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver"

From: Jean Delvare <jdelvare@suse.de>
To: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Peter Korsgaard <peter@korsgaard.com>,
	linux-i2c@vger.kernel.org, linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH i2c-tools] Revert "tools: i2ctransfer: add check for returned length from driver"
Date: Fri, 4 Jun 2021 15:57:08 +0200	[thread overview]
Message-ID: <20210604155708.14159db0@endymion> (raw)
In-Reply-To: <YLew8cFWTRQKrBuk@kunai>

On Wed, 2 Jun 2021 18:25:21 +0200, Wolfram Sang wrote:
> Well, I thought mainly about this patch "tools: i2cbusses: Handle bus
> names like /dev/i2c-0" but you took care of it faster than I did.
> 
> There is only one patch left in patchwork from 2016:
> 
> http://patchwork.ozlabs.org/project/linux-i2c/patch/044b3af6a47dfa92e047f0ff57e39a5b61e00058.1463165295.git.leonard.crestez@intel.com/
> "[i2c-tools] i2cget: Add support for i2c block data"
> 
> That's all what's left in patchwork.
> 
> Dunno if you care about the old patch, but I think we are good to go for
> a release.

Yes I do care. Apparently I wasn't Cc'd on that thread so I wasn't even
aware that patch existed. It would be very nice if you could bump the
thread to me (unless there's a way to retrieve it from patchwork that I
didn't find?)

I read the thread, my initial answer would have matched Guenter's (as
always...) but the contributor has a point, the range option is not
supported by i2cdump in I2C block mode. Whether it's better to add
support for it there, or in i2cget, I'll see.

I think i2cget has my preference, because i2cdump pretty much assumes
that I2C block reads retrieve successive 8-bit register values, so you
can have block boundaries anywhere without changing the result. As a
matter of fact, while i2cdump attempts to make 32-byte I2C block reads,
it will transparently handle short reads by issuing additional block
reads at arbitrary offsets. This works nicely for EEPROMs but could
fail for other devices.

If we only want *one* I2C block read at a specified offset then i2cget
seems more appropriate indeed.

Looking at the code, I see that i2cdump already supports SMBus block
mode, in a way which is very similar to what the contributor asked for,
i.e. it doesn't really dump the chip's registers, but merely reads one
SMBus block at a specific offset. It's questionable why this should
belong to i2cdump in the first place, beyond the fact that it returns
several values when i2cget typically returns only one value.

So one option would be get rid of "s" mode in i2cdump, and add support
for both I2C and SMBus block read to i2cget.

Might be possible to add support for range limitation to I2C block
reads in i2cdump nevertheless, as it could be useful to read only
portions of EEPROMs.

-- 
Jean Delvare
SUSE L3 Support