| Message ID | 1405663186-26464-3-git-send-email-cw00.choi@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday 18 July 2014 14:59:44 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. Do you know if any of the older ADC blocks have an "sclk" input as well? Further, why is it called "sclk_adc" rather than just "sclk"? > @@ -199,13 +262,20 @@ static void exynos_adc_v2_start_conv(struct exynos_adc *info, > writel(con1 | ADC_CON_EN_START, ADC_V2_CON1(info->regs)); > } > > +#define __EXYNOS_ADC_V2_DATA \ > + .num_channels = MAX_ADC_V2_CHANNELS, \ > + .init_hw = exynos_adc_v2_init_hw, \ > + .exit_hw = exynos_adc_v2_exit_hw, \ > + .clear_irq = exynos_adc_v2_clear_irq, \ > + .start_conv = exynos_adc_v2_start_conv, \ > + > static struct exynos_adc_data const exynos_adc_v2_data = { > - .num_channels = MAX_ADC_V2_CHANNELS, > + __EXYNOS_ADC_V2_DATA > +}; > > - .init_hw = exynos_adc_v2_init_hw, > - .exit_hw = exynos_adc_v2_exit_hw, > - .clear_irq = exynos_adc_v2_clear_irq, > - .start_conv = exynos_adc_v2_start_conv, > +static struct exynos_adc_data const exynos3250_adc_v2_data = { > + __EXYNOS_ADC_V2_DATA > + .needs_sclk = true, > }; I think the macro hurts readability. Please just duplicate the definition here. > static const struct of_device_id exynos_adc_match[] = { > @@ -215,6 +285,9 @@ static const struct of_device_id exynos_adc_match[] = { > }, { > .compatible = "samsung,exynos-adc-v2", > .data = (void *)&exynos_adc_v2_data, > + }, { > + .compatible = "samsung,exynos3250-adc-v2", > + .data = (void *)&exynos3250_adc_v2_data, > }, > {}, Remove the '(void *)' cast here and mark the structure as 'const'. We intentionally use a 'const void *' type here to verify that the driver doesn't modify the per-device type data at runtime, which would be bad if you ever have multiple device instances. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, On 07/18/2014 06:47 PM, Arnd Bergmann wrote: > On Friday 18 July 2014 14:59:44 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. > > Do you know if any of the older ADC blocks have an "sclk" input as well? No, I didn't check older ADC blocks. I only checked it on Exynos3250, Exynos4210/4212/4412, Exynos5250/5420. > > Further, why is it called "sclk_adc" rather than just "sclk"? The sclk means 'special clock' in Exynos TRM. Exynos SoC has varisou sclk clocks. 'sclk_adc' is only used for ADC IP. > >> @@ -199,13 +262,20 @@ static void exynos_adc_v2_start_conv(struct exynos_adc *info, >> writel(con1 | ADC_CON_EN_START, ADC_V2_CON1(info->regs)); >> } >> >> +#define __EXYNOS_ADC_V2_DATA \ >> + .num_channels = MAX_ADC_V2_CHANNELS, \ >> + .init_hw = exynos_adc_v2_init_hw, \ >> + .exit_hw = exynos_adc_v2_exit_hw, \ >> + .clear_irq = exynos_adc_v2_clear_irq, \ >> + .start_conv = exynos_adc_v2_start_conv, \ >> + >> static struct exynos_adc_data const exynos_adc_v2_data = { >> - .num_channels = MAX_ADC_V2_CHANNELS, >> + __EXYNOS_ADC_V2_DATA >> +}; >> >> - .init_hw = exynos_adc_v2_init_hw, >> - .exit_hw = exynos_adc_v2_exit_hw, >> - .clear_irq = exynos_adc_v2_clear_irq, >> - .start_conv = exynos_adc_v2_start_conv, >> +static struct exynos_adc_data const exynos3250_adc_v2_data = { >> + __EXYNOS_ADC_V2_DATA >> + .needs_sclk = true, >> }; > > I think the macro hurts readability. Please just duplicate the definition > here. OK, I'll fix it. > >> static const struct of_device_id exynos_adc_match[] = { >> @@ -215,6 +285,9 @@ static const struct of_device_id exynos_adc_match[] = { >> }, { >> .compatible = "samsung,exynos-adc-v2", >> .data = (void *)&exynos_adc_v2_data, >> + }, { >> + .compatible = "samsung,exynos3250-adc-v2", >> + .data = (void *)&exynos3250_adc_v2_data, >> }, >> {}, > > Remove the '(void *)' cast here and mark the structure as 'const'. > We intentionally use a 'const void *' type here to verify that > the driver doesn't modify the per-device type data at runtime, > which would be bad if you ever have multiple device instances. OK, I'll remove it. Thanks, Chanwoo Choi -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 18 July 2014 19:00:48 Chanwoo Choi wrote: > On 07/18/2014 06:47 PM, Arnd Bergmann wrote: > > > > Further, why is it called "sclk_adc" rather than just "sclk"? > > The sclk means 'special clock' in Exynos TRM. Exynos SoC has varisou sclk clocks. > 'sclk_adc' is only used for ADC IP. But that sounds like sclk_adc is the name of the global name of the clock signal coming out of the clock controller. I still think it would be best to name it 'sclk' as the input for the adc. It shouldn't rely on a particular name of the clock controller. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 18, 2014 at 8:14 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Friday 18 July 2014 19:00:48 Chanwoo Choi wrote: >> On 07/18/2014 06:47 PM, Arnd Bergmann wrote: >> > >> > Further, why is it called "sclk_adc" rather than just "sclk"? >> >> The sclk means 'special clock' in Exynos TRM. Exynos SoC has varisou sclk clocks. >> 'sclk_adc' is only used for ADC IP. > > But that sounds like sclk_adc is the name of the global name > of the clock signal coming out of the clock controller. > > I still think it would be best to name it 'sclk' as the input > for the adc. It shouldn't rely on a particular name of the > clock controller. I think 'sclk' is too common name. 'sclk' don't include specific device name. As I know, usual clock name includes the name of IP or includes the specific meaning for each IP. In my opinion, I think sclk_adc is better than 'sclk'. Thanks, Chanwoo Choi -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 19 July 2014 00:15:35 Chanwoo Choi wrote: > On Fri, Jul 18, 2014 at 8:14 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Friday 18 July 2014 19:00:48 Chanwoo Choi wrote: > >> On 07/18/2014 06:47 PM, Arnd Bergmann wrote: > >> > > >> > Further, why is it called "sclk_adc" rather than just "sclk"? > >> > >> The sclk means 'special clock' in Exynos TRM. Exynos SoC has varisou sclk clocks. > >> 'sclk_adc' is only used for ADC IP. > > > > But that sounds like sclk_adc is the name of the global name > > of the clock signal coming out of the clock controller. > > > > I still think it would be best to name it 'sclk' as the input > > for the adc. It shouldn't rely on a particular name of the > > clock controller. > > I think 'sclk' is too common name. 'sclk' don't include specific device name. > As I know, usual clock name includes the name of IP or includes the > specific meaning for each IP. No, normally it does not include the name of the IP, that's my whole point. Including the name of the IP is completely pointless because that is implied by the fact that it's being used by that particular IP. Ideally you would find the data sheet for the ADC IP block and figure out what this clock is used for, then find the right name for that. In a lot of cases, we are actually better off not naming the clocks at all but simply enumerating them if nobody knows what they are good for. In that case, you would simply have the first clock and the second clock of the ADC part and enable them both. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jul 19, 2014 at 12:23 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Saturday 19 July 2014 00:15:35 Chanwoo Choi wrote: >> On Fri, Jul 18, 2014 at 8:14 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Friday 18 July 2014 19:00:48 Chanwoo Choi wrote: >> >> On 07/18/2014 06:47 PM, Arnd Bergmann wrote: >> >> > >> >> > Further, why is it called "sclk_adc" rather than just "sclk"? >> >> >> >> The sclk means 'special clock' in Exynos TRM. Exynos SoC has varisou sclk clocks. >> >> 'sclk_adc' is only used for ADC IP. >> > >> > But that sounds like sclk_adc is the name of the global name >> > of the clock signal coming out of the clock controller. >> > >> > I still think it would be best to name it 'sclk' as the input >> > for the adc. It shouldn't rely on a particular name of the >> > clock controller. >> >> I think 'sclk' is too common name. 'sclk' don't include specific device name. >> As I know, usual clock name includes the name of IP or includes the >> specific meaning for each IP. > > No, normally it does not include the name of the IP, that's my whole point. > Including the name of the IP is completely pointless because that is > implied by the fact that it's being used by that particular IP. > > Ideally you would find the data sheet for the ADC IP block and figure > out what this clock is used for, then find the right name for that. I mentioned the meaning of clocks ('adc', 'sclk_adc') as following in patch description. But, you think that need the more proper name instead of 'sclk_adc' to mean the correct operation of 'sclk_adc'. I'll check once again the meaning of 'sclk_adc' in data sheet and renaming it. 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 > In a lot of cases, we are actually better off not naming the clocks > at all but simply enumerating them if nobody knows what they are good > for. In that case, you would simply have the first clock and the second > clock of the ADC part and enable them both. Thanks for your review. Chanwoo Choi -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 19 July 2014 01:11:53 Chanwoo Choi wrote: > > 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 The bus clock is often named "apb_pclk", but it's too late to change that now, and it's not clear if the device is actually on APB. The problem with "sclk_adc" is that it's very close to the name used in the clock provider, with is CLK_SCLK_TSADC. Please try to avoid the ambiguity here and make it clear that they are not referring to the same thing. "internal" or "special" might be good if you want to avoid "sclk". Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jul 19, 2014 at 1:31 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Saturday 19 July 2014 01:11:53 Chanwoo Choi wrote: >> >> 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 > > The bus clock is often named "apb_pclk", but it's too late to change > that now, and it's not clear if the device is actually on APB. > > The problem with "sclk_adc" is that it's very close to the name used > in the clock provider, with is CLK_SCLK_TSADC. Please try to avoid > the ambiguity here and make it clear that they are not referring > to the same thing. "internal" or "special" might be good if you want > to avoid "sclk". OK, thanks for your feedback. I'll try to find proper name. Chanwoo Choi -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c index 00d67fd..b63e882 100644 --- a/drivers/iio/adc/exynos_adc.c +++ b/drivers/iio/adc/exynos_adc.c @@ -81,9 +81,11 @@ struct exynos_adc { struct exynos_adc_data *data; + struct device *dev; void __iomem *regs; void __iomem *enable_reg; struct clk *clk; + struct clk *sclk; unsigned int irq; struct regulator *vdd; @@ -95,6 +97,7 @@ struct exynos_adc { struct exynos_adc_data { int num_channels; + bool needs_sclk; void (*init_hw)(struct exynos_adc *info); void (*exit_hw)(struct exynos_adc *info); @@ -102,6 +105,66 @@ struct exynos_adc_data { void (*start_conv)(struct exynos_adc *info, unsigned long addr); }; +static void exynos_adc_unprepare_clk(struct exynos_adc *info) +{ + if (info->data->needs_sclk) + clk_unprepare(info->sclk); + clk_unprepare(info->clk); +} + +static int exynos_adc_prepare_clk(struct exynos_adc *info) +{ + int ret; + + ret = clk_prepare(info->clk); + if (ret) { + dev_err(info->dev, "failed preparing adc clock: %d\n", ret); + return ret; + } + + if (info->data->needs_sclk) { + ret = clk_prepare(info->sclk); + if (ret) { + clk_unprepare(info->clk); + dev_err(info->dev, + "failed preparing sclk_adc clock: %d\n", ret); + return ret; + } + } + + return 0; +} + +static void exynos_adc_disable_clk(struct exynos_adc *info) +{ + if (info->data->needs_sclk) + clk_disable(info->sclk); + clk_disable(info->clk); +} + +static int exynos_adc_enable_clk(struct exynos_adc *info) +{ + int ret; + + ret = clk_enable(info->clk); + if (ret) { + dev_err(info->dev, "failed enabling adc clock: %d\n", ret); + return ret; + } + + if (info->data->needs_sclk) { + ret = clk_enable(info->sclk); + if (ret) { + clk_disable(info->clk); + dev_err(info->dev, + "failed enabling sclk_adc clock: %d\n", ret); + return ret; + } + } + + return 0; +} + static void exynos_adc_v1_init_hw(struct exynos_adc *info) { u32 con1; @@ -199,13 +262,20 @@ static void exynos_adc_v2_start_conv(struct exynos_adc *info, writel(con1 | ADC_CON_EN_START, ADC_V2_CON1(info->regs)); } +#define __EXYNOS_ADC_V2_DATA \ + .num_channels = MAX_ADC_V2_CHANNELS, \ + .init_hw = exynos_adc_v2_init_hw, \ + .exit_hw = exynos_adc_v2_exit_hw, \ + .clear_irq = exynos_adc_v2_clear_irq, \ + .start_conv = exynos_adc_v2_start_conv, \ + static struct exynos_adc_data const exynos_adc_v2_data = { - .num_channels = MAX_ADC_V2_CHANNELS, + __EXYNOS_ADC_V2_DATA +}; - .init_hw = exynos_adc_v2_init_hw, - .exit_hw = exynos_adc_v2_exit_hw, - .clear_irq = exynos_adc_v2_clear_irq, - .start_conv = exynos_adc_v2_start_conv, +static struct exynos_adc_data const exynos3250_adc_v2_data = { + __EXYNOS_ADC_V2_DATA + .needs_sclk = true, }; static const struct of_device_id exynos_adc_match[] = { @@ -215,6 +285,9 @@ static const struct of_device_id exynos_adc_match[] = { }, { .compatible = "samsung,exynos-adc-v2", .data = (void *)&exynos_adc_v2_data, + }, { + .compatible = "samsung,exynos3250-adc-v2", + .data = (void *)&exynos3250_adc_v2_data, }, {}, }; @@ -376,6 +449,7 @@ static int exynos_adc_probe(struct platform_device *pdev) } info->irq = irq; + info->dev = &pdev->dev; init_completion(&info->completion); @@ -386,6 +460,16 @@ static int exynos_adc_probe(struct platform_device *pdev) return PTR_ERR(info->clk); } + if (info->data->needs_sclk) { + info->sclk = devm_clk_get(&pdev->dev, "sclk_adc"); + if (IS_ERR(info->sclk)) { + dev_err(&pdev->dev, + "failed getting sclk_adc, err = %ld\n", + PTR_ERR(info->sclk)); + return PTR_ERR(info->sclk); + } + } + info->vdd = devm_regulator_get(&pdev->dev, "vdd"); if (IS_ERR(info->vdd)) { dev_err(&pdev->dev, "failed getting regulator, err = %ld\n", @@ -397,10 +481,14 @@ static int exynos_adc_probe(struct platform_device *pdev) if (ret) return ret; - ret = clk_prepare_enable(info->clk); + ret = exynos_adc_prepare_clk(info); if (ret) goto err_disable_reg; + ret = exynos_adc_enable_clk(info); + if (ret) + goto err_unprepare_clk; + platform_set_drvdata(pdev, indio_dev); indio_dev->name = dev_name(&pdev->dev); @@ -443,7 +531,9 @@ err_irq: err_disable_clk: if (info->data->exit_hw) info->data->exit_hw(info); - clk_disable_unprepare(info->clk); + exynos_adc_disable_clk(info); +err_unprepare_clk: + exynos_adc_unprepare_clk(info); err_disable_reg: regulator_disable(info->vdd); return ret; @@ -460,7 +550,8 @@ static int exynos_adc_remove(struct platform_device *pdev) free_irq(info->irq, info); if (info->data->exit_hw) info->data->exit_hw(info); - clk_disable_unprepare(info->clk); + exynos_adc_disable_clk(info); + exynos_adc_unprepare_clk(info); regulator_disable(info->vdd); return 0; @@ -474,8 +565,7 @@ static int exynos_adc_suspend(struct device *dev) if (info->data->exit_hw) info->data->exit_hw(info); - - clk_disable_unprepare(info->clk); + exynos_adc_disable_clk(info); regulator_disable(info->vdd); return 0; @@ -491,7 +581,7 @@ static int exynos_adc_resume(struct device *dev) if (ret) return ret; - ret = clk_prepare_enable(info->clk); + ret = exynos_adc_enable_clk(info); if (ret) return ret;