On Tue, Mar 18, 2014 at 2:52 PM, Thierry Reding
<thierry.reding@xxxxxxxxx> wrote:
> On Wed, Mar 12, 2014 at 01:15:43PM -0700, Tim Kryger wrote:
> [...]
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 22f2f28..1fd42af 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -62,6 +62,16 @@ config PWM_ATMEL_TCB
>> To compile this driver as a module, choose M here: the module
>> will be called pwm-atmel-tcb.
>>
>> +config PWM_BCM_KONA
>> + tristate "Kona PWM support"
>> + depends on ARCH_BCM_MOBILE
>> + default y
>
> No "default y", please.
Even though it depends on ARCH_BCM_MOBILE? It seems like a reasonable default.
These SoCs target mobile phones which almost always have a backlight
controlled by the PWM.
>
>> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> [...]
>> +static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
>> +{
>> + unsigned long value = readl(kp->base + PWM_CONTROL_OFFSET);
>> +
>> + /*
>> + * New duty and period settings are only reflected in the PWM output
>> + * after a rising edge of the enable bit. When smooth bit is set, the
>> + * new settings are delayed until the prior PWM period has completed.
>> + * Furthermore, if the smooth bit is set, the PWM continues to output a
>> + * waveform based on the old settings during the time that the enable
>> + * bit is low. Otherwise the output is a constant high signal while
>> + * the enable bit is low.
>> + */
>> +
>> + /* clear enable bit but set smooth bit to maintain old output */
>> + value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
>
> There's no need for the parentheses here.
Ok
>
>> + value &= ~(1 << PWM_CONTROL_ENABLE_SHIFT(chan));
>> + writel(value, kp->base + PWM_CONTROL_OFFSET);
>> +
>> + /* set enable bit and clear smooth bit to apply new settings */
>> + value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
>> + value |= (1 << PWM_CONTROL_ENABLE_SHIFT(chan));
>
> Nor here.
Ok
>
>> + writel(value, kp->base + PWM_CONTROL_OFFSET);
>
> I'm curious about how this work. The above writes the control register
> once with smooth set and enable cleared, then immediately rewrites it
> again with smooth cleared and enable set. Are these writes somehow
> queued in hardware, so that the subsequent write becomes active only
> after the current period?
It isn't exactly queuing up the writes but if smooth mode is set, it
will wait to apply the new settings.
>
>> +static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> + int duty_ns, int period_ns)
>
> Please align arguments on subsequent lines with those on the first line.
Sure
>
>> +{
>> + struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
>
> The proper way to do this would be upcasting using container_of().
> Better yet, define a static inline function that wraps container_of(),
> just like very other driver does.
>
I will convert to use this approach (except in kona_pwmc_remove)
>> + u64 val, div, clk_rate;
>> + unsigned long prescale = PRESCALE_MIN, pc, dc;
>> + unsigned int value, chan = pwm->hwpwm;
>> +
>> + /*
>> + * Find period count, duty count and prescale to suit duty_ns and
>> + * period_ns. This is done according to formulas described below:
>> + *
>> + * period_ns = 10^9 * (PRESCALE + 1) * PC / PWM_CLK_RATE
>> + * duty_ns = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE
>> + *
>> + * PC = (PWM_CLK_RATE * period_ns) / (10^9 * (PRESCALE + 1))
>> + * DC = (PWM_CLK_RATE * duty_ns) / (10^9 * (PRESCALE + 1))
>> + */
>> +
>> + clk_rate = clk_get_rate(kp->clk);
>
> "rate" is 50% shorter and would do equally well.
That works for me.
>
>> +
>> + /* There is polarity support in HW but it is easier to manage in SW */
>> + if (pwm->polarity == PWM_POLARITY_INVERSED)
>> + duty_ns = period_ns - duty_ns;
>
> No, this is wrong. You're not actually changing the *polarity* here.
Please elaborate. I don't understand what is wrong here.
Does polarity influence the output while a PWM is disabled?
>
> Also I think this is missing a pair of clk_prepare_enable() and
> clk_disable_unprepare().
There is no issue here since the hardware registers are only touched
in kona_pwmc_config if the channel was already enabled.
>
>> +static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>> + enum pwm_polarity polarity)
>> +{
>> + /*
>> + * The framework only allows the polarity to be changed when a PWM is
>> + * disabled so no immediate action is required here. When a channel is
>> + * enabled, the polarity gets handled as part of the re-config step.
>> + */
>> +
>> + return 0;
>> +}
>
> See above. If you don't want to implement the hardware support for
> inversed polarity, then simply don't implement this.
I had originally planned to omit polarity support but because it
affects the binding (which is treated as ABI), it wouldn't be possible
to add it in later without defining a new compatible string.
>
>> +static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
>> + int ret;
>> +
>> + /*
>> + * The PWM framework does not clear the enable bit in the flags if an
>> + * error is returned from a PWM driver's enable function so it must be
>> + * cleared here if any trouble is encountered.
>> + */
>> +
>> + ret = clk_prepare_enable(kp->clk);
>> + if (ret < 0) {
>> + dev_err(chip->dev, "failed to enable clock: %d\n", ret);
>> + clear_bit(PWMF_ENABLED, &pwm->flags);
>
> You're not supposed to touch these. This is a bug in the core, and it
> should be fixed in the core.
Okay. I'll drop the clear_bit lines from this patch.
>
>> + return ret;
>> + }
>> +
>> + ret = kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
>> + if (ret < 0) {
>> + clk_disable_unprepare(kp->clk);
>> + clear_bit(PWMF_ENABLED, &pwm->flags);
>> + return ret;
>> + }
>
> Why are you doing this? .config() should be setting everything up
> according to the given duty cycle and period and .enable() should only
> be enabling a specific channel. Please don't conflate the two.
The hardware only supports a configure operation.
Thus disable has to be simulated by configuring zero duty.
During an enable operation we have to program the last configuration.
>
>> +static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
>> + unsigned int chan = pwm->hwpwm;
>> +
>> + /*
>> + * The "enable" bits in the control register only affect when settings
>> + * start to take effect so the only real way to disable the PWM output
>> + * is to program a zero duty cycle.
>> + */
>
> What's wrong about waiting for the settings to take effect? There's
> nothing about disable that says it should happen *immediately*.
The current code does wait for the settings to take effect.
Can you clarify what you mean?
>
>> +
>> + writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
>> + kona_pwmc_apply_settings(kp, chan);
>> +
>> + /*
>> + * When the PWM clock is disabled, the output is pegged high or low
>> + * depending on its state at that instant. To guarantee that the new
>> + * settings have taken effect and the output is low a delay of 400ns is
>> + * required.
>> + */
>> +
>> + ndelay(400);
>
> Where does this number come from?
Hardware documentation
>
>> +static int kona_pwmc_probe(struct platform_device *pdev)
>> +{
>> + struct kona_pwmc *kp;
>> + struct resource *res;
>> + unsigned int chan, value;
> [...]
>> + /* Set smooth mode, push/pull, and normal polarity for all channels */
>> + for (value = 0, chan = 0; chan < kp->chip.npwm; chan++) {
>
> Can't you initialize value to 0 when you declare it? That way the for
> loop becomes much more idiomatic.
Sure.
>
>> + value |= (1 << PWM_CONTROL_SMOOTH_SHIFT(chan));
>> + value |= (1 << PWM_CONTROL_TYPE_SHIFT(chan));
>> + value |= (1 << PWM_CONTROL_POLARITY_SHIFT(chan));
>> + }
>> + writel(value, kp->base + PWM_CONTROL_OFFSET);
>
> I'd prefer a blank line between the above two for readability.
Ok
>
>> +static struct platform_driver kona_pwmc_driver = {
>> +
>
> Gratuitous blank line.
Ok
>
>> + .driver = {
>> + .name = "bcm-kona-pwm",
>> + .of_match_table = bcm_kona_pwmc_dt,
>> + },
>> + .probe = kona_pwmc_probe,
>> + .remove = kona_pwmc_remove,
>> +};
>> +
>
> And here.
Ok
>
>> +module_platform_driver(kona_pwmc_driver);
>> +
>> +MODULE_AUTHOR("Broadcom Corporation <bcm-kernel-feedback-list@xxxxxxxxxxxx>");
>> +MODULE_AUTHOR("Tim Kryger <tkryger@xxxxxxxxxxxx>");
>> +MODULE_DESCRIPTION("Driver for Kona PWM controller");
>
> Perhaps "Broadcom Kona PWM"?
Sure
Thanks for the review.
-Tim
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
