On Mon, Jun 7, 2021 at 2:05 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxx> wrote:
> > >
> > > Hi Dan,
> > >
> > > On Mon, Jun 7, 2021 at 12:37 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxx>
> > > > > >
> > > > > > 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).
I see :). But in this case since we are getting that without using
devm_* APIS the put is needed, AFAICT.
Thanks,
Sergio Paracuellos
>
> regards,
> dan carpenter
>
