Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1591868882-16553-1-git-send-email-rbokka@codeaurora.org> <1591868882-16553-3-git-send-email-rbokka@codeaurora.org>
In-Reply-To: <1591868882-16553-3-git-send-email-rbokka@codeaurora.org>
From:   Doug Anderson <dianders@chromium.org>
Date:   Sat, 13 Jun 2020 13:33:48 -0700
Message-ID: <CAD=FV=XSfMPehyCLB25hct+GE8JtU=5J=04KyU4AoHfg9cs50A@mail.gmail.com>
Subject: Re: [RFC v2 2/3] drivers: nvmem: Add QTI qfprom-efuse support
To:     Ravi Kumar Bokka <rbokka@codeaurora.org>
Cc:     Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        Rajendra Nayak <rnayak@codeaurora.org>,
        Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>,
        dhavalp@codeaurora.org, mturney@codeaurora.org,
        sparate@codeaurora.org, c_rbokka@codeaurora.org,
        mkurumel@codeaurora.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.


>           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