[PATCH v2 00/13] leds: st1202: Naming cleanups and pre-existing fixes

[Manuel Fombuena]

This series collects the issues identified during review of the two
previous ST1202 submissions, the pattern engine fix series and the
hardware-accelerated blink support:

https://lore.kernel.org/all/GV1PR08MB8497C0B898789BB73ACE6EE3C5F52@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
https://lore.kernel.org/all/GV1PR08MB84974C7ACD911E66864C5008C5DA2@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/

Both left behind pre-existing problems that were out of scope at the
time and deferred to a follow-up. They are grouped here because they
share that origin, rather than being sent as a string of two and three
patch series.

Patches 1 to 4 are naming and readability only, with no functional
change; two of them address comments from the blink support review.
Patches 5 to 12 are fixes and carry Fixes tags, and patch 13 moves the
channel current ceiling into a single helper.

Patch 5 is worth a note. The eight pattern duration registers are
chip-wide while the PWM registers are per-channel, so a channel can
inherit a sequencer timeline another channel programmed and then sit at
full brightness for the slots it never set up. It clears the channel's
own PWM for those slots rather than the shared durations, which would
have truncated a longer pattern still running elsewhere on the device.

Patches 6 and 7 belong together. Pattern brightness values were written
into the 12-bit PWM registers unscaled, so a step asking for full
brightness produced about 6% duty cycle; and the analog current register
was never programmed at all, so a channel whose current had been left at
zero stayed dark however bright the pattern. Together they make a
pattern step of N produce the same output as writing N to brightness,
which is what the documented hw_pattern range implies. Patch 7 takes the
current up in pattern_set() and releases it in pattern_clear(), so a
rejected hw_pattern write cannot leave a channel latched on, and a board
declaring a lower ceiling through max-brightness keeps it. Releasing it
also disables the channel, because a current of zero still leaves a
faint glow on this part. Probe clears every channel, so the LEDs now
reach user space switched off, rather than lit at the power-on current.

Patch 9 is the one worth the most attention. st1202_brightness_set()
takes a mutex and performs I2C transfers while installed as the
non-blocking brightness_set callback, so it can sleep in atomic context.
brightness_set_blocking is installed as well, but the core only falls
back to it when brightness_set is absent, and that callback ignored the
requested brightness in any case. Neither can be fixed without the other.

Patches 10 and 11 do for st1202_blink_set() what patches 5 and 7 do for
the pattern path.

blink_set() programs only the two slots its on and off cycle needs, and
leaves the other six at full scale. Those six are normally invisible
because their duration registers are zero. Durations are shared across
the chip, though, so another channel can set them again, and the
blinking channel then lights during steps it never programmed. This was
reproduced on hardware by blinking one channel and giving a second one a
four step pattern.

blink_set() also drives the current register to full scale, ignoring a
lower ceiling from the max-brightness property. Patch 11 makes it use
the same value the brightness and pattern paths already use.

Patches 12 and 13 deal with a max-brightness above 255, which the device
tree allows but the 8-bit current register cannot hold. Patch 12 caps
the value st1202_led_set() writes, which previously wrapped, and is kept
to that alone so it can be backported. Patch 13 then gives the ceiling a
single helper shared by the three paths that program the current.

Three points raised in earlier reviews are deliberately left alone:

  - brightness_set() programming every PWM slot to full scale, and so
    overriding a running pattern, is intended. It is what makes the
    brightness visible while the global sequencer runs, per
    commit 7cbe470366bd ("leds: st1202: Fix brightness having no effect
    while pattern mode is active"). Stopping the sequencer instead would
    halt the patterns of every other channel.

  - st1202_blink_set() turning the other active channels off while it
    runs is intended, as described in commit 35cbd34a6a05 ("leds: st1202:
    Add hardware-accelerated blink support"). The sequencer cannot keep
    their own timing once a blink takes it over, so they are kept dark
    rather than outputting unintended values.

  - Only the pattern timing is shared; the per-channel PWM envelopes are
    independent, and running patterns on several channels at once is
    what the hardware is designed for. A channel whose pattern is
    shorter than the active timeline therefore keeps the longer period.
    Correcting that needs the driver to track sequencer usage across
    channels, which is outside the scope of this series.

Tested on LED1202 hardware over I2C, on a Linksys MX4200v2 router
running OpenWrt: brightness values reaching the current register, the
atomic-context path via the heartbeat trigger, hw_pattern on two
channels of one device with different pattern lengths, a pattern
programmed on a channel whose current had been left at zero, timer
trigger delay quantisation, a rejected or empty hw_pattern write leaving
the channel dark, the LEDs staying off while the driver loads, and
driver unbind/rebind cycles for the firmware node reference counting.
dmesg clean throughout.

Patch 6 was additionally checked at the register level with i2cget. Left
unscaled, a pattern step asking for full brightness programs 0x0FF of a
12-bit register, 6.25% duty cycle; with the patch it programs 0x0FFF.
The datasheet (DS12875 rev 2, Table 7) confirms the PWM register pair is
a plain little endian 12-bit value, low byte followed by the high
nibble. Its helper scales against max_brightness, clamps both ends of a
signed input, and guards the division, although the LED core never
registers a zero max_brightness.

--- Changes in v2 ---

  In response to automated review feedback (Sashiko) on v1:

  Patch 5: the commit message attributed the full-scale PWM left in the
  unused slots to probe only. The brightness path and every activation
  of the pattern trigger leave it there too. Commit message corrected;
  no code change.

  Patch 6: the documentation now says that a max-brightness above 255
  saturates, since the current register is 8 bits wide.

  Patch 7: v1 wrote the channel current after starting the sequencer, so
  a new pattern began at the previous current until that write landed.
  The current is now programmed before the channel is enabled and the
  sequencer started. pattern_clear() still releases it first, and now
  also disables the channel: on hardware, a current of zero still left
  a faint glow after a rejected hw_pattern write and at boot. Probe no
  longer enables the channels; st1202_setup() already disables them
  all, and each is enabled when it is lit.

  Patch 8: the missing reference dates from the original driver, which
  already registered the LED class devices after the iterator had
  dropped its reference to the node. The Fixes tag now names the
  driver's original commit instead of commit c72e455b89f2 ("leds:
  leds-st1202: Fix NULL pointer access on race condition"), and the
  commit message covers both.

  Patch 9: st1202_channel_set() is removed, as it no longer has any
  callers.

  Patch 11: no longer moves the current write in st1202_blink_set(), so
  it stays before the sequencer is started, for the same reason as
  patch 7.

  Patch 12 (new): st1202_led_set() caps the channel current at the width
  of the register.

  Patch 13 (new): the current ceiling moves into a single helper, so the
  three paths that program the current share one limit.

  Other pre-existing issues identified by the automated review stem from
  the sequencer and its duration registers being shared by all channels,
  and are outside the scope of this series, as noted above.

v1: https://lore.kernel.org/all/GV1PR08MB8497422B7A44424C4968AEC1C5BD2@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/

Manuel Fombuena (13):
  leds: st1202: Correct the name of the prescaler conversion helper
  leds: st1202: Spell out the phase-shift configuration bit
  leds: st1202: Name pattern index variables consistently
  leds: st1202: Use u8 consistently for 8-bit values
  leds: st1202: Clear unused pattern slots in pattern_set()
  leds: st1202: Scale pattern brightness to the 12-bit PWM range
  leds: st1202: Program the channel current for hardware patterns
  leds: st1202: Take a reference on the LED firmware node
  leds: st1202: Do not set brightness from atomic context
  leds: st1202: Clear unused pattern slots in blink_set()
  leds: st1202: Honour max_brightness when blinking
  leds: st1202: Cap the channel current in st1202_led_set()
  leds: st1202: Give the channel current ceiling a single helper

 Documentation/leds/leds-st1202.rst |   5 +
 drivers/leds/leds-st1202.c         | 163 +++++++++++++++++++----------
 2 files changed, 111 insertions(+), 57 deletions(-)

-- 
2.55.0