[PATCH 1/2] leds/leds-is31fl32xx: add support for is31fl3236a

[PATCH 1/2] leds/leds-is31fl32xx: add support for is31fl3236a

[Pawel Zalewski]

This commit is adding an additional and optional control
register for setting the output PWM frequency to 22kHz.
The default is 3kHz and this option puts the operational
frequency outside of the audible range. The control over this
parameter was added via the device tree parser function,
as really whether to use this functionality or not would
depend on the board.

Signed-off-by: Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
---
 drivers/leds/leds-is31fl32xx.c | 35 ++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
index 8793330dd414..d23260f3f6ce 100644
--- a/drivers/leds/leds-is31fl32xx.c
+++ b/drivers/leds/leds-is31fl32xx.c
@@ -32,6 +32,8 @@
 #define IS31FL3216_CONFIG_SSD_ENABLE  BIT(7)
 #define IS31FL3216_CONFIG_SSD_DISABLE 0
 
+#define IS31FL32XX_PWM_FREQUENCY_22kHz  0x01
+
 struct is31fl32xx_priv;
 struct is31fl32xx_led_data {
 	struct led_classdev cdev;
@@ -57,6 +59,7 @@ struct is31fl32xx_priv {
  * @pwm_registers_reversed: : true if PWM registers count down instead of up
  * @led_control_register_base : address of first LED control register (optional)
  * @enable_bits_per_led_control_register: number of LEDs enable bits in each
+ * @output_frequency_setting_register: address of outupt frequency register (optional)
  * @reset_func          : pointer to reset function
  * @sw_shutdown_func    : pointer to software shutdown function
  *
@@ -80,6 +83,7 @@ struct is31fl32xx_chipdef {
 	bool	pwm_registers_reversed;
 	u8	led_control_register_base;
 	u8	enable_bits_per_led_control_register;
+	u8	output_frequency_setting_register;
 	int (*reset_func)(struct is31fl32xx_priv *priv);
 	int (*sw_shutdown_func)(struct is31fl32xx_priv *priv, bool enable);
 };
@@ -93,6 +97,19 @@ static const struct is31fl32xx_chipdef is31fl3236_cdef = {
 	.pwm_register_base			= 0x01,
 	.led_control_register_base		= 0x26,
 	.enable_bits_per_led_control_register	= 1,
+	.output_frequency_setting_register = IS31FL32XX_REG_NONE,
+};
+
+static const struct is31fl32xx_chipdef is31fl3236a_cdef = {
+	.channels				= 36,
+	.shutdown_reg				= 0x00,
+	.pwm_update_reg				= 0x25,
+	.global_control_reg			= 0x4a,
+	.reset_reg				= 0x4f,
+	.pwm_register_base			= 0x01,
+	.led_control_register_base		= 0x26,
+	.enable_bits_per_led_control_register	= 1,
+	.output_frequency_setting_register = 0x4b,
 };
 
 static const struct is31fl32xx_chipdef is31fl3235_cdef = {
@@ -104,6 +121,7 @@ static const struct is31fl32xx_chipdef is31fl3235_cdef = {
 	.pwm_register_base			= 0x05,
 	.led_control_register_base		= 0x2a,
 	.enable_bits_per_led_control_register	= 1,
+	.output_frequency_setting_register = IS31FL32XX_REG_NONE,
 };
 
 static const struct is31fl32xx_chipdef is31fl3218_cdef = {
@@ -115,6 +133,7 @@ static const struct is31fl32xx_chipdef is31fl3218_cdef = {
 	.pwm_register_base			= 0x01,
 	.led_control_register_base		= 0x13,
 	.enable_bits_per_led_control_register	= 6,
+	.output_frequency_setting_register = IS31FL32XX_REG_NONE,
 };
 
 static int is31fl3216_reset(struct is31fl32xx_priv *priv);
@@ -130,6 +149,7 @@ static const struct is31fl32xx_chipdef is31fl3216_cdef = {
 	.pwm_registers_reversed			= true,
 	.led_control_register_base		= 0x01,
 	.enable_bits_per_led_control_register	= 8,
+	.output_frequency_setting_register = IS31FL32XX_REG_NONE,
 	.reset_func				= is31fl3216_reset,
 	.sw_shutdown_func			= is31fl3216_software_shutdown,
 };
@@ -363,8 +383,21 @@ static struct is31fl32xx_led_data *is31fl32xx_find_led_data(
 static int is31fl32xx_parse_dt(struct device *dev,
 			       struct is31fl32xx_priv *priv)
 {
+	const struct is31fl32xx_chipdef *cdef = priv->cdef;
 	int ret = 0;
 
+	if (of_property_read_bool(dev_of_node(dev), "is31fl32xx,22kHz-pwm")
+	&& (cdef->output_frequency_setting_register != IS31FL32XX_REG_NONE)) {
+
+		ret = is31fl32xx_write(priv, cdef->output_frequency_setting_register,
+							IS31FL32XX_PWM_FREQUENCY_22kHz);
+
+		if (ret) {
+			dev_err(dev, "Failed to write output pwm frequency register\n");
+			return ret;
+		}
+	}
+
 	for_each_available_child_of_node_scoped(dev_of_node(dev), child) {
 		struct led_init_data init_data = {};
 		struct is31fl32xx_led_data *led_data =
@@ -405,6 +438,7 @@ static int is31fl32xx_parse_dt(struct device *dev,
 
 static const struct of_device_id of_is31fl32xx_match[] = {
 	{ .compatible = "issi,is31fl3236", .data = &is31fl3236_cdef, },
+	{ .compatible = "issi,is31fl3236a", .data = &is31fl3236a_cdef, },
 	{ .compatible = "issi,is31fl3235", .data = &is31fl3235_cdef, },
 	{ .compatible = "issi,is31fl3218", .data = &is31fl3218_cdef, },
 	{ .compatible = "si-en,sn3218",    .data = &is31fl3218_cdef, },
@@ -466,6 +500,7 @@ static void is31fl32xx_remove(struct i2c_client *client)
  */
 static const struct i2c_device_id is31fl32xx_id[] = {
 	{ "is31fl3236" },
+	{ "is31fl3236a" },
 	{ "is31fl3235" },
 	{ "is31fl3218" },
 	{ "sn3218" },
-- 
2.48.1

[PATCH 2/2] dt-bindigs: leds: is31fl32xx: add optional properties section

[Pawel Zalewski]

Add optional support for is31fl3236a PWM frequency switch.

Signed-off-by: Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
---
 Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
index 926c2117942c..aa38a0638bad 100644
--- a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
+++ b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
@@ -7,6 +7,7 @@ Each LED is represented as a sub-node of the device.
 Required properties:
 - compatible: one of
 	issi,is31fl3236
+	issi,is31fl3236a
 	issi,is31fl3235
 	issi,is31fl3218
 	issi,is31fl3216
@@ -16,6 +17,11 @@ Required properties:
 - address-cells : must be 1
 - size-cells : must be 0
 
+Optional properties:
+- is31fl32xx,22kHz-pwm : When present, the chip's PWM will operate at
+	~22kHz as opposed to ~3kHz to move the operating frequency out of the
+	audible range.
+
 LED sub-node properties:
 - reg : LED channel number (1..N)
 - label :  (optional)
-- 
2.48.1

Re: [PATCH 2/2] dt-bindigs: leds: is31fl32xx: add optional properties section

[Krzysztof Kozlowski]

On 19/06/2025 15:19, Pawel Zalewski wrote:
> Add optional support for is31fl3236a PWM frequency switch.
> 
> Signed-off-by: Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
> ---
>  Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt | 6 ++++++

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>


>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
> index 926c2117942c..aa38a0638bad 100644
> --- a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
> +++ b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
> @@ -7,6 +7,7 @@ Each LED is represented as a sub-node of the device.
>  Required properties:
>  - compatible: one of
>  	issi,is31fl3236
> +	issi,is31fl3236a
>  	issi,is31fl3235
>  	issi,is31fl3218
>  	issi,is31fl3216
> @@ -16,6 +17,11 @@ Required properties:
>  - address-cells : must be 1
>  - size-cells : must be 0
>  
> +Optional properties:
> +- is31fl32xx,22kHz-pwm : When present, the chip's PWM will operate at

No new properties for TXT. Convert first to DT schema.

Also, there is no such thing as is31fl32xx company.

> +	~22kHz as opposed to ~3kHz to move the operating frequency out of the
> +	audible range.
> +
>  LED sub-node properties:
>  - reg : LED channel number (1..N)
>  - label :  (optional)


Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindigs: leds: is31fl32xx: add optional properties section

[Krzysztof Kozlowski]

On 19/06/2025 15:19, Pawel Zalewski wrote:
> Add optional support for is31fl3236a PWM frequency switch.
> 
> Signed-off-by: Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
> ---
>  Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt | 6 ++++++
>  1 file changed, 6 insertions(+)

Eh, plus typo in subject prefix, it is dt-bindings. Please organize the
patch documenting compatible (DT bindings) before their user.
See also:
https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/submitting-patches.rst#L46

Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindigs: leds: is31fl32xx: add optional properties section

[Pawel Zalewski]

> Please kindly resend and include all necessary To/Cc entries.

Right on, thanks for pointing it out, forgot about that indeed, will do in v2.

> No new properties for TXT. Convert first to DT schema.

Noted.

> Also, there is no such thing as is31fl32xx company.

Noted.

> Eh, plus typo in subject prefix, it is dt-bindings. Please organize the
> patch documenting compatible (DT bindings) before their user.
> See also:
> https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/submitting-patches.rst#L46

Eh, you are right.

Kind regards,
Pawel

Re: [PATCH 1/2] leds/leds-is31fl32xx: add support for is31fl3236a

[Lee Jones]

On Thu, 19 Jun 2025, Pawel Zalewski wrote:

> This commit is adding an additional and optional control
> register for setting the output PWM frequency to 22kHz.
> The default is 3kHz and this option puts the operational
> frequency outside of the audible range. The control over this
> parameter was added via the device tree parser function,
> as really whether to use this functionality or not would
> depend on the board.
> 
> Signed-off-by: Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
> ---
>  drivers/leds/leds-is31fl32xx.c | 35 ++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index 8793330dd414..d23260f3f6ce 100644
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c
> @@ -32,6 +32,8 @@
>  #define IS31FL3216_CONFIG_SSD_ENABLE  BIT(7)
>  #define IS31FL3216_CONFIG_SSD_DISABLE 0
>  
> +#define IS31FL32XX_PWM_FREQUENCY_22kHz  0x01
> +
>  struct is31fl32xx_priv;
>  struct is31fl32xx_led_data {
>  	struct led_classdev cdev;
> @@ -57,6 +59,7 @@ struct is31fl32xx_priv {
>   * @pwm_registers_reversed: : true if PWM registers count down instead of up
>   * @led_control_register_base : address of first LED control register (optional)
>   * @enable_bits_per_led_control_register: number of LEDs enable bits in each
> + * @output_frequency_setting_register: address of outupt frequency register (optional)
>   * @reset_func          : pointer to reset function
>   * @sw_shutdown_func    : pointer to software shutdown function
>   *
> @@ -80,6 +83,7 @@ struct is31fl32xx_chipdef {
>  	bool	pwm_registers_reversed;
>  	u8	led_control_register_base;
>  	u8	enable_bits_per_led_control_register;
> +	u8	output_frequency_setting_register;
>  	int (*reset_func)(struct is31fl32xx_priv *priv);
>  	int (*sw_shutdown_func)(struct is31fl32xx_priv *priv, bool enable);
>  };
> @@ -93,6 +97,19 @@ static const struct is31fl32xx_chipdef is31fl3236_cdef = {
>  	.pwm_register_base			= 0x01,
>  	.led_control_register_base		= 0x26,
>  	.enable_bits_per_led_control_register	= 1,
> +	.output_frequency_setting_register = IS31FL32XX_REG_NONE,

Line up with the others please </ocd_voice>

> +};
> +
> +static const struct is31fl32xx_chipdef is31fl3236a_cdef = {
> +	.channels				= 36,
> +	.shutdown_reg				= 0x00,
> +	.pwm_update_reg				= 0x25,
> +	.global_control_reg			= 0x4a,
> +	.reset_reg				= 0x4f,
> +	.pwm_register_base			= 0x01,
> +	.led_control_register_base		= 0x26,
> +	.enable_bits_per_led_control_register	= 1,
> +	.output_frequency_setting_register = 0x4b,

As above.

>  };
>  
>  static const struct is31fl32xx_chipdef is31fl3235_cdef = {
> @@ -104,6 +121,7 @@ static const struct is31fl32xx_chipdef is31fl3235_cdef = {
>  	.pwm_register_base			= 0x05,
>  	.led_control_register_base		= 0x2a,
>  	.enable_bits_per_led_control_register	= 1,
> +	.output_frequency_setting_register = IS31FL32XX_REG_NONE,

Here too - and everywhere.

>  };
>  
>  static const struct is31fl32xx_chipdef is31fl3218_cdef = {
> @@ -115,6 +133,7 @@ static const struct is31fl32xx_chipdef is31fl3218_cdef = {
>  	.pwm_register_base			= 0x01,
>  	.led_control_register_base		= 0x13,
>  	.enable_bits_per_led_control_register	= 6,
> +	.output_frequency_setting_register = IS31FL32XX_REG_NONE,
>  };
>  
>  static int is31fl3216_reset(struct is31fl32xx_priv *priv);
> @@ -130,6 +149,7 @@ static const struct is31fl32xx_chipdef is31fl3216_cdef = {
>  	.pwm_registers_reversed			= true,
>  	.led_control_register_base		= 0x01,
>  	.enable_bits_per_led_control_register	= 8,
> +	.output_frequency_setting_register = IS31FL32XX_REG_NONE,

How do you sleep at night!   =;-)

>  	.reset_func				= is31fl3216_reset,
>  	.sw_shutdown_func			= is31fl3216_software_shutdown,
>  };
> @@ -363,8 +383,21 @@ static struct is31fl32xx_led_data *is31fl32xx_find_led_data(
>  static int is31fl32xx_parse_dt(struct device *dev,
>  			       struct is31fl32xx_priv *priv)
>  {
> +	const struct is31fl32xx_chipdef *cdef = priv->cdef;
>  	int ret = 0;
>  
> +	if (of_property_read_bool(dev_of_node(dev), "is31fl32xx,22kHz-pwm")
> +	&& (cdef->output_frequency_setting_register != IS31FL32XX_REG_NONE)) {

Please the '&&' and the end of the previous line, then indent this one.

> +
> +		ret = is31fl32xx_write(priv, cdef->output_frequency_setting_register,
> +							IS31FL32XX_PWM_FREQUENCY_22kHz);

Line-up with the '('.

> +
> +		if (ret) {
> +			dev_err(dev, "Failed to write output pwm frequency register\n");

"PWM"

> +			return ret;
> +		}
> +	}
> +
>  	for_each_available_child_of_node_scoped(dev_of_node(dev), child) {
>  		struct led_init_data init_data = {};
>  		struct is31fl32xx_led_data *led_data =
> @@ -405,6 +438,7 @@ static int is31fl32xx_parse_dt(struct device *dev,
>  
>  static const struct of_device_id of_is31fl32xx_match[] = {
>  	{ .compatible = "issi,is31fl3236", .data = &is31fl3236_cdef, },
> +	{ .compatible = "issi,is31fl3236a", .data = &is31fl3236a_cdef, },
>  	{ .compatible = "issi,is31fl3235", .data = &is31fl3235_cdef, },
>  	{ .compatible = "issi,is31fl3218", .data = &is31fl3218_cdef, },
>  	{ .compatible = "si-en,sn3218",    .data = &is31fl3218_cdef, },
> @@ -466,6 +500,7 @@ static void is31fl32xx_remove(struct i2c_client *client)
>   */
>  static const struct i2c_device_id is31fl32xx_id[] = {
>  	{ "is31fl3236" },
> +	{ "is31fl3236a" },
>  	{ "is31fl3235" },
>  	{ "is31fl3218" },
>  	{ "sn3218" },
> -- 
> 2.48.1
> 
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH 1/2] leds/leds-is31fl32xx: add support for is31fl3236a

[Pawel Zalewski]

> Line up with the others please </ocd_voice>

My tabs were not switched to 8,  checkpatch does not pick up on it,
realised after sending the email :)

>  How do you sleep at night!   =;-)

Ah, not much sleep recently !

>  Please the '&&' and the end of the previous line, then indent this one.
>  Line-up with the '('.
> "PWM"

Thanks for the review, will do all of the changes in V2.