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@xxxxxxxxxxxxxx>
> ---
> 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.
> + return -ERANGE;
> +
> + err = pwm_samsung_config(chip, pwm, state->duty_cycle, state->period);
> + if (err)
> + return err;
Best regards,
Krzysztof
