Re: [PATCH v3 0/2] pwm: Add the Ingenic X1600 PWM controller

[H. Nikolaus Schaller]

Hi,

I have started to review and compare with our (old) pwm x1600 driver
which is based on some vendor driver. It appears to somehow support
DMA.

> Am 02.11.2026 um 21:13 schrieb Olivier Louvignes <olivier@xxxxxxxxx>:
> 
> The Ingenic X1600 has a PWM controller of its own, separate from the TCU
> timers that pwm-jz4740 drives on the earlier Ingenic SoCs. This adds its
> binding and a driver built on the waveform callbacks.
> 
> Changes in v3, after Uwe's review of v2 (thanks):
> - Krzysztof's Reviewed-by on the binding, unchanged since v2.
> - Kconfig and Makefile entries in alphabetical order.
> - The programming manual's link in the driver's header.
> - The limitations comment says what 0 and 100% do: the channel is
> switched off, its output held at its idle level.
> - No mod_devicetable.h include; bitops.h for fls64().
> - X1600_PWM_MAX_COUNT from FIELD_MAX(). PRESCALE is four bits wide, but
> the manual reserves its values from 8 up (section 13.4.7), as a
> comment now says.
> - A comment on why x1600_pwm_ns() cannot overflow.
> - The prescaler computed without a loop; it matches v2's loop on 300,000
> random waveforms.
> - A channel that is off reads back as 0 or 100% by its idle level, over
> the period its counts give, and as disabled only with both counts
> zero. Its initial level plays no part then.
> - .write_waveform() no longer waits for BUSY, the clock rate is taken
> exclusively and cached at probe, and the chip is atomic.
> - Fixed: v2 changed the divider or the initial level of a running
> channel by switching it off and on again. A channel switched off
> finishes its period, though, and switched on again before that, it
> keeps its old word, so the new waveform was lost. v3 writes the
> divider, then loads the initial level and the word through the update
> register, running or not.
> 
> Uwe's questions about the registers, measured on the R1's channel 0
> through /dev/mem, with its pin sampled through the GPIO port about every
> microsecond and the clock divided by 128 (a 10 ms period):
> - IDL is the level while off, and nothing else: set on a running
> channel, it changes nothing; on a channel that is off, the output
> follows it at once. So it does not depend on the offset, and every
> write now sets it.
> - INL is loaded with the word: written alone it changes nothing, and
> with an update the new level starts with the next period.
> - A word written without an update changes nothing. After an update,
> BUSY stays set until the period ends (8.7 ms in one run), and a second
> word written while BUSY is set replaces the first, which never shows.
> - The divider takes effect as soon as it is written, in the middle of a
> period.
> - ENC clears EN at once, but the output finishes its period before it
> goes to the idle level.
> - An update on a channel that is off keeps BUSY set; ENS then loads the
> word, and BUSY clears at the end of the first period.
> 
> v3 itself was tested on the R1 with PWM_DEBUG, sampling the pin the same
> way. Through pwm-backlight, 30, 60 and 99% measured 30.8, 59.8 and 99.0%
> high, and 0 and 100% switched the channel off with its output held low
> and high. Through sysfs on channel 0, with the backlight unbound, all on
> a running channel: a new duty cycle; a period from 100 us to 1 ms, so a
> new divider, after which it ran at 1 ms and 25%; the polarity inversed,
> so a new initial level, after which it was high 75% of the time; then
> 0 and 100% inversed, and off. PWM_DEBUG reported nothing.
> 
> The X1600 has no device tree upstream yet; its support is being posted
> one subsystem at a time.
> 
> v2: https://lore.kernel.org/r/20261025191624.9219-1-olivier@xxxxxxxxx
> v1: https://lore.kernel.org/r/20261025084643.93167-1-olivier@xxxxxxxxx
> 
> Olivier Louvignes (2):
> dt-bindings: pwm: Add the Ingenic X1600 PWM controller
> pwm: Add a driver for the Ingenic X1600 PWM controller
> 
> .../bindings/pwm/ingenic,x1600-pwm.yaml       |  47 +++
> MAINTAINERS                                   |   6 +
> drivers/pwm/Kconfig                           |  11 +
> drivers/pwm/Makefile                          |   1 +
> drivers/pwm/pwm-ingenic-x1600.c               | 289 ++++++++++++++++++

Here I have a first question about the naming scheme. It does not seem to be common
to have pwm-[vendor]-chip.c

The only exceptions I can see are pwm-bcm-*-c, pwm-intel-lgm.c, pwm-renesas-tpu.c.

Since there is already pwm-jz4740.c without vendor-infix, I'd suggest to simply use
pwm-x1600.c unless the pwm maintainers disagree.

> 5 files changed, 354 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/ingenic,x1600-pwm.yaml
> create mode 100644 drivers/pwm/pwm-ingenic-x1600.c

More to come after trying to test...

BR and thanks,
Nikolaus