On Mon, Jun 15, 2020 at 4:44 AM Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx> wrote: > > > > On 12/06/2020 22:59, Doug Anderson wrote: > > Hi, > > > > On Thu, Jun 11, 2020 at 2:49 AM Ravi Kumar Bokka <rbokka@xxxxxxxxxxxxxx> wrote: > >> > >> This patch adds dt-bindings document for qfprom-efuse controller. > >> > >> Signed-off-by: Ravi Kumar Bokka <rbokka@xxxxxxxxxxxxxx> > >> --- > >> .../devicetree/bindings/nvmem/qfprom.yaml | 52 ++++++++++++++++++++++ > >> 1 file changed, 52 insertions(+) > > > > Overall comment: I reviewed your v1 series and so I'm obviously > > interested in your series. Please CC me on future versions. > > > > I would also note that, since this is relevant to Qualcomm SoCs that > > you probably should be CCing "linux-arm-msm@xxxxxxxxxxxxxxx" on your > > series. > > > > > >> create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom.yaml > >> > >> diff --git a/Documentation/devicetree/bindings/nvmem/qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qfprom.yaml > >> new file mode 100644 > >> index 0000000..7c8fc31 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/nvmem/qfprom.yaml > >> @@ -0,0 +1,52 @@ > >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >> +%YAML 1.2 > >> +--- > >> +$id: http://devicetree.org/schemas/nvmem/qfprom.yaml# > >> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >> + > >> +title: Qualcomm Technologies Inc, QFPROM Efuse bindings > >> + > >> +maintainers: > >> + - Ravi Kumar Bokka <rbokka@xxxxxxxxxxxxxx> > >> + > >> +allOf: > >> + - $ref: "nvmem.yaml#" > >> + > >> +properties: > >> + compatible: > >> + enum: > >> + - qcom,qfprom > > > > As per discussion in patch #1, I believe SoC compatible should be here > > too in case it is ever needed. This is standard practice for dts > > files for IP blocks embedded in an SoC. AKA, this should be: > > > > items: > > - enum: > > - qcom,apq8064-qfprom > > - qcom,apq8084-qfprom > > - qcom,msm8974-qfprom > > - qcom,msm8916-qfprom > > - qcom,msm8996-qfprom > > - qcom,msm8998-qfprom > > - qcom,qcs404-qfprom > > - qcom,sc7180-qfprom > > - qcom,sdm845-qfprom > > > Above is not required for now in this patchset, as we can attach data at > runtime specific to version of the qfprom. > > This can be added when required! > > > - const: qcom,qfprom > > > > NOTE: old SoCs won't have both of these and thus they will get flagged > > with "dtbs_check", but I believe that's fine (Rob can correct me if > > I'm wrong). The code should still work OK if the SoC isn't there but > > it would be good to fix old dts files to have the SoC specific string > > too. > > > > > >> + > >> + reg: > >> + maxItems: 3 > > > > Please address feedback feedback on v1. If you disagree with my > > feedback it's OK to say so (I make no claims of being always right), > > but silently ignoring my feedback and sending the next version doesn't > > make me feel like it's a good use of my time to keep reviewing your > > series. Specifically I suggested that you actually add descriptions > > rather than just putting "maxItems: 3". > > > > With all that has been discussed, I think the current best thing to > > put there is: > > > > # If the QFPROM is read-only OS image then only the corrected region > > # needs to be provided. If the QFPROM is writable then all 3 regions > > # must be provided. > > oneOf: > > - items: > > - description: The start of the corrected region. > > - items: > > - description: The start of the raw region. > > - description: The start of the config region. > > - description: The start of the corrected region. > > > >> + > > > > You missed a bunch of things that you should document: > > > > # Clocks must be provided if QFPROM is writable from the OS image. > > clocks: > > maxItems: 1 > > clock-names: > > const: sec > > > > # Supply reference must be provided if QFPROM is writable from the OS image. > > vcc-supply: > > description: Our power supply. > > > > # Needed if any child nodes are present. > > "#address-cells": > > const: 1 > > "#size-cells": > > const: 1 > > > >> +required: > >> + - compatible > >> + - reg > >> + - reg-names > > > > reg-names is discouraged. Please remove. I always point people here > > as a reference: > > > > https://lore.kernel.org/r/CAL_Jsq+MMunmVWqeW9v2RyzsMKP+=kMzeTHNMG4JDHM7Fy0HBg@xxxxxxxxxxxxxx/ > > > > You can just figure out whether there are 3 register fields or 1 register field. > > Am not sure if I understand this correctly, reg-names are very useful in > this particular case as we are dealing with multiple memory ranges with > holes. I agree with not having this for cases where we have only one > resource. 1 or 3 doesn't sound like a case with holes. > But having the ordering in DT without proper names associated with it > seems fragile to me! And it makes very difficult to debug issues with > wrong resource ordering in DT. > > Rob, Is this the guidance for new bindings? This has *always* been the guidance since *-names were added. > I have not seen any strong suggestion or guidance either in > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/resource-names.txt?h=v5.8-rc1 The key word is 'supplemental'. Perhaps that could be clearer, but DT always required a defined order and the supplemental information doesn't throw that out. > Or in ./drivers/of/address.c How could it? Order is defined by the specific binding. > > Am I missing anything here?
