Message-ID: <imxfhjb5h5bneql6hadxanmzshxi2ev2tosprrncrerxrhvocl@lnoi32zupqct>
Date: Fri, 22 Nov 2024 19:05:33 +0100
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Ming Yu <a0282524688@...il.com>
Cc: tmyu0@...oton.com, lee@...nel.org, linus.walleij@...aro.org, 
	brgl@...ev.pl, andi.shyti@...nel.org, mkl@...gutronix.de, 
	mailhol.vincent@...adoo.fr, andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com, 
	kuba@...nel.org, pabeni@...hat.com, wim@...ux-watchdog.org, linux@...ck-us.net, 
	jdelvare@...e.com, jic23@...nel.org, lars@...afoo.de, alexandre.belloni@...tlin.com, 
	linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org, 
	linux-can@...r.kernel.org, netdev@...r.kernel.org, linux-watchdog@...r.kernel.org, 
	linux-hwmon@...r.kernel.org, linux-iio@...r.kernel.org, linux-pwm@...r.kernel.org, 
	linux-rtc@...r.kernel.org
Subject: Re: [PATCH v1 8/9] pwm: Add Nuvoton NCT6694 PWM support

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@...oton.com>
> ---
>  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@...oton.com>");
> +MODULE_LICENSE("GPL");

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists