Re: [PATCH 02/10] pwm: Add SI-EN SN3112 PWM support

2024-04-25 Thread Neil Armstrong

On 25/04/2024 02:57, Junhao Xie wrote:

On 2024/4/25 03:37, Konrad Dybcio wrote:

On 4/24/24 17:29, Xilin Wu via B4 Relay wrote:

From: Junhao Xie 

Add a new driver for the SI-EN SN3112 12-channel 8-bit PWM LED controller.

Signed-off-by: Junhao Xie 
---

[...]

+    return sn3112_write_reg(priv, SN3112_REG_PWM_EN + reg,
+    priv->pwm_en_reg[reg]);


This looks like a weird reimplementation of regmap_update_bits



We cannot use regmap_update_bits because this chip does not support read 
command.
It will discard all read command.


You could use regmap cache with all registers marked as cacheable, but not sure 
it's worth
doing this.

Neil




+}
+

[...]


devm_pwmchip_add?

Konrad


Thank you for your reply, I will fix them.





Re: [PATCH 02/10] pwm: Add SI-EN SN3112 PWM support

2024-04-25 Thread Junhao Xie
On 2024/4/25 14:02, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, Apr 24, 2024 at 11:29:07PM +0800, Xilin Wu via B4 Relay wrote:
>> From: Junhao Xie 
>>
>> Add a new driver for the SI-EN SN3112 12-channel 8-bit PWM LED controller.
>>
[...]
>> +MODULE_LICENSE("GPL");
> 
> Best regards
> Uwe
> 

Thank you for your reply! I will fix them and resend this commit.

This is the link of datasheet for SI-EN SN3112, but it is written in Chinese:
https://datasheetspdf.com/pdf-down/S/N/3/SN3112-12-SI-EN.pdf


Re: [PATCH 02/10] pwm: Add SI-EN SN3112 PWM support

2024-04-25 Thread Uwe Kleine-König
Hello,

On Wed, Apr 24, 2024 at 09:37:25PM +0200, Konrad Dybcio wrote:
> On 4/24/24 17:29, Xilin Wu via B4 Relay wrote:
> > +
> > +   /* use random value to apply changes */
> > +   ret = sn3112_write_reg(priv, SN3112_REG_APPLY, 0x66);
> 
> "a random value"? sounds suspicious..

I smiled about that one, too, remembering https://xkcd.com/221/

> [...]
> > +#if IS_ENABLED(CONFIG_GPIOLIB)
> > +   /* enable hardware shutdown pin */
> > +   if (priv->sdb)
> > +   gpiod_set_value(priv->sdb, 1);
> > +#endif
> > +
> > +   /* power-off sn5112 power vdd */
> > +   regulator_disable(priv->vdd);
> > +
> > +   pwmchip_remove(chip);
> 
> devm_pwmchip_add?

Note using devm_xyz only works if all requests before are also using
devm. (There are a few exceptions, but these need proper thinking and
extensive commenting.)

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature


Re: [PATCH 02/10] pwm: Add SI-EN SN3112 PWM support

2024-04-25 Thread Uwe Kleine-König
Hello,

On Wed, Apr 24, 2024 at 04:55:26PM +0100, Bryan O'Donoghue wrote:
> On 24/04/2024 16:29, Xilin Wu via B4 Relay wrote:
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> 
> Includes should be alphabetised

Also check if you need them all. (E.g. I wonder about delay.h)

> > +   dev_dbg(priv->pdev, "request regmap_write 0x%x 0x%x\n", reg, val);
> > +   err = regmap_write(priv->regmap, reg, val);
> > +   if (err)
> > +   dev_warn_ratelimited(
> > +   priv->pdev,
> > +   "regmap_write to register 0x%x failed: %pe\n", reg,
> > +   ERR_PTR(err));
> 
> Multi-line should be encapsulated in {}
> 
> if (err) {
>   stuff
>   goes here
> }

In my eyes a single state doesn't need {} even when spanning multiple
lines.

> > +   return err;
> > +}

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature


Re: [PATCH 02/10] pwm: Add SI-EN SN3112 PWM support

2024-04-25 Thread Uwe Kleine-König
Hello,

On Wed, Apr 24, 2024 at 11:29:07PM +0800, Xilin Wu via B4 Relay wrote:
> From: Junhao Xie 
> 
> Add a new driver for the SI-EN SN3112 12-channel 8-bit PWM LED controller.
> 
> Signed-off-by: Junhao Xie 

Missing S-o-b for patch sender.

> ---
>  drivers/pwm/Kconfig  |  10 ++
>  drivers/pwm/Makefile |   1 +
>  drivers/pwm/pwm-sn3112.c | 336 
> +++
>  3 files changed, 347 insertions(+)
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 1dd7921194f5..e21c37c7991e 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -553,6 +553,16 @@ config PWM_SL28CPLD
> To compile this driver as a module, choose M here: the module
> will be called pwm-sl28cpld.
>  
> +config PWM_SN3112
> + tristate "SI-EN SN3112 PWM driver"
> + depends on I2C
> + select REGMAP_I2C
> + help
> +   Generic PWM framework driver for SI-EN SN3112 LED controller.
> +
> +   To compile this driver as a module, choose M here: the module
> +   will be called pwm-sn3112.
> +
>  config PWM_SPEAR
>   tristate "STMicroelectronics SPEAr PWM support"
>   depends on PLAT_SPEAR || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 90913519f11a..6aab2d113159 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -50,6 +50,7 @@ obj-$(CONFIG_PWM_RZ_MTU3)   += pwm-rz-mtu3.o
>  obj-$(CONFIG_PWM_SAMSUNG)+= pwm-samsung.o
>  obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
>  obj-$(CONFIG_PWM_SL28CPLD)   += pwm-sl28cpld.o
> +obj-$(CONFIG_PWM_SN3112) += pwm-sn3112.o
>  obj-$(CONFIG_PWM_SPEAR)  += pwm-spear.o
>  obj-$(CONFIG_PWM_SPRD)   += pwm-sprd.o
>  obj-$(CONFIG_PWM_STI)+= pwm-sti.o
> diff --git a/drivers/pwm/pwm-sn3112.c b/drivers/pwm/pwm-sn3112.c
> new file mode 100644
> index ..38ef948602a3
> --- /dev/null
> +++ b/drivers/pwm/pwm-sn3112.c
> @@ -0,0 +1,336 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Driver for SN3112 12-channel 8-bit PWM LED controller
> + *
> + * Copyright (c) 2024 Junhao Xie 
> + *
Please document here some hardware features in the same format as e.g.
pwm-sl28cpld.c such that

sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c

can easily extract it. Interesting facts that I want to have documented
are:

 - How does the HW behave on reconfiguration, i.e. does it complete the
   active period or is it aborted and can it happen that the signal
   gliches (e.g. because it emits for a moment a signal using the old
   period but the new duty cycle).

 - How does the HW behave on disable? Does it complete the active
   period? Does it emit low? Or the inactive level? Or does it freeze?

 - "Doesn't support read-back of configured output" belongs here.

 - Only supports a single fixed period and normal polarity.

> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define SN3112_CHANNELS 12
> +#define SN3112_REG_ENABLE 0x00
> +#define SN3112_REG_PWM_VAL 0x04
> +#define SN3112_REG_PWM_EN 0x13
> +#define SN3112_REG_APPLY 0x16
> +#define SN3112_REG_RESET 0x17
> +
> +struct sn3112 {
> + struct device *pdev;

pdev is a usual name for pointers to struct platform_device or struct
pci_device. For struct device please use "dev".

> + struct regmap *regmap;
> + struct mutex lock;
> + struct regulator *vdd;
> + uint8_t pwm_val[SN3112_CHANNELS];
> + uint8_t pwm_en_reg[3];
> + bool pwm_en[SN3112_CHANNELS];
> +#if IS_ENABLED(CONFIG_GPIOLIB)
> + struct gpio_desc *sdb;
> +#endif
> +};
> +
> +static int sn3112_write_reg(struct sn3112 *priv, unsigned int reg,
> + unsigned int val)
> +{
> + int err;
> +
> + dev_dbg(priv->pdev, "request regmap_write 0x%x 0x%x\n", reg, val);
> + err = regmap_write(priv->regmap, reg, val);
> + if (err)
> + dev_warn_ratelimited(
> + priv->pdev,
> + "regmap_write to register 0x%x failed: %pe\n", reg,
> + ERR_PTR(err));
> +
> + return err;
> +}
> +
> +static int sn3112_set_en_reg(struct sn3112 *priv, unsigned int channel,
> +  bool enabled, bool write)
> +{
> + unsigned int reg, bit;
> +
> + if (channel >= SN3112_CHANNELS)
> + return -EINVAL;
> +
> + /* LED_EN1: BIT5:BIT3 = OUT3:OUT1 */
> + if (channel >= 0 && channel <= 2)
> + reg = 0, bit = channel + 3;
> + /* LED_EN2: BIT5:BIT0 = OUT9:OUT4 */
> + else if (channel >= 3 && channel <= 8)
> + reg = 1, bit = channel - 3;
> + /* LED_EN3: BIT2:BIT0 = OUT12:OUT10 */
> + else if (channel >= 9 && channel <= 11)
> + reg = 2, bit = channel - 9;
Please use ; instead of , and proper { }.

And huh, this is inconsitent. Is it possible to renumber somehow such
that this simplifies to

reg = channel / 3;
 

Re: [PATCH 02/10] pwm: Add SI-EN SN3112 PWM support

2024-04-24 Thread Junhao Xie
On 2024/4/25 03:37, Konrad Dybcio wrote:
> On 4/24/24 17:29, Xilin Wu via B4 Relay wrote:
>> From: Junhao Xie 
>>
>> Add a new driver for the SI-EN SN3112 12-channel 8-bit PWM LED controller.
>>
>> Signed-off-by: Junhao Xie 
>> ---
>[...]
>> +    return sn3112_write_reg(priv, SN3112_REG_PWM_EN + reg,
>> +    priv->pwm_en_reg[reg]);
> 
> This looks like a weird reimplementation of regmap_update_bits
> 

We cannot use regmap_update_bits because this chip does not support read 
command.
It will discard all read command.

>> +}
>> +
[...]
> 
> devm_pwmchip_add?
> 
> Konrad

Thank you for your reply, I will fix them.


Re: [PATCH 02/10] pwm: Add SI-EN SN3112 PWM support

2024-04-24 Thread Konrad Dybcio




On 4/24/24 17:29, Xilin Wu via B4 Relay wrote:

From: Junhao Xie 

Add a new driver for the SI-EN SN3112 12-channel 8-bit PWM LED controller.

Signed-off-by: Junhao Xie 
---


[...]


+static int sn3112_set_en_reg(struct sn3112 *priv, unsigned int channel,
+bool enabled, bool write)
+{
+   unsigned int reg, bit;
+
+   if (channel >= SN3112_CHANNELS)
+   return -EINVAL;
+
+   /* LED_EN1: BIT5:BIT3 = OUT3:OUT1 */
+   if (channel >= 0 && channel <= 2)
+   reg = 0, bit = channel + 3;
+   /* LED_EN2: BIT5:BIT0 = OUT9:OUT4 */
+   else if (channel >= 3 && channel <= 8)
+   reg = 1, bit = channel - 3;
+   /* LED_EN3: BIT2:BIT0 = OUT12:OUT10 */
+   else if (channel >= 9 && channel <= 11)
+   reg = 2, bit = channel - 9;
+   else
+   return -EINVAL;
+
+   dev_dbg(priv->pdev, "channel %u enabled %u\n", channel, enabled);
+   dev_dbg(priv->pdev, "reg %u bit %u\n", reg, bit);
+   if (enabled)
+   set_bit(bit, (ulong *)>pwm_en_reg[reg]);
+   else
+   clear_bit(bit, (ulong *)>pwm_en_reg[reg]);
+   dev_dbg(priv->pdev, "set enable reg %u to %u\n", reg,
+   priv->pwm_en_reg[reg]);
+
+   if (!write)
+   return 0;
+   return sn3112_write_reg(priv, SN3112_REG_PWM_EN + reg,
+   priv->pwm_en_reg[reg]);


This looks like a weird reimplementation of regmap_update_bits



+}
+
+static int sn3112_set_val_reg(struct sn3112 *priv, unsigned int channel,
+ uint8_t val, bool write)
+{
+   if (channel >= SN3112_CHANNELS)
+   return -EINVAL;
+   priv->pwm_val[channel] = val;
+   dev_dbg(priv->pdev, "set value reg %u to %u\n", channel,
+   priv->pwm_val[channel]);
+
+   if (!write)
+   return 0;


There's only a single call, with write == true


+   return sn3112_write_reg(priv, SN3112_REG_PWM_VAL + channel,
+   priv->pwm_val[channel]);
+}
+
+static int sn3112_write_all(struct sn3112 *priv)
+{
+   int i, ret;
+
+   /* regenerate enable register values */
+   for (i = 0; i < SN3112_CHANNELS; i++) {
+   ret = sn3112_set_en_reg(priv, i, priv->pwm_en[i], false);
+   if (ret != 0)
+   return ret;
+   }
+
+   /* use random value to clear all registers */
+   ret = sn3112_write_reg(priv, SN3112_REG_RESET, 0x66);
+   if (ret != 0)


if (ret) is the same as if (ret != 0)

[...]


+
+   /* use random value to apply changes */
+   ret = sn3112_write_reg(priv, SN3112_REG_APPLY, 0x66);


"a random value"? sounds suspicious..


+   if (ret != 0)
+   return ret;
+
+   dev_dbg(priv->pdev, "reinitialized\n");


Please remove such "got here" messages once you're done with testing
the driver locally

[...]


+
+#if IS_ENABLED(CONFIG_GPIOLIB)


I'm not sure this would be ever disabled on any embedded system nowadays.
Especially with I2C.

[...]


+
+   dev_info(>dev,
+"Found SI-EN SN3112 12-channel 8-bit PWM LED controller\n");


This sort of message only makes sense if there's a CHIP_ID register that
you can actually validate. If you bind this driver to a device at the same
expected address, it will say it's there even if it's not.



+   return 0;
+}
+
+static void sn3112_pwm_remove(struct i2c_client *client)
+{
+   struct pwm_chip *chip = i2c_get_clientdata(client);
+   struct sn3112 *priv = pwmchip_get_drvdata(chip);
+
+   dev_dbg(priv->pdev, "remove\n");
+
+   /* set software enable register */
+   sn3112_write_reg(priv, SN3112_REG_ENABLE, 0);
+
+   /* use random value to apply changes */
+   sn3112_write_reg(priv, SN3112_REG_APPLY, 0x66);
+
+#if IS_ENABLED(CONFIG_GPIOLIB)
+   /* enable hardware shutdown pin */
+   if (priv->sdb)
+   gpiod_set_value(priv->sdb, 1);
+#endif
+
+   /* power-off sn5112 power vdd */
+   regulator_disable(priv->vdd);
+
+   pwmchip_remove(chip);


devm_pwmchip_add?

Konrad


Re: [PATCH 02/10] pwm: Add SI-EN SN3112 PWM support

2024-04-24 Thread BigfootACA
On 24/4/2024 23:55, Bryan O'Donoghue wrote:
> On 24/04/2024 16:29, Xilin Wu via B4 Relay wrote:
>> From: Junhao Xie 
>>
>> Add a new driver for the SI-EN SN3112 12-channel 8-bit PWM LED controller.
>>
>> Signed-off-by: Junhao Xie 
>> ---
>>   drivers/pwm/Kconfig  |  10 ++
>>   drivers/pwm/Makefile |   1 +
>>   drivers/pwm/pwm-sn3112.c | 336 
>> +++
>>   3 files changed, 347 insertions(+)
[...]
> 
> CHECK: Prefer kernel type 'u8' over 'uint8_t'
> #145: FILE: drivers/pwm/pwm-sn3112.c:91:
> +  uint8_t val, bool write)
> 

I'll fix the commit and resend v2 later, thanks for your reply!


Re: [PATCH 02/10] pwm: Add SI-EN SN3112 PWM support

2024-04-24 Thread Bryan O'Donoghue

On 24/04/2024 16:29, Xilin Wu via B4 Relay wrote:

From: Junhao Xie 

Add a new driver for the SI-EN SN3112 12-channel 8-bit PWM LED controller.

Signed-off-by: Junhao Xie 
---
  drivers/pwm/Kconfig  |  10 ++
  drivers/pwm/Makefile |   1 +
  drivers/pwm/pwm-sn3112.c | 336 +++
  3 files changed, 347 insertions(+)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 1dd7921194f5..e21c37c7991e 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -553,6 +553,16 @@ config PWM_SL28CPLD
  To compile this driver as a module, choose M here: the module
  will be called pwm-sl28cpld.
  
+config PWM_SN3112

+   tristate "SI-EN SN3112 PWM driver"
+   depends on I2C
+   select REGMAP_I2C
+   help
+ Generic PWM framework driver for SI-EN SN3112 LED controller.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-sn3112.
+
  config PWM_SPEAR
tristate "STMicroelectronics SPEAr PWM support"
depends on PLAT_SPEAR || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 90913519f11a..6aab2d113159 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_PWM_RZ_MTU3) += pwm-rz-mtu3.o
  obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
  obj-$(CONFIG_PWM_SIFIVE)  += pwm-sifive.o
  obj-$(CONFIG_PWM_SL28CPLD)+= pwm-sl28cpld.o
+obj-$(CONFIG_PWM_SN3112)   += pwm-sn3112.o
  obj-$(CONFIG_PWM_SPEAR)   += pwm-spear.o
  obj-$(CONFIG_PWM_SPRD)+= pwm-sprd.o
  obj-$(CONFIG_PWM_STI) += pwm-sti.o
diff --git a/drivers/pwm/pwm-sn3112.c b/drivers/pwm/pwm-sn3112.c
new file mode 100644
index ..38ef948602a3
--- /dev/null
+++ b/drivers/pwm/pwm-sn3112.c
@@ -0,0 +1,336 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for SN3112 12-channel 8-bit PWM LED controller
+ *
+ * Copyright (c) 2024 Junhao Xie 
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 


Includes should be alphabetised


+
+#define SN3112_CHANNELS 12
+#define SN3112_REG_ENABLE 0x00
+#define SN3112_REG_PWM_VAL 0x04
+#define SN3112_REG_PWM_EN 0x13
+#define SN3112_REG_APPLY 0x16
+#define SN3112_REG_RESET 0x17
+
+struct sn3112 {
+   struct device *pdev;
+   struct regmap *regmap;
+   struct mutex lock;
+   struct regulator *vdd;
+   uint8_t pwm_val[SN3112_CHANNELS];
+   uint8_t pwm_en_reg[3];
+   bool pwm_en[SN3112_CHANNELS];
+#if IS_ENABLED(CONFIG_GPIOLIB)
+   struct gpio_desc *sdb;
+#endif
+};
+
+static int sn3112_write_reg(struct sn3112 *priv, unsigned int reg,
+   unsigned int val)
+{
+   int err;
+
+   dev_dbg(priv->pdev, "request regmap_write 0x%x 0x%x\n", reg, val);
+   err = regmap_write(priv->regmap, reg, val);
+   if (err)
+   dev_warn_ratelimited(
+   priv->pdev,
+   "regmap_write to register 0x%x failed: %pe\n", reg,
+   ERR_PTR(err));


Multi-line should be encapsulated in {}

if (err) {
stuff
goes here
}


+   return err;
+}
+
+static int sn3112_set_en_reg(struct sn3112 *priv, unsigned int channel,
+bool enabled, bool write)
+{
+   unsigned int reg, bit;
+
+   if (channel >= SN3112_CHANNELS)
+   return -EINVAL;
+
+   /* LED_EN1: BIT5:BIT3 = OUT3:OUT1 */
+   if (channel >= 0 && channel <= 2)
+   reg = 0, bit = channel + 3;
+   /* LED_EN2: BIT5:BIT0 = OUT9:OUT4 */
+   else if (channel >= 3 && channel <= 8)
+   reg = 1, bit = channel - 3;
+   /* LED_EN3: BIT2:BIT0 = OUT12:OUT10 */
+   else if (channel >= 9 && channel <= 11)
+   reg = 2, bit = channel - 9;
+   else
+   return -EINVAL;
+
+   dev_dbg(priv->pdev, "channel %u enabled %u\n", channel, enabled);
+   dev_dbg(priv->pdev, "reg %u bit %u\n", reg, bit);
+   if (enabled)
+   set_bit(bit, (ulong *)>pwm_en_reg[reg]);
+   else
+   clear_bit(bit, (ulong *)>pwm_en_reg[reg]);
+   dev_dbg(priv->pdev, "set enable reg %u to %u\n", reg,
+   priv->pwm_en_reg[reg]);
+
+   if (!write)
+   return 0;

newline

+   return sn3112_write_reg(priv, SN3112_REG_PWM_EN + reg,
+   priv->pwm_en_reg[reg]);
+}
+
+static int sn3112_set_val_reg(struct sn3112 *priv, unsigned int channel,
+ uint8_t val, bool write)
+{
+   if (channel >= SN3112_CHANNELS)
+   return -EINVAL;

newline

+   priv->pwm_val[channel] = val;
+   dev_dbg(priv->pdev, "set value reg %u to %u\n", channel,
+   priv->pwm_val[channel]);
+
+   if (!write)
+   return 0;

newline

+   return sn3112_write_reg(priv, SN3112_REG_PWM_VAL + channel,
+  

[PATCH 02/10] pwm: Add SI-EN SN3112 PWM support

2024-04-24 Thread Xilin Wu via B4 Relay
From: Junhao Xie 

Add a new driver for the SI-EN SN3112 12-channel 8-bit PWM LED controller.

Signed-off-by: Junhao Xie 
---
 drivers/pwm/Kconfig  |  10 ++
 drivers/pwm/Makefile |   1 +
 drivers/pwm/pwm-sn3112.c | 336 +++
 3 files changed, 347 insertions(+)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 1dd7921194f5..e21c37c7991e 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -553,6 +553,16 @@ config PWM_SL28CPLD
  To compile this driver as a module, choose M here: the module
  will be called pwm-sl28cpld.
 
+config PWM_SN3112
+   tristate "SI-EN SN3112 PWM driver"
+   depends on I2C
+   select REGMAP_I2C
+   help
+ Generic PWM framework driver for SI-EN SN3112 LED controller.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-sn3112.
+
 config PWM_SPEAR
tristate "STMicroelectronics SPEAr PWM support"
depends on PLAT_SPEAR || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 90913519f11a..6aab2d113159 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_PWM_RZ_MTU3) += pwm-rz-mtu3.o
 obj-$(CONFIG_PWM_SAMSUNG)  += pwm-samsung.o
 obj-$(CONFIG_PWM_SIFIVE)   += pwm-sifive.o
 obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o
+obj-$(CONFIG_PWM_SN3112)   += pwm-sn3112.o
 obj-$(CONFIG_PWM_SPEAR)+= pwm-spear.o
 obj-$(CONFIG_PWM_SPRD) += pwm-sprd.o
 obj-$(CONFIG_PWM_STI)  += pwm-sti.o
diff --git a/drivers/pwm/pwm-sn3112.c b/drivers/pwm/pwm-sn3112.c
new file mode 100644
index ..38ef948602a3
--- /dev/null
+++ b/drivers/pwm/pwm-sn3112.c
@@ -0,0 +1,336 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for SN3112 12-channel 8-bit PWM LED controller
+ *
+ * Copyright (c) 2024 Junhao Xie 
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define SN3112_CHANNELS 12
+#define SN3112_REG_ENABLE 0x00
+#define SN3112_REG_PWM_VAL 0x04
+#define SN3112_REG_PWM_EN 0x13
+#define SN3112_REG_APPLY 0x16
+#define SN3112_REG_RESET 0x17
+
+struct sn3112 {
+   struct device *pdev;
+   struct regmap *regmap;
+   struct mutex lock;
+   struct regulator *vdd;
+   uint8_t pwm_val[SN3112_CHANNELS];
+   uint8_t pwm_en_reg[3];
+   bool pwm_en[SN3112_CHANNELS];
+#if IS_ENABLED(CONFIG_GPIOLIB)
+   struct gpio_desc *sdb;
+#endif
+};
+
+static int sn3112_write_reg(struct sn3112 *priv, unsigned int reg,
+   unsigned int val)
+{
+   int err;
+
+   dev_dbg(priv->pdev, "request regmap_write 0x%x 0x%x\n", reg, val);
+   err = regmap_write(priv->regmap, reg, val);
+   if (err)
+   dev_warn_ratelimited(
+   priv->pdev,
+   "regmap_write to register 0x%x failed: %pe\n", reg,
+   ERR_PTR(err));
+
+   return err;
+}
+
+static int sn3112_set_en_reg(struct sn3112 *priv, unsigned int channel,
+bool enabled, bool write)
+{
+   unsigned int reg, bit;
+
+   if (channel >= SN3112_CHANNELS)
+   return -EINVAL;
+
+   /* LED_EN1: BIT5:BIT3 = OUT3:OUT1 */
+   if (channel >= 0 && channel <= 2)
+   reg = 0, bit = channel + 3;
+   /* LED_EN2: BIT5:BIT0 = OUT9:OUT4 */
+   else if (channel >= 3 && channel <= 8)
+   reg = 1, bit = channel - 3;
+   /* LED_EN3: BIT2:BIT0 = OUT12:OUT10 */
+   else if (channel >= 9 && channel <= 11)
+   reg = 2, bit = channel - 9;
+   else
+   return -EINVAL;
+
+   dev_dbg(priv->pdev, "channel %u enabled %u\n", channel, enabled);
+   dev_dbg(priv->pdev, "reg %u bit %u\n", reg, bit);
+   if (enabled)
+   set_bit(bit, (ulong *)>pwm_en_reg[reg]);
+   else
+   clear_bit(bit, (ulong *)>pwm_en_reg[reg]);
+   dev_dbg(priv->pdev, "set enable reg %u to %u\n", reg,
+   priv->pwm_en_reg[reg]);
+
+   if (!write)
+   return 0;
+   return sn3112_write_reg(priv, SN3112_REG_PWM_EN + reg,
+   priv->pwm_en_reg[reg]);
+}
+
+static int sn3112_set_val_reg(struct sn3112 *priv, unsigned int channel,
+ uint8_t val, bool write)
+{
+   if (channel >= SN3112_CHANNELS)
+   return -EINVAL;
+   priv->pwm_val[channel] = val;
+   dev_dbg(priv->pdev, "set value reg %u to %u\n", channel,
+   priv->pwm_val[channel]);
+
+   if (!write)
+   return 0;
+   return sn3112_write_reg(priv, SN3112_REG_PWM_VAL + channel,
+   priv->pwm_val[channel]);
+}
+
+static int sn3112_write_all(struct sn3112 *priv)
+{
+   int i, ret;
+
+   /* regenerate enable register values */
+   for (i = 0; i < SN3112_CHANNELS; i++) {
+