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@xxxxxxx> > > > > Add PWM function support for MFD adp5585. > > > > Reviewed-by: Haibo Chen <haibo.chen@xxxxxxx> > > Signed-off-by: Clark Wang <xiaoning.wang@xxxxxxx> > > Signed-off-by: Jindong Yue <jindong.yue@xxxxxxx> > > Signed-off-by: Frank Li <Frank.Li@xxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxxx/ 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, ®_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@xxxxxxx>"); > > 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");
