Messages in this thread | | | Subject | Re: [PATCH v5 11/44] clk: davinci: Add platform information for TI DA830 PSC | From | David Lechner <> | Date | Wed, 17 Jan 2018 11:32:31 -0600 |
| |
On 01/17/2018 06:18 AM, Sekhar Nori wrote: > On Tuesday 16 January 2018 10:46 PM, David Lechner wrote: > >>>> +static const struct davinci_psc_clk_info da830_psc0_info[] >>>> __initconst = { >>>> + LPSC(0, 0, tpcc, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>>> + LPSC(1, 0, tptc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>>> + LPSC(2, 0, tptc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>>> + LPSC(3, 0, aemif, pll0_sysclk3, LPSC_ALWAYS_ENABLED), >>>> + LPSC(4, 0, spi0, pll0_sysclk2, 0), >>>> + LPSC(5, 0, mmcsd, pll0_sysclk2, 0), >>>> + LPSC(6, 0, aintc, pll0_sysclk4, LPSC_ALWAYS_ENABLED), >>>> + LPSC(7, 0, arm_rom, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>>> + LPSC(8, 0, secu_mgr, pll0_sysclk4, LPSC_ALWAYS_ENABLED), >>>> + LPSC(9, 0, uart0, pll0_sysclk2, 0), >>>> + LPSC(10, 0, scr0_ss, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>>> + LPSC(11, 0, scr1_ss, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>>> + LPSC(12, 0, scr2_ss, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>>> + LPSC(13, 0, dmax, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> >>> pruss is better (I know the name is coming from existing code). >>> >>>> + LPSC(14, 0, arm, pll0_sysclk6, LPSC_ALWAYS_ENABLED), >>> >>> This is LPSC 15 which controls DSP too. But its missing from existing >>> code. Not sure why. Probably a note for future. For now okay with >>> ignoring it. >>> >>>> + { } >>>> +}; >>> >>> Tables like these are much easier to parse if columns are spaced using a >>> tab. >>> >> >> Tabs make the lines over 80 columns. How about spaces instead? > > That works for me. I guess checkpatch will complain, but hopefully > maintainers will agree on the exception. > >>>> +void __init da830_psc_clk_init(void __iomem *psc0, void __iomem *psc1) >>>> +{ >>>> + struct clk_onecell_data *clk_data; >>>> + >>>> + clk_data = davinci_psc_register_clocks(psc0, da830_psc0_info, 16); >>>> + if (!clk_data) >>>> + return; >>>> + >>>> + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); >>>> + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); >>>> + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); >>>> + clk_register_clkdev(clk_data->clks[14], "arm", NULL); >>>> + >>>> + clk_free_onecell_data(clk_data); >>>> + >>>> + clk_data = davinci_psc_register_clocks(psc1, da830_psc1_info, 32); >>>> + if (!clk_data) >>>> + return; >>>> + >>>> + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); >>>> + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); >>>> + clk_register_clkdev(clk_data->clks[2], NULL, "ohci-da8xx"); >>>> + clk_register_clkdev(clk_data->clks[3], "gpio", NULL); >>> >>> This is pretty bad (and no fault of yours) - having a con_id but no >>> device name. Can you please make a pre-series which passes NULL con_id >>> in gpio-davinci.c? >> >> I'll give it a try. This is complicated by the fact that the con_id has >> made it's way into the device tree bindings. However, I think we can >> safely deprecate clock-names = "gpio" in the device tree bindings since >> we can make the driver ignore that property to preserve backwards >> compatibility.
Agreed.
> I don't think this breaks DT-backward compatibility. Passing a NULL > con_id in driver should find the clock for that device even if DT entry > has clock-names present. As far as I can read clk_find(). > > The less intrusive alternate is to add the GPIO device name in the table > here, while keeping the con_id and keeping the driver untouched. The > advantage of that is lesser number of dependent patches for this series > to go in. > > Later once CCF conversion has been there in the kernel for one full > release and no regressions, these other clean-ups can be done.
I like this approach.
| |