On Mon, Jun 7, 2021 at 2:05 PM Dan Carpenter [off-list ref] wrote:

On Mon, Jun 07, 2021 at 01:30:58PM +0200, Sergio Paracuellos wrote:

quoted

Hi Dan,

On Mon, Jun 7, 2021 at 1:10 PM Sergio Paracuellos
[off-list ref] wrote:

quoted

Hi Dan,

On Mon, Jun 7, 2021 at 12:37 PM Dan Carpenter [off-list ref] wrote:

quoted

On Mon, Jun 07, 2021 at 09:11:13AM +0200, Sergio Paracuellos wrote:

quoted

Hi Dan,

On Mon, Jun 7, 2021 at 8:59 AM Dan Carpenter [off-list ref] wrote:

quoted

On Sat, Jun 05, 2021 at 09:30:23AM +0200, Sergio Paracuellos wrote:

quoted

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.

quoted

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.

quoted

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