Messages in this thread | | | Subject | Re: [RFC v2 2/3] drivers: nvmem: Add QTI qfprom-efuse support | From | Srinivas Kandagatla <> | Date | Mon, 15 Jun 2020 11:44:35 +0100 |
| |
On 13/06/2020 21:33, Doug Anderson wrote: > Hi, > > On Thu, Jun 11, 2020 at 2:49 AM Ravi Kumar Bokka <rbokka@codeaurora.org> 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@codeaurora.org> >> --- > > 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. >
NAK from my side!
> >> 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.
We could create an additional nvmem read-only provider in future if required to read raw!.
> > * 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 >
| |