| Message ID | 1397466426-13870-2-git-send-email-cw00.choi@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jonathan, Any comment of this patchset? Best Regards, Chanwoo Choi On 04/14/2014 06:07 PM, 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_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@kernel.org> > Cc: Kukjin Kim <kgene.kim@samsung.com> > Cc: Naveen Krishna Chatradhi > Cc: linux-iio@vger.kernel.org > Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> > Acked-by: Kyungmin Park <kyungmin.park@samsung.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), > }; > > /* 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) { > 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); > + if (info->version == ADC_V3) > + clk_prepare_enable(info->sclk); > + > + } else { > + if (info->version == ADC_V3) > + clk_disable_unprepare(info->sclk); > + clk_disable_unprepare(info->clk); > + } > +} > + > static void exynos_adc_hw_init(struct exynos_adc *info) > { > u32 con1, con2; > > - if (info->version == ADC_V2) { > + if (info->version & ADC_V2) { > con1 = ADC_V2_CON1_SOFT_RESET; > writel(con1, ADC_V2_CON1(info->regs)); > > @@ -300,6 +317,8 @@ static int exynos_adc_probe(struct platform_device *pdev) > > writel(1, info->enable_reg); > > + info->version = exynos_adc_get_version(pdev); > + > info->clk = devm_clk_get(&pdev->dev, "adc"); > if (IS_ERR(info->clk)) { > dev_err(&pdev->dev, "failed getting clock, err = %ld\n", > @@ -308,6 +327,17 @@ static int exynos_adc_probe(struct platform_device *pdev) > goto err_irq; > } > > + if (info->version == ADC_V3) { > + info->sclk = devm_clk_get(&pdev->dev, "sclk_tsadc"); > + if (IS_ERR(info->sclk)) { > + dev_warn(&pdev->dev, > + "failed getting sclk clock, err = %ld\n", > + PTR_ERR(info->sclk)); > + ret = PTR_ERR(info->sclk); > + goto err_irq; > + } > + } > + > info->vdd = devm_regulator_get(&pdev->dev, "vdd"); > if (IS_ERR(info->vdd)) { > dev_err(&pdev->dev, "failed getting regulator, err = %ld\n", > @@ -316,8 +346,6 @@ static int exynos_adc_probe(struct platform_device *pdev) > goto err_irq; > } > > - info->version = exynos_adc_get_version(pdev); > - > platform_set_drvdata(pdev, indio_dev); > > indio_dev->name = dev_name(&pdev->dev); > @@ -340,7 +368,7 @@ static int exynos_adc_probe(struct platform_device *pdev) > if (ret) > goto err_iio_dev; > > - clk_prepare_enable(info->clk); > + exynos_adc_enable_clock(info, true); > > exynos_adc_hw_init(info); > > @@ -356,7 +384,7 @@ err_of_populate: > device_for_each_child(&pdev->dev, NULL, > exynos_adc_remove_devices); > regulator_disable(info->vdd); > - clk_disable_unprepare(info->clk); > + exynos_adc_enable_clock(info, false); > err_iio_dev: > iio_device_unregister(indio_dev); > err_irq: > @@ -372,7 +400,7 @@ static int exynos_adc_remove(struct platform_device *pdev) > device_for_each_child(&pdev->dev, NULL, > exynos_adc_remove_devices); > regulator_disable(info->vdd); > - clk_disable_unprepare(info->clk); > + exynos_adc_enable_clock(info, false); > writel(0, info->enable_reg); > iio_device_unregister(indio_dev); > free_irq(info->irq, info); > @@ -387,7 +415,7 @@ static int exynos_adc_suspend(struct device *dev) > struct exynos_adc *info = iio_priv(indio_dev); > u32 con; > > - if (info->version == ADC_V2) { > + if (info->version & ADC_V2) { > con = readl(ADC_V2_CON1(info->regs)); > con &= ~ADC_CON_EN_START; > writel(con, ADC_V2_CON1(info->regs)); > @@ -397,7 +425,7 @@ static int exynos_adc_suspend(struct device *dev) > writel(con, ADC_V1_CON(info->regs)); > } > > - clk_disable_unprepare(info->clk); > + exynos_adc_enable_clock(info, false); > writel(0, info->enable_reg); > regulator_disable(info->vdd); > > @@ -415,7 +443,7 @@ static int exynos_adc_resume(struct device *dev) > return ret; > > writel(1, info->enable_reg); > - clk_prepare_enable(info->clk); > + exynos_adc_enable_clock(info, true); > > exynos_adc_hw_init(info); > >
Hi Chanwoo, On 14 April 2014 14:37, Chanwoo Choi <cw00.choi@samsung.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@kernel.org> > Cc: Kukjin Kim <kgene.kim@samsung.com> > Cc: Naveen Krishna Chatradhi > Cc: linux-iio@vger.kernel.org > Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> > Acked-by: Kyungmin Park <kyungmin.park@samsung.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? > }; > > /* 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? > 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? > + if (info->version == ADC_V3) > + clk_prepare_enable(info->sclk); ditto. > + > + } else { > + if (info->version == ADC_V3) > + clk_disable_unprepare(info->sclk); > + clk_disable_unprepare(info->clk); > + } > +} > + > static void exynos_adc_hw_init(struct exynos_adc *info) > { > u32 con1, con2; > > - if (info->version == ADC_V2) { > + if (info->version & ADC_V2) { > con1 = ADC_V2_CON1_SOFT_RESET; > writel(con1, ADC_V2_CON1(info->regs)); > > @@ -300,6 +317,8 @@ static int exynos_adc_probe(struct platform_device *pdev) > > writel(1, info->enable_reg); > > + info->version = exynos_adc_get_version(pdev); > + > info->clk = devm_clk_get(&pdev->dev, "adc"); > if (IS_ERR(info->clk)) { > dev_err(&pdev->dev, "failed getting clock, err = %ld\n", > @@ -308,6 +327,17 @@ static int exynos_adc_probe(struct platform_device *pdev) > goto err_irq; > } > > + if (info->version == ADC_V3) { > + info->sclk = devm_clk_get(&pdev->dev, "sclk_tsadc"); > + if (IS_ERR(info->sclk)) { > + dev_warn(&pdev->dev, > + "failed getting sclk clock, err = %ld\n", > + PTR_ERR(info->sclk)); > + ret = PTR_ERR(info->sclk); nit: you could move this line above dev_warn and use 'ret' in the print statement.
Hi Sachin, On 04/16/2014 12:48 PM, Sachin Kamat wrote: > Hi Chanwoo, > > On 14 April 2014 14:37, Chanwoo Choi <cw00.choi@samsung.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@kernel.org> >> Cc: Kukjin Kim <kgene.kim@samsung.com> >> Cc: Naveen Krishna Chatradhi >> Cc: linux-iio@vger.kernel.org >> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >> Acked-by: Kyungmin Park <kyungmin.park@samsung.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? > > >> 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. > >> + if (info->version == ADC_V3) >> + clk_prepare_enable(info->sclk); > > ditto. ditto. > >> + >> + } else { >> + if (info->version == ADC_V3) >> + clk_disable_unprepare(info->sclk); >> + clk_disable_unprepare(info->clk); >> + } >> +} >> + >> static void exynos_adc_hw_init(struct exynos_adc *info) >> { >> u32 con1, con2; >> >> - if (info->version == ADC_V2) { >> + if (info->version & ADC_V2) { >> con1 = ADC_V2_CON1_SOFT_RESET; >> writel(con1, ADC_V2_CON1(info->regs)); >> >> @@ -300,6 +317,8 @@ static int exynos_adc_probe(struct platform_device *pdev) >> >> writel(1, info->enable_reg); >> >> + info->version = exynos_adc_get_version(pdev); >> + >> info->clk = devm_clk_get(&pdev->dev, "adc"); >> if (IS_ERR(info->clk)) { >> dev_err(&pdev->dev, "failed getting clock, err = %ld\n", >> @@ -308,6 +327,17 @@ static int exynos_adc_probe(struct platform_device *pdev) >> goto err_irq; >> } >> >> + if (info->version == ADC_V3) { >> + info->sclk = devm_clk_get(&pdev->dev, "sclk_tsadc"); >> + if (IS_ERR(info->sclk)) { >> + dev_warn(&pdev->dev, >> + "failed getting sclk clock, err = %ld\n", >> + PTR_ERR(info->sclk)); >> + ret = PTR_ERR(info->sclk); > > nit: you could move this line above dev_warn and use 'ret' in the print > statement. As I knew, usually show log meesage and then initialize return value. But If you find this code ugly, I can fix it. Thanks for your review. Best Regards, Chanwoo Choi
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@samsung.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@kernel.org> >>> Cc: Kukjin Kim <kgene.kim@samsung.com> >>> Cc: Naveen Krishna Chatradhi >>> Cc: linux-iio@vger.kernel.org >>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >>> Acked-by: Kyungmin Park <kyungmin.park@samsung.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. Best Regards, Chanwoo Choi
Hi Chanwoo, On 16 April 2014 10:25, Chanwoo Choi <cw00.choi@samsung.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@samsung.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@kernel.org> >>>> Cc: Kukjin Kim <kgene.kim@samsung.com> >>>> Cc: Naveen Krishna Chatradhi >>>> Cc: linux-iio@vger.kernel.org >>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.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). OK. Just a readability concern. Probably a check something like (version >= ADC_V2) would have made it more explicit. > >>> >>> >>>> 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? It is a good practice to check the return values for errors. Having said that it depends on your s/w design and the h/w requirements. If proceeding with the error does not cause any functional issues, then it is OK to ignore them. However I would atleast prefer to print a warning/info about such failures.
Hi Sachin, On 04/16/2014 02:04 PM, Sachin Kamat wrote: > Hi Chanwoo, > > On 16 April 2014 10:25, Chanwoo Choi <cw00.choi@samsung.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@samsung.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@kernel.org> >>>>> Cc: Kukjin Kim <kgene.kim@samsung.com> >>>>> Cc: Naveen Krishna Chatradhi >>>>> Cc: linux-iio@vger.kernel.org >>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >>>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.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). > > OK. Just a readability concern. Probably a check something like > (version >= ADC_V2) would > have made it more explicit. OK I'll modify it as your comment. (version >= ADC_V2) > >> >>>> >>>> >>>>> 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? > > It is a good practice to check the return values for errors. Having > said that it depends on > your s/w design and the h/w requirements. If proceeding with the error > does not cause any > functional issues, then it is OK to ignore them. However I would > atleast prefer to print > a warning/info about such failures. OK, I'll fix it. Thanks, Chanwoo Choi
On April 16, 2014 2:13:39 AM GMT+01:00, Chanwoo Choi <cw00.choi@samsung.com> wrote: >Hi Jonathan, > >Any comment of this patchset? > >Best Regards, >Chanwoo Choi Hi Chanwoo Not got to it yet I'm afraid. May be sometime next week before I do. Jonathan > >On 04/14/2014 06:07 PM, 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_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@kernel.org> >> Cc: Kukjin Kim <kgene.kim@samsung.com> >> Cc: Naveen Krishna Chatradhi >> Cc: linux-iio@vger.kernel.org >> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >> Acked-by: Kyungmin Park <kyungmin.park@samsung.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), >> }; >> >> /* 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) { >> 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); >> + if (info->version == ADC_V3) >> + clk_prepare_enable(info->sclk); >> + >> + } else { >> + if (info->version == ADC_V3) >> + clk_disable_unprepare(info->sclk); >> + clk_disable_unprepare(info->clk); >> + } >> +} >> + >> static void exynos_adc_hw_init(struct exynos_adc *info) >> { >> u32 con1, con2; >> >> - if (info->version == ADC_V2) { >> + if (info->version & ADC_V2) { >> con1 = ADC_V2_CON1_SOFT_RESET; >> writel(con1, ADC_V2_CON1(info->regs)); >> >> @@ -300,6 +317,8 @@ static int exynos_adc_probe(struct >platform_device *pdev) >> >> writel(1, info->enable_reg); >> >> + info->version = exynos_adc_get_version(pdev); >> + >> info->clk = devm_clk_get(&pdev->dev, "adc"); >> if (IS_ERR(info->clk)) { >> dev_err(&pdev->dev, "failed getting clock, err = %ld\n", >> @@ -308,6 +327,17 @@ static int exynos_adc_probe(struct >platform_device *pdev) >> goto err_irq; >> } >> >> + if (info->version == ADC_V3) { >> + info->sclk = devm_clk_get(&pdev->dev, "sclk_tsadc"); >> + if (IS_ERR(info->sclk)) { >> + dev_warn(&pdev->dev, >> + "failed getting sclk clock, err = %ld\n", >> + PTR_ERR(info->sclk)); >> + ret = PTR_ERR(info->sclk); >> + goto err_irq; >> + } >> + } >> + >> info->vdd = devm_regulator_get(&pdev->dev, "vdd"); >> if (IS_ERR(info->vdd)) { >> dev_err(&pdev->dev, "failed getting regulator, err = %ld\n", >> @@ -316,8 +346,6 @@ static int exynos_adc_probe(struct >platform_device *pdev) >> goto err_irq; >> } >> >> - info->version = exynos_adc_get_version(pdev); >> - >> platform_set_drvdata(pdev, indio_dev); >> >> indio_dev->name = dev_name(&pdev->dev); >> @@ -340,7 +368,7 @@ static int exynos_adc_probe(struct >platform_device *pdev) >> if (ret) >> goto err_iio_dev; >> >> - clk_prepare_enable(info->clk); >> + exynos_adc_enable_clock(info, true); >> >> exynos_adc_hw_init(info); >> >> @@ -356,7 +384,7 @@ err_of_populate: >> device_for_each_child(&pdev->dev, NULL, >> exynos_adc_remove_devices); >> regulator_disable(info->vdd); >> - clk_disable_unprepare(info->clk); >> + exynos_adc_enable_clock(info, false); >> err_iio_dev: >> iio_device_unregister(indio_dev); >> err_irq: >> @@ -372,7 +400,7 @@ static int exynos_adc_remove(struct >platform_device *pdev) >> device_for_each_child(&pdev->dev, NULL, >> exynos_adc_remove_devices); >> regulator_disable(info->vdd); >> - clk_disable_unprepare(info->clk); >> + exynos_adc_enable_clock(info, false); >> writel(0, info->enable_reg); >> iio_device_unregister(indio_dev); >> free_irq(info->irq, info); >> @@ -387,7 +415,7 @@ static int exynos_adc_suspend(struct device *dev) >> struct exynos_adc *info = iio_priv(indio_dev); >> u32 con; >> >> - if (info->version == ADC_V2) { >> + if (info->version & ADC_V2) { >> con = readl(ADC_V2_CON1(info->regs)); >> con &= ~ADC_CON_EN_START; >> writel(con, ADC_V2_CON1(info->regs)); >> @@ -397,7 +425,7 @@ static int exynos_adc_suspend(struct device *dev) >> writel(con, ADC_V1_CON(info->regs)); >> } >> >> - clk_disable_unprepare(info->clk); >> + exynos_adc_enable_clock(info, false); >> writel(0, info->enable_reg); >> regulator_disable(info->vdd); >> >> @@ -415,7 +443,7 @@ static int exynos_adc_resume(struct device *dev) >> return ret; >> >> writel(1, info->enable_reg); >> - clk_prepare_enable(info->clk); >> + exynos_adc_enable_clock(info, true); >> >> exynos_adc_hw_init(info); >> >>
On April 16, 2014 5:55:17 AM GMT+01:00, Chanwoo Choi <cw00.choi@samsung.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@samsung.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@kernel.org> >>>> Cc: Kukjin Kim <kgene.kim@samsung.com> >>>> Cc: Naveen Krishna Chatradhi >>>> Cc: linux-iio@vger.kernel.org >>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.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. > >Best Regards, >Chanwoo Choi >-- >To unsubscribe from this list: send the line "unsubscribe linux-iio" in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html
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@samsung.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@samsung.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@kernel.org> >>>>> Cc: Kukjin Kim <kgene.kim@samsung.com> >>>>> Cc: Naveen Krishna Chatradhi >>>>> Cc: linux-iio@vger.kernel.org >>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >>>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.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
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), }; /* 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) { 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); + if (info->version == ADC_V3) + clk_prepare_enable(info->sclk); + + } else { + if (info->version == ADC_V3) + clk_disable_unprepare(info->sclk); + clk_disable_unprepare(info->clk); + } +} + static void exynos_adc_hw_init(struct exynos_adc *info) { u32 con1, con2; - if (info->version == ADC_V2) { + if (info->version & ADC_V2) { con1 = ADC_V2_CON1_SOFT_RESET; writel(con1, ADC_V2_CON1(info->regs)); @@ -300,6 +317,8 @@ static int exynos_adc_probe(struct platform_device *pdev) writel(1, info->enable_reg); + info->version = exynos_adc_get_version(pdev); + info->clk = devm_clk_get(&pdev->dev, "adc"); if (IS_ERR(info->clk)) { dev_err(&pdev->dev, "failed getting clock, err = %ld\n", @@ -308,6 +327,17 @@ static int exynos_adc_probe(struct platform_device *pdev) goto err_irq; } + if (info->version == ADC_V3) { + info->sclk = devm_clk_get(&pdev->dev, "sclk_tsadc"); + if (IS_ERR(info->sclk)) { + dev_warn(&pdev->dev, + "failed getting sclk clock, err = %ld\n", + PTR_ERR(info->sclk)); + ret = PTR_ERR(info->sclk); + goto err_irq; + } + } + info->vdd = devm_regulator_get(&pdev->dev, "vdd"); if (IS_ERR(info->vdd)) { dev_err(&pdev->dev, "failed getting regulator, err = %ld\n", @@ -316,8 +346,6 @@ static int exynos_adc_probe(struct platform_device *pdev) goto err_irq; } - info->version = exynos_adc_get_version(pdev); - platform_set_drvdata(pdev, indio_dev); indio_dev->name = dev_name(&pdev->dev); @@ -340,7 +368,7 @@ static int exynos_adc_probe(struct platform_device *pdev) if (ret) goto err_iio_dev; - clk_prepare_enable(info->clk); + exynos_adc_enable_clock(info, true); exynos_adc_hw_init(info); @@ -356,7 +384,7 @@ err_of_populate: device_for_each_child(&pdev->dev, NULL, exynos_adc_remove_devices); regulator_disable(info->vdd); - clk_disable_unprepare(info->clk); + exynos_adc_enable_clock(info, false); err_iio_dev: iio_device_unregister(indio_dev); err_irq: @@ -372,7 +400,7 @@ static int exynos_adc_remove(struct platform_device *pdev) device_for_each_child(&pdev->dev, NULL, exynos_adc_remove_devices); regulator_disable(info->vdd); - clk_disable_unprepare(info->clk); + exynos_adc_enable_clock(info, false); writel(0, info->enable_reg); iio_device_unregister(indio_dev); free_irq(info->irq, info); @@ -387,7 +415,7 @@ static int exynos_adc_suspend(struct device *dev) struct exynos_adc *info = iio_priv(indio_dev); u32 con; - if (info->version == ADC_V2) { + if (info->version & ADC_V2) { con = readl(ADC_V2_CON1(info->regs)); con &= ~ADC_CON_EN_START; writel(con, ADC_V2_CON1(info->regs)); @@ -397,7 +425,7 @@ static int exynos_adc_suspend(struct device *dev) writel(con, ADC_V1_CON(info->regs)); } - clk_disable_unprepare(info->clk); + exynos_adc_enable_clock(info, false); writel(0, info->enable_reg); regulator_disable(info->vdd); @@ -415,7 +443,7 @@ static int exynos_adc_resume(struct device *dev) return ret; writel(1, info->enable_reg); - clk_prepare_enable(info->clk); + exynos_adc_enable_clock(info, true); exynos_adc_hw_init(info);