Re: [PATCH v5 2/2] leds: add ktd202x driver
Hello André,
On Sun, Oct 01, 2023 at 06:56:20PM +0200, André Apitzsch wrote:
> Am Sonntag, dem 01.10.2023 um 17:15 +0200 schrieb Christophe JAILLET:
> > Le 01/10/2023 à 15:52, André Apitzsch a écrit :
> > > + for_each_available_child_of_node(np, child) {
> > > + u32 mono_color = 0;
> >
> > Un-needed init.
> > And, why is it defined here, while reg is defined out-side the loop?
>
> I'll move it out-side the loop (without initialization).
In my book a variable with a narrow scope is better. I didn't check, but
if you can restrict both variables to the for loop, that's nicer.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v6 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
On Mon, Apr 19, 2021 at 09:00:07AM +0900, Nobuhiro Iwamatsu wrote: > Add driver for the PWM controller on Toshiba Visconti ARM SoC. > > Signed-off-by: Nobuhiro Iwamatsu Reviewed-by: Uwe Kleine-König Thanks for your endurance to improve the driver Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v5 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
Hello,
just a few smaller issues left to fix.
On Sun, Apr 18, 2021 at 08:09:04PM +0900, Nobuhiro Iwamatsu wrote:
> diff --git a/drivers/pwm/pwm-visconti.c b/drivers/pwm/pwm-visconti.c
> new file mode 100644
> index ..166b18ac1a3a
> --- /dev/null
> +++ b/drivers/pwm/pwm-visconti.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Toshiba Visconti pulse-width-modulation controller driver
> + *
> + * Copyright (c) 2020 TOSHIBA CORPORATION
> + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation
We're in 2021, so you might want to adapt the year in the copy right
notice.
> + *
> + * Authors: Nobuhiro Iwamatsu
> + *
> + * Limitations:
> + * - PIPGM_PWMC is a 2-bit divider (00: 1, 01: 2, 10: 4, 11: 8).
This is too detailed for the purpose of this section. Please either drop
it or make this:
- The fixed input clock is running at 1 MHz and is divided by either 1,
2, 4 or 8.
> + * - Fixed input clock running at 1 MHz.
> + * - When the settings of the PWM are modified, the new values are shadowed
> + * in hardware until the PIPGM_PCSR register is written and the currently
> + * running period is completed. This way the hardware switches atomically
> + * from the old setting to the new.
> + * - Disabling the hardware completes the currently running period and keeps
> + * the output at low level at all times.
> + */
> +
> [...]
> + /*
> + * PWMC controls a divider that divides the input clk by a
> + * power of two between 1 and 8. As a smaller divider yields
> + * higher precision, pick the smallest possible one.
> + */
> + if (period > 0x) {
> + pwmc0 = ilog2(period >> 16);
> + BUG_ON(pwmc0 > 3);
> + } else
> + pwmc0 = 0;
The linux coding style mandates that you should use braces for both
branches. (i.e.
+ if (period > 0x) {
+ pwmc0 = ilog2(period >> 16);
+ BUG_ON(pwmc0 > 3);
+ } else {
+ pwmc0 = 0;
+ }
)
> + period >>= pwmc0;
> + duty_cycle >>= pwmc0;
> +
> + if (state->polarity == PWM_POLARITY_INVERSED)
> + pwmc0 |= PIPGM_PWMC_PWMACT;
> + writel(pwmc0, priv->base + PIPGM_PWMC(pwm->hwpwm));
> + writel(duty_cycle, priv->base + PIPGM_PDUT(pwm->hwpwm));
> + writel(period, priv->base + PIPGM_PCSR(pwm->hwpwm));
> +
> + return 0;
> +}
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v8 4/8] dt-bindings: pwm: Support new PWM_USAGE_POWER flag
Hello Thierry,
On Fri, Apr 16, 2021 at 12:45:10PM +0200, Thierry Reding wrote:
> On Fri, Apr 16, 2021 at 11:32:12AM +0200, Uwe Kleine-König wrote:
> > On Thu, Apr 15, 2021 at 06:27:02PM +0200, Thierry Reding wrote:
> > > On Tue, Apr 13, 2021 at 07:56:31PM +0200, Uwe Kleine-König wrote:
> > > > Please define a rule that allows to judge if any given implementation is
> > > > correct or not. For the record neither "within reasonable bounds" nor "a
> > > > factor of 10 is not within reason" is good enough.
> > >
> > > We haven't had any rules thus far and I have yet to see a single report
> > > that drivers get this completely wrong. So "within reason", which I
> > > think is what driver authors will do by default, is good enough in
> > > practice.
> >
> > For me commit 11fc4edc483b ("pwm: bcm2835: Improve precision of PWM")
> > indicates that there is a problem. Someone used the pwm-ir-tx driver on
> > top of pwm-bcm2835. The former driver's expectation is that
> >
> > period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
> > duty = DIV_ROUND_CLOSEST(pwm_ir->duty_cycle * period, 100);
> > pwm_config(pwm, duty, period);
> >
> > yields a frequency near pwm_ir->carrier (minimizing
> >
> > abs(1 / implemented_freq - 1 / pwm_ir->carrier)
> >
> > (or should it minimize abs(implemented_freq - pwm_ir->carrier) instead?
> > Not entirely sure.). The result was that the pwm-bcm2835 driver was
> > changed to implement a different rounding. I took the time to look at
> > the drivers in 11fc4edc483b^, with the following result:
> >
> > - pwm-ab8500.c: ignores period_ns in .config
> > - pwm-atmel.c: rounds down period
> > - pwm-atmel-hlcdc.c: rounds up period (unsure)
> > - pwm-atmel-tcb.c: rounds down period (unsure)
> > - pwm-bcm2835.c: rounds down period
> > - pwm-bcm-iproc.c: rounds down period
> > - pwm-bcm-kona.c: rounds down period
> > - pwm-berlin.c: rounds down period
> > - pwm-brcmstb.c: rounds down period
> > - pwm-clps711x.c: doesn't support changing period (IMHO in a buggy way
> >because the period in the dts is just overwritten)
> > - pwm-crc: rounds down period
> > - pwm-cros-ec.c: doesn't support changing period
> > - pwm-ep93xx.c: rounds down period
> > - pwm-fsl-ftm.c: rounds down period
> > - pwm-hibvt.c: rounds down period
> > o pwm-img.c: confusing rounding behaviour
> > - pwm-imx1.c: just implements relative duty cycle
> > - pwm-imx27.c: rounds down period
> > + pwm-imx-tpm.c: rounds to nearest period (unsure)
> > - pwm-jz4740.c: rounds down period
> > - pwm-lp3943.c: rounds down period (apart from corner cases)
> > - pwm-lpc18xx-sct.c: rounds down period
> > - pwm-lpc32xx.c: rounds down period
> > - pwm-lpss.c: rounds up period
> > + pwm-mediatek.c: tries to implement round-nearest
> > - pwm-meson.c: tries to round down period
> > o pwm-mtk-disp.c: confusing rounding behaviour
> > - pwm-mxs.c: rounds down period
> > + pwm-omap-dmtimer.c: rounds to closest period
> > + pwm-pca9685.c: rounds to closest period
> > - pwm-puv3.c: rounds down period
> > - pwm-pxa.c: rounds down period
> > - pwm-rcar.c: rounds down period
> > o pwm-renesas-tpu.c: confusing rounding behaviour
> > + pwm-rockchip.c: rounds closest period
> > - pwm-samsung.c: rounds down period (unsure)
> > - pwm-sifive.c: rounds down period
> > - pwm-spear.c: rounds down period
> > - pwm-sti.c: rounds down period
> > - pwm-stm32.c: rounds down period
> > - pwm-stm32-lp.c: rounds down period
> > - pwm-stmpe.c: just implements relative duty cycle
> > + pwm-sun4i.c: rounds closest period
> > + pwm-tegra.c: tries to round nearest period
> > - pwm-tiecap.c: rounds down period
> > - pwm-tiehrpwm.c: rounds down period
> > - pwm-twl.c: just implements relative duty cycle
> > - pwm-twl-led.c: just implements relative duty cycle
> > - pwm-vt8500.c: rounds down period
> > - pwm-zx.c: rounds down period
> >
> > (- = doesn't behave "reasonable" to be used by pwm-ir-tx, + = behaves
> > "reasonable" for pwm-ir-tx, o = don't know, too complicated for me to
> > understand quickly (should we count that as -?))
>
> I'm not sure I understand correctly, but aren't you actually making a
> point against always using round-down now?
Not really. This is indeed a case where round-down isn't the right thing
for a consumer. As there are also use cases where ro
Re: [PATCH v9 8/8] pwm: pca9685: Add error messages for failed regmap calls
On Thu, Apr 15, 2021 at 02:14:55PM +0200, Clemens Gruber wrote: > Regmap operations can fail if the underlying subsystem is not working > properly (e.g. hogged I2C bus, etc.) > As this is useful information for the user, print an error message if it > happens. > Let probe fail if the first regmap_read or the first regmap_write fails. > > Signed-off-by: Clemens Gruber Acked-by: Uwe Kleine-König Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v9 7/8] pwm: pca9685: Restrict period change for enabled PWMs
On Thu, Apr 15, 2021 at 02:14:54PM +0200, Clemens Gruber wrote: > Previously, the last used PWM channel could change the global prescale > setting, even if other channels are already in use. > > Fix it by only allowing the first enabled PWM to change the global > chip-wide prescale setting. If there is more than one channel in use, > the prescale settings resulting from the chosen periods must match. > > GPIOs do not count as enabled PWMs as they are not using the prescaler > and can't change it. > > Signed-off-by: Clemens Gruber Acked-by: Uwe Kleine-König Thanks Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v9 2/8] pwm: pca9685: Support hardware readout
Hello, On Thu, Apr 15, 2021 at 02:14:49PM +0200, Clemens Gruber wrote: > Implement .get_state to read-out the current hardware state. > > The hardware readout may return slightly different values than those > that were set in apply due to the limited range of possible prescale and > counter register values. > > Also note that although the datasheet mentions 200 Hz as default > frequency when using the internal 25 MHz oscillator, the calculated > period from the default prescaler register setting of 30 is 5079040ns. > > Signed-off-by: Clemens Gruber > --- > Changes since v8: > - As we left the math in apply as-is, use DIV_ROUND_DOWN in get_state I first thought this was wrong, because .apply uses ROUND_CLOSEST for period and ROUND_UP for duty. But as the calculation for period is exact this doesn't matter and round-down is indeed correct here. Reviewed-by: Uwe Kleine-König Thanks Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v9 1/8] pwm: pca9685: Switch to atomic API
On Thu, Apr 15, 2021 at 02:14:48PM +0200, Clemens Gruber wrote: > The switch to the atomic API goes hand in hand with a few fixes to > previously experienced issues: > - The duty cycle is no longer lost after disable/enable (previously the > OFF registers were cleared in disable and the user was required to > call config to restore the duty cycle settings) > - If one sets a period resulting in the same prescale register value, > the sleep and write to the register is now skipped > - Previously, only the full ON bit was toggled in GPIO mode (and full > OFF cleared if set to high), which could result in both full OFF and > full ON not being set and on=0, off=0, which is not allowed according > to the datasheet > - The OFF registers were reset to 0 in probe, which could lead to the > forbidden on=0, off=0. Fixed by resetting to POR default (full OFF) > > Signed-off-by: Clemens Gruber (I sent my ack to v8 before, but indeed this was the version I intended to ack) Acked-by: Uwe Kleine-König -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
Hello Nobuhiro, On Fri, Apr 16, 2021 at 09:15:23PM +0900, Nobuhiro Iwamatsu wrote: > > > > For me the critical (and only) difference between "off" and > > > > "duty cycle = 0" is that when a new configuration is to be applied. In > > > > the "off" state a new period can (and should) start immediately, while > > > > with "duty_cycle = 0" the rising edge should be delayed until the > > > > currently running period is over.[1] > > > > > > > > So the thing to do here (IMHO) is: > > > > > > > > Iff with PIPGM_PCSR = 0 configuring a new setting (that is finalized > > > > with writing a non-zero value to PIPGM_PCSR) completes the currently > > > > running period, then always assume the PWM as enabled. > > > > > > Yes, this device works that way. > > > > OK, then please use > > > > state->enabled = true > > > > unconditionally in visconti_pwm_get_state(). > > Please let me check. > If I unconditionally add 'state->enabled = true' to visconti_pwm_get_state(), > state->enabled is set to true because visconti_pwm_get_state() is called when > the device is created (this is when I write the device number to the export of > /sys/class/pwm/pwmchip0 ). > And since PIPGM_PCSR is 0 in this state, the pulse by PWM is not output. A PWM that is currently configured with .enabled = true and .duty_cycle = 0 doesn't have a pulse, so this is fine. > However, I think this means that the device is working as this driver. I don't understand this sentence. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v8 8/8] pwm: pca9685: Add error messages for failed regmap calls
On Mon, Apr 12, 2021 at 03:27:45PM +0200, Clemens Gruber wrote: > Regmap operations can fail if the underlying subsystem is not working > properly (e.g. hogged I2C bus, etc.) > As this is useful information for the user, print an error message if it > happens. > Let probe fail if the first regmap_read or the first regmap_write fails. > > Signed-off-by: Clemens Gruber Acked-by: Uwe Kleine-König Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v8 7/8] pwm: pca9685: Restrict period change for enabled PWMs
On Mon, Apr 12, 2021 at 03:27:44PM +0200, Clemens Gruber wrote: > Previously, the last used PWM channel could change the global prescale > setting, even if other channels are already in use. > > Fix it by only allowing the first enabled PWM to change the global > chip-wide prescale setting. If there is more than one channel in use, > the prescale settings resulting from the chosen periods must match. > > GPIOs do not count as enabled PWMs as they are not using the prescaler > and can't change it. > > Signed-off-by: Clemens Gruber I think this patch could be a tad simpler (by just counting the number of enabled channels instead of maintaining a bitmap). Still this is beneficial, so: Acked-by: Uwe Kleine-König Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v8 4/8] dt-bindings: pwm: Support new PWM_USAGE_POWER flag
On Fri, Apr 16, 2021 at 05:54:29PM +0200, Clemens Gruber wrote: > On Fri, Apr 16, 2021 at 03:55:11PM +0200, Thierry Reding wrote: > > On Mon, Apr 12, 2021 at 03:27:41PM +0200, Clemens Gruber wrote: > > > Add the flag and corresponding documentation for PWM_USAGE_POWER. > > > > > > Cc: Rob Herring > > > Signed-off-by: Clemens Gruber > > > --- > > > Documentation/devicetree/bindings/pwm/pwm.txt | 3 +++ > > > include/dt-bindings/pwm/pwm.h | 1 + > > > 2 files changed, 4 insertions(+) > > > > Rob, what are your thoughts on this? I've been thinking about this some > > more and I'm having second thoughts about putting this into device tree > > because it doesn't actually describe a property of the PWM hardware but > > rather a use-case specific hint. It's a bit of a gray area because this > > is just part of the PWM specifier which already has use-case specific > > "configuration", such as the period and the polarity. This is something I'd prefer over making it part of the device tree API. I still don't think it's a good idea but when we keep it in-kernel we can at least easier modify it in the future. > > Perhaps a better place for this is within the PWM API? We could add the > > same information into struct pwm_state and then consumers that don't > > care about specifics of the signal (such as pwm-backlight) can set that > > flag when they request a state to be applied. > > I just want to note that in my opinion, this is not a flag that is > changed often, so is it really a good idea to require setting this > wherever PWM state is applied? Also, this can't be read-out in > .get_state. Not being able to read it out isn't a problem in my eyes. > Thierry: If this discussion carries on and a v10 is required: Could you > maybe merge the uncontroversial patches 1 to 3 of v9 separately and > maybe get those in 5.12 ? Patches 4 to 8 can probably wait for 5.13 and > have some time in linux-next. I'm ok in getting those into next now and than into the upcoming merge window. That won't make them part of 5.12 however, but 5.13-rc1. IMHO patches 7 and 8 can go in, too. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v8 1/8] pwm: pca9685: Switch to atomic API
On Mon, Apr 12, 2021 at 03:27:38PM +0200, Clemens Gruber wrote: > The switch to the atomic API goes hand in hand with a few fixes to > previously experienced issues: > - The duty cycle is no longer lost after disable/enable (previously the > OFF registers were cleared in disable and the user was required to > call config to restore the duty cycle settings) > - If one sets a period resulting in the same prescale register value, > the sleep and write to the register is now skipped > - Previously, only the full ON bit was toggled in GPIO mode (and full > OFF cleared if set to high), which could result in both full OFF and > full ON not being set and on=0, off=0, which is not allowed according > to the datasheet > - The OFF registers were reset to 0 in probe, which could lead to the > forbidden on=0, off=0. Fixed by resetting to POR default (full OFF) I didn't recheck all details, but the patch is definitively an improvement, so: Acked-by: Uwe Kleine-König Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
Hello Nobuhiro, On Fri, Apr 16, 2021 at 05:07:21PM +0900, Nobuhiro Iwamatsu wrote: > On Mon, Apr 12, 2021 at 09:02:32AM +0200, Uwe Kleine-König wrote: > > On Mon, Apr 12, 2021 at 11:55:36AM +0900, Nobuhiro Iwamatsu wrote: > > > On Sat, Apr 10, 2021 at 03:53:21PM +0200, Uwe Kleine-König wrote: > > > > Can you please put a paragraph analogous to the one in pwm-sifive in the > > > > same format. This simplified keeping an overview about the oddities of > > > > the various supported chips. > > > > > > OK, I will check pwm-sifive's, and add. > > I will add the following : > > * Limitations: > * - PIPGM_PWMC is a 2-bit divider (00: 1, 01: 2, 10: 4, 11: 8) for the input > * clock running at 1 MHz. I would strip that to: - Fixed input clock running at 1 MHz > * - When the settings of the PWM are modified, the new values are shadowed > * in hardware until the PIPGM_PCSR register is written and the currently > * running period is completed. This way the hardware switches atomically > * from the old setting to the new. > * - Disabling the hardware completes the currently running period and keeps > * the output at low level at all times. This looks fine. > > For me the critical (and only) difference between "off" and > > "duty cycle = 0" is that when a new configuration is to be applied. In > > the "off" state a new period can (and should) start immediately, while > > with "duty_cycle = 0" the rising edge should be delayed until the > > currently running period is over.[1] > > > > So the thing to do here (IMHO) is: > > > > Iff with PIPGM_PCSR = 0 configuring a new setting (that is finalized > > with writing a non-zero value to PIPGM_PCSR) completes the currently > > running period, then always assume the PWM as enabled. > > Yes, this device works that way. OK, then please use state->enabled = true unconditionally in visconti_pwm_get_state(). Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v8 4/8] dt-bindings: pwm: Support new PWM_USAGE_POWER flag
Hello Thierry,
On Thu, Apr 15, 2021 at 06:27:02PM +0200, Thierry Reding wrote:
> On Tue, Apr 13, 2021 at 07:56:31PM +0200, Uwe Kleine-König wrote:
> > On Tue, Apr 13, 2021 at 01:51:15PM +0200, Thierry Reding wrote:
> > > On Mon, Apr 12, 2021 at 06:27:23PM +0200, Uwe Kleine-König wrote:
> > > > On Mon, Apr 12, 2021 at 03:27:41PM +0200, Clemens Gruber wrote:
> > > > > Cc: Rob Herring
> > > > > Signed-off-by: Clemens Gruber
> > > > > ---
> > > > > Documentation/devicetree/bindings/pwm/pwm.txt | 3 +++
> > > > > include/dt-bindings/pwm/pwm.h | 1 +
> > > > > 2 files changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt
> > > > > b/Documentation/devicetree/bindings/pwm/pwm.txt
> > > > > index 084886bd721e..fe3a28f887c0 100644
> > > > > --- a/Documentation/devicetree/bindings/pwm/pwm.txt
> > > > > +++ b/Documentation/devicetree/bindings/pwm/pwm.txt
> > > > > @@ -46,6 +46,9 @@ period in nanoseconds.
> > > > > Optionally, the pwm-specifier can encode a number of flags (defined
> > > > > in
> > > > > ) in a third cell:
> > > > > - PWM_POLARITY_INVERTED: invert the PWM signal polarity
> > > > > +- PWM_USAGE_POWER: Only care about the power output of the signal.
> > > > > This
> > > > > + allows drivers (if supported) to optimize the signals, for example
> > > > > to
> > > > > + improve EMI and reduce current spikes.
> > > >
> > > > IMHO there are too many open questions about which freedom this gives to
> > > > the lowlevel driver. If the consumer requests .duty_cycle = 25ns +
> > > > .period = 100ns, can the driver provide .duty_cycle = 25s + .period =
> > > > 100s which nominally has the same power output? Let's not introduce more
> > > > ambiguity than there already is.
> > >
> > > The freedom given to the driver should be to adjust the signal within
> > > reasonable bounds. Changing the time unit by a factor of 10 is
> > > not within reason, and I doubt anyone would interpret it that way, even
> > > if we didn't document this at all.
> >
> > Please define a rule that allows to judge if any given implementation is
> > correct or not. For the record neither "within reasonable bounds" nor "a
> > factor of 10 is not within reason" is good enough.
>
> We haven't had any rules thus far and I have yet to see a single report
> that drivers get this completely wrong. So "within reason", which I
> think is what driver authors will do by default, is good enough in
> practice.
For me commit 11fc4edc483b ("pwm: bcm2835: Improve precision of PWM")
indicates that there is a problem. Someone used the pwm-ir-tx driver on
top of pwm-bcm2835. The former driver's expectation is that
period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
duty = DIV_ROUND_CLOSEST(pwm_ir->duty_cycle * period, 100);
pwm_config(pwm, duty, period);
yields a frequency near pwm_ir->carrier (minimizing
abs(1 / implemented_freq - 1 / pwm_ir->carrier)
(or should it minimize abs(implemented_freq - pwm_ir->carrier) instead?
Not entirely sure.). The result was that the pwm-bcm2835 driver was
changed to implement a different rounding. I took the time to look at
the drivers in 11fc4edc483b^, with the following result:
- pwm-ab8500.c: ignores period_ns in .config
- pwm-atmel.c: rounds down period
- pwm-atmel-hlcdc.c: rounds up period (unsure)
- pwm-atmel-tcb.c: rounds down period (unsure)
- pwm-bcm2835.c: rounds down period
- pwm-bcm-iproc.c: rounds down period
- pwm-bcm-kona.c: rounds down period
- pwm-berlin.c: rounds down period
- pwm-brcmstb.c: rounds down period
- pwm-clps711x.c: doesn't support changing period (IMHO in a buggy way
because the period in the dts is just overwritten)
- pwm-crc: rounds down period
- pwm-cros-ec.c: doesn't support changing period
- pwm-ep93xx.c: rounds down period
- pwm-fsl-ftm.c: rounds down period
- pwm-hibvt.c: rounds down period
o pwm-img.c: confusing rounding behaviour
- pwm-imx1.c: just implements relative duty cycle
- pwm-imx27.c: rounds down period
+ pwm-imx-tpm.c: rounds to nearest period (unsure)
- pwm-jz4740.c: rounds down period
- pwm-lp3943.c: rounds down period (apart from corner cases)
- pwm-lpc18xx-sct.c: rounds down period
- pwm-lpc32xx.c: rounds down period
- pwm-lpss.c: rounds up period
+ pwm-mediatek.c: tries to implement round-nearest
- pwm-meson.c: tries to round down period
o p
Re: [PATCH] pwm: mediatek: remove unused function
On Thu, Apr 15, 2021 at 04:35:53PM +0800, Jiapeng Chong wrote: > Fix the following clang warning: > > drivers/pwm/pwm-mediatek.c:110:19: warning: unused function > 'pwm_mediatek_readl' [-Wunused-function]. > > Reported-by: Abaci Robot > Signed-off-by: Jiapeng Chong Looks right, even though I would prefer to see a patch implementing .get_state instead (which probably would make use of this function). Acked-by: Uwe Kleine-König Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH 55/57] staging: comedi: drivers: ni_mio_common: Move 'range_ni_E_ao_ext' to where it is used
Hello Lee,
nitpick: You move range_ni_E_ao_ext to the header. However that header
doesn't use range_ni_E_ao_ext, so the subject is technically wrong.
On Wed, Apr 14, 2021 at 07:11:27PM +0100, Lee Jones wrote:
> ... and mark it as __maybe_unused since not all users of the
> header file reference it.
>
> Fixes the following W=1 kernel build warning(s):
>
> drivers/staging/comedi/drivers/ni_mio_common.c:163:35: warning:
> ‘range_ni_E_ao_ext’ defined but not used [-Wunused-const-variable=]
>
> Cc: Ian Abbott
> Cc: H Hartley Sweeten
> Cc: Greg Kroah-Hartman
> Cc: Thierry Reding
> Cc: "Uwe Kleine-König"
> Cc: Lee Jones
> Cc: "David A. Schleef"
> Cc: Mori Hess
> Cc: Truxton Fulton
> Cc: linux-stag...@lists.linux.dev
> Cc: linux-...@vger.kernel.org
> Signed-off-by: Lee Jones
> ---
> drivers/staging/comedi/drivers/ni_mio_common.c | 9 -
> drivers/staging/comedi/drivers/ni_stc.h| 9 -
> 2 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c
> b/drivers/staging/comedi/drivers/ni_mio_common.c
> index 4f80a4991f953..37615b4e2c10d 100644
> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> @@ -160,15 +160,6 @@ static const struct comedi_lrange range_ni_M_ai_628x = {
> }
> };
>
> -static const struct comedi_lrange range_ni_E_ao_ext = {
> - 4, {
> - BIP_RANGE(10),
> - UNI_RANGE(10),
> - RANGE_ext(-1, 1),
> - RANGE_ext(0, 1)
> - }
> -};
> -
> static const struct comedi_lrange *const ni_range_lkup[] = {
> [ai_gain_16] = _ni_E_ai,
> [ai_gain_8] = _ni_E_ai_limited,
> diff --git a/drivers/staging/comedi/drivers/ni_stc.h
> b/drivers/staging/comedi/drivers/ni_stc.h
> index fbc0b753a0f59..0822e65f709dd 100644
> --- a/drivers/staging/comedi/drivers/ni_stc.h
> +++ b/drivers/staging/comedi/drivers/ni_stc.h
> @@ -1137,6 +1137,13 @@ struct ni_private {
> u8 rgout0_usage;
> };
>
> -static const struct comedi_lrange range_ni_E_ao_ext;
> +static const struct comedi_lrange __maybe_unused range_ni_E_ao_ext = {
> + 4, {
> + BIP_RANGE(10),
> + UNI_RANGE(10),
> + RANGE_ext(-1, 1),
> + RANGE_ext(0, 1)
> + }
> +};
I think a downside of this solution is that range_ni_E_ao_ext might be
duplicated in the object files. (Ditto for the status quo BTW.)
I think the right approach to fix the compiler warning and the
duplication is to declare range_ni_E_ao_ext external in the header and
keep the definition in ni_mio_common.c.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v8 1/8] pwm: pca9685: Switch to atomic API
On Wed, Apr 14, 2021 at 09:45:32PM +0200, Clemens Gruber wrote: > On Wed, Apr 14, 2021 at 09:21:31PM +0200, Uwe Kleine-König wrote: > > On Wed, Apr 14, 2021 at 02:09:14PM +0200, Clemens Gruber wrote: > > > Hi Uwe, > > > > > > On Tue, Apr 13, 2021 at 09:38:18PM +0200, Uwe Kleine-König wrote: > > > > Hello Clemens, > > > > > > > > On Tue, Apr 13, 2021 at 02:11:38PM +0200, Clemens Gruber wrote: > > > > > On Mon, Apr 12, 2021 at 10:10:19PM +0200, Uwe Kleine-König wrote: > > > > > > On Mon, Apr 12, 2021 at 06:39:28PM +0200, Clemens Gruber wrote: > > > > > > > With your suggested round-down, the example with frequency of 200 > > > > > > > Hz > > > > > > > would no longer result in 30 but 29 and that contradicts the > > > > > > > datasheet. > > > > > > > > > > > > Well, with PRESCALE = 30 we get a frequency of 196.88 Hz and with > > > > > > PRESCALE = 29 we get a frequency of 203.45 Hz. So no matter if you > > > > > > pick > > > > > > 29 or 30, you don't get 200 Hz. And which of the two possible > > > > > > values is > > > > > > the better one depends on the consumer, no matter what rounding > > > > > > algorithm the data sheet suggests. Also note that the math here > > > > > > contains > > > > > > surprises you don't expect at first. For example, what PRESCALE > > > > > > value > > > > > > would you pick to get 284 Hz? [If my mail was a video, I'd suggest > > > > > > to > > > > > > press Space now to pause and let you think first :-)] The data > > > > > > sheet's > > > > > > formula suggests: > > > > > > > > > > > > round(25 MHz / (4096 * 284)) - 1 = 20 > > > > > > > > > > > > The resulting frequency when picking PRESCALE = 20 is 290.644 Hz > > > > > > (so an > > > > > > error of 6.644 Hz). If instead you pick PRESCALE = 21 you get > > > > > > 277.433 Hz > > > > > > (error = 6.567 Hz), so 21 is the better choice. > > > > > > > > > > > > Exercise for the reader: > > > > > > What is the correct formula to really determine the PRESCALE value > > > > > > that > > > > > > yields the best approximation (i.e. minimizing > > > > > > abs(real_freq - target_freq)) for a given target_freq? > > > > > > > > I wonder if you tried this. > > > > > > We could calculate both round-up and round-down and decide which one is > > > closer to "real freq" (even though that is not the actual frequency but > > > just our backwards-calculated frequency). > > > > Yeah, the backwards-calculated frequency is the best assumption we > > have. > > > > > But I can't give you a formula with minimized abs(real_freq-target_freq) > > > Is it a different round point than 0.5 and maybe relative to f ? > > > > > > Please enlighten us :-) > > > > Sorry, I cannot. I spend ~20 min today after lunch with pencil and > > paper, but without success. I was aware that it isn't trivial and this > > is the main reason I established round-down as default for new drivers > > instead of round-nearest. > > Oh, I thought you already solved it. I tried too for a while but was > unsuccessful. Not trivial indeed! > > But regarding you establishing round-down: Wouldn't it be even better if > the driver did what I suggested above, namely calculate backwards from > both the rounded-up as well as the rounded-down prescale value and then > write the one with the smallest abs(f_target - f_real) to the register? No, I don't think so for several reasons. First, just rounding down is easier (and keeping lowlevel drivers rules and implementation easy is IMHO a good goal). The second reason is that round-nearest is a bit ambigous because round to the nearest frequency is slightly different to round to the nearest period length. So to actually implement (or use) it correctly, people have to grasp that difference. Compared to that rounding down the period length corresponds 1:1 to rounding up frequency. That's easy. For the third reason I have to backup a bit: I intend to introduce a function pwm_round_rate that predicts what pwm_apply_rate will actually implement. Of course it must have the same rounding rules. This allows to implement efficient search for consumers that e.g. prefer round-nearest time, or round-nearest frequency. I'm convinced that searching the optimal request to make is easier if round_rate uses round-down and not round-nearest. All three reasons boil down to "the math for round-down is just simpler (for implementers and for users) than with round-nearest". Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v8 1/8] pwm: pca9685: Switch to atomic API
On Wed, Apr 14, 2021 at 02:09:14PM +0200, Clemens Gruber wrote: > Hi Uwe, > > On Tue, Apr 13, 2021 at 09:38:18PM +0200, Uwe Kleine-König wrote: > > Hello Clemens, > > > > On Tue, Apr 13, 2021 at 02:11:38PM +0200, Clemens Gruber wrote: > > > On Mon, Apr 12, 2021 at 10:10:19PM +0200, Uwe Kleine-König wrote: > > > > On Mon, Apr 12, 2021 at 06:39:28PM +0200, Clemens Gruber wrote: > > > > > With your suggested round-down, the example with frequency of 200 Hz > > > > > would no longer result in 30 but 29 and that contradicts the > > > > > datasheet. > > > > > > > > Well, with PRESCALE = 30 we get a frequency of 196.88 Hz and with > > > > PRESCALE = 29 we get a frequency of 203.45 Hz. So no matter if you pick > > > > 29 or 30, you don't get 200 Hz. And which of the two possible values is > > > > the better one depends on the consumer, no matter what rounding > > > > algorithm the data sheet suggests. Also note that the math here contains > > > > surprises you don't expect at first. For example, what PRESCALE value > > > > would you pick to get 284 Hz? [If my mail was a video, I'd suggest to > > > > press Space now to pause and let you think first :-)] The data sheet's > > > > formula suggests: > > > > > > > > round(25 MHz / (4096 * 284)) - 1 = 20 > > > > > > > > The resulting frequency when picking PRESCALE = 20 is 290.644 Hz (so an > > > > error of 6.644 Hz). If instead you pick PRESCALE = 21 you get 277.433 Hz > > > > (error = 6.567 Hz), so 21 is the better choice. > > > > > > > > Exercise for the reader: > > > > What is the correct formula to really determine the PRESCALE value that > > > > yields the best approximation (i.e. minimizing > > > > abs(real_freq - target_freq)) for a given target_freq? > > > > I wonder if you tried this. > > We could calculate both round-up and round-down and decide which one is > closer to "real freq" (even though that is not the actual frequency but > just our backwards-calculated frequency). Yeah, the backwards-calculated frequency is the best assumption we have. > But I can't give you a formula with minimized abs(real_freq-target_freq) > Is it a different round point than 0.5 and maybe relative to f ? > > Please enlighten us :-) Sorry, I cannot. I spend ~20 min today after lunch with pencil and paper, but without success. I was aware that it isn't trivial and this is the main reason I established round-down as default for new drivers instead of round-nearest. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v8 1/8] pwm: pca9685: Switch to atomic API
Hello Clemens, On Tue, Apr 13, 2021 at 02:11:38PM +0200, Clemens Gruber wrote: > On Mon, Apr 12, 2021 at 10:10:19PM +0200, Uwe Kleine-König wrote: > > On Mon, Apr 12, 2021 at 06:39:28PM +0200, Clemens Gruber wrote: > > > On Mon, Apr 12, 2021 at 06:18:08PM +0200, Uwe Kleine-König wrote: > > > > On Mon, Apr 12, 2021 at 03:27:38PM +0200, Clemens Gruber wrote: > > > > > The switch to the atomic API goes hand in hand with a few fixes to > > > > > previously experienced issues: > > > > > - The duty cycle is no longer lost after disable/enable (previously > > > > > the > > > > > OFF registers were cleared in disable and the user was required to > > > > > call config to restore the duty cycle settings) > > > > > - If one sets a period resulting in the same prescale register value, > > > > > the sleep and write to the register is now skipped > > > > > - Previously, only the full ON bit was toggled in GPIO mode (and full > > > > > OFF cleared if set to high), which could result in both full OFF and > > > > > full ON not being set and on=0, off=0, which is not allowed > > > > > according > > > > > to the datasheet > > > > > - The OFF registers were reset to 0 in probe, which could lead to the > > > > > forbidden on=0, off=0. Fixed by resetting to POR default (full OFF) > > > > > > > > > > Signed-off-by: Clemens Gruber > > > > > --- > > > > > Changes since v7: > > > > > - Moved check for !state->enabled before prescaler configuration > > > > > - Removed unnecessary cast > > > > > - Use DIV_ROUND_DOWN in .apply > > > > > > > > > > Changes since v6: > > > > > - Order of a comparison switched for improved readability > > > > > > > > > > Changes since v5: > > > > > - Function documentation for set_duty > > > > > - Variable initializations > > > > > - Print warning if all LEDs channel > > > > > - Changed EOPNOTSUPP to EINVAL > > > > > - Improved error messages > > > > > - Register reset corrections moved to this patch > > > > > > > > > > Changes since v4: > > > > > - Patches split up > > > > > - Use a single set_duty function > > > > > - Improve readability / new macros > > > > > - Added a patch to restrict prescale changes to the first user > > > > > > > > > > Changes since v3: > > > > > - Refactoring: Extracted common functions > > > > > - Read prescale register value instead of caching it > > > > > - Return all zeros and disabled for "all LEDs" channel state > > > > > - Improved duty calculation / mapping to 0..4096 > > > > > > > > > > Changes since v2: > > > > > - Always set default prescale value in probe > > > > > - Simplified probe code > > > > > - Inlined functions with one callsite > > > > > > > > > > Changes since v1: > > > > > - Fixed a logic error > > > > > - Impoved PM runtime handling and fixed !CONFIG_PM > > > > > - Write default prescale reg value if invalid in probe > > > > > - Reuse full_off/_on functions throughout driver > > > > > - Use cached prescale value whenever possible > > > > > > > > > > drivers/pwm/pwm-pca9685.c | 259 > > > > > +- > > > > > 1 file changed, 89 insertions(+), 170 deletions(-) > > > > > > > > > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c > > > > > index 4a55dc18656c..827b57ced3c2 100644 > > > > > --- a/drivers/pwm/pwm-pca9685.c > > > > > +++ b/drivers/pwm/pwm-pca9685.c > > > > > @@ -51,7 +51,6 @@ > > > > > #define PCA9685_PRESCALE_MAX 0xFF/* => min. frequency of 24 Hz */ > > > > > > > > > > #define PCA9685_COUNTER_RANGE4096 > > > > > -#define PCA9685_DEFAULT_PERIOD 500 /* Default period_ns = > > > > > 1/200 Hz */ > > > > > #define PCA9685_OSC_CLOCK_MHZ25 /* Internal oscillator > > > > > with 25 MHz */ > > > > > > > > > > #define PCA9685_NUMREGS 0xFF > > > > &
Re: [PATCH v8 4/8] dt-bindings: pwm: Support new PWM_USAGE_POWER flag
On Tue, Apr 13, 2021 at 01:51:15PM +0200, Thierry Reding wrote: > On Mon, Apr 12, 2021 at 06:27:23PM +0200, Uwe Kleine-König wrote: > > On Mon, Apr 12, 2021 at 03:27:41PM +0200, Clemens Gruber wrote: > > > Add the flag and corresponding documentation for PWM_USAGE_POWER. > > > > My concern here in the previous round was that PWM_USAGE_POWER isn't a > > name that intuitively suggests its semantic. Do you disagree? > > I suggested PWM_USAGE_POWER because I think it accurately captures what > we want here. > > > > Cc: Rob Herring > > > Signed-off-by: Clemens Gruber > > > --- > > > Documentation/devicetree/bindings/pwm/pwm.txt | 3 +++ > > > include/dt-bindings/pwm/pwm.h | 1 + > > > 2 files changed, 4 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt > > > b/Documentation/devicetree/bindings/pwm/pwm.txt > > > index 084886bd721e..fe3a28f887c0 100644 > > > --- a/Documentation/devicetree/bindings/pwm/pwm.txt > > > +++ b/Documentation/devicetree/bindings/pwm/pwm.txt > > > @@ -46,6 +46,9 @@ period in nanoseconds. > > > Optionally, the pwm-specifier can encode a number of flags (defined in > > > ) in a third cell: > > > - PWM_POLARITY_INVERTED: invert the PWM signal polarity > > > +- PWM_USAGE_POWER: Only care about the power output of the signal. This > > > + allows drivers (if supported) to optimize the signals, for example to > > > + improve EMI and reduce current spikes. > > > > IMHO there are too many open questions about which freedom this gives to > > the lowlevel driver. If the consumer requests .duty_cycle = 25ns + > > .period = 100ns, can the driver provide .duty_cycle = 25s + .period = > > 100s which nominally has the same power output? Let's not introduce more > > ambiguity than there already is. > > The freedom given to the driver should be to adjust the signal within > reasonable bounds. Changing the time unit by a factor of 10 is > not within reason, and I doubt anyone would interpret it that way, even > if we didn't document this at all. Please define a rule that allows to judge if any given implementation is correct or not. For the record neither "within reasonable bounds" nor "a factor of 10 is not within reason" is good enough. This is not only important to be able to review drivers that implement it, but also for consumers, because they should know what to expect. > To be frank I think that quest of yours to try and rid the PWM API of > all ambiguity is futile. I consider my quest about rounding reasonable. And I think this is painful because when the PWM framework was introduced it was too much ad hoc and the APIs were not thought through enough. And because I don't want to have that repeated, I express my concerns here. > I've been trying to be lenient because you seem > motivated, but I think you're taking this too far. There are always > going to be cases that aren't completely clear-cut and where drivers > need the flexibility to cheat in order to be useful at all. If we get to > a point where everything needs to be 100% accurate, the majority of the > PWM controllers won't be usable at all. > > Don't let perfect be the enemy of good. I admit here I don't have a constructive idea how to define what is needed. For example if we only care about the relative duty cycle, a consumer requests .period = 1045 .duty_cyle = 680 and the driver can provide multiples of 100 ns for both .period and .duty_cycle, the candidates that might be sensible to chose from are (IMHO): - exact relative duty: .period = 104500 .duty_cycle = 68000 - round both values in the same direction, minimizing error .period = 1100 .duty_cycle = 700 (requested relative duty = 65.07%, implemented = 63.64%; when rounding both down we get 60%) - round both values mathematically: .period = 1000 .duty_cycle = 700 (yielding a relative duty of 70% instead of the requested 65.07%) - Maybe .period = 1000 .duty_cycle = 600 might also be preferable for some consumers?! (60%) - Maybe .period = 2000 .duty_cycle = 1300 is a good compromise because the relative duty is nearly exactly matched and the period is only stretched by a factor < 2. In my eyes a driver author should be told which of these options should be picked. Do you consider it obvious which of these options is the objective best? If so why? Do you agree that we should tell driver authors how to implement this before we have several drivers that all implement their own ideas and getting this in a consistent state is another pain? (My be
Re: [PATCH v8 4/8] dt-bindings: pwm: Support new PWM_USAGE_POWER flag
Hello, On Mon, Apr 12, 2021 at 06:46:51PM +0200, Clemens Gruber wrote: > On Mon, Apr 12, 2021 at 06:27:23PM +0200, Uwe Kleine-König wrote: > > On Mon, Apr 12, 2021 at 03:27:41PM +0200, Clemens Gruber wrote: > > > Add the flag and corresponding documentation for PWM_USAGE_POWER. > > > > My concern here in the previous round was that PWM_USAGE_POWER isn't a > > name that intuitively suggests its semantic. Do you disagree? > > No. It is more abstract and requires documentation. But I also didn't > want to waste too much time on discussing names, so I used Thierry's > suggestion. If you introduce API thinking about the name before actually introducing it is a good idea in general. (OK, the name doesn't become part of the (binary) dt API, but we don't even agree about its semantic here.) And IMHO a bad name with a good documentation isn't good enough. Otherwise we can better just agree on using plain numbers in the .dts files. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v8 6/8] pwm: pca9685: Support new PWM_USAGE_POWER flag
Hi Clemens,
On Mon, Apr 12, 2021 at 07:11:58PM +0200, Clemens Gruber wrote:
> On Mon, Apr 12, 2021 at 06:30:45PM +0200, Uwe Kleine-König wrote:
> > On Mon, Apr 12, 2021 at 03:27:43PM +0200, Clemens Gruber wrote:
> > > static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int
> > > channel)
> > > {
> > > - unsigned int off_h = 0, val = 0;
> > > + struct pwm_device *pwm = >chip.pwms[channel];
> > > + unsigned int off = 0, on = 0, val = 0;
> > >
> > > if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
> > > /* HW does not support reading state of "all LEDs" channel */
> > > return 0;
> > > }
> > >
> > > - regmap_read(pca->regmap, LED_N_OFF_H(channel), _h);
> > > - if (off_h & LED_FULL) {
> > > + regmap_read(pca->regmap, LED_N_OFF_H(channel), );
> > > + if (off & LED_FULL) {
> > > /* Full OFF bit is set */
> > > return 0;
> > > }
> > >
> > > - regmap_read(pca->regmap, LED_N_ON_H(channel), );
> > > - if (val & LED_FULL) {
> > > + regmap_read(pca->regmap, LED_N_ON_H(channel), );
> > > + if (on & LED_FULL) {
> > > /* Full ON bit is set */
> > > return PCA9685_COUNTER_RANGE;
> > > }
> > >
> > > - val = 0;
> > > regmap_read(pca->regmap, LED_N_OFF_L(channel), );
> > > - return ((off_h & 0xf) << 8) | (val & 0xff);
> > > + off = ((off & 0xf) << 8) | (val & 0xff);
> > > + if (!pwm->args.usage_power)
> > > + return off;
> > > +
> > > + /* Read ON register to calculate duty cycle of staggered output */
> > > + val = 0;
> > > + regmap_read(pca->regmap, LED_N_ON_L(channel), );
> > > + on = ((on & 0xf) << 8) | (val & 0xff);
> > > + return (off - on) & (PCA9685_COUNTER_RANGE - 1);
> >
> > If LED_N_ON is != 0 but usage_power is false, the returned state is
> > bogus.
>
> If usage_power is false, LED_N_ON is guaranteed to be 0. It is reset to
> 0 in probe and never changed. Or did I miss something?
Ah right, so my concern is only a challenge once the reset in probe goes
away.
> > > }
> > >
> > > #if IS_ENABLED(CONFIG_GPIOLIB)
> > > @@ -439,9 +469,11 @@ static int pca9685_pwm_probe(struct i2c_client
> > > *client,
> > > reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
> > > regmap_write(pca->regmap, PCA9685_MODE1, reg);
> > >
> > > - /* Reset OFF registers to POR default */
> > > + /* Reset OFF/ON registers to POR default */
> > > regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, LED_FULL);
> > > regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);
> > > + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
> > > + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
> > >
> > > pca->chip.ops = _pwm_ops;
> > > /* Add an extra channel for ALL_LED */
> > > @@ -450,6 +482,9 @@ static int pca9685_pwm_probe(struct i2c_client
> > > *client,
> > > pca->chip.dev = >dev;
> > > pca->chip.base = -1;
> > >
> > > + pca->chip.of_xlate = of_pwm_xlate_with_flags;
> > > + pca->chip.of_pwm_n_cells = 3;
> > > +
> >
> > Huh, you're incompatibly changing the device tree binding here.
>
> No, I don't think so:
>
> The third cell is optional with of_pwm_xlate_with_flags.
> So previous DTs with pwm-cells = <2> will still work.
I thought that .of_pwm_n_cells was enforced, let me check the code ... I
had in mind that of_pwm_get() enforced that, but I cannot find it, so I
guess you're right and my concern is unjustified.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v8 1/8] pwm: pca9685: Switch to atomic API
Hello Clemens, On Mon, Apr 12, 2021 at 06:39:28PM +0200, Clemens Gruber wrote: > On Mon, Apr 12, 2021 at 06:18:08PM +0200, Uwe Kleine-König wrote: > > On Mon, Apr 12, 2021 at 03:27:38PM +0200, Clemens Gruber wrote: > > > The switch to the atomic API goes hand in hand with a few fixes to > > > previously experienced issues: > > > - The duty cycle is no longer lost after disable/enable (previously the > > > OFF registers were cleared in disable and the user was required to > > > call config to restore the duty cycle settings) > > > - If one sets a period resulting in the same prescale register value, > > > the sleep and write to the register is now skipped > > > - Previously, only the full ON bit was toggled in GPIO mode (and full > > > OFF cleared if set to high), which could result in both full OFF and > > > full ON not being set and on=0, off=0, which is not allowed according > > > to the datasheet > > > - The OFF registers were reset to 0 in probe, which could lead to the > > > forbidden on=0, off=0. Fixed by resetting to POR default (full OFF) > > > > > > Signed-off-by: Clemens Gruber > > > --- > > > Changes since v7: > > > - Moved check for !state->enabled before prescaler configuration > > > - Removed unnecessary cast > > > - Use DIV_ROUND_DOWN in .apply > > > > > > Changes since v6: > > > - Order of a comparison switched for improved readability > > > > > > Changes since v5: > > > - Function documentation for set_duty > > > - Variable initializations > > > - Print warning if all LEDs channel > > > - Changed EOPNOTSUPP to EINVAL > > > - Improved error messages > > > - Register reset corrections moved to this patch > > > > > > Changes since v4: > > > - Patches split up > > > - Use a single set_duty function > > > - Improve readability / new macros > > > - Added a patch to restrict prescale changes to the first user > > > > > > Changes since v3: > > > - Refactoring: Extracted common functions > > > - Read prescale register value instead of caching it > > > - Return all zeros and disabled for "all LEDs" channel state > > > - Improved duty calculation / mapping to 0..4096 > > > > > > Changes since v2: > > > - Always set default prescale value in probe > > > - Simplified probe code > > > - Inlined functions with one callsite > > > > > > Changes since v1: > > > - Fixed a logic error > > > - Impoved PM runtime handling and fixed !CONFIG_PM > > > - Write default prescale reg value if invalid in probe > > > - Reuse full_off/_on functions throughout driver > > > - Use cached prescale value whenever possible > > > > > > drivers/pwm/pwm-pca9685.c | 259 +- > > > 1 file changed, 89 insertions(+), 170 deletions(-) > > > > > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c > > > index 4a55dc18656c..827b57ced3c2 100644 > > > --- a/drivers/pwm/pwm-pca9685.c > > > +++ b/drivers/pwm/pwm-pca9685.c > > > @@ -51,7 +51,6 @@ > > > #define PCA9685_PRESCALE_MAX 0xFF/* => min. frequency of 24 Hz */ > > > > > > #define PCA9685_COUNTER_RANGE4096 > > > -#define PCA9685_DEFAULT_PERIOD 500 /* Default period_ns = 1/200 Hz > > > */ > > > #define PCA9685_OSC_CLOCK_MHZ25 /* Internal oscillator with 25 > > > MHz */ > > > > > > #define PCA9685_NUMREGS 0xFF > > > @@ -71,10 +70,14 @@ > > > #define LED_N_OFF_H(N) (PCA9685_LEDX_OFF_H + (4 * (N))) > > > #define LED_N_OFF_L(N) (PCA9685_LEDX_OFF_L + (4 * (N))) > > > > > > +#define REG_ON_H(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_H > > > : LED_N_ON_H((C))) > > > +#define REG_ON_L(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_L > > > : LED_N_ON_L((C))) > > > +#define REG_OFF_H(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_H > > > : LED_N_OFF_H((C))) > > > +#define REG_OFF_L(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_L > > > : LED_N_OFF_L((C))) > > > > I'd like to see these named PCA9685_REG_ON_H etc. > > I did not use the prefix because the existing LED_N_ON/OFF_H/L also do > not have a prefix. If the prefix is mandatory, I think LED_N_.. should > also be prefixed, right? I'd like to seem the prefixed (a
Re: [PATCH v8 6/8] pwm: pca9685: Support new PWM_USAGE_POWER flag
> #if IS_ENABLED(CONFIG_GPIOLIB) > @@ -439,9 +469,11 @@ static int pca9685_pwm_probe(struct i2c_client *client, > reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3); > regmap_write(pca->regmap, PCA9685_MODE1, reg); > > - /* Reset OFF registers to POR default */ > + /* Reset OFF/ON registers to POR default */ > regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, LED_FULL); > regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL); > + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0); > + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0); > > pca->chip.ops = _pwm_ops; > /* Add an extra channel for ALL_LED */ > @@ -450,6 +482,9 @@ static int pca9685_pwm_probe(struct i2c_client *client, > pca->chip.dev = >dev; > pca->chip.base = -1; > > + pca->chip.of_xlate = of_pwm_xlate_with_flags; > + pca->chip.of_pwm_n_cells = 3; > + Huh, you're incompatibly changing the device tree binding here. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v8 4/8] dt-bindings: pwm: Support new PWM_USAGE_POWER flag
On Mon, Apr 12, 2021 at 03:27:41PM +0200, Clemens Gruber wrote: > Add the flag and corresponding documentation for PWM_USAGE_POWER. My concern here in the previous round was that PWM_USAGE_POWER isn't a name that intuitively suggests its semantic. Do you disagree? > Cc: Rob Herring > Signed-off-by: Clemens Gruber > --- > Documentation/devicetree/bindings/pwm/pwm.txt | 3 +++ > include/dt-bindings/pwm/pwm.h | 1 + > 2 files changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt > b/Documentation/devicetree/bindings/pwm/pwm.txt > index 084886bd721e..fe3a28f887c0 100644 > --- a/Documentation/devicetree/bindings/pwm/pwm.txt > +++ b/Documentation/devicetree/bindings/pwm/pwm.txt > @@ -46,6 +46,9 @@ period in nanoseconds. > Optionally, the pwm-specifier can encode a number of flags (defined in > ) in a third cell: > - PWM_POLARITY_INVERTED: invert the PWM signal polarity > +- PWM_USAGE_POWER: Only care about the power output of the signal. This > + allows drivers (if supported) to optimize the signals, for example to > + improve EMI and reduce current spikes. IMHO there are too many open questions about which freedom this gives to the lowlevel driver. If the consumer requests .duty_cycle = 25ns + .period = 100ns, can the driver provide .duty_cycle = 25s + .period = 100s which nominally has the same power output? Let's not introduce more ambiguity than there already is. This is a NAck. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v8 2/8] pwm: pca9685: Support hardware readout
Hello Clemens, On Mon, Apr 12, 2021 at 03:27:39PM +0200, Clemens Gruber wrote: > Implement .get_state to read-out the current hardware state. > > The hardware readout may return slightly different values than those > that were set in apply due to the limited range of possible prescale and > counter register values. > > Also note that although the datasheet mentions 200 Hz as default > frequency when using the internal 25 MHz oscillator, the calculated > period from the default prescaler register setting of 30 is 5079040ns. > > Signed-off-by: Clemens Gruber Reviewed-by: Uwe Kleine-König Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v8 1/8] pwm: pca9685: Switch to atomic API
its(pca->regmap, reg, LED_FULL, 0x0); > + /* Wake the chip up */ > + pca9685_set_sleep_mode(pca, false); > + } > > + duty = PCA9685_COUNTER_RANGE * state->duty_cycle; > + duty = DIV_ROUND_DOWN_ULL(duty, state->period); If you round down here you should probably also round down in the calculation of prescale. Also note that you're losing precision by using state->period. Consider the following request: state->period = 4177921 [ns] + state->duty_cycle = 10 [ns], then we get PRESCALE = round(25 * state->period / 4096000) - 1 = 25 and so an actual period of 4096000 / 25 * (25 + 1) = 4259840 [ns]. If you now calculate the duty using 4096 * 10 / 4177920 = 98, this corresponds to an actual duty cycle of 98 * 4259840 / 4096 = 101920 ns while you should actually configure 96 to get 99840 ns. So in the end I'd like to have the following: PRESCALE = round-down(25 * state->period / 4096000) - 1 (to get the biggest period not bigger than state->period) and duty = round-down(state->duty_cycle * 25 / ((PRESCALE + 1) * 1000)) (to get the biggest duty cycle not bigger than state->duty_cycle). With the example above this yields PRESCALE = 24 duty = 100 which results in .period = 4096000 / 25 * 25 = 4096000 [ns] .duty_cycle = 10 [ns] Now you have a mixture of old and new with no consistent behaviour. So please either stick to the old behaviour or do it right immediately. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH 1/4] dt-bindings: Add bindings for aspeed pwm-tach.
Hello, On Mon, Apr 12, 2021 at 05:54:54PM +0800, Billy Tsai wrote: > + - Billy Tsai I object because the MTA at aspeedtech.com doesn't know this email address. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH 3/4] pwm: Add Aspeed ast2600 PWM support
aspeed_set_pwm_freq(priv, pwm, freq);
> + aspeed_set_pwm_duty(priv, pwm, duty_pt);
> + aspeed_set_pwm_polarity(priv, pwm, state->polarity);
How does the hardware behave between these calls? E.g. can it happen
that it already emits a normal period when inversed polarity is
requested just before aspeed_set_pwm_polarity is called? Or there is a
period with the previous duty cycle and the new period?
> + } else {
> + aspeed_set_pwm_duty(priv, pwm, 0);
> + }
> + cur_state->period = state->period;
> + cur_state->duty_cycle = state->duty_cycle;
> + cur_state->polarity = state->polarity;
> + cur_state->enabled = state->enabled;
The driver is not supposed to modify pwm->state.
> + return 0;
> +}
From your code I understood: The period of the signal is
(PWM_PERIOD + 1) * (2 ** DIV_H) * (DIV_L + 1) / 200 MHz
. The duty cycle is
PWM_FALLING_POINT * (2 ** DIV_H) * (DIV_L + 1) / 200 MHz
. So the PWM cannot provide a 100% relative duty cycle.
Is this right?
> +static const struct pwm_ops aspeed_pwm_ops = {
> + .request = aspeed_pwm_request,
> + .free = aspeed_pwm_free,
> + .apply = aspeed_pwm_apply,
Please test your driver with PWM_DEBUG enabled.
> + .owner = THIS_MODULE,
> +};
> +
> +static int aspeed_pwm_probe(struct platform_device *pdev)
> +{
> + struct device *dev = >dev;
> + struct clk *clk;
> + int ret;
> + struct aspeed_pwm_data *priv;
> + struct device_node *np;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + np = pdev->dev.parent->of_node;
> + if (!of_device_is_compatible(np, "aspeed,ast2600-pwm-tach")) {
> + dev_err(dev, "unsupported pwm device binding\n");
> + return -ENODEV;
> + }
Is this pwm-tach an mfd?
> + priv->regmap = syscon_node_to_regmap(np);
> + if (IS_ERR(priv->regmap)) {
> + dev_err(dev, "Couldn't get regmap\n");
> + return -ENODEV;
> + }
> +
> + clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(clk))
> + return -ENODEV;
> + priv->clk_freq = clk_get_rate(clk);
If you intend to use this clock, you have to enable it.
> + priv->reset = reset_control_get_shared(>dev, NULL);
> + if (IS_ERR(priv->reset)) {
> + dev_err(>dev, "can't get aspeed_pwm_tacho reset\n");
> + return PTR_ERR(priv->reset);
> + }
> + reset_control_deassert(priv->reset);
missing error checking
> + priv->chip.dev = dev;
> + priv->chip.ops = _pwm_ops;
> + priv->chip.base = -1;
This isn't necessary since f9a8ee8c8bcd118e800d88772c6457381db45224,
please drop the assignment to base.
> + priv->chip.npwm = ASPEED_NR_PWMS;
> + priv->chip.of_xlate = of_pwm_xlate_with_flags;
> + priv->chip.of_pwm_n_cells = 3;
> +
> + ret = pwmchip_add(>chip);
> + if (ret < 0) {
> + dev_err(>dev, "failed to add PWM chip: %d\n", ret);
Please use %pe to make the error messages better readable.
> + return ret;
> + }
> + dev_set_drvdata(dev, priv);
> + return ret;
> +}
> +
> +static const struct of_device_id of_pwm_match_table[] = {
> + {
> + .compatible = "aspeed,ast2600-pwm",
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, of_pwm_match_table);
> +
> +static struct platform_driver aspeed_pwm_driver = {
> + .probe = aspeed_pwm_probe,
Please implement a .remove callback.
> + .driver = {
> + .name = "aspeed_pwm",
> + .of_match_table = of_pwm_match_table,
> + },
> +};
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH 4/4] pwm: Add support for aspeed pwm controller
Hello, On Mon, Apr 12, 2021 at 05:54:57PM +0800, Billy Tsai wrote: > Add support for the pwm controller which can be found at aspeed ast2600 > soc. This driver is part function of multi-funciton of device "pwm-tach > controller". please squash this into patch 3. > Signed-off-by: Billy Tsai > --- > drivers/pwm/Kconfig | 6 ++ > drivers/pwm/Makefile | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 63be5362fd3a..947ed642debe 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -42,6 +42,12 @@ config PWM_DEBUG > It is expected to introduce some runtime overhead and diagnostic > output to the kernel log, so only enable while working on a driver. > > +config PWM_ASPEED_G6 > + tristate "ASPEEDG6 PWM support" No depends? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
On Mon, Apr 12, 2021 at 11:55:36AM +0900, Nobuhiro Iwamatsu wrote:
> Hi Uwe,
>
> Thanks for your review.
>
> On Sat, Apr 10, 2021 at 03:53:21PM +0200, Uwe Kleine-König wrote:
> > Hello,
> >
> > just a few small details left to criticize ...
> >
> > On Sat, Apr 10, 2021 at 08:08:37AM +0900, Nobuhiro Iwamatsu wrote:
> > > diff --git a/drivers/pwm/pwm-visconti.c b/drivers/pwm/pwm-visconti.c
> > > new file mode 100644
> > > index ..99d83f94ed86
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-visconti.c
> > > @@ -0,0 +1,188 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Toshiba Visconti pulse-width-modulation controller driver
> > > + *
> > > + * Copyright (c) 2020 TOSHIBA CORPORATION
> > > + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation
> > > + *
> > > + * Authors: Nobuhiro Iwamatsu
> > > + *
> > > + */
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +#define PIPGM_PCSR(ch) (0x400 + 4 * (ch))
> > > +#define PIPGM_PDUT(ch) (0x420 + 4 * (ch))
> > > +#define PIPGM_PWMC(ch) (0x440 + 4 * (ch))
> > > +
> > > +#define PIPGM_PWMC_PWMACTBIT(5)
> > > +#define PIPGM_PWMC_CLK_MASK GENMASK(1, 0)
> > > +#define PIPGM_PWMC_POLARITY_MASK GENMASK(5, 5)
> > > +
> > > +struct visconti_pwm_chip {
> > > + struct pwm_chip chip;
> > > + void __iomem *base;
> > > +};
> > > +
> > > +static inline struct visconti_pwm_chip *to_visconti_chip(struct pwm_chip
> > > *chip)
> > > +{
> > > + return container_of(chip, struct visconti_pwm_chip, chip);
> > > +}
> > > +
> > > +static int visconti_pwm_apply(struct pwm_chip *chip, struct pwm_device
> > > *pwm,
> > > + const struct pwm_state *state)
> > > +{
> > > + struct visconti_pwm_chip *priv = to_visconti_chip(chip);
> > > + u32 period, duty_cycle, pwmc0;
> > > +
> > > + /*
> > > + * pwmc is a 2-bit divider for the input clock running at 1 MHz.
> > > + * When the settings of the PWM are modified, the new values are
> > > shadowed in hardware until
> > > + * the period register (PCSR) is written and the currently running
> > > period is completed. This
> > > + * way the hardware switches atomically from the old setting to the new.
> > > + * Also, disabling the hardware completes the currently running period
> > > and keeps the output
> > > + * at low level at all times.
> >
> > Can you please put a paragraph analogous to the one in pwm-sifive in the
> > same format. This simplified keeping an overview about the oddities of
> > the various supported chips.
>
> OK, I will check pwm-sifive's, and add.
>
> >
> > > + */
> > > + if (!state->enabled) {
> > > + writel(0, priv->base + PIPGM_PCSR(pwm->hwpwm));
> > > + return 0;
> > > + }
> > > +
> > > [...]
> > > +
> > > +static void visconti_pwm_get_state(struct pwm_chip *chip, struct
> > > pwm_device *pwm,
> > > +struct pwm_state *state)
> > > +{
> > > + struct visconti_pwm_chip *priv = chip_to_priv(chip);
> > > + u32 period, duty, pwmc0, pwmc0_clk;
> > > +
> > > + period = readl(priv->base + PIPGM_PCSR(pwm->hwpwm));
> > > + if (period)
> > > + state->enabled = true;
> > > + else
> > > + state->enabled = false;
> >
> > If PIPGM_PCSR is 0 the hardware is still active and setting a new
> > configuration completes the currently running period, right? Then I
> > think having always state->enabled = true is a better match.
>
> If PIPGM_PCSR is 0, the hardware is stopped. Even if the settings of
> other registers are written, the values of other registers will not work
> unless PIPGM_PCSR is written.
Correct me if I'm wrong, but how I understand it, PCSR is special as the
other registers are shadow until PCSR is written. (And that is
irrespective of which value is active in PCSR or what is written to
PCSR.)
> However, as a logic, if PIPGM_PCSR becomes 0, it is only
> visconti_pwm_apply () in this driver,
> so I think that this process should always be state-> enabled = true
> as you pointed out.
> I wil drop
Re: [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
Hello, one more comment: On Sat, Apr 10, 2021 at 08:08:37AM +0900, Nobuhiro Iwamatsu wrote: > +static inline struct visconti_pwm_chip *to_visconti_chip(struct pwm_chip > *chip) all functions but this one start have the common prefix "visconti_pwm_". I like the concept of a common prefix and so you could rename this function to visconti_pwm_from_chip or similar. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v7 4/8] dt-bindings: pwm: Support new PWM_STAGGERING_ALLOWED flag
Hello Rob, On Tue, Apr 06, 2021 at 06:41:36PM +0200, Clemens Gruber wrote: > Add the flag and corresponding documentation for the new PWM staggering > mode feature. > > Cc: Rob Herring For now reviewing this patch is not necessary, we're discussing a better name for this flag. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v7 4/8] dt-bindings: pwm: Support new PWM_STAGGERING_ALLOWED flag
Hello Thierry, On Fri, Apr 09, 2021 at 02:27:34PM +0200, Thierry Reding wrote: > On Wed, Apr 07, 2021 at 07:33:57AM +0200, Uwe Kleine-König wrote: > > On Tue, Apr 06, 2021 at 06:41:36PM +0200, Clemens Gruber wrote: > > > Add the flag and corresponding documentation for the new PWM staggering > > > mode feature. > > > > > > Cc: Rob Herring > > > Signed-off-by: Clemens Gruber > > > > For the record, I don't like this and still prefer to make this > > staggering explicit for the consumer by expanding struct pwm_state with > > an .offset member to shift the active phase in the period. > > How are consumers supposed to know which offset to choose? And worse: > how should consumers even know that the driver supports phase shifts? I'm aware that we're a long way from being able to use this. The clean approach would be to get the offset from the device tree in the same way as the period. And in the meantime I agree that introducing a flag that allows to shift the active part in the period is a sane idea. So I suggest we concentrate on getting the details in the corresponding discussion straight. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v3 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
Hello,
On Sat, Apr 10, 2021 at 06:34:55AM +0900, Nobuhiro Iwamatsu wrote:
> > > +static int visconti_pwm_remove(struct platform_device *pdev)
> > > +{
> > > + struct visconti_pwm_chip *priv = platform_get_drvdata(pdev);
> > > +
> > > + return pwmchip_remove(>chip);
> >
> > I think Uwe would prefer this to be done separately because he's working
> > towards removing the return value from pwmchip_remove() and if we start
> > ignoring it in new drivers that will make life easier going forward.
> >
> > So this should just be:
> >
> > pwmchip_remove(>chip);
> >
> > return 0;
>
> I understand your suggestion.
> However, it looks like the pwmchip_remove() hasn't been updated yet.
> I will wait for the update of pwmchip_remove.
pwmchip_remove will always return 0 since b2c200e3f2fd which is in v5.3.
So Thierry's suggestion is safe and indeed welcome.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
Hello,
just a few small details left to criticize ...
On Sat, Apr 10, 2021 at 08:08:37AM +0900, Nobuhiro Iwamatsu wrote:
> diff --git a/drivers/pwm/pwm-visconti.c b/drivers/pwm/pwm-visconti.c
> new file mode 100644
> index ..99d83f94ed86
> --- /dev/null
> +++ b/drivers/pwm/pwm-visconti.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Toshiba Visconti pulse-width-modulation controller driver
> + *
> + * Copyright (c) 2020 TOSHIBA CORPORATION
> + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation
> + *
> + * Authors: Nobuhiro Iwamatsu
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define PIPGM_PCSR(ch) (0x400 + 4 * (ch))
> +#define PIPGM_PDUT(ch) (0x420 + 4 * (ch))
> +#define PIPGM_PWMC(ch) (0x440 + 4 * (ch))
> +
> +#define PIPGM_PWMC_PWMACTBIT(5)
> +#define PIPGM_PWMC_CLK_MASK GENMASK(1, 0)
> +#define PIPGM_PWMC_POLARITY_MASK GENMASK(5, 5)
> +
> +struct visconti_pwm_chip {
> + struct pwm_chip chip;
> + void __iomem *base;
> +};
> +
> +static inline struct visconti_pwm_chip *to_visconti_chip(struct pwm_chip
> *chip)
> +{
> + return container_of(chip, struct visconti_pwm_chip, chip);
> +}
> +
> +static int visconti_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct visconti_pwm_chip *priv = to_visconti_chip(chip);
> + u32 period, duty_cycle, pwmc0;
> +
> + /*
> + * pwmc is a 2-bit divider for the input clock running at 1 MHz.
> + * When the settings of the PWM are modified, the new values are
> shadowed in hardware until
> + * the period register (PCSR) is written and the currently running
> period is completed. This
> + * way the hardware switches atomically from the old setting to the new.
> + * Also, disabling the hardware completes the currently running period
> and keeps the output
> + * at low level at all times.
Can you please put a paragraph analogous to the one in pwm-sifive in the
same format. This simplified keeping an overview about the oddities of
the various supported chips.
> + */
> + if (!state->enabled) {
> + writel(0, priv->base + PIPGM_PCSR(pwm->hwpwm));
> + return 0;
> + }
> +
> [...]
> +
> +static void visconti_pwm_get_state(struct pwm_chip *chip, struct pwm_device
> *pwm,
> +struct pwm_state *state)
> +{
> + struct visconti_pwm_chip *priv = chip_to_priv(chip);
> + u32 period, duty, pwmc0, pwmc0_clk;
> +
> + period = readl(priv->base + PIPGM_PCSR(pwm->hwpwm));
> + if (period)
> + state->enabled = true;
> + else
> + state->enabled = false;
If PIPGM_PCSR is 0 the hardware is still active and setting a new
configuration completes the currently running period, right? Then I
think having always state->enabled = true is a better match.
> + duty = readl(priv->base + PIPGM_PDUT(pwm->hwpwm));
> + pwmc0 = readl(priv->base + PIPGM_PWMC(pwm->hwpwm));
> + pwmc0_clk = pwmc0 & PIPGM_PWMC_CLK_MASK;
> +
> + state->period = (period << pwmc0_clk) * NSEC_PER_USEC;
> + state->duty_cycle = (duty << pwmc0_clk) * NSEC_PER_USEC;
> + if (pwmc0 & PIPGM_PWMC_POLARITY_MASK)
> + state->polarity = PWM_POLARITY_INVERSED;
> + else
> + state->polarity = PWM_POLARITY_NORMAL;
> +}
> +
> [...]
> +
> +static int visconti_pwm_probe(struct platform_device *pdev)
> +{
> + struct device *dev = >dev;
> + struct visconti_pwm_chip *priv;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(priv->base))
> + return PTR_ERR(priv->base);
> +
> + platform_set_drvdata(pdev, priv);
> +
> + priv->chip.dev = dev;
> + priv->chip.ops = _pwm_ops;
> + priv->chip.npwm = 4;
> +
> + ret = pwmchip_add(>chip);
> + if (ret < 0)
> + return dev_err_probe(>dev, ret, "Cannot register visconti
> PWM\n");
Thierry told to have picked up my patch to add the function
devm_pwmchip_add. I just double checked and it didn't made it into his
for-next branch yet. When you respin this series please check if the
patch landed in the mean time and then make use of it.
> + return 0;
> +}
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [v3,PATCH 3/3] pwm: mtk_disp: implement .get_state()
On Fri, Apr 09, 2021 at 02:24:43PM +0200, Thierry Reding wrote:
> On Tue, Apr 06, 2021 at 12:27:56PM +0200, Uwe Kleine-König wrote:
> > On Tue, Apr 06, 2021 at 05:57:42PM +0800, Rex-BC Chen wrote:
> > > implement get_state function for pwm-mtk-disp
> > >
> > > Signed-off-by: Rex-BC Chen
> > > Signed-off-by: Jitao Shi
> >
> > Ideally you S-o-b line is the last one to show the order in which this
> > patch went from one person to another.
> >
> > > ---
> > > drivers/pwm/pwm-mtk-disp.c | 46 ++
> > > 1 file changed, 46 insertions(+)
> > >
> > > diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
> > > index 502228adf718..166e0a8ca703 100644
> > > --- a/drivers/pwm/pwm-mtk-disp.c
> > > +++ b/drivers/pwm/pwm-mtk-disp.c
> > > @@ -179,8 +179,54 @@ static int mtk_disp_pwm_apply(struct pwm_chip *chip,
> > > struct pwm_device *pwm,
> > > return mtk_disp_pwm_enable(chip, state);
> > > }
> > >
> > > +static void mtk_disp_pwm_get_state(struct pwm_chip *chip,
> > > +struct pwm_device *pwm,
> > > +struct pwm_state *state)
> > > +{
> > > + struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> > > + u32 clk_div, period, high_width, con0, con1;
> > > + u64 rate;
> > > + int err;
> > > +
> > > + err = clk_prepare_enable(mdp->clk_main);
> > > + if (err < 0) {
> > > + dev_err(chip->dev, "Can't enable mdp->clk_main: %d\n", err);
> > > + return;
> > > + }
> > > + err = clk_prepare_enable(mdp->clk_mm);
> > > + if (err < 0) {
> > > + dev_err(chip->dev, "Can't enable mdp->clk_mm: %d\n", err);
> > > + clk_disable_unprepare(mdp->clk_main);
> >
> > As before: %pe please
>
> According to the documentation %pe only works on pointers for which
> IS_ERR() is true, so I'm not sure it can be used with plain integer
> error codes.
It cannot.
> Looks like there's a bunch of drivers that will do %pe and then use
> ERR_PTR(err) to make this work, but to be honest, that seems like
> jumping through hoops.
When I suggested to implement %dE to print error codes this was shot
down by the printk guys who's position is that %pe has to be good enough
for everybody. And yes, you'd need to pass ERR_PTR(err) then.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag
Hello Thierry, On Fri, Apr 09, 2021 at 01:25:36PM +0200, Thierry Reding wrote: > On Thu, Apr 08, 2021 at 07:36:37PM +0200, Uwe Kleine-König wrote: > > On Thu, Apr 08, 2021 at 05:51:36PM +0200, Clemens Gruber wrote: > > > On Thu, Apr 08, 2021 at 02:50:40PM +0200, Thierry Reding wrote: > > > > Yes, I think that's basically what this is saying. I think we're perhaps > > > > getting hung up on the terminology here. PWM_STAGGERING_ALLOWED gives > > > > the impression that we're dealing with some provider-specific feature, > > > > whereas what we really want to express is that the PWM doesn't care > > > > exactly when the active cycle starts and based on that a provider that > > > > can support it may optimize the EMI behavior. > > > > > > > > Maybe we can find a better name for this? Ultimately what this means is > > > > that the consumer is primarily interested in the power output of the PWM > > > > rather than the exact shape of the signal. So perhaps something like > > > > PWM_USAGE_POWER would be more appropriate. > > > > > > Yes, although it would then no longer be obvious that this feature leads > > > to improved EMI behavior, as long as we mention that in the docs, I > > > think it's a good idea > > > > > > Maybe document it as follows? > > > PWM_USAGE_POWER - Allow the driver to delay the start of the cycle > > > for EMI improvements, as long as the power output stays the same > > > > I don't like both names, because for someone who is only halfway into > > PWM stuff it is not understandable. Maybe ALLOW_PHASE_SHIFT? > > Heh... how's that any more understandable? The questions that come to (my) mind when reading PWM_USAGE_POWER are: So the PWM is allowed to use some power? The PWM is used to provide a power source (like a regulator)? Has this something to do with a permission to use the PWM (the power to use it)? Please try googling for "usage power", and compare it with the results you get for "phase shift". > > When a consumer is only interested in the power output than > > > > .period = 20 > > .duty_cycle = 5 > > > > would also be an allowed response for the request > > > > .period = 200 > > .duty_cycle = 50 > > > > and this is not what is in the focus here. > > Actually, that's *exactly* what's important here. From a consumer point > of view the output power is the key in this case. OK, so if I understand you correctly, you want indeed allow .period = 200 .duty_cycle = 50 when .period = 20 .duty_cycle = 5 was requested? Do you want also allow .period = 2 + .duty_cycle = 5000? How would you limit what is allowed? I'd expect we don't want to allow .period = 200 + .duty_cycle = 50? What should a driver for a PWM backlight pass to pwm_apply_state if the PWM period should be between 400 ns and 1666 ns? (This comes from the first backlight datasheet I found where the valid range for the brightness input is 60 to 250Hz.) Maybe saying that only making the period smaller would be an idea; but the motor bridge I recently worked with[1] limits the PWM frequency to "up to 20 kHz", so this isn't an universally good idea either. [1] https://www.st.com/resource/en/datasheet/vnh5019a-e.pdf > The specifier is a description of a particular PWM in the consumer > context. And the consumer not going to care what exactly the PWM > controller might end up configuring to achieve best results. If the > controller allows the phase shift to be changed and the constraints > allow it, then that's great, but it isn't something that the consumer > has to know if all it wants is that the power output is as requested. Yes, if ALLOW_PHASE_SHIFT isn't what the consumer actually wants, they shouldn't use it. Agreed. > Put another way, the more generically we can describe the constraints > or use cases, the more flexibility we get for drivers to fulfill those > constraints. Yes, from the POV of a lowlevel driver the more general the better. From the POV of a consumer this isn't universally true, because the consumer might only accept a subset of the freedom this general flag gives to the lowlevel driver. > For example one controller might support phase shifting > and use that for PWM_USAGE_POWER for better EMI behaviour. But another > PWM controller may not support it. But it could perhaps want to > optimize the PWM signal by reversing the polarity of one channel or > whatever other mechanism there may be. > > If we add a flag such as ALLOW_PHASE_SHIFT, then only controllers that > support programmable phase shift will be able to support
Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag
On Thu, Apr 08, 2021 at 05:51:36PM +0200, Clemens Gruber wrote: > On Thu, Apr 08, 2021 at 02:50:40PM +0200, Thierry Reding wrote: > > Yes, I think that's basically what this is saying. I think we're perhaps > > getting hung up on the terminology here. PWM_STAGGERING_ALLOWED gives > > the impression that we're dealing with some provider-specific feature, > > whereas what we really want to express is that the PWM doesn't care > > exactly when the active cycle starts and based on that a provider that > > can support it may optimize the EMI behavior. > > > > Maybe we can find a better name for this? Ultimately what this means is > > that the consumer is primarily interested in the power output of the PWM > > rather than the exact shape of the signal. So perhaps something like > > PWM_USAGE_POWER would be more appropriate. > > Yes, although it would then no longer be obvious that this feature leads > to improved EMI behavior, as long as we mention that in the docs, I > think it's a good idea > > Maybe document it as follows? > PWM_USAGE_POWER - Allow the driver to delay the start of the cycle > for EMI improvements, as long as the power output stays the same I don't like both names, because for someone who is only halfway into PWM stuff it is not understandable. Maybe ALLOW_PHASE_SHIFT? When a consumer is only interested in the power output than .period = 20 .duty_cycle = 5 would also be an allowed response for the request .period = 200 .duty_cycle = 50 and this is not what is in the focus here. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v2 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
Hello Nobuhiro, On Thu, Apr 08, 2021 at 08:15:48AM +0900, Nobuhiro Iwamatsu wrote: > > > + /* > > > + * pwmc is a 2-bit divider for the input clock running at 1 MHz. > > > + * When the settings of the PWM are modified, the new values are > > > shadowed in hardware until > > > + * the period register (PCSR) is written and the currently running > > > period is completed. This > > > + * way the hardware switches atomically from the old setting to the new. > > > + * Also, disabling the hardware completes the currently running period > > > and keeps the output > > > + * at low level at all times. > > > > Did you just copy my optimal description or is your hardware really that > > nice? > > Yes, this hardware works as you wrote. > And I added about the state if the sinnal when this hardware disabled. > > > > > Do you know scripts/checkpatch.pl? I bet it will tell you to limit your > > lines to approx. 80 chars where sensible. > > Yes, I know. I ran scripts/checkpatch.pl before send patch. > I understand that the number of characters per line has been changed to > 100 characters. Does the pwm driver recommend 80 characters? For free-text comments I'd still recommend 80, yes. For code lines I'd be indeed more lax, as a line break in function calls reduces readability. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v7 8/8] pwm: pca9685: Add error messages for failed regmap calls
Hello Clemens, On Wed, Apr 07, 2021 at 10:47:45PM +0200, Clemens Gruber wrote: > On Wed, Apr 07, 2021 at 08:16:19AM +0200, Uwe Kleine-König wrote: > > You didn't check all return codes? How did you select the calls to > > check? > > No, because it would become a big mess and really obstruct readability > in my opinion. > > So I chose some kind of middleground: > I decided to check the first regmap_read and regmap_write in probe and > return the error code if something goes wrong there. > If something goes wrong after probe, I only print an error message. > > Is that acceptable? I wanted to have that in the commit log, but just noticed that I didn't read it carefully enough, it's already there. So if you change %d in the error messages to %pe I'm happy with this patch. Thanks Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v7 7/8] pwm: pca9685: Restrict period change for enabled PWMs
On Wed, Apr 07, 2021 at 10:41:27PM +0200, Clemens Gruber wrote:
> On Wed, Apr 07, 2021 at 08:12:29AM +0200, Uwe Kleine-König wrote:
> > On Tue, Apr 06, 2021 at 06:41:39PM +0200, Clemens Gruber wrote:
> > > Previously, the last used PWM channel could change the global prescale
> > > setting, even if other channels are already in use.
> > >
> > > Fix it by only allowing the first enabled PWM to change the global
> > > chip-wide prescale setting. If there is more than one channel in use,
> > > the prescale settings resulting from the chosen periods must match.
> > >
> > > GPIOs do not count as enabled PWMs as they are not using the prescaler
> > > and can't change it.
> > >
> > > Signed-off-by: Clemens Gruber
> > > ---
> > > Changes since v6:
> > > - Only allow the first PWM that is enabled to change the prescaler, not
> > > the first one that uses the prescaler
> > >
> > > drivers/pwm/pwm-pca9685.c | 66 +--
> > > 1 file changed, 56 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > > index 24221ee7a77a..cf0c98e4ef44 100644
> > > --- a/drivers/pwm/pwm-pca9685.c
> > > +++ b/drivers/pwm/pwm-pca9685.c
> > > @@ -23,11 +23,11 @@
> > > #include
> > >
> > > /*
> > > - * Because the PCA9685 has only one prescaler per chip, changing the
> > > period of
> > > - * one channel affects the period of all 16 PWM outputs!
> > > - * However, the ratio between each configured duty cycle and the
> > > chip-wide
> > > - * period remains constant, because the OFF time is set in proportion to
> > > the
> > > - * counter range.
> > > + * Because the PCA9685 has only one prescaler per chip, only the first
> > > channel
> > > + * that is enabled is allowed to change the prescale register.
> > > + * PWM channels requested afterwards must use a period that results in
> > > the same
> > > + * prescale setting as the one set by the first requested channel.
> > > + * GPIOs do not count as enabled PWMs as they are not using the
> > > prescaler.
> > > */
> > >
> > > #define PCA9685_MODE10x00
> > > @@ -78,8 +78,9 @@
> > > struct pca9685 {
> > > struct pwm_chip chip;
> > > struct regmap *regmap;
> > > -#if IS_ENABLED(CONFIG_GPIOLIB)
> > > struct mutex lock;
> > > + DECLARE_BITMAP(pwms_enabled, PCA9685_MAXCHAN + 1);
> > > +#if IS_ENABLED(CONFIG_GPIOLIB)
> > > struct gpio_chip gpio;
> > > DECLARE_BITMAP(pwms_inuse, PCA9685_MAXCHAN + 1);
> > > #endif
> > > @@ -90,6 +91,22 @@ static inline struct pca9685 *to_pca(struct pwm_chip
> > > *chip)
> > > return container_of(chip, struct pca9685, chip);
> > > }
> > >
> > > +/* This function is supposed to be called with the lock mutex held */
> > > +static bool pca9685_prescaler_can_change(struct pca9685 *pca, int
> > > channel)
> > > +{
> > > + /* No PWM enabled: Change allowed */
> > > + if (bitmap_empty(pca->pwms_enabled, PCA9685_MAXCHAN + 1))
> > > + return true;
> > > + /* More than one PWM enabled: Change not allowed */
> > > + if (bitmap_weight(pca->pwms_enabled, PCA9685_MAXCHAN + 1) > 1)
> > > + return false;
> > > + /*
> > > + * Only one PWM enabled: Change allowed if the PWM about to
> > > + * be changed is the one that is already enabled
> > > + */
> > > + return test_bit(channel, pca->pwms_enabled);
> >
> > Maybe this is a bit more effective?:
> >
> > DECLARE_BITMAP(blablub, PCA9685_MAXCHAN + 1);
> > bitmap_zero(blablub, PCA9685_MAXCHAN + 1);
> > bitmap_set(blablub, channel);
> > return bitmap_subset(pca->pwms_enabled, blablub);
>
> But if no PWM is enabled, it should return true, not false.
If no PWM is enabled we have pca->pwms_enabled = empty set which is a
subset of every set. So I'd expect this case to be handled just fine.
> > (but that's a minor issue, the suggested algorithm is correct.)
>
> I would prefer to keep it explicit because it is a little easier to
> follow and probably not worth optimizing.
I didn't find it hard to follow, but I'm willing to accept that this
isn't representative. I'm ok with keeping the code as is.
> I agree that it would be nice to drop the ALL channel support.
Maybe deprecate it using a config item? But no hurry.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag
On Wed, Apr 07, 2021 at 10:21:10PM +0200, Clemens Gruber wrote: > On Wed, Apr 07, 2021 at 07:46:58AM +0200, Uwe Kleine-König wrote: > > On Tue, Apr 06, 2021 at 06:41:37PM +0200, Clemens Gruber wrote: > > > If the flag PWM_STAGGERING_ALLOWED is set on a channel, the PWM driver > > > may (if supported by the HW) delay the ON time of the channel relative > > > to the channel number. > > > This does not alter the duty cycle ratio and is only relevant for PWM > > > chips with less prescalers than channels, which would otherwise assert > > > multiple or even all enabled channels at the same time. > > > > > > If this feature is supported by the driver and the flag is set on > > > multiple channels, their ON times are spread out to improve EMI and > > > reduce current spikes. > > > > As said in reply to patch 4/8 already: I don't like this idea and > > think this should be made explicit using a new offset member in struct > > pwm_state instead. That's because I think that the wave form a PWM > > generates should be (completely) defined by the consumer and not by a > > mix between consumer and device tree. Also the consumer has no (sane) > > way to determine if staggering is in use or not. > > I don't think offsets are ideal for this feature: It makes it more > cumbersome for the user, because he has to allocate the offsets > himself instead of a simple on/off switch. > The envisioned usecase is: "I want better EMI behavior and don't care > about the individual channels no longer being asserted at the exact same > time". The formal thing is: "I want better EMI behavior and don't care if periods start with the active phase, it might be anywhere, even over a period boundary." Being asserted at the exact same time is just a detail for the pca9685. > > One side effect (at least for the pca9685) is that when programming a > > new duty cycle it takes a bit longer than without staggering until the > > new setting is active. > > Yes, but it can be turned off if this is a problem, now even per-PWM. Yes and that is a good thing. (BTW: I'd call it per-PWM-consumer, but details.) > > Another objection I have is that we already have some technical debt > > because there are already two different types of drivers (.apply vs > > .config+.set_polarity+.enable+.disable) and I would like to unify this > > first before introducing new stuff. > > But there is already PWM_POLARITY_INVERTED, which can be set in the DT. > I am only adding another flag. I understand your reasoning, and similar to "This diplay backlight needs an inverted PWM (as a low duty-cycle results in a high brightness" this semantic "This consumer doesn't care if the active cycle is anywhere in the period". Hmm, maybe I just have to think about it a bit more to become friends with that thought. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v7 2/8] pwm: pca9685: Support hardware readout
On Wed, Apr 07, 2021 at 09:33:20AM +0200, Clemens Gruber wrote:
> On Wed, Apr 07, 2021 at 07:31:35AM +0200, Uwe Kleine-König wrote:
> > On Tue, Apr 06, 2021 at 06:41:34PM +0200, Clemens Gruber wrote:
> > > Implements .get_state to read-out the current hardware state.
> > >
> > > The hardware readout may return slightly different values than those
> > > that were set in apply due to the limited range of possible prescale and
> > > counter register values.
> > >
> > > Also note that although the datasheet mentions 200 Hz as default
> > > frequency when using the internal 25 MHz oscillator, the calculated
> > > period from the default prescaler register setting of 30 is 5079040ns.
> > >
> > > Signed-off-by: Clemens Gruber
> > > ---
> > > Changes since v6:
> > > - Added a comment regarding the division (Suggested by Uwe)
> > > - Rebased
> > >
> > > drivers/pwm/pwm-pca9685.c | 46 +++
> > > 1 file changed, 46 insertions(+)
> > >
> > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > > index 5a2ce97e71fd..d4474c5ff96f 100644
> > > --- a/drivers/pwm/pwm-pca9685.c
> > > +++ b/drivers/pwm/pwm-pca9685.c
> > > @@ -333,6 +333,51 @@ static int pca9685_pwm_apply(struct pwm_chip *chip,
> > > struct pwm_device *pwm,
> > > return 0;
> > > }
> > >
> > > +static void pca9685_pwm_get_state(struct pwm_chip *chip, struct
> > > pwm_device *pwm,
> > > + struct pwm_state *state)
> > > +{
> > > + struct pca9685 *pca = to_pca(chip);
> > > + unsigned long long duty;
> > > + unsigned int val = 0;
> > > +
> > > + /* Calculate (chip-wide) period from prescale value */
> > > + regmap_read(pca->regmap, PCA9685_PRESCALE, );
> > > + /*
> > > + * PCA9685_OSC_CLOCK_MHZ is 25, i.e. an integer divider of 1000.
> > > + * The following calculation is therefore only a multiplication
> > > + * and we are not losing precision.
> > > + */
> > > + state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
> > > + (val + 1);
> > > +
> > > + /* The (per-channel) polarity is fixed */
> > > + state->polarity = PWM_POLARITY_NORMAL;
> > > +
> > > + if (pwm->hwpwm >= PCA9685_MAXCHAN) {
> > > + /*
> > > + * The "all LEDs" channel does not support HW readout
> > > + * Return 0 and disabled for backwards compatibility
> > > + */
> > > + state->duty_cycle = 0;
> > > + state->enabled = false;
> > > + return;
> > > + }
> > > +
> > > + duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
> > > +
> > > + state->enabled = !!duty;
> > > + if (!state->enabled) {
> > > + state->duty_cycle = 0;
> > > + return;
> > > + } else if (duty == PCA9685_COUNTER_RANGE) {
> > > + state->duty_cycle = state->period;
> > > + return;
> > > + }
> > > +
> > > + duty *= state->period;
> > > + state->duty_cycle = duty / PCA9685_COUNTER_RANGE;
> >
> > Given that with duty = 0 the chip is still "on" and changing the duty
> > will first complete the currently running period, I'd model duty=0 as
> > enabled. This also simplifies the code a bit, to something like:
> >
> >
> > state->enabled = true;
> > duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
> > state->duty_cycle = div_round_up(duty * state->period,
> > PCA9685_COUNTER_RANGE);
> >
> > (I'm using round-up here assuming apply uses round-down to get
> > idempotency. In the current patch set state this is wrong however.)
>
> So, in your opinion, every requested PWM of the pca9685 should always be
> enabled by default (from the PWM core viewpoint) ?
>
> And this wouldn't break the following because pwm_get_state does not
> actually read out the hw state:
> pwm_get_state -> enabled=true duty=0
> pwm_apply_state -> enabled =false duty=0
> pwm_get_state -> enabled=false duty=0
I don't see any breakage here. Either there is none or I failed to grasp
where you see a problem.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v7 8/8] pwm: pca9685: Add error messages for failed regmap calls
On Tue, Apr 06, 2021 at 06:41:40PM +0200, Clemens Gruber wrote:
> Regmap operations can fail if the underlying subsystem is not working
> properly (e.g. hogged I2C bus, etc.)
> As this is useful information for the user, print an error message if it
> happens.
> Let probe fail if the first regmap_read or the first regmap_write fails.
>
> Signed-off-by: Clemens Gruber
> ---
> Changes since v6:
> - Rebased
>
> drivers/pwm/pwm-pca9685.c | 83 ---
> 1 file changed, 59 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index cf0c98e4ef44..8a4993882b40 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -107,6 +107,30 @@ static bool pca9685_prescaler_can_change(struct pca9685
> *pca, int channel)
> return test_bit(channel, pca->pwms_enabled);
> }
>
> +static int pca9685_read_reg(struct pca9685 *pca, unsigned int reg, unsigned
> int *val)
> +{
> + struct device *dev = pca->chip.dev;
> + int err;
> +
> + err = regmap_read(pca->regmap, reg, val);
> + if (err != 0)
> + dev_err(dev, "regmap_read of register 0x%x failed: %d\n", reg,
> err);
Please use %pe to emit the error code instead of %d.
> +
> + return err;
> +}
> +
> +static int pca9685_write_reg(struct pca9685 *pca, unsigned int reg, unsigned
> int val)
> +{
> + struct device *dev = pca->chip.dev;
> + int err;
> +
> + err = regmap_write(pca->regmap, reg, val);
> + if (err != 0)
> + dev_err(dev, "regmap_write to register 0x%x failed: %d\n", reg,
> err);
> +
> + return err;
> +}
> +
> /* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048
> -> 50%) */
> static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned
> int duty)
> {
> @@ -115,12 +139,12 @@ static void pca9685_pwm_set_duty(struct pca9685 *pca,
> int channel, unsigned int
>
> if (duty == 0) {
> /* Set the full OFF bit, which has the highest precedence */
> - regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
> + pca9685_write_reg(pca, REG_OFF_H(channel), LED_FULL);
You didn't check all return codes? How did you select the calls to
check?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v7 7/8] pwm: pca9685: Restrict period change for enabled PWMs
On Tue, Apr 06, 2021 at 06:41:39PM +0200, Clemens Gruber wrote:
> Previously, the last used PWM channel could change the global prescale
> setting, even if other channels are already in use.
>
> Fix it by only allowing the first enabled PWM to change the global
> chip-wide prescale setting. If there is more than one channel in use,
> the prescale settings resulting from the chosen periods must match.
>
> GPIOs do not count as enabled PWMs as they are not using the prescaler
> and can't change it.
>
> Signed-off-by: Clemens Gruber
> ---
> Changes since v6:
> - Only allow the first PWM that is enabled to change the prescaler, not
> the first one that uses the prescaler
>
> drivers/pwm/pwm-pca9685.c | 66 +--
> 1 file changed, 56 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 24221ee7a77a..cf0c98e4ef44 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -23,11 +23,11 @@
> #include
>
> /*
> - * Because the PCA9685 has only one prescaler per chip, changing the period
> of
> - * one channel affects the period of all 16 PWM outputs!
> - * However, the ratio between each configured duty cycle and the chip-wide
> - * period remains constant, because the OFF time is set in proportion to the
> - * counter range.
> + * Because the PCA9685 has only one prescaler per chip, only the first
> channel
> + * that is enabled is allowed to change the prescale register.
> + * PWM channels requested afterwards must use a period that results in the
> same
> + * prescale setting as the one set by the first requested channel.
> + * GPIOs do not count as enabled PWMs as they are not using the prescaler.
> */
>
> #define PCA9685_MODE10x00
> @@ -78,8 +78,9 @@
> struct pca9685 {
> struct pwm_chip chip;
> struct regmap *regmap;
> -#if IS_ENABLED(CONFIG_GPIOLIB)
> struct mutex lock;
> + DECLARE_BITMAP(pwms_enabled, PCA9685_MAXCHAN + 1);
> +#if IS_ENABLED(CONFIG_GPIOLIB)
> struct gpio_chip gpio;
> DECLARE_BITMAP(pwms_inuse, PCA9685_MAXCHAN + 1);
> #endif
> @@ -90,6 +91,22 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> return container_of(chip, struct pca9685, chip);
> }
>
> +/* This function is supposed to be called with the lock mutex held */
> +static bool pca9685_prescaler_can_change(struct pca9685 *pca, int channel)
> +{
> + /* No PWM enabled: Change allowed */
> + if (bitmap_empty(pca->pwms_enabled, PCA9685_MAXCHAN + 1))
> + return true;
> + /* More than one PWM enabled: Change not allowed */
> + if (bitmap_weight(pca->pwms_enabled, PCA9685_MAXCHAN + 1) > 1)
> + return false;
> + /*
> + * Only one PWM enabled: Change allowed if the PWM about to
> + * be changed is the one that is already enabled
> + */
> + return test_bit(channel, pca->pwms_enabled);
Maybe this is a bit more effective?:
DECLARE_BITMAP(blablub, PCA9685_MAXCHAN + 1);
bitmap_zero(blablub, PCA9685_MAXCHAN + 1);
bitmap_set(blablub, channel);
return bitmap_subset(pca->pwms_enabled, blablub);
(but that's a minor issue, the suggested algorithm is correct.)
So:
Acked-by: Uwe Kleine-König
(side-note: I wonder if the handling of the set-all channel is correct
here. But given that it is messy anyhow, (e.g. because setting some
state to this set-all channel doesn't influence pwm_get_state for the
individual channels) I don't object if there is another problem in this
corner case. IMHO just dropping this virtual channel would be nice.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag
On Tue, Apr 06, 2021 at 06:41:37PM +0200, Clemens Gruber wrote: > If the flag PWM_STAGGERING_ALLOWED is set on a channel, the PWM driver > may (if supported by the HW) delay the ON time of the channel relative > to the channel number. > This does not alter the duty cycle ratio and is only relevant for PWM > chips with less prescalers than channels, which would otherwise assert > multiple or even all enabled channels at the same time. > > If this feature is supported by the driver and the flag is set on > multiple channels, their ON times are spread out to improve EMI and > reduce current spikes. As said in reply to patch 4/8 already: I don't like this idea and think this should be made explicit using a new offset member in struct pwm_state instead. That's because I think that the wave form a PWM generates should be (completely) defined by the consumer and not by a mix between consumer and device tree. Also the consumer has no (sane) way to determine if staggering is in use or not. One side effect (at least for the pca9685) is that when programming a new duty cycle it takes a bit longer than without staggering until the new setting is active. Another objection I have is that we already have some technical debt because there are already two different types of drivers (.apply vs .config+.set_polarity+.enable+.disable) and I would like to unify this first before introducing new stuff. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v7 4/8] dt-bindings: pwm: Support new PWM_STAGGERING_ALLOWED flag
On Tue, Apr 06, 2021 at 06:41:36PM +0200, Clemens Gruber wrote: > Add the flag and corresponding documentation for the new PWM staggering > mode feature. > > Cc: Rob Herring > Signed-off-by: Clemens Gruber For the record, I don't like this and still prefer to make this staggering explicit for the consumer by expanding struct pwm_state with an .offset member to shift the active phase in the period. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v7 2/8] pwm: pca9685: Support hardware readout
On Tue, Apr 06, 2021 at 06:41:34PM +0200, Clemens Gruber wrote:
> Implements .get_state to read-out the current hardware state.
>
> The hardware readout may return slightly different values than those
> that were set in apply due to the limited range of possible prescale and
> counter register values.
>
> Also note that although the datasheet mentions 200 Hz as default
> frequency when using the internal 25 MHz oscillator, the calculated
> period from the default prescaler register setting of 30 is 5079040ns.
>
> Signed-off-by: Clemens Gruber
> ---
> Changes since v6:
> - Added a comment regarding the division (Suggested by Uwe)
> - Rebased
>
> drivers/pwm/pwm-pca9685.c | 46 +++
> 1 file changed, 46 insertions(+)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 5a2ce97e71fd..d4474c5ff96f 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -333,6 +333,51 @@ static int pca9685_pwm_apply(struct pwm_chip *chip,
> struct pwm_device *pwm,
> return 0;
> }
>
> +static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device
> *pwm,
> + struct pwm_state *state)
> +{
> + struct pca9685 *pca = to_pca(chip);
> + unsigned long long duty;
> + unsigned int val = 0;
> +
> + /* Calculate (chip-wide) period from prescale value */
> + regmap_read(pca->regmap, PCA9685_PRESCALE, );
> + /*
> + * PCA9685_OSC_CLOCK_MHZ is 25, i.e. an integer divider of 1000.
> + * The following calculation is therefore only a multiplication
> + * and we are not losing precision.
> + */
> + state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
> + (val + 1);
> +
> + /* The (per-channel) polarity is fixed */
> + state->polarity = PWM_POLARITY_NORMAL;
> +
> + if (pwm->hwpwm >= PCA9685_MAXCHAN) {
> + /*
> + * The "all LEDs" channel does not support HW readout
> + * Return 0 and disabled for backwards compatibility
> + */
> + state->duty_cycle = 0;
> + state->enabled = false;
> + return;
> + }
> +
> + duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
> +
> + state->enabled = !!duty;
> + if (!state->enabled) {
> + state->duty_cycle = 0;
> + return;
> + } else if (duty == PCA9685_COUNTER_RANGE) {
> + state->duty_cycle = state->period;
> + return;
> + }
> +
> + duty *= state->period;
> + state->duty_cycle = duty / PCA9685_COUNTER_RANGE;
Given that with duty = 0 the chip is still "on" and changing the duty
will first complete the currently running period, I'd model duty=0 as
enabled. This also simplifies the code a bit, to something like:
state->enabled = true;
duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
state->duty_cycle = div_round_up(duty * state->period,
PCA9685_COUNTER_RANGE);
(I'm using round-up here assuming apply uses round-down to get
idempotency. In the current patch set state this is wrong however.)
> +}
> +
> static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> {
> struct pca9685 *pca = to_pca(chip);
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v7 1/8] pwm: pca9685: Switch to atomic API
85_ALL_LED_OFF_H;
> - else
> - reg = LED_N_OFF_H(pwm->hwpwm);
> + /* Change the chip-wide output frequency */
> + regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale);
The cast isn't necessary, is it?
> - regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> + /* Wake the chip up */
> + pca9685_set_sleep_mode(pca, false);
> + }
>
> + pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
> return 0;
> }
>
> -static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device
> *pwm)
> -{
> - struct pca9685 *pca = to_pca(chip);
> - unsigned int reg;
> -
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_H;
> - else
> - reg = LED_N_OFF_H(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, LED_FULL);
> -
> - /* Clear the LED_OFF counter. */
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_L;
> - else
> - reg = LED_N_OFF_L(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, 0x0);
> -}
> -
> static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> {
> struct pca9685 *pca = to_pca(chip);
> @@ -422,15 +348,13 @@ static void pca9685_pwm_free(struct pwm_chip *chip,
> struct pwm_device *pwm)
> {
> struct pca9685 *pca = to_pca(chip);
>
> - pca9685_pwm_disable(chip, pwm);
> + pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> pm_runtime_put(chip->dev);
> pca9685_pwm_clear_inuse(pca, pwm->hwpwm);
> }
>
> static const struct pwm_ops pca9685_pwm_ops = {
> - .enable = pca9685_pwm_enable,
> - .disable = pca9685_pwm_disable,
> - .config = pca9685_pwm_config,
> + .apply = pca9685_pwm_apply,
> .request = pca9685_pwm_request,
> .free = pca9685_pwm_free,
> .owner = THIS_MODULE,
> @@ -461,7 +385,6 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> ret);
> return ret;
> }
> - pca->period_ns = PCA9685_DEFAULT_PERIOD;
>
> i2c_set_clientdata(client, pca);
>
> @@ -484,9 +407,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
> regmap_write(pca->regmap, PCA9685_MODE1, reg);
>
> - /* Clear all "full off" bits */
> - regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);
> - regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, 0);
> + /* Reset OFF registers to POR default */
> + regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, LED_FULL);
> + regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);
Is this hunk unrelated to the patch description?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout
Hello Thierry,
On Tue, Apr 06, 2021 at 03:47:15PM +0200, Thierry Reding wrote:
> On Tue, Apr 06, 2021 at 08:33:57AM +0200, Uwe Kleine-König wrote:
> > On Wed, Mar 31, 2021 at 05:52:45PM +0200, Thierry Reding wrote:
> > > On Wed, Mar 31, 2021 at 12:25:43PM +0200, Uwe Kleine-König wrote:
> > > > On Mon, Mar 22, 2021 at 09:34:21AM +0100, Thierry Reding wrote:
> > > > > On Mon, Jan 11, 2021 at 09:43:50PM +0100, Uwe Kleine-König wrote:
> > > > > > On Sun, Jan 03, 2021 at 06:04:10PM +0100, Clemens Gruber wrote:
> > > > > > > Another point is the period: Sven suggested we do not read out the
> > > > > > > period at all, as the PWM is disabled anyway (see above).
> > > > > > > Is this acceptable?
> > > > > >
> > > > > > In my eyes consumers should consider the period value as "don't
> > > > > > care" if
> > > > > > the PWM is off. But this doesn't match reality (and maybe also it
> > > > > > doesn't match Thierry's opinion). See for example the
> > > > > > drivers/video/backlight/pwm_bl.c driver which uses the idiom:
> > > > > >
> > > > > > pwm_get_state(mypwm, );
> > > > > > state.enabled = true;
> > > > > > pwm_apply_state(pb->pwm, );
> > > > > >
> > > > > > which breaks if .period is invalid. (OK, this isn't totally accurate
> > > > > > because currently the .get_state callback has only little to do with
> > > > > > pwm_get_state(), but you get the point.)
> > > > >
> > > > > The idea behind atomic states in the PWM API is to provide accurate
> > > > > snapshots of a PWM channel's settings. It's not a representation of
> > > > > the PWM channel's physical output, although in some cases they may
> > > > > be the same.
> > > >
> > > > I think the pwm_state returned by .get_state() should be an accurate
> > > > representation of the PWM channel's physical output.
> > >
> > > Yeah, like I said, in most cases that will be true. However, as
> > > mentioned below, if there's no 1:1 correspondence between the settings
> > > and what's actually coming out of the PWM, this isn't always possible.
> > > But yes, it should always be as accurate as possible.
> >
> > It should always be true, not only in most cases. For any given PWM
> > output you can provide a pwm_state that describes this output (unless
> > you'd need a value bigger than u64 to describe it or a higher precision
> > as pwm_state only uses integer values). So it's a 1:n relation: You
> > should always be able to provide a pwm_state and in some cases there are
> > more than one such states (and you should still provide one of them).
>
> My point is that the correspondence between the pwm_state and what's
> programmed to hardware can't always be read back to unambiguously give
> the same result. As you mentioned there are these cases where the PWM
> channel doesn't have a separate enable bit, in which case it must be
> emulated by setting the duty cycle to 0.
OK, I think we agree here.
> In such cases it doesn't make sense to always rely on ->get_state() to
> read back the logical state because it just can't.
If with "logical state" you mean the state that was just requested to be
programmed, then I agree here, too.
> How is it supposed to know from reading that 0 duty cycle whether that
> means the PWM is on or off? So for the initial read-out we can't do
> any better so we have to pick one. The easiest would probably be to
> just assume PWM disabled when duty cycle is 0 and in most cases that
> will be just fine as the resulting PWM will be the same regardless of
> which of the two options we pick.
If the current period is completed before a new setting is applied
returning .enabled = true always is probably the more accurate view. But
details.
> However, what I'm also saying is that once the consumer has called
> pwm_apply_state(), there's no reason to read back the value from
> hardware and potentially loose information about the state that isn't
> contained in hardware.
Yes, as long as the consumer is only interested in their requested
values and not what was actually implemented, that's true.
> If the consumer has applied this state:
>
> {
> .period = 100,
> .duty_cycle = 0,
> .polarity = PWM_POLARITY_NORMAL,
> .enabled = true,
> }
>
> then why would we want to have
Re: [PATCH] pwm: Rename pwm_get_state() to better reflect its semantic
Hello Thierry, On Tue, Apr 06, 2021 at 01:16:31PM +0200, Thierry Reding wrote: > On Tue, Apr 06, 2021 at 09:30:36AM +0200, Uwe Kleine-König wrote: > > Given that lowlevel drivers usually cannot implement exactly what a > > consumer requests with pwm_apply_state() there is some rounding involved. > > > > pwm_get_state() traditionally returned the setting that was requested most > > recently by the consumer (opposed to what was actually implemented in > > hardware in reply to the last request). To make this semantic obvious > > rename the function. > > > > Signed-off-by: Uwe Kleine-König > > --- > > Documentation/driver-api/pwm.rst | 6 +++- > > drivers/clk/clk-pwm.c | 2 +- > > drivers/gpu/drm/i915/display/intel_panel.c | 4 +-- > > drivers/input/misc/da7280.c| 2 +- > > drivers/input/misc/pwm-beeper.c| 2 +- > > drivers/input/misc/pwm-vibra.c | 4 +-- > > drivers/pwm/core.c | 4 +-- > > drivers/pwm/pwm-atmel-hlcdc.c | 2 +- > > drivers/pwm/pwm-atmel.c| 2 +- > > drivers/pwm/pwm-imx27.c| 2 +- > > drivers/pwm/pwm-rockchip.c | 2 +- > > drivers/pwm/pwm-stm32-lp.c | 4 +-- > > drivers/pwm/pwm-sun4i.c| 2 +- > > drivers/pwm/sysfs.c| 18 ++-- > > drivers/regulator/pwm-regulator.c | 4 +-- > > drivers/video/backlight/pwm_bl.c | 10 +++ > > include/linux/pwm.h| 34 ++ > > 17 files changed, 59 insertions(+), 45 deletions(-) > > Honestly, I don't think this is worth the churn. If you think people > will easily get confused by this then a better solution might be to more > explicitly document the pwm_get_state() function to say exactly what it > returns. I'm not so optimistic that people become aware of the semantic just because there is documentation describing it and I strongly believe that a good name for functions is more important than accurate documentation. If you don't agree, what do you think about the updated wording in Documentation/driver-api/pwm.rst? > But there's no need to make life difficult for everyone by > renaming this to something as cumbersome as this. I don't expect any merge conflicts (and if still a problem occurs resolving should be trivial enough). So I obviously don't agree to your weighing. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [v3,PATCH 3/3] pwm: mtk_disp: implement .get_state()
On Tue, Apr 06, 2021 at 05:57:42PM +0800, Rex-BC Chen wrote:
> implement get_state function for pwm-mtk-disp
>
> Signed-off-by: Rex-BC Chen
> Signed-off-by: Jitao Shi
Ideally you S-o-b line is the last one to show the order in which this
patch went from one person to another.
> ---
> drivers/pwm/pwm-mtk-disp.c | 46 ++
> 1 file changed, 46 insertions(+)
>
> diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
> index 502228adf718..166e0a8ca703 100644
> --- a/drivers/pwm/pwm-mtk-disp.c
> +++ b/drivers/pwm/pwm-mtk-disp.c
> @@ -179,8 +179,54 @@ static int mtk_disp_pwm_apply(struct pwm_chip *chip,
> struct pwm_device *pwm,
> return mtk_disp_pwm_enable(chip, state);
> }
>
> +static void mtk_disp_pwm_get_state(struct pwm_chip *chip,
> +struct pwm_device *pwm,
> +struct pwm_state *state)
> +{
> + struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> + u32 clk_div, period, high_width, con0, con1;
> + u64 rate;
> + int err;
> +
> + err = clk_prepare_enable(mdp->clk_main);
> + if (err < 0) {
> + dev_err(chip->dev, "Can't enable mdp->clk_main: %d\n", err);
> + return;
> + }
> + err = clk_prepare_enable(mdp->clk_mm);
> + if (err < 0) {
> + dev_err(chip->dev, "Can't enable mdp->clk_mm: %d\n", err);
> + clk_disable_unprepare(mdp->clk_main);
As before: %pe please
> + return;
> + }
> +
> + rate = clk_get_rate(mdp->clk_main);
> +
> + con0 = readl(mdp->base + mdp->data->con0);
> + con1 = readl(mdp->base + mdp->data->con1);
> +
> + state->polarity = con0 & PWM_POLARITY ?
> + PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> + state->enabled = !!(con0 & BIT(0));
> +
> + clk_div = (con0 & PWM_CLKDIV_MASK) >> PWM_CLKDIV_SHIFT;
> + period = con1 & PWM_PERIOD_MASK;
> + state->period = div_u64(period * (clk_div + 1) * NSEC_PER_SEC, rate);
> + high_width = (con1 & PWM_HIGH_WIDTH_MASK) >> PWM_HIGH_WIDTH_SHIFT;
> + state->duty_cycle = div_u64(high_width * (clk_div + 1) * NSEC_PER_SEC,
> + rate);
Please round up these divisions as in .apply() we're rounding down.
Otherwise .get_state isn't the inverse of .apply().
> +
> + if (!state->enabled) {
> + clk_disable_unprepare(mdp->clk_mm);
> + clk_disable_unprepare(mdp->clk_main);
That's wrong, you enabled unconditionally so you should also disable
unconditionally. If you want to prevent that clocks of a running PWM are
disabled by the unused clk initcall, this must be done in .probe() to be
sure it runs exactly once and early enough.
> + }
> +
> + mdp->enabled = state->enabled;
> +}
> +
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [v3,PATCH 2/3] pwm: mtk_disp: convert the driver to atomic API
Hello,
On Tue, Apr 06, 2021 at 05:57:41PM +0800, Rex-BC Chen wrote:
> @@ -84,33 +86,47 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip,
> struct pwm_device *pwm,
>* period = (PWM_CLK_RATE * period_ns) / (10^9 * (clk_div + 1)) - 1
>* high_width = (PWM_CLK_RATE * duty_ns) / (10^9 * (clk_div + 1))
>*/
> + if (!mdp->enabled) {
> + err = clk_prepare_enable(mdp->clk_main);
> + if (err < 0) {
> + dev_err(chip->dev, "Can't enable mdp->clk_main: %d\n",
> + err);
Please use %pe, as this yields better readable error messages.
Also it might be sensible to first use the fact that (without patch 1
from this series) the clocks are always on and then rework the clk usage
in a separate patch.
> + return err;
> + }
> + err = clk_prepare_enable(mdp->clk_mm);
> + if (err < 0) {
> + dev_err(chip->dev, "Can't enable mdp->clk_mm: %d\n",
> + err);
> + clk_disable_unprepare(mdp->clk_main);
> + return err;
> + }
> + }
> rate = clk_get_rate(mdp->clk_main);
> - clk_div = div_u64(rate * period_ns, NSEC_PER_SEC) >>
> + clk_div = div_u64(rate * state->period, NSEC_PER_SEC) >>
> PWM_PERIOD_BIT_WIDTH;
rate * state->period might overflow, it would be great if this could be
catched. (But I don't consider this a stopper for this series.)
> - if (clk_div > PWM_CLKDIV_MAX)
> + if (clk_div > PWM_CLKDIV_MAX) {
> + dev_err(chip->dev, "clock rate is too high: rate = %d Hz\n",
> + rate);
rate is an u64, %d isn't the right format for it. Doesn't this result in
a compiler warning?
> + clk_disable_unprepare(mdp->clk_mm);
> + clk_disable_unprepare(mdp->clk_main);
> return -EINVAL;
> -
> + }
> div = NSEC_PER_SEC * (clk_div + 1);
> - period = div64_u64(rate * period_ns, div);
> + period = div64_u64(rate * state->period, div);
> if (period > 0)
> period--;
>
> - high_width = div64_u64(rate * duty_ns, div);
> + high_width = div64_u64(rate * state->duty_cycle, div);
> value = period | (high_width << PWM_HIGH_WIDTH_SHIFT);
> -
> - err = clk_enable(mdp->clk_main);
> - if (err < 0)
> - return err;
> -
> - err = clk_enable(mdp->clk_mm);
> - if (err < 0) {
> - clk_disable(mdp->clk_main);
> - return err;
> - }
> + polarity = 0;
> + if (state->polarity == PWM_POLARITY_INVERSED)
> + polarity = PWM_POLARITY;
I'm unsure if support for polarity should be added en passant in this
patch. Maybe it would be clearer to add is separately.
> mtk_disp_pwm_update_bits(mdp, mdp->data->con0,
>PWM_CLKDIV_MASK,
>clk_div << PWM_CLKDIV_SHIFT);
> + mtk_disp_pwm_update_bits(mdp, mdp->data->con0,
> + PWM_POLARITY, polarity);
> mtk_disp_pwm_update_bits(mdp, mdp->data->con1,
>PWM_PERIOD_MASK | PWM_HIGH_WIDTH_MASK,
>value);
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [v3,PATCH 1/3] pwm: mtk_disp: clear the clock operations
Hello, On Tue, Apr 06, 2021 at 05:57:40PM +0800, Rex-BC Chen wrote: > Remove the clk_prepare from mtk_disp_pwm_probe. > Remove the clk_unprepare from mtk_disp_pwm_remove. > > After using atomic API and get_state() function which are implemented > in PATCH [2/3], [3/3], Refering to the following patches as 2/3 and 3/3 doesn't make sense once these patches are applied to a tree. > clk_prepare/clk_unprepare are useless in probe/remove function. > So we remove clk_prepare/clk_unprepare in probe/remove fuinction. Does the driver still work with only this patch applied? If not, please rearrange and order this patch after the conversion to the atomic API. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
[PATCH] pwm: Rename pwm_get_state() to better reflect its semantic
Given that lowlevel drivers usually cannot implement exactly what a
consumer requests with pwm_apply_state() there is some rounding involved.
pwm_get_state() traditionally returned the setting that was requested most
recently by the consumer (opposed to what was actually implemented in
hardware in reply to the last request). To make this semantic obvious
rename the function.
Signed-off-by: Uwe Kleine-König
---
Documentation/driver-api/pwm.rst | 6 +++-
drivers/clk/clk-pwm.c | 2 +-
drivers/gpu/drm/i915/display/intel_panel.c | 4 +--
drivers/input/misc/da7280.c| 2 +-
drivers/input/misc/pwm-beeper.c| 2 +-
drivers/input/misc/pwm-vibra.c | 4 +--
drivers/pwm/core.c | 4 +--
drivers/pwm/pwm-atmel-hlcdc.c | 2 +-
drivers/pwm/pwm-atmel.c| 2 +-
drivers/pwm/pwm-imx27.c| 2 +-
drivers/pwm/pwm-rockchip.c | 2 +-
drivers/pwm/pwm-stm32-lp.c | 4 +--
drivers/pwm/pwm-sun4i.c| 2 +-
drivers/pwm/sysfs.c| 18 ++--
drivers/regulator/pwm-regulator.c | 4 +--
drivers/video/backlight/pwm_bl.c | 10 +++
include/linux/pwm.h| 34 ++
17 files changed, 59 insertions(+), 45 deletions(-)
diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst
index ab62f1bb0366..381f3c46cdac 100644
--- a/Documentation/driver-api/pwm.rst
+++ b/Documentation/driver-api/pwm.rst
@@ -55,7 +55,11 @@ several parameter at once. For example, if you see
pwm_config() and
pwm_{enable,disable}() calls in the same function, this probably means you
should switch to pwm_apply_state().
-The PWM user API also allows one to query the PWM state with pwm_get_state().
+The PWM user API also allows one to query the last applied PWM state with
+pwm_get_last_applied_state(). Note this is different to what the driver has
+actually implemented if the request cannot be implemented exactly with the
+hardware in use. There is currently no way for consumers to get the actually
+implemented settings.
In addition to the PWM state, the PWM API also exposes PWM arguments, which
are the reference PWM config one should use on this PWM.
diff --git a/drivers/clk/clk-pwm.c b/drivers/clk/clk-pwm.c
index da2c8eddfd9f..7cfaff32980b 100644
--- a/drivers/clk/clk-pwm.c
+++ b/drivers/clk/clk-pwm.c
@@ -49,7 +49,7 @@ static int clk_pwm_get_duty_cycle(struct clk_hw *hw, struct
clk_duty *duty)
struct clk_pwm *clk_pwm = to_clk_pwm(hw);
struct pwm_state state;
- pwm_get_state(clk_pwm->pwm, );
+ pwm_get_last_applied_state(clk_pwm->pwm, );
duty->num = state.duty_cycle;
duty->den = state.period;
diff --git a/drivers/gpu/drm/i915/display/intel_panel.c
b/drivers/gpu/drm/i915/display/intel_panel.c
index 5fdf52643150..3aebf9be6b6a 100644
--- a/drivers/gpu/drm/i915/display/intel_panel.c
+++ b/drivers/gpu/drm/i915/display/intel_panel.c
@@ -627,7 +627,7 @@ static u32 ext_pwm_get_backlight(struct intel_connector
*connector, enum pipe un
struct intel_panel *panel = >panel;
struct pwm_state state;
- pwm_get_state(panel->backlight.pwm, );
+ pwm_get_last_applied_state(panel->backlight.pwm, );
return pwm_get_relative_duty_cycle(, 100);
}
@@ -1915,7 +1915,7 @@ static int ext_pwm_setup_backlight(struct intel_connector
*connector,
if (pwm_is_enabled(panel->backlight.pwm)) {
/* PWM is already enabled, use existing settings */
- pwm_get_state(panel->backlight.pwm,
>backlight.pwm_state);
+ pwm_get_last_applied_state(panel->backlight.pwm,
>backlight.pwm_state);
level = pwm_get_relative_duty_cycle(>backlight.pwm_state,
100);
diff --git a/drivers/input/misc/da7280.c b/drivers/input/misc/da7280.c
index b08610d6e575..409670be1d2b 100644
--- a/drivers/input/misc/da7280.c
+++ b/drivers/input/misc/da7280.c
@@ -333,7 +333,7 @@ static int da7280_haptic_set_pwm(struct da7280_haptic
*haptics, bool enabled)
return -EINVAL;
}
- pwm_get_state(haptics->pwm_dev, );
+ pwm_get_last_applied_state(haptics->pwm_dev, );
state.enabled = enabled;
if (enabled) {
period_mag_multi = (u64)state.period * haptics->gain;
diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index d6b12477748a..4c5f09201ecf 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -33,7 +33,7 @@ static int pwm_beeper_on(struct pwm_beeper *beeper, unsigned
long period)
struct pwm_state state;
int error;
- pwm_get_state(beeper->pwm, );
+ pwm_get_last_applied_state(beeper->pwm, );
state.ena
Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout
Hello Thierry,
On Wed, Mar 31, 2021 at 05:52:45PM +0200, Thierry Reding wrote:
> On Wed, Mar 31, 2021 at 12:25:43PM +0200, Uwe Kleine-König wrote:
> > On Mon, Mar 22, 2021 at 09:34:21AM +0100, Thierry Reding wrote:
> > > On Mon, Jan 11, 2021 at 09:43:50PM +0100, Uwe Kleine-König wrote:
> > > > On Sun, Jan 03, 2021 at 06:04:10PM +0100, Clemens Gruber wrote:
> > > > > Another point is the period: Sven suggested we do not read out the
> > > > > period at all, as the PWM is disabled anyway (see above).
> > > > > Is this acceptable?
> > > >
> > > > In my eyes consumers should consider the period value as "don't care" if
> > > > the PWM is off. But this doesn't match reality (and maybe also it
> > > > doesn't match Thierry's opinion). See for example the
> > > > drivers/video/backlight/pwm_bl.c driver which uses the idiom:
> > > >
> > > > pwm_get_state(mypwm, );
> > > > state.enabled = true;
> > > > pwm_apply_state(pb->pwm, );
> > > >
> > > > which breaks if .period is invalid. (OK, this isn't totally accurate
> > > > because currently the .get_state callback has only little to do with
> > > > pwm_get_state(), but you get the point.)
> > >
> > > The idea behind atomic states in the PWM API is to provide accurate
> > > snapshots of a PWM channel's settings. It's not a representation of
> > > the PWM channel's physical output, although in some cases they may
> > > be the same.
> >
> > I think the pwm_state returned by .get_state() should be an accurate
> > representation of the PWM channel's physical output.
>
> Yeah, like I said, in most cases that will be true. However, as
> mentioned below, if there's no 1:1 correspondence between the settings
> and what's actually coming out of the PWM, this isn't always possible.
> But yes, it should always be as accurate as possible.
It should always be true, not only in most cases. For any given PWM
output you can provide a pwm_state that describes this output (unless
you'd need a value bigger than u64 to describe it or a higher precision
as pwm_state only uses integer values). So it's a 1:n relation: You
should always be able to provide a pwm_state and in some cases there are
more than one such states (and you should still provide one of them).
> > > However, given that we want a snapshot of the currently configured
> > > settings, we can't simply assume that there's a 1:1 correspondence and
> > > then use shortcuts to simplify the hardware state representation because
> > > it isn't going to be accurate.
> >
> > When we assume that pwm_get_state returns the state of the hardware,
> > relying on these values might be too optimistic. Consider a hardware
> > (e.g. pwm-cros-ec) that has no disable switch. Disabling is modeled by
> > .duty_cycle = 0. So after:
> >
> > pwm_apply_state(mypwm, { .period = 1000, .duty_cycle = 500, .enabled =
> > false })
> >
> > doing:
> >
> > pwm_get_state(pwm, );
> > mystate.enabled = true;
> > pwm_apply_state(pwm, );
> >
> > the PWM stays configured with .duty_cycle = 0.
>
> Uhm... no, that's not what should be happening.
Agreed, it shouldn't be happening. Note the paragraph started with "When
we assume that pwm_get_state returns the state of the hardware" and with
that my statement is correct. And so the conclusion is that calculations
by the consumer should always be done based on requested states, not on
actually implemented settings.
> pwm_get_state() copies the PWM channel's internal software state. It
> doesn't actually go and read the hardware state. We only ever read the
> hardware state when the PWM is requested. After that there should be
> no reason to ever read back the hardware state again because the
> consumer owns the state and that state must not change unless
> explicitly requested by the owner.
I have problems to follow your reasoning. What is the semantic of "PWM
channel's internal software state"? What is "currently configured
setting"?
There are two different possible semantics for pwm_get_state():
a) The state that was passed to pwm_apply_state() before.
b) What is actually configured in hardware.
Currently the implemented semantic is a). (Apart from the very beginning
when there wasn't anything applied before.) And there is no way for a
consumer to get b). If you only ever want to yield a) there is indeed
no need to read the state from hardware.
> So in the above case, mystate.duty_cycle should be 500 after that call
> to pwm_get_state(). Th
Re: [PATCH v6 4/7] pwm: pca9685: Support staggered output ON times
On Wed, Mar 31, 2021 at 02:26:14PM +0200, Clemens Gruber wrote: > On Mon, Mar 29, 2021 at 08:02:06PM +0200, Uwe Kleine-König wrote: > > On Mon, Mar 29, 2021 at 07:16:38PM +0200, Clemens Gruber wrote: > > > On Mon, Mar 29, 2021 at 07:03:57PM +0200, Uwe Kleine-König wrote: > > > > On Mon, Mar 29, 2021 at 02:57:04PM +0200, Clemens Gruber wrote: > > > > > The PCA9685 supports staggered LED output ON times to minimize current > > > > > surges and reduce EMI. > > > > > When this new option is enabled, the ON times of each channel are > > > > > delayed by channel number x counter range / 16, which avoids asserting > > > > > all enabled outputs at the same counter value while still maintaining > > > > > the configured duty cycle of each output. > > > > > > > > > > Signed-off-by: Clemens Gruber > > > > > > > > Is there a reason to not want this staggered output? If it never hurts I > > > > suggest to always stagger and drop the dt property. > > > > > > There might be applications where you want multiple outputs to assert at > > > the same time / to be synchronized. > > > With staggered outputs mode always enabled, this would no longer be > > > possible as they are spread out according to their channel number. > > > > > > Not sure how often that usecase is required, but just enforcing the > > > staggered mode by default sounds risky to me. > > > > There is no such guarantee in the PWM framework, so I don't think we > > need to fear breaking setups. Thierry? > > Still, someone might rely on it? But let's wait for Thierry's opinion. > > > > > One reason we might not want staggering is if we have a consumer who > > cares about config transitions. (This however is moot it the hardware > > doesn't provide sane transitions even without staggering.) > > > > Did I already ask about races in this driver? I assume there is a > > free running counter and the ON and OFF registers just define where in > > the period the transitions happen, right? Given that changing ON and OFF > > needs two register writes probably all kind of strange things can > > happen, right? (Example thought: for simplicity's sake I assume ON is > > always 0. Then if you want to change from OFF = 0xaaa to OFF = 0xccc we > > might see a period with 0xacc. Depending on how the hardware works we > > might even see 4 edges in a single period then.) > > Yes, there is a free running counter from 0 to 4095. > And it is probably true, that there can be short intermediate states > with our two register writes. > > There is a separate mode "Update on ACK" (MODE2 register, bit 3 "OCH"), > which is 0 by default (Outputs change on STOP command) but could be set > to 1 (Outputs change on ACK): > "Update on ACK requires all 4 PWM channel registers to be loaded before > outputs will change on the last ACK." After reading the manual I understood that at least a bit now. The Output chang on STOP is only needed to synchronize two or more PCA9685 chips. Also with "Update on ACK" it is possible to prevent glitches when writing with the auto increment mode. Not sure what happens with the way it is implemented now, as it isn't described in manual when the registers are written in four separate transfers. I agree that addressing this in a separater patch is sensible. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v6 5/7] dt-bindings: pwm: pca9685: Add nxp,staggered-outputs property
Hello Clemens, On Mon, Mar 29, 2021 at 02:57:05PM +0200, Clemens Gruber wrote: > The pca9685 driver supports a new nxp,staggered-outputs property for > reduced current surges and EMI. This adds documentation for the new DT > property. > > Signed-off-by: Clemens Gruber This patch would need an ack by the dt guys, so you should Cc: them in the next round if this (or a similar) patch is still part of the series. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v6 4/7] pwm: pca9685: Support staggered output ON times
Hello, On Wed, Mar 31, 2021 at 06:21:32PM +0200, Thierry Reding wrote: > However, I'm a bit hesitant about this staggering output mode. From what > I understand what's going to happen for these is basically that overall > each PWM will be running at the requested duty cycle, but the on/off > times will be evenly spread out over the whole period. In other words, > the output *power* of the PWM signal will be the same as if the signal > was a single on/off cycle. That's not technically a PWM signal as the > PWM framework defines it. See the kerneldoc for enum pwm_polarity for > what signals are expected to look like. After reading this thread I had the impression that there is no (externally visible) difference between using ON = 0 plus programming a new setting when the counter is say 70 and using ON = 30 plus programming a new setting when the counter is 100. But that's not the case and I agree that defaulting to staggering is a bad idea. Having said that I doubt that adding a property to the device tree is a good solution, because it changes behaviour without the consumer being aware and additionally it's not really a hardware description. The solution I'd prefer is to change struct pwm_state to include the delay in it. (This would then make the polarity obsolete, because .duty_cycle = 30 .period = 100 .polarity = POLARITY_INVERTED .offset = 0 is equivalent to .duty_cycle = 30 .period = 100 .polarity = POLARITY_NORMAL .offset = 70 . Other inverted states can be modified similarily.) Then consumers can be coordinated to use different offsets. I'm aware changing this isn't trivial, and it's not thought out completely, but I think the end result is rechnically superior to the approach suggested in the patch under discussion. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v6 4/7] pwm: pca9685: Support staggered output ON times
On Wed, Mar 31, 2021 at 03:55:49PM +0200, Clemens Gruber wrote: > On Wed, Mar 31, 2021 at 02:26:14PM +0200, Clemens Gruber wrote: > > On Mon, Mar 29, 2021 at 08:02:06PM +0200, Uwe Kleine-König wrote: > > > On Mon, Mar 29, 2021 at 07:16:38PM +0200, Clemens Gruber wrote: > > > > On Mon, Mar 29, 2021 at 07:03:57PM +0200, Uwe Kleine-König wrote: > > > > > On Mon, Mar 29, 2021 at 02:57:04PM +0200, Clemens Gruber wrote: > > > > > > The PCA9685 supports staggered LED output ON times to minimize > > > > > > current > > > > > > surges and reduce EMI. > > > > > > When this new option is enabled, the ON times of each channel are > > > > > > delayed by channel number x counter range / 16, which avoids > > > > > > asserting > > > > > > all enabled outputs at the same counter value while still > > > > > > maintaining > > > > > > the configured duty cycle of each output. > > > > > > > > > > > > Signed-off-by: Clemens Gruber > > > > > > > > > > Is there a reason to not want this staggered output? If it never > > > > > hurts I > > > > > suggest to always stagger and drop the dt property. > > > > > > > > There might be applications where you want multiple outputs to assert at > > > > the same time / to be synchronized. > > > > With staggered outputs mode always enabled, this would no longer be > > > > possible as they are spread out according to their channel number. > > > > > > > > Not sure how often that usecase is required, but just enforcing the > > > > staggered mode by default sounds risky to me. > > > > > > There is no such guarantee in the PWM framework, so I don't think we > > > need to fear breaking setups. Thierry? > > > > Still, someone might rely on it? But let's wait for Thierry's opinion. > > > > > > > > One reason we might not want staggering is if we have a consumer who > > > cares about config transitions. (This however is moot it the hardware > > > doesn't provide sane transitions even without staggering.) > > > > > > Did I already ask about races in this driver? I assume there is a > > > free running counter and the ON and OFF registers just define where in > > > the period the transitions happen, right? Given that changing ON and OFF > > > needs two register writes probably all kind of strange things can > > > happen, right? (Example thought: for simplicity's sake I assume ON is > > > always 0. Then if you want to change from OFF = 0xaaa to OFF = 0xccc we > > > might see a period with 0xacc. Depending on how the hardware works we > > > might even see 4 edges in a single period then.) > > > > Yes, there is a free running counter from 0 to 4095. > > And it is probably true, that there can be short intermediate states > > with our two register writes. > > > > There is a separate mode "Update on ACK" (MODE2 register, bit 3 "OCH"), > > which is 0 by default (Outputs change on STOP command) but could be set > > to 1 (Outputs change on ACK): > > "Update on ACK requires all 4 PWM channel registers to be loaded before > > outputs will change on the last ACK." > > This would require the auto-increment feature to be enabled, then > multiple registers could be written before the STOP condition: > LEDn_ON_L, LEDn_ON_H, LEDn_OFF_L & LEDn_OFF_H > (With OCH=0 in MODE2) Maybe a continued START would work, too?! Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v6 4/7] pwm: pca9685: Support staggered output ON times
Hello Clemens, On Wed, Mar 31, 2021 at 02:26:14PM +0200, Clemens Gruber wrote: > On Mon, Mar 29, 2021 at 08:02:06PM +0200, Uwe Kleine-König wrote: > > On Mon, Mar 29, 2021 at 07:16:38PM +0200, Clemens Gruber wrote: > > > On Mon, Mar 29, 2021 at 07:03:57PM +0200, Uwe Kleine-König wrote: > > > > On Mon, Mar 29, 2021 at 02:57:04PM +0200, Clemens Gruber wrote: > > > > > The PCA9685 supports staggered LED output ON times to minimize current > > > > > surges and reduce EMI. > > > > > When this new option is enabled, the ON times of each channel are > > > > > delayed by channel number x counter range / 16, which avoids asserting > > > > > all enabled outputs at the same counter value while still maintaining > > > > > the configured duty cycle of each output. > > > > > > > > > > Signed-off-by: Clemens Gruber > > > > > > > > Is there a reason to not want this staggered output? If it never hurts I > > > > suggest to always stagger and drop the dt property. > > > > > > There might be applications where you want multiple outputs to assert at > > > the same time / to be synchronized. > > > With staggered outputs mode always enabled, this would no longer be > > > possible as they are spread out according to their channel number. > > > > > > Not sure how often that usecase is required, but just enforcing the > > > staggered mode by default sounds risky to me. > > > > There is no such guarantee in the PWM framework, so I don't think we > > need to fear breaking setups. Thierry? > > Still, someone might rely on it? But let's wait for Thierry's opinion. Someone might rely on the pca9685 driver being as racy as a driver with legacy bindings usually is. Should this be the reason to drop this whole series? > > One reason we might not want staggering is if we have a consumer who > > cares about config transitions. (This however is moot it the hardware > > doesn't provide sane transitions even without staggering.) > > > > Did I already ask about races in this driver? I assume there is a > > free running counter and the ON and OFF registers just define where in > > the period the transitions happen, right? Given that changing ON and OFF > > needs two register writes probably all kind of strange things can > > happen, right? (Example thought: for simplicity's sake I assume ON is > > always 0. Then if you want to change from OFF = 0xaaa to OFF = 0xccc we > > might see a period with 0xacc. Depending on how the hardware works we > > might even see 4 edges in a single period then.) > > Yes, there is a free running counter from 0 to 4095. > And it is probably true, that there can be short intermediate states > with our two register writes. > > There is a separate mode "Update on ACK" (MODE2 register, bit 3 "OCH"), > which is 0 by default (Outputs change on STOP command) but could be set > to 1 (Outputs change on ACK): > "Update on ACK requires all 4 PWM channel registers to be loaded before > outputs will change on the last ACK." This is about the ACK and STOP in the i2c communication, right? I fail to understand the relevance of this difference. I guess I have to read the manual myself. > The chip datasheet also states: > "Because the loading of the LEDn_ON and LEDn_OFF registers is via the > I2C-bus, and asynchronous to the internal oscillator, we want to ensure > that we do not see any visual artifacts of changing the ON and OFF > values. This is achieved by updating the changes at the end of the LOW > cycle." So we're only out of luck if the first register write happens before and the second after the end of the LOW cycle, aren't we? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout
Hello Thierry,
On Mon, Mar 22, 2021 at 09:34:21AM +0100, Thierry Reding wrote:
> On Mon, Jan 11, 2021 at 09:43:50PM +0100, Uwe Kleine-König wrote:
> > On Sun, Jan 03, 2021 at 06:04:10PM +0100, Clemens Gruber wrote:
> > > Another point is the period: Sven suggested we do not read out the
> > > period at all, as the PWM is disabled anyway (see above).
> > > Is this acceptable?
> >
> > In my eyes consumers should consider the period value as "don't care" if
> > the PWM is off. But this doesn't match reality (and maybe also it
> > doesn't match Thierry's opinion). See for example the
> > drivers/video/backlight/pwm_bl.c driver which uses the idiom:
> >
> > pwm_get_state(mypwm, );
> > state.enabled = true;
> > pwm_apply_state(pb->pwm, );
> >
> > which breaks if .period is invalid. (OK, this isn't totally accurate
> > because currently the .get_state callback has only little to do with
> > pwm_get_state(), but you get the point.)
>
> The idea behind atomic states in the PWM API is to provide accurate
> snapshots of a PWM channel's settings. It's not a representation of
> the PWM channel's physical output, although in some cases they may
> be the same.
I think the pwm_state returned by .get_state() should be an accurate
representation of the PWM channel's physical output.
> However, there's no 1:1 correspondence between those two. For example,
> when looking purely at the physical output of a PWM it is in most cases
> not possible to make the distinction between these two states:
>
> - duty: 0
> period: 100
>
> - duty: 0
> period: 200
>
> Because the output will be a constant 0 (or 1, depending on polarity).
I agree that there isn't in all cases a unique pwm_state that formalizes
the current output. That's because with .enabled=false the settings
.duty_cycle and .period hardly matter for the output and when
.duty_cycle = 0 or = .period the actual period also (mostly) doesn't
matter.
> However, given that we want a snapshot of the currently configured
> settings, we can't simply assume that there's a 1:1 correspondence and
> then use shortcuts to simplify the hardware state representation because
> it isn't going to be accurate.
When we assume that pwm_get_state returns the state of the hardware,
relying on these values might be too optimistic. Consider a hardware
(e.g. pwm-cros-ec) that has no disable switch. Disabling is modeled by
.duty_cycle = 0. So after:
pwm_apply_state(mypwm, { .period = 1000, .duty_cycle = 500, .enabled =
false })
doing:
pwm_get_state(pwm, );
mystate.enabled = true;
pwm_apply_state(pwm, );
the PWM stays configured with .duty_cycle = 0.
There are also more delicate surprises. Consider a PWM that can
implement all duty_cycles and periods with a resolution of 30 ns up to
the consumers preferred period of 2000 ns and uses the usual round-down
strategy. Consider the consumer wants to repeatedly switch between 75%
and 50% relative duty cycle.
When relying on .get_state, the first configuration is likely to still
be 1500/2000. .get_state() then returns
.duty_cycle = 1500
.period = 1980
and then to implement the 50% relative duty the resulting request is
.duty_cycle = 990
.period = 1980
which can be implemented exactly. When then switching back to 75% the
request is
.duty_cycle = 1485
.period = 1980
yielding a period of 1470 ns. So this is a different setting than on the
first go to implement 75% because the rounding errors accumulate.
The IMHO only sane way out is that the consumer should always request
1500/2000 and 1000/2000.
So I think calculating the state from the previously implemented state
has too many surprises and should not be the recommended way.
Consumers should just request what they want and provide this state
without relying on .get_state(). And then the needs to cache the
duty_cycle for a disabled PWM or reading the period for a disabled PWM
just vanish in thin air.
> It is entirely expected that consumers will be able to use an existing
> atomic state, update it and then apply it without necessarily having to
> recompute the whole state. So what pwm-backlight is doing is expressly
> allowed (and in fact was one specific feature that we wanted to have in
> the atomic API).
Who is "we"?
> Similarly it's a valid use-case to do something like this:
>
> /* set duty cycle to 50% */
> pwm_get_state(pwm, );
> state.duty_cycle = state.period / 2;
> pwm_apply_state(pwm, );
The cost to continue doing that is that the consumer has to cache the
state. Then the idiom looks as follows:
state = _data->state;
state->duty_cycle = state->period / 2;
pwm_apply_state(pwm,
Re: [PATCH v6 4/7] pwm: pca9685: Support staggered output ON times
On Mon, Mar 29, 2021 at 07:16:38PM +0200, Clemens Gruber wrote: > On Mon, Mar 29, 2021 at 07:03:57PM +0200, Uwe Kleine-König wrote: > > On Mon, Mar 29, 2021 at 02:57:04PM +0200, Clemens Gruber wrote: > > > The PCA9685 supports staggered LED output ON times to minimize current > > > surges and reduce EMI. > > > When this new option is enabled, the ON times of each channel are > > > delayed by channel number x counter range / 16, which avoids asserting > > > all enabled outputs at the same counter value while still maintaining > > > the configured duty cycle of each output. > > > > > > Signed-off-by: Clemens Gruber > > > > Is there a reason to not want this staggered output? If it never hurts I > > suggest to always stagger and drop the dt property. > > There might be applications where you want multiple outputs to assert at > the same time / to be synchronized. > With staggered outputs mode always enabled, this would no longer be > possible as they are spread out according to their channel number. > > Not sure how often that usecase is required, but just enforcing the > staggered mode by default sounds risky to me. There is no such guarantee in the PWM framework, so I don't think we need to fear breaking setups. Thierry? One reason we might not want staggering is if we have a consumer who cares about config transitions. (This however is moot it the hardware doesn't provide sane transitions even without staggering.) Did I already ask about races in this driver? I assume there is a free running counter and the ON and OFF registers just define where in the period the transitions happen, right? Given that changing ON and OFF needs two register writes probably all kind of strange things can happen, right? (Example thought: for simplicity's sake I assume ON is always 0. Then if you want to change from OFF = 0xaaa to OFF = 0xccc we might see a period with 0xacc. Depending on how the hardware works we might even see 4 edges in a single period then.) Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v6 6/7] pwm: pca9685: Restrict period change for prescaler users
Hello Clemens,
On Mon, Mar 29, 2021 at 07:33:56PM +0200, Clemens Gruber wrote:
> On Mon, Mar 29, 2021 at 07:15:59PM +0200, Uwe Kleine-König wrote:
> > On Mon, Mar 29, 2021 at 02:57:06PM +0200, Clemens Gruber wrote:
> > > @@ -330,14 +345,22 @@ static int pca9685_pwm_apply(struct pwm_chip *chip,
> > > struct pwm_device *pwm,
> > >
> > > if (!state->enabled || duty < 1) {
> > > pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> > > + clear_bit(pwm->hwpwm, pca->prescaler_users);
> >
> > Hmm, so if "my" channel runs at say
> >
> > .duty_cycle = 2539520 ns
> > .period = 5079040 ns
> >
> > and I call pwm_apply_state(mypwm, { .duty_cycle = 0, .period = 5079040,
> > enabled = true }); it might happen that another channel modifies the
> > period and I won't be able to return to the initial setting.
>
> Yes, that's correct.
>
> But that also applies to PWMs set to 100%:
>
> pwm_apply_state(mypwm, { .duty_cycle = 5079040, .period = 5079040,
> enabled = true });
>
> As this sets the full ON bit and does not use the prescaler, with the
> current code, another channel could modify the period and you wouldn't
> be able to return to the initial setting of 50%.
>
> > So I think it's sensible to only clear the user bit if the PWM is
> > disabled, but not if it is configured for duty_cycle = 0.
> >
> > Does this make sense?
>
> With both cases in mind, you are suggesting we block modifications of
> the prescaler if other PWMs are enabled and not if other PWMs are using
> the prescaler?
That sounds sensible, yes.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v6 2/7] pwm: pca9685: Support hardware readout
On Mon, Mar 29, 2021 at 07:11:53PM +0200, Clemens Gruber wrote:
> On Mon, Mar 29, 2021 at 06:54:29PM +0200, Uwe Kleine-König wrote:
> > On Mon, Mar 29, 2021 at 02:57:02PM +0200, Clemens Gruber wrote:
> > > [...]
> > > + /* Calculate (chip-wide) period from prescale value */
> > > + regmap_read(pca->regmap, PCA9685_PRESCALE, );
> > > + state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
> > > + (val + 1);
> >
> > As we have PCA9685_OSC_CLOCK_MHZ = 25 this is an integer calculation
> > without loss of precision. It might be worth to point that out in a
> > comment. (Otherwise doing the division at the end might be more
> > sensible.)
>
> What comment do you have in mind?
> /* 1 integer multiplication (without loss of precision) at runtime */ ?
Something like:
/*
* PCA9685_OSC_CLOCK_MHZ is 25 and so an integer divider of
* 1000. So the calculation here is only a multiplication and
* we're not loosing precision.
*/
> > > + /* The (per-channel) polarity is fixed */
> > > + state->polarity = PWM_POLARITY_NORMAL;
> > > +
> > > + if (pwm->hwpwm >= PCA9685_MAXCHAN) {
> > > + /*
> > > + * The "all LEDs" channel does not support HW readout
> > > + * Return 0 and disabled for backwards compatibility
> > > + */
> > > + state->duty_cycle = 0;
> > > + state->enabled = false;
> > > + return;
> > > + }
> > > +
> > > + duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
> > > +
> > > + state->enabled = !!duty;
> > > + if (!state->enabled) {
> > > + state->duty_cycle = 0;
> > > + return;
> > > + } else if (duty == PCA9685_COUNTER_RANGE) {
> > > + state->duty_cycle = state->period;
> > > + return;
> > > + }
> > > +
> > > + duty *= state->period;
> > > + state->duty_cycle = duty / PCA9685_COUNTER_RANGE;
> >
> > .apply uses ROUND_CLOSEST to calculate duty from state->duty_cycle,
> > still using / here (instead of ROUND_CLOSEST), but again as
> > PCA9685_OSC_CLOCK_MHZ is 25 this calculation doesn't suffer from
> > rounding errors. So if you feed the state returned here into .apply
> > again, there is (as I want it) no change.
> >
> > The only annoyance is that if PCA9685_PRESCALE holds a value smaller
> > than 3, .apply() will fail. Not sure there is any saner way to handle
> > this.
>
> According to the datasheet, "The hardware forces a minimum value that
> can be loaded into the PRE_SCALE register at '3'", so there should never
> be anything below 3 in that register.
Did you verify that the register reads back a 3 if you write a lower
value into the register?
Maybe the most defensive way would be:
+ regmap_read(pca->regmap, PCA9685_PRESCALE, );
+ /*
+* According to the datasheet, the hardware forces a minimum
+* value that can be loaded is 3, so if we read something lower
+* assume that the hardware actually implemented a 3.
+*/
+ if (val < 3)
+ val = 3;
+ state->period = ...
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v6 6/7] pwm: pca9685: Restrict period change for prescaler users
On Mon, Mar 29, 2021 at 02:57:06PM +0200, Clemens Gruber wrote:
> @@ -330,14 +345,22 @@ static int pca9685_pwm_apply(struct pwm_chip *chip,
> struct pwm_device *pwm,
>
> if (!state->enabled || duty < 1) {
> pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> + clear_bit(pwm->hwpwm, pca->prescaler_users);
Hmm, so if "my" channel runs at say
.duty_cycle = 2539520 ns
.period = 5079040 ns
and I call pwm_apply_state(mypwm, { .duty_cycle = 0, .period = 5079040,
enabled = true }); it might happen that another channel modifies the
period and I won't be able to return to the initial setting.
So I think it's sensible to only clear the user bit if the PWM is
disabled, but not if it is configured for duty_cycle = 0.
Does this make sense?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v6 4/7] pwm: pca9685: Support staggered output ON times
On Mon, Mar 29, 2021 at 02:57:04PM +0200, Clemens Gruber wrote: > The PCA9685 supports staggered LED output ON times to minimize current > surges and reduce EMI. > When this new option is enabled, the ON times of each channel are > delayed by channel number x counter range / 16, which avoids asserting > all enabled outputs at the same counter value while still maintaining > the configured duty cycle of each output. > > Signed-off-by: Clemens Gruber Is there a reason to not want this staggered output? If it never hurts I suggest to always stagger and drop the dt property. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v6 2/7] pwm: pca9685: Support hardware readout
On Mon, Mar 29, 2021 at 02:57:02PM +0200, Clemens Gruber wrote:
> Implements .get_state to read-out the current hardware state.
>
> The hardware readout may return slightly different values than those
> that were set in apply due to the limited range of possible prescale and
> counter register values.
>
> Also note that although the datasheet mentions 200 Hz as default
> frequency when using the internal 25 MHz oscillator, the calculated
> period from the default prescaler register setting of 30 is 5079040ns.
>
> Signed-off-by: Clemens Gruber
> ---
> drivers/pwm/pwm-pca9685.c | 41 +++
> 1 file changed, 41 insertions(+)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 0ed1013737e3..fb026a25fb61 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -333,6 +333,46 @@ static int pca9685_pwm_apply(struct pwm_chip *chip,
> struct pwm_device *pwm,
> return 0;
> }
>
> +static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device
> *pwm,
> + struct pwm_state *state)
> +{
> + struct pca9685 *pca = to_pca(chip);
> + unsigned long long duty;
> + unsigned int val = 0;
> +
> + /* Calculate (chip-wide) period from prescale value */
> + regmap_read(pca->regmap, PCA9685_PRESCALE, );
> + state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
> + (val + 1);
As we have PCA9685_OSC_CLOCK_MHZ = 25 this is an integer calculation
without loss of precision. It might be worth to point that out in a
comment. (Otherwise doing the division at the end might be more
sensible.)
> + /* The (per-channel) polarity is fixed */
> + state->polarity = PWM_POLARITY_NORMAL;
> +
> + if (pwm->hwpwm >= PCA9685_MAXCHAN) {
> + /*
> + * The "all LEDs" channel does not support HW readout
> + * Return 0 and disabled for backwards compatibility
> + */
> + state->duty_cycle = 0;
> + state->enabled = false;
> + return;
> + }
> +
> + duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
> +
> + state->enabled = !!duty;
> + if (!state->enabled) {
> + state->duty_cycle = 0;
> + return;
> + } else if (duty == PCA9685_COUNTER_RANGE) {
> + state->duty_cycle = state->period;
> + return;
> + }
> +
> + duty *= state->period;
> + state->duty_cycle = duty / PCA9685_COUNTER_RANGE;
.apply uses ROUND_CLOSEST to calculate duty from state->duty_cycle,
still using / here (instead of ROUND_CLOSEST), but again as
PCA9685_OSC_CLOCK_MHZ is 25 this calculation doesn't suffer from
rounding errors. So if you feed the state returned here into .apply
again, there is (as I want it) no change.
The only annoyance is that if PCA9685_PRESCALE holds a value smaller
than 3, .apply() will fail. Not sure there is any saner way to handle
this.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v6 3/7] pwm: pca9685: Improve runtime PM behavior
On Mon, Mar 29, 2021 at 02:57:03PM +0200, Clemens Gruber wrote:
> The chip does not come out of POR in active state but in sleep state.
> To be sure (in case the bootloader woke it up) we force it to sleep in
> probe.
>
> On kernels without CONFIG_PM, we wake the chip in .probe and put it to
> sleep in .remove.
What is the effect of sleep state? Does it continue to oscilate it the
bootloader set up some configuration?
> Signed-off-by: Clemens Gruber
> ---
> drivers/pwm/pwm-pca9685.c | 20
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index fb026a25fb61..4d6684b90819 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -469,14 +469,19 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> return ret;
> }
>
> - /* The chip comes out of power-up in the active state */
> - pm_runtime_set_active(>dev);
> /*
> - * Enable will put the chip into suspend, which is what we
> - * want as all outputs are disabled at this point
> + * The chip comes out of power-up in the sleep state,
> + * but force it to sleep in case it was woken up before
>*/
> + pca9685_set_sleep_mode(pca, true);
> + pm_runtime_set_suspended(>dev);
> pm_runtime_enable(>dev);
>
> + if (!IS_ENABLED(CONFIG_PM)) {
> + /* Wake the chip up on non-PM environments */
> + pca9685_set_sleep_mode(pca, false);
I admit I didn't grasp all the PM framework details, but I wonder if
it's right to first call set_sleep_mode(true) and then in some cases to
false again.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v6 2/7] pwm: pca9685: Support hardware readout
Hello Clemens, On Mon, Mar 29, 2021 at 02:57:02PM +0200, Clemens Gruber wrote: > The hardware readout may return slightly different values than those > that were set in apply due to the limited range of possible prescale and > counter register values. This is fine and for most hardware that's not preventable. My requirement here is that .get_state(pwm, ); .apply_state(pwm, ); doesn't yield any changes. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH] serial: imx: drop workaround for forced irq threading
Hello Sebastian,
On Tue, Mar 23, 2021 at 10:04:13AM +0100, Sebastian Andrzej Siewior wrote:
> On 2021-03-23 08:34:47 [+0100], Uwe Kleine-König wrote:
> > On Mon, Mar 22, 2021 at 09:48:36PM +0100, Sebastian Andrzej Siewior wrote:
> > > On 2021-03-22 14:40:32 [+0100], Uwe Kleine-König wrote:
> > > > From a strictly logically point of view you indeed cannot. But if you go
> > > > to the street and say to people there that they can park their car in
> > > > this street free of charge between Monday and Friday, I expect that most
> > > > of them will assume that they have to pay for parking on weekends.
> > >
> > > Uwe, the patch reverts a change which was needed for !RT + threadirqs.
> >
> > This would be a useful information for the commit log.
> >
> > > The commit message claims that since the referenced commit "… interrupt
> > > handlers always run with interrupts disabled on non-RT… ". This has
> > > nothing to do with _this_ change. It argues why the workaround is not
> > > needed.
> >
> > It argues why the work around is not needed on non-RT. It might be
> > obvious for someone who is firm in the RT concepts, but IMHO commit logs
> > should be understandable by and make sense for a wider audience than the
> > deep experts. From what I know about RT "Force-threaded interrupt
> > handlers used to run with interrupts enabled" still applies there.
>
> Yes. The commit Johan referenced explains it in more detail.
In my book the commit log should be understandable without reading the
referenced commits.
> > > If the referenced commit breaks RT then this is another story.
> >
> > I'm surprised to hear that from you. With the goal to get RT into
> > mainline I would expect you to be happy if people consider the effects
> > on RT in their reviews.
>
> Correct, I do and I am glad if people consider other aspects of the
> kernel in their review including RT.
>
> > > > So when you said that on on-RT the reason why it used to need a
> > > > workaround is gone made me wonder what that implies for RT.
> > >
> > > There was never reason (or a lockdep splat) for it on RT. If so you
> > > should have seen it, right?
> >
> > No, I don't consider myself to be an RT expert who is aware of all the
> > problems. So I admit that for me the effect on RT of the patch under
> > discussion isn't obvious. I just wonder that the change is justified
> > with being OK on non-RT. So it's either bad that it breaks RT *or*
> > improving the commit log would be great.
> >
> > And even if I had reason to believe that there is no problem with the
> > commit on RT, I'd still wish that the commit log wouldn't suggest to the
> > casual reader that there might be a problem.
>
> Okay. I added a sentence. What about this rewording:
>
> Force-threaded interrupt handlers used to run with interrupts enabled,
> something which could lead to deadlocks in case a threaded handler
> shared a lock with code running in hard interrupt context (e.g. timer
> callbacks) and did not explicitly disable interrupts.
>
> This was specifically the case for serial drivers that take the port
> lock in their console write path as printk can be called from hard
> interrupt context also with forced threading ("threadirqs").
>
> Since commit 81e2073c175b ("genirq: Disable interrupts for force
> threaded handlers") interrupt handlers always run with interrupts
> disabled on non-RT so that drivers no longer need to do handle this.
> RT is not affected by the referenced commit and the workaround, that is
> reverted, was not required because spinlock_t must not be acquired on
> RT in hardirq context.
>
> Drop the now obsolete workaround added by commit 33f16855dcb9 ("tty:
> serial: imx: fix potential deadlock").
This resolves my concerns. Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH] serial: imx: drop workaround for forced irq threading
Hello Johan,
On Tue, Mar 23, 2021 at 03:37:29PM +0100, Johan Hovold wrote:
> On Mon, Mar 22, 2021 at 02:40:32PM +0100, Uwe Kleine-König wrote:
> > On Mon, Mar 22, 2021 at 02:20:57PM +0100, Johan Hovold wrote:
> > > On Mon, Mar 22, 2021 at 12:55:36PM +0100, Uwe Kleine-König wrote:
> > > > On Mon, Mar 22, 2021 at 12:39:18PM +0100, Sebastian Andrzej Siewior
> > > > wrote:
> > > > > On 2021-03-22 12:34:02 [+0100], Uwe Kleine-König wrote:
> > > > > > On Mon, Mar 22, 2021 at 12:10:36PM +0100, Johan Hovold wrote:
> > > > > > > Force-threaded interrupt handlers used to run with interrupts
> > > > > > > enabled,
> > > > > > > something which could lead to deadlocks in case a threaded handler
> > > > > > > shared a lock with code running in hard interrupt context (e.g.
> > > > > > > timer
> > > > > > > callbacks) and did not explicitly disable interrupts.
> > > > > > >
> > > > > > > This was specifically the case for serial drivers that take the
> > > > > > > port
> > > > > > > lock in their console write path as printk can be called from hard
> > > > > > > interrupt context also with forced threading ("threadirqs").
> > > > > > >
> > > > > > > Since commit 81e2073c175b ("genirq: Disable interrupts for force
> > > > > > > threaded handlers") interrupt handlers always run with interrupts
> > > > > > > disabled on non-RT so that drivers no longer need to do handle
> > > > > > > this.
> > > > > >
> > > > > > So we're breaking RT knowingly here? If this is the case I'm not
> > > > > > happy
> > > > > > with your change. (And if RT is not affected a different wording
> > > > > > would
> > > > > > be good.)
> > > > >
> > > > > Which wording, could you be more specific? It looks good from here and
> > > > > no, RT is not affected.
> > > >
> > > > The commit log says essentially: "The change is fine on non-RT" which
> > > > suggests there is a problem on RT.
> > >
> > > I don't think you can read that into the commit message.
> >
> > From a strictly logically point of view you indeed cannot. But if you go
> > to the street and say to people there that they can park their car in
> > this street free of charge between Monday and Friday, I expect that most
> > of them will assume that they have to pay for parking on weekends.
>
> That analogy would almost seem to suggest bad intent on my side.
That analogy's purpose was to put over my point that writing
(paraphrased) "Since non-RT changed, this workaround isn't necessary any
more" suggests to me that the change might be bad for RT. So again,
there was no harm intended, this is just a call for clearing up either
the commit log to make it obvious the change is right or to fix the
problem on RT if there is any.
> To say that this workaround is no longer needed on !RT does not imply
> that it is needed on RT. If anything it suggests I have considered RT,
> I'd say.
The code in question was used for both RT and non-RT. You drop it for
both cases and only justify one of them. OK, fine, you considered both
cases. Just from reading the commit log I considered you didn't. It's
completely ok for me to be wrong here, but I still think the chosen
words are not optimal and stumbling as I did is easy. So I still see a
potential to improve the wording.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH] serial: imx: drop workaround for forced irq threading
Hello Sebastian, On Mon, Mar 22, 2021 at 09:48:36PM +0100, Sebastian Andrzej Siewior wrote: > On 2021-03-22 14:40:32 [+0100], Uwe Kleine-König wrote: > > From a strictly logically point of view you indeed cannot. But if you go > > to the street and say to people there that they can park their car in > > this street free of charge between Monday and Friday, I expect that most > > of them will assume that they have to pay for parking on weekends. > > If I hear that parking is free on weekdays and on paid on weekends, I > expect it to be a scam. I don't feel taken seriously with this reply. > Uwe, the patch reverts a change which was needed for !RT + threadirqs. This would be a useful information for the commit log. > The commit message claims that since the referenced commit "… interrupt > handlers always run with interrupts disabled on non-RT… ". This has > nothing to do with _this_ change. It argues why the workaround is not > needed. It argues why the work around is not needed on non-RT. It might be obvious for someone who is firm in the RT concepts, but IMHO commit logs should be understandable by and make sense for a wider audience than the deep experts. From what I know about RT "Force-threaded interrupt handlers used to run with interrupts enabled" still applies there. > If the referenced commit breaks RT then this is another story. I'm surprised to hear that from you. With the goal to get RT into mainline I would expect you to be happy if people consider the effects on RT in their reviews. > > So when you said that on on-RT the reason why it used to need a > > workaround is gone made me wonder what that implies for RT. > > There was never reason (or a lockdep splat) for it on RT. If so you > should have seen it, right? No, I don't consider myself to be an RT expert who is aware of all the problems. So I admit that for me the effect on RT of the patch under discussion isn't obvious. I just wonder that the change is justified with being OK on non-RT. So it's either bad that it breaks RT *or* improving the commit log would be great. And even if I had reason to believe that there is no problem with the commit on RT, I'd still wish that the commit log wouldn't suggest to the casual reader that there might be a problem. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v3 0/3] clk: provide new devm helpers for prepared and enabled clocks
Hello, On Mon, Mar 01, 2021 at 02:50:50PM +0100, Uwe Kleine-König wrote: > Uwe Kleine-König (3): > clk: generalize devm_clk_get() a bit > clk: Provide new devm_clk_helpers for prepared and enabled clocks > pwm: atmel: Simplify using devm_clk_get_prepared() > > drivers/clk/clk-devres.c | 96 +--- > drivers/pwm/pwm-atmel.c | 15 +-- > include/linux/clk.h | 87 +++- > 3 files changed, 168 insertions(+), 30 deletions(-) can I get some feedback on this series please? The idea is on the list since October last year with absolutely no maintainer feedback. I think it's a good idea and not too hard to review, so I wonder what is stopping you. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH] serial: imx: drop workaround for forced irq threading
Hello Johan,
On Mon, Mar 22, 2021 at 02:20:57PM +0100, Johan Hovold wrote:
> On Mon, Mar 22, 2021 at 12:55:36PM +0100, Uwe Kleine-König wrote:
> > On Mon, Mar 22, 2021 at 12:39:18PM +0100, Sebastian Andrzej Siewior wrote:
> > > On 2021-03-22 12:34:02 [+0100], Uwe Kleine-König wrote:
> > > > On Mon, Mar 22, 2021 at 12:10:36PM +0100, Johan Hovold wrote:
> > > > > Force-threaded interrupt handlers used to run with interrupts enabled,
> > > > > something which could lead to deadlocks in case a threaded handler
> > > > > shared a lock with code running in hard interrupt context (e.g. timer
> > > > > callbacks) and did not explicitly disable interrupts.
> > > > >
> > > > > This was specifically the case for serial drivers that take the port
> > > > > lock in their console write path as printk can be called from hard
> > > > > interrupt context also with forced threading ("threadirqs").
> > > > >
> > > > > Since commit 81e2073c175b ("genirq: Disable interrupts for force
> > > > > threaded handlers") interrupt handlers always run with interrupts
> > > > > disabled on non-RT so that drivers no longer need to do handle this.
> > > >
> > > > So we're breaking RT knowingly here? If this is the case I'm not happy
> > > > with your change. (And if RT is not affected a different wording would
> > > > be good.)
> > >
> > > Which wording, could you be more specific? It looks good from here and
> > > no, RT is not affected.
> >
> > The commit log says essentially: "The change is fine on non-RT" which
> > suggests there is a problem on RT.
>
> I don't think you can read that into the commit message.
From a strictly logically point of view you indeed cannot. But if you go
to the street and say to people there that they can park their car in
this street free of charge between Monday and Friday, I expect that most
of them will assume that they have to pay for parking on weekends.
So when you said that on on-RT the reason why it used to need a
workaround is gone made me wonder what that implies for RT.
> Why would I knowingly break RT?
No offence intended. Priorities are different for different maintainers
and I wouldn't bet that all maintainers care for RT.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout
Hello, On Mon, Mar 22, 2021 at 02:15:08PM +0200, Andy Shevchenko wrote: > On Mon, Mar 22, 2021 at 1:48 PM Uwe Kleine-König > wrote: > > On Mon, Mar 22, 2021 at 01:40:57PM +0200, Andy Shevchenko wrote: > > > On Mon, Mar 22, 2021 at 1:22 PM Uwe Kleine-König > > > wrote: > > > > When the PWM driver is loaded and the PWM configuration is invalid, it > > > > was already invalid for the time between power up (or warm start) and > > > > PWM driver load time. Then it doesn't really hurt to keep the PWM > > > > in this invalid state for a little moment longer until the consumer of > > > > the PWM becomes active. > > > > > > But this won't work in the cases when we have a chip with a shared > > > settings for period and/or duty cycle. You will never have a user come > > > due to -EBUSY. > > > > That's wrong, the first consumer to enable the PWM (in software) is > > supposed to be able to change the settings. > > If it's a critical PWM, how can you be allowed to do that? You seem to have a tight concept of a critical PWM. I don't, so I have problems following you. What is your picture about what is to be allowed/denied for a critical PWM? > And if so, what is the difference between resetting the device in this > case? The difference is that we have a consumer that knows what to do with the PWM then. > You may consider it as a change to the settings by the first > consumer. .. but without knowing if the first consumer is a backlight driver or a motor control it's hard to know if disabling the PWM is OK. So I like the concept of not doing anything until a process comes along that knows better. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH] serial: imx: drop workaround for forced irq threading
Hallo Sebastian,
On Mon, Mar 22, 2021 at 12:39:18PM +0100, Sebastian Andrzej Siewior wrote:
> On 2021-03-22 12:34:02 [+0100], Uwe Kleine-König wrote:
> > On Mon, Mar 22, 2021 at 12:10:36PM +0100, Johan Hovold wrote:
> > > Force-threaded interrupt handlers used to run with interrupts enabled,
> > > something which could lead to deadlocks in case a threaded handler
> > > shared a lock with code running in hard interrupt context (e.g. timer
> > > callbacks) and did not explicitly disable interrupts.
> > >
> > > This was specifically the case for serial drivers that take the port
> > > lock in their console write path as printk can be called from hard
> > > interrupt context also with forced threading ("threadirqs").
> > >
> > > Since commit 81e2073c175b ("genirq: Disable interrupts for force
> > > threaded handlers") interrupt handlers always run with interrupts
> > > disabled on non-RT so that drivers no longer need to do handle this.
> >
> > So we're breaking RT knowingly here? If this is the case I'm not happy
> > with your change. (And if RT is not affected a different wording would
> > be good.)
>
> Which wording, could you be more specific? It looks good from here and
> no, RT is not affected.
The commit log says essentially: "The change is fine on non-RT" which
suggests there is a problem on RT. So something like:
On non-RT interrupts are disabled also for force threaded handlers
(since commit 81e2073c175b ...). On RT there is no problem either
because ... So we don't need to handle this case in the driver any more.
would be preferable.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout
On Mon, Mar 22, 2021 at 01:40:57PM +0200, Andy Shevchenko wrote: > On Mon, Mar 22, 2021 at 1:22 PM Uwe Kleine-König > wrote: > > On Mon, Mar 22, 2021 at 11:38:40AM +0200, Andy Shevchenko wrote: > > > On Monday, March 22, 2021, Thierry Reding > > > wrote: > > > > On Fri, Jan 29, 2021 at 09:37:47PM +0100, Clemens Gruber wrote: > > > > > Thierry: Would you accept it if we continue to reset the registers in > > > > > .probe? > > > > > > > > Yes, I think it's fine to continue to reset the registers since that's > > > > basically what the driver already does. It'd be great if you could > > > > follow up with a patch that removes the reset and leaves the hardware in > > > > whatever state the bootloader has set up. Then we can take that patch > > > > for a ride and see if there are any complains about it breaking. If > > > > there are we can always try to fix them, but as a last resort we can > > > > also revert, which then may be something we have to live with. But I > > > > think we should at least try to make this consistent with how other > > > > drivers do this so that people don't stumble over this particular > > > > driver's > > > > > > I guess we may miss (a PCB / silicon design flaw or warm boot case) when > > > boot loader left device completely untouched and device either in wrong > > > state because if failed reset (saw this on PCA9555 which has a > > > corresponding errata), or simply we have done a warm reset of the system. > > > So, we also have to understand how to properly exit. > > > > I don't think that not resetting is a real problem. My argumentation > > goes as follows: > > > > When the PWM driver is loaded and the PWM configuration is invalid, it > > was already invalid for the time between power up (or warm start) and > > PWM driver load time. Then it doesn't really hurt to keep the PWM > > in this invalid state for a little moment longer until the consumer of > > the PWM becomes active. > > But this won't work in the cases when we have a chip with a shared > settings for period and/or duty cycle. You will never have a user come > due to -EBUSY. That's wrong, the first consumer to enable the PWM (in software) is supposed to be able to change the settings. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH] serial: imx: drop workaround for forced irq threading
On Mon, Mar 22, 2021 at 12:10:36PM +0100, Johan Hovold wrote:
> Force-threaded interrupt handlers used to run with interrupts enabled,
> something which could lead to deadlocks in case a threaded handler
> shared a lock with code running in hard interrupt context (e.g. timer
> callbacks) and did not explicitly disable interrupts.
>
> This was specifically the case for serial drivers that take the port
> lock in their console write path as printk can be called from hard
> interrupt context also with forced threading ("threadirqs").
>
> Since commit 81e2073c175b ("genirq: Disable interrupts for force
> threaded handlers") interrupt handlers always run with interrupts
> disabled on non-RT so that drivers no longer need to do handle this.
So we're breaking RT knowingly here? If this is the case I'm not happy
with your change. (And if RT is not affected a different wording would
be good.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout
Hello Andy, On Mon, Mar 22, 2021 at 11:38:40AM +0200, Andy Shevchenko wrote: > On Monday, March 22, 2021, Thierry Reding wrote: > > On Fri, Jan 29, 2021 at 09:37:47PM +0100, Clemens Gruber wrote: > > > Thierry: Would you accept it if we continue to reset the registers in > > > .probe? > > > > Yes, I think it's fine to continue to reset the registers since that's > > basically what the driver already does. It'd be great if you could > > follow up with a patch that removes the reset and leaves the hardware in > > whatever state the bootloader has set up. Then we can take that patch > > for a ride and see if there are any complains about it breaking. If > > there are we can always try to fix them, but as a last resort we can > > also revert, which then may be something we have to live with. But I > > think we should at least try to make this consistent with how other > > drivers do this so that people don't stumble over this particular > > driver's > > I guess we may miss (a PCB / silicon design flaw or warm boot case) when > boot loader left device completely untouched and device either in wrong > state because if failed reset (saw this on PCA9555 which has a > corresponding errata), or simply we have done a warm reset of the system. > So, we also have to understand how to properly exit. I don't think that not resetting is a real problem. My argumentation goes as follows: When the PWM driver is loaded and the PWM configuration is invalid, it was already invalid for the time between power up (or warm start) and PWM driver load time. Then it doesn't really hurt to keep the PWM in this invalid state for a little moment longer until the consumer of the PWM becomes active. Together with the use cases where not resetting is the right thing to do, I'm convinced not resetting is the better strategy. > Another point, CCF has a bit “is critical”, and u guess PWM may get the > same and make the all assumptions much easier. So I think complicating the PWM framework for this isn't the right thing to do. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v8 11/11] pwm: Add Raspberry Pi Firmware based PWM bus
Hello Nicolas, On Fri, Mar 12, 2021 at 01:24:54PM +0100, Nicolas Saenz Julienne wrote: > Adds support to control the PWM bus available in official Raspberry Pi > PoE HAT. Only RPi's co-processor has access to it, so commands have to > be sent through RPi's firmware mailbox interface. > > Signed-off-by: Nicolas Saenz Julienne > > --- > > Changes since v7: > - Remove unwarranted RPI_PWM_DEF_DUTY_REG usage > > Changes since v6: > - Use %pe > - Round divisions properly > - Use dev_err_probe() > - Pass check_patch > > Changes since v3: > - Rename compatible string to be more explicit WRT to bus's limitations > > Changes since v2: > - Use devm_rpi_firmware_get() > - Rename driver > - Small cleanups > > Changes since v1: > - Use default pwm bindings and get rid of xlate() function > - Correct spelling errors > - Correct apply() function > - Round values > - Fix divisions in arm32 mode > - Small cleanups > > drivers/pwm/Kconfig | 9 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-raspberrypi-poe.c | 206 ++ > 3 files changed, 216 insertions(+) > create mode 100644 drivers/pwm/pwm-raspberrypi-poe.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index a7a7a9f26aef..d3371ac7b871 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -431,6 +431,15 @@ config PWM_PXA > To compile this driver as a module, choose M here: the module > will be called pwm-pxa. > > +config PWM_RASPBERRYPI_POE > + tristate "Raspberry Pi Firwmware PoE Hat PWM support" > + # Make sure not 'y' when RASPBERRYPI_FIRMWARE is 'm'. This can only > + # happen when COMPILE_TEST=y, hence the added !RASPBERRYPI_FIRMWARE. > + depends on RASPBERRYPI_FIRMWARE || (COMPILE_TEST && > !RASPBERRYPI_FIRMWARE) > + help > + Enable Raspberry Pi firmware controller PWM bus used to control the > + official RPI PoE hat > + > config PWM_RCAR > tristate "Renesas R-Car PWM support" > depends on ARCH_RENESAS || COMPILE_TEST > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 4e35a55fa7b6..d3879619bd76 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -39,6 +39,7 @@ obj-$(CONFIG_PWM_NTXEC) += pwm-ntxec.o > obj-$(CONFIG_PWM_OMAP_DMTIMER) += pwm-omap-dmtimer.o > obj-$(CONFIG_PWM_PCA9685)+= pwm-pca9685.o > obj-$(CONFIG_PWM_PXA)+= pwm-pxa.o > +obj-$(CONFIG_PWM_RASPBERRYPI_POE)+= pwm-raspberrypi-poe.o > obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o > obj-$(CONFIG_PWM_RENESAS_TPU)+= pwm-renesas-tpu.o > obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o > diff --git a/drivers/pwm/pwm-raspberrypi-poe.c > b/drivers/pwm/pwm-raspberrypi-poe.c > new file mode 100644 > index ..71ade5e55069 > --- /dev/null > +++ b/drivers/pwm/pwm-raspberrypi-poe.c > @@ -0,0 +1,206 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright 2020 Nicolas Saenz Julienne 2021? > + * For more information on Raspberry Pi's PoE hat see: > + * https://www.raspberrypi.org/products/poe-hat/ Out of personal interest: Is this hat also able to power a RPi CM4? > + * Limitations: > + * - No disable bit, so a disabled PWM is simulated by duty_cycle 0 > + * - Only normal polarity > + * - Fixed 12.5 kHz period > + * > + * The current period is completed when HW is reconfigured. > + */ Other than that as mentioned in the previous round: This looks good, Reviewed-by: Uwe Kleine-König What is your thought about how to get this series merged? At least input, staging, armsoc, clk, reset anf firmware are touched. Do you prepare a branch for merging in the relevant trees (once you have all the necessary Acks)? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v7 11/11] pwm: Add Raspberry Pi Firmware based PWM bus
Hello Nicolas,
On Thu, Mar 11, 2021 at 02:01:00PM +0100, Nicolas Saenz Julienne wrote:
> On Wed, 2021-03-10 at 12:50 +0100, Uwe Kleine-König wrote:
> > On Mon, Jan 18, 2021 at 01:32:44PM +0100, Nicolas Saenz Julienne wrote:
>
> [...]
>
> > > + /*
> > > + * This sets the default duty cycle after resetting the board, we
> > > + * updated it every time to mimic Raspberry Pi's downstream's driver
> > > + * behaviour.
> > > + */
> > > + ret = raspberrypi_pwm_set_property(rpipwm->firmware,
> > > RPI_PWM_DEF_DUTY_REG,
> > > +duty_cycle);
> > > + if (ret) {
> > > + dev_err(chip->dev, "Failed to set default duty cycle: %pe\n",
> > > + ERR_PTR(ret));
> > > + return ret;
> >
> > This only has an effect for the next reboot, right?
>
> It effects all reboots until it's further changed.
>
> > If so I wonder if it is a good idea in general. (Think: The current PWM
> > setting enables a motor that makes a self-driving car move at 100 km/h.
> > Consider the rpi crashes, do I want to car to pick up driving 100 km/h at
> > power up even before Linux is up again?)
>
> I get your point. But this isn't used as a general purpose PWM. For now the
> interface is solely there to drive a PWM fan that's arguably harmless. This
> doesn't mean that the RPi foundation will not reuse the firmware interface for
> other means in the future. In such case we can always use a new DT compatible
> and bypass this feature (the current DT string is
> 'raspberrypi,firmware-poe-pwm', which is specific to this use-case).
>
> My aim here is to be on par feature wise with RPi's downstream implementation.
Just because the downstream kernel does it should not be the (single)
reason to do that. My gut feeling is: For a motor restoring the PWM
config on reboot is bad and for a fan it doesn't really hurt if it
doesn't restart automatically. So I'd prefer to to drop this feature.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v7 11/11] pwm: Add Raspberry Pi Firmware based PWM bus
Hello Nicolas,
On Mon, Jan 18, 2021 at 01:32:44PM +0100, Nicolas Saenz Julienne wrote:
> diff --git a/drivers/pwm/pwm-raspberrypi-poe.c
> b/drivers/pwm/pwm-raspberrypi-poe.c
> new file mode 100644
> index ..ca845e8fabe6
> --- /dev/null
> +++ b/drivers/pwm/pwm-raspberrypi-poe.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2020 Nicolas Saenz Julienne
> + * For more information on Raspberry Pi's PoE hat see:
> + * https://www.raspberrypi.org/products/poe-hat/
> + *
> + * Limitations:
> + * - No disable bit, so a disabled PWM is simulated by duty_cycle 0
> + * - Only normal polarity
> + * - Fixed 12.5 kHz period
> + *
> + * The current period is completed when HW is reconfigured.
nice.
> + */
> +
> [...]
> +static int raspberrypi_pwm_apply(struct pwm_chip *chip, struct pwm_device
> *pwm,
> + const struct pwm_state *state)
> +{
> + struct raspberrypi_pwm *rpipwm = raspberrypi_pwm_from_chip(chip);
> + unsigned int duty_cycle;
> + int ret;
> +
> + if (state->period < RPI_PWM_PERIOD_NS ||
> + state->polarity != PWM_POLARITY_NORMAL)
> + return -EINVAL;
> +
> + if (!state->enabled)
> + duty_cycle = 0;
> + else if (state->duty_cycle < RPI_PWM_PERIOD_NS)
> + duty_cycle = DIV_ROUND_DOWN_ULL(state->duty_cycle *
> RPI_PWM_MAX_DUTY,
> + RPI_PWM_PERIOD_NS);
> + else
> + duty_cycle = RPI_PWM_MAX_DUTY;
> +
> + if (duty_cycle == rpipwm->duty_cycle)
> + return 0;
> +
> + ret = raspberrypi_pwm_set_property(rpipwm->firmware,
> RPI_PWM_CUR_DUTY_REG,
> +duty_cycle);
> + if (ret) {
> + dev_err(chip->dev, "Failed to set duty cycle: %pe\n",
> + ERR_PTR(ret));
> + return ret;
> + }
> +
> + /*
> + * This sets the default duty cycle after resetting the board, we
> + * updated it every time to mimic Raspberry Pi's downstream's driver
> + * behaviour.
> + */
> + ret = raspberrypi_pwm_set_property(rpipwm->firmware,
> RPI_PWM_DEF_DUTY_REG,
> +duty_cycle);
> + if (ret) {
> + dev_err(chip->dev, "Failed to set default duty cycle: %pe\n",
> + ERR_PTR(ret));
> + return ret;
This only has an effect for the next reboot, right? If so I wonder if it
is a good idea in general. (Think: The current PWM setting enables a
motor that makes a self-driving car move at 100 km/h. Consider the rpi
crashes, do I want to car to pick up driving 100 km/h at power up even
before Linux is up again?) And if we agree it's a good idea: Should
raspberrypi_pwm_apply return 0 if setting the duty cycle succeeded and
only setting the default didn't?
Other than that the patch looks fine.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [GIT PULL] Immutable branch between MFD, PWM and RTC due for the v5.13 merge window
Hello Lee, On Tue, Mar 09, 2021 at 08:05:20PM +, Lee Jones wrote: > On Mon, 01 Mar 2021, Lee Jones wrote: > > > The following changes since commit fe07bfda2fb9cdef8a4d4008a409bb02f35f1bd8: > > > > Linux 5.12-rc1 (2021-02-28 16:05:19 -0800) > > > > are available in the Git repository at: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git > > ib-mfd-pwm-rtc-v5.13 > > > > for you to fetch changes up to 80629611215d1c5d52ed3cf723fd6d24a5872504: > > > > MAINTAINERS: Add entry for Netronix embedded controller (2021-03-01 > > 10:26:17 +) > > > > > > Immutable branch between MFD, PWM and RTC due for the v5.13 merge window > > > > > > [...] > > FYI, if anyone has pulled this, they should probably rebase it onto > v5.12-rc2 and delete the v5.12-rc1 tag from their tree: > > https://lwn.net/Articles/848431/ I'm not directly affected, but I wonder: The idea of an immutable branch is that the same history gets included in different trees. If now each maintainer rebases individually the result isn't the same history any more in each tree which somewhat defeats the idea of using immutable branches. IMHO there are two ways forward: Either someone (Lee again?) creates a new pull request for this series rebased on -rc2; or we accept that these few patches are based on -rc1. For the latter it would be beneficial to merge the tag into a tree that is already based on -rc2. Currently this branch makes it into next only via mfd[1]. A little bit of statistics for the interested: Between the broken commit 48d15436fde6 and its fix (caf6912f3f4a) there are 655 commits that are broken (git rev-list --ancestry-path 48d15436fde6..caf6912f3f4a | wc -l). We won't get rid of these. (Well unless Linus descides to rewrite history which would surprise me.) In current next (b01d57bfdc41c8f635b08b8a5af8a31217d46936) there are 3244 commits that include the broken commit 48d15436fde6 (git rev-list --ancestry-path 48d15436fde6..next/master | wc -l) and only 1411 of them also include the fix (git rev-list --ancestry-path caf6912f3f4a..next/master | wc -l). So next currently introduces 3244 - 1411 - 655 = 1178 additional broken commits. My feeling is that unless this number goes down considerably, we don't have to recourse to special measures to fix the 6 commits in this pull request and merging it based on -rc1-dontuse should be fine. A list of merges into next that contain the problematic commit but not its fix can be generated using git rev-list --merges --first-parent linus/master..next/master | while read c; do if git merge-base --is-ancestor 48d15436fde6 $c^2 && ! git merge-base --is-ancestor caf6912f3f4a $c^2; then git show -s --pretty=oneline "$c"; fi; done | nl It currently shows 37 merges. Best regards Uwe [1] git rev-list --merges --first-parent 80629611215d1c5d52ed3cf723fd6d24a5872504..next/master | while read c; do if git merge-base --is-ancestor 80629611215d1c5d52ed3cf723fd6d24a5872504 $c^2; then git show -s --pretty=oneline "$c"; fi; done -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v2] can: c_can: move runtime PM enable/disable to c_can_platform
On Mon, Mar 01, 2021 at 09:55:40PM -0500, Tong Zhang wrote: > Currently doing modprobe c_can_pci will make kernel complain > "Unbalanced pm_runtime_enable!", this is caused by pm_runtime_enable() > called before pm is initialized. > This fix is similar to 227619c3ff7c, move those pm_enable/disable code to > c_can_platform. I can confirm this makes the warning go away on a Congatec Atom board. I didn't do any further runtime tests. Tested-by: Uwe Kleine-König Thanks Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v3 1/2] pwm: sunxi: Add Allwinner SoC PWM controller driver
uty_ns;
> + c = DIV_ROUND_CLOSEST_ULL(c, period_ns);
> + active_cycles = c;
> + if (entire_cycles == 0)
> + entire_cycles++;
> +
> + /* config clk div_m*/
> + config = sun8i_pwm_readl(pc, PWM_GET_CLK_OFFSET(pwm->hwpwm));
> + config = SET_BITS(PWM_DIV_M, PWM_DIV_M_MASK, config, div_m);
> + sun8i_pwm_writel(pc, config, PWM_GET_CLK_OFFSET(pwm->hwpwm));
> +
> + /* config prescaler */
> + config = sun8i_pwm_readl(pc, PWM_CTL_REG(pwm->hwpwm));
> + config = SET_BITS(PWM_PRESCAL_K, PWM_PRESCAL_K_MASK, config, prescaler);
> + sun8i_pwm_writel(pc, config, PWM_CTL_REG(pwm->hwpwm));
> +
> + /* config active and period cycles */
> + config = sun8i_pwm_readl(pc, PWM_PERIOD_REG(pwm->hwpwm));
> + config = SET_BITS(PWM_ACT_CYCLE, PWM_ACT_CYCLE_MASK, config,
> active_cycles);
> + config = SET_BITS(PWM_ENTIRE_CYCLE, PWM_ENTIRE_CYCLE_MASK,
> + config, (entire_cycles - 1));
> + sun8i_pwm_writel(pc, config, PWM_PERIOD_REG(pwm->hwpwm));
> +
> + dev_dbg(chip->dev, "active_cycles=%lu entire_cycles=%lu prescaler=%u
> div_m=%u\n",
> +active_cycles, entire_cycles, prescaler, div_m);
I assume the output wave form starts to change as soon as you do the
first register write, so there might happen some glitches that need
mentioning in the Limitations section above.
> +
> +exit:
> + return ret;
> +}
> +
> +static void sun8i_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
> + bool enable)
> +{
> + struct sun8i_pwm_chip *pc = to_sun8i_pwm_chip(chip);
> + u32 clk, pwm_en;
> +
> + clk = sun8i_pwm_readl(pc, PWM_CLK_REG);
> + pwm_en = sun8i_pwm_readl(pc, PWM_ENABLE_REG);
> +
> + if (enable) {
> + clk |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> + sun8i_pwm_writel(pc, clk, PWM_CLK_REG);
> +
> + pwm_en |= BIT_CH(PWM_EN, pwm->hwpwm);
> + sun8i_pwm_writel(pc, pwm_en, PWM_ENABLE_REG);
> + } else {
> + pwm_en &= ~BIT_CH(PWM_EN, pwm->hwpwm);
> + sun8i_pwm_writel(pc, pwm_en, PWM_ENABLE_REG);
> +
> + clk &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> + sun8i_pwm_writel(pc, clk, PWM_CLK_REG);
> + }
> +}
> +
> +static int sun8i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct pwm_state curstate;
> + int ret;
> +
> + pwm_get_state(pwm, );
Please don't use pwm_get_state. There is a problem in the PWM framework
that might require that we add locking to these functions. Using
consumer functions in a lowlevel driver would probably deadlock then.
> + ret = sun8i_pwm_config(chip, pwm, state);
> +
> + if (state->polarity != curstate.polarity)
> + sun8i_pwm_set_polarity(chip, pwm, state->polarity);
> +
> + if (state->enabled != curstate.enabled)
> + sun8i_pwm_enable(chip, pwm, state->enabled);
If your PWM is enabled and the consumer calls:
pwm_apply_state(mypwm, { .enabled = false, .period = someperiod,
.duty_cycle = someduty});
the hardware might emit someperiod,someduty for a moment, right? Please
ensure that in this case you disable first.
> + return ret;
> +}
> +
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH 40/44] tty: hvc, drop unneeded forward declarations
Hello Jiri, On Tue, Mar 02, 2021 at 07:22:10AM +0100, Jiri Slaby wrote: > Forward declarations make the code larger and rewrites harder. Harder as > they are often omitted from global changes. Remove forward declarations > which are not really needed, i.e. the definition of the function is > before its first use. > > Signed-off-by: Jiri Slaby > Cc: linuxppc-...@lists.ozlabs.org > --- > drivers/tty/hvc/hvcs.c | 25 - note this conflicts with commit 386a966f5ce71a0364b158c5d0a6971f4e418ea8 that currently sits in the powerpc tree. I think it's easy to resolve. Other than that: Acked-by: Uwe Kleine-König Best regards Uwe > 1 file changed, 25 deletions(-) > > diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c > index c90848919644..0b89d878a108 100644 > --- a/drivers/tty/hvc/hvcs.c > +++ b/drivers/tty/hvc/hvcs.c > @@ -290,36 +290,11 @@ static LIST_HEAD(hvcs_structs); > static DEFINE_SPINLOCK(hvcs_structs_lock); > static DEFINE_MUTEX(hvcs_init_mutex); > > -static void hvcs_unthrottle(struct tty_struct *tty); > -static void hvcs_throttle(struct tty_struct *tty); > -static irqreturn_t hvcs_handle_interrupt(int irq, void *dev_instance); > - > -static int hvcs_write(struct tty_struct *tty, > - const unsigned char *buf, int count); > -static int hvcs_write_room(struct tty_struct *tty); > -static int hvcs_chars_in_buffer(struct tty_struct *tty); > - > -static int hvcs_has_pi(struct hvcs_struct *hvcsd); > -static void hvcs_set_pi(struct hvcs_partner_info *pi, > - struct hvcs_struct *hvcsd); > static int hvcs_get_pi(struct hvcs_struct *hvcsd); > static int hvcs_rescan_devices_list(void); > > -static int hvcs_partner_connect(struct hvcs_struct *hvcsd); > static void hvcs_partner_free(struct hvcs_struct *hvcsd); > > -static int hvcs_enable_device(struct hvcs_struct *hvcsd, > - uint32_t unit_address, unsigned int irq, struct vio_dev *dev); > - > -static int hvcs_open(struct tty_struct *tty, struct file *filp); > -static void hvcs_close(struct tty_struct *tty, struct file *filp); > -static void hvcs_hangup(struct tty_struct * tty); > - > -static int hvcs_probe(struct vio_dev *dev, > - const struct vio_device_id *id); > -static int hvcs_remove(struct vio_dev *dev); > -static int __init hvcs_module_init(void); > -static void __exit hvcs_module_exit(void); > static int hvcs_initialize(void); > > #define HVCS_SCHED_READ 0x0001 > -- > 2.30.1 > > -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: linux-next: build failure after merge of the powerpc-fixes tree
Hello,
On 3/2/21 3:09 AM, Michael Ellerman wrote:
Stephen Rothwell writes:
Hi all,
After merging the powerpc-fixes tree, today's linux-next build (powerpc
allyesconfig) failed like this:
drivers/net/ethernet/ibm/ibmvnic.c:5399:13: error: conflicting types for
'ibmvnic_remove'
5399 | static void ibmvnic_remove(struct vio_dev *dev)
| ^~
drivers/net/ethernet/ibm/ibmvnic.c:81:12: note: previous declaration of
'ibmvnic_remove' was here
81 | static int ibmvnic_remove(struct vio_dev *);
|^~
Caused by commit
1bdd1e6f9320 ("vio: make remove callback return void")
Gah, is IBMVNIC in any of our defconfigs?! ... no it's not.
Would you accept a patch to add the driver to one of the defconfigs as
an excuse for the build breakage I created? Which would be appropriate?
ppc64_defconfig?
I have applied the following patch for today:
Thanks, I'll squash it in.
Also thanks for catching to Stephen and to Michael for the fixup.
Best regards
Uwe
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH 10/44] tty: imx, use ms_to_ktime
Hello Jiri,
On Tue, Mar 02, 2021 at 07:21:40AM +0100, Jiri Slaby wrote:
> This really eliminates multiplications from the assembly. I would have
> thought they are optimized by inlining ktime_set, but not on x86_64 with
> gcc 10.
>
> Signed-off-by: Jiri Slaby
> Cc: Shawn Guo
> Cc: Sascha Hauer
> Cc: Pengutronix Kernel Team
> Cc: Fabio Estevam
> Cc: NXP Linux Team
> ---
> drivers/tty/serial/imx.c | 6 +-
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 8257597d034d..3f69356937ef 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -394,11 +394,7 @@ static void imx_uart_rts_inactive(struct imx_port
> *sport, u32 *ucr2)
>
> static void start_hrtimer_ms(struct hrtimer *hrt, unsigned long msec)
> {
> - long sec = msec / MSEC_PER_SEC;
> - long nsec = (msec % MSEC_PER_SEC) * 100;
> - ktime_t t = ktime_set(sec, nsec);
> -
> - hrtimer_start(hrt, t, HRTIMER_MODE_REL);
> + hrtimer_start(hrt, ms_to_ktime(msec), HRTIMER_MODE_REL);
> }
What about:
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 8257597d034d..8731d4f82cb8 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -392,15 +392,6 @@ static void imx_uart_rts_inactive(struct imx_port *sport,
u32 *ucr2)
mctrl_gpio_set(sport->gpios, sport->port.mctrl);
}
-static void start_hrtimer_ms(struct hrtimer *hrt, unsigned long msec)
-{
- long sec = msec / MSEC_PER_SEC;
- long nsec = (msec % MSEC_PER_SEC) * 100;
- ktime_t t = ktime_set(sec, nsec);
-
- hrtimer_start(hrt, t, HRTIMER_MODE_REL);
-}
-
/* called with port.lock taken and irqs off */
static void imx_uart_start_rx(struct uart_port *port)
{
@@ -457,8 +448,9 @@ static void imx_uart_stop_tx(struct uart_port *port)
if (port->rs485.flags & SER_RS485_ENABLED) {
if (sport->tx_state == SEND) {
sport->tx_state = WAIT_AFTER_SEND;
- start_hrtimer_ms(>trigger_stop_tx,
-port->rs485.delay_rts_after_send);
+ hrtimer_start(>trigger_stop_tx,
+
ms_to_ktime(port->rs485.delay_rts_after_send),
+ HRTIMER_MODE_REL);
return;
}
@@ -697,8 +689,9 @@ static void imx_uart_start_tx(struct uart_port *port)
imx_uart_stop_rx(port);
sport->tx_state = WAIT_AFTER_RTS;
- start_hrtimer_ms(>trigger_start_tx,
-port->rs485.delay_rts_before_send);
+ hrtimer_start(>trigger_start_tx,
+
ms_to_ktime(port->rs485.delay_rts_before_send),
+ HRTIMER_MODE_REL);
return;
}
instead?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout
Hello, On Mon, Feb 01, 2021 at 06:24:02PM +0100, Clemens Gruber wrote: > Hi Sven, Thierry, Uwe, > > On Fri, Jan 29, 2021 at 05:16:51PM -0500, Sven Van Asbroeck wrote: > > Hi Clemens, > > > > On Fri, Jan 29, 2021 at 4:24 PM Sven Van Asbroeck > > wrote: > > > > > > LEN_ON = 409, LED_OFF = 1228 and > > > LED_ON = 419, LED_OFF = 1238 > > > produce the same result. you can't see the difference between the two > > > when scoping the channel. there are probably more ways to do this, > > > some might surprise us. It's a tricky chip. > > > > Please ignore this example, it's bogus. In my defence, it's a Friday > > afternoon here :) > > Happens to the best of us :) > > > > > But consider the following: imagine the bootloader has enabled a few > > pwm channels, and the driver's .probe() has left them on/unchanged. > > Then the user enables another pwm channel, and tries to change the > > period/prescaler. How would pca9685_may_change_prescaler() know > > if changing the prescaler is allowed? > > > > And the following: imagine the bootloader has enabled a few > > pwm channels, and the driver's .probe() has left them on/unchanged. > > After .probe(), the runtime_pm will immediately put the chip to sleep, > > because it's unaware that some channels are alive. > > (We could read out the state in .probe. If a pwm is already enabled by > the bootloader, then the user can't change the period. Also, the chip > would not be put to sleep. > > The user then can export channels and see if they are enabled. If he > wants to change the period, he needs to find the one enabled by the > bootloader and change the period there, before he requests more. > If the bootloader enabled more than one, then he has to disable all but > one to change the period. > > Or did I miss something?) > > > > > I'm sure I'm overlooking a few complications here. probe not changing > > the existing configuration, will add a lot of complexity to the driver. > > I'm not saying this is necessarily bad, just a tradeoff. Or, a management > > decision. > > But I agree that it is simpler if we keep the resets in probe. It would > also avoid a potentially breaking change for users that do not reset > their pca9685 chips in their bootloader code. I would prefer to drop the reset. If the bootloader left with an invalid state, this is active for sure until the PWM driver is loaded. If you don't reset, the time is extended (usually) until the consumer comes along and corrects the setting. So the downside of not resetting is quite limited, but if you disable the PWM in .probe() the effect can be worse. And consistency dictates to not reset. > Removing the resets could then be left as something to discuss further > in the future and something that belongs in a separate patch series? That would be fine for me, too. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v5 4/7] pwm: pca9685: Reset registers to POR state in probe
Hello Clemens, On Tue, Dec 15, 2020 at 10:22:25PM +0100, Clemens Gruber wrote: > Reset the prescale and ON/OFF registers to their POR default state in > the probe function. Otherwise, the PWMs could still be active after a > watchdog reset and reboot, etc. My memories are swapped out because it's already so long ago I looked into this series. I thought it was already said that taking over the configured state in probe is the nice thing to do?! Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v5 1/7] pwm: pca9685: Switch to atomic API
Hello Clemens, On Tue, Dec 15, 2020 at 10:22:22PM +0100, Clemens Gruber wrote: > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -EOPNOTSUPP; We agreed on -EINVAL for that one since 2b1c1a5d5148. Other than that the patch looks ok (but note I only looked quickly). Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v5 1/7] pwm: pca9685: Switch to atomic API
On Thu, Dec 17, 2020 at 12:10:10PM -0500, Sven Van Asbroeck wrote: > On Thu, Dec 17, 2020 at 11:48 AM Clemens Gruber > wrote: > > > > I can initialize the values to 0 of course and check the file for other > > places with missing initializations. > > > > Or would it be better to check the return codes of regmap_read/write in > > such cases? I'm not sure. > > I think that checking the regmap_read/write return values is overkill > in this driver. These functions can't realistically fail, except if the i2c > bus is bad, i.e. h/w failure or intermittency. And that's an externality > which I believe we can ignore. > > Maybe Thierry or Uwe have further insights here. I'm a fan of full checking, but I'm not sure what's Thierry's position on that. My reasoning is: If the bus is bad and a request to modify the PWM fails because of that, the PWM consumer probably wants to know. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
[PATCH] pcmcia: ds: Remove if with always false condition
pcmcia_device_remove() is only ever called by the driver core with dev->driver pointing to a valid driver. (And even if dev->driver was NULL, p_drv wouldn't be NULL as p_drv is assigned as follows: p_drv = to_pcmcia_drv(dev->driver); and to_pcmcia_drv is a container_of operation on struct pcmcia_driver::drv which isn't the first member in struct pcmcia_driver.) Signed-off-by: Uwe Kleine-König --- drivers/pcmcia/ds.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/pcmcia/ds.c b/drivers/pcmcia/ds.c index 72114907c0e4..bb096a3b76aa 100644 --- a/drivers/pcmcia/ds.c +++ b/drivers/pcmcia/ds.c @@ -371,9 +371,6 @@ static int pcmcia_device_remove(struct device *dev) pcmcia_card_remove(p_dev->socket, p_dev); /* detach the "instance" */ - if (!p_drv) - return 0; - if (p_drv->remove) p_drv->remove(p_dev); -- 2.30.0
[PATCH v3 3/3] pwm: atmel: Simplify using devm_clk_get_prepared()
With devm_clk_get_prepared() caring to unprepare the clock the error
path and remove callback can be simplified accordingly.
Signed-off-by: Uwe Kleine-König
---
drivers/pwm/pwm-atmel.c | 15 ++-
1 file changed, 2 insertions(+), 13 deletions(-)
diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index 5813339b597b..d65e23da2582 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -415,16 +415,10 @@ static int atmel_pwm_probe(struct platform_device *pdev)
if (IS_ERR(atmel_pwm->base))
return PTR_ERR(atmel_pwm->base);
- atmel_pwm->clk = devm_clk_get(>dev, NULL);
+ atmel_pwm->clk = devm_clk_get_prepared(>dev, NULL);
if (IS_ERR(atmel_pwm->clk))
return PTR_ERR(atmel_pwm->clk);
- ret = clk_prepare(atmel_pwm->clk);
- if (ret) {
- dev_err(>dev, "failed to prepare PWM clock\n");
- return ret;
- }
-
atmel_pwm->chip.dev = >dev;
atmel_pwm->chip.ops = _pwm_ops;
atmel_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
@@ -435,23 +429,18 @@ static int atmel_pwm_probe(struct platform_device *pdev)
ret = pwmchip_add(_pwm->chip);
if (ret < 0) {
dev_err(>dev, "failed to add PWM chip %d\n", ret);
- goto unprepare_clk;
+ return ret;
}
platform_set_drvdata(pdev, atmel_pwm);
return ret;
-
-unprepare_clk:
- clk_unprepare(atmel_pwm->clk);
- return ret;
}
static int atmel_pwm_remove(struct platform_device *pdev)
{
struct atmel_pwm_chip *atmel_pwm = platform_get_drvdata(pdev);
- clk_unprepare(atmel_pwm->clk);
mutex_destroy(_pwm->isr_lock);
return pwmchip_remove(_pwm->chip);
--
2.30.0
[PATCH v3 0/3] clk: provide new devm helpers for prepared and enabled clocks
Hello,
this is a brown paper bag version (aka v3) of my series adding
devm_clk_get_enabled() et al.
Changes since v2 (sent with Message-Id:
20210301110821.1445756-1-...@kleine-koenig.org):
- send it from the right email account to have the sender matching the
SoB line
- make __devm_clk_get static
- fix EXPORT_SYMBOL usage
The last two were found by the kernel test robot.
Range-diff can be found below.
Best regards
Uwe
Uwe Kleine-König (3):
clk: generalize devm_clk_get() a bit
clk: Provide new devm_clk_helpers for prepared and enabled clocks
pwm: atmel: Simplify using devm_clk_get_prepared()
drivers/clk/clk-devres.c | 96 +---
drivers/pwm/pwm-atmel.c | 15 +--
include/linux/clk.h | 87 +++-
3 files changed, 168 insertions(+), 30 deletions(-)
Range-diff against v2:
1: 7203dc0837af ! 1: 3faadae49fed clk: generalize devm_clk_get a bit
@@ drivers/clk/clk-devres.c
}
-struct clk *devm_clk_get(struct device *dev, const char *id)
-+struct clk *__devm_clk_get(struct device *dev, const char *id,
-+ struct clk *(*get)(struct device *dev, const char
*id),
-+ int (*init)(struct clk *clk),
-+ void (*exit)(struct clk *clk))
++static struct clk *__devm_clk_get(struct device *dev, const char *id,
++struct clk *(*get)(struct device *dev, const
char *id),
++int (*init)(struct clk *clk),
++void (*exit)(struct clk *clk))
{
- struct clk **ptr, *clk;
+ struct devm_clk_state *state;
2: 4d2107992b8c ! 2: 82005b4a9ea1 clk: Provide new devm_clk_helpers for
prepared and enabled clocks
@@ drivers/clk/clk-devres.c: struct clk *devm_clk_get(struct device *dev,
const cha
+clk_prepare_enable, clk_disable_unprepare);
+
+}
-+EXPORT_SYMBOL(devm_clk_get_prepared);
++EXPORT_SYMBOL(devm_clk_get_enabled);
+
struct clk *devm_clk_get_optional(struct device *dev, const char *id)
{
@@ drivers/clk/clk-devres.c: struct clk *devm_clk_get(struct device *dev,
const cha
+clk_prepare_enable, clk_disable_unprepare);
+
+}
-+EXPORT_SYMBOL(devm_clk_get_optional_prepared);
++EXPORT_SYMBOL(devm_clk_get_optional_enabled);
+
struct clk_bulk_devres {
struct clk_bulk_data *clks;
3: 63f799a4ff32 = 3: 1f73d17d4da7 pwm: atmel: Simplify using
devm_clk_get_prepared()
base-commit: fe07bfda2fb9cdef8a4d4008a409bb02f35f1bd8
--
2.30.0
[PATCH v3 1/3] clk: generalize devm_clk_get() a bit
Allow to add an exit hook to devm managed clocks. Also use
clk_get_optional() in devm_clk_get_optional instead of open coding it.
The generalisation will be used in the next commit to add some more
devm_clk helpers.
Signed-off-by: Uwe Kleine-König
---
drivers/clk/clk-devres.c | 67 ++--
1 file changed, 50 insertions(+), 17 deletions(-)
diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index be160764911b..91c995815b57 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -4,39 +4,72 @@
#include
#include
+struct devm_clk_state {
+ struct clk *clk;
+ void (*exit)(struct clk *clk);
+};
+
static void devm_clk_release(struct device *dev, void *res)
{
- clk_put(*(struct clk **)res);
+ struct devm_clk_state *state = *(struct devm_clk_state **)res;
+
+ if (state->exit)
+ state->exit(state->clk);
+
+ clk_put(state->clk);
}
-struct clk *devm_clk_get(struct device *dev, const char *id)
+static struct clk *__devm_clk_get(struct device *dev, const char *id,
+ struct clk *(*get)(struct device *dev, const
char *id),
+ int (*init)(struct clk *clk),
+ void (*exit)(struct clk *clk))
{
- struct clk **ptr, *clk;
+ struct devm_clk_state *state;
+ struct clk *clk;
+ int ret;
- ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL);
- if (!ptr)
+ state = devres_alloc(devm_clk_release, sizeof(*state), GFP_KERNEL);
+ if (!state)
return ERR_PTR(-ENOMEM);
- clk = clk_get(dev, id);
- if (!IS_ERR(clk)) {
- *ptr = clk;
- devres_add(dev, ptr);
- } else {
- devres_free(ptr);
+ clk = get(dev, id);
+ if (IS_ERR(clk)) {
+ ret = PTR_ERR(clk);
+ goto err_clk_get;
}
+ if (init) {
+ ret = init(clk);
+ if (ret)
+ goto err_clk_init;
+ }
+
+ state->clk = clk;
+ state->exit = exit;
+
+ devres_add(dev, state);
+
return clk;
+
+err_clk_init:
+
+ clk_put(clk);
+err_clk_get:
+
+ devres_free(state);
+ return ERR_PTR(ret);
}
-EXPORT_SYMBOL(devm_clk_get);
-struct clk *devm_clk_get_optional(struct device *dev, const char *id)
+struct clk *devm_clk_get(struct device *dev, const char *id)
{
- struct clk *clk = devm_clk_get(dev, id);
+ return __devm_clk_get(dev, id, clk_get, NULL, NULL);
- if (clk == ERR_PTR(-ENOENT))
- return NULL;
+}
+EXPORT_SYMBOL(devm_clk_get);
- return clk;
+struct clk *devm_clk_get_optional(struct device *dev, const char *id)
+{
+ return __devm_clk_get(dev, id, clk_get_optional, NULL, NULL);
}
EXPORT_SYMBOL(devm_clk_get_optional);
--
2.30.0
[PATCH v3 2/3] clk: Provide new devm_clk_helpers for prepared and enabled clocks
When a driver keeps a clock prepared (or enabled) during the whole
lifetime of the driver, these helpers allow to simplify the drivers.
Signed-off-by: Uwe Kleine-König
---
drivers/clk/clk-devres.c | 31 ++
include/linux/clk.h | 87 +++-
2 files changed, 117 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 91c995815b57..b54f7f0f2a35 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -67,12 +67,43 @@ struct clk *devm_clk_get(struct device *dev, const char *id)
}
EXPORT_SYMBOL(devm_clk_get);
+struct clk *devm_clk_get_prepared(struct device *dev, const char *id)
+{
+ return __devm_clk_get(dev, id, clk_get, clk_prepare, clk_unprepare);
+
+}
+EXPORT_SYMBOL(devm_clk_get_prepared);
+
+struct clk *devm_clk_get_enabled(struct device *dev, const char *id)
+{
+ return __devm_clk_get(dev, id, clk_get,
+ clk_prepare_enable, clk_disable_unprepare);
+
+}
+EXPORT_SYMBOL(devm_clk_get_enabled);
+
struct clk *devm_clk_get_optional(struct device *dev, const char *id)
{
return __devm_clk_get(dev, id, clk_get_optional, NULL, NULL);
}
EXPORT_SYMBOL(devm_clk_get_optional);
+struct clk *devm_clk_get_optional_prepared(struct device *dev, const char *id)
+{
+ return __devm_clk_get(dev, id, clk_get_optional,
+ clk_prepare, clk_unprepare);
+
+}
+EXPORT_SYMBOL(devm_clk_get_optional_prepared);
+
+struct clk *devm_clk_get_optional_enabled(struct device *dev, const char *id)
+{
+ return __devm_clk_get(dev, id, clk_get_optional,
+ clk_prepare_enable, clk_disable_unprepare);
+
+}
+EXPORT_SYMBOL(devm_clk_get_optional_enabled);
+
struct clk_bulk_devres {
struct clk_bulk_data *clks;
int num_clks;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 266e8de3cb51..b3c5da388b08 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -449,7 +449,7 @@ int __must_check devm_clk_bulk_get_all(struct device *dev,
* the clock producer. (IOW, @id may be identical strings, but
* clk_get may return different clock producers depending on @dev.)
*
- * Drivers must assume that the clock source is not enabled.
+ * Drivers must assume that the clock source is neither prepared nor enabled.
*
* devm_clk_get should not be called from within interrupt context.
*
@@ -458,6 +458,47 @@ int __must_check devm_clk_bulk_get_all(struct device *dev,
*/
struct clk *devm_clk_get(struct device *dev, const char *id);
+/**
+ * devm_clk_get_prepared - devm_clk_get() + clk_prepare()
+ * @dev: device for clock "consumer"
+ * @id: clock consumer ID
+ *
+ * Returns a struct clk corresponding to the clock producer, or
+ * valid IS_ERR() condition containing errno. The implementation
+ * uses @dev and @id to determine the clock consumer, and thereby
+ * the clock producer. (IOW, @id may be identical strings, but
+ * clk_get may return different clock producers depending on @dev.)
+ *
+ * The returned clk (if valid) is prepared. Drivers must however assume that
the
+ * clock is not enabled.
+ *
+ * devm_clk_get_prepared should not be called from within interrupt context.
+ *
+ * The clock will automatically be unprepared and freed when the
+ * device is unbound from the bus.
+ */
+struct clk *devm_clk_get_prepared(struct device *dev, const char *id);
+
+/**
+ * devm_clk_get_enabled - devm_clk_get() + clk_prepare_enable()
+ * @dev: device for clock "consumer"
+ * @id: clock consumer ID
+ *
+ * Returns a struct clk corresponding to the clock producer, or
+ * valid IS_ERR() condition containing errno. The implementation
+ * uses @dev and @id to determine the clock consumer, and thereby
+ * the clock producer. (IOW, @id may be identical strings, but
+ * clk_get may return different clock producers depending on @dev.)
+ *
+ * The returned clk (if valid) is prepared and enabled.
+ *
+ * devm_clk_get_prepared should not be called from within interrupt context.
+ *
+ * The clock will automatically be disabled, unprepared and freed when the
+ * device is unbound from the bus.
+ */
+struct clk *devm_clk_get_enabled(struct device *dev, const char *id);
+
/**
* devm_clk_get_optional - lookup and obtain a managed reference to an optional
*clock producer.
@@ -469,6 +510,26 @@ struct clk *devm_clk_get(struct device *dev, const char
*id);
*/
struct clk *devm_clk_get_optional(struct device *dev, const char *id);
+/**
+ * devm_clk_get_optional_prepared - devm_clk_get_optional() + clk_prepare()
+ * @dev: device for clock "consumer"
+ * @id: clock consumer ID
+ *
+ * Behaves the same as devm_clk_get_prepared() except where there is no clock
producer.
+ * In this case, instead of returning -ENOENT, the function returns NULL.
+ */
+struct clk *devm_clk_get_optional_prepared(struct device *dev, const char *id);
+
+/**
+ *
