From mboxrd@z Thu Jan 1 00:00:00 1970 From: Guenter Roeck Subject: Re: [PATCH v2] i2c: stub: Add support for SMBus block commands Date: Tue, 08 Jul 2014 13:05:41 -0700 Message-ID: <53BC4F15.9030608@roeck-us.net> References: <1404742983-27303-1-git-send-email-linux@roeck-us.net> <20140708215453.0677d3ed@endymion.delvare> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140708215453.0677d3ed-R0o5gVi9kd7kN2dkZ6Wm7A@public.gmane.org> Sender: linux-i2c-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Jean Delvare Cc: Wolfram Sang , Randy Dunlap , linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-i2c@vger.kernel.org Hi Jean, On 07/08/2014 12:54 PM, Jean Delvare wrote: > Hi Guenter, > > On Mon, 7 Jul 2014 07:23:03 -0700, Guenter Roeck wrote: >> SMBus block commands are different to I2C block commands since >> the returned data is not normally accessible with byte or word >> commands on other command offsets. Add linked list of 'block' >> commands to support those commands. >> >> Access mechanism is quite simple: Block commands must be written >> before they can be read. The first write selects the block length. >> Subsequent writes can be partial. Block read commands always return >> the number of bytes selected with the first write. >> >> Signed-off-by: Guenter Roeck >> --- >> v2: Make new functionality only available on request via functionality >> module parameter >> Add more details about SMBus block mode support to documentation >> Use correct sizeof() variable in devm_kzalloc >> Use stub_find_block() only in SMBus block command itself. >> Store first word of block data in chip->words[]. >> When writing block data and the written data is longer than >> the first write, bail out with debug message indicating the reason >> for the error. > > Looks good, thanks for the quick update. > > Reviewed-by: Jean Delvare > > Just one thing I have been thinking about while reviewing the updated > code... You decided to make the first SMBus block write select the > maximum block length, and you always use that for SMBus block reads. > However you accept partial writes. The fact that the order in which > writes are performed has an effect on which writes are accepted is > somewhat unexpected. > > Wouldn't it make more sense to accept all SMBus block writes, > regardless of the size (as long as it is within the limits of the SMBus > standard, of course)? Then the only thing left to decide is whether > SMBus block reads use the maximum size or the size of the most recent > SMBus block write. > > I suspect this would mimic the behavior of real chips better. What do > you think? > Not really sure what the expected behavior is. My original code accepted all writes and returned the most recent write, including the most recent write length. I thought this was untypical, and that it would be more typical for the chip to return a fixed length. But ultimately I don't really know, and I am fine either way. Guenter