Re: [PATCH 3/4] dt-bindings: power: reset: Add regmap support to the SYSCON reboot-mode bindings

[Sergey Semin]

On Wed, Mar 18, 2020 at 05:14:25PM -0600, Rob Herring wrote:
> On Fri, Mar 13, 2020 at 7:03 AM Sergey Semin
> <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote:
> > > On Fri, Mar 06, 2020 at 04:03:40PM +0300, Sergey.Semin@xxxxxxxxxxxxxxxxxxxx wrote:
> > > > From: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> > > >
> > > > Optional regmap property will be used to refer to a syscon-controller
> > > > having a reboot tolerant register mapped.
> > >
> > > NAK. It should simply be a child node of the 'syscon-controller'.
> >
> > Hm, It's dilemma. The driver maintainer said ack, while you disagree.)
> > So the code change will be merged while the doc-part won't? Lets discuss then
> > to settle the issue.
> >
> > Why 'syscon-reboot' can be out of syscon-controller node, while
> > 'syscon-reboot-mode' can't?
> 
> Look at the history and you will see one was reviewed by DT
> maintainers and one wasn't.
> 
> > They both belong to the same usecase: save
> > cause id and reboot. So having similar properties-set and declaring their
> > nodes someplace nearby is natural.
> 
> Which is what I'm asking for. Where else in the tree does it make
> sense to locate the 'syscon-reboot-mode' node? Locate nodes where they
> logically belong.
> 
> > According to the driver 'syscon-reboot'
> > can't lack the regmap property because it's mandatory, while here you refuse
> > to have even optional support. Additionally in most of the cases the
> > 'syscon-reboot' nodes aren't declared as a child of a system controller
> > node. Why 'syscon-reboot-mode' can't work in a similar way?
> 
> There's plenty of bad or "don't follow current best practice" examples
> in the tree for all sorts of things. That is not a reason for doing
> something in a new binding or adding to an existing one.
> 
> Rob

Alright. I see your point. What about I'd provide a sort of opposite
implementation? I could make the "regmap"-phandle reference being optional
in the !"syscon-reboot"! driver instead of adding the regmap-property
support to the "syscon-reboot-mode" driver. So if regmap property isn't
defined in the "syscon-reboot"-compatible node, the driver will try to
get a syscon regmap from the parental node as it's done in the
"syscon-reboot-mode" driver.

Seeing you think that regmap-property-based design is a bad practice in
this case, I also could mark the property as deprecated in the "syscon-reboot"
dt schema and print a warning from the "syscon-reboot" driver if one is defined.

What do you think?

Regards,
-Sergey