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@xxxxxxx>");
> +MODULE_DESCRIPTION("ADP5585 PWM Driver");
> +MODULE_LICENSE("GPL");
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
