Re: [PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs
From: Joel Stanley
Date: Thu Oct 05 2017 - 02:23:19 EST
On Thu, Oct 5, 2017 at 6:48 AM, Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote:
> On 10/03, Joel Stanley wrote:
>> On Tue, Oct 3, 2017 at 6:54 AM, Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote:
>> > On 09/21, Joel Stanley wrote:
>> >> @@ -160,6 +191,104 @@ static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
>> >> + /*
>> >> + * Memory controller (M-PLL) PLL. This clock is configured by the
>> >> + * bootloader, and is exposed to Linux as a read-only clock rate.
>> >> + */
>> >> + regmap_read(map, ASPEED_MPLL_PARAM, &val);
>> >> + aspeed_clk_data->hws[ASPEED_CLK_MPLL] = aspeed_calc_pll("mpll", val);
>> >> +
>> >> + /* SD/SDIO clock divider (TODO: There's a gate too) */
>> >> + hw = clk_hw_register_divider_table(NULL, "sdio", "hpll", 0,
>> > Please pass your dev pointer here from the platform device.
>> >> + scu_base + ASPEED_CLK_SELECTION, 12, 3, 0,
>> >> + div_table,
>> >> + &aspeed_clk_lock);
>> > And check for errors? Perhaps use devm_clk_hw_regsiter() APIs and
>> > construct the dividers and muxes directly instead of using the
>> > basic type registration APIs.
>> Do you think that devm_ is overkill, given we will never unload this driver?
> Is probe defer going to happen? Even if unload can't happen,
> probe defer is a concern unless that is also ruled out.
As I understand it, I will only get an EPROBE_DEFER if I'm requesting
something from another driver?
If that's the case then we can rule that out.
>> Can you explain why you suggest to construct the structures directly
>> instead of using the APIs?
> There aren't devm APIs for some of these basic clk type
> registration functions.
>> I had a read of the basic type registration functions, and the
>> relevant failure paths are memory allocation failures. If we're out of
>> memory that early in boot then things have gone pretty bad.
>> I can add checks for null and bail out; I don't think there's value in
>> freeing the allocated memory: if a system can't load it's clock driver
>> then it's super hosed.
> If we can't proceed without this driver because it's super hosed,
> then perhaps we need to panic the system on errors here. Should
> be simple enough to add some error checks and goto panic("Things
> are super hosed").
Okay. I added checks in v4. Does that look ok you you?