linux-kernel - Re: [PATCH v2 2/2] pwm: add this patch to support the new pwm of Rockchip SoCs

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20140721085001.GG8843@ulmo>
Date:	Mon, 21 Jul 2014 10:50:03 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Caesar Wang <caesar.wang@...k-chips.com>
Cc:	heiko@...ech.de, b.galvani@...il.com, cf@...k-chips.com,
	huangtao@...k-chips.com, addy.ke@...k-chips.com,
	xjq@...k-chips.com, linux-pwm@...r.kernel.org,
	devicetree@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] pwm: add this patch to support the new pwm of
 Rockchip SoCs

On Sat, Jul 19, 2014 at 08:55:29PM +0800, Caesar Wang wrote:
> Suggested-by: Beniamino Galvani <b.galvani@...il.com>
> Signed-off-by: Caesar Wang <caesar.wang@...k-chips.com>
> ---
>  drivers/pwm/pwm-rockchip.c | 97 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 79 insertions(+), 18 deletions(-)

This patch is missing a proper description. Also "pwm" should be spelled
"PWM" in prose.

And the subject is somewhat long and generic. How about:

	pwm: rockchip: Add support for RK3288 SoCs

?

> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
[...]
> @@ -12,6 +13,7 @@
>  #include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/pwm.h>
>  #include <linux/time.h>
> @@ -23,14 +25,36 @@
>  #define PWM_CTRL_TIMER_EN	(1 << 0)
>  #define PWM_CTRL_OUTPUT_EN	(1 << 3)
>  
> +#define RK_PRESCALER		1
>  #define PRESCALER		2

I don't think there's a need to keep these around now that they're part
of the rockchip_pwm_data structure.

> +#define PWM_CONTINUOUS		(1 << 1)
> +#define PWM_DUTY_POSITIVE	(1 << 3)
> +#define PWM_INACTIVE_NEGATIVE	(0 << 4)
> +#define PWM_OUTPUT_LEFT		(0 << 5)
> +#define PWM_LP_DISABLE		(0 << 8)
> +
> +#define RK_PWM_ENABLE		(1 << 0)

I think you can drop the RK_ prefix here since none of the above have it
either and they're used in the same context.

> +
> +#define VOP_PWM_CTRL		0x00		/* VOP-PWM Control Register */
> +#define VOP_PWM_CNTR		0x0c

Similarly for these.

> +
>  struct rockchip_pwm_chip {
>  	struct pwm_chip chip;
>  	struct clk *clk;
> +	const struct rockchip_pwm_data *data;
>  	void __iomem *base;
>  };
>  
> +struct rockchip_pwm_data {
> +	u32 duty;
> +	u32 period;
> +	u32 reg_cntr;
> +	u32 reg_ctrl;

duty and period are register offsets, right? Why don't they have the
reg_ prefix? Alternatively you could drop the reg_ prefix altogether.
Yet another alternative would be to introduce a substructure that
contains the registers, to separate the fields logically:

	struct rockchip_pwm_data {
		struct {
			unsigned long duty;
			unsigned long period;
			unsigned long cntr;
			unsigned long ctrl;
		} regs;
		...
	};

As shown, I think it's more natural for register offsets to be unsigned
long rather than a strictly-sized type.

> +	int prescaler;

unsigned int

> +	u32 enable_conf;
> +};

For this I think it would be more readable to provide function pointers
rather than a variable. That is:

	struct rockchip_pwm_data {
		...
		int (*enable)(struct pwm_chip *chip, struct pwm_device *pwm);
		int (*disable)(struct pwm_chip *chip, struct pwm_device *pwm);
	};

Then you can implement these for each variant of the chip and call them
from the common rockchip_pwm_enable(), somewhat like this:

	static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
	{
		struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
		int ret;

		ret = clk_enable(pc->clk);
		if (ret)
			return ret;

		ret = pc->data->enable(chip, pwm);
		if (ret)
			clk_disable(pc->clk);

		return ret;
	}

And similarly for rockchip_pwm_disable().

> +static struct rockchip_pwm_data pwm_data_v1 = {

static const please.

> +static struct rockchip_pwm_data pwm_data_v2 = {

Here too.

> +static struct rockchip_pwm_data pwm_data_vop = {

And here.

> +	const struct of_device_id *of_id =

I don't think the of_ prefix is necessary here.

> +	    of_match_device(rockchip_pwm_dt_ids, &pdev->dev);

And perhaps rather than split this over two lines here you can move this
assignment somewhere below the variable declarations?

>  	struct rockchip_pwm_chip *pc;
>  	struct resource *r;
>  	int ret;
> @@ -119,7 +182,10 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	pc->base = devm_ioremap_resource(&pdev->dev, r);
> +	if (!strcmp(of_id->compatible, "rockchip,vop-pwm"))
> +		pc->base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
> +	else
> +		pc->base = devm_ioremap_resource(&pdev->dev, r);

Sorry, this still isn't an option. You really shouldn't remap I/O
regions that other drivers may be using. You hinted at a shared register
space during the review of the initial version. Can you provide more
detail about what exactly the memory map looks like of the rk3288? Is
there some kind of technical reference manual that I could look at? Or
do you have a device tree extract that shows what the memory map looks
like?

Thierry

Content of type "application/pgp-signature" skipped