Re: [PATCH 1/4] pwm: Make of_pwm_xlate_with_flags() work with #pwm-cells = <2>

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3324 bytes --]

On Sat, Apr 24, 2021 at 01:29:57PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Fri, Apr 09, 2021 at 03:36:40PM +0200, Thierry Reding wrote:
> > On Mon, Mar 15, 2021 at 12:11:21PM +0100, Uwe Kleine-König wrote:
> > > The two functions of_pwm_simple_xlate() and of_pwm_xlate_with_flags() are
> > > quite similar. of_pwm_simple_xlate() only supports two pwm-cells while
> > > of_pwm_xlate_with_flags() only support >= 3 pwm-cells. The latter can
> > > easily be modified to behave identically to of_pwm_simple_xlate for two
> > > pwm-cells. This is implemented here and allows to drop
> > > of_pwm_simple_xlate() in the next commit.
> > > 
> > > There is a small detail that is different now between of_pwm_simple_xlate()
> > > and of_pwm_xlate_with_flags() with pwm-cells = <2>: pwm->args.polarity is
> > > unconditionally initialized to PWM_POLARITY_NORMAL in the latter. I didn't
> > > find a case where this matters and doing that explicitly is the more
> > > robust approach.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  drivers/pwm/core.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > index b1adf3bb8508..39b0ad506bdd 100644
> > > --- a/drivers/pwm/core.c
> > > +++ b/drivers/pwm/core.c
> > > @@ -126,8 +126,7 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
> > >  {
> > >  	struct pwm_device *pwm;
> > >  
> > > -	/* check, whether the driver supports a third cell for flags */
> > > -	if (pc->of_pwm_n_cells < 3)
> > > +	if (pc->of_pwm_n_cells < 2)
> > >  		return ERR_PTR(-EINVAL);
> > >  
> > >  	/* flags in the third cell are optional */
> > > @@ -144,7 +143,8 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
> > >  	pwm->args.period = args->args[1];
> > >  	pwm->args.polarity = PWM_POLARITY_NORMAL;
> > >  
> > > -	if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED)
> > > +	if (pc->of_pwm_n_cells >= 3 && args->args_count > 2 &&
> > > +	    args->args[2] & PWM_POLARITY_INVERTED)
> > 
> > This might more clearly look like a superset of of_pwm_xlate_simple() if
> > you split up the conditional this way:
> > 
> > 	if (pc->of_pwm_n_cells >= 3) {
> > 		if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED)
> > 			...
> > 	}
> 
> I'm not convinced here. Being (more) obviously a superset of
> of_pwm_xlate_simple has very limited impact, as of_pwm_xlate_simple is
> going away.

That's precisely the point here. Since you are replacing the existing
of_pwm_xlate_simple() with this one, it's useful to make it as clear as
possible that this is, in fact, a superset.

> Also I expect that a construct like that will attract monkey
> patchers that will change the construct back to what I initially
> suggested. (I'm surprised that there isn't a coccinelle recipe to
> simplify such a construct, but maybe I just failed to find it.)

I don't mind rejecting non-sense patches that some script suggested. If
there's indeed some script or coccinelle recipe that would suggest
merging the conditions in this case, then I would argue that it's broken
and should be fixed or removed.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]