| Message ID | 20240716-adi-v1-4-79c0122986e7@nxp.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | mfd: add adp5585 gpio and pwm support | expand |
Hello Clark, On Tue, Jul 16, 2024 at 03:28:27PM -0400, Frank Li wrote: > From: Clark Wang <xiaoning.wang@nxp.com> > > Add PWM function support for MFD adp5585. > > Reviewed-by: Haibo Chen <haibo.chen@nxp.com> > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > Signed-off-by: Jindong Yue <jindong.yue@nxp.com> > Signed-off-by: Frank Li <Frank.Li@nxp.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. > 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@nxp.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");
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@nxp.com> > > > > Add PWM function support for MFD adp5585. > > > > Reviewed-by: Haibo Chen <haibo.chen@nxp.com> > > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > > Signed-off-by: Jindong Yue <jindong.yue@nxp.com> > > Signed-off-by: Frank Li <Frank.Li@nxp.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, ®_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@nxp.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");
On Wed, Jul 17, 2024 at 11:29 AM Frank Li <Frank.li@nxp.com> wrote: > 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. Adding Laurent.
On Wed, Jul 17, 2024 at 12:35:29PM -0300, Fabio Estevam wrote: > On Wed, Jul 17, 2024 at 11:29 AM Frank Li <Frank.li@nxp.com> wrote: > > > 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. > > Adding Laurent. Thanks Fabio. I had a quick look at this series, and I think mine is better as it supports more features and more chip variants.
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 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 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 +#define ADP5585_PWM_FASTEST_PERIOD_NS 2000 +#define ADP5585_PWM_SLOWEST_PERIOD_NS 131070000 + +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); + + 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); + + /* 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); + + 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; + + /* enable PWM and set to continuous PWM mode*/ + 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); + + 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); + + 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); +} + +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); + + 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); + + return ret; +} + +static void adp5585_pwm_remove(struct platform_device *pdev) +{ + struct pwm_chip *chip = platform_get_drvdata(pdev); + + pwmchip_remove(chip); +} + +static const struct of_device_id adp5585_pwm_of_match[] = { + {.compatible = "adp5585-pwm", }, + { /* 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@nxp.com>"); +MODULE_DESCRIPTION("ADP5585 PWM Driver"); +MODULE_LICENSE("GPL");