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). regards, dan carpenter
