On 13/06/2020 21:33, Doug Anderson wrote:
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.
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
