[alsa-devel] [PATCH 2/5] clk: bcm2835: enable management of PCM clock

[Martin Sperl]

> On 10.01.2016, at 19:56, Geert Uytterhoeven <geert at linux-m68k.org> wrote:
> 
> Hi Martin,
> 
> On Sun, Jan 10, 2016 at 7:01 PM, Martin Sperl <kernel at martin.sperl.org> wrote:
>>> On 10.01.2016, at 14:02, Geert Uytterhoeven <geert at linux-m68k.org> wrote:
>>> I wrote:
>>> | Hence it can better be replaced (it seems to be unused in dts files, but you
>>> | can keep the definition to be 100% sure) by an ARRAY_SIZE() in the C driver.
>>> | This requires changing the driver to e.g. initialize clks[] in
>>> |bcm2835_clk_probe() based on a table instead of explicit code.
>>> 
>>> If you fill in clks[] from a static table, you can take ARRAY_SIZE of
>>> the static table.
>> 
>> You mean something like the below?
>> (note: copy/paste from console issues - spaces instead of tabs)
> 
> More or less.
> 
>> };
>> 
>> static inline void cprman_write(struct bcm2835_cprman *cprman, u32 reg, u32 val
>> @@ -496,6 +496,14 @@ static const struct bcm2835_pll_data bcm2835_pllh_data = {
>>        .max_fb_rate = BCM2835_MAX_FB_RATE,
>> };
>> 
>> +static const struct bcm2835_pll_data *bcm2835_plls[] = {
>> +       [BCM2835_PLLA] = &bcm2835_plla_data,
>> +       [BCM2835_PLLB] = &bcm2835_pllb_data,
>> +       [BCM2835_PLLC] = &bcm2835_pllc_data,
>> +       [BCM2835_PLLD] = &bcm2835_plld_data,
>> +       [BCM2835_PLLH] = &bcm2835_pllh_data,
>> +};
> 
> This is a sparse array?
Yes - for all.

> 
>> struct bcm2835_pll {
>>        struct clk_hw hw;
>>        struct bcm2835_cprman *cprman;
>> @@ -1560,8 +1600,13 @@ static int bcm2835_clk_probe(struct platform_device *pdev)
>>        struct clk **clks;
>>        struct bcm2835_cprman *cprman;
>>        struct resource *res;
>> +       const int clks_cnt = max(ARRAY_SIZE(bcm2835_plls),
>> +                                max(ARRAY_SIZE(bcm2835_pll_divs),
>> +                                    ARRAY_SIZE(bcm2835_clks))) + 1;
>> +       size_t alloc = sizeof(*cprman) + clks_cnt * sizeof(*clks);
>> +       size_t i;
> 
> If you combine all 3 arrays in a single non-sparse array, you could get rid
> of the dynamic allocation using the maximum of the 3 sizes, and can just
> use a single ARRAY_SIZE().


The problem is that we need different initialization methods for pll, pll-dividers
and derived clocks, so we can not easily make them a common setting unless we
would use function and void pointers to work arround this, which would make it
less readable (and more code again just for the - repeated - assignment).

As far as I can tell from my testing the patched version works and the kernel
boots correctly.

I will respin the patchset tomorrow - it might take slightly longer, as I
found that the fractional clock divider is not working unless the MASH
functionality is activated in the registers. This results in “frequency shifts”
for audio, which we want to avoid - this means we need another patch to
enable this MASH feature.