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