| 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: <53D0D212.40400@rock-chips.com>
Date: Thu, 24 Jul 2014 17:29:54 +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,
在 2014年07月24日 16:05, Heiko Stübner 写道:
> 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
:-( ok, I will fix this and the other issuses in v4, thanks.
>> 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