Re: [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC

[caesar <caesar.wang-TNX95d0MmH7DzftRWevZcw@public.gmane.org>]

Thierry,

在 2014年08月07日 14:18, Thierry Reding 写道:
> On Thu, Jul 24, 2014 at 06:21:35PM +0800, Caesar Wang wrote:
>> This patch added to support the PWM controller found on
>> RK3288 SoC.
>>
>> Signed-off-by: Caesar Wang <caesar.wang-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> ---
>>   drivers/pwm/pwm-rockchip.c | 124 ++++++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 105 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
>> index eec2145..59c2513 100644
>> --- a/drivers/pwm/pwm-rockchip.c
>> +++ b/drivers/pwm/pwm-rockchip.c
>> @@ -2,6 +2,7 @@
>>    * PWM driver for Rockchip SoCs
>>    *
>>    * Copyright (C) 2014 Beniamino Galvani <b.galvani-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> + * Copyright (C) 2014 ROCKCHIP, Inc.
>>    *
>>    * This program is free software; you can redistribute it and/or
>>    * modify it under the terms of the GNU General Public License
>> @@ -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>
>> @@ -25,17 +27,72 @@
>>   
>>   #define PRESCALER		2
>>   
>> +#define PWM_ENABLE		(1 << 0)
>> +#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)
>> +
>>   struct rockchip_pwm_chip {
>>   	struct pwm_chip chip;
>>   	struct clk *clk;
>> +	const struct rockchip_pwm_data *data;
>>   	void __iomem *base;
>>   };
>>   
>> +struct rockchip_pwm_regs {
>> +	unsigned long duty;
>> +	unsigned long period;
>> +	unsigned long cntr;
>> +	unsigned long ctrl;
>> +};
>> +
>> +struct rockchip_pwm_data {
>> +	struct rockchip_pwm_regs regs;
>> +	unsigned int prescaler;
>> +
>> +	void (*set_enable)(struct pwm_chip *chip, bool enable);
>> +};
>> +
>>   static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c)
>>   {
>>   	return container_of(c, struct rockchip_pwm_chip, chip);
>>   }
>>   
>> +static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
>> +{
>> +	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
>> +	u32 val = 0;
>> +	u32 enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN;
>> +
>> +	val = readl_relaxed(pc->base + pc->data->regs.ctrl);
>> +
>> +	if (enable)
>> +		val |= enable_conf;
>> +	else
>> +		val &= ~enable_conf;
>> +
>> +	writel_relaxed(val, pc->base + pc->data->regs.ctrl);
>> +}
>> +
>> +static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
>> +{
>> +	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
>> +	u32 val = 0;
>> +	u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
>> +		PWM_CONTINUOUS | PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE;
>> +
>> +	val = readl_relaxed(pc->base + pc->data->regs.ctrl);
>> +
>> +	if (enable)
>> +		val |= enable_conf;
>> +	else
>> +		val &= ~enable_conf;
>> +
>> +	writel_relaxed(val, pc->base + pc->data->regs.ctrl);
>> +}
>> +
>>   static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>   			       int duty_ns, int period_ns)
>>   {
>> @@ -52,20 +109,20 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>   	 * default prescaler value for all practical clock rate values.
>>   	 */
>>   	div = clk_rate * period_ns;
>> -	do_div(div, PRESCALER * NSEC_PER_SEC);
>> +	do_div(div, pc->data->prescaler * NSEC_PER_SEC);
>>   	period = div;
>>   
>>   	div = clk_rate * duty_ns;
>> -	do_div(div, PRESCALER * NSEC_PER_SEC);
>> +	do_div(div, pc->data->prescaler * NSEC_PER_SEC);
>>   	duty = div;
>>   
>>   	ret = clk_enable(pc->clk);
>>   	if (ret)
>>   		return ret;
>>   
>> -	writel(period, pc->base + PWM_LRC);
>> -	writel(duty, pc->base + PWM_HRC);
>> -	writel(0, pc->base + PWM_CNTR);
>> +	writel(period, pc->base + pc->data->regs.period);
>> +	writel(duty, pc->base + pc->data->regs.duty);
>> +	writel(0, pc->base + pc->data->regs.cntr);
>>   
>>   	clk_disable(pc->clk);
>>   
>> @@ -76,15 +133,12 @@ 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;
>> -	u32 val;
>>   
>>   	ret = clk_enable(pc->clk);
>>   	if (ret)
>>   		return ret;
>>   
>> -	val = readl_relaxed(pc->base + PWM_CTRL);
>> -	val |= PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN;
>> -	writel_relaxed(val, pc->base + PWM_CTRL);
>> +	pc->data->set_enable(chip, true);
>>   
>>   	return 0;
>>   }
>> @@ -92,11 +146,8 @@ static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>>   static void rockchip_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>>   {
>>   	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
>> -	u32 val;
>>   
>> -	val = readl_relaxed(pc->base + PWM_CTRL);
>> -	val &= ~(PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN);
>> -	writel_relaxed(val, pc->base + PWM_CTRL);
>> +	pc->data->set_enable(chip, false);
>>   
>>   	clk_disable(pc->clk);
>>   }
>> @@ -108,12 +159,52 @@ static const struct pwm_ops rockchip_pwm_ops = {
>>   	.owner = THIS_MODULE,
>>   };
>>   
>> +static const struct rockchip_pwm_data pwm_data_v1 = {
>> +	.regs.duty = PWM_HRC,
>> +	.regs.period = PWM_LRC,
>> +	.regs.cntr = PWM_CNTR,
>> +	.regs.ctrl = PWM_CTRL,
> Perhaps a slightly more idiomatic way to write this would be:
>
> 	static const struct rockchip_pwm_data pwm_data_v1 = {
> 		.regs = {
> 			.duty = PWM_HRC,
> 			.period = PWM_LRC,
> 			.cntr = PWM_CNTR,
> 			.ctrl = PWM_CTRL,
> 		},
> 		...
> 	};
>
> And similar for the v2 and vop structures. And like I said in another
> reply, since the defines are now only used in this structure it's a
> little redundant to give them symbolic names, so the above could equally
> well be:
>
> 	static const struct rockchip_pwm_data pwm_data_v1 = {
> 		.regs = {
> 			.duty = 0x04,
> 			.period = 0x08,
> 			.cntr = 0x00,
> 			.ctrl = 0x0c,
> 		},
> 		...
> 	};
>
>> +	.prescaler = PRESCALER,
> Similarly for the prescaler value, it can now simply be 2 here.
>
>> +	.set_enable = rockchip_pwm_set_enable_v1,
>> +};
>> +
>> +static const struct rockchip_pwm_data pwm_data_v2 = {
>> +	.regs.duty = PWM_LRC,
>> +	.regs.period = PWM_HRC,
>> +	.regs.cntr = PWM_CNTR,
>> +	.regs.ctrl = PWM_CTRL,
>> +	.prescaler = PRESCALER-1,
> And 1 here.
>
>> +	.set_enable = rockchip_pwm_set_enable_v2,
>> +};
>> +
>> +static const struct rockchip_pwm_data pwm_data_vop = {
>> +	.regs.duty = PWM_LRC,
>> +	.regs.period = PWM_HRC,
>> +	.regs.cntr = PWM_CTRL,
>> +	.regs.ctrl = PWM_CNTR,
>> +	.prescaler = PRESCALER-1,
> And 1 here.

As you say, I will rewrite the about if it's really need  do so it.
For example:

static const struct rockchip_pwm_data pwm_data_v1 = {
     .regs = {
                 .duty = 0x04,
                 .period = 0x08,
                 .cntr = 0x00,
                 .ctrl = 0x0c,
     },
     .prescaler = 2,
     .set_enable = rockchip_pwm_set_enable_v1,
};

static const struct rockchip_pwm_data pwm_data_v2 = {
     .regs = {
                 .duty = 0x08,
                 .period = 0x04,
                 .cntr = 0x00,
                 .ctrl = 0x0c,
     },
     .prescaler = 1,
     .set_enable = rockchip_pwm_set_enable_v2,
};

static const struct rockchip_pwm_data pwm_data_vop = {
     .regs = {
                 .duty = 0x08,
                 .period = 0x04,
                 .cntr = 0x0c,
                 .ctrl = 0x00,
     },
     .prescaler = 1,
     .set_enable = rockchip_pwm_set_enable_v2,
};

Is that right?

>> +	.set_enable = rockchip_pwm_set_enable_v2,
>> +};
> No need for the double indirection.

Sorry, I think is need if you mean a double indirection for ".set_enable".



Caesar
>
> Thierry


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html