Re: [PATCH v2 1/2] hwmon: (pmbus/isl68137) add debugfs config and black box endpoints

[Grant Peltier]

On Sat, Apr 25, 2020 at 10:43:18AM -0700, Guenter Roeck wrote:
> On Wed, Apr 22, 2020 at 12:27:14PM -0500, Grant Peltier wrote:
> > Add debugfs endpoints to support features of 2nd generation Renesas
> > digital multiphase voltage regulators that are not compatible with
> > sysfs.
> > 
> > The read_black_box endpoint allows users to read the contents of a
> > RAM segment used to record fault conditions within Gen 2 devices.
> > 
> > The write_config endpoint allows users to write configuration hex
> > files to Gen 2 devices which modify how they operate.
> > 
> > Signed-off-by: Grant Peltier <grantpeltier93@xxxxxxxxx>
> 
> Comments inline.
> 
> However, the more I look into this, the more concerns I have.
> I think we should limit debugfs functions, if they are needed,
> to reporting detailed device status. Can you consider handling
> configuration from userspace using i2cget / i2cset commands ?

The reason we decided to try to implement configuration writes within the
driver is that we found a userspace implementation to be unstable. The
process requires anywhere from approximately 650 to a few thousand 32-bit
writes (dependent on number of NVM slots contained in the file). The entire
write process therefore takes a non-trivial amount of CPU time to complete
and the userspace process was often interrupted which would cause for it
to fail. Writing the configuration directly from the driver has been less
error prone.

> > +	res = i2c_smbus_read_i2c_block_data(ctrl->client, PMBUS_IC_DEVICE_REV,
> > +					    5, dev_rev);
> 
> It still puzzles me, quite frankly, why i2c_smbus_read_block_data()
> would not work here.
>

i2c_smbus_read_block_data() requires the underlying driver/controller to handle
interpretting the initial length byte read from the client device and then
continuing to read that number of bytes. Not all controllers (e.g. BCM2835)
support this. On the other hand, i2c_smbus_read_i2c_block_data() just does a
fixed-length read based on the given length parameter.

> > +static int raa_dmpvr2_cfg_write_result(struct raa_dmpvr2_ctrl *ctrl,
> > +				       struct raa_dmpvr2_cfg *cfg)
> > +{
> > +	u8 data[4] = {0};
> > +	u8 data1[4];
> > +	u8 *dptr;
> > +	unsigned long start;
> > +	int i, j, status;
> > +
> > +	// Check programmer status
> > +	start = jiffies;
> > +	i2c_smbus_write_word_data(ctrl->client, RAA_DMPVR2_DMA_ADDR,
> > +				  RAA_DMPVR2_PRGM_STATUS_ADDR);
> > +	while (data[0] == 0 && !time_after(jiffies, start + HZ + HZ)) {
> > +		raa_dmpvr2_smbus_read32(ctrl->client, RAA_DMPVR2_DMA_FIX,
> > +					data);
> > +	}
> > +	if (data[0] != 1)
> > +		return -ETIME;
> 
> Are you sure ? Normally I would expect ETIMEDOUT.
 
My understanding is that ETIME is meant for timer expiration whereas ETIMEDOUT
is meant for connection timeout errors. Is that correct? In this case, we are
not really waiting on the device to respond but instead are constantly polling
until the device responds with the desired value. However, I can understand an
argument for ETIMEDOUT here and can swtich to that if you think it is more
appropriate.


Thank you for your other notes. I will refactor as requested.

Grant