Message ID | 20240608141633.2562-5-laurent.pinchart@ideasonboard.com |
---|---|
State | New |
Headers | show |
Series | ADP5585 GPIO expander, PWM and keypad controller support | expand |
Sat, Jun 08, 2024 at 05:16:33PM +0300, Laurent Pinchart kirjoitti: > 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. ... > +#define ADP5585_PWM_OSC_FREQ_HZ 1000000U (1 * HZ_PER_MHZ) ? Variant to use MEGA. Or even #define MHz in units.h if you wish. Putting a few 0:s in a row is error prone. ... > + ret = regmap_write(regmap, ADP5585_PWM_OFFT_LOW, > + off & 0xff); > + if (ret) > + return ret; > + ret = regmap_write(regmap, ADP5585_PWM_OFFT_HIGH, > + (off >> 8) & 0xff); > + if (ret) > + return ret; This is regular I²C regmap, why do you avoid using regmap bulk APIs? > + ret = regmap_write(regmap, ADP5585_PWM_ONT_LOW, > + on & 0xff); > + if (ret) > + return ret; > + ret = regmap_write(regmap, ADP5585_PWM_ONT_HIGH, > + (on >> 8) & 0xff); > + if (ret) > + return ret; Ditto. ... > +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; > + > + regmap_read(regmap, ADP5585_PWM_OFFT_LOW, &off); > + regmap_read(regmap, ADP5585_PWM_OFFT_HIGH, &val); > + off |= val << 8; Ditto. > + regmap_read(regmap, ADP5585_PWM_ONT_LOW, &on); > + regmap_read(regmap, ADP5585_PWM_ONT_HIGH, &val); > + on |= val << 8; Ditto. > + 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; Besides that, how do you guarantee that no IO may happen in between of those calls? Probably you want a driver explicit lock? In that case, would you still want to have a regmap internal lock? > +} ... > +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); Still unclear to me why only few drivers use this. > + 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 */ }, Drop comma. Otherwise it's not a sentinel strictly speaking. > +};
On Mon, Jun 10, 2024 at 06:06:51PM +0300, Andy Shevchenko wrote: > Sat, Jun 08, 2024 at 05:16:33PM +0300, Laurent Pinchart kirjoitti: > > 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. > > ... > > > +#define ADP5585_PWM_OSC_FREQ_HZ 1000000U > > (1 * HZ_PER_MHZ) ? > > Variant to use MEGA. Or even #define MHz in units.h if you wish. > Putting a few 0:s in a row is error prone. Feel free to send follow-up patches. Andy, we're reaching a level of nitpicking and yakshaving that even I can't deal with. I will have to simply ignore the comments I disagree with. > ... > > > + ret = regmap_write(regmap, ADP5585_PWM_OFFT_LOW, > > + off & 0xff); > > + if (ret) > > + return ret; > > + ret = regmap_write(regmap, ADP5585_PWM_OFFT_HIGH, > > + (off >> 8) & 0xff); > > + if (ret) > > + return ret; > > This is regular I²C regmap, why do you avoid using regmap bulk APIs? > > > + ret = regmap_write(regmap, ADP5585_PWM_ONT_LOW, > > + on & 0xff); > > + if (ret) > > + return ret; > > + ret = regmap_write(regmap, ADP5585_PWM_ONT_HIGH, > > + (on >> 8) & 0xff); > > + if (ret) > > + return ret; > > Ditto. > > ... > > > +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; > > + > > + regmap_read(regmap, ADP5585_PWM_OFFT_LOW, &off); > > + regmap_read(regmap, ADP5585_PWM_OFFT_HIGH, &val); > > + off |= val << 8; > > Ditto. > > > + regmap_read(regmap, ADP5585_PWM_ONT_LOW, &on); > > + regmap_read(regmap, ADP5585_PWM_ONT_HIGH, &val); > > + on |= val << 8; > > Ditto. > > > + 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; > > Besides that, how do you guarantee that no IO may happen in between of those > calls? Probably you want a driver explicit lock? In that case, would you still > want to have a regmap internal lock? > > > +} > > ... > > > +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); > > Still unclear to me why only few drivers use this. > > > + 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 */ }, > > Drop comma. Otherwise it's not a sentinel strictly speaking. > > > +};
On Mon, Jun 10, 2024 at 6:28 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Mon, Jun 10, 2024 at 06:06:51PM +0300, Andy Shevchenko wrote: > > Sat, Jun 08, 2024 at 05:16:33PM +0300, Laurent Pinchart kirjoitti: ... > Andy, we're reaching a level of nitpicking and yakshaving that even I > can't deal with. I will have to simply ignore the comments I disagree > with. Do you think using bulk APIs is nit-picking?
diff --git a/MAINTAINERS b/MAINTAINERS index 80f3544d07aa..b15bf6e2485c 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..7f226c287efe --- /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 <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; + 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; + + ret = regmap_write(regmap, ADP5585_PWM_OFFT_LOW, + off & 0xff); + if (ret) + return ret; + ret = regmap_write(regmap, ADP5585_PWM_OFFT_HIGH, + (off >> 8) & 0xff); + if (ret) + return ret; + ret = regmap_write(regmap, ADP5585_PWM_ONT_LOW, + on & 0xff); + if (ret) + return ret; + ret = regmap_write(regmap, ADP5585_PWM_ONT_HIGH, + (on >> 8) & 0xff); + 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; + + regmap_read(regmap, ADP5585_PWM_OFFT_LOW, &off); + regmap_read(regmap, ADP5585_PWM_OFFT_HIGH, &val); + off |= val << 8; + + regmap_read(regmap, ADP5585_PWM_ONT_LOW, &on); + regmap_read(regmap, ADP5585_PWM_ONT_HIGH, &val); + on |= val << 8; + + 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");