Re: [PATCH 1/2] dt-bindings: Add syscon-names support

From: Arnd Bergmann
Date: Tue Nov 19 2019 - 09:20:34 EST

On Mon, Nov 18, 2019 at 9:42 AM Orson Zhai <orson.zhai@xxxxxxxxxxxxxx> wrote:
> Hi Arnd,
> On Fri, Nov 15, 2019 at 10:33:30AM +0100, Arnd Bergmann wrote:
> > On Thu, Nov 14, 2019 at 12:48 PM Orson Zhai <orson.zhai@xxxxxxxxxx> wrote:
> > >
> > >
> > > Make life easier when syscon consumer want to access multiple syscon
> > > nodes.
> > > Add syscon-names and relative properties to help manage complicated
> > > cases when accessing more one syscon node.
> > >
> > > Signed-off-by: Orson Zhai <orson.zhai@xxxxxxxxxx>
> >
> > Hi Orson,
> >
> > Can you explain why the number of cells in this binding is specific
> > to the syscon node rather than the node referencing it?
> The story is like this. I found there are too many global registers in
> Unisoc(former Spreadtrum) chips. Dozens of offset with dozens of modules
> were needed to be specified. So I thought the dts files would seem "horrible"
> with a big chunk of syscon-xxx (say more than 20 lines)
> I learned from reg-names way which might look clean to hold all these mess things.
> But to implement this, the users need to konw the cell-size if we add arguments to syscon node.
> I thought to add cell-size into every syscon consumer node is a duplicated work and
> I wanted to take advantage of of_parse_phandle_with_args.
> So the bindings were created then.

Ok, that makes sense.

> > The way would otherwise handle the example from your binding
> > would be with two separate properties in the display node, like
> >
> > syscon-enable = <&ap_apb_regs 0x4 0xf00>;
> > syscon-power = <&aon_regs 0x8>;
> This is an option for consumers all the time.
> Acturally my patches are not going to replace this.
> I'd like to provide another option to save people like desperate engineers in Spreadtrum :)
> >
> > in which case, the syscon driver does not need to know anything
> Whould it be better if I add syscon-cells into consumer's node?

As I see it, there is no reason to put the syscon-cells property into any node,
as this is implied by the driver binding using the syscon reference. I would
only use the #xyz-cells style property if there are multiple interpretations
that all make sense for the same binding.

> Then I could read the cell size and use "of_parse_phandle_with_fixed_args()" instead.
> This will not involve syscon node itself at all.

This sounds better to me, yes. I had not even remembered this function
exists, but I think this is a good idea.

I can also see a point in favor of adding more infrastructure around this,
possibly naming the entries in a syscon-names property as you suggested,
combining of_parse_phandle_with_fixed_args() with a name, or
combining with syscon_regmap_lookup_by_phandle() for convenience.

This should all be possible without adding complexity to the syscon
DT binding itself, and it would give more structure to the way it
is used by drivers.