Hi, On Thu, Jun 11, 2020 at 2:49 AM Ravi Kumar Bokka <rbokka@xxxxxxxxxxxxxx> wrote: > > This patch adds support for QTI qfprom-efuse controller. This driver can > access the raw qfprom regions for fuse blowing. > > The current existed qfprom driver is only supports for cpufreq, thermal sensors > drivers by read out calibration data, speed bins..etc which is stored > by qfprom efuses. > > Signed-off-by: Ravi Kumar Bokka <rbokka@xxxxxxxxxxxxxx> > --- It makes all your code reviewers much happier (and much more likely to take the time to review your patches) if you include a changelog with what changed from one version to the next. If you would like some help maintaining such a thing, might I suggest "patman": https://gitlab.denx.de/u-boot/u-boot/-/blob/master/tools/patman/README ...pay no mind that it's hosted in the U-Boot repo--it's really quite a great tool. > drivers/nvmem/Kconfig | 1 + > drivers/nvmem/qfprom.c | 405 ++++++++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 385 insertions(+), 21 deletions(-) > > diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig > index d7b7f6d..623d59e 100644 > --- a/drivers/nvmem/Kconfig > +++ b/drivers/nvmem/Kconfig > @@ -117,6 +117,7 @@ config QCOM_QFPROM > help > Say y here to enable QFPROM support. The QFPROM provides access > functions for QFPROM data to rest of the drivers via nvmem interface. > + And this driver provides access QTI qfprom efuse via nvmem interface. I'm not sure it was necessary to add that line, but I won't object if you/others really like it. > This driver can also be built as a module. If so, the module > will be called nvmem_qfprom. > diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c > index 8a91717..312318c 100644 > --- a/drivers/nvmem/qfprom.c > +++ b/drivers/nvmem/qfprom.c You've still mostly not addressed most of the review feedback I've now given you 3 times. Rather than repeating comments, I have simply provided a patch that makes the driver into a state that I'm happy with: https://crrev.com/c/2244932 Rough summary: * There should be no reason to provide "reset" values for things. For anything that you change for fuse blowing, just save and restore after. * Use the major/minor version read from 0x6000 to pick the parameters to use. Please double-check that I got this right. * Reading should still read "corrected", not "raw". Added a sysfs knob to allow you to read "raw", though. * Simplified the SoC data structure. * No need for quite so many levels of abstraction for setting clocks / regulator. * Don't set regulator voltage. Rely on device tree to make sure it's right. * Properly undo things in the case of failure. * Don't just keep enabling the regulator over and over again. * Enable / disable the clock each time; now we don't need a .remove function and yet we still don't leave the clock enabled/prepared. * Polling every 100 us but timing out in 10 us didn't make sense. Swap those. Also no reason for 100 us to be SoC specific. * No need for reg-names. * We shouldn't be creating two separate nvmem devices. In general I'm happy to post my series to the list myself to get review feedback. For now I'm expecting that you can squash my changes in and send the next version. -Doug
