| Message ID | 20240608141633.2562-5-laurent.pinchart@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series | ADP5585 GPIO expander, PWM and keypad controller support | expand |
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?
Hi Andy, (CC'ing Mark) On Mon, Jun 10, 2024 at 07:31:11PM +0300, Andy Shevchenko wrote: > On Mon, Jun 10, 2024 at 6:28 PM Laurent Pinchart 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? In this case I do. If we were dealing with more 16-bit registers in this driver I would agree with you. This being said, I'd like to get this driver merged, and I'll burn some of the mental energy I've recovered thanks to the last two weeks of holidays and submit a v5 using the bulk API. It's getting mentally exhausting though. Overall, I think it would be nice to improve support for variable-length register maps, in a similar way as done in include/media/v4l2-cci.h. This driver, as well as many other drivers, could really benefit from it. Mark, do you have an opinion, is v4l2-cci something that we could fold in regmap itself ?
On Fri, Jul 19, 2024 at 4:07 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Andy, > > (CC'ing Mark) > > On Mon, Jun 10, 2024 at 07:31:11PM +0300, Andy Shevchenko wrote: > > On Mon, Jun 10, 2024 at 6:28 PM Laurent Pinchart 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? > > In this case I do. If we were dealing with more 16-bit registers in this > driver I would agree with you. This being said, I'd like to get this > driver merged, and I'll burn some of the mental energy I've recovered > thanks to the last two weeks of holidays and submit a v5 using the bulk > API. It's getting mentally exhausting though. OK. > Overall, I think it would be nice to improve support for variable-length > register maps, in a similar way as done in include/media/v4l2-cci.h. > This driver, as well as many other drivers, could really benefit from > it. Mark, do you have an opinion, is v4l2-cci something that we could > fold in regmap itself ? +Cc: Hans as he might have even considered this (I'm speculating, but considering his quite a wide involvement in v4l2 sensor drivers and drivers that use regmap this idea might have come).
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");