Messages in this thread |  | | Date | Tue, 28 Jul 2020 10:21:22 +0200 | From | Michael Walle <> | Subject | Re: [PATCH v6 06/13] pwm: add support for sl28cpld PWM controller |
| |
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, ®); > > 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
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, ®); >> + 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
|  |