Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12/16/16 2:46 AM, Andy Shevchenko wrote:
On Fri, Dec 16, 2016 at 7:15 AM, Pierre-Louis Bossart
<pierre-louis.bossart@xxxxxxxxxxxxxxx> wrote:
Hi Stephen,

can you elaborate on the last comment?

Please don't do top posting.

devm_kasprintf()

Please no.

That's why I used modal verb "might" instead of "would".

It's all local to this function, devm isn't helping anything.
Having one kfree() would be good though. And using init.name for
the clkdev lookup is probably wrong and should be replaced with
something more generic along with an associated device name.

I am not sure I understand this last comment.
init.name is not a constant, it's made of the "pmc_plt_clk_" string
concatenated with an id which directly maps to which hardware clock is
registered. Clients use devm_clk_get() with a "pmc_plt_clk_<n>" argument.

Giving more thoughts about design and use of this I would propose to
do the following.

1. Create under clock framework something like clk-pmc-atom clock
driver (see, for example, clk-fractional-divider, though this one
should indeed go under x86 folder).

apart from the name the current code already does this with code in drivers/clk/x86

2. In real provider, i.e. pmc_atom, create the necessary clock tree
with *names*.

Scheme with ID is fragile, imagine another version of PMC where
ordering would be mixed up? It's not hypothetical since we used to
have this already in pmc_atom for some registers and bits.

I don't want to deal with hypothetical stuff happening to legacy hardware. If there is a problem at some point, it's no big deal to add a platform-dependent lookup table and change the registers being accessed.



--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux