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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 24/04/2024 16:29, Xilin Wu via B4 Relay wrote:
From: Junhao Xie <bigfoot@xxxxxxxxxxx>

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

Signed-off-by: Junhao Xie <bigfoot@xxxxxxxxxxx>
---
  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 000000000000..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 <bigfoot@xxxxxxxxxxx>
+ *
+ */
+
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>

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 *)&priv->pwm_en_reg[reg]);
+	else
+		clear_bit(bit, (ulong *)&priv->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,
+				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)
+		return ret;
+
+	/* set software enable register */
+	ret = sn3112_write_reg(priv, SN3112_REG_ENABLE, 1);
+	if (ret != 0)
+		return ret;
+
+	/* rewrite pwm value register */
+	for (i = 0; i < SN3112_CHANNELS; i++) {
+		ret = sn3112_write_reg(priv, SN3112_REG_PWM_VAL + i,
+				       priv->pwm_val[i]);
+		if (ret != 0)
+			return ret;
+	}
+
+	/* rewrite pwm enable register */
+	for (i = 0; i < 3; i++) {
+		ret = sn3112_write_reg(priv, SN3112_REG_PWM_EN + i,
+				       priv->pwm_en_reg[i]);
+		if (ret != 0)
+			return ret;
+	}
+
+	/* use random value to apply changes */
+	ret = sn3112_write_reg(priv, SN3112_REG_APPLY, 0x66);
+	if (ret != 0)
+		return ret;
+
+	dev_dbg(priv->pdev, "reinitialized\n");
+	return 0;
+}
+
+static int sn3112_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct sn3112 *priv = pwmchip_get_drvdata(chip);
+
+	if (pwm->hwpwm >= SN3112_CHANNELS)
+		return -EINVAL;
+
+	dev_dbg(priv->pdev, "sn3112 request channel %u\n", pwm->hwpwm);
+	pwm->args.period = 1000000;
newline
+	return 0;
+}
+
+static int sn3112_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			    const struct pwm_state *state)
+{
+	u64 val = 0;
+	struct sn3112 *priv = pwmchip_get_drvdata(chip);
+
+	if (pwm->hwpwm >= SN3112_CHANNELS)
+		return -EINVAL;
+
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
+
+	if (state->period <= 0)
+		return -EINVAL;
+
+	val = mul_u64_u64_div_u64(state->duty_cycle, 0xff, state->period);
+	dev_dbg(priv->pdev, "duty_cycle %llu period %llu\n", state->duty_cycle,
+		state->period);
+	dev_dbg(priv->pdev, "set channel %u value to %llu\n", pwm->hwpwm, val);
+	dev_dbg(priv->pdev, "set channel %u enabled to %u\n", pwm->hwpwm,
+		state->enabled);
+
+	mutex_lock(&priv->lock);
+	sn3112_set_en_reg(priv, pwm->hwpwm, state->enabled, true);
+	sn3112_set_val_reg(priv, pwm->hwpwm, val, true);
+	sn3112_write_reg(priv, SN3112_REG_APPLY, 0x66);
+	mutex_unlock(&priv->lock);
+
+	return 0;
+}
+
+static const struct pwm_ops sn3112_pwm_ops = {
+	.apply = sn3112_pwm_apply,
+	.request = sn3112_pwm_request,
+};
+
+static const struct regmap_config sn3112_regmap_i2c_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 24,
+	.cache_type = REGCACHE_NONE,
+};
+
+static int sn3112_pwm_probe(struct i2c_client *client)
+{
+	struct pwm_chip *chip;
+	struct sn3112 *priv;
+	int ret, i;
+
+	dev_dbg(&client->dev, "probing\n");
You can probably live without that in an upstream driver..

+	chip = devm_pwmchip_alloc(&client->dev, SN3112_CHANNELS, sizeof(*priv));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+	priv = pwmchip_get_drvdata(chip);
+	priv->pdev = &client->dev;
+
+	/* initialize sn3112 (chip does not support read command) */
+	for (i = 0; i < SN3112_CHANNELS; i++)
+		priv->pwm_en[i] = false;
+	for (i = 0; i < SN3112_CHANNELS; i++)
+		priv->pwm_val[i] = 0;
+	for (i = 0; i < 3; i++)
+		priv->pwm_en_reg[i] = 0;

Why does pwm_val have a define to constrain the array but pwm_en_reg have hard-coded 3 ?

Suggest using a #define for the 3 value for consistency / extensibility.

+
+	/* enable sn5112 power vdd */
+	priv->vdd = devm_regulator_get(priv->pdev, "vdd");
+	if (IS_ERR(priv->vdd)) {
+		ret = PTR_ERR(priv->vdd);
+		dev_err(priv->pdev, "Unable to get vdd regulator: %d\n", ret);
+		return ret;
+	}
+
+#if IS_ENABLED(CONFIG_GPIOLIB)
+	/* sn5112 hardware shutdown pin */
+	priv->sdb = devm_gpiod_get_optional(priv->pdev, "sdb", GPIOD_OUT_LOW);
+	if (PTR_ERR(priv->sdb) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+#endif
+
+	/* enable sn5112 power vdd */
+	ret = regulator_enable(priv->vdd);
+	if (ret < 0) {
+		dev_err(priv->pdev, "Unable to enable regulator: %d\n", ret);
+		return ret;
+	}
+
+	priv->regmap = devm_regmap_init_i2c(client, &sn3112_regmap_i2c_config);
+	if (IS_ERR(priv->regmap)) {
+		ret = PTR_ERR(priv->regmap);
+		dev_err(priv->pdev, "Failed to initialize register map: %d\n",
+			ret);
+		return ret;
+	}
+
+	i2c_set_clientdata(client, chip);
+	mutex_init(&priv->lock);
+
+	chip->ops = &sn3112_pwm_ops;
+	ret = pwmchip_add(chip);
+	if (ret < 0)
+		return ret;
+
+#if IS_ENABLED(CONFIG_GPIOLIB)
+	/* disable hardware shutdown pin */
+	if (priv->sdb)
+		gpiod_set_value(priv->sdb, 0);
+#endif
+
+	/* initialize registers */
+	ret = sn3112_write_all(priv);
+	if (ret != 0) {
+		dev_err(priv->pdev, "Failed to initialize sn3112: %d\n", ret);
+		return ret;
+	}
+
+	dev_info(&client->dev,
+		 "Found SI-EN SN3112 12-channel 8-bit PWM LED controller\n");
newline
+	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");
suggest dropping from both probe() and remove()
+
+	/* 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);
+}
+
+static const struct i2c_device_id sn3112_id[] = {
+	{ "sn3112", 0 },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i2c, sn3112_id);
+
+#ifdef CONFIG_OF
+static const struct of_device_id sn3112_dt_ids[] = {
+	{ .compatible = "si-en,sn3112-pwm", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sn3112_dt_ids);
+#endif
+
+static struct i2c_driver sn3112_i2c_driver = {
+	.driver = {
+		.name = "sn3112-pwm",
+		.of_match_table = of_match_ptr(sn3112_dt_ids),
+	},
+	.probe = sn3112_pwm_probe,
+	.remove = sn3112_pwm_remove,
+	.id_table = sn3112_id,
+};
+
+module_i2c_driver(sn3112_i2c_driver);
+
+MODULE_AUTHOR("BigfootACA <bigfoot@xxxxxxxxxxx>");
BigFootACA not Xilin Wu ?
+MODULE_DESCRIPTION("PWM driver for SI-EN SN3112");
+MODULE_LICENSE("GPL");


Also please consider the following checkpatch errors

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#50:
new file mode 100644

CHECK: struct mutex definition without comment
#83: FILE: drivers/pwm/pwm-sn3112.c:29:
+	struct mutex lock;

CHECK: Prefer kernel type 'u8' over 'uint8_t'
#85: FILE: drivers/pwm/pwm-sn3112.c:31:
+	uint8_t pwm_val[SN3112_CHANNELS];

CHECK: Prefer kernel type 'u8' over 'uint8_t'
#86: FILE: drivers/pwm/pwm-sn3112.c:32:
+	uint8_t pwm_en_reg[3];

CHECK: Lines should not end with a '('
#101: FILE: drivers/pwm/pwm-sn3112.c:47:
+		dev_warn_ratelimited(

CHECK: Prefer kernel type 'u8' over 'uint8_t'
#145: FILE: drivers/pwm/pwm-sn3112.c:91:
+			      uint8_t val, bool write)




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux