Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support

On Mon, 20 Mar 2017 14:42:52 +0100
Thierry Reding [off-list ref] wrote:

quoted

diff --git a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt

b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt index
a6f3bec..86932e3 100644 ---
a/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt +++
b/Documentation/devicetree/bindings/gpio/gpio-mvebu.txt @@ -38,6
+38,23 @@ Required properties:
 - #gpio-cells: Should be two. The first cell is the pin number. The
   second cell is reserved for flags, unused at the moment.
 
+Optional properties:
+
+In order to use the gpio lines in PWM mode, some additional
optional +properties are required. Only Armada 370 and XP support
these properties. +
+- reg: an additional register set is needed, for the GPIO Blink
+  Counter on/off registers.
+
+- reg-names: Must contain an entry "pwm" corresponding to the
+  additional register range needed for pwm operation.
+
+- #pwm-cells: Should be two. The first cell is the pin number. The
+  second cell is reserved for flags and should be set to 0, so it
has a
+  known value. It then becomes possible to use it in the future.  

That's usually not how we do this. Either your hardware can support
the flags (which at this point effectively means polarity) or it
can't. Any potential future feature can be enabled when it emerges.
No need to concern ourselves with something that doesn't exist yet.

So for short:
  #pwm-cells: Should be one. The first cell is the pin number.

or just a blatant copy of #gpio-cells as in the above hunk.

quoted

@@ -109,6 +139,11 @@ static void __iomem

*mvebu_gpioreg_blink(struct mvebu_gpio_chip *mvchip) return
mvchip->membase + GPIO_BLINK_EN_OFF; }
 
+static void __iomem *mvebu_gpioreg_blink_select(struct
mvebu_gpio_chip *mvchip) +{
+	return mvchip->membase + GPIO_BLINK_CNT_SELECT_OFF;
+}  

That's a really weird thing to do. Why not just use this expression in
your calls to readl() and writel() directly? Seems a lot of additional
code for no gain.

How to hide a tree in the forest. Just following suite with the rest of
the file. So I'd leave it as is but certainly don't mind changing
it.

quoted

+
+static int mvebu_pwm_request(struct pwm_chip *chip, struct
pwm_device *pwmd) +{
+	struct mvebu_pwm *pwm = to_mvebu_pwm(chip);
+	struct mvebu_gpio_chip *mvchip = pwm->mvchip;
+	struct gpio_desc *desc = gpio_to_desc(pwmd->pwm);
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&pwm->lock, flags);
+	if (pwm->used) {
+		ret = -EBUSY;
+	} else {
+		if (!desc) {
+			ret = -ENODEV;
+			goto out;
+		}
+		ret = gpiod_request(desc, "mvebu-pwm");
+		if (ret)
+			goto out;
+
+		ret = gpiod_direction_output(desc, 0);
+		if (ret) {
+			gpiod_free(desc);
+			goto out;
+		}
+
+		pwm->pin = pwmd->pwm - mvchip->chip.base;  

pwm->pin = pwmd->hwpwm? But then, why store something that you can
always access directly?

Agreed.

quoted

+
+static const struct pwm_ops mvebu_pwm_ops = {
+	.request = mvebu_pwm_request,
+	.free = mvebu_pwm_free,
+	.config = mvebu_pwm_config,
+	.enable = mvebu_pwm_enable,
+	.disable = mvebu_pwm_disable,
+	.owner = THIS_MODULE,
+};  

Can you please implement the atomic PWM API? Specifically the
->apply() and ->get_state() implementations replace ->config(),
->enable() and ->disable().  

Will do for v3.

quoted

+/*
+ * Armada 370/XP has simple PWM support for gpio lines. Other SoCs
+ * don't have this hardware. So if we don't have the necessary
+ * resource, it is not an error.
+ */  

There's a bit of inconsistency in this file regarding "pwm" -> "PWM"
and "gpio" -> "GPIO". In prose, please always use the uppercase
version for these abbreviations.

Will do as told for this series and maybe send another cleanup patch
as well.

quoted

+static int mvebu_pwm_probe(struct platform_device *pdev,
+		    struct mvebu_gpio_chip *mvchip,
+		    int id)  

Is there any reason why id would want to be negative?

v2 dropped id from the function signature as I moved id to the
struct mvebu_gpio_chip. Then it's also apparent why not unsigned was
used. Cast it?

quoted

+{
+	struct device *dev = &pdev->dev;
+	struct mvebu_pwm *pwm;
+	struct resource *res;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
"pwm");
+	if (!res)
+		return 0;
+
+	pwm = devm_kzalloc(dev, sizeof(struct mvebu_pwm),
GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+	mvchip->pwm = pwm;
+	pwm->mvchip = mvchip;
+
+	pwm->membase = devm_ioremap_resource(dev, res);
+	if (IS_ERR(pwm->membase))
+		return PTR_ERR(pwm->membase);
+
+	if (id < 0 || id > 1)
+		return -EINVAL;  

You check for negative values here, so might as well turn id into an
unsigned to prohibit them altogether.

See above. Though the test for id < 0 is redundant as we checked this
earlier already.

quoted

+	pwm->id = id;
+
+	if (IS_ERR(mvchip->clk))
+		return PTR_ERR(mvchip->clk);
+
+	pwm->clk_rate = clk_get_rate(mvchip->clk);
+	if (!pwm->clk_rate) {
+		dev_err(dev, "failed to get clock rate\n");
+		return -EINVAL;
+	}
+
+	pwm->chip.dev = dev;
+	pwm->chip.ops = &mvebu_pwm_ops;
+	pwm->chip.base = mvchip->chip.base;
+	pwm->chip.npwm = mvchip->chip.ngpio;  

Isn't that a lie? The code above suggests you can only ever have a
single GPIO turn into a PWM, so I'd expect ".npwm = 1" here.

Agreed.

Thanks
Ralph