Message-ID: <ZpfVNHQQaJvdnB+B@lizhi-Precision-Tower-5810>
Date: Wed, 17 Jul 2024 10:29:08 -0400
From: Frank Li <Frank.li@....com>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>
Cc: Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
	linux-pwm@...r.kernel.org, imx@...ts.linux.dev,
	linux-arm-kernel@...ts.infradead.org,
	Clark Wang <xiaoning.wang@....com>, Haibo Chen <haibo.chen@....com>,
	Jindong Yue <jindong.yue@....com>
Subject: Re: [PATCH 4/6] pwm: adp5585: add adp5585 PWM support

On Wed, Jul 17, 2024 at 11:04:50AM +0200, Uwe Kleine-König wrote:
> Hello Clark,
> 
> On Tue, Jul 16, 2024 at 03:28:27PM -0400, Frank Li wrote:
> > From: Clark Wang <xiaoning.wang@....com>
> > 
> > Add PWM function support for MFD adp5585.
> > 
> > Reviewed-by: Haibo Chen <haibo.chen@....com>
> > Signed-off-by: Clark Wang <xiaoning.wang@....com>
> > Signed-off-by: Jindong Yue <jindong.yue@....com>
> > Signed-off-by: Frank Li <Frank.Li@....com>
> > ---
> >  drivers/pwm/Kconfig       |   8 ++
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-adp5585.c | 215 ++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 224 insertions(+)
> > 
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 3e53838990f5b..baaadf877b9c6 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -38,6 +38,14 @@ config PWM_DEBUG
> >  	  It is expected to introduce some runtime overhead and diagnostic
> >  	  output to the kernel log, so only enable while working on a driver.
> >  
> > +config PWM_ADP5585
> > +	tristate "ADP5585 PWM support"
> > +	depends on MFD_ADP5585
> > +	help
> > +	  This option enables support for on-chip PWM found
> > +	  on Analog Devices ADP5585.
> > +
> > +
> >  config PWM_AB8500
> >  	tristate "AB8500 PWM support"
> >  	depends on AB8500_CORE && ARCH_U8500
> 
> alphabetic ordering (by config symbol) please.

Thank you for you review. I just found someone already submit similar patch

https://lore.kernel.org/linux-gpio/20240608141633.2562-5-laurent.pinchart@ideasonboard.com/

Let's wait for laurent. If he is busy, I can rework base on the above one.

Frank

> 
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 0be4f3e6dd432..161131a261e94 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -2,6 +2,7 @@
> >  obj-$(CONFIG_PWM)		+= core.o
> >  obj-$(CONFIG_PWM_AB8500)	+= pwm-ab8500.o
> >  obj-$(CONFIG_PWM_APPLE)		+= pwm-apple.o
> > +obj-$(CONFIG_PWM_ADP5585)	+= pwm-adp5585.o
> >  obj-$(CONFIG_PWM_ATMEL)		+= pwm-atmel.o
> >  obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM)	+= pwm-atmel-hlcdc.o
> >  obj-$(CONFIG_PWM_ATMEL_TCB)	+= pwm-atmel-tcb.o
> 
> alphabetic ordering please.
> 
> > diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c
> > new file mode 100644
> > index 0000000000000..f578d24df5c74
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-adp5585.c
> > @@ -0,0 +1,215 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * PWM driver for Analog Devices ADP5585 MFD
> > + *
> > + * Copyright 2024 NXP
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/mfd/adp5585.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/slab.h>
> > +#include <linux/time.h>
> > +
> > +#define ADP5585_PWM_CHAN_NUM		1
> 
> This is only used once. I'd prefer to pass the 1 verbatim to
> pwmchip_alloc.
> 
> > +#define ADP5585_PWM_FASTEST_PERIOD_NS	2000
> > +#define ADP5585_PWM_SLOWEST_PERIOD_NS	131070000
> 
> Funny number. I wonder where this comes from.
> 
> > +struct adp5585_pwm_chip {
> > +	struct device *parent;
> > +	struct mutex lock;
> > +	u8 pin_config_val;
> > +};
> > +
> > +static inline struct adp5585_pwm_chip *
> > +to_adp5585_pwm_chip(struct pwm_chip *chip)
> > +{
> > +	return pwmchip_get_drvdata(chip);
> > +}
> > +
> > +static int adp5585_pwm_reg_read(struct adp5585_pwm_chip *adp5585_pwm, u8 reg, u8 *val)
> > +{
> > +	struct adp5585_dev *adp5585  = dev_get_drvdata(adp5585_pwm->parent);
> 
> s/  / /;
> ditto below in adp5585_pwm_reg_write().
> 
> > +
> > +	return adp5585->read_reg(adp5585, reg, val);
> > +}
> > +
> > +static int adp5585_pwm_reg_write(struct adp5585_pwm_chip *adp5585_pwm, u8 reg, u8 val)
> > +{
> > +	struct adp5585_dev *adp5585  = dev_get_drvdata(adp5585_pwm->parent);
> > +
> > +	return adp5585->write_reg(adp5585, reg, val);
> > +}
> > +
> > +static int pwm_adp5585_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > +				 struct pwm_state *state)
> > +{
> > +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> > +	u32 on, off;
> > +	u8 temp;
> > +
> > +	/* get period */
> > +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_OFFT_LOW, &temp);
> > +	off = temp;
> > +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_OFFT_HIGH, &temp);
> > +	off |= temp << 8;
> > +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_ONT_LOW, &temp);
> > +	on = temp;
> > +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_ONT_HIGH, &temp);
> > +	on |= temp << 8;
> > +	state->period = (on + off) * NSEC_PER_USEC;
> > +
> > +	state->duty_cycle = on;
> > +	state->polarity = PWM_POLARITY_NORMAL;
> > +
> > +	/* get channel status */
> > +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_CFG, &temp);
> > +	state->enabled = temp & ADP5585_PWM_CFG_EN;
> > +
> > +	return 0;
> > +}
> > +
> > +static int pwm_adp5585_apply(struct pwm_chip *chip,
> > +			     struct pwm_device *pwm,
> > +			     const struct pwm_state *state)
> > +{
> > +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> > +	u32 on, off;
> > +	u8 enabled;
> > +	int ret;
> > +
> > +	if (state->period > ADP5585_PWM_SLOWEST_PERIOD_NS ||
> > +	    state->period < ADP5585_PWM_FASTEST_PERIOD_NS)
> > +		return -EINVAL;
> > +
> > +	guard(mutex)(&adp5585_pwm->lock);
> 
> What does this protect? You're allowed (and expected) to assume that the
> consumer serializes calls to .apply() for a single pwm_device. Given
> that you have npwm=1 I think this lock can be dropped.
> 
> > +	/* set on/off cycle*/
> > +	on = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, NSEC_PER_USEC);
> > +	off = DIV_ROUND_CLOSEST_ULL((state->period - state->duty_cycle), NSEC_PER_USEC);
> 
> Please enable PWM_DEBUG your tests and make sure it doesn't produce
> warnings. (Hint: round_closest is wrong)
> 
> > +	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_OFFT_LOW, off & ADP5585_REG_MASK);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_OFFT_HIGH,
> > +				    (off >> 8) & ADP5585_REG_MASK);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_ONT_LOW, on & ADP5585_REG_MASK);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_ONT_HIGH,
> > +				    (on >> 8) & ADP5585_REG_MASK);
> > +	if (ret)
> > +		return ret;
> 
> How does the hardware behave in between these register writes? Can it
> happen that an intermediate state is visible on the output pin? (E.g.
> because off is already written but on is still the old value. Or even
> off is only partly written after the first byte write.)
> 
> Please document this behaviour in a paragraph at the top of the driver
> in the same way as many other PWM drivers do it. The details should be
> extractable by
> 
> 	sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c
> 
> > +
> > +	/* enable PWM and set to continuous PWM mode*/
> 
> Missing space before comment ending delimiter
> 
> > +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_CFG, &enabled);
> > +	if (state->enabled)
> > +		ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_CFG, ADP5585_PWM_CFG_EN);
> > +	else
> > +		ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_CFG, 0);
> > +
> > +	return ret;
> > +}
> > +
> > +static int pwm_adp5585_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> > +	u8 reg_cfg;
> > +	int ret;
> > +
> > +	guard(mutex)(&adp5585_pwm->lock);
> > +
> > +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PIN_CONFIG_C, &adp5585_pwm->pin_config_val);
> > +	reg_cfg = adp5585_pwm->pin_config_val & ~ADP5585_PIN_CONFIG_R3_MASK;
> > +	reg_cfg |= ADP5585_PIN_CONFIG_R3_PWM;
> > +	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PIN_CONFIG_C, reg_cfg);
> 
> ret is only written to here, ditto for &adp5585_pwm->pin_config_val.
> 
> > +
> > +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_GENERAL_CFG, &adp5585_pwm->pin_config_val);
> > +	reg_cfg |= ADP5585_GENERAL_CFG_OSC_EN;
> > +	ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_GENERAL_CFG, reg_cfg);
> 
> Please add a comment about what is happening here. I assume this sets up
> pinmuxing and enabled the oscillator. I wonder if it is sensible to do
> the latter only in .apply() iff state->enabled = true.
> 
> > +
> > +	return ret;
> > +}
> > +
> > +static void pwm_adp5585_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> > +	u8 reg_cfg;
> > +
> > +	guard(mutex)(&adp5585_pwm->lock);
> > +
> > +	adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PIN_CONFIG_C, &reg_cfg);
> > +	reg_cfg &= ~ADP5585_PIN_CONFIG_R3_MASK;
> > +	reg_cfg |= adp5585_pwm->pin_config_val & ADP5585_PIN_CONFIG_R3_MASK;
> > +	adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PIN_CONFIG_C, reg_cfg);
> 
> It would be consequent to clear ADP5585_GENERAL_CFG_OSC_EN in this
> function given that it's set in .request().
> 
> > +}
> > +
> > +static const struct pwm_ops adp5585_pwm_ops = {
> > +	.request = pwm_adp5585_request,
> > +	.free = pwm_adp5585_free,
> > +	.get_state = pwm_adp5585_get_state,
> > +	.apply = pwm_adp5585_apply,
> > +};
> > +
> > +static int adp5585_pwm_probe(struct platform_device *pdev)
> > +{
> > +	struct adp5585_pwm_chip *adp5585_pwm;
> > +	struct pwm_chip *chip;
> > +	unsigned int npwm = ADP5585_PWM_CHAN_NUM;
> > +	int ret;
> > +
> > +	chip = devm_pwmchip_alloc(&pdev->dev, npwm, sizeof(*adp5585_pwm));
> > +	if (IS_ERR(chip))
> > +		return PTR_ERR(chip);
> 
> Error message using dev_err_probe() please.
> 
> > +
> > +	adp5585_pwm = to_adp5585_pwm_chip(chip);
> > +	adp5585_pwm->parent = pdev->dev.parent;
> > +
> > +	platform_set_drvdata(pdev, adp5585_pwm);
> > +
> > +	chip->ops = &adp5585_pwm_ops;
> > +	mutex_init(&adp5585_pwm->lock);
> > +
> > +	ret = devm_pwmchip_add(&pdev->dev, chip);
> > +	if (ret)
> > +		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
> 
> Please use dev_err_probe().
> 
> > +
> > +	return ret;
> > +}
> > +
> > +static void adp5585_pwm_remove(struct platform_device *pdev)
> > +{
> > +	struct pwm_chip *chip = platform_get_drvdata(pdev);
> > +
> > +	pwmchip_remove(chip);
> 
> Did you test this? I'd expect this to explode.
> 
> > +}
> > +
> > +static const struct of_device_id adp5585_pwm_of_match[] = {
> > +	{.compatible = "adp5585-pwm", },
> 
> Missing space after the opening brace.
> 
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, adp5585_pwm_of_match);
> > +
> > +static struct platform_driver adp5585_pwm_driver = {
> > +	.driver	= {
> > +		.name	= "adp5585-pwm",
> > +		.of_match_table = adp5585_pwm_of_match,
> > +	},
> > +	.probe		= adp5585_pwm_probe,
> > +	.remove		= adp5585_pwm_remove,
> > +};
> > +module_platform_driver(adp5585_pwm_driver);
> > +
> > +MODULE_AUTHOR("Xiaoning Wang <xiaoning.wang@....com>");
> 
> The email address matches one of the S-o-b lines, the name however is
> different. Is this by mistake?
> 
> > +MODULE_DESCRIPTION("ADP5585 PWM Driver");
> > +MODULE_LICENSE("GPL");

Powered by blists - more mailing lists