Re: [PATCH] dt-bindings: nvmem: add binding for QTI SPMI SDAM

[Rob Herring <robh@kernel.org>]

On Fri, Jan 10, 2020 at 2:54 AM <sthella@codeaurora.org> wrote:
>
> On 2020-01-09 21:01, Rob Herring wrote:
> > On Thu, Jan 9, 2020 at 4:57 AM <sthella@codeaurora.org> wrote:
> >>
> >> On 2020-01-08 22:09, Rob Herring wrote:
> >> > On Tue, Dec 24, 2019 at 11:02:12AM +0530, Shyam Kumar Thella wrote:
> >> >> QTI SDAM allows PMIC peripherals to access the shared memory that is
> >> >> available on QTI PMICs. Add documentation for it.
> >> >>
> >> >> Signed-off-by: Shyam Kumar Thella <sthella@codeaurora.org>
> >> >> ---
> >> >>  .../devicetree/bindings/nvmem/qcom,spmi-sdam.yaml  | 79
> >> >> ++++++++++++++++++++++
> >> >>  1 file changed, 79 insertions(+)
> >> >>  create mode 100644
> >> >> Documentation/devicetree/bindings/nvmem/qcom,spmi-sdam.yaml
> >> >>
> >> >> diff --git
> >> >> a/Documentation/devicetree/bindings/nvmem/qcom,spmi-sdam.yaml
> >> >> b/Documentation/devicetree/bindings/nvmem/qcom,spmi-sdam.yaml
> >> >> new file mode 100644
> >> >> index 0000000..8961a99
> >> >> --- /dev/null
> >> >> +++ b/Documentation/devicetree/bindings/nvmem/qcom,spmi-sdam.yaml
> >> >> @@ -0,0 +1,79 @@
> >> >> +# SPDX-License-Identifier: GPL-2.0
> >> >
> >> > Dual license new bindings:
> >> >
> >> > (GPL-2.0-only OR BSD-2-Clause)
> >> >
> >> > Please spread the word in QCom.
> >> Sure. I will add Dual license in next patchset.
> >> >
> >> >> +%YAML 1.2
> >> >> +---
> >> >> +$id: http://devicetree.org/schemas/nvmem/qcom,spmi-sdam.yaml#
> >> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> >> +
> >> >> +title: Qualcomm Technologies, Inc. SPMI SDAM DT bindings
> >> >> +
> >> >> +maintainers:
> >> >> +  - Shyam Kumar Thella <sthella@codeaurora.org>
> >> >> +
> >> >> +description: |
> >> >> +  The SDAM provides scratch register space for the PMIC clients. This
> >> >> +  memory can be used by software to store information or communicate
> >> >> +  to/from the PBUS.
> >> >> +
> >> >> +allOf:
> >> >> +  - $ref: "nvmem.yaml#"
> >> >> +
> >> >> +properties:
> >> >> +  compatible:
> >> >> +    enum:
> >> >> +      - qcom,spmi-sdam
> >> >> +
> >> >> +  reg:
> >> >> +    maxItems: 1
> >> >> +
> >> >> +  "#address-cells":
> >> >> +    const: 1
> >> >> +
> >> >> +  "#size-cells":
> >> >> +    const: 1
> >> >
> >> > ranges? The child addresses should be translateable I assume.
> >> The addresses are not memory mapped on the CPU's address domain. They
> >> are the SPMI addresses which can be accessed over SPMI controller.
> >
> > Doesn't have to be a CPU address. Are the child offsets within the
> > range defined in the parent 'reg'? If so, then it should have
> > 'ranges'.
> Yes the child offsets fall within parent reg's address space.
> I will add ranges in the next patch set.
> >
> >> >
> >> >> +
> >> >> +required:
> >> >> +  - compatible
> >> >> +  - reg
> >> >> +
> >> >> +patternProperties:
> >> >> +  "^.*@[0-9a-f]+$":
> >> >> +    type: object
> >> >> +
> >> >> +    properties:
> >> >> +      reg:
> >> >> +        maxItems: 1
> >> >> +        description:
> >> >> +          Offset and size in bytes within the storage device.
> >> >> +
> >> >> +      bits:
> >> >
> >> > Needs a type reference.
> >> Yes. I will add a reference in the next patch set.
> >> >
> >> >> +        maxItems: 1
> >> >> +        items:
> >> >> +          items:
> >> >> +            - minimum: 0
> >> >> +              maximum: 7
> >> >> +              description:
> >> >> +                Offset in bit within the address range specified by
> >> >> reg.
> >> >> +            - minimum: 1
> >> >
> >> > max is 7?
> >> I don't think it is limited to 7 as it is the size within the address
> >> range specified by reg. If the address range is more than a byte size
> >> can be more.
> >
> > Then why is the maximum offset 7?
> I see. Offset can be more than 7 within the address range specified in
> case
> of data cells with more than a byte. I will remove maximum in the next
> patch set.

That's the wrong thing to do though. If the offset is more than 7, you
should just increase 'reg' value. IOW, 'bits' should only be used to
express bit position up to the minimum alignment of 'reg'. I guess you
could have an unaligned multi-byte field, so I guess this is fine
as-is.

Rob