[PATCH v8 2/4] pwm: sun8i: Add H616 PWM support
James Hilliard
james.hilliard1 at gmail.com
Wed Sep 23 01:59:49 PDT 2026
On Wed, Sep 23, 2026 at 2:52 AM Richard GENOUD
<richard.genoud at bootlin.com> wrote:
>
> Le 23/09/2026 à 10:01, James Hilliard a écrit :
> > On Wed, Sep 23, 2026 at 1:16 AM Richard GENOUD
> > <richard.genoud at bootlin.com> wrote:
> >>
> >> Le 22/09/2026 à 17:46, James Hilliard a écrit :
> >>> On Tue, Sep 22, 2026 at 9:13 AM Richard GENOUD
> >>> <richard.genoud at bootlin.com> wrote:
> >>>>
> >>>> Le 22/09/2026 à 02:27, James Hilliard a écrit :
> >>>>> On Mon, Sep 21, 2026 at 10:28 AM Uwe Kleine-König <ukleinek at kernel.org> wrote:
> >>>>>>
> >>>>>> On Tue, Aug 04, 2026 at 03:27:25PM -0600, James Hilliard wrote:
> >>>>>>> From: Richard Genoud <richard.genoud at bootlin.com>
> >>>>>>>
> >>>>>>> Add a driver for the Allwinner H616 PWM controller, supporting six
> >>>>>>> channels. Each channel output can carry either a PWM waveform or a clock
> >>>>>>> from its bypass path.
> >>>>>>>
> >>>>>>> The channels are paired as 0/1, 2/3 and 4/5. Each pair shares a mux, gate
> >>>>>>> and prescaler, while each channel has its own prescaler and bypass.
> >>>>>>>
> >>>>>>> Register each shared pair mux, divider and gate with the Common Clock
> >>>>>>> Framework, and expose each channel bypass as a clock output. Program the
> >>>>>>> channel-specific divider directly in the PWM path. Arbitrate each output
> >>>>>>> when the PWM is requested or the bypass clock is prepared, and hold the
> >>>>>>> shared pair rate exclusively while either sibling output is active.
> >>>>>>>
> >>>>>>> CCF reference counts do not describe outputs left active by firmware.
> >>>>>>> Before disabling a pair gate or changing its rate, also inspect the
> >>>>>>> hardware channel-enable bits. Preserve inherited outputs through generic
> >>>>>>> unused-clock cleanup so declared consumers can claim them. At driver
> >>>>>>> sync_state(), stop channels which remain unowned and reconcile an empty
> >>>>>>> shared pair gate through CCF. This prevents both premature shutdown of a
> >>>>>>> late-probing consumer and indefinite unclaimed outputs.
> >>>>>>>
> >>>>>>> The clock-provider interface is needed because pwm-clock cannot accurately
> >>>>>>> represent the bypass path. For example, pwm-clock represents 24 MHz as an
> >>>>>>> integer 42 ns PWM period. The PWM waveform-rounding rules happen to select
> >>>>>>> the H616 bypass and produce 24 MHz for that request, but the pwm-clock API
> >>>>>>> does not require its advertised clock rate to equal the rounded hardware
> >>>>>>> waveform.
> >>>>>>
> >>>>>> I don't understand the issue here. When you write pwm-clock, do you mean
> >>>>>> drivers/clk/clk-pwm.c or drivers/pwm/pwm-clk.c? (I guess the former.)
> >>>>>
> >>>>> Yes, drivers/clk/clk-pwm.c. You're right that the 42 ns period, 21 ns duty
> >>>>> request selects the 24 MHz bypass when that rate is available.
> >>>>>
> >>>>
> >>>> I've played a bit with the v9, and indeed the 24MHz bypass is selected
> >>>> when the requested period in between 42ns and 49ns (23.8MHz and 20.4Mhz)
> >>>> So, for those periods, we only have a access to 0%, 50% and 100% duty.
> >>>> And for the periods between 20ns to 41ns, the 100MHz clock is selected
> >>>> without bypass:
> >>>> - 20ns->29ns: 2 period cycles, pwm prd=50MHz (so, 0% and 100% duty)
> >>>> - 30ns->39ns: 3 period cycles, pwm prd=33MHz (so, 0%, 50% and 100% duty)
> >>>> - 40ns->41ns: 4 period cycles, pwm prd=25MHz (so, 0%, 33%, 66% and 100%
> >>>> duty)
> >>>> - 42ns->49ns: 24MHz bypass (so 0%, 50% and 100% duty)
> >>>> Then, the 100MHz clock is selected again:
> >>>> - 50ns->59ns: 5 period cycles, pwm prd=20MHz (so, 0%, 25%, 50%, 75 and
> >>>> 100% duty)
> >>>> and so on.
> >>>>
> >>>> It seems a little bit strange to have a 24MHz clock with only 3 duty
> >>>> steps available between 42ns and 49ns.
> >>>
> >>> Yes, I reproduce those three output levels with the v9 rounding helpers.
> >>> The 50% output uses bypass; constant low and high use cycle mode.
> >>>
> >>> However, removing bypass candidates alone doesn't make the driver choose
> >>> the 40 ns alternative. A single 24 MHz cycle still rounds to 42 ns and
> >>> wins on period, leaving just constant low/high. The low resolution here
> >>> is therefore not solely caused by considering bypass.
> >>>
> >>>> We have the same behavior at each bypass, for example at 12MHz (24MHz
> >>>> and /2 prescaler):
> >>>> for periods 84ns to 89ns, we have the 24MHz/2 bypass selected, with only
> >>>> 3 duty steps.
> >>>> (for 83ns, we have the 100MHz clock and 8 period cycles)
> >>>> same for 10667ns->10669ns (24MHz/256) => 3 duty steps available
> >>>> whereas at 10666ns, we have 1066 duty steps available with the 100MHz clock.
> >>>
> >>> I don't reproduce the three-level limit in that case when the sibling
> >>> doesn't constrain the pair. Ordinary PWM at 24 MHz with 256 period ticks
> >>> has the same rounded 10667 ns period and provides 257 duty levels,
> >>> including the two constant levels. For example, 10667/2667 ns and
> >>> 10667/8000 ns both select cycle mode and round exactly in integer ns.
> >> Indeed, my bad.
> >> At those steps, the bypass is not selected, the drivers switches to the
> >> 24MHz clock. It's selected only between 42ns and 49ns
> >>
> >>>
> >>> Bypass can win a tie for a 50% request if the pair is already at 93750 Hz,
> >>> but the mode and pair rate are reconsidered for the next duty request.
> >>> Was the sibling holding the pair rate in your test?
> >>>
> >>> Also, the short-period duty tables seem off by one: the period register
> >>> stores ticks minus one. Two 100 MHz ticks allow 0%, 50% and 100%; three
> >>> allow 0%, 1/3, 2/3 and 100%.
> >> yes, so we have:
> >> - 20ns->29ns: 2 period cycles, pwm prd=50MHz (so, 0%, 50% and 100% duty)
> >> - 30ns->39ns: 3 period cycles, pwm prd=33MHz (so, 0%, 33%, 66% and 100%
> >> duty)
> >> - 40ns->41ns: 4 period cycles, pwm prd=25MHz (so, 0%, 25%, 50%, 75% and
> >> 100% duty)
> >> - 42ns->49ns: 24MHz bypass (so 0%, 50% and 100% duty)
> >> - 50ns->59ns: 5 period cycles, pwm prd=20MHz (so, 0%, 20%, 40%, 60%, 80%
> >> and 100% duty)
> >>
> >>>
> >>>> If the bypass is chosen over a non-bypassed alternative, then we can get
> >>>> rid of the clk-provider and use pwm-clock instead, but we loose duty
> >>>> steps around each bypassed frequency.
> >>>
> >>> There is still the separate rate-reporting issue. If a sibling holds the
> >>> pair at 100 MHz, the 42/21 ns request becomes 40/20 ns, i.e. 25 MHz,
> >>> while pwm-clock continues to advertise its configured 24 MHz. Selecting
> >>> bypass when it is available doesn't remove that mismatch.
> >>>
> >>>> IMHO, the PWM driver has not enough informations to choose between the
> >>>> bypass and maximizing the duty steps number. That's why I prefer more
> >>>> duty steps and adding a clock-provider (and also because when the bypass
> >>>> is enabled, there's no pulse-width modulation anymore, just a clock :))
> >>>>
> >>>> if we had the period, duty_cycle and the max duty steps required as
> >>>> input, we could choose, but that's not the case.
> >>>>
> >>>> So I guess that the real question here is:
> >>>> Is it better to have more duty steps, or to have a closer clock?
> >>>
> >>> The resolution tradeoff is real, but the waveform API currently specifies
> >>> the ordering: choose the largest period not exceeding the request, then
> >>> the largest compatible duty length not exceeding the request, then the
> >>> offset. That's the ordering implemented in v9.
> >>>
> >> The ordering stated by pwm_round_waveform_might_sleep() documentation is
> >> period_length_ns, duty_length_ns and then duty_offset_ns, but what about
> >> duty steps?
> >> It's not stated in there because it's not a user input, but still, it's
> >> a quite important value for a pulse width *modulator*.
> >>
> >> My point here is that there's a choice between 2 implementations:
> >> - Consider the bypass as an emergency tool in order to get frequencies
> >> that are out of bound of the PWM logic (so that would mean only 100MHz)
> >> and in this case, we have more duty steps for the other periods, like a
> >> standard pulse width modulator
> >> - Or consider that the bypass is part of the PWM logic and don't bother
> >> looking at duty steps. In this case, it's used to get closer frequencies
> >> to what is requested, and have only 0/50/100% duty for those case.
> >> (and for this driver, the only case is for the 42ns to 49ns periods)
> >>
> >> I'm more enclined to choose the 1st case, because a bypass bypasses the
> >> PWM logic (!) and we can say that it's not part of the PWM and should
> >> only be used for the clk-provider part and to reach the 100MHz PWM
> >> frequency, but that's only my point of view.
> >
> > I agree that duty resolution matters. I think the remaining question is
> > which configurations we should expose to the waveform API, rather than
> > whether bypass itself is PWM.
> >
> > Restricting bypass alone would not give the higher-resolution
> > alternative. With the 24 MHz and 100 MHz parents and an unconstrained
> > pair, a single 24 MHz cycle still reports a 42 ns period and wins over
> > 40 ns. For example, a 42 ns period / 31 ns high-time request currently
> > becomes 42/21 ns; without bypass candidates it becomes 42/0 ns, not
> > 40/30 ns.
> No, because for a 42ns period, the PWM logic can't choose the 24MHz
> clock, it's out of bounds. The lowest achievable period with the 24MHz
> clock and no bybass is 84ns.
> So, the only remaining possibility without bypass is the 100MHz clock.
Yes, producing both a high and a low interval requires at least two
ticks, so the shortest non-constant waveform from 24 MHz has an
83.333 ns period, reported as 84 ns.
My 42/0 ns example was a constant-low output, not a toggling 24 MHz
waveform. I should have made that explicit. The manual documents
PWM_ENTIRE_CYCLE = 0 as one cycle, and the current rounding helper
includes that setting. For the 42/31 ns request, it calculates one
period tick and zero active ticks.
If we exclude one-tick cycle-mode candidates as well as the 24 MHz
bypass, I agree that the result becomes 40/30 ns using 100 MHz.
Removing the bypass candidates alone does not do that in the current
implementation.
>
> >
> > The same tradeoff occurs entirely in cycle mode: at 84 ns, two 24 MHz
> > ticks provide three duty levels, while eight 100 MHz ticks provide nine
> > levels at 80 ns.
> >
> > So achieving the resolution preference would also require restricting
> > ordinary cycle-mode candidates or changing the period-first selection
> > policy.
> >
> >>
> >> Regards,
> >> Richard.
> >>>>
> >>>> Regards,
> >>>> Richard
> >>>>
> >>>>>> If you do
> >>>>>>
> >>>>>> compatible = "pwm-clock";
> >>>>>> clock-frequency = <24000000>;
> >>>>>> pwms = <&pwm0 42 ...>;
> >>>>>>
> >>>>>> that should give you a reliable match?!
> >>>>>>
> >>>>>>> Exposing the bypass through CCF provides an exact rate request,
> >>>>>>> conveys that duty-cycle and polarity control are not needed, and makes the
> >>>>>>> shared pair clock visible to CCF rate arbitration.
> >>>>>>
> >>>>>> Are you "just" suffering that clocks are programmed in Hz which PWMs are
> >>>>>> programmed in ns and thus your losing precision on divisions?
> >>>>>
> >>>>> There is also the distinction between the advertised and resulting rate.
> >>>>> clk-pwm advertises its configured fixed frequency without checking the
> >>>>> rounded PWM frequency. If an active sibling holds the shared pair at
> >>>>> 100 MHz, that same request rounds to 40 ns with 20 ns high time, i.e.
> >>>>> 25 MHz, while pwm-clock still reports 24 MHz.
> >>>>>
> >>>>> The direct provider reports the actual pair rate and exposes its shared
> >>>>> rate protection to CCF. CCF can still round a rate request, so this is not
> >>>>> an unconditional exact-rate guarantee. I've clarified that in v9.
> >>>>>
> >>>>>>> Wait for the hardware period-update handshake before replacing PPR. Track
> >>>>>>> the active and newly programmed periods so normal updates use a tight
> >>>>>>> timeout, while an unknown firmware update conservatively allows the maximum
> >>>>>>> hardware period. Scale the sleep interval to keep the number of MMIO polls
> >>>>>>> bounded even for very long periods.
> >>>>>>>
> >>>>>>> Dead-zone mode is not representable by the PWM API. Reject it while either
> >>>>>>> output in the pair is active, but clear dormant dead-zone enable state when
> >>>>>>> both outputs are disabled so stale firmware configuration does not
> >>>>>>> permanently prevent ordinary PWM use.
> >>>>>>>
> >>>>>>> Signed-off-by: Richard Genoud <richard.genoud at bootlin.com>
> >>>>>>> Co-developed-by: James Hilliard <james.hilliard1 at gmail.com>
> >>>>>>> Signed-off-by: James Hilliard <james.hilliard1 at gmail.com>
> >>>>>>> ---
> >>>>>>> Changes v7 -> v8:
> >>>>>>> - clear the complete two-bit clock-source selector field
> >>>>>>> - leave rate and period registers untouched when disabling a channel
> >>>>>>> - distinguish PWM-owned bypass signals from clock-owned bypass outputs
> >>>>>>> - round waveforms over all parent, pair-divider, channel-divider and bypass
> >>>>>>> choices according to the PWM core ordering
> >>>>>>> - encode constant levels without overflowing the 16-bit active-cycle field
> >>>>>>> - allocate clock topology state per device and use the fixed parent map
> >>>>>>> (reported by Sashiko)
> >>>>>>> - use managed clock registrations for complete probe-error cleanup
> >>>>>>> (reported by Sashiko and suggested by Philipp)
> >>>>>>> - deassert reset before registering clocks and keep the reset handle local
> >>>>>>> (reported by Sashiko and suggested by Philipp)
> >>>>>>> - arbitrate channel ownership in clock prepare/unprepare and select bypass
> >>>>>>> only while the clock output is enabled (reported by Sashiko)
> >>>>>>> - roll back failed PWM requests and serialize channel release
> >>>>>>> (reported by Sashiko)
> >>>>>>> - protect the shared pair source while either sibling channel is active
> >>>>>>> (reported by Sashiko)
> >>>>>>> - preserve the 65536-tick duty intermediate before polarity conversion
> >>>>>>> (reported by Sashiko)
> >>>>>>> - avoid replaying write-one control bits while changing channel settings
> >>>>>>> (reported by Sashiko)
> >>>>>>> - keep the hardware-waveform state within the PWM core storage limit
> >>>>>>> - clarify that either co-packaged AC200 or AC300 PHY uses the bypass clock
> >>>>>>> - drop Tested-by tags after the clock implementation was reworked
> >>>>>>> - quantize inverted waveforms in hardware ticks and report their
> >>>>>>> physical offset
> >>>>>>> - force released channels off so stale waveforms cannot restart with a
> >>>>>>> sibling
> >>>>>>> - track the active and pending cycle for the period-update handshake and
> >>>>>>> adapt the sleep interval to bound MMIO polling on long periods
> >>>>>>> - quiesce rate, polarity and bypass transitions before reprogramming the
> >>>>>>> output
> >>>>>>> - clear unsupported pulse mode when programming periodic waveforms
> >>>>>>> - preserve firmware-active outputs through generic unused-clock cleanup,
> >>>>>>> stop any still-unclaimed channels at sync_state(), reconcile empty pair
> >>>>>>> gates through CCF, and reject pair retuning while either output is
> >>>>>>> active
> >>>>>>> - hold pair-rate exclusivity only while an output is active
> >>>>>>> - make pair-rate changes transactional and restore a quiesced output on
> >>>>>>> failure
> >>>>>>> - clear dormant dead-zone state when both pair outputs are disabled,
> >>>>>>> while rejecting active dead-zone and pulse modes which the PWM API
> >>>>>>> cannot represent
> >>>>>>> - correct the hardware topology to show the pair gate before div_m
> >>>>>>> - remove unused register definitions and simplify the fixed parent
> >>>>>>> topology
> >>>>>>> ---
> >>>>>>> drivers/pwm/Kconfig | 12 +
> >>>>>>> drivers/pwm/Makefile | 1 +
> >>>>>>> drivers/pwm/pwm-sun8i.c | 1542 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>> 3 files changed, 1555 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >>>>>>> index e8886a9b64d9..e7fd2e66b1aa 100644
> >>>>>>> --- a/drivers/pwm/Kconfig
> >>>>>>> +++ b/drivers/pwm/Kconfig
> >>>>>>> @@ -748,6 +748,18 @@ config PWM_SUN4I
> >>>>>>> To compile this driver as a module, choose M here: the module
> >>>>>>> will be called pwm-sun4i.
> >>>>>>>
> >>>>>>> +config PWM_SUN8I
> >>>>>>> + tristate "Allwinner sun8i/sun50i PWM support"
> >>>>>>> + depends on ARCH_SUNXI || COMPILE_TEST
> >>>>>>> + depends on HAS_IOMEM && COMMON_CLK
> >>>>>>> + help
> >>>>>>> + PWM framework driver for Allwinner controllers whose channels share
> >>>>>>> + paired source clocks. In addition to PWM operation, each channel's
> >>>>>>> + hardware bypass path can be exported as a clock output.
> >>>>>>> +
> >>>>>>> + To compile this driver as a module, choose M here: the module
> >>>>>>> + will be called pwm-sun8i.
> >>>>>>> +
> >>>>>>> config PWM_SUNPLUS
> >>>>>>> tristate "Sunplus PWM support"
> >>>>>>> depends on ARCH_SUNPLUS || COMPILE_TEST
> >>>>>>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> >>>>>>> index 5630a521a7cf..9c922b1fd0b4 100644
> >>>>>>> --- a/drivers/pwm/Makefile
> >>>>>>> +++ b/drivers/pwm/Makefile
> >>>>>>> @@ -68,6 +68,7 @@ obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
> >>>>>>> obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o
> >>>>>>> obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o
> >>>>>>> obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o
> >>>>>>> +obj-$(CONFIG_PWM_SUN8I) += pwm-sun8i.o
> >>>>>>> obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o
> >>>>>>> obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
> >>>>>>> obj-$(CONFIG_PWM_TH1520) += pwm_th1520.o
> >>>>>>> diff --git a/drivers/pwm/pwm-sun8i.c b/drivers/pwm/pwm-sun8i.c
> >>>>>>> new file mode 100644
> >>>>>>> index 000000000000..5c3580a0924d
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/drivers/pwm/pwm-sun8i.c
> >>>>>>> @@ -0,0 +1,1542 @@
> >>>>>>> +// SPDX-License-Identifier: GPL-2.0-only
> >>>>>>> +/*
> >>>>>>> + * Driver for Allwinner sun8i Pulse Width Modulation Controller
> >>>>>>> + *
> >>>>>>> + * (C) Copyright 2025 Richard Genoud, Bootlin <richard.genoud at bootlin.com>
> >>>>>>> + * (C) Copyright 2026 James Hilliard <james.hilliard1 at gmail.com>
> >>>>>>> + *
> >>>>>>> + * Based on drivers/pwm/pwm-sun4i.c with Copyright:
> >>>>>>> + *
> >>>>>>> + * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni at bootlin.com>
> >>>>>>> + *
> >>>>>>> + * Channels are paired (0/1, 2/3, 4/5). Each pair shares a clock source and
> >>>>>>> + * first prescaler (div_m), while each channel has its own second prescaler
> >>>>>>> + * (div_k) and bypass path.
> >>>>>>> + *
> >>>>>>> + */
> >>>>>>> +
> >>>>>>> +#include <linux/bitfield.h>
> >>>>>>> +#include <linux/bits.h>
> >>>>>>> +#include <linux/clk.h>
> >>>>>>> +#include <linux/clk-provider.h>
> >>>>>>> +#include <linux/device.h>
> >>>>>>> +#include <linux/err.h>
> >>>>>>> +#include <linux/io.h>
> >>>>>>> +#include <linux/iopoll.h>
> >>>>>>> +#include <linux/limits.h>
> >>>>>>> +#include <linux/math64.h>
> >>>>>>> +#include <linux/module.h>
> >>>>>>> +#include <linux/notifier.h>
> >>>>>>> +#include <linux/of.h>
> >>>>>>> +#include <linux/platform_device.h>
> >>>>>>> +#include <linux/pwm.h>
> >>>>>>> +#include <linux/reset.h>
> >>>>>>> +#include <linux/slab.h>
> >>>>>>> +#include <linux/spinlock.h>
> >>>>>>> +#include <linux/time.h>
> >>>>>>> +
> >>>>>>> +/* PWMCC Pairs Clock Configuration Registers */
> >>>>>>> +#define SUN8I_PWM_PCCR(pair) (0x20 + ((pair) * 0x4))
> >>>>>>> +#define SUN8I_PWM_PCCR_SRC_SHIFT 7
> >>>>>>> +#define SUN8I_PWM_PCCR_SRC_MASK GENMASK(1, 0)
> >>>>>>> +#define SUN8I_PWM_PCCR_GATE_BIT 4
> >>>>>>> +#define SUN8I_PWM_PCCR_GATE BIT(SUN8I_PWM_PCCR_GATE_BIT)
> >>>>>>> +#define SUN8I_PWM_PCCR_BYPASS_BIT(chan) ((chan) % 2 + 5)
> >>>>>>> +#define SUN8I_PWM_PCCR_DIV_M_SHIFT 0
> >>>>>>> +#define SUN8I_PWM_PCCR_DIV_M_WIDTH 4
> >>>>>>> +
> >>>>>>> +/* PWM Enable Register */
> >>>>>>> +#define SUN8I_PWM_PER 0x40
> >>>>>>> +#define SUN8I_PWM_ENABLE(chan) BIT(chan)
> >>>>>>> +
> >>>>>>> +/* PWM Control Register */
> >>>>>>> +#define SUN8I_PWM_PCR(chan) (0x60 + (chan) * 0x20)
> >>>>>>> +#define SUN8I_PWM_PCR_PRESCAL_K_MASK GENMASK(7, 0)
> >>>>>>> +#define SUN8I_PWM_PCR_ACTIVE_STATE BIT(8)
> >>>>>>> +#define SUN8I_PWM_PCR_MODE BIT(9)
> >>>>>>> +#define SUN8I_PWM_PCR_PULSE_START BIT(10)
> >>>>>>> +#define SUN8I_PWM_PCR_PERIOD_READY BIT(11) /* 0: ready, 1: busy */
> >>>>>>> +
> >>>>>>> +/* PWM Period Register */
> >>>>>>> +#define SUN8I_PWM_PPR(chan) (0x64 + (chan) * 0x20)
> >>>>>>> +#define SUN8I_PWM_PPR_PERIOD_MASK GENMASK(31, 16)
> >>>>>>> +#define SUN8I_PWM_PPR_DUTY_MASK GENMASK(15, 0)
> >>>>>>> +#define SUN8I_PWM_PPR_PERIOD_VALUE(reg) (FIELD_GET(SUN8I_PWM_PPR_PERIOD_MASK, reg) + 1)
> >>>>>>> +#define SUN8I_PWM_PPR_DUTY_VALUE(reg) FIELD_GET(SUN8I_PWM_PPR_DUTY_MASK, reg)
> >>>>>>> +#define SUN8I_PWM_PPR_PERIOD(prd) FIELD_PREP(SUN8I_PWM_PPR_PERIOD_MASK, (prd) - 1)
> >>>>>>> +#define SUN8I_PWM_PPR_DUTY(dty) FIELD_PREP(SUN8I_PWM_PPR_DUTY_MASK, dty)
> >>>>>>> +#define SUN8I_PWM_PPR_PERIOD_MAX (FIELD_MAX(SUN8I_PWM_PPR_PERIOD_MASK) + 1)
> >>>>>>> +
> >>>>>>> +/* PWM pair dead-zone control registers. */
> >>>>>>> +#define SUN8I_PWM_PDZCR(pair) (0x30 + ((pair) * 0x4))
> >>>>>>> +#define SUN8I_PWM_PDZCR_ENABLE BIT(0)
> >>>>>>> +
> >>>>>>> +#define SUN8I_PWM_PERIOD_READY_MARGIN_US 50
> >>>>>>> +#define SUN8I_PWM_PERIOD_READY_MIN_POLL_US 10
> >>>>>>> +#define SUN8I_PWM_PERIOD_READY_MAX_POLL_US 10000
> >>>>>>> +#define SUN8I_PWM_PERIOD_READY_POLL_COUNT 1024
> >>>>>>> +
> >>>>>>> +#define SUN8I_PWM_PAIR_IDX(chan) ((chan) >> 1)
> >>>>>>> +
> >>>>>>> +/*
> >>>>>>> + * Block diagram of the PWM clock controller:
> >>>>>>> + *
> >>>>>>> + * _____ ______ ________
> >>>>>>> + * OSC24M --->| | | | | |
> >>>>>>> + * APB1 ----->| Mux |--->| Gate |--->| /div_m |-----> SUN8I_PWM_clock_src_xy
> >>>>>>> + * |_____| |______| |________|
> >>>>>>> + * ________
> >>>>>>> + * | |
> >>>>>>> + * +->| /div_k |---> SUN8I_PWM_clock_x
> >>>>>>> + * | |________|
> >>>>>>> + * | ______
> >>>>>>> + * | | |
> >>>>>>> + * +-->| Gate |----> SUN8I_PWM_bypass_clock_x
> >>>>>>> + * | |______|
> >>>>>>> + * SUN8I_PWM_clock_src_xy ---+ ________
> >>>>>>> + * | | |
> >>>>>>> + * +->| /div_k |---> SUN8I_PWM_clock_y
> >>>>>>> + * | |________|
> >>>>>>> + * | ______
> >>>>>>> + * | | |
> >>>>>>> + * +-->| Gate |----> SUN8I_PWM_bypass_clock_y
> >>>>>>> + * |______|
> >>>>>>> + *
> >>>>>>> + * NB: when the bypass is set, all the PWM logic is bypassed.
> >>>>>>> + * So, the duty cycle and polarity can't be modified (we just have a clock).
> >>>>>>> + * The bypass in PWM mode is used to achieve a 1/2 relative duty cycle with the
> >>>>>>> + * fastest clock.
> >>>>>>> + *
> >>>>>>> + * SUN8I_PWM_clock_x/y serve for the PWM purpose.
> >>>>>>> + * SUN8I_PWM_bypass_clock_x/y serve for the clock-provider purpose.
> >>>>>>> + *
> >>>>>>> + */
> >>>>>>> +
> >>>>>>> +/* /div_m is a power-of-two divider limited to /256. */
> >>>>>>> +static const struct clk_div_table sun8i_pwm_div_m_table[] = {
> >>>>>>> + { .val = 0, .div = 1 },
> >>>>>>> + { .val = 1, .div = 2 },
> >>>>>>> + { .val = 2, .div = 4 },
> >>>>>>> + { .val = 3, .div = 8 },
> >>>>>>> + { .val = 4, .div = 16 },
> >>>>>>> + { .val = 5, .div = 32 },
> >>>>>>> + { .val = 6, .div = 64 },
> >>>>>>> + { .val = 7, .div = 128 },
> >>>>>>> + { .val = 8, .div = 256 },
> >>>>>>> + { /* sentinel */ }
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +enum sun8i_pwm_mode {
> >>>>>>> + SUN8I_PWM_MODE_NONE,
> >>>>>>> + SUN8I_PWM_MODE_PWM,
> >>>>>>> + SUN8I_PWM_MODE_CLK,
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +struct sun8i_pwm_data {
> >>>>>>> + unsigned int npwm;
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +struct sun8i_pwm_chip;
> >>>>>>> +
> >>>>>>> +struct sun8i_pwm_pair {
> >>>>>>> + struct clk_mux mux;
> >>>>>>> + struct clk_hw gate_hw;
> >>>>>>> + struct clk_divider divider;
> >>>>>>> + struct clk_hw *hw;
> >>>>>>> + struct notifier_block rate_nb;
> >>>>>>> + struct sun8i_pwm_chip *chip;
> >>>>>>> + unsigned int index;
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +struct sun8i_pwm_bypass {
> >>>>>>> + struct clk_hw hw;
> >>>>>>> + struct sun8i_pwm_chip *chip;
> >>>>>>> + unsigned int index;
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +struct sun8i_pwm_channel {
> >>>>>>> + struct sun8i_pwm_bypass bypass;
> >>>>>>> + struct clk *pair_clk;
> >>>>>>> + enum sun8i_pwm_mode mode;
> >>>>>>> + u64 pending_period_ns;
> >>>>>>> + bool rate_exclusive;
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +struct sun8i_pwm_chip {
> >>>>>>> + struct clk_hw_onecell_data *hw_data;
> >>>>>>> + struct sun8i_pwm_pair *pairs;
> >>>>>>> + struct sun8i_pwm_channel *channels;
> >>>>>>> + struct clk *bus_clk;
> >>>>>>> + void __iomem *base;
> >>>>>>> + const struct sun8i_pwm_data *data;
> >>>>>>> + /* Protects shared registers and channel ownership. */
> >>>>>>> + spinlock_t lock;
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +struct sun8i_pwm_waveform {
> >>>>>>> + u32 duty_ticks;
> >>>>>>> + u32 period_ticks;
> >>>>>>> + u32 pair_rate;
> >>>>>>> + u16 div_k;
> >>>>>>> + u8 enabled:1;
> >>>>>>> + u8 active_state:1;
> >>>>>>> + u8 bypass_en:1;
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +static inline struct sun8i_pwm_chip *sun8i_pwm_from_chip(const struct pwm_chip *chip)
> >>>>>>> +{
> >>>>>>> + return pwmchip_get_drvdata(chip);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static inline u32 sun8i_pwm_readl(struct sun8i_pwm_chip *sun8i_chip,
> >>>>>>> + unsigned long offset)
> >>>>>>> +{
> >>>>>>> + return readl(sun8i_chip->base + offset);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static inline void sun8i_pwm_writel(struct sun8i_pwm_chip *sun8i_chip,
> >>>>>>> + u32 val, unsigned long offset)
> >>>>>>> +{
> >>>>>>> + writel(val, sun8i_chip->base + offset);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static void sun8i_pwm_set_bypass_locked(struct sun8i_pwm_chip *sun8i_chip,
> >>>>>>> + unsigned int idx, bool enable)
> >>>>>>> +{
> >>>>>>> + unsigned long reg_offset;
> >>>>>>> + u32 val;
> >>>>>>> +
> >>>>>>> + lockdep_assert_held(&sun8i_chip->lock);
> >>>>>>> +
> >>>>>>> + reg_offset = SUN8I_PWM_PCCR(SUN8I_PWM_PAIR_IDX(idx));
> >>>>>>> + val = sun8i_pwm_readl(sun8i_chip, reg_offset);
> >>>>>>> + if (enable)
> >>>>>>> + val |= BIT(SUN8I_PWM_PCCR_BYPASS_BIT(idx));
> >>>>>>> + else
> >>>>>>> + val &= ~BIT(SUN8I_PWM_PCCR_BYPASS_BIT(idx));
> >>>>>>> +
> >>>>>>> + sun8i_pwm_writel(sun8i_chip, val, reg_offset);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static void sun8i_pwm_set_enabled_locked(struct sun8i_pwm_chip *sun8i_chip,
> >>>>>>> + unsigned int idx, bool enable)
> >>>>>>> +{
> >>>>>>> + u32 val;
> >>>>>>> +
> >>>>>>> + lockdep_assert_held(&sun8i_chip->lock);
> >>>>>>> +
> >>>>>>> + val = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PER);
> >>>>>>> + if (enable)
> >>>>>>> + val |= SUN8I_PWM_ENABLE(idx);
> >>>>>>> + else
> >>>>>>> + val &= ~SUN8I_PWM_ENABLE(idx);
> >>>>>>> + sun8i_pwm_writel(sun8i_chip, val, SUN8I_PWM_PER);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static bool
> >>>>>>> +sun8i_pwm_channel_is_enabled_locked(struct sun8i_pwm_chip *sun8i_chip,
> >>>>>>> + unsigned int idx)
> >>>>>>> +{
> >>>>>>> + unsigned int pair = SUN8I_PWM_PAIR_IDX(idx);
> >>>>>>> + u32 pccr, per;
> >>>>>>> +
> >>>>>>> + lockdep_assert_held(&sun8i_chip->lock);
> >>>>>>> +
> >>>>>>> + pccr = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PCCR(pair));
> >>>>>>> + per = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PER);
> >>>>>>> +
> >>>>>>> + return (pccr & SUN8I_PWM_PCCR_GATE) &&
> >>>>>>> + (per & SUN8I_PWM_ENABLE(idx));
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static u32 sun8i_pwm_pair_enable_mask(struct sun8i_pwm_chip *sun8i_chip,
> >>>>>>> + unsigned int pair)
> >>>>>>> +{
> >>>>>>> + unsigned int first = pair * 2;
> >>>>>>> + u32 mask = SUN8I_PWM_ENABLE(first);
> >>>>>>> +
> >>>>>>> + if (first + 1 < sun8i_chip->data->npwm)
> >>>>>>> + mask |= SUN8I_PWM_ENABLE(first + 1);
> >>>>>>> +
> >>>>>>> + return mask;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static u32
> >>>>>>> +sun8i_pwm_pair_owner_mask_locked(struct sun8i_pwm_chip *sun8i_chip,
> >>>>>>> + unsigned int pair)
> >>>>>>> +{
> >>>>>>> + unsigned int first = pair * 2;
> >>>>>>> + u32 mask = 0;
> >>>>>>> +
> >>>>>>> + lockdep_assert_held(&sun8i_chip->lock);
> >>>>>>> +
> >>>>>>> + if (sun8i_chip->channels[first].mode != SUN8I_PWM_MODE_NONE)
> >>>>>>> + mask |= SUN8I_PWM_ENABLE(first);
> >>>>>>> + if (first + 1 < sun8i_chip->data->npwm &&
> >>>>>>> + sun8i_chip->channels[first + 1].mode != SUN8I_PWM_MODE_NONE)
> >>>>>>> + mask |= SUN8I_PWM_ENABLE(first + 1);
> >>>>>>> +
> >>>>>>> + return mask;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static inline struct sun8i_pwm_pair *
> >>>>>>> +sun8i_pwm_pair_from_gate_hw(struct clk_hw *hw)
> >>>>>>> +{
> >>>>>>> + return container_of(hw, struct sun8i_pwm_pair, gate_hw);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static int sun8i_pwm_pair_gate_enable(struct clk_hw *hw)
> >>>>>>> +{
> >>>>>>> + struct sun8i_pwm_pair *pair = sun8i_pwm_pair_from_gate_hw(hw);
> >>>>>>> + struct sun8i_pwm_chip *sun8i_chip = pair->chip;
> >>>>>>> + unsigned long flags;
> >>>>>>> + u32 pccr;
> >>>>>>> +
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + pccr = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PCCR(pair->index));
> >>>>>>> + pccr |= SUN8I_PWM_PCCR_GATE;
> >>>>>>> + sun8i_pwm_writel(sun8i_chip, pccr, SUN8I_PWM_PCCR(pair->index));
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> +
> >>>>>>> + return 0;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static void sun8i_pwm_pair_gate_disable(struct clk_hw *hw)
> >>>>>>> +{
> >>>>>>> + struct sun8i_pwm_pair *pair = sun8i_pwm_pair_from_gate_hw(hw);
> >>>>>>> + struct sun8i_pwm_chip *sun8i_chip = pair->chip;
> >>>>>>> + unsigned long reg = SUN8I_PWM_PCCR(pair->index);
> >>>>>>> + unsigned long flags;
> >>>>>>> + u32 pccr, per;
> >>>>>>> +
> >>>>>>> + /* CCF counts do not include outputs awaiting firmware-state handoff. */
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + per = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PER);
> >>>>>>> + if (!(per & sun8i_pwm_pair_enable_mask(sun8i_chip, pair->index))) {
> >>>>>>> + pccr = sun8i_pwm_readl(sun8i_chip, reg);
> >>>>>>> + pccr &= ~SUN8I_PWM_PCCR_GATE;
> >>>>>>> + sun8i_pwm_writel(sun8i_chip, pccr, reg);
> >>>>>>> + }
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static int sun8i_pwm_pair_gate_is_enabled(struct clk_hw *hw)
> >>>>>>> +{
> >>>>>>> + struct sun8i_pwm_pair *pair = sun8i_pwm_pair_from_gate_hw(hw);
> >>>>>>> + struct sun8i_pwm_chip *sun8i_chip = pair->chip;
> >>>>>>> + unsigned long reg = SUN8I_PWM_PCCR(pair->index);
> >>>>>>> + unsigned long flags;
> >>>>>>> + bool enabled;
> >>>>>>> +
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + enabled = sun8i_pwm_readl(sun8i_chip, reg) & SUN8I_PWM_PCCR_GATE;
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> +
> >>>>>>> + return enabled;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static const struct clk_ops sun8i_pwm_pair_gate_ops = {
> >>>>>>> + .enable = sun8i_pwm_pair_gate_enable,
> >>>>>>> + .disable = sun8i_pwm_pair_gate_disable,
> >>>>>>> + .is_enabled = sun8i_pwm_pair_gate_is_enabled,
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +static int sun8i_pwm_pair_rate_notifier(struct notifier_block *nb,
> >>>>>>> + unsigned long event, void *data)
> >>>>>>> +{
> >>>>>>> + struct sun8i_pwm_pair *pair =
> >>>>>>> + container_of(nb, struct sun8i_pwm_pair, rate_nb);
> >>>>>>> + struct sun8i_pwm_chip *sun8i_chip = pair->chip;
> >>>>>>> + unsigned long flags;
> >>>>>>> + bool active;
> >>>>>>> +
> >>>>>>> + if (event != PRE_RATE_CHANGE)
> >>>>>>> + return NOTIFY_DONE;
> >>>>>>> +
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + active = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PER) &
> >>>>>>> + sun8i_pwm_pair_enable_mask(sun8i_chip, pair->index);
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> +
> >>>>>>> + return active ? NOTIFY_BAD : NOTIFY_OK;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static inline struct sun8i_pwm_bypass *
> >>>>>>> +sun8i_pwm_bypass_from_hw(struct clk_hw *hw)
> >>>>>>> +{
> >>>>>>> + return container_of(hw, struct sun8i_pwm_bypass, hw);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static int sun8i_pwm_bypass_prepare(struct clk_hw *hw)
> >>>>>>> +{
> >>>>>>> + struct sun8i_pwm_bypass *bypass = sun8i_pwm_bypass_from_hw(hw);
> >>>>>>> + struct sun8i_pwm_chip *sun8i_chip = bypass->chip;
> >>>>>>> + struct sun8i_pwm_channel *chan = &sun8i_chip->channels[bypass->index];
> >>>>>>> + unsigned long flags;
> >>>>>>> + int ret = 0;
> >>>>>>> +
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + if (chan->mode != SUN8I_PWM_MODE_NONE)
> >>>>>>> + ret = -EBUSY;
> >>>>>>> + else
> >>>>>>> + chan->mode = SUN8I_PWM_MODE_CLK;
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> +
> >>>>>>> + return ret;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static void sun8i_pwm_bypass_unprepare(struct clk_hw *hw)
> >>>>>>> +{
> >>>>>>> + struct sun8i_pwm_bypass *bypass = sun8i_pwm_bypass_from_hw(hw);
> >>>>>>> + struct sun8i_pwm_chip *sun8i_chip = bypass->chip;
> >>>>>>> + struct sun8i_pwm_channel *chan = &sun8i_chip->channels[bypass->index];
> >>>>>>> + unsigned long flags;
> >>>>>>> +
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + if (WARN_ON_ONCE(chan->mode != SUN8I_PWM_MODE_CLK))
> >>>>>>> + goto out_unlock;
> >>>>>>> +
> >>>>>>> + chan->mode = SUN8I_PWM_MODE_NONE;
> >>>>>>> +
> >>>>>>> +out_unlock:
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static int sun8i_pwm_bypass_enable(struct clk_hw *hw)
> >>>>>>> +{
> >>>>>>> + struct sun8i_pwm_bypass *bypass = sun8i_pwm_bypass_from_hw(hw);
> >>>>>>> + struct sun8i_pwm_chip *sun8i_chip = bypass->chip;
> >>>>>>> + struct sun8i_pwm_channel *chan = &sun8i_chip->channels[bypass->index];
> >>>>>>> + unsigned int pair = SUN8I_PWM_PAIR_IDX(bypass->index);
> >>>>>>> + unsigned long flags;
> >>>>>>> + u32 pccr, per;
> >>>>>>> + int ret = 0;
> >>>>>>> +
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + if (WARN_ON_ONCE(chan->mode != SUN8I_PWM_MODE_CLK)) {
> >>>>>>> + ret = -EBUSY;
> >>>>>>> + goto out_unlock;
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + pccr = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PCCR(pair));
> >>>>>>> + per = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PER);
> >>>>>>> + if (!(pccr & BIT(SUN8I_PWM_PCCR_BYPASS_BIT(bypass->index))) ||
> >>>>>>> + !(per & SUN8I_PWM_ENABLE(bypass->index))) {
> >>>>>>> + sun8i_pwm_set_enabled_locked(sun8i_chip, bypass->index, false);
> >>>>>>> + sun8i_pwm_set_bypass_locked(sun8i_chip, bypass->index, true);
> >>>>>>> + sun8i_pwm_set_enabled_locked(sun8i_chip, bypass->index, true);
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> +out_unlock:
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> +
> >>>>>>> + return ret;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static void sun8i_pwm_bypass_disable(struct clk_hw *hw)
> >>>>>>> +{
> >>>>>>> + struct sun8i_pwm_bypass *bypass = sun8i_pwm_bypass_from_hw(hw);
> >>>>>>> + struct sun8i_pwm_chip *sun8i_chip = bypass->chip;
> >>>>>>> + struct sun8i_pwm_channel *chan = &sun8i_chip->channels[bypass->index];
> >>>>>>> + unsigned long flags;
> >>>>>>> +
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + if (WARN_ON_ONCE(chan->mode != SUN8I_PWM_MODE_CLK))
> >>>>>>> + goto out_unlock;
> >>>>>>> +
> >>>>>>> + sun8i_pwm_set_enabled_locked(sun8i_chip, bypass->index, false);
> >>>>>>> + sun8i_pwm_set_bypass_locked(sun8i_chip, bypass->index, false);
> >>>>>>> +
> >>>>>>> +out_unlock:
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static void sun8i_pwm_bypass_disable_unused(struct clk_hw *hw)
> >>>>>>> +{
> >>>>>>> + /* Defer inherited-output cleanup to sun8i_pwm_sync_state(). */
> >>>>>>> +}
> >>>>>>
> >>>>>> I would expect that you can drop this empty callback?!
> >>>>>
> >>>>> Removed in v9, with CLK_IGNORE_UNUSED replacing it. Simply removing the
> >>>>> callback would make CCF fall back to .disable(). The flag expresses the
> >>>>> intent to preserve unclaimed firmware outputs through unused-clock
> >>>>> cleanup; normal consumer disable still turns the output off.
> >>>>>
> >>>>>>
> >>>>>>> +static int sun8i_pwm_bypass_is_enabled(struct clk_hw *hw)
> >>>>>>> +{
> >>>>>>> + struct sun8i_pwm_bypass *bypass = sun8i_pwm_bypass_from_hw(hw);
> >>>>>>> + struct sun8i_pwm_chip *sun8i_chip = bypass->chip;
> >>>>>>> + unsigned int pair = SUN8I_PWM_PAIR_IDX(bypass->index);
> >>>>>>> + unsigned long flags;
> >>>>>>> + bool enabled;
> >>>>>>> + u32 val;
> >>>>>>> +
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> +
> >>>>>>> + val = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PCCR(pair));
> >>>>>>> + enabled = (val & SUN8I_PWM_PCCR_GATE) &&
> >>>>>>> + (val & BIT(SUN8I_PWM_PCCR_BYPASS_BIT(bypass->index)));
> >>>>>>> +
> >>>>>>> + val = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PER);
> >>>>>>> + enabled = enabled && (val & SUN8I_PWM_ENABLE(bypass->index));
> >>>>>>> +
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> +
> >>>>>>> + return enabled;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static unsigned long sun8i_pwm_bypass_recalc_rate(struct clk_hw *hw,
> >>>>>>> + unsigned long parent_rate)
> >>>>>>> +{
> >>>>>>> + return parent_rate;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static const struct clk_ops sun8i_pwm_bypass_ops = {
> >>>>>>> + .prepare = sun8i_pwm_bypass_prepare,
> >>>>>>> + .unprepare = sun8i_pwm_bypass_unprepare,
> >>>>>>> + .enable = sun8i_pwm_bypass_enable,
> >>>>>>> + .disable = sun8i_pwm_bypass_disable,
> >>>>>>> + .disable_unused = sun8i_pwm_bypass_disable_unused,
> >>>>>>> + .is_enabled = sun8i_pwm_bypass_is_enabled,
> >>>>>>> + .recalc_rate = sun8i_pwm_bypass_recalc_rate,
> >>>>>>> +};
> >>>>>>
> >>>>>> I have problems grokking how the clk and pwm frameworks interact here. I
> >>>>>> think that would be easier to understand if you split this patch into
> >>>>>> a basic pwm driver and in a 2nd patch add the clk stuff. Maybe also
> >>>>>> split out the bypass stuff?
> >>>>>
> >>>>> V9 separates PWM support in patch 2 from the exported bypass clocks and
> >>>>> PWM/clock ownership arbitration in patch 3.
> >>>>>
> >>>>> PWM request/free and clock prepare/unprepare arbitrate ownership of the
> >>>>> physical output, so the two interfaces cannot own it simultaneously.
> >>>>>
> >>>>> The internal CCF pair clocks remain in patch 2, so both patches use the
> >>>>> same implementation of the shared mux, divider and gate. Hardware bypass
> >>>>> rounding also stays there: it is part of PWM waveform generation and
> >>>>> inherited-state readback, independently of exporting a clock provider
> >>>>>
> >>>>>>> +static int sun8i_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> >>>>>>> +{
> >>>>>>> + struct sun8i_pwm_chip *sun8i_chip = sun8i_pwm_from_chip(chip);
> >>>>>>> + struct sun8i_pwm_channel *chan = &sun8i_chip->channels[pwm->hwpwm];
> >>>>>>> + unsigned int idx = pwm->hwpwm;
> >>>>>>> + unsigned long flags;
> >>>>>>> + bool was_enabled = false;
> >>>>>>> + int ret;
> >>>>>>> +
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + if (chan->mode != SUN8I_PWM_MODE_NONE) {
> >>>>>>> + ret = -EBUSY;
> >>>>>>> + } else {
> >>>>>>> + was_enabled =
> >>>>>>> + sun8i_pwm_channel_is_enabled_locked(sun8i_chip, idx);
> >>>>>>> + chan->mode = SUN8I_PWM_MODE_PWM;
> >>>>>>> + ret = 0;
> >>>>>>> + }
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> + if (ret)
> >>>>>>> + return ret;
> >>>>>>
> >>>>>> With scoped_guard() this can be written in a nicer way.
> >>>>>
> >>>>> Converted to guard()/scoped_guard() in v9.
> >>>>>
> >>>>>>> + if (was_enabled) {
> >>>>>>> + ret = clk_rate_exclusive_get(chan->pair_clk);
> >>>>>>> + if (ret)
> >>>>>>> + goto err_release_channel;
> >>>>>>> + WRITE_ONCE(chan->rate_exclusive, true);
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + ret = clk_prepare_enable(chan->pair_clk);
> >>>>>>> + if (ret) {
> >>>>>>> + if (READ_ONCE(chan->rate_exclusive)) {
> >>>>>>> + clk_rate_exclusive_put(chan->pair_clk);
> >>>>>>> + WRITE_ONCE(chan->rate_exclusive, false);
> >>>>>>> + }
> >>>>>>> + goto err_release_channel;
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + return 0;
> >>>>>>> +
> >>>>>>> +err_release_channel:
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + chan->mode = SUN8I_PWM_MODE_NONE;
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> +
> >>>>>>> + return ret;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static void sun8i_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> >>>>>>> +{
> >>>>>>> + struct sun8i_pwm_chip *sun8i_chip = sun8i_pwm_from_chip(chip);
> >>>>>>> + struct sun8i_pwm_channel *chan = &sun8i_chip->channels[pwm->hwpwm];
> >>>>>>> + unsigned long flags;
> >>>>>>> +
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + if (WARN_ON_ONCE(chan->mode != SUN8I_PWM_MODE_PWM)) {
> >>>>>>
> >>>>>> WARN_ON is usually frowned upon, see e.g.
> >>>>>> https://lore.kernel.org/all/2026091953-cherub-empty-ef35@gregkh/.
> >>>>>
> >>>>> Removed the ownership WARN_ON_ONCE() calls.
> >>>>>
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> + return;
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + sun8i_pwm_set_enabled_locked(sun8i_chip, pwm->hwpwm, false);
> >>>>>>> + sun8i_pwm_set_bypass_locked(sun8i_chip, pwm->hwpwm, false);
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> +
> >>>>>>> + if (READ_ONCE(chan->rate_exclusive)) {
> >>>>>>
> >>>>>> given this isn't performance critical (is it?) and we have a lock
> >>>>>> anyhow, I'd prefer to have chan->rate_exclusive protected by that lock,
> >>>>>> too, instead of mixing different atomics here.
> >>>>>
> >>>>> All accesses to rate_exclusive now use the shared register/ownership
> >>>>> lock. The CCF operations stay outside that spinlock because they take the
> >>>>> prepare mutex and can call back into the driver.
> >>>>>
> >>>>>>> + clk_rate_exclusive_put(chan->pair_clk);
> >>>>>>> + WRITE_ONCE(chan->rate_exclusive, false);
> >>>>>>> + }
> >>>>>>> + clk_disable_unprepare(chan->pair_clk);
> >>>>>>> +
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + chan->mode = SUN8I_PWM_MODE_NONE;
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static int sun8i_pwm_read_waveform(struct pwm_chip *chip,
> >>>>>>> + struct pwm_device *pwm,
> >>>>>>> + void *_wfhw)
> >>>>>>> +{
> >>>>>>> + struct sun8i_pwm_waveform *wfhw = _wfhw;
> >>>>>>> + struct sun8i_pwm_chip *sun8i_chip = sun8i_pwm_from_chip(chip);
> >>>>>>> + struct sun8i_pwm_channel *chan = &sun8i_chip->channels[pwm->hwpwm];
> >>>>>>> + unsigned int pair = SUN8I_PWM_PAIR_IDX(pwm->hwpwm);
> >>>>>>> + unsigned long flags, pair_rate;
> >>>>>>> + u32 pccr, pcr, pdzcr, per, ppr;
> >>>>>>> +
> >>>>>>> + pair_rate = clk_get_rate(chan->pair_clk);
> >>>>>>> + if (pair_rate > U32_MAX)
> >>>>>>> + return -ERANGE;
> >>>>>>
> >>>>>> That should not happen. You lock the rate in .request(). So please check
> >>>>>> the rate already there and return an error code.
> >>>>>
> >>>>> The rate is only protected while the PWM is active, not for its entire
> >>>>> requested lifetime. A requested but disabled PWM must still allow its
> >>>>> sibling to change the pair rate. Also, debugfs reads hardware state for
> >>>>> channels which have not been requested.
> >>>>>
> >>>>> I therefore kept the checks where the rate is used rather than moving
> >>>>> them solely to .request().
> >>>>>
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + pccr = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PCCR(pair));
> >>>>>>> + per = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PER);
> >>>>>>> + pcr = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PCR(pwm->hwpwm));
> >>>>>>> + ppr = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PPR(pwm->hwpwm));
> >>>>>>> + pdzcr = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PDZCR(pair));
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> +
> >>>>>>> + *wfhw = (struct sun8i_pwm_waveform) {
> >>>>>>> + .enabled = !!(pccr & SUN8I_PWM_PCCR_GATE) &&
> >>>>>>> + !!(per & SUN8I_PWM_ENABLE(pwm->hwpwm)),
> >>>>>>> + .bypass_en = !!(pccr &
> >>>>>>> + BIT(SUN8I_PWM_PCCR_BYPASS_BIT(pwm->hwpwm))),
> >>>>>>> + .active_state = !!(pcr & SUN8I_PWM_PCR_ACTIVE_STATE),
> >>>>>>> + .duty_ticks = SUN8I_PWM_PPR_DUTY_VALUE(ppr),
> >>>>>>> + .period_ticks = SUN8I_PWM_PPR_PERIOD_VALUE(ppr),
> >>>>>>> + .pair_rate = pair_rate,
> >>>>>>> + .div_k = FIELD_GET(SUN8I_PWM_PCR_PRESCAL_K_MASK, pcr) + 1,
> >>>>>>> + };
> >>>>>>> +
> >>>>>>> + if (wfhw->enabled && !wfhw->bypass_en &&
> >>>>>>> + (pdzcr & SUN8I_PWM_PDZCR_ENABLE))
> >>>>>>> + return -EOPNOTSUPP;
> >>>>>>> + if (wfhw->enabled && !wfhw->bypass_en &&
> >>>>>>> + (pcr & SUN8I_PWM_PCR_MODE))
> >>>>>>> + return -EOPNOTSUPP;
> >>>>>>
> >>>>>> These need at least a comment.
> >>>>>
> >>>>> Added a comment explaining that the waveform API cannot represent pulse
> >>>>> mode or coupled dead-zone operation, and combined the rejection checks.
> >>>>>
> >>>>>>> + return 0;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static u64 sun8i_pwm_ticks_to_ns(u32 ticks, u16 div_k, u32 pair_rate)
> >>>>>>> +{
> >>>>>>> + return DIV_ROUND_UP_ULL(NSEC_PER_SEC * (u64)ticks * div_k,
> >>>>>>> + pair_rate);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static void
> >>>>>>> +sun8i_pwm_waveform_to_ns(const struct sun8i_pwm_waveform *wfhw,
> >>>>>>> + struct pwm_waveform *wf)
> >>>>>>> +{
> >>>>>>> + u32 pair_rate = wfhw->pair_rate;
> >>>>>>> + u16 div_k = wfhw->div_k;
> >>>>>>> +
> >>>>>>> + wf->duty_offset_ns = 0;
> >>>>>>> +
> >>>>>>> + if (!wfhw->enabled || !pair_rate) {
> >>>>>>> + wf->period_length_ns = 0;
> >>>>>>> + wf->duty_length_ns = 0;
> >>>>>>> + return;
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + if (wfhw->bypass_en) {
> >>>>>>> + wf->period_length_ns = DIV_ROUND_UP_ULL(NSEC_PER_SEC,
> >>>>>>> + pair_rate);
> >>>>>>> + wf->duty_length_ns =
> >>>>>>> + DIV_ROUND_UP_ULL(NSEC_PER_SEC,
> >>>>>>> + 2ULL * pair_rate);
> >>>>>>> + } else {
> >>>>>>> + wf->period_length_ns = sun8i_pwm_ticks_to_ns(wfhw->period_ticks,
> >>>>>>> + div_k, pair_rate);
> >>>>>>> + if (wfhw->active_state) {
> >>>>>>> + u32 duty_ticks = min(wfhw->duty_ticks,
> >>>>>>> + wfhw->period_ticks);
> >>>>>>> +
> >>>>>>> + wf->duty_length_ns =
> >>>>>>> + sun8i_pwm_ticks_to_ns(duty_ticks, div_k, pair_rate);
> >>>>>>> + } else if (!wfhw->duty_ticks) {
> >>>>>>> + wf->duty_length_ns = wf->period_length_ns;
> >>>>>>> + } else if (wfhw->duty_ticks >= wfhw->period_ticks) {
> >>>>>>> + wf->duty_length_ns = 0;
> >>>>>>> + } else {
> >>>>>>> + u32 low_ticks = wfhw->duty_ticks;
> >>>>>>> + u32 high_ticks = wfhw->period_ticks - wfhw->duty_ticks;
> >>>>>>> +
> >>>>>>> + wf->duty_length_ns =
> >>>>>>> + sun8i_pwm_ticks_to_ns(high_ticks, div_k, pair_rate);
> >>>>>>> + wf->duty_offset_ns =
> >>>>>>> + sun8i_pwm_ticks_to_ns(low_ticks, div_k, pair_rate);
> >>>>>>> + }
> >>>>>>> + }
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static int sun8i_pwm_round_waveform_fromhw(struct pwm_chip *chip,
> >>>>>>> + struct pwm_device *pwm,
> >>>>>>> + const void *_wfhw,
> >>>>>>> + struct pwm_waveform *wf)
> >>>>>>> +{
> >>>>>>> + const struct sun8i_pwm_waveform *wfhw = _wfhw;
> >>>>>>> +
> >>>>>>> + sun8i_pwm_waveform_to_ns(wfhw, wf);
> >>>>>>
> >>>>>> I suggest to rename that function to __sun8i_pwm_round_waveform_fromhw()
> >>>>>> or similar to make the relation between the two functions obvious.
> >>>>>
> >>>>> Renamed to __sun8i_pwm_round_waveform_fromhw().
> >>>>>
> >>>>>>> + dev_dbg(pwmchip_parent(chip),
> >>>>>>> + "pwm#%u: pair-rate=%u, div-k=%u, period=%u, duty=%u, bypass=%u, active-high=%u -> %llu/%llu [+%llu]\n",
> >>>>>>> + pwm->hwpwm, wfhw->pair_rate, wfhw->div_k,
> >>>>>>> + wfhw->period_ticks, wfhw->duty_ticks, wfhw->bypass_en,
> >>>>>>> + wfhw->active_state,
> >>>>>>> + wf->duty_length_ns, wf->period_length_ns,
> >>>>>>> + wf->duty_offset_ns);
> >>>>>>> +
> >>>>>>> + return 0;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static bool
> >>>>>>> +sun8i_pwm_pair_rate_constrained(struct sun8i_pwm_chip *sun8i_chip,
> >>>>>>> + unsigned int idx)
> >>>>>>> +{
> >>>>>>> + unsigned int sibling = idx ^ 1;
> >>>>>>> + unsigned long flags;
> >>>>>>> + bool constrained;
> >>>>>>> +
> >>>>>>> + if (sibling >= sun8i_chip->data->npwm)
> >>>>>>> + return false;
> >>>>>>> +
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + constrained = sun8i_chip->channels[sibling].mode ==
> >>>>>>> + SUN8I_PWM_MODE_CLK ||
> >>>>>>> + READ_ONCE(sun8i_chip->channels[sibling].rate_exclusive) ||
> >>>>>>> + sun8i_pwm_channel_is_enabled_locked(sun8i_chip,
> >>>>>>> + sibling);
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> +
> >>>>>>> + return constrained;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static int sun8i_pwm_clear_idle_deadzone(struct sun8i_pwm_chip *sun8i_chip,
> >>>>>>> + unsigned int idx)
> >>>>>>> +{
> >>>>>>> + unsigned int pair = SUN8I_PWM_PAIR_IDX(idx);
> >>>>>>> + unsigned long flags;
> >>>>>>> + u32 pdzcr, per;
> >>>>>>> + int ret = 0;
> >>>>>>> +
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + pdzcr = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PDZCR(pair));
> >>>>>>> + if (!(pdzcr & SUN8I_PWM_PDZCR_ENABLE))
> >>>>>>> + goto out_unlock;
> >>>>>>> +
> >>>>>>> + per = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PER);
> >>>>>>> + if (per & sun8i_pwm_pair_enable_mask(sun8i_chip, pair)) {
> >>>>>>> + ret = -EOPNOTSUPP;
> >>>>>>> + goto out_unlock;
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + pdzcr &= ~SUN8I_PWM_PDZCR_ENABLE;
> >>>>>>> + sun8i_pwm_writel(sun8i_chip, pdzcr, SUN8I_PWM_PDZCR(pair));
> >>>>>>> +
> >>>>>>> +out_unlock:
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> +
> >>>>>>> + return ret;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static u64 sun8i_pwm_max_period_ns(struct sun8i_pwm_chip *sun8i_chip,
> >>>>>>> + unsigned int idx)
> >>>>>>> +{
> >>>>>>> + struct sun8i_pwm_channel *chan = &sun8i_chip->channels[idx];
> >>>>>>> + unsigned long flags, pair_rate;
> >>>>>>> + u32 pcr;
> >>>>>>> + u16 div_k;
> >>>>>>> +
> >>>>>>> + pair_rate = clk_get_rate(chan->pair_clk);
> >>>>>>> + if (!pair_rate || pair_rate > U32_MAX)
> >>>>>>> + return 0;
> >>>>>>> +
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + pcr = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PCR(idx));
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> + div_k = FIELD_GET(SUN8I_PWM_PCR_PRESCAL_K_MASK, pcr) + 1;
> >>>>>>> +
> >>>>>>> + return sun8i_pwm_ticks_to_ns(SUN8I_PWM_PPR_PERIOD_MAX, div_k,
> >>>>>>> + pair_rate);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static int sun8i_pwm_wait_period_ready(struct sun8i_pwm_chip *sun8i_chip,
> >>>>>>> + unsigned int idx, u64 *timeout_us)
> >>>>>>> +{
> >>>>>>> + struct sun8i_pwm_channel *chan = &sun8i_chip->channels[idx];
> >>>>>>> + unsigned long poll_us;
> >>>>>>> + u64 period_ns;
> >>>>>>> + u32 val;
> >>>>>>> + int ret;
> >>>>>>> +
> >>>>>>> + /* PPR contains the pending value, not necessarily the active period. */
> >>>>>>> + period_ns = chan->pending_period_ns;
> >>>>>>> + if (!period_ns)
> >>>>>>> + period_ns = sun8i_pwm_max_period_ns(sun8i_chip, idx);
> >>>>>>> + *timeout_us = DIV_ROUND_UP_ULL(period_ns, NSEC_PER_USEC) +
> >>>>>>> + SUN8I_PWM_PERIOD_READY_MARGIN_US;
> >>>>>>> + poll_us = clamp_t(u64,
> >>>>>>> + DIV_ROUND_UP_ULL(*timeout_us,
> >>>>>>> + SUN8I_PWM_PERIOD_READY_POLL_COUNT),
> >>>>>>> + SUN8I_PWM_PERIOD_READY_MIN_POLL_US,
> >>>>>>> + SUN8I_PWM_PERIOD_READY_MAX_POLL_US);
> >>>>>>> +
> >>>>>>> + ret = readl_poll_timeout(sun8i_chip->base + SUN8I_PWM_PCR(idx), val,
> >>>>>>> + !(val & SUN8I_PWM_PCR_PERIOD_READY), poll_us,
> >>>>>>> + *timeout_us);
> >>>>>>> + if (!ret)
> >>>>>>> + chan->pending_period_ns = 0;
> >>>>>>> +
> >>>>>>> + return ret;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static int sun8i_pwm_write_waveform(struct pwm_chip *chip,
> >>>>>>> + struct pwm_device *pwm, const void *_wfhw)
> >>>>>>> +{
> >>>>>>> + const struct sun8i_pwm_waveform *wfhw = _wfhw;
> >>>>>>> + struct sun8i_pwm_chip *sun8i_chip = sun8i_pwm_from_chip(chip);
> >>>>>>> + struct sun8i_pwm_channel *chan = &sun8i_chip->channels[pwm->hwpwm];
> >>>>>>> + struct device *dev = pwmchip_parent(chip);
> >>>>>>> + unsigned long bus_rate, current_rate, restored_rate;
> >>>>>>> + unsigned long flags;
> >>>>>>> + u64 new_ns, old_ns, timeout_us;
> >>>>>>> + bool acquired_exclusive = false;
> >>>>>>> + bool can_restore_enable = true;
> >>>>>>> + bool current_bypass, needs_quiesce, was_enabled;
> >>>>>>> + u16 current_div_k;
> >>>>>>> + u32 old_ticks, pcr, ppr, val;
> >>>>>>> + int restore_ret, ret;
> >>>>>>> +
> >>>>>>> + if (!wfhw->enabled) {
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + sun8i_pwm_set_enabled_locked(sun8i_chip, pwm->hwpwm, false);
> >>>>>>> + sun8i_pwm_set_bypass_locked(sun8i_chip, pwm->hwpwm, false);
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> + if (READ_ONCE(chan->rate_exclusive)) {
> >>>>>>> + clk_rate_exclusive_put(chan->pair_clk);
> >>>>>>> + WRITE_ONCE(chan->rate_exclusive, false);
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + return 0;
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + if (!wfhw->pair_rate ||
> >>>>>>> + (!wfhw->bypass_en &&
> >>>>>>> + (!wfhw->div_k || wfhw->div_k > 256 ||
> >>>>>>> + !wfhw->period_ticks ||
> >>>>>>> + wfhw->period_ticks > SUN8I_PWM_PPR_PERIOD_MAX ||
> >>>>>>> + wfhw->duty_ticks > FIELD_MAX(SUN8I_PWM_PPR_DUTY_MASK))))
> >>>>>>> + return -EINVAL;
> >>>>>>> +
> >>>>>>> + if (!wfhw->bypass_en) {
> >>>>>>> + ret = sun8i_pwm_clear_idle_deadzone(sun8i_chip, pwm->hwpwm);
> >>>>>>> + if (ret)
> >>>>>>> + return ret;
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + current_rate = clk_get_rate(chan->pair_clk);
> >>>>>>> + if (!current_rate || current_rate > U32_MAX)
> >>>>>>> + return -ERANGE;
> >>>>>>> + bus_rate = clk_get_rate(sun8i_chip->bus_clk);
> >>>>>>> +
> >>>>>>> + if (!wfhw->bypass_en) {
> >>>>>>> + /* Do not overwrite a PPR update which has not latched yet. */
> >>>>>>> + ret = sun8i_pwm_wait_period_ready(sun8i_chip, pwm->hwpwm,
> >>>>>>> + &timeout_us);
> >>>>>>> + if (ret) {
> >>>>>>> + dev_err_ratelimited(dev, "pwm#%u: update timeout after %llu us\n",
> >>>>>>> + pwm->hwpwm, timeout_us);
> >>>>>>> + return ret;
> >>>>>>> + }
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + was_enabled = sun8i_pwm_channel_is_enabled_locked(sun8i_chip,
> >>>>>>> + pwm->hwpwm);
> >>>>>>> + val = sun8i_pwm_readl(sun8i_chip,
> >>>>>>> + SUN8I_PWM_PCCR(SUN8I_PWM_PAIR_IDX(pwm->hwpwm)));
> >>>>>>> + current_bypass = !!(val &
> >>>>>>> + BIT(SUN8I_PWM_PCCR_BYPASS_BIT(pwm->hwpwm)));
> >>>>>>> + pcr = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PCR(pwm->hwpwm));
> >>>>>>> + ppr = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PPR(pwm->hwpwm));
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> + current_div_k = FIELD_GET(SUN8I_PWM_PCR_PRESCAL_K_MASK, pcr) + 1;
> >>>>>>> +
> >>>>>>> + if (current_rate != wfhw->pair_rate &&
> >>>>>>> + sun8i_pwm_pair_rate_constrained(sun8i_chip, pwm->hwpwm))
> >>>>>>> + return -EBUSY;
> >>>>>>> +
> >>>>>>> + if (was_enabled && !READ_ONCE(chan->rate_exclusive)) {
> >>>>>>> + ret = clk_rate_exclusive_get(chan->pair_clk);
> >>>>>>> + if (ret)
> >>>>>>> + return ret;
> >>>>>>> + WRITE_ONCE(chan->rate_exclusive, true);
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + /*
> >>>>>>> + * Updating PPR while running is safe only if the input clocks and
> >>>>>>> + * polarity remain unchanged and PCLK is faster than the PWM clock.
> >>>>>>> + * Otherwise stop the channel before touching its configuration.
> >>>>>>> + */
> >>>>>>> + needs_quiesce = was_enabled &&
> >>>>>>> + (current_bypass != wfhw->bypass_en ||
> >>>>>>> + current_rate != wfhw->pair_rate ||
> >>>>>>> + (!wfhw->bypass_en &&
> >>>>>>> + (!!(pcr & SUN8I_PWM_PCR_ACTIVE_STATE) !=
> >>>>>>> + wfhw->active_state ||
> >>>>>>> + current_div_k != wfhw->div_k ||
> >>>>>>> + (pcr & SUN8I_PWM_PCR_MODE) ||
> >>>>>>> + !bus_rate ||
> >>>>>>> + bus_rate <= DIV_ROUND_UP_ULL(wfhw->pair_rate,
> >>>>>>> + wfhw->div_k))));
> >>>>>>> +
> >>>>>>> + if (needs_quiesce) {
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + sun8i_pwm_set_enabled_locked(sun8i_chip, pwm->hwpwm, false);
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + if (!was_enabled && !READ_ONCE(chan->rate_exclusive)) {
> >>>>>>> + if (current_rate == wfhw->pair_rate)
> >>>>>>> + ret = clk_rate_exclusive_get(chan->pair_clk);
> >>>>>>> + else
> >>>>>>> + ret = clk_set_rate_exclusive(chan->pair_clk,
> >>>>>>> + wfhw->pair_rate);
> >>>>>>> + if (ret)
> >>>>>>> + return ret;
> >>>>>>> + WRITE_ONCE(chan->rate_exclusive, true);
> >>>>>>> + acquired_exclusive = true;
> >>>>>>> + } else if (current_rate != wfhw->pair_rate) {
> >>>>>>> + ret = clk_set_rate(chan->pair_clk, wfhw->pair_rate);
> >>>>>>> + if (ret)
> >>>>>>> + goto restore_enable;
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + if (clk_get_rate(chan->pair_clk) != wfhw->pair_rate) {
> >>>>>>> + ret = -EINVAL;
> >>>>>>> + goto restore_rate;
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + if (!wfhw->bypass_en) {
> >>>>>>> + /* Return the channel to cycle mode without replaying W1S bits. */
> >>>>>>> + pcr &= ~(SUN8I_PWM_PCR_PRESCAL_K_MASK |
> >>>>>>> + SUN8I_PWM_PCR_ACTIVE_STATE | SUN8I_PWM_PCR_MODE |
> >>>>>>> + SUN8I_PWM_PCR_PULSE_START |
> >>>>>>> + SUN8I_PWM_PCR_PERIOD_READY);
> >>>>>>> + pcr |= FIELD_PREP(SUN8I_PWM_PCR_PRESCAL_K_MASK,
> >>>>>>> + wfhw->div_k - 1);
> >>>>>>> + if (wfhw->active_state)
> >>>>>>> + pcr |= SUN8I_PWM_PCR_ACTIVE_STATE;
> >>>>>>> + sun8i_pwm_writel(sun8i_chip, pcr, SUN8I_PWM_PCR(pwm->hwpwm));
> >>>>>>> +
> >>>>>>> + val = SUN8I_PWM_PPR_DUTY(wfhw->duty_ticks);
> >>>>>>> + val |= SUN8I_PWM_PPR_PERIOD(wfhw->period_ticks);
> >>>>>>> + old_ns = 0;
> >>>>>>> + if (was_enabled && !current_bypass) {
> >>>>>>> + old_ticks = SUN8I_PWM_PPR_PERIOD_VALUE(ppr);
> >>>>>>> + old_ns = sun8i_pwm_ticks_to_ns(old_ticks, current_div_k,
> >>>>>>> + current_rate);
> >>>>>>> + }
> >>>>>>> + new_ns = sun8i_pwm_ticks_to_ns(wfhw->period_ticks,
> >>>>>>> + wfhw->div_k, wfhw->pair_rate);
> >>>>>>> + chan->pending_period_ns = max(old_ns, new_ns);
> >>>>>>> + sun8i_pwm_writel(sun8i_chip, val, SUN8I_PWM_PPR(pwm->hwpwm));
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + sun8i_pwm_set_bypass_locked(sun8i_chip, pwm->hwpwm, wfhw->bypass_en);
> >>>>>>> + sun8i_pwm_set_enabled_locked(sun8i_chip, pwm->hwpwm, true);
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> +
> >>>>>>> + return 0;
> >>>>>>> +
> >>>>>>> +restore_rate:
> >>>>>>> + if (current_rate != wfhw->pair_rate) {
> >>>>>>> + restore_ret = clk_set_rate(chan->pair_clk, current_rate);
> >>>>>>> + restored_rate = clk_get_rate(chan->pair_clk);
> >>>>>>> + if (restore_ret || restored_rate != current_rate) {
> >>>>>>> + dev_err(dev,
> >>>>>>> + "pwm#%u: failed to restore pair clock rate %lu: %d (got %lu)\n",
> >>>>>>> + pwm->hwpwm, current_rate, restore_ret,
> >>>>>>> + restored_rate);
> >>>>>>> + can_restore_enable = false;
> >>>>>>> + }
> >>>>>>> + }
> >>>>>>> + if (acquired_exclusive) {
> >>>>>>> + clk_rate_exclusive_put(chan->pair_clk);
> >>>>>>> + WRITE_ONCE(chan->rate_exclusive, false);
> >>>>>>> + }
> >>>>>>> +restore_enable:
> >>>>>>> + if (needs_quiesce && can_restore_enable) {
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + sun8i_pwm_set_enabled_locked(sun8i_chip, pwm->hwpwm, true);
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + return ret;
> >>>>>>
> >>>>>> For a function that is supposed to just write wfhw into the hardware
> >>>>>> that is really complicated. Is this really necessary?
> >>>>>
> >>>>> There was avoidable complexity here. V9 encodes the channel register
> >>>>> values during waveform conversion and uses one preparation helper for
> >>>>> rate protection, quiescing and rollback. The write callback then applies
> >>>>> those values, without the redundant private-field validation.
> >>>>>
> >>>>> I kept the pending-period handshake and the checks for whether an update
> >>>>> can safely run live. Clock, prescaler, polarity and bypass transitions
> >>>>> may require stopping the output. After a failed rate change, restarting
> >>>>> it also requires restoring the old clock first; otherwise the old
> >>>>> register values could produce a different waveform.
> >>>>>
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +struct sun8i_pwm_rounding {
> >>>>>>> + const struct pwm_waveform *requested;
> >>>>>>> + struct sun8i_pwm_waveform best;
> >>>>>>> + struct pwm_waveform best_wf;
> >>>>>>> + u32 current_pair_rate;
> >>>>>>> + bool have_best;
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +static bool
> >>>>>>> +sun8i_pwm_candidate_is_better(const struct sun8i_pwm_rounding *rounding,
> >>>>>>> + const struct sun8i_pwm_waveform *candidate,
> >>>>>>> + const struct pwm_waveform *candidate_wf)
> >>>>>>> +{
> >>>>>>> + const struct pwm_waveform *requested = rounding->requested;
> >>>>>>> + bool candidate_period_down, best_period_down;
> >>>>>>> +
> >>>>>>> + /* A zero-duty, zero-offset cycle-mode candidate always exists. */
> >>>>>>> + if (candidate_wf->duty_length_ns > requested->duty_length_ns ||
> >>>>>>> + candidate_wf->duty_offset_ns > requested->duty_offset_ns)
> >>>>>>> + return false;
> >>>>>>> +
> >>>>>>> + if (!rounding->have_best)
> >>>>>>> + return true;
> >>>>>>> +
> >>>>>>> + candidate_period_down =
> >>>>>>> + candidate_wf->period_length_ns <= requested->period_length_ns;
> >>>>>>> + best_period_down =
> >>>>>>> + rounding->best_wf.period_length_ns <= requested->period_length_ns;
> >>>>>>> + if (candidate_period_down != best_period_down)
> >>>>>>> + return candidate_period_down;
> >>>>>>> +
> >>>>>>> + if (candidate_wf->period_length_ns !=
> >>>>>>> + rounding->best_wf.period_length_ns) {
> >>>>>>> + if (candidate_period_down)
> >>>>>>> + return candidate_wf->period_length_ns >
> >>>>>>> + rounding->best_wf.period_length_ns;
> >>>>>>> +
> >>>>>>> + return candidate_wf->period_length_ns <
> >>>>>>> + rounding->best_wf.period_length_ns;
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + if (candidate_wf->duty_length_ns !=
> >>>>>>> + rounding->best_wf.duty_length_ns)
> >>>>>>> + return candidate_wf->duty_length_ns >
> >>>>>>> + rounding->best_wf.duty_length_ns;
> >>>>>>> +
> >>>>>>> + if (candidate_wf->duty_offset_ns !=
> >>>>>>> + rounding->best_wf.duty_offset_ns)
> >>>>>>> + return candidate_wf->duty_offset_ns >
> >>>>>>> + rounding->best_wf.duty_offset_ns;
> >>>>>>> +
> >>>>>>> + /* Avoid a shared pair-rate change when two settings are equivalent. */
> >>>>>>> + return candidate->pair_rate == rounding->current_pair_rate &&
> >>>>>>> + rounding->best.pair_rate != rounding->current_pair_rate;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static void
> >>>>>>> +sun8i_pwm_consider_candidate(struct sun8i_pwm_rounding *rounding,
> >>>>>>> + const struct sun8i_pwm_waveform *candidate)
> >>>>>>> +{
> >>>>>>> + struct pwm_waveform candidate_wf;
> >>>>>>> +
> >>>>>>> + sun8i_pwm_waveform_to_ns(candidate, &candidate_wf);
> >>>>>>> + if (!sun8i_pwm_candidate_is_better(rounding, candidate,
> >>>>>>> + &candidate_wf))
> >>>>>>> + return;
> >>>>>>> +
> >>>>>>> + rounding->best = *candidate;
> >>>>>>> + rounding->best_wf = candidate_wf;
> >>>>>>> + rounding->have_best = true;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static void
> >>>>>>> +sun8i_pwm_consider_normal(struct sun8i_pwm_rounding *rounding, u32 pair_rate,
> >>>>>>> + u16 div_k)
> >>>>>>> +{
> >>>>>>> + const struct pwm_waveform *requested = rounding->requested;
> >>>>>>> + struct sun8i_pwm_waveform candidate = {
> >>>>>>> + .enabled = true,
> >>>>>>> + .pair_rate = pair_rate,
> >>>>>>> + .div_k = div_k,
> >>>>>>> + };
> >>>>>>> + u64 denominator = NSEC_PER_SEC * (u64)div_k;
> >>>>>>> + u64 duty_ticks, period_ticks;
> >>>>>>> + u64 duty_ns = requested->duty_length_ns;
> >>>>>>> + u64 period_ns = requested->period_length_ns;
> >>>>>>> + u32 inactive_ticks;
> >>>>>>> +
> >>>>>>> + period_ticks = mul_u64_u64_div_u64(period_ns, pair_rate, denominator);
> >>>>>>> + period_ticks = clamp_t(u64, period_ticks, 1, SUN8I_PWM_PPR_PERIOD_MAX);
> >>>>>>> + candidate.period_ticks = period_ticks;
> >>>>>>> + if (candidate.period_ticks > 1 &&
> >>>>>>> + sun8i_pwm_ticks_to_ns(candidate.period_ticks, div_k, pair_rate) >
> >>>>>>> + requested->period_length_ns)
> >>>>>>> + candidate.period_ticks--;
> >>>>>>> +
> >>>>>>> + duty_ticks = mul_u64_u64_div_u64(duty_ns, pair_rate, denominator);
> >>>>>>> + duty_ticks = min_t(u64, duty_ticks, candidate.period_ticks);
> >>>>>>> + if (duty_ticks &&
> >>>>>>> + sun8i_pwm_ticks_to_ns(duty_ticks, div_k, pair_rate) >
> >>>>>>> + requested->duty_length_ns)
> >>>>>>> + duty_ticks--;
> >>>>>>> +
> >>>>>>> + /*
> >>>>>>> + * Zero active ticks encode either constant level. For a toggling
> >>>>>>> + * waveform, active-low mode places the rising edge after the inactive
> >>>>>>> + * part, which is the only non-zero offset supported by the hardware.
> >>>>>>> + */
> >>>>>>> + if (!duty_ticks) {
> >>>>>>> + candidate.active_state = true;
> >>>>>>> + candidate.duty_ticks = 0;
> >>>>>>> + } else if (duty_ticks == candidate.period_ticks) {
> >>>>>>> + candidate.active_state = false;
> >>>>>>> + candidate.duty_ticks = 0;
> >>>>>>> + } else {
> >>>>>>> + inactive_ticks = candidate.period_ticks - duty_ticks;
> >>>>>>> + if (sun8i_pwm_ticks_to_ns(inactive_ticks, div_k, pair_rate) <=
> >>>>>>> + requested->duty_offset_ns) {
> >>>>>>> + candidate.active_state = false;
> >>>>>>> + candidate.duty_ticks = inactive_ticks;
> >>>>>>> + } else {
> >>>>>>> + candidate.active_state = true;
> >>>>>>> + candidate.duty_ticks = duty_ticks;
> >>>>>>> + }
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + sun8i_pwm_consider_candidate(rounding, &candidate);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static void
> >>>>>>> +sun8i_pwm_consider_pair_rate(struct sun8i_pwm_rounding *rounding,
> >>>>>>> + u32 pair_rate)
> >>>>>>> +{
> >>>>>>> + struct sun8i_pwm_waveform bypass = {
> >>>>>>> + .duty_ticks = 1,
> >>>>>>> + .period_ticks = 1,
> >>>>>>> + .pair_rate = pair_rate,
> >>>>>>> + .div_k = 1,
> >>>>>>> + .enabled = true,
> >>>>>>> + .active_state = true,
> >>>>>>> + .bypass_en = true,
> >>>>>>> + };
> >>>>>>> +
> >>>>>>> + for (unsigned int div_k = 1; div_k <= 256; div_k++)
> >>>>>>> + sun8i_pwm_consider_normal(rounding, pair_rate, div_k);
> >>>>>>> +
> >>>>>>> + sun8i_pwm_consider_candidate(rounding, &bypass);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static int sun8i_pwm_round_waveform_tohw(struct pwm_chip *chip,
> >>>>>>> + struct pwm_device *pwm,
> >>>>>>> + const struct pwm_waveform *wf,
> >>>>>>> + void *_wfhw)
> >>>>>>> +{
> >>>>>>> + struct sun8i_pwm_chip *sun8i_chip = sun8i_pwm_from_chip(chip);
> >>>>>>> + struct sun8i_pwm_channel *chan = &sun8i_chip->channels[pwm->hwpwm];
> >>>>>>> + struct sun8i_pwm_pair *pair =
> >>>>>>> + &sun8i_chip->pairs[SUN8I_PWM_PAIR_IDX(pwm->hwpwm)];
> >>>>>>> + struct sun8i_pwm_waveform *wfhw = _wfhw;
> >>>>>>> + struct sun8i_pwm_rounding rounding = {
> >>>>>>> + .requested = wf,
> >>>>>>> + };
> >>>>>>> + unsigned long current_rate;
> >>>>>>> +
> >>>>>>> + if (!wf->period_length_ns) {
> >>>>>>> + *wfhw = (struct sun8i_pwm_waveform) {
> >>>>>>> + .enabled = false,
> >>>>>>> + };
> >>>>>>> + return 0;
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + current_rate = clk_get_rate(chan->pair_clk);
> >>>>>>> + if (!current_rate || current_rate > U32_MAX)
> >>>>>>> + return -ERANGE;
> >>>>>>> + rounding.current_pair_rate = current_rate;
> >>>>>>> +
> >>>>>>> + if (sun8i_pwm_pair_rate_constrained(sun8i_chip, pwm->hwpwm)) {
> >>>>>>> + sun8i_pwm_consider_pair_rate(&rounding, current_rate);
> >>>>>>
> >>>>>> These consider functions make an over-engineered impression on me. Over
> >>>>>> all I have the impression this driver is more complicated than
> >>>>>> necessary. I didn't recheck on Richard's previous iteration, but that
> >>>>>> seemed simpler (read: better) to me.
> >>>>>
> >>>>> The fixed-rate/divider conversion is now a pure helper, and candidate
> >>>>> comparison and selection use one function. The redundant corrective
> >>>>> rounding steps are gone too.
> >>>>>
> >>>>> I retained the bounded search across parents, dividers and bypass so the
> >>>>> choice follows the period, duty and offset ordering rather than a
> >>>>> preferred-parent heuristic. It still handles inverted waveforms,
> >>>>> constant levels and the 65536-tick period boundary.
> >>>>>
> >>>>>>> + } else {
> >>>>>>> + for (unsigned int parent_idx = 0;
> >>>>>>> + parent_idx < clk_hw_get_num_parents(pair->hw);
> >>>>>>> + parent_idx++) {
> >>>>>>> + struct clk_hw *parent;
> >>>>>>> + unsigned long parent_rate;
> >>>>>>> +
> >>>>>>> + parent = clk_hw_get_parent_by_index(pair->hw, parent_idx);
> >>>>>>> + if (!parent)
> >>>>>>> + continue;
> >>>>>>> + parent_rate = clk_hw_get_rate(parent);
> >>>>>>> + if (!parent_rate)
> >>>>>>> + continue;
> >>>>>>> +
> >>>>>>> + for (unsigned int i = 0; sun8i_pwm_div_m_table[i].div;
> >>>>>>> + i++) {
> >>>>>>> + u16 div_m = sun8i_pwm_div_m_table[i].div;
> >>>>>>> + u64 pair_rate;
> >>>>>>> +
> >>>>>>> + pair_rate = DIV_ROUND_UP_ULL(parent_rate, div_m);
> >>>>>>> + if (!pair_rate || pair_rate > U32_MAX)
> >>>>>>> + continue;
> >>>>>>> + sun8i_pwm_consider_pair_rate(&rounding, pair_rate);
> >>>>>>> + }
> >>>>>>> + }
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + if (!rounding.have_best)
> >>>>>>> + return -EINVAL;
> >>>>>>> + *wfhw = rounding.best;
> >>>>>>> +
> >>>>>>> + dev_dbg(pwmchip_parent(chip),
> >>>>>>> + "pwm#%u: %llu/%llu [+%llu] -> pair-rate=%u, div-k=%u, period=%u, duty=%u, bypass=%u, active-high=%u\n",
> >>>>>>> + pwm->hwpwm, wf->duty_length_ns, wf->period_length_ns,
> >>>>>>> + wf->duty_offset_ns, wfhw->pair_rate, wfhw->div_k,
> >>>>>>> + wfhw->period_ticks, wfhw->duty_ticks, wfhw->bypass_en,
> >>>>>>> + wfhw->active_state);
> >>>>>>> +
> >>>>>>> + return rounding.best_wf.period_length_ns > wf->period_length_ns;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static const struct pwm_ops sun8i_pwm_ops = {
> >>>>>>> + .request = sun8i_pwm_request,
> >>>>>>> + .free = sun8i_pwm_free,
> >>>>>>> + .sizeof_wfhw = sizeof(struct sun8i_pwm_waveform),
> >>>>>>> + .round_waveform_tohw = sun8i_pwm_round_waveform_tohw,
> >>>>>>> + .round_waveform_fromhw = sun8i_pwm_round_waveform_fromhw,
> >>>>>>> + .read_waveform = sun8i_pwm_read_waveform,
> >>>>>>> + .write_waveform = sun8i_pwm_write_waveform,
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +/* Register the shared mux, gate and /div_m clock for each channel pair. */
> >>>>>>> +static int sun8i_pwm_register_pair_clocks(struct device *dev,
> >>>>>>> + struct sun8i_pwm_chip *sun8i_chip)
> >>>>>>> +{
> >>>>>>> + static const struct clk_parent_data parent_data[] = {
> >>>>>>> + { .index = 0 },
> >>>>>>> + { .index = 1 },
> >>>>>>> + };
> >>>>>>> + unsigned int num_pairs = DIV_ROUND_UP(sun8i_chip->data->npwm, 2);
> >>>>>>> +
> >>>>>>> + for (unsigned int i = 0; i < num_pairs; i++) {
> >>>>>>> + struct sun8i_pwm_pair *pair = &sun8i_chip->pairs[i];
> >>>>>>> + void __iomem *reg = sun8i_chip->base + SUN8I_PWM_PCCR(i);
> >>>>>>> + struct clk *pair_clk;
> >>>>>>> + const char *name;
> >>>>>>> + int ret;
> >>>>>>> +
> >>>>>>> + name = devm_kasprintf(dev, GFP_KERNEL,
> >>>>>>> + "%s#pwm-clk-src%u%u", dev_name(dev),
> >>>>>>> + i * 2, i * 2 + 1);
> >>>>>>> + if (!name)
> >>>>>>> + return -ENOMEM;
> >>>>>>> +
> >>>>>>> + pair->mux.reg = reg;
> >>>>>>> + pair->mux.shift = SUN8I_PWM_PCCR_SRC_SHIFT;
> >>>>>>> + pair->mux.mask = SUN8I_PWM_PCCR_SRC_MASK;
> >>>>>>> + pair->mux.flags = CLK_MUX_ROUND_CLOSEST;
> >>>>>>> + pair->mux.lock = &sun8i_chip->lock;
> >>>>>>> +
> >>>>>>> + pair->chip = sun8i_chip;
> >>>>>>> + pair->index = i;
> >>>>>>> +
> >>>>>>> + pair->divider.reg = reg;
> >>>>>>> + pair->divider.shift = SUN8I_PWM_PCCR_DIV_M_SHIFT;
> >>>>>>> + pair->divider.width = SUN8I_PWM_PCCR_DIV_M_WIDTH;
> >>>>>>> + pair->divider.table = sun8i_pwm_div_m_table;
> >>>>>>> + pair->divider.lock = &sun8i_chip->lock;
> >>>>>>> +
> >>>>>>> + pair->hw = devm_clk_hw_register_composite_pdata(dev, name,
> >>>>>>> + parent_data,
> >>>>>>> + ARRAY_SIZE(parent_data),
> >>>>>>> + &pair->mux.hw, &clk_mux_ops,
> >>>>>>> + &pair->divider.hw, &clk_divider_ops,
> >>>>>>> + &pair->gate_hw, &sun8i_pwm_pair_gate_ops, 0);
> >>>>>>> + if (IS_ERR(pair->hw))
> >>>>>>> + return dev_err_probe(dev, PTR_ERR(pair->hw),
> >>>>>>> + "Failed to register pair %u clock\n", i);
> >>>>>>> +
> >>>>>>> + pair_clk = devm_clk_hw_get_clk(dev, pair->hw, NULL);
> >>>>>>> + if (IS_ERR(pair_clk))
> >>>>>>> + return dev_err_probe(dev, PTR_ERR(pair_clk),
> >>>>>>> + "Failed to get pair %u clock\n", i);
> >>>>>>> +
> >>>>>>> + pair->rate_nb.notifier_call = sun8i_pwm_pair_rate_notifier;
> >>>>>>> + ret = devm_clk_notifier_register(dev, pair_clk, &pair->rate_nb);
> >>>>>>> + if (ret)
> >>>>>>> + return dev_err_probe(dev, ret,
> >>>>>>> + "Failed to protect pair %u clock rate\n", i);
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + return 0;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +/* Register a pair-clock consumer and the bypass clock for each channel. */
> >>>>>>> +static int sun8i_pwm_register_channel_clocks(struct device *dev,
> >>>>>>> + struct sun8i_pwm_chip *sun8i_chip)
> >>>>>>> +{
> >>>>>>> + for (unsigned int i = 0; i < sun8i_chip->data->npwm; i++) {
> >>>>>>> + struct sun8i_pwm_channel *chan = &sun8i_chip->channels[i];
> >>>>>>> + struct clk_hw *parent = sun8i_chip->pairs[SUN8I_PWM_PAIR_IDX(i)].hw;
> >>>>>>> + struct clk_parent_data parent_data = { .hw = parent };
> >>>>>>> + struct clk_init_data init = {};
> >>>>>>> + const char *name;
> >>>>>>> + int ret;
> >>>>>>> +
> >>>>>>> + /* Separate handles let CCF arbitrate sibling rate exclusivity. */
> >>>>>>> + chan->pair_clk = devm_clk_hw_get_clk(dev, parent, NULL);
> >>>>>>> + if (IS_ERR(chan->pair_clk))
> >>>>>>> + return dev_err_probe(dev, PTR_ERR(chan->pair_clk),
> >>>>>>> + "Failed to get pair clock for PWM %u\n", i);
> >>>>>>> +
> >>>>>>> + name = devm_kasprintf(dev, GFP_KERNEL, "%s#pwm-bypass%u",
> >>>>>>> + dev_name(dev), i);
> >>>>>>> + if (!name)
> >>>>>>> + return -ENOMEM;
> >>>>>>> +
> >>>>>>> + chan->bypass.chip = sun8i_chip;
> >>>>>>> + chan->bypass.index = i;
> >>>>>>> + init.name = name;
> >>>>>>> + init.ops = &sun8i_pwm_bypass_ops;
> >>>>>>> + init.parent_data = &parent_data;
> >>>>>>> + init.num_parents = 1;
> >>>>>>> + /* Keep the shared pair rate stable for the prepared lifetime. */
> >>>>>>> + init.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE;
> >>>>>>> + chan->bypass.hw.init = &init;
> >>>>>>> +
> >>>>>>> + ret = devm_clk_hw_register(dev, &chan->bypass.hw);
> >>>>>>> + if (ret)
> >>>>>>> + return dev_err_probe(dev, ret,
> >>>>>>> + "Failed to register bypass clock %u\n", i);
> >>>>>>> +
> >>>>>>> + sun8i_chip->hw_data->hws[i] = &chan->bypass.hw;
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + return 0;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +/*
> >>>>>>> + * A disabled pair gate makes any set PER bits ineffective. Clear those stale
> >>>>>>> + * enables before a clock consumer can turn the shared gate on and expose an
> >>>>>>> + * unrequested output.
> >>>>>>> + */
> >>>>>>> +static void
> >>>>>>> +sun8i_pwm_sanitize_disabled_pairs(struct sun8i_pwm_chip *sun8i_chip)
> >>>>>>> +{
> >>>>>>> + unsigned int num_pairs = DIV_ROUND_UP(sun8i_chip->data->npwm, 2);
> >>>>>>> + unsigned long flags;
> >>>>>>> + u32 per;
> >>>>>>> +
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + per = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PER);
> >>>>>>> + for (unsigned int pair = 0; pair < num_pairs; pair++) {
> >>>>>>> + unsigned int first = pair * 2;
> >>>>>>> + u32 pccr;
> >>>>>>> +
> >>>>>>> + pccr = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PCCR(pair));
> >>>>>>> + if (pccr & SUN8I_PWM_PCCR_GATE)
> >>>>>>> + continue;
> >>>>>>> +
> >>>>>>> + per &= ~SUN8I_PWM_ENABLE(first);
> >>>>>>> + pccr &= ~BIT(SUN8I_PWM_PCCR_BYPASS_BIT(first));
> >>>>>>> + if (first + 1 < sun8i_chip->data->npwm) {
> >>>>>>> + per &= ~SUN8I_PWM_ENABLE(first + 1);
> >>>>>>> + pccr &= ~BIT(SUN8I_PWM_PCCR_BYPASS_BIT(first + 1));
> >>>>>>> + }
> >>>>>>> + sun8i_pwm_writel(sun8i_chip, pccr, SUN8I_PWM_PCCR(pair));
> >>>>>>> + }
> >>>>>>> + sun8i_pwm_writel(sun8i_chip, per, SUN8I_PWM_PER);
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +/*
> >>>>>>> + * Keep firmware-active outputs running until all declared consumers have had
> >>>>>>> + * a chance to claim them. Once the driver core calls sync_state(), any channel
> >>>>>>> + * which still has no PWM or clock owner can be stopped.
> >>>>>>
> >>>>>> Never saw that callback before, I hesitate to like it. The idea is to
> >>>>>> disable the PWMs if there is no consumer after boot-up? If this is
> >>>>>> considered a good idea, I'd prefer to do that at the pwm core level for
> >>>>>> all drivers/devices in the same way.
> >>>>>
> >>>>> Removed sync_state() and the driver-local unclaimed-output shutdown
> >>>>> policy. V9 leaves firmware-active outputs running until a consumer
> >>>>> changes them, while retaining the hardware-enable checks which protect
> >>>>> their shared gate and rate.
> >>>>>
> >>>>>>> + */
> >>>>>>> +static void sun8i_pwm_sync_state(struct device *dev)
> >>>>>>> +{
> >>>>>>> + struct sun8i_pwm_chip *sun8i_chip = dev_get_drvdata(dev);
> >>>>>>> + unsigned int num_pairs = DIV_ROUND_UP(sun8i_chip->data->npwm, 2);
> >>>>>>> + unsigned long empty_pairs = 0;
> >>>>>>> + unsigned long flags;
> >>>>>>> + u32 active, per, unclaimed = 0;
> >>>>>>> +
> >>>>>>> + spin_lock_irqsave(&sun8i_chip->lock, flags);
> >>>>>>> + per = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PER);
> >>>>>>> + for (unsigned int pair = 0; pair < num_pairs; pair++) {
> >>>>>>> + u32 pair_mask = sun8i_pwm_pair_enable_mask(sun8i_chip, pair);
> >>>>>>> + u32 owner_mask;
> >>>>>>> + u32 pdzcr;
> >>>>>>> +
> >>>>>>> + owner_mask = sun8i_pwm_pair_owner_mask_locked(sun8i_chip, pair);
> >>>>>>> + pdzcr = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PDZCR(pair));
> >>>>>>> + /* Dead-zone mode couples both outputs into one waveform. */
> >>>>>>> + if ((pdzcr & SUN8I_PWM_PDZCR_ENABLE) &&
> >>>>>>> + (per & owner_mask))
> >>>>>>> + continue;
> >>>>>>> +
> >>>>>>> + unclaimed |= pair_mask & ~owner_mask;
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + active = per & unclaimed;
> >>>>>>> + per &= ~unclaimed;
> >>>>>>> + sun8i_pwm_writel(sun8i_chip, per, SUN8I_PWM_PER);
> >>>>>>> +
> >>>>>>> + for (unsigned int pair = 0; pair < num_pairs; pair++) {
> >>>>>>> + unsigned int first = pair * 2;
> >>>>>>> + u32 pccr;
> >>>>>>> +
> >>>>>>> + pccr = sun8i_pwm_readl(sun8i_chip, SUN8I_PWM_PCCR(pair));
> >>>>>>> + if (unclaimed & SUN8I_PWM_ENABLE(first))
> >>>>>>> + pccr &= ~BIT(SUN8I_PWM_PCCR_BYPASS_BIT(first));
> >>>>>>> + if (first + 1 < sun8i_chip->data->npwm &&
> >>>>>>> + unclaimed & SUN8I_PWM_ENABLE(first + 1))
> >>>>>>> + pccr &= ~BIT(SUN8I_PWM_PCCR_BYPASS_BIT(first + 1));
> >>>>>>> + sun8i_pwm_writel(sun8i_chip, pccr, SUN8I_PWM_PCCR(pair));
> >>>>>>> +
> >>>>>>> + if (!(per & sun8i_pwm_pair_enable_mask(sun8i_chip, pair)))
> >>>>>>> + empty_pairs |= BIT(pair);
> >>>>>>> + }
> >>>>>>> + spin_unlock_irqrestore(&sun8i_chip->lock, flags);
> >>>>>>> +
> >>>>>>> + /*
> >>>>>>> + * Reconcile empty pair gates through CCF. Taking and dropping a
> >>>>>>> + * temporary reference leaves a gate with another CCF user enabled, but
> >>>>>>> + * disables a firmware-enabled gate whose software count is still zero.
> >>>>>>> + */
> >>>>>>> + for (unsigned int pair = 0; pair < num_pairs; pair++) {
> >>>>>>> + struct clk *pair_clk;
> >>>>>>> + int ret;
> >>>>>>> +
> >>>>>>> + if (!(empty_pairs & BIT(pair)))
> >>>>>>> + continue;
> >>>>>>> +
> >>>>>>> + pair_clk = sun8i_chip->channels[pair * 2].pair_clk;
> >>>>>>> + ret = clk_prepare_enable(pair_clk);
> >>>>>>> + if (ret) {
> >>>>>>> + dev_warn(dev, "Failed to synchronize pair %u gate: %pe\n",
> >>>>>>> + pair, ERR_PTR(ret));
> >>>>>>> + continue;
> >>>>>>> + }
> >>>>>>> + clk_disable_unprepare(pair_clk);
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + if (active)
> >>>>>>> + dev_dbg(dev, "disabled unclaimed firmware outputs %#x\n", active);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static int sun8i_pwm_init_clocks(struct device *dev,
> >>>>>>> + struct sun8i_pwm_chip *sun8i_chip)
> >>>>>>> +{
> >>>>>>> + unsigned int num_pairs = DIV_ROUND_UP(sun8i_chip->data->npwm, 2);
> >>>>>>> + size_t hw_data_size;
> >>>>>>> + int ret;
> >>>>>>> +
> >>>>>>> + sun8i_chip->pairs = devm_kcalloc(dev, num_pairs,
> >>>>>>> + sizeof(*sun8i_chip->pairs), GFP_KERNEL);
> >>>>>>> + if (!sun8i_chip->pairs)
> >>>>>>> + return -ENOMEM;
> >>>>>>> +
> >>>>>>> + hw_data_size = struct_size(sun8i_chip->hw_data, hws,
> >>>>>>> + sun8i_chip->data->npwm);
> >>>>>>> + sun8i_chip->hw_data = devm_kzalloc(dev, hw_data_size, GFP_KERNEL);
> >>>>>>> + if (!sun8i_chip->hw_data)
> >>>>>>> + return -ENOMEM;
> >>>>>>
> >>>>>> Can you fold these allocations into a single one (that is, have
> >>>>>> devm_pwmchip_alloc() as only allocation call)? That should give better
> >>>>>> cache locality and reduce fragmentation.
> >>>>>
> >>>>> The three pair objects and six channel objects are now embedded in the
> >>>>> private structure allocated by devm_pwmchip_alloc(). The separate onecell
> >>>>> table is gone; clock lookup uses the channel array directly. Clock-name
> >>>>> allocations and CCF's own allocations remain.
> >>>>>
> >>>>>>> + sun8i_chip->hw_data->num = sun8i_chip->data->npwm;
> >>>>>>> + sun8i_pwm_sanitize_disabled_pairs(sun8i_chip);
> >>>>>>> +
> >>>>>>> + ret = sun8i_pwm_register_pair_clocks(dev, sun8i_chip);
> >>>>>>> + if (ret)
> >>>>>>> + return ret;
> >>>>>>> +
> >>>>>>> + return sun8i_pwm_register_channel_clocks(dev, sun8i_chip);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static int sun8i_pwm_probe(struct platform_device *pdev)
> >>>>>>> +{
> >>>>>>> + const struct sun8i_pwm_data *data;
> >>>>>>> + struct device *dev = &pdev->dev;
> >>>>>>> + struct sun8i_pwm_chip *sun8i_chip;
> >>>>>>> + struct reset_control *rst;
> >>>>>>> + struct pwm_chip *chip;
> >>>>>>> + int ret;
> >>>>>>> +
> >>>>>>> + data = of_device_get_match_data(dev);
> >>>>>>> + if (!data)
> >>>>>>> + return dev_err_probe(dev, -ENODEV,
> >>>>>>> + "Missing specific data structure\n");
> >>>>>>> +
> >>>>>>> + chip = devm_pwmchip_alloc(dev, data->npwm, sizeof(*sun8i_chip));
> >>>>>>> + if (IS_ERR(chip))
> >>>>>>> + return dev_err_probe(dev, PTR_ERR(chip),
> >>>>>>> + "Failed to allocate pwmchip\n");
> >>>>>>> +
> >>>>>>> + sun8i_chip = sun8i_pwm_from_chip(chip);
> >>>>>>> + sun8i_chip->data = data;
> >>>>>>> + spin_lock_init(&sun8i_chip->lock);
> >>>>>>> + sun8i_chip->base = devm_platform_ioremap_resource(pdev, 0);
> >>>>>>> + if (IS_ERR(sun8i_chip->base))
> >>>>>>> + return dev_err_probe(dev, PTR_ERR(sun8i_chip->base),
> >>>>>>> + "Failed to get PWM base address\n");
> >>>>>>> +
> >>>>>>> + sun8i_chip->bus_clk = devm_clk_get_enabled(dev, "bus");
> >>>>>>> + if (IS_ERR(sun8i_chip->bus_clk))
> >>>>>>> + return dev_err_probe(dev, PTR_ERR(sun8i_chip->bus_clk),
> >>>>>>> + "Failed to get bus clock\n");
> >>>>>>> +
> >>>>>>> + rst = devm_reset_control_get_shared_deasserted(dev, NULL);
> >>>>>>> + if (IS_ERR(rst))
> >>>>>>> + return dev_err_probe(dev, PTR_ERR(rst),
> >>>>>>> + "Failed to get reset control\n");
> >>>>>>> +
> >>>>>>> + sun8i_chip->channels = devm_kcalloc(dev, data->npwm,
> >>>>>>> + sizeof(*sun8i_chip->channels),
> >>>>>>> + GFP_KERNEL);
> >>>>>>> + if (!sun8i_chip->channels)
> >>>>>>> + return dev_err_probe(dev, -ENOMEM,
> >>>>>>> + "Failed to allocate %d channels array\n",
> >>>>>>> + data->npwm);
> >>>>>>> +
> >>>>>>> + chip->ops = &sun8i_pwm_ops;
> >>>>>>> +
> >>>>>>> + ret = sun8i_pwm_init_clocks(dev, sun8i_chip);
> >>>>>>> + if (ret)
> >>>>>>> + return ret;
> >>>>>>> +
> >>>>>>> + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> >>>>>>> + sun8i_chip->hw_data);
> >>>>>>> + if (ret)
> >>>>>>> + return dev_err_probe(dev, ret, "Failed to add HW clock provider\n");
> >>>>>>> +
> >>>>>>> + ret = devm_pwmchip_add(dev, chip);
> >>>>>>> + if (ret < 0)
> >>>>>>> + return dev_err_probe(dev, ret, "Failed to add PWM chip\n");
> >>>>>>> +
> >>>>>>> + platform_set_drvdata(pdev, sun8i_chip);
> >>>>>>> +
> >>>>>>> + return 0;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static const struct sun8i_pwm_data sun50i_h616_pwm_data = {
> >>>>>>> + .npwm = 6,
> >>>>>>
> >>>>>> Are there new variants of that hardware already in the pipeline and
> >>>>>> these are known to differ by npwm only? If not, please hardcode that 6,
> >>>>>> or use a dt property (`npwms = <6>`). There is no need to do device
> >>>>>> variant handling if there is only a single variant.
> >>>>>
> >>>>> Removed the single-variant match data and fixed the topology at six
> >>>>> channels. No npwms property is added.
> >>>>>
> >>>>>>
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +static const struct of_device_id sun8i_pwm_dt_ids[] = {
> >>>>>>> + {
> >>>>>>> + .compatible = "allwinner,sun50i-h616-pwm",
> >>>>>>> + .data = &sun50i_h616_pwm_data,
> >>>>>>> + }, {
> >>>>>>> + /* sentinel */
> >>>>>>> + }
> >>>>>>> +};
> >>>>>>> +MODULE_DEVICE_TABLE(of, sun8i_pwm_dt_ids);
> >>>>>>> +
> >>>>>>> +static struct platform_driver sun8i_pwm_driver = {
> >>>>>>> + .driver = {
> >>>>>>> + .name = "sun8i-pwm",
> >>>>>>> + .of_match_table = sun8i_pwm_dt_ids,
> >>>>>>> + .sync_state = sun8i_pwm_sync_state,
> >>>>>>> + },
> >>>>>>> + .probe = sun8i_pwm_probe,
> >>>>>>> +};
> >>>>>>> +module_platform_driver(sun8i_pwm_driver);
> >>>>>>> +
> >>>>>>> +MODULE_AUTHOR("Richard Genoud <richard.genoud at bootlin.com>");
> >>>>>>> +MODULE_DESCRIPTION("Allwinner sun8i PWM driver");
> >>>>>>> +MODULE_LICENSE("GPL");
> >>>>>>>
> >>>>>>> --
> >>>>>>> 2.53.0
> >>>>>>>
> >>>>
> >>>>
> >>>> --
> >>>> Richard Genoud, Bootlin
> >>>> Embedded Linux and Kernel engineering
> >>>> https://bootlin.com
> >>
> >>
> >> --
> >> Richard Genoud, Bootlin
> >> Embedded Linux and Kernel engineering
> >> https://bootlin.com
>
>
> --
> Richard Genoud, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
More information about the linux-arm-kernel
mailing list