Re: [PATCH v1] reset: Make optional stuff optional for all users

[Andy Shevchenko]

On Mon, 2017-04-03 at 16:27 +0200, Philipp Zabel wrote:
> Hi Andy,
> 
> thank you for the patch.
> 
> On Mon, 2017-04-03 at 15:26 +0300, Andy Shevchenko wrote:
> > There is Device Tree oriented check for optional resource. Of course
> > it will fail on non-DT platforms.
> > 
> > Remove this check to make things optional for all users.
> > 
> > Fixes: bb475230b8e5 ("reset: make optional functions really
> > optional")
> > Cc: Ramiro Oliveira <Ramiro.Oliveira@xxxxxxxxxxxx>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > ---
> > 
> > ÂThe reset framework is too Device Tree oriented, and who knows what
> > Âthe logic was behind the commit which introduced devm_reset_*()
> > Âfunctions without thinking out of the DT box.
> 
> At the time, there was nothing outside of the box to describe reset
> lines, and as far as I am aware there still isn't, so the devm_reset_*
> should behave as if the reset line is not specified in the non-DT
> case.

> Returning -EINVAL was reasonable in that case, before the API was
> changed to describe unavailable, optional reset controls as rstc =
> NULL.

Fair enough.

> 
> > ÂThis commit fixes almost all Intel newest boards that have no
> > legacy
> > ÂUART since UART driver started using this DT-centric framework.
> 
> Is this is about 8250_dw.c? Unfortunately it sometimes takes a little
> while for me to get updated on the big picture, as I only get the
> actual
> reset driver and framework patches in my inbox. Usually I only see the
> reset consumer changes when I actively look for them.

Yes, 8250_dw.c in this case.

> 
> But the fault in this case was is with me not considering all possible
> code paths influenced by commit bb475230b8e5, in the configurations
> that
> I can't test myself.

Understood.

> 
> > Âdrivers/reset/core.c | 3 ---
> > Â1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> > index f1e5e65388bb..62314e663f29 100644
> > --- a/drivers/reset/core.c
> > +++ b/drivers/reset/core.c
> > @@ -331,9 +331,6 @@ struct reset_control
> > *__of_reset_control_get(struct device_node *node,
> 
> Ideally, __of_reset_control_get would not be called at all in the non-
> DT
> case. I'll change that in the next round, but for now I'd prefer a
> small
> fix in place.
> 
> > Â	int rstc_id;
> > Â	int ret;
> > Â
> > -	if (!node)
> > -		return ERR_PTR(-EINVAL);
> > -
> 
> This should be
> 
> 	if (!node)
> 		return optional ? NULL : ERR_PTR(-EINVAL);
> 
> instead. Can you confirm this works for Intel boards with DW UART? I
> can
> fix it up when applying if you agree.

I don't think it worth to change. I specifically checked all of_* calls
in that function and they cope pretty nice with node == NULL.

So, I rather to go with my initial change.

Thanks for review!

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy