[5/5] pwm: adp5585: Add Analog Devices ADP5585 support

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>
---
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 | 230 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 239 insertions(+)
 create mode 100644 drivers/pwm/pwm-adp5585.c

Hello Laurent,

On Mon, May 20, 2024 at 10:59:41PM +0300, Laurent Pinchart wrote:
> diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c
> new file mode 100644
> index 000000000000..709713d8f47a
> --- /dev/null
> +++ b/drivers/pwm/pwm-adp5585.c
> @@ -0,0 +1,230 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices ADP5585 PWM driver
> + *
> + * Copyright 2022 NXP
> + * Copyright 2024 Ideas on Board Oy
> + */

Please document some hardware properties here in the same format as many
other PWM drivers. The things I'd like to read there are:

 - Only supports normal polarity
 - How does the output pin behave when the hardware is disabled
   (typically "low" or "high-Z" or "freeze")
 - Does changing parameters or disabling complete the currently running
   period?
 - Are there glitches in .apply()? E.g. when the new duty_cycle is
   already written but the new period is not.

> +#include <linux/container_of.h>
> +#include <linux/device.h>
> +#include <linux/math.h>
> +#include <linux/minmax.h>
> +#include <linux/mfd/adp5585.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/time.h>

Do you need these all? I wounder about time.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)
> +
> +struct adp5585_pwm_chip {
> +	struct pwm_chip chip;
> +	struct regmap *regmap;
> +	struct mutex lock;

What does this mutex protect against? You can safely assume that there
are no concurrent calls of the callbacks. (This isn't ensured yet, but I
consider a consumer who does this buggy and it will soon be ensured.)

> +	u8 pin_config_val;
> +};
> +
> +static inline struct adp5585_pwm_chip *
> +to_adp5585_pwm_chip(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct adp5585_pwm_chip, chip);
> +}
> +
> +static int pwm_adp5585_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> +	unsigned int val;
> +	int ret;
> +
> +	guard(mutex)(&adp5585_pwm->lock);
> +
> +	ret = regmap_read(adp5585_pwm->regmap, ADP5585_PIN_CONFIG_C, &val);
> +	if (ret)
> +		return ret;
> +
> +	adp5585_pwm->pin_config_val = val;
> +
> +	ret = regmap_update_bits(adp5585_pwm->regmap, ADP5585_PIN_CONFIG_C,
> +				 ADP5585_R3_EXTEND_CFG_MASK,
> +				 ADP5585_R3_EXTEND_CFG_PWM_OUT);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(adp5585_pwm->regmap, ADP5585_GENERAL_CFG,
> +				 ADP5585_OSC_EN, ADP5585_OSC_EN);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

The last four lines are equivalent to

	return ret;

What is the purpose of this function? Setup some kind of pinmuxing? The
answer to that question goes into a code comment. If it's pinmuxing, is
this a hint to use the pinctrl subsystem? (Maybe it's overkill, but if
it's considered a good idea later, it might be hard to extend the dt
bindings, so thinking about that now might be a good idea.)

> +}
> +
> +static void pwm_adp5585_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> +
> +	guard(mutex)(&adp5585_pwm->lock);
> +
> +	regmap_update_bits(adp5585_pwm->regmap, ADP5585_PIN_CONFIG_C,
> +			   ADP5585_R3_EXTEND_CFG_MASK,
> +			   adp5585_pwm->pin_config_val);

I wonder if writing a deterministic value instead of whatever was in
that register before .request() would be more robust and less
surprising.

> +	regmap_update_bits(adp5585_pwm->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 adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> +	u32 on, off;
> +	int ret;
> +
> +	if (!state->enabled) {
> +		guard(mutex)(&adp5585_pwm->lock);
> +
> +		return regmap_update_bits(adp5585_pwm->regmap, ADP5585_PWM_CFG,
> +					  ADP5585_PWM_EN, 0);
> +	}
> +
> +	if (state->period < ADP5585_PWM_MIN_PERIOD_NS ||
> +	    state->period > ADP5585_PWM_MAX_PERIOD_NS)
> +		return -EINVAL;

Make this:

	if (state->period < ADP5585_PWM_MIN_PERIOD_NS)
		return -EINVAL;

	period = min(ADP5585_PWM_MAX_PERIOD_NS, state->period)
	duty_cycle = min(period, state->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_ROUND_CLOSEST_ULL(state->duty_cycle,
> +				   NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ);
> +	off = DIV_ROUND_CLOSEST_ULL(state->period - state->duty_cycle,
> +				    NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ);

round-closest is wrong. Testing with PWM_DEBUG should point that out.
The right algorithm is:

	on = duty_cycle / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ)
	off = period / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) - on


> +	if (state->polarity == PWM_POLARITY_INVERSED)
> +		swap(on, off);

Uhh, no. Either you can do inverted polarity or you cannot. Don't claim
you can.

> [...]
> +static int adp5585_pwm_probe(struct platform_device *pdev)
> +{
> +	struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
> +	struct adp5585_pwm_chip *adp5585_pwm;
> +	int ret;
> +
> +	adp5585_pwm = devm_kzalloc(&pdev->dev, sizeof(*adp5585_pwm), GFP_KERNEL);
> +	if (!adp5585_pwm)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, adp5585_pwm);
> +
> +	adp5585_pwm->regmap = adp5585->regmap;
> +
> +	mutex_init(&adp5585_pwm->lock);
> +
> +	adp5585_pwm->chip.dev = &pdev->dev;
> +	adp5585_pwm->chip.ops = &adp5585_pwm_ops;
> +	adp5585_pwm->chip.npwm = ADP5585_PWM_CHAN_NUM;

That is wrong since commit
05947224ff46 ("pwm: Ensure that pwm_chips are allocated using pwmchip_alloc()")

> +	ret = devm_pwmchip_add(&pdev->dev, &adp5585_pwm->chip);
> +	if (ret) {
> +		mutex_destroy(&adp5585_pwm->lock);
> +		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static void adp5585_pwm_remove(struct platform_device *pdev)
> +{
> +	struct adp5585_pwm_chip *adp5585_pwm = platform_get_drvdata(pdev);
> +
> +	mutex_destroy(&adp5585_pwm->lock);

Huh, this is a bad idea. The mutex is gone while the pwmchip is still
registered. AFAIK calling mutex_destroy() is optional, and
adp5585_pwm_remove() can just be dropped. Ditto in the error paths of
.probe().

> +}
> +
> +static const struct of_device_id adp5585_pwm_of_match[] = {
> +	{ .compatible = "adi,adp5585-pwm" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, adp5585_pwm_of_match);

Is it normal/usual for mfd drivers to use of stuff? I thought they use
plain platform style binding, not sure though.

> +static struct platform_driver adp5585_pwm_driver = {
> +	.driver	= {
> +		.name = "adp5585-pwm",
> +		.of_match_table = adp5585_pwm_of_match,
> +	},
> +	.probe = adp5585_pwm_probe,
> +	.remove_new = 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");

Best regards
Uwe

Hi Uwe,

Thank you for the quick review.

On Tue, May 21, 2024 at 10:51:26AM +0200, Uwe Kleine-König wrote:
> On Mon, May 20, 2024 at 10:59:41PM +0300, Laurent Pinchart wrote:
> > diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c
> > new file mode 100644
> > index 000000000000..709713d8f47a
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-adp5585.c
> > @@ -0,0 +1,230 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Analog Devices ADP5585 PWM driver
> > + *
> > + * Copyright 2022 NXP
> > + * Copyright 2024 Ideas on Board Oy
> > + */
> 
> Please document some hardware properties here in the same format as many
> other PWM drivers. The things I'd like to read there are:
> 
>  - Only supports normal polarity
>  - How does the output pin behave when the hardware is disabled
>    (typically "low" or "high-Z" or "freeze")
>  - Does changing parameters or disabling complete the currently running
>    period?
>  - Are there glitches in .apply()? E.g. when the new duty_cycle is
>    already written but the new period is not.
> 
> > +#include <linux/container_of.h>
> > +#include <linux/device.h>
> > +#include <linux/math.h>
> > +#include <linux/minmax.h>
> > +#include <linux/mfd/adp5585.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +#include <linux/time.h>
> 
> Do you need these all? I wounder about time.h.

Yes I've checked them all :-) time.h is for NSEC_PER_SEC (defined in
vdso/time64.h, which I thought would be better replaced by time.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)
> > +
> > +struct adp5585_pwm_chip {
> > +	struct pwm_chip chip;
> > +	struct regmap *regmap;
> > +	struct mutex lock;
> 
> What does this mutex protect against? You can safely assume that there
> are no concurrent calls of the callbacks. (This isn't ensured yet, but I
> consider a consumer who does this buggy and it will soon be ensured.)

That's good to know. I couldn't find that information. I'll revisit the
locking in v2, and add a comment to document the mutex in case it's
still needed.

> > +	u8 pin_config_val;
> > +};
> > +
> > +static inline struct adp5585_pwm_chip *
> > +to_adp5585_pwm_chip(struct pwm_chip *chip)
> > +{
> > +	return container_of(chip, struct adp5585_pwm_chip, chip);
> > +}
> > +
> > +static int pwm_adp5585_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	guard(mutex)(&adp5585_pwm->lock);
> > +
> > +	ret = regmap_read(adp5585_pwm->regmap, ADP5585_PIN_CONFIG_C, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	adp5585_pwm->pin_config_val = val;
> > +
> > +	ret = regmap_update_bits(adp5585_pwm->regmap, ADP5585_PIN_CONFIG_C,
> > +				 ADP5585_R3_EXTEND_CFG_MASK,
> > +				 ADP5585_R3_EXTEND_CFG_PWM_OUT);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(adp5585_pwm->regmap, ADP5585_GENERAL_CFG,
> > +				 ADP5585_OSC_EN, ADP5585_OSC_EN);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> 
> The last four lines are equivalent to
> 
> 	return ret;

I prefer the existing code but can also change it.

> What is the purpose of this function? Setup some kind of pinmuxing? The
> answer to that question goes into a code comment. If it's pinmuxing, is
> this a hint to use the pinctrl subsystem? (Maybe it's overkill, but if
> it's considered a good idea later, it might be hard to extend the dt
> bindings, so thinking about that now might be a good idea.)

The ADP5585_R3_EXTEND_CFG_PWM_OUT bit is about pinmuxing, yes. I'll add
a comment. I considered pinctrl too, but I think it's overkill.

> > +}
> > +
> > +static void pwm_adp5585_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> > +
> > +	guard(mutex)(&adp5585_pwm->lock);
> > +
> > +	regmap_update_bits(adp5585_pwm->regmap, ADP5585_PIN_CONFIG_C,
> > +			   ADP5585_R3_EXTEND_CFG_MASK,
> > +			   adp5585_pwm->pin_config_val);
> 
> I wonder if writing a deterministic value instead of whatever was in
> that register before .request() would be more robust and less
> surprising.

I'll change that. It looks like the last remains of the original code
are going away :-)

> > +	regmap_update_bits(adp5585_pwm->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 adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> > +	u32 on, off;
> > +	int ret;
> > +
> > +	if (!state->enabled) {
> > +		guard(mutex)(&adp5585_pwm->lock);
> > +
> > +		return regmap_update_bits(adp5585_pwm->regmap, ADP5585_PWM_CFG,
> > +					  ADP5585_PWM_EN, 0);
> > +	}
> > +
> > +	if (state->period < ADP5585_PWM_MIN_PERIOD_NS ||
> > +	    state->period > ADP5585_PWM_MAX_PERIOD_NS)
> > +		return -EINVAL;
> 
> Make this:
> 
> 	if (state->period < ADP5585_PWM_MIN_PERIOD_NS)
> 		return -EINVAL;
> 
> 	period = min(ADP5585_PWM_MAX_PERIOD_NS, state->period)
> 	duty_cycle = min(period, state->period);

I haven't been able to find documentation about the expected behaviour.
What's the rationale for returning an error if the period is too low,
but silently clamping it if it's too high ?

> > +
> > +	/*
> > +	 * Compute the on and off time. As the internal oscillator frequency is
> > +	 * 1MHz, the calculation can be simplified without loss of precision.
> > +	 */
> > +	on = DIV_ROUND_CLOSEST_ULL(state->duty_cycle,
> > +				   NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ);
> > +	off = DIV_ROUND_CLOSEST_ULL(state->period - state->duty_cycle,
> > +				    NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ);
> 
> round-closest is wrong. Testing with PWM_DEBUG should point that out.
> The right algorithm is:
> 
> 	on = duty_cycle / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ)
> 	off = period / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) - on
> 
> 
> > +	if (state->polarity == PWM_POLARITY_INVERSED)
> > +		swap(on, off);
> 
> Uhh, no. Either you can do inverted polarity or you cannot. Don't claim
> you can.

OK, but what's the rationale ? This is also an area where I couldn't
find documentation.

> > [...]
> > +static int adp5585_pwm_probe(struct platform_device *pdev)
> > +{
> > +	struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
> > +	struct adp5585_pwm_chip *adp5585_pwm;
> > +	int ret;
> > +
> > +	adp5585_pwm = devm_kzalloc(&pdev->dev, sizeof(*adp5585_pwm), GFP_KERNEL);
> > +	if (!adp5585_pwm)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, adp5585_pwm);
> > +
> > +	adp5585_pwm->regmap = adp5585->regmap;
> > +
> > +	mutex_init(&adp5585_pwm->lock);
> > +
> > +	adp5585_pwm->chip.dev = &pdev->dev;
> > +	adp5585_pwm->chip.ops = &adp5585_pwm_ops;
> > +	adp5585_pwm->chip.npwm = ADP5585_PWM_CHAN_NUM;
> 
> That is wrong since commit
> 05947224ff46 ("pwm: Ensure that pwm_chips are allocated using pwmchip_alloc()")

I'll update the code.

> > +	ret = devm_pwmchip_add(&pdev->dev, &adp5585_pwm->chip);
> > +	if (ret) {
> > +		mutex_destroy(&adp5585_pwm->lock);
> > +		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void adp5585_pwm_remove(struct platform_device *pdev)
> > +{
> > +	struct adp5585_pwm_chip *adp5585_pwm = platform_get_drvdata(pdev);
> > +
> > +	mutex_destroy(&adp5585_pwm->lock);
> 
> Huh, this is a bad idea. The mutex is gone while the pwmchip is still
> registered. AFAIK calling mutex_destroy() is optional, and
> adp5585_pwm_remove() can just be dropped. Ditto in the error paths of
> .probe().

mutex_destroy() is a no-op when !CONFIG_DEBUG_MUTEXES. When the config
option is selected, it gets more useful. I would prefer moving away from
the devm_* registration, and unregister the pwm_chip in .remove()
manually, before destroying the mutex.

> > +}
> > +
> > +static const struct of_device_id adp5585_pwm_of_match[] = {
> > +	{ .compatible = "adi,adp5585-pwm" },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, adp5585_pwm_of_match);
> 
> Is it normal/usual for mfd drivers to use of stuff? I thought they use
> plain platform style binding, not sure though.

I'll test it.

> > +static struct platform_driver adp5585_pwm_driver = {
> > +	.driver	= {
> > +		.name = "adp5585-pwm",
> > +		.of_match_table = adp5585_pwm_of_match,
> > +	},
> > +	.probe = adp5585_pwm_probe,
> > +	.remove_new = 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");

On Tue, May 21, 2024 at 03:05:53PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> [dropping Alexandru Ardelean from Cc as their address bounces]
> 
> On Tue, May 21, 2024 at 01:09:22PM +0300, Laurent Pinchart wrote:
> > On Tue, May 21, 2024 at 10:51:26AM +0200, Uwe Kleine-König wrote:
> > > On Mon, May 20, 2024 at 10:59:41PM +0300, Laurent Pinchart wrote:
> > > > +	ret = regmap_update_bits(adp5585_pwm->regmap, ADP5585_GENERAL_CFG,
> > > > +				 ADP5585_OSC_EN, ADP5585_OSC_EN);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	return 0;
> > > 
> > > The last four lines are equivalent to
> > > 
> > > 	return ret;
> > 
> > I prefer the existing code but can also change it.
> 
> Well, I see the upside of your approach. If this was my only concern I
> wouldn't refuse to apply the patch.

While I have my preferences, I also favour consistency, so I'm happy to
comply with the preferred coding style for the subsystem :-) I'll
update this in the next version.

> > > > +	regmap_update_bits(adp5585_pwm->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 adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> > > > +	u32 on, off;
> > > > +	int ret;
> > > > +
> > > > +	if (!state->enabled) {
> > > > +		guard(mutex)(&adp5585_pwm->lock);
> > > > +
> > > > +		return regmap_update_bits(adp5585_pwm->regmap, ADP5585_PWM_CFG,
> > > > +					  ADP5585_PWM_EN, 0);
> > > > +	}
> > > > +
> > > > +	if (state->period < ADP5585_PWM_MIN_PERIOD_NS ||
> > > > +	    state->period > ADP5585_PWM_MAX_PERIOD_NS)
> > > > +		return -EINVAL;
> > > 
> > > Make this:
> > > 
> > > 	if (state->period < ADP5585_PWM_MIN_PERIOD_NS)
> > > 		return -EINVAL;
> > > 
> > > 	period = min(ADP5585_PWM_MAX_PERIOD_NS, state->period)
> > > 	duty_cycle = min(period, state->period);
> > 
> > I haven't been able to find documentation about the expected behaviour.
> > What's the rationale for returning an error if the period is too low,
> > but silently clamping it if it's too high ?
> 
> Well, it's only implicitly documented in the implementation of
> PWM_DEBUG. The reasoning is a combination of the following thoughts:
> 
>  - Requiring exact matches is hard to work with, so some deviation
>    between request and configured value should be allowed.
>  - Rounding in both directions has strange and surprising effects. The
>    corner cases (for all affected parties (=consumer, lowlevel driver
>    and pwm core)) are easier if you only round in one direction.
>    One ugly corner case in your suggested patch is:
>    ADP5585_PWM_MAX_PERIOD_NS corresponds to 0xffff clock ticks.
>    If the consumer requests period=64000.2 clock ticks, you configure
>    for 64000. If the consumer requests period=65535.2 clock ticks you
>    return -EINVAL.
>    Another strange corner case is: Consider a hardware that can
>    implement the following periods 499.7 ns, 500.2 ns, 500.3 ns and then
>    only values >502 ns.
>    If you configure for 501 ns, you'd get 500.3 ns. get_state() would
>    tell you it's running at 500 ns. If you then configure 500 ns you
>    won't get 500.3 ns any more.
>  - If you want to allow 66535.2 clock ticks (and return 65535), what
>    should be the maximal value that should yield 65535? Each cut-off
>    value is arbitrary, so using \infty looks reasonable (to me at
>    least).
>  - Rounding down is easier than rounding up, because that's what C's /
>    does. (Well, this is admittedly a bit arbitrary, because if you round
>    down in .apply() you have to round up in .get_state().)

Thank you for the detailed explanation.

> > > round-closest is wrong. Testing with PWM_DEBUG should point that out.
> > > The right algorithm is:
> > > 
> > > 	on = duty_cycle / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ)
> > > 	off = period / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) - on
> > > 
> > > 
> > > > +	if (state->polarity == PWM_POLARITY_INVERSED)
> > > > +		swap(on, off);
> > > 
> > > Uhh, no. Either you can do inverted polarity or you cannot. Don't claim
> > > you can.
> > 
> > OK, but what's the rationale ? This is also an area where I couldn't
> > find documentation.
> 
> I don't have a good rationale here. IMHO this inverted polarity stuff is
> only a convenience for consumers because the start of the period isn't
> visible from the output wave form (apart from (maybe) the moment where
> you change the configuration) and so
> 
> 	.period = 5000, duty_cycle = 1000, polarity = PWM_POLARITY_NORMAL
> 
> isn't distinguishable from
> 
> 	.period = 5000, duty_cycle = 4000, polarity = PWM_POLARITY_INVERSED
> 
> . But it's a historic assumption of the pwm core that there is a
> relevant difference between the two polarities and I want at least a
> consistent behaviour among the lowlevel drivers. BTW, this convenience
> is the reason I'm not yet clear how I want to implemement a duty_offset.

Consistency is certainly good. Inverting the duty cycle to implement
inverted polarity would belong in the PWM core if we wanted to implement
it in software I suppose. I'll drop it from the driver.

> > > > +	ret = devm_pwmchip_add(&pdev->dev, &adp5585_pwm->chip);
> > > > +	if (ret) {
> > > > +		mutex_destroy(&adp5585_pwm->lock);
> > > > +		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void adp5585_pwm_remove(struct platform_device *pdev)
> > > > +{
> > > > +	struct adp5585_pwm_chip *adp5585_pwm = platform_get_drvdata(pdev);
> > > > +
> > > > +	mutex_destroy(&adp5585_pwm->lock);
> > > 
> > > Huh, this is a bad idea. The mutex is gone while the pwmchip is still
> > > registered. AFAIK calling mutex_destroy() is optional, and
> > > adp5585_pwm_remove() can just be dropped. Ditto in the error paths of
> > > .probe().
> > 
> > mutex_destroy() is a no-op when !CONFIG_DEBUG_MUTEXES. When the config
> > option is selected, it gets more useful. I would prefer moving away from
> > the devm_* registration, and unregister the pwm_chip in .remove()
> > manually, before destroying the mutex.
> 
> In that case I'd prefer a devm_mutex_init()?!

Maybe that would be useful :-) Let's see if I can drop the mutex though.

Hello Laurent,

On Wed, May 22, 2024 at 01:13:35PM +0300, Laurent Pinchart wrote:
> On Tue, May 21, 2024 at 03:05:53PM +0200, Uwe Kleine-König wrote:
> > On Tue, May 21, 2024 at 01:09:22PM +0300, Laurent Pinchart wrote:
> > > On Tue, May 21, 2024 at 10:51:26AM +0200, Uwe Kleine-König wrote:
> > > > On Mon, May 20, 2024 at 10:59:41PM +0300, Laurent Pinchart wrote:
> > > > > +	if (state->polarity == PWM_POLARITY_INVERSED)
> > > > > +		swap(on, off);
> > > > 
> > > > Uhh, no. Either you can do inverted polarity or you cannot. Don't claim
> > > > you can.
> > > 
> > > OK, but what's the rationale ? This is also an area where I couldn't
> > > find documentation.
> > 
> > I don't have a good rationale here. IMHO this inverted polarity stuff is
> > only a convenience for consumers because the start of the period isn't
> > visible from the output wave form (apart from (maybe) the moment where
> > you change the configuration) and so
> > 
> > 	.period = 5000, duty_cycle = 1000, polarity = PWM_POLARITY_NORMAL
> > 
> > isn't distinguishable from
> > 
> > 	.period = 5000, duty_cycle = 4000, polarity = PWM_POLARITY_INVERSED
> > 
> > . But it's a historic assumption of the pwm core that there is a
> > relevant difference between the two polarities and I want at least a
> > consistent behaviour among the lowlevel drivers. BTW, this convenience
> > is the reason I'm not yet clear how I want to implemement a duty_offset.
> 
> Consistency is certainly good. Inverting the duty cycle to implement
> inverted polarity would belong in the PWM core if we wanted to implement
> it in software I suppose. I'll drop it from the driver.

This isn't as easy as it sounds however. From the POV of the PWM core
the capabilities of the currently used hardware are unclear. So if a
request with (say) normal polarity and a certain duty_cycle + period
fails, it's unknown if it would be beneficial to try with inverted
polarity and if that is OK for the requesting consumer. So there is
information missing in both directions.

Best regards
Uwe