| 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: <53A380AD.3090307@gmail.com>
Date: Fri, 20 Jun 2014 02:30:37 +0200
From: Tomasz Figa <tomasz.figa@...il.com>
To: Chanwoo Choi <cw00.choi@...sung.com>
CC: jic23@...nel.org, ch.naveen@...sung.com, t.figa@...sung.com,
kgene.kim@...sung.com, robh+dt@...nel.org, pawel.moll@....com,
mark.rutland@....com, ijc+devicetree@...lion.org.uk,
galak@...eaurora.org, rdunlap@...radead.org,
sachin.kamat@...aro.org, linux-iio@...r.kernel.org,
linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCHv4 2/4] iio: adc: exynos_adc: Control special clock of
ADC to support Exynos3250 ADC
On 20.06.2014 02:28, Chanwoo Choi wrote:
> On 06/20/2014 09:24 AM, Tomasz Figa wrote:
>> On 20.06.2014 02:22, Chanwoo Choi wrote:
>>> Hi Tomasz,
>>>
>>> On 06/18/2014 04:58 PM, Tomasz Figa wrote:
>>>> Hi Chanwoo,
>>>>
>>>> On 18.06.2014 04:20, Chanwoo Choi wrote:
>>>>> This patch control special clock for ADC in Exynos series's FSYS block.
>>>>> If special clock of ADC is registerd on clock list of common clk framework,
>>>>> Exynos ADC drvier have to control this clock.
>>>>>
>>>>> Exynos3250/Exynos4/Exynos5 has 'adc' clock as following:
>>>>> - 'adc' clock: bus clock for ADC
>>>>>
>>>>> Exynos3250 has additional 'sclk_adc' clock as following:
>>>>> - 'sclk_adc' clock: special clock for ADC which provide clock to internal ADC
>>>>>
>>>>> Exynos 4210/4212/4412 and Exynos5250/5420 has not included 'sclk_adc' clock
>>>>> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included 'sclk_adc'
>>>>> clock in FSYS_BLK.
>>>>>
>>>>> Signed-off-by: Chanwoo Choi <cw00.choi@...sung.com>
>>>>> Acked-by: Kyungmin Park <kyungmin.park@...sung.com>
>>>>> ---
>>>>> drivers/iio/adc/exynos_adc.c | 93 ++++++++++++++++++++++++++++++++++++++------
>>>>> 1 file changed, 81 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
>>>>> index c30def6..6b026ac 100644
>>>>> --- a/drivers/iio/adc/exynos_adc.c
>>>>> +++ b/drivers/iio/adc/exynos_adc.c
>>>>> @@ -41,7 +41,8 @@
>>>>>
>>>>> enum adc_version {
>>>>> ADC_V1,
>>>>> - ADC_V2
>>>>> + ADC_V2,
>>>>> + ADC_V2_EXYNOS3250,
>>>>> };
>>>>>
>>>>> /* EXYNOS4412/5250 ADC_V1 registers definitions */
>>>>> @@ -85,9 +86,11 @@ enum adc_version {
>>>>> #define EXYNOS_ADC_TIMEOUT (msecs_to_jiffies(100))
>>>>>
>>>>> struct exynos_adc {
>>>>> + struct device *dev;
>>>>> void __iomem *regs;
>>>>> void __iomem *enable_reg;
>>>>> struct clk *clk;
>>>>> + struct clk *sclk;
>>>>> unsigned int irq;
>>>>> struct regulator *vdd;
>>>>> struct exynos_adc_ops *ops;
>>>>> @@ -96,6 +99,7 @@ struct exynos_adc {
>>>>>
>>>>> u32 value;
>>>>> unsigned int version;
>>>>> + bool needs_sclk;
>>>>
>>>> This should be rather a part of the variant struct. See my comments to
>>>> patch 1/4.
>>>
>>> OK, I'll include 'needs_sclk' in "variant" structure.
>>>
>>>>
>>>>> };
>>>>>
>>>>> struct exynos_adc_ops {
>>>>> @@ -103,11 +107,21 @@ struct exynos_adc_ops {
>>>>> void (*clear_irq)(struct exynos_adc *info);
>>>>> void (*start_conv)(struct exynos_adc *info, unsigned long addr);
>>>>> void (*stop_conv)(struct exynos_adc *info);
>>>>> + void (*disable_clk)(struct exynos_adc *info);
>>>>> + int (*enable_clk)(struct exynos_adc *info);
>>>>> };
>>>>>
>>>>> static const struct of_device_id exynos_adc_match[] = {
>>>>> - { .compatible = "samsung,exynos-adc-v1", .data = (void *)ADC_V1 },
>>>>> - { .compatible = "samsung,exynos-adc-v2", .data = (void *)ADC_V2 },
>>>>> + {
>>>>> + .compatible = "samsung,exynos-adc-v1",
>>>>> + .data = (void *)ADC_V1,
>>>>> + }, {
>>>>> + .compatible = "samsung,exynos-adc-v2",
>>>>> + .data = (void *)ADC_V2,
>>>>> + }, {
>>>>> + .compatible = "samsung,exynos3250-adc-v2",
>>>>> + .data = (void *)ADC_V2_EXYNOS3250,
>>>>> + },
>>>>> {},
>>>>> };
>>>>> MODULE_DEVICE_TABLE(of, exynos_adc_match);
>>>>> @@ -156,11 +170,42 @@ static void exynos_adc_v1_stop_conv(struct exynos_adc *info)
>>>>> writel(con, ADC_V1_CON(info->regs));
>>>>> }
>>>>>
>>>>> +static void exynos_adc_disable_clk(struct exynos_adc *info)
>>>>> +{
>>>>> + if (info->needs_sclk)
>>>>> + clk_disable_unprepare(info->sclk);
>>>>> + clk_disable_unprepare(info->clk);
>>>>> +}
>>>>> +
>>>>> +static int exynos_adc_enable_clk(struct exynos_adc *info)
>>>>> +{
>>>>> + int ret;
>>>>> +
>>>>> + ret = clk_prepare_enable(info->clk);
>>>>> + if (ret) {
>>>>> + dev_err(info->dev, "failed enabling adc clock: %d\n", ret);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + if (info->needs_sclk) {
>>>>> + ret = clk_prepare_enable(info->sclk);
>>>>> + if (ret) {
>>>>> + clk_disable_unprepare(info->clk);
>>>>> + dev_err(info->dev,
>>>>> + "failed enabling sclk_tsadc clock: %d\n", ret);
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> static struct exynos_adc_ops exynos_adc_v1_ops = {
>>>>> .init_hw = exynos_adc_v1_init_hw,
>>>>> .clear_irq = exynos_adc_v1_clear_irq,
>>>>> .start_conv = exynos_adc_v1_start_conv,
>>>>> .stop_conv = exynos_adc_v1_stop_conv,
>>>>> + .disable_clk = exynos_adc_disable_clk,
>>>>> + .enable_clk = exynos_adc_enable_clk,
>>>>> };
>>>>>
>>>>> static void exynos_adc_v2_init_hw(struct exynos_adc *info)
>>>>> @@ -210,6 +255,8 @@ static struct exynos_adc_ops exynos_adc_v2_ops = {
>>>>> .start_conv = exynos_adc_v2_start_conv,
>>>>> .clear_irq = exynos_adc_v2_clear_irq,
>>>>> .stop_conv = exynos_adc_v2_stop_conv,
>>>>> + .disable_clk = exynos_adc_disable_clk,
>>>>> + .enable_clk = exynos_adc_enable_clk,
>>>>
>>>> Based on the fact that all variants use the same function, I don't think
>>>> there is a reason to add .{disable,enable}_clk in the ops struct. If
>>>> they diverge in future, they could be added later, but right now it
>>>> doesn't have any value.
>>>
>>> OK, I'll not add .{disable,enable}_clk and then just use following functions for clock control:
>>> - exynos_adc_prepare_clk() : once execute this function in _probe()
>>> - exynos_adc_unprepare_clk() : once execute this function in _remove()
>>> - exynos_adc_enable_clk()
>>> - exynos_adc_disable_clk()
>>
>> Is there any need to separate prepare/unprepare from enable/disable?
>> Otherwise sounds good, thanks.
>
> Naveen Krishna Chatradhi want to execute once prepare/unpreare in probe/remove function.
>
> - Following comment of Naveen Krishna Chatradhi
>> +static void exynos_adc_disable_clk(struct exynos_adc *info)
>> +{
>> + if (info->needs_sclk)
>> + clk_disable_unprepare(info->sclk);
>> + clk_disable_unprepare(info->clk);
>
> (Just a nit pick) As a part of cleanup can we also change to use
> clk_disable() here and clk_unprepare() once and for all at the end.
>
>> +}
>> +
>> +static int exynos_adc_enable_clk(struct exynos_adc *info)
>> +{
>> + int ret;
>> +
>> + ret = clk_prepare_enable(info->clk);
>> + if (ret) {
>> + dev_err(info->dev, "failed enabling adc clock: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + if (info->needs_sclk) {
>> + ret = clk_prepare_enable(info->sclk);
> Can we use clk_enable() here and clk_prepare() some where during the probe.
I still don't see any reason to do it. Naveen, what's the motivation for
this change? For me, it only complicates the code, without any added value.
Best regards,
Tomasz
--
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