Re: [PATCH V2 1/2] dt-bindings: syscon: Add syscon-names to refer to syscon easily

[Rob Herring]

On Wed, Dec 04, 2019 at 06:00:17PM +0100, Arnd Bergmann wrote:
> On Wed, Dec 4, 2019 at 5:38 PM Rob Herring <robh@xxxxxxxxxx> wrote:
> > On Wed, Nov 20, 2019 at 11:41:47PM +0800, Orson Zhai wrote:
> > > Make life easier when syscon consumer want to access multiple syscon
> > > nodes with dozens of items.
> > > Add syscon-names and relative properties to help to manage different
> > > cases when accessing more than one syscon node even with arguments.
> > >
> > > Signed-off-by: Orson Zhai <orson.zhai@xxxxxxxxxx>
> > > ---
> > >  .../devicetree/bindings/mfd/syscon.txt        | 43 +++++++++++++++++++
> > >  1 file changed, 43 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
> > > index 25d9e9c2fd53..4c7bdb74bb0a 100644
> > > --- a/Documentation/devicetree/bindings/mfd/syscon.txt
> > > +++ b/Documentation/devicetree/bindings/mfd/syscon.txt
> > > @@ -30,3 +30,46 @@ hwlock1: hwspinlock@40500000 {
> > >         reg = <0x40500000 0x1000>;
> > >         #hwlock-cells = <1>;
> > >  };
> > > +
> > > +
> > > +
> > > +==Syscon Name==
> > > +
> > > +Syscon name is a helper to be used in consumer nodes who refer to system
> > > +controller node. It provides a way to refer to syscon node by names with
> > > +phandle args in syscon consumer node. It will help people who have a lot
> > > +of syscon references to be managed. It is not a must feature and has no
> > > +effect to syscon node itself at all.
> > > +
> > > +Required properties:
> > > +- syscons: List of phandles and any number of arguments if needed. Arguments
> > > +  is specific to differnet vendors and its usage should be described at each
> > > +  vendor's bindings. For example: In Unisoc SoCs, the 1st arg will be treated
> > > +  as register address offset and the 2nd is bit mask.
> > > +
> > > +- syscon-names:        List of syscon node name strings sorted in the same order as
> > > +  what it represents in property syscons.
> > > +
> > > +Optional property:
> > > +- #.*-cells: Represents the number of arguments in single phandle in syscons
> > > +  list. ".*" is vendor specific. If this property is not set, default value
> > > +  will be 0.
> >
> > This breaks the normal pattern of how '#.*-cells' works. While Arnd
> > suggests removing it, I don't think that works well either with having a
> > generic 'syscons' property. That means every syscon in a system has to
> > have the same number of cells.
> >
> > I don't really want to see syscon binding expanded. Really, I'd like
> > 'syscon' to go away. It's nothing more than a flag to create a regmap.
> 
> Not expanding the syscon binding is the point about not having a #*-cells:
> In the examples that Orson listed, the cell count would always be
> specific to the user of the syscon regmap, and not interpreted by the
> syscon itself.
> 
> > I think it is better to keep the property names specific to exactly what
> > the functionality is for each syscon phandle rather than a generic
> > binding.
> >
> > What are the eamples of where you want to use this?
> 
> I think generally speaking this would be useful for random registers that
> logically belong to one device but are grouped with other unrelated
> registers in a syscon, and that are in a different register offset for
> each chip that has them. Using named properties instead of a list
> of phandle/arg tuples with names is clearly a simpler alternative
> and more like we do it today, but I can also see some API simplification
> with Orson's patch without the binding getting out of hand.

I understand when a phandle to a syscon is used. That's nothing new. 
What's special about Unisoc SoC that needs something new/different? 
I saw there's a large number of syscons, but I don't understand what's 
in them. 

If the API is this:

struct regmap *syscon_regmap_lookup_by_name(struct device_node *np,                                                    
                                       const char *name,
                                       int arg_count, __u32 *out_args)

How is 'name' being an entry in syscon-names simpler than just being the 
property name? The implementation for the latter would certainly be 
simpler.

It also makes the property unparseable without knowledge outside of the 
DT (i.e. in the driver). I suppose if the number of cells for each entry 
is fixed, we could count the number of syscon-names entries to figure 
out the stride. But then if one entry needs a lot of cells, then they 
all have to have padding cells.

Rob