Re: [PATCH 5/5] staging: mt7621-pci: parse some dt properties from root port child nodes

[Dan Carpenter <dan.carpenter@oracle.com>]

On Mon, Jun 07, 2021 at 01:30:58PM +0200, Sergio Paracuellos wrote:
> Hi Dan,
> 
> On Mon, Jun 7, 2021 at 1:10 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > Hi Dan,
> >
> > On Mon, Jun 7, 2021 at 12:37 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Mon, Jun 07, 2021 at 09:11:13AM +0200, Sergio Paracuellos wrote:
> > > > Hi Dan,
> > > >
> > > > On Mon, Jun 7, 2021 at 8:59 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > >
> > > > > On Sat, Jun 05, 2021 at 09:30:23AM +0200, Sergio Paracuellos wrote:
> > > > > > Properties 'clocks', 'resets' and 'phys' have been moved from parent
> > > > > > node to the root port childs. Hence we have to adapt the way device
> > > > > > tree is parsed in driver code to properly align things and make all
> > > > > > the stuff work.
> > > > > >
> > > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > >
> > > > > It sounds like this commit needs a fixes tag?  What does "to properly
> > > > > align things and make all the stuff work." in terms of what the user
> > > > > sees?
> > > >
> > > > I submitted this driver to get mainlined and when bindings have been
> > > > reviewed I've been told to move this stuff into child nodes. Until now
> > > > all was also being properly working but with these properties defined
> > > > in the parent node. So I don't think any Fixes tag is needed here. So
> > > > hopefully changes on this patchset are the last need to get this
> > > > properly mainlined. I've been told to just make a 'git mv' without
> > > > zero changes from the staging driver, that's why I am submitting
> > > > changes to staging before.
> > >
> > > I'm really trying to understand how this affects the user experience but
> > > it sounds like you don't know either but you were told it was the
> > > correct thing and it seems to work?
> >
> > What do you mean with "user experience" here? So to work with the
> > future mainlined driver we need the dts file to be aligned with device
> > tree parsing code. If we move these properties into child nodes
> > (previous patch do this) and the code is properly aligned, for the
> > user the change is transparent. This SoC is mostly used in openWRT
> > where new versions compile new code and device tree completely so I
> > don't expect any compatibility problems also because of these changes,
> > AFAICT.
> >
> > > That's not ideal but I can live
> > > with it I guess...  I guess hopefully no change but it's just a
> > > correctness issue?
> >
> > Seems that the bindings are more correct, moving the properties into
> > child nodes.
> >
> > >
> > > Btw, we moved from devm_reset_control_get_exclusive() to
> > > of_reset_control_get_exclusive().  Does that mean we need to add a call
> > > to reset_control_put() in the remove() path?
> >
> > Yes this has moved because we need to access the child node with
> > 'device_node' instead of 'device'. It seems there is not another
> > possibility with devm_* like the ones we have with clk and phy apis.
> > Ok, so I have to manually call 'reset_control_put'. Will add it, thanks.
> 
> Should this be enough for error and remove paths, right?

Looks good to me...  I'm not an expert on this at all...  (When I ask
a question about something it's never rhetorical question so if you had
told me it wasn't required then I would have accepted that as an answer
as well).

regards,
dan carpenter