| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200728074352.ckyfso5zwsfgupjx@pengutronix.de>
Date: Tue, 28 Jul 2020 09:43:52 +0200
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To: Michael Walle <michael@...le.cc>
Cc: linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
linux-pwm@...r.kernel.org, linux-watchdog@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <bgolaszewski@...libre.com>,
Rob Herring <robh+dt@...nel.org>,
Jean Delvare <jdelvare@...e.com>,
Guenter Roeck <linux@...ck-us.net>,
Lee Jones <lee.jones@...aro.org>,
Thierry Reding <thierry.reding@...il.com>,
Wim Van Sebroeck <wim@...ux-watchdog.org>,
Shawn Guo <shawnguo@...nel.org>, Li Yang <leoyang.li@....com>,
Thomas Gleixner <tglx@...utronix.de>,
Jason Cooper <jason@...edaemon.net>,
Marc Zyngier <maz@...nel.org>, Mark Brown <broonie@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>, Pavel Machek <pavel@....cz>
Subject: Re: [PATCH v6 06/13] pwm: add support for sl28cpld PWM controller
Hello,
just a few minor issues left:
On Sun, Jul 26, 2020 at 01:18:27AM +0200, Michael Walle wrote:
> diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
> new file mode 100644
> index 000000000000..956fa09f3aba
> --- /dev/null
> +++ b/drivers/pwm/pwm-sl28cpld.c
> @@ -0,0 +1,223 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * sl28cpld PWM driver
> + *
> + * Copyright (c) 2020 Michael Walle <michael@...le.cc>
> + *
> + * There is no public datasheet available for this PWM core. But it is easy
> + * enough to be briefly explained. It consists of one 8-bit counter. The PWM
> + * supports four distinct frequencies by selecting when to reset the counter.
> + * With the prescaler setting you can select which bit of the counter is used
> + * to reset it. This implies that the higher the frequency the less remaining
> + * bits are available for the actual counter.
> + *
> + * Let cnt[7:0] be the counter, clocked at 32kHz:
> + * +-----------+--------+--------------+-----------+
> + * | prescaler | reset | counter bits | frequency |
> + * +-----------+--------+--------------+-----------+
> + * | 0 | cnt[7] | cnt[6:0] | 250Hz |
> + * | 1 | cnt[6] | cnt[5:0] | 500Hz |
> + * | 2 | cnt[5] | cnt[4:0] | 1kHz |
> + * | 3 | cnt[4] | cnt[3:0] | 2kHz |
> + * +-----------+--------+--------------+-----------+
Very nice. I'd add a "period length" column, as this is what the PWM
core uses.
For your convenience (and as I created that table anyhow for further
checking of the formulas below):
* +-----------+--------+--------------+-----------+--------+
* | prescaler | reset | counter bits | frequency | period |
* | | | | | length |
* +-----------+--------+--------------+-----------+--------+
* | 0 | cnt[7] | cnt[6:0] | 250Hz | 4000ns |
* | 1 | cnt[6] | cnt[5:0] | 500Hz | 2000ns |
* | 2 | cnt[5] | cnt[4:0] | 1kHz | 1000ns |
* | 3 | cnt[4] | cnt[3:0] | 2kHz | 500ns |
* +-----------+--------+--------------+-----------+--------+
> + *
> + * Limitations:
> + * - The hardware cannot generate a 100% duty cycle if the prescaler is 0.
> + * - The hardware cannot atomically set the prescaler and the counter value,
> + * which might lead to glitches and inconsistent states if a write fails.
> + * - The counter is not reset if you switch the prescaler which leads
> + * to glitches, too.
> + * - The duty cycle will switch immediately and not after a complete cycle.
> + * - Depending on the actual implementation, disabling the PWM might have
> + * side effects. For example, if the output pin is shared with a GPIO pin
> + * it will automatically switch back to GPIO mode.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +/*
> + * PWM timer block registers.
> + */
> +#define SL28CPLD_PWM_CTRL 0x00
> +#define SL28CPLD_PWM_CTRL_ENABLE BIT(7)
> +#define SL28CPLD_PWM_CTRL_PRESCALER_MASK GENMASK(1, 0)
> +#define SL28CPLD_PWM_CYCLE 0x01
> +#define SL28CPLD_PWM_CYCLE_MAX GENMASK(6, 0)
> +
> +#define SL28CPLD_PWM_CLK 32000 /* 32 kHz */
> +#define SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler) (1 << (7 - (prescaler)))
> +#define SL28CPLD_PWM_PERIOD(prescaler) \
> + (NSEC_PER_SEC / SL28CPLD_PWM_CLK * SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler))
> +
> +/*
> + * We calculate the duty cycle like this:
> + * duty_cycle_ns = pwm_cycle_reg * max_period_ns / max_duty_cycle
> + *
> + * With
> + * max_period_ns = (1 << 7 - prescaler) / pwm_clk * NSEC_PER_SEC
> + * max_duty_cycle = 1 << (7 - prescaler)
If you don't need parenthesis in the max_period_ns around 7 - prescaler,
you don't need them either in the max_duty_cycle line.
> + * this then simplifies to:
> + * duty_cycle_ns = pwm_cycle_reg / pwm_clk * NSEC_PER_SEC
> + */
> +#define SL28CPLD_PWM_TO_DUTY_CYCLE(reg) \
> + (NSEC_PER_SEC / SL28CPLD_PWM_CLK * (reg))
For those who copy from your driver maybe add a comment like:
* NSEC_PER_SEC / SL28CPLD_PWM_CLK is integer here, so we're not loosing
* precision by doing the division first.
> +#define SL28CPLD_PWM_FROM_DUTY_CYCLE(duty_cycle) \
> + (DIV_ROUND_DOWN_ULL((duty_cycle), NSEC_PER_SEC / SL28CPLD_PWM_CLK))
> +
> +struct sl28cpld_pwm {
> + struct pwm_chip pwm_chip;
> + struct regmap *regmap;
> + u32 offset;
> +};
> +
> +static void sl28cpld_pwm_get_state(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev);
> + unsigned int reg;
> + int prescaler;
> +
> + regmap_read(priv->regmap, priv->offset + SL28CPLD_PWM_CTRL, ®);
Would it make sense to hide this using e.g.:
#define sl28cpkd_pwm_read(priv, reg, val) regmap_read((priv)->regmap, (priv)->offset + (reg), val)
The line would then become:
sl28cpkd_pwm_read(priv, SL28CPLD_PWM_CTRL, ®);
which is a bit prettier. Up to you to decide. If you do it, please do
the same for write
> + state->enabled = reg & SL28CPLD_PWM_CTRL_ENABLE;
> +
> + prescaler = FIELD_GET(SL28CPLD_PWM_CTRL_PRESCALER_MASK, reg);
> + state->period = SL28CPLD_PWM_PERIOD(prescaler);
> +
> + regmap_read(priv->regmap, priv->offset + SL28CPLD_PWM_CYCLE, ®);
> + state->duty_cycle = SL28CPLD_PWM_TO_DUTY_CYCLE(reg);
> + state->polarity = PWM_POLARITY_NORMAL;
> +}
> +
> +static int sl28cpld_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev);
> + unsigned int cycle, prescaler;
> + int ret;
> + u8 ctrl;
> +
> + /* Polarity inversion is not supported */
> + if (state->polarity != PWM_POLARITY_NORMAL)
> + return -EINVAL;
> +
> + /*
> + * Calculate the prescaler. Pick the the biggest period that isn't
> + * bigger than the requested period.
> + */
> + prescaler = DIV_ROUND_UP_ULL(SL28CPLD_PWM_PERIOD(0), state->period);
> + prescaler = order_base_2(prescaler);
> +
> + if (prescaler > field_max(SL28CPLD_PWM_CTRL_PRESCALER_MASK))
> + return -ERANGE;
The calculation looks right.
Did you check the generated code? Maybe using an if or switch here is
more effective? (optional task for bonus points :-)
> + ctrl = FIELD_PREP(SL28CPLD_PWM_CTRL_PRESCALER_MASK, prescaler);
> + if (state->enabled)
> + ctrl |= SL28CPLD_PWM_CTRL_ENABLE;
> +
> + cycle = SL28CPLD_PWM_FROM_DUTY_CYCLE(state->duty_cycle);
> + cycle = min_t(unsigned int, cycle, SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler));
> +
> + /*
> + * Work around the hardware limitation. See also above. Trap 100% duty
> + * cycle if the prescaler is 0. Set prescaler to 1 instead. We don't
> + * care about the frequency because its "all-one" in either case.
> + *
> + * We don't need to check the actual prescaler setting, because only
> + * if the prescaler is 0 we can have this particular value.
> + */
> + if (cycle == SL28CPLD_PWM_MAX_DUTY_CYCLE(0)) {
> + ctrl &= ~SL28CPLD_PWM_CTRL_PRESCALER_MASK;
> + ctrl |= FIELD_PREP(SL28CPLD_PWM_CTRL_PRESCALER_MASK, 1);
> + cycle = SL28CPLD_PWM_MAX_DUTY_CYCLE(1);
> + }
> +
> + ret = regmap_write(priv->regmap, priv->offset + SL28CPLD_PWM_CTRL, ctrl);
> + if (ret)
> + return ret;
> +
> + return regmap_write(priv->regmap, priv->offset + SL28CPLD_PWM_CYCLE, (u8)cycle);
This cast isn't needed, is it?
> +}
> +
> +static const struct pwm_ops sl28cpld_pwm_ops = {
> + .apply = sl28cpld_pwm_apply,
> + .get_state = sl28cpld_pwm_get_state,
> + .owner = THIS_MODULE,
> +};
> +
> +static int sl28cpld_pwm_probe(struct platform_device *pdev)
> +{
> + struct sl28cpld_pwm *priv;
> + struct pwm_chip *chip;
> + int ret;
> +
> + if (!pdev->dev.parent)
> + return -ENODEV;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> + if (!priv->regmap)
Error message here?
> + return -ENODEV;
> +
> + ret = device_property_read_u32(&pdev->dev, "reg", &priv->offset);
> + if (ret) {
> + dev_err(&pdev->dev, "no 'reg' property found (%pe)\n",
> + ERR_PTR(ret));
> + return -EINVAL;
> + }
> +
> + /* Initialize the pwm_chip structure */
> + chip = &priv->pwm_chip;
> + chip->dev = &pdev->dev;
> + chip->ops = &sl28cpld_pwm_ops;
> + chip->base = -1;
> + chip->npwm = 1;
> +
> + ret = pwmchip_add(&priv->pwm_chip);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to add PWM chip (%pe)",
> + ERR_PTR(ret));
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, priv);
> +
> + return 0;
> +}
> +
> +static int sl28cpld_pwm_remove(struct platform_device *pdev)
> +{
> + struct sl28cpld_pwm *priv = platform_get_drvdata(pdev);
> +
> + return pwmchip_remove(&priv->pwm_chip);
> +}
> +
> +static const struct of_device_id sl28cpld_pwm_of_match[] = {
> + { .compatible = "kontron,sl28cpld-pwm" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, sl28cpld_pwm_of_match);
> +
> +static struct platform_driver sl28cpld_pwm_driver = {
> + .probe = sl28cpld_pwm_probe,
> + .remove = sl28cpld_pwm_remove,
> + .driver = {
> + .name = "sl28cpld-pwm",
> + .of_match_table = sl28cpld_pwm_of_match,
> + },
> +};
> +module_platform_driver(sl28cpld_pwm_driver);
> +
> +MODULE_DESCRIPTION("sl28cpld PWM Driver");
> +MODULE_AUTHOR("Michael Walle <michael@...le.cc>");
> +MODULE_LICENSE("GPL");
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists