Hello,
On Thu, Oct 24, 2024 at 04:59:21PM +0800, Ming Yu wrote:
> This driver supports PWM functionality for NCT6694 MFD device
> based on USB interface.
>
> Signed-off-by: Ming Yu <tmyu0@xxxxxxxxxxx>
> ---
> MAINTAINERS | 1 +
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-nct6694.c | 245 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 257 insertions(+)
> create mode 100644 drivers/pwm/pwm-nct6694.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5c350eac187d..4d5a5eded3b9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16444,6 +16444,7 @@ F: drivers/iio/adc/nct6694_adc.c
> F: drivers/i2c/busses/i2c-nct6694.c
> F: drivers/mfd/nct6694.c
> F: drivers/net/can/nct6694_canfd.c
> +F: drivers/pwm/pwm-nct6694.c
> F: drivers/watchdog/nct6694_wdt.c
> F: include/linux/mfd/nct6694.h
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 0915c1e7df16..00b5eb13f99d 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -471,6 +471,16 @@ config PWM_NTXEC
> controller found in certain e-book readers designed by the original
> design manufacturer Netronix.
>
> +config PWM_NCT6694
> + tristate "Nuvoton NCT6694 PWM support"
> + depends on MFD_NCT6694
> + help
> + If you say yes to this option, support will be included for Nuvoton
> + NCT6694, a USB device to PWM controller.
> +
> + This driver can also be built as a module. If so, the module
> + will be called pwm-nct6694.
> +
> config PWM_OMAP_DMTIMER
> tristate "OMAP Dual-Mode Timer PWM support"
> depends on OF
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9081e0c0e9e0..5c5602b79402 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -42,6 +42,7 @@ obj-$(CONFIG_PWM_MICROCHIP_CORE) += pwm-microchip-core.o
> obj-$(CONFIG_PWM_MTK_DISP) += pwm-mtk-disp.o
> obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
> obj-$(CONFIG_PWM_NTXEC) += pwm-ntxec.o
> +obj-$(CONFIG_PWM_NCT6694) += pwm-nct6694.o
> obj-$(CONFIG_PWM_OMAP_DMTIMER) += pwm-omap-dmtimer.o
> obj-$(CONFIG_PWM_PCA9685) += pwm-pca9685.o
> obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
> diff --git a/drivers/pwm/pwm-nct6694.c b/drivers/pwm/pwm-nct6694.c
> new file mode 100644
> index 000000000000..915a2ab50834
> --- /dev/null
> +++ b/drivers/pwm/pwm-nct6694.c
> @@ -0,0 +1,245 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Nuvoton NCT6694 PWM driver based on USB interface.
> + *
> + * Copyright (C) 2024 Nuvoton Technology Corp.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pwm.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/nct6694.h>
> +
> +#define DRVNAME "nct6694-pwm"
> +
> +#define NR_PWM 10
> +#define MAX_PERIOD_NS 40000 /* PWM Maximum Frequency = 25kHz */
Please use a prefix for your defines, otherwise they make a more general
impression than justified.
> +#define PERIOD_NS_CONST 10200000 /* Period_ns to Freq_reg */
What is Freq_reg?
> +/* Host interface */
> +#define REQUEST_RPT_MOD 0xFF
> +#define REQUEST_HWMON_MOD 0x00
> +#define REQUEST_PWM_MOD 0x01
> +
> +/* Report Channel */
> +#define HWMON_PWM_IDX(x) (0x70 + (x))
> +
> +/* Message Channel -HWMON */
> +/* Command 00h */
> +#define REQUEST_HWMON_CMD0_LEN 0x40
> +#define REQUEST_HWMON_CMD0_OFFSET 0x0000 /* OFFSET = SEL|CMD */
> +#define HWMON_PWM_EN(x) (0x06 + (x))
> +#define HWMON_PWM_PP(x) (0x08 + (x))
> +#define HWMON_PWM_FREQ_IDX(x) (0x30 + (x))
> +
> +/* Message Channel -FAN */
> +/* Command 00h */
> +#define REQUEST_PWM_CMD0_LEN 0x08
> +#define REQUEST_PWM_CMD0_OFFSET 0x0000 /* OFFSET = SEL|CMD */
> +#define PWM_CH_EN(x) (x ? 0x00 : 0x01)
> +/* Command 01h */
> +#define REQUEST_PWM_CMD1_LEN 0x20
> +#define REQUEST_PWM_CMD1_OFFSET 0x0001 /* OFFSET = SEL|CMD */
> +#define PWM_MAL_EN(x) (x ? 0x00 : 0x01)
> +#define PWM_MAL_VAL(x) (0x02 + (x))
> +
> +/*
> + * Frequency <-> Period
> + * (10^9 * 255) / (25000 * Freq_reg) = Period_ns
> + * 10200000 / Freq_reg = Period_ns
> + *
> + * | Freq_reg | Freq_Hz | Period_ns |
> + * | 1 (01h | 98.039 | 10200000 |
missing )
> + * | 2 (02h) | 196.078 | 5100000 |
> + * | 3 (03h) | 294.117 | 3400000 |
> + * | ... |
> + * | ... |
> + * | ... |
Better use spaces for indention here.
> + * | 253 (FDh)| 24803.9 | 40316.20 |
> + * | 254 (FEh)| 24901.9 | 40157.48 |
> + * | 255 (FFh)| 25000 | 40000 |
Is this table useful?
I'd just write:
The emitted period P depends on the value F configured in
the freq register:
P = 255 s / (25000 * F)
= 10200000 ns / F
I wonder what happens if F == 0?
> + */
> +
> +struct nct6694_pwm_data {
> + struct nct6694 *nct6694;
> + unsigned char hwmon_cmd0_buf[REQUEST_HWMON_CMD0_LEN];
> + unsigned char pwm_cmd0_buf[REQUEST_PWM_CMD0_LEN];
> + unsigned char pwm_cmd1_buf[REQUEST_PWM_CMD1_LEN];
What are these arrays? register caches?
> +};
> +
> +static inline struct nct6694_pwm_data *to_nct6694_pwm_data(struct pwm_chip *chip)
> +{
> + return pwmchip_get_drvdata(chip);
> +}
> +
> +static int nct6694_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct nct6694_pwm_data *data = to_nct6694_pwm_data(chip);
> + unsigned char ch_enable = data->pwm_cmd0_buf[PWM_CH_EN(pwm->hwpwm / 8)];
> + unsigned char mal_enable = data->pwm_cmd1_buf[PWM_MAL_EN(pwm->hwpwm / 8)];
> + bool ch_en = ch_enable & BIT(pwm->hwpwm % 8);
> + bool mal_en = mal_enable & BIT(pwm->hwpwm % 8);
What is "mal"?
> +
> + if (!(ch_en && mal_en)) {
> + pr_err("%s: PWM(%d) is running in other mode!\n",
> + __func__, pwm->hwpwm);
> + return -EINVAL;
> + }
No error messages after .probe() please. dev_dbg() is fine however.
> + return 0;
> +}
> +
> +static int nct6694_pwm_get_state(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct nct6694_pwm_data *data = to_nct6694_pwm_data(chip);
> + unsigned char freq_reg, duty;
> +
> + /* Get pwm device initial state */
> + state->enabled = true;
> +
> + freq_reg = data->hwmon_cmd0_buf[HWMON_PWM_FREQ_IDX(pwm->hwpwm)];
> + state->period = PERIOD_NS_CONST / freq_reg;
I doubt you extensively tested your driver with PWM_DEBUG enabled. Hint:
You should probably use DIV_ROUND_UP here.
> + duty = data->pwm_cmd1_buf[PWM_MAL_VAL(pwm->hwpwm)];
> + state->duty_cycle = duty * state->period / 0xFF;
> +
> + return 0;
> +}
> +
> +static int nct6694_pwm_apply(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct nct6694_pwm_data *data = to_nct6694_pwm_data(chip);
> + unsigned char freq_reg, duty;
> + int ret;
> +
> + if (state->period < MAX_PERIOD_NS)
> + return -EINVAL;
> +
> + /* (10^9 * 255) / (25000 * Freq_reg) = Period_ns */
This could be less irritating if the formula included units. See above.
> + freq_reg = (unsigned char)(PERIOD_NS_CONST / state->period);
No need for the cast.
If state->period is bigger than PERIOD_NS_CONST, freq_reg ends up being
zero. That's related to the question above about F == 0.
> + data->hwmon_cmd0_buf[HWMON_PWM_FREQ_IDX(pwm->hwpwm)] = freq_reg;
> + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD,
> + REQUEST_HWMON_CMD0_OFFSET,
> + REQUEST_HWMON_CMD0_LEN,
> + data->hwmon_cmd0_buf);
> + if (ret)
> + return -EIO;
return ret;?
> +
> + /* Duty = duty * 0xFF */
I don't understand that.
> + duty = (unsigned char)(state->duty_cycle * 0xFF / state->period);
Please use the actual period implemented and not state->period.
> + data->pwm_cmd1_buf[PWM_MAL_VAL(pwm->hwpwm)] = duty;
> + if (state->enabled)
> + data->pwm_cmd1_buf[PWM_MAL_EN(pwm->hwpwm / 8)] |= BIT(pwm->hwpwm % 8);
> + else
> + data->pwm_cmd1_buf[PWM_MAL_EN(pwm->hwpwm / 8)] &= ~BIT(pwm->hwpwm % 8);
s/ / /
> + ret = nct6694_write_msg(data->nct6694, REQUEST_PWM_MOD,
> + REQUEST_PWM_CMD1_OFFSET, REQUEST_PWM_CMD1_LEN,
> + data->pwm_cmd1_buf);
> + if (ret)
> + return -EIO;
return ret;
> + return 0;
> +}
> +
> +static const struct pwm_ops nct6694_pwm_ops = {
> + .request = nct6694_pwm_request,
> + .apply = nct6694_pwm_apply,
> + .get_state = nct6694_pwm_get_state,
> +};
> +
> +static int nct6694_pwm_init(struct nct6694_pwm_data *data)
> +{
> + struct nct6694 *nct6694 = data->nct6694;
> + int ret;
> +
> + ret = nct6694_read_msg(nct6694, REQUEST_HWMON_MOD,
> + REQUEST_HWMON_CMD0_OFFSET,
> + REQUEST_HWMON_CMD0_LEN, 0,
> + REQUEST_HWMON_CMD0_LEN,
> + data->hwmon_cmd0_buf);
> + if (ret)
> + return ret;
> +
> + ret = nct6694_read_msg(nct6694, REQUEST_PWM_MOD,
> + REQUEST_PWM_CMD0_OFFSET,
> + REQUEST_PWM_CMD0_LEN, 0,
> + REQUEST_PWM_CMD0_LEN,
> + data->pwm_cmd0_buf);
> + if (ret)
> + return ret;
> +
> + ret = nct6694_read_msg(nct6694, REQUEST_PWM_MOD,
> + REQUEST_PWM_CMD1_OFFSET,
> + REQUEST_PWM_CMD1_LEN, 0,
> + REQUEST_PWM_CMD1_LEN,
> + data->pwm_cmd1_buf);
> + return ret;
> +}
> +
> +static int nct6694_pwm_probe(struct platform_device *pdev)
> +{
> + struct pwm_chip *chip;
> + struct nct6694_pwm_data *data;
> + struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent);
> + int ret;
> +
> + chip = devm_pwmchip_alloc(&pdev->dev, NR_PWM, sizeof(*data));
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
> +
> + data = to_nct6694_pwm_data(chip);
> +
> + data->nct6694 = nct6694;
> + chip->ops = &nct6694_pwm_ops;
> +
> + ret = nct6694_pwm_init(data);
> + if (ret)
> + return -EIO;
return dev_err_probe(dev, ret, "....\n");
> + /* Register pwm device to PWM framework */
> + ret = devm_pwmchip_add(&pdev->dev, chip);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to register pwm device!\n");
> + return ret;
> + }
Please use dev_err_probe().
> +
> + return 0;
> +}
> +
> +static struct platform_driver nct6694_pwm_driver = {
> + .driver = {
> + .name = DRVNAME,
DRVNAME is only used once (and a too generic name). Please just put the
string here directly.
> + },
> + .probe = nct6694_pwm_probe,
> +};
I don't like your aligning choices. Why do you intend the = for .probe
but not for .driver? My preferred style is a single space before the =.
While aligning the = is a subjective choice, a relevant downside is that
if later a longer member should get initialized you have to realign all
the otherwise unaffected lines to restore the alignment.
> +static int __init nct6694_init(void)
> +{
> + int err;
> +
> + err = platform_driver_register(&nct6694_pwm_driver);
> + if (!err) {
> + if (err)
Huh?
> + platform_driver_unregister(&nct6694_pwm_driver);
> + }
> +
> + return err;
> +}
> +subsys_initcall(nct6694_init);
Do you really need this to be at subsys init time?
> +static void __exit nct6694_exit(void)
> +{
> + platform_driver_unregister(&nct6694_pwm_driver);
> +}
> +module_exit(nct6694_exit);
> +
> +MODULE_DESCRIPTION("USB-PWM driver for NCT6694");
> +MODULE_AUTHOR("Ming Yu <tmyu0@xxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
