mbox series

[v2,0/6] leds: Fix/Add is31fl319{0,1,3} support

Message ID 20220701134415.4017794-1-vincent.knecht@mailoo.org
Headers show
Series leds: Fix/Add is31fl319{0,1,3} support | expand

Message

Vincent Knecht July 1, 2022, 1:44 p.m. UTC
Changes since v2:
- keep original bindings license and maintainer/owner (Rob)
- squash bindings patches 2 & 4 (Krzysztof)

Changes since v1:
- no change, resending after configuring git to accomodate
  for smtp provider limit of 5 emails per batch
- just change cover-letter to mention si-en chip for idol347


The is31fl3190, is31fl3191 and is31fl3193 chips (1 or 3 PWM channels)
cannot be handled the same as is31fl3196 and is31fl3199,
if only because the register map is different.
Also:
- the software shutdown bit is reversed
- and additional field needs to be set to enable all channels
- the led-max-microamp current values and setting are not the same

Datasheets:
https://lumissil.com/assets/pdf/core/IS31FL3190_DS.pdf
https://lumissil.com/assets/pdf/core/IS31FL3191_DS.pdf
https://lumissil.com/assets/pdf/core/IS31FL3193_DS.pdf
https://lumissil.com/assets/pdf/core/IS31FL3196_DS.pdf
https://lumissil.com/assets/pdf/core/IS31FL3199_DS.pdf

This series:

- converts dt-bindings to dtschema, adding all si-en compatibles
  for convenience and consistency, and adding constraints on
  supported values for eg. reg address and led-max-microamp

- changes vars, structs and defines to not use 319X suffix
  but 3190 for 319{0,1,3} and 3196 for 319{6,9}

- adds fields in chipdef struct for chip-specific values

- only in the last patch, adds is31fl319{0,1,3} specific values
  so those chips can work.

Tested on msm8916-alcatel-idol347, which probably has an
si-en,sn3190 or si-en,sn3191 (only one white led indicator).

Vincent Knecht (6):
  dt-bindings: leds: Convert is31fl319x to dtschema
  dt-bindings: leds: is31fl319x: Document variants specificities
  leds: is31fl319x: Add missing si-en compatibles
  leds: is31fl319x: Use non-wildcard names for vars, structs and defines
  leds: is31fl319x: Move chipset-specific values in chipdef struct
  leds: is31fl319x: Add support for is31fl319{0,1,3} chips

 .../bindings/leds/issi,is31fl319x.yaml        | 193 +++++++++
 .../bindings/leds/leds-is31fl319x.txt         |  61 ---
 drivers/leds/leds-is31fl319x.c                | 406 +++++++++++++-----
 3 files changed, 488 insertions(+), 172 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/issi,is31fl319x.yaml
 delete mode 100644 Documentation/devicetree/bindings/leds/leds-is31fl319x.txt

Comments

Andy Shevchenko July 1, 2022, 8:47 p.m. UTC | #1
On Fri, Jul 1, 2022 at 3:45 PM Vincent Knecht <vincent.knecht@mailoo.org> wrote:
>
> In order to add real support for is31fl3190, is31fl3191 and is31fl3193,
> rename variant-dependent elements to not use 319X where needed.
>
> 3190 suffix is used for is31fl3190, is31fl3191 and is31fl3193 circuits.
> 3196 suffix is used for is31fl3196 and is31fl3199.
>
> Those two groups have different register maps, current settings and even
> a different interpretation of the software shutdown bit:
> https://lumissil.com/assets/pdf/core/IS31FL3190_DS.pdf
> https://lumissil.com/assets/pdf/core/IS31FL3191_DS.pdf
> https://lumissil.com/assets/pdf/core/IS31FL3193_DS.pdf
> https://lumissil.com/assets/pdf/core/IS31FL3196_DS.pdf
> https://lumissil.com/assets/pdf/core/IS31FL3199_DS.pdf
>
> Rename variables, stuctures and defines in preparation of the splitting.

structures

> No functional nor behaviour change.

...

> +#define IS31FL3196_CONFIG2_CS_MASK     0x7

GENMASK() ?

...

> +#define IS31FL3196_CURRENT_MIN         ((u32)5000)
> +#define IS31FL3196_CURRENT_MAX         ((u32)40000)
> +#define IS31FL3196_CURRENT_STEP                ((u32)5000)
> +#define IS31FL3196_CURRENT_DEFAULT     ((u32)20000)

> +#define IS31FL3196_AUDIO_GAIN_DB_MAX   ((u32)21)
> +#define IS31FL3196_AUDIO_GAIN_DB_STEP  ((u32)3)

Why do you need all these castings? Wouldn't u/U suffice if you really
want to have them unsigned?
Andy Shevchenko July 1, 2022, 8:49 p.m. UTC | #2
On Fri, Jul 1, 2022 at 10:47 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Jul 1, 2022 at 3:45 PM Vincent Knecht <vincent.knecht@mailoo.org> wrote:

...

> > +#define IS31FL3196_CURRENT_MIN         ((u32)5000)
> > +#define IS31FL3196_CURRENT_MAX         ((u32)40000)
> > +#define IS31FL3196_CURRENT_STEP                ((u32)5000)
> > +#define IS31FL3196_CURRENT_DEFAULT     ((u32)20000)

Also why no units as below, for example, has?

> > +#define IS31FL3196_AUDIO_GAIN_DB_MAX   ((u32)21)
> > +#define IS31FL3196_AUDIO_GAIN_DB_STEP  ((u32)3)