linux-kernel - Re: [resend rfc v4]pwm: add BCM2835 PWM driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <542A32E1.8040003@wwwdotorg.org>
Date:	Mon, 29 Sep 2014 22:34:41 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Bart Tanghe <bart.tanghe@...masmore.be>, thierry.reding@...il.com
CC:	matt.porter@...aro.org, linux-kernel@...r.kernel.org,
	linux-pwm@...r.kernel.org, linux-rpi-kernel@...ts.infradead.org
Subject: Re: [resend rfc v4]pwm: add BCM2835 PWM driver

On 09/29/2014 08:40 AM, Bart Tanghe wrote:
> Add pwm driver for Broadcom BCM2835 processor (Raspberry Pi) 
> Signed-off-by: Bart Tanghe <bart.tanghe@...masmore.be>

There needs to be a blank line between the description and the tags.

> diff --git a/Documentation/devicetree/bindings/pwm/pwm-bcm2835.txt b/Documentation/devicetree/bindings/pwm/pwm-bcm2835.txt

> +Required properties:
> +- compatible: should be "brcm,bcm2835-pwm"
> +- reg: physical base address and length of the controller's registers

This needs to document the clock property too.

It'd be nice to require clock-names rather than doing clock lookup by
index, but I suppose it's not too much of a problem with this device.

> diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c

> +/*
> + * Copyright (C) 2014 Thomas more

But the git commit doesn't have "Thomas More" as the author, nor Thomas'
Signed-off-by line. Can you explain the history of this code?

> +static int bcm2835_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
> +	u32 value;
> +
> +	value = readl(pc->base) & ~(PWM_CONTROL_MASK << 8 * pwm->pwm);
> +	value |= (PWM_CONTROL_ENABLE << (8 * pwm->pwm));

It'd be nice to use a #define rather than hard-coded "8" here. Perhaps:

#define PWM_CONTROL_STRIDE 8

Why does _request() enable the PWM output; shouldn't that be deferred
until _enable()? _free() might not want to disable the output, if the
PWM core guarantees that _disable() is always called.

> +static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      int duty_ns, int period_ns)
> +{
> +	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
> +
> +	if (period_ns > MIN_PERIOD) {
> +		writel(duty_ns / pc->scaler,
> +			 pc->base + DUTY + pwm->pwm * CHANNEL);
> +		writel(period_ns / pc->scaler,
> +			pc->base + PERIOD + pwm->pwm * CHANNEL);
> +	} else {
> +		dev_err(pc->dev, "Period not supported\n");
> +	}

The "else" case should propagate the error. It'd be better to do the
error-checking first to remove an indent level from the rest of the code:

	if (period_ns <= MIN_PERIOD) {
		dev_err(...);
		return -EINVAL;
	}
	writel(...);

> +static int bcm2835_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> +				enum pwm_polarity polarity)
> +{
> +	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
> +	u32 value;
> +
> +	if (polarity == PWM_POLARITY_NORMAL) {
> +		value = readl(pc->base);
> +		value &= ~(PWM_POLARITY << 8 * pwm->pwm);
> +		writel(value, pc->base);
> +	} else if (polarity == PWM_POLARITY_INVERSED) {
> +		value = readl(pc->base);
> +		value |= PWM_POLARITY << (8 * pwm->pwm);
> +		writel(value, pc->base);
> +	}

If you move the readl/writel outside the if statement, you remove the
duplication of code.

> +static const struct pwm_ops bcm2835_pwm_ops = {
...
> +	.owner = THIS_MODULE,
> +};

Doesn't something in the driver core automatically set .owner now?
Perhaps that's only for certain subsystems though?

> +MODULE_AUTHOR("Bart Tanghe <bart.tanghe@...masmore.be");

That doesn't match the (c) message at the start of the file.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/