From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
To: wuxilin123@gmail.com,
"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
"Rob Herring" <robh@kernel.org>,
"Krzysztof Kozlowski" <krzk+dt@kernel.org>,
"Conor Dooley" <conor+dt@kernel.org>,
"Junhao Xie" <bigfoot@classfun.cn>,
"Neil Armstrong" <neil.armstrong@linaro.org>,
"Jessica Zhang" <quic_jesszhan@quicinc.com>,
"Sam Ravnborg" <sam@ravnborg.org>,
"David Airlie" <airlied@gmail.com>,
"Daniel Vetter" <daniel@ffwll.ch>,
"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
"Maxime Ripard" <mripard@kernel.org>,
"Thomas Zimmermann" <tzimmermann@suse.de>,
"Bjorn Andersson" <andersson@kernel.org>,
"Konrad Dybcio" <konrad.dybcio@linaro.org>,
"Tengfei Fan" <quic_tengfan@quicinc.com>,
"Molly Sophia" <mollysophia379@gmail.com>
Cc: linux-pwm@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH 02/10] pwm: Add SI-EN SN3112 PWM support
Date: Wed, 24 Apr 2024 16:55:26 +0100 [thread overview]
Message-ID: <17d36836-67cb-4d5a-a8b4-ecf1517a0020@linaro.org> (raw)
In-Reply-To: <20240424-ayn-odin2-initial-v1-2-e0aa05c991fd@gmail.com>
On 24/04/2024 16:29, Xilin Wu via B4 Relay wrote:
> From: Junhao Xie <bigfoot@classfun.cn>
>
> Add a new driver for the SI-EN SN3112 12-channel 8-bit PWM LED controller.
>
> Signed-off-by: Junhao Xie <bigfoot@classfun.cn>
> ---
> 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@classfun.cn>
> + *
> + */
> +
> +#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@classfun.cn>");
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)
next prev parent reply other threads:[~2024-04-24 15:55 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-24 15:29 [PATCH 00/10] AYN Odin 2 support Xilin Wu via B4 Relay
2024-04-24 15:29 ` [PATCH 01/10] dt-bindings: pwm: Add SI-EN SN3112 PWM support Xilin Wu via B4 Relay
2024-04-25 6:04 ` Uwe Kleine-König
2024-04-24 15:29 ` [PATCH 02/10] " Xilin Wu via B4 Relay
2024-04-24 15:55 ` Bryan O'Donoghue [this message]
2024-04-24 19:09 ` BigfootACA
2024-04-25 6:08 ` Uwe Kleine-König
2024-04-24 19:37 ` Konrad Dybcio
2024-04-25 0:57 ` Junhao Xie
2024-04-25 7:57 ` Neil Armstrong
2024-04-25 6:14 ` Uwe Kleine-König
2024-04-25 6:02 ` Uwe Kleine-König
2024-04-25 7:03 ` Junhao Xie
2024-04-24 15:29 ` [PATCH 03/10] dt-bindings: display: panel: Add Synaptics TD4328 Xilin Wu via B4 Relay
2024-04-25 16:11 ` Rob Herring
2024-04-28 3:25 ` Xilin Wu
2024-04-24 15:29 ` [PATCH 04/10] drm/panel: Add driver for Synaptics TD4328 LCD panel Xilin Wu via B4 Relay
2024-04-24 15:58 ` Bryan O'Donoghue
2024-04-28 3:29 ` Xilin Wu
2024-04-24 15:29 ` [PATCH 05/10] arm64: dts: qcom: pmk8550: Add PWM controller Xilin Wu via B4 Relay
2024-04-24 19:38 ` Konrad Dybcio
2024-04-24 15:29 ` [PATCH 06/10] arm64: dts: qcom: sm8550: Add UART15 Xilin Wu via B4 Relay
2024-04-24 23:39 ` Dmitry Baryshkov
2024-04-24 15:29 ` [PATCH 07/10] arm64: dts: qcom: sm8550: Update EAS properties Xilin Wu via B4 Relay
2024-04-24 22:45 ` Bryan O'Donoghue
2024-04-28 3:43 ` Xilin Wu
2024-04-24 15:29 ` [PATCH 08/10] dt-bindings: vendor-prefixes: Add AYN Technologies Xilin Wu via B4 Relay
2024-04-25 16:13 ` Rob Herring
2024-04-28 3:44 ` Xilin Wu
2024-04-24 15:29 ` [PATCH 09/10] dt-bindings: arm: qcom: Add AYN Odin 2 Xilin Wu via B4 Relay
2024-04-24 15:43 ` Bryan O'Donoghue
2024-04-24 15:29 ` [PATCH 10/10] arm64: dts: " Xilin Wu via B4 Relay
2024-04-25 6:28 ` Krzysztof Kozlowski
2024-04-28 3:54 ` Xilin Wu
2024-04-30 10:26 ` Konrad Dybcio
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=17d36836-67cb-4d5a-a8b4-ecf1517a0020@linaro.org \
--to=bryan.odonoghue@linaro.org \
--cc=airlied@gmail.com \
--cc=andersson@kernel.org \
--cc=bigfoot@classfun.cn \
--cc=conor+dt@kernel.org \
--cc=daniel@ffwll.ch \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=konrad.dybcio@linaro.org \
--cc=krzk+dt@kernel.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=mollysophia379@gmail.com \
--cc=mripard@kernel.org \
--cc=neil.armstrong@linaro.org \
--cc=quic_jesszhan@quicinc.com \
--cc=quic_tengfan@quicinc.com \
--cc=robh@kernel.org \
--cc=sam@ravnborg.org \
--cc=tzimmermann@suse.de \
--cc=u.kleine-koenig@pengutronix.de \
--cc=wuxilin123@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).