[PATCH v8 2/4] pwm: sun8i: Add H616 PWM support
Richard GENOUD
richard.genoud at bootlin.com
Wed Sep 23 00:16:28 PDT 2026
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.
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
More information about the linux-arm-kernel
mailing list