Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 22 Apr 2020 10:22:28 -0500
From:   Grant Peltier <grantpeltier93@gmail.com>
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org,
        adam.vaughn.xh@renesas.com, grant.peltier.jg@renesas.com
Subject: Re: [PATCH 1/2] hwmon: (pmbus/isl68137) add debugfs config and black
 box endpoints
Message-ID: <20200422152228.GA32630@raspberrypi>
References: <cover.1587408025.git.grantpeltier93@gmail.com>
 <203a7cd37dd95a0127cc06de14986721ab621e93.1587408025.git.grantpeltier93@gmail.com>
 <20200421185851.GA95204@roeck-us.net>
 <20200421214917.GA28170@raspberrypi>
 <20200422020404.GA126375@roeck-us.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200422020404.GA126375@roeck-us.net>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Apr 21, 2020 at 07:04:04PM -0700, Guenter Roeck wrote:
> On Tue, Apr 21, 2020 at 04:49:17PM -0500, Grant Peltier wrote:
> > On Tue, Apr 21, 2020 at 11:58:51AM -0700, Guenter Roeck wrote:
> > > Normally this is emulated for such controllers. I don't recall seeing
> > > such a need before. The code below duplicates similar code in
> > > i2c_smbus_xfer_emulated(), which is much more sophisticated.
> > > Are you sure this is needed ? Can you point me to an affected
> > > controller ?
> > > 
> > > > +static s32 raa_smbus_read40(const struct i2c_client *client, u8 command,
> > > > +			    unsigned char *data)
> > > > +{
> > > > +	int status;
> > > > +	unsigned char msgbuf[1];
> > > > +	struct i2c_msg msg[2] = {
> > > > +		{
> > > > +			.addr = client->addr,
> > > > +			.flags = client->flags,
> > > > +			.len = 1,
> > > > +			.buf = msgbuf,
> > > > +		},
> > > > +		{
> > > > +			.addr = client->addr,
> > > > +			.flags = client->flags | I2C_M_RD,
> > > > +			.len = 5,
> > > > +			.buf = data,
> > > > +		},
> > > > +	};
> > > > +
> > > > +	msgbuf[0] = command;
> > > > +	status = i2c_transfer(client->adapter, msg, 2);
> > > > +	if (status != 2)
> > > > +		return status;
> > > 
> > > i2c_transfer() can return 1 if only one of the two messages was sent.
> > > 
> > > > +	return 0;
> > > > +}
> > I have been using BCM2835 for most of my testing. I originally tried using
> > i2c_smbus_read_block_data() but that was returning errors. However, from your
> > email, I went back and tried i2c_smbus_read_i2c_block_data() and that appears
> > to be working so I will switch to that instead.
> > 
 
> This is odd, since the function should do a SMBus block read. Did you pass a
> buffer that was too small, by any chance ?

I tried with a variety of buffer sizes from 4 (data size) to 32 (max block size)
and it always returned an error. I believe that i2c_smbus_read_block_data()
attempts to do a legitimate SMBus block read which depends on the controller
interpretting and reading the correct number of bytes as signaled from the
client. This is in line with the docstring for the function and the fact that
the BCM2835 responds with false (0) from i2c_check_functionality() for
I2C_FUNC_SMBUS_READ_BLOCK_DATA. On the other hand,
i2c_smbus_read_i2c_block_data() appears to do a fixed length read similar to
the function that I wrote above.

Thanks,
Grant