From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>, "Thierry Reding" <thierry.reding@gmail.com>, "Lee Jones" <lee.jones@linaro.org> Cc: linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-pwm@vger.kernel.org, kernel@pengutronix.de Subject: Re: [PATCH] pwm: samsung: Implement .apply() callback Date: Fri, 22 Apr 2022 17:11:02 +0200 [thread overview] Message-ID: <d7030fc5-3165-0255-0055-b2bc2e387d53@linaro.org> (raw) In-Reply-To: <20220328073434.44848-1-u.kleine-koenig@pengutronix.de> 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@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. > + return -ERANGE; > + > + err = pwm_samsung_config(chip, pwm, state->duty_cycle, state->period); > + if (err) > + return err; Best regards, Krzysztof
WARNING: multiple messages have this Message-ID (diff)
From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>, "Thierry Reding" <thierry.reding@gmail.com>, "Lee Jones" <lee.jones@linaro.org> Cc: linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-pwm@vger.kernel.org, kernel@pengutronix.de Subject: Re: [PATCH] pwm: samsung: Implement .apply() callback Date: Fri, 22 Apr 2022 17:11:02 +0200 [thread overview] Message-ID: <d7030fc5-3165-0255-0055-b2bc2e387d53@linaro.org> (raw) In-Reply-To: <20220328073434.44848-1-u.kleine-koenig@pengutronix.de> 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@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. > + return -ERANGE; > + > + err = pwm_samsung_config(chip, pwm, state->duty_cycle, state->period); > + if (err) > + return err; Best regards, Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-04-22 15:11 UTC|newest] Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-03-28 7:34 [PATCH] pwm: samsung: Implement .apply() callback Uwe Kleine-König 2022-03-28 7:34 ` Uwe Kleine-König 2022-04-22 15:11 ` Krzysztof Kozlowski [this message] 2022-04-22 15:11 ` Krzysztof Kozlowski 2022-04-25 17:16 ` Uwe Kleine-König 2022-04-25 17:16 ` Uwe Kleine-König 2022-04-25 17:50 ` Krzysztof Kozlowski 2022-04-25 17:50 ` Krzysztof Kozlowski 2022-05-20 13:59 ` Thierry Reding 2022-05-20 13:59 ` Thierry Reding [not found] ` <165450388336.27627.15000754926080179182.git-patchwork-notify@kernel.org> [not found] ` <20220619202152.y7jvzs4jtkvdl67j@pengutronix.de> 2022-06-20 6:31 ` Uwe Kleine-König
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=d7030fc5-3165-0255-0055-b2bc2e387d53@linaro.org \ --to=krzysztof.kozlowski@linaro.org \ --cc=kernel@pengutronix.de \ --cc=lee.jones@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-pwm@vger.kernel.org \ --cc=linux-samsung-soc@vger.kernel.org \ --cc=thierry.reding@gmail.com \ --cc=u.kleine-koenig@pengutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.