Hello Clark,
On Tue, Jul 16, 2024 at 03:28:27PM -0400, Frank Li wrote:
> From: Clark Wang <xiaoning.wang@xxxxxxx>
>
> Add PWM function support for MFD adp5585.
>
> Reviewed-by: Haibo Chen <haibo.chen@xxxxxxx>
> Signed-off-by: Clark Wang <xiaoning.wang@xxxxxxx>
> Signed-off-by: Jindong Yue <jindong.yue@xxxxxxx>
> Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> ---
> drivers/pwm/Kconfig | 8 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-adp5585.c | 215 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 224 insertions(+)
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 3e53838990f5b..baaadf877b9c6 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -38,6 +38,14 @@ config PWM_DEBUG
> It is expected to introduce some runtime overhead and diagnostic
> output to the kernel log, so only enable while working on a driver.
>
> +config PWM_ADP5585
> + tristate "ADP5585 PWM support"
> + depends on MFD_ADP5585
> + help
> + This option enables support for on-chip PWM found
> + on Analog Devices ADP5585.
> +
> +
> config PWM_AB8500
> tristate "AB8500 PWM support"
> depends on AB8500_CORE && ARCH_U8500
alphabetic ordering (by config symbol) please.
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 0be4f3e6dd432..161131a261e94 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -2,6 +2,7 @@
> obj-$(CONFIG_PWM) += core.o
> obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o
> obj-$(CONFIG_PWM_APPLE) += pwm-apple.o
> +obj-$(CONFIG_PWM_ADP5585) += pwm-adp5585.o
> obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o
> obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM) += pwm-atmel-hlcdc.o
> obj-$(CONFIG_PWM_ATMEL_TCB) += pwm-atmel-tcb.o
alphabetic ordering please.
> diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c
> new file mode 100644
> index 0000000000000..f578d24df5c74
> --- /dev/null
> +++ b/drivers/pwm/pwm-adp5585.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * PWM driver for Analog Devices ADP5585 MFD
> + *
> + * Copyright 2024 NXP
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/mfd/adp5585.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +#include <linux/time.h>
> +
> +#define ADP5585_PWM_CHAN_NUM 1
This is only used once. I'd prefer to pass the 1 verbatim to
pwmchip_alloc.
> +#define ADP5585_PWM_FASTEST_PERIOD_NS 2000
> +#define ADP5585_PWM_SLOWEST_PERIOD_NS 131070000
Funny number. I wonder where this comes from.
> +struct adp5585_pwm_chip {
> + struct device *parent;
> + struct mutex lock;
> + u8 pin_config_val;
> +};
> +
> +static inline struct adp5585_pwm_chip *
> +to_adp5585_pwm_chip(struct pwm_chip *chip)
> +{
> + return pwmchip_get_drvdata(chip);
> +}
> +
> +static int adp5585_pwm_reg_read(struct adp5585_pwm_chip *adp5585_pwm, u8 reg, u8 *val)
> +{
> + struct adp5585_dev *adp5585 = dev_get_drvdata(adp5585_pwm->parent);
s/ / /;
ditto below in adp5585_pwm_reg_write().
> +
> + return adp5585->read_reg(adp5585, reg, val);
> +}
> +
> +static int adp5585_pwm_reg_write(struct adp5585_pwm_chip *adp5585_pwm, u8 reg, u8 val)
> +{
> + struct adp5585_dev *adp5585 = dev_get_drvdata(adp5585_pwm->parent);
> +
> + return adp5585->write_reg(adp5585, reg, val);
> +}
> +
> +static int pwm_adp5585_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> + u32 on, off;
> + u8 temp;
> +
> + /* get period */
> + adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_OFFT_LOW, &temp);
> + off = temp;
> + adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_OFFT_HIGH, &temp);
> + off |= temp << 8;
> + adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_ONT_LOW, &temp);
> + on = temp;
> + adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_ONT_HIGH, &temp);
> + on |= temp << 8;
> + state->period = (on + off) * NSEC_PER_USEC;
> +
> + state->duty_cycle = on;
> + state->polarity = PWM_POLARITY_NORMAL;
> +
> + /* get channel status */
> + adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_CFG, &temp);
> + state->enabled = temp & ADP5585_PWM_CFG_EN;
> +
> + return 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;
> + u8 enabled;
> + int ret;
> +
> + if (state->period > ADP5585_PWM_SLOWEST_PERIOD_NS ||
> + state->period < ADP5585_PWM_FASTEST_PERIOD_NS)
> + return -EINVAL;
> +
> + guard(mutex)(&adp5585_pwm->lock);
What does this protect? You're allowed (and expected) to assume that the
consumer serializes calls to .apply() for a single pwm_device. Given
that you have npwm=1 I think this lock can be dropped.
> + /* set on/off cycle*/
> + on = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, NSEC_PER_USEC);
> + off = DIV_ROUND_CLOSEST_ULL((state->period - state->duty_cycle), NSEC_PER_USEC);
Please enable PWM_DEBUG your tests and make sure it doesn't produce
warnings. (Hint: round_closest is wrong)
> + ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_OFFT_LOW, off & ADP5585_REG_MASK);
> + if (ret)
> + return ret;
> +
> + ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_OFFT_HIGH,
> + (off >> 8) & ADP5585_REG_MASK);
> + if (ret)
> + return ret;
> +
> + ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_ONT_LOW, on & ADP5585_REG_MASK);
> + if (ret)
> + return ret;
> +
> + ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_ONT_HIGH,
> + (on >> 8) & ADP5585_REG_MASK);
> + if (ret)
> + return ret;
How does the hardware behave in between these register writes? Can it
happen that an intermediate state is visible on the output pin? (E.g.
because off is already written but on is still the old value. Or even
off is only partly written after the first byte write.)
Please document this behaviour in a paragraph at the top of the driver
in the same way as many other PWM drivers do it. The details should be
extractable by
sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c
> +
> + /* enable PWM and set to continuous PWM mode*/
Missing space before comment ending delimiter
> + adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PWM_CFG, &enabled);
> + if (state->enabled)
> + ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_CFG, ADP5585_PWM_CFG_EN);
> + else
> + ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PWM_CFG, 0);
> +
> + return ret;
> +}
> +
> +static int pwm_adp5585_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> + u8 reg_cfg;
> + int ret;
> +
> + guard(mutex)(&adp5585_pwm->lock);
> +
> + adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PIN_CONFIG_C, &adp5585_pwm->pin_config_val);
> + reg_cfg = adp5585_pwm->pin_config_val & ~ADP5585_PIN_CONFIG_R3_MASK;
> + reg_cfg |= ADP5585_PIN_CONFIG_R3_PWM;
> + ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PIN_CONFIG_C, reg_cfg);
ret is only written to here, ditto for &adp5585_pwm->pin_config_val.
> +
> + adp5585_pwm_reg_read(adp5585_pwm, ADP5585_GENERAL_CFG, &adp5585_pwm->pin_config_val);
> + reg_cfg |= ADP5585_GENERAL_CFG_OSC_EN;
> + ret = adp5585_pwm_reg_write(adp5585_pwm, ADP5585_GENERAL_CFG, reg_cfg);
Please add a comment about what is happening here. I assume this sets up
pinmuxing and enabled the oscillator. I wonder if it is sensible to do
the latter only in .apply() iff state->enabled = true.
> +
> + return ret;
> +}
> +
> +static void pwm_adp5585_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> + u8 reg_cfg;
> +
> + guard(mutex)(&adp5585_pwm->lock);
> +
> + adp5585_pwm_reg_read(adp5585_pwm, ADP5585_PIN_CONFIG_C, ®_cfg);
> + reg_cfg &= ~ADP5585_PIN_CONFIG_R3_MASK;
> + reg_cfg |= adp5585_pwm->pin_config_val & ADP5585_PIN_CONFIG_R3_MASK;
> + adp5585_pwm_reg_write(adp5585_pwm, ADP5585_PIN_CONFIG_C, reg_cfg);
It would be consequent to clear ADP5585_GENERAL_CFG_OSC_EN in this
function given that it's set in .request().
> +}
> +
> +static const struct pwm_ops adp5585_pwm_ops = {
> + .request = pwm_adp5585_request,
> + .free = pwm_adp5585_free,
> + .get_state = pwm_adp5585_get_state,
> + .apply = pwm_adp5585_apply,
> +};
> +
> +static int adp5585_pwm_probe(struct platform_device *pdev)
> +{
> + struct adp5585_pwm_chip *adp5585_pwm;
> + struct pwm_chip *chip;
> + unsigned int npwm = ADP5585_PWM_CHAN_NUM;
> + int ret;
> +
> + chip = devm_pwmchip_alloc(&pdev->dev, npwm, sizeof(*adp5585_pwm));
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
Error message using dev_err_probe() please.
> +
> + adp5585_pwm = to_adp5585_pwm_chip(chip);
> + adp5585_pwm->parent = pdev->dev.parent;
> +
> + platform_set_drvdata(pdev, adp5585_pwm);
> +
> + chip->ops = &adp5585_pwm_ops;
> + mutex_init(&adp5585_pwm->lock);
> +
> + ret = devm_pwmchip_add(&pdev->dev, chip);
> + if (ret)
> + dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
Please use dev_err_probe().
> +
> + return ret;
> +}
> +
> +static void adp5585_pwm_remove(struct platform_device *pdev)
> +{
> + struct pwm_chip *chip = platform_get_drvdata(pdev);
> +
> + pwmchip_remove(chip);
Did you test this? I'd expect this to explode.
> +}
> +
> +static const struct of_device_id adp5585_pwm_of_match[] = {
> + {.compatible = "adp5585-pwm", },
Missing space after the opening brace.
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, adp5585_pwm_of_match);
> +
> +static struct platform_driver adp5585_pwm_driver = {
> + .driver = {
> + .name = "adp5585-pwm",
> + .of_match_table = adp5585_pwm_of_match,
> + },
> + .probe = adp5585_pwm_probe,
> + .remove = adp5585_pwm_remove,
> +};
> +module_platform_driver(adp5585_pwm_driver);
> +
> +MODULE_AUTHOR("Xiaoning Wang <xiaoning.wang@xxxxxxx>");
The email address matches one of the S-o-b lines, the name however is
different. Is this by mistake?
> +MODULE_DESCRIPTION("ADP5585 PWM Driver");
> +MODULE_LICENSE("GPL");
Attachment:
signature.asc
Description: PGP signature
