From mboxrd@z Thu Jan  1 00:00:00 1970
From: heiko@sntech.de (Heiko =?ISO-8859-1?Q?St=FCbner?=)
Date: Thu, 24 Jul 2014 10:05:42 +0200
Subject: [PATCH v3 2/2] pwm: rockchip: Added to support for RK3288 SoC
In-Reply-To: <53D06BE3.7010509@rock-chips.com>
References: <1406097521-6457-1-git-send-email-caesar.wang@rock-chips.com>
 <1648580.P46ct6rREY@diego> <53D06BE3.7010509@rock-chips.com>
Message-ID: <1949437.1N5XrQ9a4Y@diego>
To: linux-arm-kernel@lists.infradead.org
List-Id: linux-arm-kernel.lists.infradead.org

Hi caesar.

Am Donnerstag, 24. Juli 2014, 10:13:55 schrieb caesar:
> >> +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 void rockchip_pwm_set_enable_vop(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);
> >> +}
> > 
> > not sure if I'm just blind ... do rockchip_pwm_set_enable_v2 and
> > rockchip_pwm_set_enable_vop differ at all?
> > 
> > If they don't differ, I guess pwm_data_vop should just use
> > rockchip_pwm_set_enable_v2 instead of duplicating it.
> > 
> 
> Yes, the rockchip_pwm_set_enable_v1 & v2 & vop is similar.
> 
> So my v2 patch use "u32 enable_conf" instead of it .
> +struct rockchip_pwm_data {
> 
>  > + .........
>  > + u32 enable_conf;
>  > +};
> 
> The thierry has suggested it [1] in my v2 patch:
> 
> 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.
> 
> 
> Perhaps,thierry's suggestion I got it wrong.

Using the function pointers like Thierry suggested looks nice, so no I don't 
think you got it wrong :-)

What I meant was to simply reuse the existing function 
rockchip_pwm_set_enable_v2 when there is _no_ difference at all to 
rockchip_pwm_set_enable_vop, like

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,
	.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,
	.set_enable = rockchip_pwm_set_enable_v2,
};


Heiko

> 
> Hi thierry& Heiko :-)
> Maybe,could you suggest solve it reasonable? thanks.
> 
> [1]: https://lkml.org/lkml/2014/7/21/113
> 
> >> +
> >> 
> >>   static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device
> >> 
> >> *pwm, int duty_ns, int period_ns)
> >> 
> >>   {
> >> 
> >> @@ -52,20 +126,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 +150,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 +163,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 +176,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,
> >> +	.prescaler = PRESCALER,
> >> +	.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,
> >> +	.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,
> >> +	.set_enable = rockchip_pwm_set_enable_vop,
> >> +};
> >> +
> >> +static const struct of_device_id rockchip_pwm_dt_ids[] = {
> >> +	{ .compatible = "rockchip,rk2928-pwm", .data = &pwm_data_v1},
> >> +	{ .compatible = "rockchip,rk3288-pwm", .data = &pwm_data_v2},
> >> +	{ .compatible = "rockchip,vop-pwm", .data = &pwm_data_vop},
> >> +	{ /* sentinel */ }
> >> +};
> >> +MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids);
> >> +
> >> 
> >>   static int rockchip_pwm_probe(struct platform_device *pdev)
> >>   {
> >> 
> >> +	const struct of_device_id *id;
> >> 
> >>   	struct rockchip_pwm_chip *pc;
> >>   	struct resource *r;
> >>   	int ret;
> >> 
> >> +	id = of_match_device(rockchip_pwm_dt_ids, &pdev->dev);
> >> +	if (!id)
> >> +		return -EINVAL;
> >> +
> >> 
> >>   	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> >>   	if (!pc)
> >>   	
> >>   		return -ENOMEM;
> >> 
> >> @@ -133,6 +241,7 @@ static int rockchip_pwm_probe(struct platform_device
> >> *pdev)
> >> 
> >>   	platform_set_drvdata(pdev, pc);
> >> 
> >> +	pc->data = id->data;
> >> 
> >>   	pc->chip.dev = &pdev->dev;
> >>   	pc->chip.ops = &rockchip_pwm_ops;
> >>   	pc->chip.base = -1;
> >> 
> >> @@ -156,12 +265,6 @@ static int rockchip_pwm_remove(struct
> >> platform_device
> >> *pdev) return pwmchip_remove(&pc->chip);
> >> 
> >>   }
> >> 
> >> -static const struct of_device_id rockchip_pwm_dt_ids[] = {
> >> -	{ .compatible = "rockchip,rk2928-pwm" },
> >> -	{ /* sentinel */ }
> >> -};
> >> -MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids);
> >> -
> >> 
> >>   static struct platform_driver rockchip_pwm_driver = {
> >>   
> >>   	.driver = {
> >>   	
> >>   		.name = "rockchip-pwm",