Re: [PATCH v2 2/6] pwm: core: make the PWM_POLARITY flag in DTB optional

[Bhuvanchandra DV]

Hi Lukasz,

On 10/06/16 12:06, Lukasz Majewski wrote:

> Hi Bhuvanchandra,
>
> > From: Lothar Wassmann <LW@xxxxxxxxxxxxxxxxxxx>
> >
> > Change the pwm chip driver registration, so that a chip driver that
> > supports polarity inversion can still be used with DTBs that don't
> > provide the 'PWM_POLARITY' flag.
> >
> > This is done to provide polarity inversion support for the pwm-imx
> > driver without having to modify all existing DTS files.
> >
> > Signed-off-by: Lothar Wassmann <LW@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@xxxxxxxxxxx>
> > Suggested-by: Thierry Reding <thierry.reding@xxxxxxxxx>
> > ---
> >   drivers/pwm/core.c | 27 ++++++++++++++++-----------
> >   1 file changed, 16 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 195373e..aae8db3 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -137,9 +137,14 @@ 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)
> >   		return ERR_PTR(-EINVAL);
> >   
> > +	/* flags in the third cell are optional */
> > +	if (args->args_count < 2)
> > +		return ERR_PTR(-EINVAL);
> > +
> >   	if (args->args[0] >= pc->npwm)
> >   		return ERR_PTR(-EINVAL);
> >   
> > @@ -149,10 +154,12 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc,
> > const struct of_phandle_args *args)
> >   	pwm->args.period = args->args[1];
> >   
> > -	if (args->args[2] & PWM_POLARITY_INVERTED)
> > -		pwm->args.polarity = PWM_POLARITY_INVERSED;
> > -	else
> > -		pwm->args.polarity = PWM_POLARITY_NORMAL;
> > +	if (args->args_count > 2) {
> > +		if (args->args[2] & PWM_POLARITY_INVERTED)
> > +			pwm_set_polarity(pwm, PWM_POLARITY_INVERSED);
> 			^^^^^^^^^^^^^^^^
> 			here we should set pwm->args.polarity, since
> 			the pwm_set_polarity() calls pwm_apply_state()
> 			which requires duty_cycle and period to be set.
>
> 			In this particular moment it is not yet set and
> 			polarity is not properly configured.

Agreed. Will do a clean v3 patchset along with the patch you sent
(pwm: core: Use pwm->args.polarity to setup PWM_POLARITY_INVERSED).

--
Bhuvan

> Patch fixing this will be sent as a reply to this e-mail. Please just
> squash it and test on your platform.
>
> Best regards,
> Åukasz Majewski
>
> > +		else
> > +			pwm_set_polarity(pwm, PWM_POLARITY_NORMAL);
> > +	}
> >   
> >   	return pwm;
> >   }
> > @@ -163,9 +170,14 @@ of_pwm_simple_xlate(struct pwm_chip *pc, const
> > struct of_phandle_args *args) {
> >   	struct pwm_device *pwm;
> >   
> > +	/* sanity check driver support */
> >   	if (pc->of_pwm_n_cells < 2)
> >   		return ERR_PTR(-EINVAL);
> >   
> > +	/* all cells are required */
> > +	if (args->args_count != pc->of_pwm_n_cells)
> > +		return ERR_PTR(-EINVAL);
> > +
> >   	if (args->args[0] >= pc->npwm)
> >   		return ERR_PTR(-EINVAL);
> >   
> > @@ -672,13 +684,6 @@ struct pwm_device *of_pwm_get(struct device_node
> > *np, const char *con_id) goto put;
> >   	}
> >   
> > -	if (args.args_count != pc->of_pwm_n_cells) {
> > -		pr_debug("%s: wrong #pwm-cells for %s\n",
> > np->full_name,
> > -			 args.np->full_name);
> > -		pwm = ERR_PTR(-EINVAL);
> > -		goto put;
> > -	}
> > -
> >   	pwm = pc->of_xlate(pc, &args);
> >   	if (IS_ERR(pwm))
> >   		goto put;