| Message ID | 20240719203946.22909-5-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ADP5585 GPIO expander, PWM and keypad controller support | expand |
On Fri, Jul 19, 2024 at 11:39:46PM +0300, Laurent Pinchart wrote: > From: Clark Wang <xiaoning.wang@nxp.com> > > The ADP5585 is a 10/11 input/output port expander with a built in keypad > matrix decoder, programmable logic, reset generator, and PWM generator. > This driver supports the PWM function using the platform device > registered by the core MFD driver. > > The driver is derived from an initial implementation from NXP, available > in commit 113113742208 ("MLK-25922-1 pwm: adp5585: add adp5585 PWM > support") in their BSP kernel tree. It has been extensively rewritten. > > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Just some small thing by using regmap_set(clear)_bits to reduce a args. But it is not big deal. so Reviewed-by: Frank Li <Frank.Li@nxp.com> > --- > Changes since v4: > > - Use the regmap bulk API > > Changes since v2: > > - Add missing headers > - Sort headers > > Changes since v1: > > - Drop mutex > - Restore R3 pinconfig to known value > - Simplify error check in pwm_adp5585_request() > - Don't fake PWM_POLARITY_INVERSED > - Fix rounding of period and duty cycle > - Drop OF match table > - Drop empty .remove() handler > - Allocate pwm_chip dynamically > - Document limitations > - Add platform ID table > - Set struct device of_node manually > - Merge child DT node into parent node > > Changes compared to the NXP original version > > - Add MAINTAINERS entry > - Drop pwm_ops.owner > - Fix compilation > - Add prefix to compatible string > - Switch to regmap > - Use devm_pwmchip_add() > - Cleanup header includes > - White space fixes > - Drop ADP5585_REG_MASK > - Fix register field names > - Use mutex scope guards > - Clear OSC_EN when freeing PWM > - Reorder functions > - Clear PWM_IN_AND and PWM_MODE bits > - Support inverted polarity > - Clean up on/off computations > - Fix duty cycle computation in .get_state() > - Destroy mutex on remove > - Update copyright > - Update license to GPL-2.0-only > --- > MAINTAINERS | 1 + > drivers/pwm/Kconfig | 7 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-adp5585.c | 189 ++++++++++++++++++++++++++++++++++++++ > 4 files changed, 198 insertions(+) > create mode 100644 drivers/pwm/pwm-adp5585.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index b748af2acf9f..a2087f6647e8 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -534,6 +534,7 @@ S: Maintained > F: Documentation/devicetree/bindings/*/adi,adp5585*.yaml > F: drivers/gpio/gpio-adp5585.c > F: drivers/mfd/adp5585.c > +F: drivers/pwm/pwm-adp5585.c > F: include/linux/mfd/adp5585.h > > ADP5588 QWERTY KEYPAD AND IO EXPANDER DRIVER (ADP5588/ADP5587) > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 1dd7921194f5..b778ecee3e9b 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -47,6 +47,13 @@ config PWM_AB8500 > To compile this driver as a module, choose M here: the module > will be called pwm-ab8500. > > +config PWM_ADP5585 > + tristate "ADP5585 PWM support" > + depends on MFD_ADP5585 > + help > + This option enables support for the PWM function found in the Analog > + Devices ADP5585. > + > config PWM_APPLE > tristate "Apple SoC PWM support" > depends on ARCH_APPLE || COMPILE_TEST > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 90913519f11a..f24d518d20f2 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -1,6 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0 > obj-$(CONFIG_PWM) += core.o > obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o > +obj-$(CONFIG_PWM_ADP5585) += pwm-adp5585.o > obj-$(CONFIG_PWM_APPLE) += pwm-apple.o > obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o > obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM) += pwm-atmel-hlcdc.o > diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c > new file mode 100644 > index 000000000000..472a4c20b7a9 > --- /dev/null > +++ b/drivers/pwm/pwm-adp5585.c > @@ -0,0 +1,189 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Analog Devices ADP5585 PWM driver > + * > + * Copyright 2022 NXP > + * Copyright 2024 Ideas on Board Oy > + * > + * Limitations: > + * - The .apply() operation executes atomically, but may not wait for the > + * period to complete (this is not documented and would need to be tested). > + * - Disabling the PWM drives the output pin to a low level immediately. > + * - The hardware can only generate normal polarity output. > + */ > + > +#include <asm/byteorder.h> > + > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/math64.h> > +#include <linux/mfd/adp5585.h> > +#include <linux/minmax.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/regmap.h> > +#include <linux/time.h> > +#include <linux/types.h> > + > +#define ADP5585_PWM_CHAN_NUM 1 > + > +#define ADP5585_PWM_OSC_FREQ_HZ 1000000U > +#define ADP5585_PWM_MIN_PERIOD_NS (2ULL * NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) > +#define ADP5585_PWM_MAX_PERIOD_NS (2ULL * 0xffff * NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) > + > +static int pwm_adp5585_request(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct regmap *regmap = pwmchip_get_drvdata(chip); > + int ret; > + > + ret = regmap_update_bits(regmap, ADP5585_PIN_CONFIG_C, > + ADP5585_R3_EXTEND_CFG_MASK, > + ADP5585_R3_EXTEND_CFG_PWM_OUT); > + if (ret) > + return ret; > + > + return regmap_update_bits(regmap, ADP5585_GENERAL_CFG, > + ADP5585_OSC_EN, ADP5585_OSC_EN); regmap_set_bits() > +} > + > +static void pwm_adp5585_free(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct regmap *regmap = pwmchip_get_drvdata(chip); > + > + regmap_update_bits(regmap, ADP5585_PIN_CONFIG_C, > + ADP5585_R3_EXTEND_CFG_MASK, > + ADP5585_R3_EXTEND_CFG_GPIO4); > + regmap_update_bits(regmap, ADP5585_GENERAL_CFG, > + ADP5585_OSC_EN, 0); regmap_clear_bits() > +} > + > +static int pwm_adp5585_apply(struct pwm_chip *chip, > + struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct regmap *regmap = pwmchip_get_drvdata(chip); > + u64 period, duty_cycle; > + u32 on, off; > + __le16 val; > + int ret; > + > + if (!state->enabled) > + return regmap_update_bits(regmap, ADP5585_PWM_CFG, > + ADP5585_PWM_EN, 0); > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -EINVAL; > + > + if (state->period < ADP5585_PWM_MIN_PERIOD_NS) > + return -EINVAL; > + > + period = min(state->period, ADP5585_PWM_MAX_PERIOD_NS); > + duty_cycle = min(state->duty_cycle, period); > + > + /* > + * Compute the on and off time. As the internal oscillator frequency is > + * 1MHz, the calculation can be simplified without loss of precision. > + */ > + on = div_u64(duty_cycle, NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ); > + off = div_u64(period, NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) - on; > + > + val = cpu_to_le16(off); > + ret = regmap_bulk_write(regmap, ADP5585_PWM_OFFT_LOW, &val, 2); > + if (ret) > + return ret; > + > + val = cpu_to_le16(on); > + ret = regmap_bulk_write(regmap, ADP5585_PWM_ONT_LOW, &val, 2); > + if (ret) > + return ret; > + > + /* Enable PWM in continuous mode and no external AND'ing. */ > + ret = regmap_update_bits(regmap, ADP5585_PWM_CFG, > + ADP5585_PWM_IN_AND | ADP5585_PWM_MODE | > + ADP5585_PWM_EN, ADP5585_PWM_EN); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static int pwm_adp5585_get_state(struct pwm_chip *chip, > + struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct regmap *regmap = pwmchip_get_drvdata(chip); > + unsigned int on, off; > + unsigned int val; > + __le16 on_off; > + int ret; > + > + ret = regmap_bulk_read(regmap, ADP5585_PWM_OFFT_LOW, &on_off, 2); > + if (ret) > + return ret; > + off = le16_to_cpu(on_off); > + > + ret = regmap_bulk_read(regmap, ADP5585_PWM_ONT_LOW, &on_off, 2); > + if (ret) > + return ret; > + on = le16_to_cpu(on_off); > + > + state->duty_cycle = on * (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ); > + state->period = (on + off) * (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ); > + > + state->polarity = PWM_POLARITY_NORMAL; > + > + regmap_read(regmap, ADP5585_PWM_CFG, &val); > + state->enabled = !!(val & ADP5585_PWM_EN); > + > + return 0; > +} > + > +static const struct pwm_ops adp5585_pwm_ops = { > + .request = pwm_adp5585_request, > + .free = pwm_adp5585_free, > + .apply = pwm_adp5585_apply, > + .get_state = pwm_adp5585_get_state, > +}; > + > +static int adp5585_pwm_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct adp5585_dev *adp5585 = dev_get_drvdata(dev->parent); > + struct pwm_chip *chip; > + int ret; > + > + chip = devm_pwmchip_alloc(dev, ADP5585_PWM_CHAN_NUM, 0); > + if (IS_ERR(chip)) > + return PTR_ERR(chip); > + > + device_set_of_node_from_dev(dev, dev->parent); > + > + pwmchip_set_drvdata(chip, adp5585->regmap); > + chip->ops = &adp5585_pwm_ops; > + > + ret = devm_pwmchip_add(dev, chip); > + if (ret) > + return dev_err_probe(dev, ret, "failed to add PWM chip\n"); > + > + return 0; > +} > + > +static const struct platform_device_id adp5585_pwm_id_table[] = { > + { "adp5585-pwm" }, > + { /* Sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(platform, adp5585_pwm_id_table); > + > +static struct platform_driver adp5585_pwm_driver = { > + .driver = { > + .name = "adp5585-pwm", > + }, > + .probe = adp5585_pwm_probe, > + .id_table = adp5585_pwm_id_table, > +}; > +module_platform_driver(adp5585_pwm_driver); > + > +MODULE_AUTHOR("Xiaoning Wang <xiaoning.wang@nxp.com>"); > +MODULE_DESCRIPTION("ADP5585 PWM Driver"); > +MODULE_LICENSE("GPL"); > -- > Regards, > > Laurent Pinchart >
Hello Laurent, thanks for your reiteration of the series. Just a few questions and minor suggestions left; see below. On Fri, Jul 19, 2024 at 11:39:46PM +0300, Laurent Pinchart wrote: > From: Clark Wang <xiaoning.wang@nxp.com> > > The ADP5585 is a 10/11 input/output port expander with a built in keypad > matrix decoder, programmable logic, reset generator, and PWM generator. > This driver supports the PWM function using the platform device > registered by the core MFD driver. > > The driver is derived from an initial implementation from NXP, available > in commit 113113742208 ("MLK-25922-1 pwm: adp5585: add adp5585 PWM > support") in their BSP kernel tree. It has been extensively rewritten. > > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Would your changes justify a Co-developed-by:? > diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c > new file mode 100644 > index 000000000000..472a4c20b7a9 > --- /dev/null > +++ b/drivers/pwm/pwm-adp5585.c > @@ -0,0 +1,189 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Analog Devices ADP5585 PWM driver > + * > + * Copyright 2022 NXP > + * Copyright 2024 Ideas on Board Oy > + * > + * Limitations: > + * - The .apply() operation executes atomically, but may not wait for the > + * period to complete (this is not documented and would need to be tested). So writing to ADP5585_PWM_OFFT and ADP5585_PWM_ONT is shadowed until what happens? > + * - Disabling the PWM drives the output pin to a low level immediately. > + * - The hardware can only generate normal polarity output. > + */ > + > +#include <asm/byteorder.h> > + > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/math64.h> > +#include <linux/mfd/adp5585.h> > +#include <linux/minmax.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/regmap.h> > +#include <linux/time.h> > +#include <linux/types.h> > + > +#define ADP5585_PWM_CHAN_NUM 1 > + > +#define ADP5585_PWM_OSC_FREQ_HZ 1000000U > +#define ADP5585_PWM_MIN_PERIOD_NS (2ULL * NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) > +#define ADP5585_PWM_MAX_PERIOD_NS (2ULL * 0xffff * NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) > + > +static int pwm_adp5585_request(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct regmap *regmap = pwmchip_get_drvdata(chip); > + int ret; > + > + ret = regmap_update_bits(regmap, ADP5585_PIN_CONFIG_C, > + ADP5585_R3_EXTEND_CFG_MASK, > + ADP5585_R3_EXTEND_CFG_PWM_OUT); > + if (ret) > + return ret; > + > + return regmap_update_bits(regmap, ADP5585_GENERAL_CFG, > + ADP5585_OSC_EN, ADP5585_OSC_EN); The purpose of this function is pinmuxing and oscillator enabling, right? Would it make sense to enable the oscillator only in .apply() with .enabled = true to save some power? > +} > + > +static void pwm_adp5585_free(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct regmap *regmap = pwmchip_get_drvdata(chip); > + > + regmap_update_bits(regmap, ADP5585_PIN_CONFIG_C, > + ADP5585_R3_EXTEND_CFG_MASK, > + ADP5585_R3_EXTEND_CFG_GPIO4); > + regmap_update_bits(regmap, ADP5585_GENERAL_CFG, > + ADP5585_OSC_EN, 0); > +} > + > +static int pwm_adp5585_apply(struct pwm_chip *chip, > + struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct regmap *regmap = pwmchip_get_drvdata(chip); > + u64 period, duty_cycle; > + u32 on, off; > + __le16 val; > + int ret; > + > + if (!state->enabled) > + return regmap_update_bits(regmap, ADP5585_PWM_CFG, > + ADP5585_PWM_EN, 0); > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -EINVAL; > + > + if (state->period < ADP5585_PWM_MIN_PERIOD_NS) > + return -EINVAL; > + > + period = min(state->period, ADP5585_PWM_MAX_PERIOD_NS); > + duty_cycle = min(state->duty_cycle, period); > + > + /* > + * Compute the on and off time. As the internal oscillator frequency is > + * 1MHz, the calculation can be simplified without loss of precision. > + */ > + on = div_u64(duty_cycle, NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ); > + off = div_u64(period, NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) - on; > + > + val = cpu_to_le16(off); > + ret = regmap_bulk_write(regmap, ADP5585_PWM_OFFT_LOW, &val, 2); > + if (ret) > + return ret; > + > + val = cpu_to_le16(on); > + ret = regmap_bulk_write(regmap, ADP5585_PWM_ONT_LOW, &val, 2); > + if (ret) > + return ret; > + > + /* Enable PWM in continuous mode and no external AND'ing. */ > + ret = regmap_update_bits(regmap, ADP5585_PWM_CFG, > + ADP5585_PWM_IN_AND | ADP5585_PWM_MODE | > + ADP5585_PWM_EN, ADP5585_PWM_EN); > + if (ret) > + return ret; > + > + return 0; This could be simplified to just: return regmap_update_bits(...); (but some people feel strong here, so just a suggestion) > +} > + > +static int pwm_adp5585_get_state(struct pwm_chip *chip, > + struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct regmap *regmap = pwmchip_get_drvdata(chip); > + unsigned int on, off; > + unsigned int val; > + __le16 on_off; > + int ret; > + > + ret = regmap_bulk_read(regmap, ADP5585_PWM_OFFT_LOW, &on_off, 2); > + if (ret) > + return ret; > + off = le16_to_cpu(on_off); > + > + ret = regmap_bulk_read(regmap, ADP5585_PWM_ONT_LOW, &on_off, 2); > + if (ret) > + return ret; > + on = le16_to_cpu(on_off); > + > + state->duty_cycle = on * (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ); > + state->period = (on + off) * (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ); > + > + state->polarity = PWM_POLARITY_NORMAL; > + > + regmap_read(regmap, ADP5585_PWM_CFG, &val); > + state->enabled = !!(val & ADP5585_PWM_EN); > + > + return 0; > +} > + > +static const struct pwm_ops adp5585_pwm_ops = { > + .request = pwm_adp5585_request, > + .free = pwm_adp5585_free, > + .apply = pwm_adp5585_apply, > + .get_state = pwm_adp5585_get_state, > +}; > + > +static int adp5585_pwm_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct adp5585_dev *adp5585 = dev_get_drvdata(dev->parent); > + struct pwm_chip *chip; > + int ret; > + > + chip = devm_pwmchip_alloc(dev, ADP5585_PWM_CHAN_NUM, 0); ADP5585_PWM_CHAN_NUM is only used once. I would prefer passing a plain 1 here, as this makes the output of $(grep devm_pwmchip_alloc) a bit more useful. > + if (IS_ERR(chip)) > + return PTR_ERR(chip); > + > + device_set_of_node_from_dev(dev, dev->parent); > + > + pwmchip_set_drvdata(chip, adp5585->regmap); > + chip->ops = &adp5585_pwm_ops; > + > + ret = devm_pwmchip_add(dev, chip); > + if (ret) > + return dev_err_probe(dev, ret, "failed to add PWM chip\n"); > + > + return 0; > +} > + > +static const struct platform_device_id adp5585_pwm_id_table[] = { > + { "adp5585-pwm" }, > + { /* Sentinel */ }, The trailing comma should be dropped here. > +}; > +MODULE_DEVICE_TABLE(platform, adp5585_pwm_id_table); > + > +static struct platform_driver adp5585_pwm_driver = { > + .driver = { > + .name = "adp5585-pwm", > + }, > + .probe = adp5585_pwm_probe, > + .id_table = adp5585_pwm_id_table, > +}; > +module_platform_driver(adp5585_pwm_driver); > + > +MODULE_AUTHOR("Xiaoning Wang <xiaoning.wang@nxp.com>"); > +MODULE_DESCRIPTION("ADP5585 PWM Driver"); > +MODULE_LICENSE("GPL"); Thanks Uwe
Hi Uwe, On Sun, Jul 21, 2024 at 09:09:07AM +0200, Uwe Kleine-König wrote: > Hello Laurent, > > thanks for your reiteration of the series. > > Just a few questions and minor suggestions left; see below. > > On Fri, Jul 19, 2024 at 11:39:46PM +0300, Laurent Pinchart wrote: > > From: Clark Wang <xiaoning.wang@nxp.com> > > > > The ADP5585 is a 10/11 input/output port expander with a built in keypad > > matrix decoder, programmable logic, reset generator, and PWM generator. > > This driver supports the PWM function using the platform device > > registered by the core MFD driver. > > > > The driver is derived from an initial implementation from NXP, available > > in commit 113113742208 ("MLK-25922-1 pwm: adp5585: add adp5585 PWM > > support") in their BSP kernel tree. It has been extensively rewritten. > > > > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Would your changes justify a Co-developed-by:? Sounds like a good idea. > > diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c > > new file mode 100644 > > index 000000000000..472a4c20b7a9 > > --- /dev/null > > +++ b/drivers/pwm/pwm-adp5585.c > > @@ -0,0 +1,189 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Analog Devices ADP5585 PWM driver > > + * > > + * Copyright 2022 NXP > > + * Copyright 2024 Ideas on Board Oy > > + * > > + * Limitations: > > + * - The .apply() operation executes atomically, but may not wait for the > > + * period to complete (this is not documented and would need to be tested). > > So writing to ADP5585_PWM_OFFT and ADP5585_PWM_ONT is shadowed until > what happens? The datasheet only tells that the PWM times are latched when the ADP5585_PWM_ONT_HIGH register is written. Whether that will affect the timings immediately, or wait until the end of the current period, I don't know. > > + * - Disabling the PWM drives the output pin to a low level immediately. > > + * - The hardware can only generate normal polarity output. > > + */ > > + > > +#include <asm/byteorder.h> > > + > > +#include <linux/device.h> > > +#include <linux/err.h> > > +#include <linux/math64.h> > > +#include <linux/mfd/adp5585.h> > > +#include <linux/minmax.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/pwm.h> > > +#include <linux/regmap.h> > > +#include <linux/time.h> > > +#include <linux/types.h> > > + > > +#define ADP5585_PWM_CHAN_NUM 1 > > + > > +#define ADP5585_PWM_OSC_FREQ_HZ 1000000U > > +#define ADP5585_PWM_MIN_PERIOD_NS (2ULL * NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) > > +#define ADP5585_PWM_MAX_PERIOD_NS (2ULL * 0xffff * NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) > > + > > +static int pwm_adp5585_request(struct pwm_chip *chip, struct pwm_device *pwm) > > +{ > > + struct regmap *regmap = pwmchip_get_drvdata(chip); > > + int ret; > > + > > + ret = regmap_update_bits(regmap, ADP5585_PIN_CONFIG_C, > > + ADP5585_R3_EXTEND_CFG_MASK, > > + ADP5585_R3_EXTEND_CFG_PWM_OUT); > > + if (ret) > > + return ret; > > + > > + return regmap_update_bits(regmap, ADP5585_GENERAL_CFG, > > + ADP5585_OSC_EN, ADP5585_OSC_EN); > > The purpose of this function is pinmuxing and oscillator enabling, > right? Would it make sense to enable the oscillator only in .apply() with > .enabled = true to save some power? I'll do that. Note that the OSC_EN bit also affects the GPI scan and the key scan functions, which the driver doesn't support yet. When support for those functions will be added, we will need to move handling of the OSC_EN bit to the MFD driver, and reference-count the oscillator enable/disable calls. > > +} > > + > > +static void pwm_adp5585_free(struct pwm_chip *chip, struct pwm_device *pwm) > > +{ > > + struct regmap *regmap = pwmchip_get_drvdata(chip); > > + > > + regmap_update_bits(regmap, ADP5585_PIN_CONFIG_C, > > + ADP5585_R3_EXTEND_CFG_MASK, > > + ADP5585_R3_EXTEND_CFG_GPIO4); > > + regmap_update_bits(regmap, ADP5585_GENERAL_CFG, > > + ADP5585_OSC_EN, 0); > > +} > > + > > +static int pwm_adp5585_apply(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + const struct pwm_state *state) > > +{ > > + struct regmap *regmap = pwmchip_get_drvdata(chip); > > + u64 period, duty_cycle; > > + u32 on, off; > > + __le16 val; > > + int ret; > > + > > + if (!state->enabled) > > + return regmap_update_bits(regmap, ADP5585_PWM_CFG, > > + ADP5585_PWM_EN, 0); > > + > > + if (state->polarity != PWM_POLARITY_NORMAL) > > + return -EINVAL; > > + > > + if (state->period < ADP5585_PWM_MIN_PERIOD_NS) > > + return -EINVAL; > > + > > + period = min(state->period, ADP5585_PWM_MAX_PERIOD_NS); > > + duty_cycle = min(state->duty_cycle, period); > > + > > + /* > > + * Compute the on and off time. As the internal oscillator frequency is > > + * 1MHz, the calculation can be simplified without loss of precision. > > + */ > > + on = div_u64(duty_cycle, NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ); > > + off = div_u64(period, NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) - on; > > + > > + val = cpu_to_le16(off); > > + ret = regmap_bulk_write(regmap, ADP5585_PWM_OFFT_LOW, &val, 2); > > + if (ret) > > + return ret; > > + > > + val = cpu_to_le16(on); > > + ret = regmap_bulk_write(regmap, ADP5585_PWM_ONT_LOW, &val, 2); > > + if (ret) > > + return ret; > > + > > + /* Enable PWM in continuous mode and no external AND'ing. */ > > + ret = regmap_update_bits(regmap, ADP5585_PWM_CFG, > > + ADP5585_PWM_IN_AND | ADP5585_PWM_MODE | > > + ADP5585_PWM_EN, ADP5585_PWM_EN); > > + if (ret) > > + return ret; > > + > > + return 0; > > This could be simplified to just: > > return regmap_update_bits(...); > > (but some people feel strong here, so just a suggestion) I don't have a strong preference in this case so I'll apply your suggestion. > > +} > > + > > +static int pwm_adp5585_get_state(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + struct pwm_state *state) > > +{ > > + struct regmap *regmap = pwmchip_get_drvdata(chip); > > + unsigned int on, off; > > + unsigned int val; > > + __le16 on_off; > > + int ret; > > + > > + ret = regmap_bulk_read(regmap, ADP5585_PWM_OFFT_LOW, &on_off, 2); > > + if (ret) > > + return ret; > > + off = le16_to_cpu(on_off); > > + > > + ret = regmap_bulk_read(regmap, ADP5585_PWM_ONT_LOW, &on_off, 2); > > + if (ret) > > + return ret; > > + on = le16_to_cpu(on_off); > > + > > + state->duty_cycle = on * (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ); > > + state->period = (on + off) * (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ); > > + > > + state->polarity = PWM_POLARITY_NORMAL; > > + > > + regmap_read(regmap, ADP5585_PWM_CFG, &val); > > + state->enabled = !!(val & ADP5585_PWM_EN); > > + > > + return 0; > > +} > > + > > +static const struct pwm_ops adp5585_pwm_ops = { > > + .request = pwm_adp5585_request, > > + .free = pwm_adp5585_free, > > + .apply = pwm_adp5585_apply, > > + .get_state = pwm_adp5585_get_state, > > +}; > > + > > +static int adp5585_pwm_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct adp5585_dev *adp5585 = dev_get_drvdata(dev->parent); > > + struct pwm_chip *chip; > > + int ret; > > + > > + chip = devm_pwmchip_alloc(dev, ADP5585_PWM_CHAN_NUM, 0); > > ADP5585_PWM_CHAN_NUM is only used once. I would prefer passing a plain 1 > here, as this makes the output of $(grep devm_pwmchip_alloc) a bit more > useful. I think the macro makes the code more readable, it clearly shows that the second argument is the number of channels, while chip = devm_pwmchip_alloc(dev, 1, 0); is harder to read. If you insist, I can change it. I don't have a sentimental attachment to this driver, I just want to get it upstream to avoid carrying it locally. > > + if (IS_ERR(chip)) > > + return PTR_ERR(chip); > > + > > + device_set_of_node_from_dev(dev, dev->parent); > > + > > + pwmchip_set_drvdata(chip, adp5585->regmap); > > + chip->ops = &adp5585_pwm_ops; > > + > > + ret = devm_pwmchip_add(dev, chip); > > + if (ret) > > + return dev_err_probe(dev, ret, "failed to add PWM chip\n"); > > + > > + return 0; > > +} > > + > > +static const struct platform_device_id adp5585_pwm_id_table[] = { > > + { "adp5585-pwm" }, > > + { /* Sentinel */ }, > > The trailing comma should be dropped here. OK. > > +}; > > +MODULE_DEVICE_TABLE(platform, adp5585_pwm_id_table); > > + > > +static struct platform_driver adp5585_pwm_driver = { > > + .driver = { > > + .name = "adp5585-pwm", > > + }, > > + .probe = adp5585_pwm_probe, > > + .id_table = adp5585_pwm_id_table, > > +}; > > +module_platform_driver(adp5585_pwm_driver); > > + > > +MODULE_AUTHOR("Xiaoning Wang <xiaoning.wang@nxp.com>"); > > +MODULE_DESCRIPTION("ADP5585 PWM Driver"); > > +MODULE_LICENSE("GPL");
diff --git a/MAINTAINERS b/MAINTAINERS index b748af2acf9f..a2087f6647e8 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -534,6 +534,7 @@ S: Maintained F: Documentation/devicetree/bindings/*/adi,adp5585*.yaml F: drivers/gpio/gpio-adp5585.c F: drivers/mfd/adp5585.c +F: drivers/pwm/pwm-adp5585.c F: include/linux/mfd/adp5585.h ADP5588 QWERTY KEYPAD AND IO EXPANDER DRIVER (ADP5588/ADP5587) diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 1dd7921194f5..b778ecee3e9b 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -47,6 +47,13 @@ config PWM_AB8500 To compile this driver as a module, choose M here: the module will be called pwm-ab8500. +config PWM_ADP5585 + tristate "ADP5585 PWM support" + depends on MFD_ADP5585 + help + This option enables support for the PWM function found in the Analog + Devices ADP5585. + config PWM_APPLE tristate "Apple SoC PWM support" depends on ARCH_APPLE || COMPILE_TEST diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 90913519f11a..f24d518d20f2 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_PWM) += core.o obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o +obj-$(CONFIG_PWM_ADP5585) += pwm-adp5585.o obj-$(CONFIG_PWM_APPLE) += pwm-apple.o obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM) += pwm-atmel-hlcdc.o diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c new file mode 100644 index 000000000000..472a4c20b7a9 --- /dev/null +++ b/drivers/pwm/pwm-adp5585.c @@ -0,0 +1,189 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Analog Devices ADP5585 PWM driver + * + * Copyright 2022 NXP + * Copyright 2024 Ideas on Board Oy + * + * Limitations: + * - The .apply() operation executes atomically, but may not wait for the + * period to complete (this is not documented and would need to be tested). + * - Disabling the PWM drives the output pin to a low level immediately. + * - The hardware can only generate normal polarity output. + */ + +#include <asm/byteorder.h> + +#include <linux/device.h> +#include <linux/err.h> +#include <linux/math64.h> +#include <linux/mfd/adp5585.h> +#include <linux/minmax.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/regmap.h> +#include <linux/time.h> +#include <linux/types.h> + +#define ADP5585_PWM_CHAN_NUM 1 + +#define ADP5585_PWM_OSC_FREQ_HZ 1000000U +#define ADP5585_PWM_MIN_PERIOD_NS (2ULL * NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) +#define ADP5585_PWM_MAX_PERIOD_NS (2ULL * 0xffff * NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) + +static int pwm_adp5585_request(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct regmap *regmap = pwmchip_get_drvdata(chip); + int ret; + + ret = regmap_update_bits(regmap, ADP5585_PIN_CONFIG_C, + ADP5585_R3_EXTEND_CFG_MASK, + ADP5585_R3_EXTEND_CFG_PWM_OUT); + if (ret) + return ret; + + return regmap_update_bits(regmap, ADP5585_GENERAL_CFG, + ADP5585_OSC_EN, ADP5585_OSC_EN); +} + +static void pwm_adp5585_free(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct regmap *regmap = pwmchip_get_drvdata(chip); + + regmap_update_bits(regmap, ADP5585_PIN_CONFIG_C, + ADP5585_R3_EXTEND_CFG_MASK, + ADP5585_R3_EXTEND_CFG_GPIO4); + regmap_update_bits(regmap, ADP5585_GENERAL_CFG, + ADP5585_OSC_EN, 0); +} + +static int pwm_adp5585_apply(struct pwm_chip *chip, + struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct regmap *regmap = pwmchip_get_drvdata(chip); + u64 period, duty_cycle; + u32 on, off; + __le16 val; + int ret; + + if (!state->enabled) + return regmap_update_bits(regmap, ADP5585_PWM_CFG, + ADP5585_PWM_EN, 0); + + if (state->polarity != PWM_POLARITY_NORMAL) + return -EINVAL; + + if (state->period < ADP5585_PWM_MIN_PERIOD_NS) + return -EINVAL; + + period = min(state->period, ADP5585_PWM_MAX_PERIOD_NS); + duty_cycle = min(state->duty_cycle, period); + + /* + * Compute the on and off time. As the internal oscillator frequency is + * 1MHz, the calculation can be simplified without loss of precision. + */ + on = div_u64(duty_cycle, NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ); + off = div_u64(period, NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) - on; + + val = cpu_to_le16(off); + ret = regmap_bulk_write(regmap, ADP5585_PWM_OFFT_LOW, &val, 2); + if (ret) + return ret; + + val = cpu_to_le16(on); + ret = regmap_bulk_write(regmap, ADP5585_PWM_ONT_LOW, &val, 2); + if (ret) + return ret; + + /* Enable PWM in continuous mode and no external AND'ing. */ + ret = regmap_update_bits(regmap, ADP5585_PWM_CFG, + ADP5585_PWM_IN_AND | ADP5585_PWM_MODE | + ADP5585_PWM_EN, ADP5585_PWM_EN); + if (ret) + return ret; + + return 0; +} + +static int pwm_adp5585_get_state(struct pwm_chip *chip, + struct pwm_device *pwm, + struct pwm_state *state) +{ + struct regmap *regmap = pwmchip_get_drvdata(chip); + unsigned int on, off; + unsigned int val; + __le16 on_off; + int ret; + + ret = regmap_bulk_read(regmap, ADP5585_PWM_OFFT_LOW, &on_off, 2); + if (ret) + return ret; + off = le16_to_cpu(on_off); + + ret = regmap_bulk_read(regmap, ADP5585_PWM_ONT_LOW, &on_off, 2); + if (ret) + return ret; + on = le16_to_cpu(on_off); + + state->duty_cycle = on * (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ); + state->period = (on + off) * (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ); + + state->polarity = PWM_POLARITY_NORMAL; + + regmap_read(regmap, ADP5585_PWM_CFG, &val); + state->enabled = !!(val & ADP5585_PWM_EN); + + return 0; +} + +static const struct pwm_ops adp5585_pwm_ops = { + .request = pwm_adp5585_request, + .free = pwm_adp5585_free, + .apply = pwm_adp5585_apply, + .get_state = pwm_adp5585_get_state, +}; + +static int adp5585_pwm_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct adp5585_dev *adp5585 = dev_get_drvdata(dev->parent); + struct pwm_chip *chip; + int ret; + + chip = devm_pwmchip_alloc(dev, ADP5585_PWM_CHAN_NUM, 0); + if (IS_ERR(chip)) + return PTR_ERR(chip); + + device_set_of_node_from_dev(dev, dev->parent); + + pwmchip_set_drvdata(chip, adp5585->regmap); + chip->ops = &adp5585_pwm_ops; + + ret = devm_pwmchip_add(dev, chip); + if (ret) + return dev_err_probe(dev, ret, "failed to add PWM chip\n"); + + return 0; +} + +static const struct platform_device_id adp5585_pwm_id_table[] = { + { "adp5585-pwm" }, + { /* Sentinel */ }, +}; +MODULE_DEVICE_TABLE(platform, adp5585_pwm_id_table); + +static struct platform_driver adp5585_pwm_driver = { + .driver = { + .name = "adp5585-pwm", + }, + .probe = adp5585_pwm_probe, + .id_table = adp5585_pwm_id_table, +}; +module_platform_driver(adp5585_pwm_driver); + +MODULE_AUTHOR("Xiaoning Wang <xiaoning.wang@nxp.com>"); +MODULE_DESCRIPTION("ADP5585 PWM Driver"); +MODULE_LICENSE("GPL");