Re: [PATCH v5 11/44] clk: davinci: Add platform information for TI DA830 PSC

[David Lechner]

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.