Re: [PATCH v6 06/13] pwm: add support for sl28cpld PWM controller

[Michael Walle <michael@walle.cc>]

Hi,

Am 2020-07-28 09:43, schrieb Uwe Kleine-König:
> Hello,
> 
> just a few minor issues left:

thanks for the review.

> 
> 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@walle.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 |
>  * +-----------+--------+--------------+-----------+--------+

sure :)

> 
>> + *
>> + * 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.

mhh this should be "1 << (7 - prescaler)" in both cases. So
max_period_ns is wrong:
   max_period_ns = 1 << (7 - prescaler) / pwm_clk * NSEC_PER_SEC


>> + * 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.

ok.

>> +#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, &reg);
> 
> 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, &reg);
> 
> which is a bit prettier. Up to you to decide. If you do it, please do
> the same for write


I don't have a strong opinion on that. I can change it. Although there
will be checkpatch warning about multiple uses of the macro argument,
I'd presume.

>> +	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, &reg);
>> +	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 :-)

I varied between this and some if/switch. This hard to read IMHO (as
was your your ilog(n+1)+1), but you could easily change the range
of the prescaler without having to change this. Also if/switch
looked ugly too *g*. I'll check again.

> 
>> +	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?

Due to the clamping, it is not, correct. I'll remove 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?

This shouldn't really happen and I put it into the same category
as the two above and report no error. But I can add it.

Generally, it looked to me that more and more drivers don't
really report errors anymore, but just return with an -EWHATEVER.
So if someone can shed some light here, I'm all ears.

-michael