[PATCH] pwm: samsung: Implement .apply() callback

[Uwe Kleine-König]

On Fri, Apr 22, 2022 at 05:11:02PM +0200, Krzysztof Kozlowski wrote:
> On 28/03/2022 09:34, Uwe Kleine-König wrote:
> > To eventually get rid of all legacy drivers convert this driver to the
> > modern world implementing .apply().
> > 
> > The size check for state->period is moved to .apply() to make sure that
> > the values of state->duty_cycle and state->period are passed to
> > pwm_samsung_config without change while they are discarded to int.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig at pengutronix.de>
> > ---
> >  drivers/pwm/pwm-samsung.c | 54 ++++++++++++++++++++++++++++++---------
> >  1 file changed, 42 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
> > index 0a4ff55fad04..9c5b4f515641 100644
> > --- a/drivers/pwm/pwm-samsung.c
> > +++ b/drivers/pwm/pwm-samsung.c
> > @@ -321,14 +321,6 @@ static int __pwm_samsung_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	struct samsung_pwm_channel *chan = pwm_get_chip_data(pwm);
> >  	u32 tin_ns = chan->tin_ns, tcnt, tcmp, oldtcmp;
> >  
> > -	/*
> > -	 * We currently avoid using 64bit arithmetic by using the
> > -	 * fact that anything faster than 1Hz is easily representable
> > -	 * by 32bits.
> > -	 */
> > -	if (period_ns > NSEC_PER_SEC)
> > -		return -ERANGE;
> > -
> >  	tcnt = readl(our_chip->base + REG_TCNTB(pwm->hwpwm));
> >  	oldtcmp = readl(our_chip->base + REG_TCMPB(pwm->hwpwm));
> >  
> > @@ -438,13 +430,51 @@ static int pwm_samsung_set_polarity(struct pwm_chip *chip,
> >  	return 0;
> >  }
> >  
> > +static int pwm_samsung_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			     const struct pwm_state *state)
> > +{
> > +	int err, enabled = pwm->state.enabled;
> > +
> > +	if (state->polarity != pwm->state.polarity) {
> > +		if (enabled) {
> > +			pwm_samsung_disable(chip, pwm);
> > +			enabled = false;
> > +		}
> > +
> > +		err = pwm_samsung_set_polarity(chip, pwm, state->polarity);
> > +		if (err)
> > +			return err;
> > +	}
> > +
> > +	if (!state->enabled) {
> > +		if (enabled)
> > +			pwm_samsung_disable(chip, pwm);
> > +
> > +		return 0;
> > +	}
> > +
> > +	/*
> > +	 * We currently avoid using 64bit arithmetic by using the
> > +	 * fact that anything faster than 1Hz is easily representable
> > +	 * by 32bits.
> > +	 */
> > +	if (state->period > NSEC_PER_SEC)
> 
> Isn't this changing a bit logic in case of setting wrong period and
> inverted signal?
> 
> Before, the config would return early and do nothing. Now, you disable
> the PWM, check the condition here and exit with PWM disabled. I think
> this should reverse the tasks done, or the check should be done early.

The intension here is to just push the legacy implementation of .apply()
(i.e.  pwm_apply_legacy()) into the driver, to be able to get rid of the
legacy handing in the core. I think the problem you point out is real,
but it is not introduced by my change which doesn't change behaviour.

The problem is just that it's not possible to "see" that period is
invalid at the time the polarity is changed and so it exists since at
least 5ec803edcb703fe379836f13560b79dfac79b01d, my patch just made it
more obvious.

So yes, there is potential to further improve the driver, but that's out
of scope for this 1:1 conversion patch.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20220425/47df3ec7/attachment.sig>