Re: [PATCH v2 2/6] dt-bindings: soc: actions: Add Actions Semi Owl socinfo binding

[Cristian Ciocaltea <cristian.ciocaltea@gmail.com>]

On Fri, Apr 02, 2021 at 11:34:07PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Mar 30, 2021 at 04:48:17PM +0300, Cristian Ciocaltea wrote:
> > Add devicetree binding for the Actions Semi Owl socinfo driver.
> > 
> 
> Devicetree binding shouldn't be added for a driver instead for an IP or hw.
> 
> > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> > ---
> >  .../bindings/soc/actions/owl-socinfo.yaml     | 57 +++++++++++++++++++
> >  1 file changed, 57 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/soc/actions/owl-socinfo.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/soc/actions/owl-socinfo.yaml b/Documentation/devicetree/bindings/soc/actions/owl-socinfo.yaml
> > new file mode 100644
> > index 000000000000..01e4a8b4f5ac
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/actions/owl-socinfo.yaml
> > @@ -0,0 +1,57 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/soc/actions/owl-socinfo.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Actions Semi Owl SoC info module
> > +
> > +maintainers:
> > +  - Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> > +
> > +description: |
> > +  Actions Semi Owl SoC info module provides access to various information
> > +  about the S500, S700 and S900 SoC variants, such as serial number or id.
> > +
> 
> S700/S900 are not yet confirmed, so please avoid them.
> 
> > +select:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        enum:
> > +          - actions,s500-soc
> > +          - actions,s700-soc
> > +          - actions,s900-soc
> > +  required:
> > +    - compatible
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - actions,s500-soc
> > +          - actions,s700-soc
> > +          - actions,s900-soc
> > +      - const: simple-bus
> > +
> > +required:
> > +  - compatible
> > +
> > +additionalProperties: true
> > +
> > +examples:
> > +  - |
> > +    / {
> > +        compatible = "roseapplepi,roseapplepi", "actions,s500";
> > +        model = "Roseapple Pi";
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> > +
> > +        soc {
> > +            compatible = "actions,s500-soc", "simple-bus";
> 
> No. This shouldn't fall under /soc. I think you should added a separate
> compatible for the reserved memory itself and add a corresponding socinfo
> driver under drivers/soc.
> 
> This way it is obvious that the information is contained in a memory region and
> a driver is used for parsing that.

I avoided on purpose to bind the driver on the reserved memory node
in order to be able to handle also the S700 and S900 SoCs, for which
we currently provide (only) the following information: machine, family,
soc_id.

I assumed the serial_number is not mandatory and we can use the driver
as it is for all SoC variants. I don't know how the serial number for
S700 and S900 could be read, but I think it is very likely they require
a different access method.

Hence my intention was to keep the driver generic, not coupled strictly
with the serial number information.

Regards,
Cristi

> Thanks,
> Mani
> 
> > +            #address-cells = <1>;
> > +            #size-cells = <1>;
> > +            ranges;
> > +        };
> > +    };
> > +
> > +...
> > -- 
> > 2.31.1
> >