Re: [PATCH v2 4/4] clk: meson: gxbb-aoclk: Add CEC 32k clock

From: Neil Armstrong
Date: Mon Jul 31 2017 - 04:11:12 EST


Hi,

On 07/29/2017 10:04 AM, Chris Moore wrote:
> Hi,
>
> Sorry I forgot to reply to all in my previous post :(
> I hope this corrects things.
>
> Le 28/07/2017 Ã 11:53, Neil Armstrong a Ãcrit :
>
> [snip]
>
>> +static long aoclk_cec_32k_round_rate(struct clk_hw *hw, unsigned long rate,
>> + unsigned long *prate)
>> +{
>> + const struct cec_32k_freq_table *freq = find_cec_32k_freq(rate,
>> + *prate);
>> +
>> + /* If invalid return first one */
>> + if (!freq)
>> + return freq[0].target_rate;
>
> Wouldn't this dereference a null pointer (or am I being stupid this morning)?

No, it's me !!
Will fix it in v3...

>
>> +
>> + return freq->target_rate;
>> +}
>> +
>> +static int aoclk_cec_32k_set_rate(struct clk_hw *hw, unsigned long rate,
>> + unsigned long parent_rate)
>> +{
>> + const struct cec_32k_freq_table *freq = find_cec_32k_freq(rate,
>> + parent_rate);
>> + struct aoclk_cec_32k *cec_32k = to_aoclk_cec_32k(hw);
>> + u32 reg = 0;
>> +
>> + if (!freq)
>> + return -EINVAL;
>> +
>> + /* Disable clock */
>> + regmap_update_bits(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0,
>> + CLK_CNTL0_IN_GATE_EN | CLK_CNTL0_OUT_GATE_EN, 0);
>> +
>> + if (freq->dualdiv)
>> + reg = CLK_CNTL0_DUALDIV_EN |
>> + FIELD_PREP(CLK_CNTL0_N1_MASK, freq->n1 - 1) |
>> + FIELD_PREP(CLK_CNTL0_N2_MASK, freq->n2 - 1);
>> + else
>> + reg = FIELD_PREP(CLK_CNTL0_N1_MASK, freq->n1 - 1);
>> +
>
> Suggestion:
>
> + reg = FIELD_PREP(CLK_CNTL0_N1_MASK, freq->n1 - 1);
> + if (freq->dualdiv)
> + reg |= CLK_CNTL0_DUALDIV_EN |
> + FIELD_PREP(CLK_CNTL0_N2_MASK, freq->n2 - 1);
>
> is shorter but maybe generates the same code.
>
>> + regmap_write(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0, reg);
>> +
>> + if (freq->dualdiv)
>> + reg = FIELD_PREP(CLK_CNTL1_M1_MASK, freq->m1 - 1) |
>> + FIELD_PREP(CLK_CNTL1_M2_MASK, freq->m2 - 1);
>> + else
>> + reg = FIELD_PREP(CLK_CNTL1_M1_MASK, freq->m1 - 1);
>> +
>
> Idem:
>
> + reg = FIELD_PREP(CLK_CNTL1_M1_MASK, freq->m1 - 1);
> + if (freq->dualdiv)
> + reg |= FIELD_PREP(CLK_CNTL1_M2_MASK, freq->m2 - 1);
>

Thanks for the suggestions, I will send a v3 asap.

Neil
> Cheers,
> Chris