LKML Archive on
help / color / mirror / Atom feed
From: Wesley Terpstra <>
To: Thierry Reding <>
Cc: "Rob Herring" <>,
	"Mark Rutland" <>,
	"Andreas Färber" <>,
	"Noralf Trønnes" <>,
	"David Lechner" <>,
	"Alexandre Belloni" <>,
	"SZ Lin" <>,,,,
	"Wesley W . Terpstra" <>
Subject: Re: [PATCH 3/3] pwm-sifive: add a driver for SiFive SoC PWM
Date: Mon, 30 Apr 2018 12:09:11 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <20180430093907.GA2476@ulmo>

First of all, thank you very much for this detailed review! I really
appreciate it, as this is just the first driver of several we would
like to upstream and getting the reviews front-loaded like this will
really help us improve the subsequent drivers before trying to
upstream them.

On Mon, Apr 30, 2018 at 2:39 AM, Thierry Reding
<> wrote:
> Please include a SPDX license identifier here. That's the preferred way
> to specify the license these days.


>> +#include <dt-bindings/pwm/pwm.h>
> There should be no need to include this in a driver.


>> +             if (state->polarity == PWM_POLARITY_NORMAL)
>> +                     state->duty_cycle = state->period - state->duty_cycle;
> That's not the right way to handle polarity. The above often has the
> same effect as inversed polarity, but it's not generally the right thing
> to do. Please only support polarity if your hardware can actually really
> reverse it. The above is something that PWM consumers (such as the
> backlight driver) should take care of.

I don't know how to declare non-support for polarity. How do I do that?

I still want DTS references to state whether or not the polarity
should be reversed, because the PWM might be connected with either
positive or negative logic to an LED, for example.

>> +     state->polarity   = PWM_POLARITY_INVERSED;
> Is the polarity really reversed by default on this IP?

Yes. In the sense that the PWM is low while the counter is less than
the duty-cycle, and high when >= the duty-cycle.

Note that this also means it's possible to achieve a constant high
value, but impossible to achieve a constant low value, other than

> I don't think this is a good idea. Nobody should be allowed to just
> change the PWM period without letting the PWM controller have a say in
> the matter.

I agree and I intend to fight hard to make sure that future chips don't do this.

> Is this ever really an issue?

Unfortunately, yes. This chip has only one PLL output that controls
almost everything. The core runs at the PLL's output. The bus runs at
half the PLL's output. Each peripheral clock is a divided down version
of the bus clock.

> Does the PWM controller use a shared clock that changes rate frequently?

When you want to change the CPU frequency, you have to update all the
peripheral device clock dividers. It sucks, but that's what has to
happen if the core clock is changed. Fortunately, this is not done
dynamically, but might be done during boot.

For PWM, this is not a big issue, because the period mostly does not
matter as it's broken already. However, for an active SPI transfer,
this can be problematic.

>> +     ret = of_property_read_u32(node, "sifive,npwm", &chip->npwm);
>> +     if (ret < 0 || chip->npwm > MAX_PWM)
>> +             chip->npwm = MAX_PWM;
> I don't see this documented in the DT bindings. I also don't see a need
> for it. Under what circumstances would you want to change this?

The PWM controller can have less than 4 channels. I could try to probe
how many there are by poking registers, but that seems risky if the
PWM is attached to something.

Shall I just add this to the dt-bindings?

>> +     pwm->irq = platform_get_irq(pdev, 0);
>> +     if (pwm->irq < 0) {
>> +             dev_err(dev, "Unable to find interrupt\n");
>> +             return pwm->irq;
>> +     }
> You don't do anything with this, why bother retrieving it?

Mostly to ensure that it is included in the DTS so that I can rely on
it being there in any future driver updates.

>> +     dev_info(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);
> Please don't do this.


  reply	other threads:[~2018-04-30 19:09 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-27 22:59 [PATCH 0/3] SiFive SoC PWM driver Wesley W. Terpstra
2018-04-27 22:59 ` [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation Wesley W. Terpstra
2018-04-29  5:54   ` Thierry Reding
2018-04-29 20:51     ` Wesley Terpstra
2018-04-29 21:01       ` Andreas Färber
2018-04-29 21:08         ` Wesley Terpstra
2018-04-30  8:19           ` Thierry Reding
2018-04-30 10:45             ` Andreas Färber
2018-05-01 16:11               ` Rob Herring
2018-04-30  8:27       ` Thierry Reding
2018-04-30 19:09         ` Wesley Terpstra
2018-04-30  9:42   ` Thierry Reding
2018-04-27 22:59 ` [PATCH 2/3] dt-bindings: Add "sifive" vendor prefix Wesley W. Terpstra
2018-04-28 11:21   ` Andreas Färber
     [not found]     ` <>
2018-05-01 16:14       ` Rob Herring
2018-05-01 16:32         ` Palmer Dabbelt
2018-04-27 22:59 ` [PATCH 3/3] pwm-sifive: add a driver for SiFive SoC PWM Wesley W. Terpstra
2018-04-30  9:39   ` Thierry Reding
2018-04-30 19:09     ` Wesley Terpstra [this message]
2018-05-04  8:43   ` kbuild test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \ \ \ \ \
    --subject='Re: [PATCH 3/3] pwm-sifive: add a driver for SiFive SoC PWM' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).