| 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
| ||
|
Message-ID: <53D06BE3.7010509@rock-chips.com>
Date: Thu, 24 Jul 2014 10:13:55 +0800
From: caesar <caesar.wang@...k-chips.com>
To: Heiko Stübner <heiko@...ech.de>
CC: thierry.reding@...il.com, b.galvani@...il.com,
linux-arm-kernel@...ts.infradead.org, linux-pwm@...r.kernel.org,
linux-kernel@...r.kernel.org, cf@...k-chips.com,
huangtao@...k-chips.com, hj@...k-chips.com, xjq@...k-chips.com,
addy.ke@...k-chips.com
Subject: Re: [PATCH v3 2/2] pwm: rockchip: Added to support for RK3288 SoC
Hi Heiko & thierry,
Thank you for your suggestion.
于 2014年07月24日 00:01, Heiko Stübner 写道:
> Hi Caesar.
>
> Am Mittwoch, 23. Juli 2014, 14:38:41 schrieb Caesar Wang:
>> This patch added to support the PWM controller found on
>> RK3288 SoC.
>>
>> Signed-off-by: Caesar Wang <caesar.wang@...k-chips.com>
>> ---
>> drivers/pwm/pwm-rockchip.c | 141
>> +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 122
>> insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
>> index eec2145..8d72a98 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@...il.com>
>> + * Copyright (C) 2014 Caesar Wang <caesar.wang@...k-chips.com>
> you might want to check who actually holds the copyright for your
> contributions, I guess a "Copyright (C) 2014 Rockchip"-something would be more
> appropriate?
>
Yes,you are right.
>> *
>> * 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,89 @@
>>
>> #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 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.
>
>
> Heiko
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.
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",
>
>
>
--
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/
Powered by blists - more mailing lists