| 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: <534E3BDC.5020009@samsung.com>
Date: Wed, 16 Apr 2014 17:14:20 +0900
From: Chanwoo Choi <cw00.choi@...sung.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Sachin Kamat <sachin.kamat@...aro.org>,
naveen krishna <ch.naveen@...sung.com>,
Kukjin Kim <kgene.kim@...sung.com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>, rdunlap@...radead.org,
Tomasz Figa <t.figa@...sung.com>, linux-iio@...r.kernel.org,
linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
linux-doc@...r.kernel.org
Subject: Re: [PATCHv2 1/2] iio: adc: exynos_adc: Control special clock of ADC
to support Exynos3250 ADC
Hi Jonathan,
On 04/16/2014 04:05 PM, Jonathan Cameron wrote:
>
>
> On April 16, 2014 5:55:17 AM GMT+01:00, Chanwoo Choi <cw00.choi@...sung.com> wrote:
>> Hi Sachin,
>>
>> On 04/16/2014 01:44 PM, Chanwoo Choi wrote:
>>> Hi Sachin,
>>>
>>> On 04/16/2014 12:48 PM, Sachin Kamat wrote:
>>>> Hi Chanwoo,
>>>>
>>>> On 14 April 2014 14:37, Chanwoo Choi <cw00.choi@...sung.com> 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_tsadc' clock as following:
>>>>> - 'sclk_tsadc' clock: special clock for ADC which provide clock to
>> internal ADC
>>>>>
>>>>> Exynos 4210/4212/4412 and Exynos5250/5420 has not included
>> 'sclk_tsadc' clock
>>>>> in FSYS_BLK. But, Exynos3250 based on Cortex-A7 has only included
>> 'sclk_tsadc'
>>>>> clock in FSYS_BLK.
>>>>>
>>>>> Cc: Jonathan Cameron <jic23@...nel.org>
>>>>> Cc: Kukjin Kim <kgene.kim@...sung.com>
>>>>> Cc: Naveen Krishna Chatradhi
>>>>> Cc: linux-iio@...r.kernel.org
>>>>> Signed-off-by: Chanwoo Choi <cw00.choi@...sung.com>
>>>>> Acked-by: Kyungmin Park <kyungmin.park@...sung.com>
>>>>> ---
>>>>> drivers/iio/adc/exynos_adc.c | 54
>> +++++++++++++++++++++++++++++++++-----------
>>>>> 1 file changed, 41 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/adc/exynos_adc.c
>> b/drivers/iio/adc/exynos_adc.c
>>>>> index d25b262..3c99243 100644
>>>>> --- a/drivers/iio/adc/exynos_adc.c
>>>>> +++ b/drivers/iio/adc/exynos_adc.c
>>>>> @@ -40,8 +40,9 @@
>>>>> #include <linux/iio/driver.h>
>>>>>
>>>>> enum adc_version {
>>>>> - ADC_V1,
>>>>> - ADC_V2
>>>>> + ADC_V1 = 0x1,
>>>>> + ADC_V2 = 0x2,
>>>>> + ADC_V3 = (ADC_V1 | ADC_V2),
>>>>
>>>> Can't this be simply 0x3? Or is this not really a h/w version?
>>>
>>> Even thought ADC_V3 isn't h/w revision, ADC_V3 include all featues of
>> ADC_V2
>>> and only one difference of clock(sclk_tsadc) from ADC_V2.
>>> I want to describethat ADC_V3 include ADC_V2 feature So, I add as
>> following:
>>> >> + ADC_V3 = (ADC_V1 | ADC_V2),
>>>
>>>>
>>>>> };
>>>>>
>>>>> /* EXYNOS4412/5250 ADC_V1 registers definitions */
>>>>> @@ -88,6 +89,7 @@ struct exynos_adc {
>>>>> void __iomem *regs;
>>>>> void __iomem *enable_reg;
>>>>> struct clk *clk;
>>>>> + struct clk *sclk;
>>>>> unsigned int irq;
>>>>> struct regulator *vdd;
>>>>>
>>>>> @@ -100,6 +102,7 @@ struct exynos_adc {
>>>>> 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-v3", .data = (void
>> *)ADC_V3 },
>>>>> {},
>>>>> };
>>>>> MODULE_DEVICE_TABLE(of, exynos_adc_match);
>>>>> @@ -128,7 +131,7 @@ static int exynos_read_raw(struct iio_dev
>> *indio_dev,
>>>>> mutex_lock(&indio_dev->mlock);
>>>>>
>>>>> /* Select the channel to be used and Trigger conversion */
>>>>> - if (info->version == ADC_V2) {
>>>>> + if (info->version & ADC_V2) {
>>>>
>>>> So, now this would be applicable for ADC_V3 too, right?
>>
>> ADC_V3 isn't h/w version. So, I think this code is proper instead of
>> using ADC_V3 direclty.
>> I want to use ADC_V3 version on checking clock(sclk_tsadc).
>>
>>>>
>>>>
>>>>> con2 = readl(ADC_V2_CON2(info->regs));
>>>>> con2 &= ~ADC_V2_CON2_ACH_MASK;
>>>>> con2 |= ADC_V2_CON2_ACH_SEL(chan->address);
>>>>> @@ -165,7 +168,7 @@ static irqreturn_t exynos_adc_isr(int irq, void
>> *dev_id)
>>>>> info->value = readl(ADC_V1_DATX(info->regs)) &
>>>>> ADC_DATX_MASK;
>>>>> /* clear irq */
>>>>> - if (info->version == ADC_V2)
>>>>> + if (info->version & ADC_V2)
>>>>> writel(1, ADC_V2_INT_ST(info->regs));
>>>>> else
>>>>> writel(1, ADC_V1_INTCLR(info->regs));
>>>>> @@ -226,11 +229,25 @@ static int exynos_adc_remove_devices(struct
>> device *dev, void *c)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +static void exynos_adc_enable_clock(struct exynos_adc *info, bool
>> enable)
>>>>> +{
>>>>> + if (enable) {
>>>>> + clk_prepare_enable(info->clk);
>>>>
>>>> This could fail. Is it OK without any checks?
>>>
>>> OK, I'll check return value.
>>
>> Do you want to check return value always?
>> I think again, Some device drivers in mainline would not check
>> return value of clock function. If maintainer confirm this
>> modification,
>> I'll fix it as your comment.
> Its general good practice to check all return values. Even if a function doesn't return an
> error now, it might in future. There is lots of old or lazy code out there doing many much
> stranger things than this!
>
> So yes, please check return values and pass on up the call stack if an error.
OK, I'll check return value of clock function. Thanks.
Best Regards,
Chanwoo Choi
--
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