| Message ID | 20200409195841.18901-14-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | ASoC/SOF/clk/gpio/dt: add Hifiberry DAC+ PRO support | expand |
Quoting Pierre-Louis Bossart (2020-04-09 12:58:38) > devm_clk_get() fails on ACPI platforms when a NULL string is used. > Create a "sclk" lookup to make sure codec and machine drivers can get > the clock. > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/clk/clk-hifiberry-dacpro.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/clk/clk-hifiberry-dacpro.c b/drivers/clk/clk-hifiberry-dacpro.c > index d01a90fed51b..36210f52c624 100644 > --- a/drivers/clk/clk-hifiberry-dacpro.c > +++ b/drivers/clk/clk-hifiberry-dacpro.c > @@ -24,10 +24,12 @@ > * struct clk_hifiberry_hw - Common struct to the HiFiBerry DAC Pro > * @hw: clk_hw for the common clk framework > * @mode: 0 => CLK44EN, 1 => CLK48EN > + * @sclk_lookup: handle for "sclk" > */ > struct clk_hifiberry_hw { > struct clk_hw hw; > u8 mode; > + struct clk_lookup *sclk_lookup; > }; > > #define to_hifiberry_clk(_hw) container_of(_hw, struct clk_hifiberry_hw, hw) > @@ -121,15 +123,34 @@ static int clk_hifiberry_dacpro_probe(struct platform_device *pdev) > ret = devm_clk_hw_register_clkdev(dev, &proclk->hw, > init.name, NULL); > #endif > + if (ret) { > + dev_err(dev, "Fail to add clock driver\n"); > + return ret; > + } > + > + proclk->sclk_lookup = clkdev_hw_create(&proclk->hw, "sclk", NULL); > + if (!proclk->sclk_lookup) { > +#ifndef CONFIG_ACPI Is it to save code space? Otherwise the ifdefs are pretty ugly and I'd prefer we just call of_clk APIs and rely on the inline stubs when CONFIG_OF isn't enabled to be optimized out. > + of_clk_del_provider(dev->of_node); > +#endif > + return -ENOMEM; > + } > +
>> + proclk->sclk_lookup = clkdev_hw_create(&proclk->hw, "sclk", NULL); >> + if (!proclk->sclk_lookup) { >> +#ifndef CONFIG_ACPI > > Is it to save code space? Otherwise the ifdefs are pretty ugly and I'd > prefer we just call of_clk APIs and rely on the inline stubs when > CONFIG_OF isn't enabled to be optimized out. CONFIG_OF was added as a dependency (see patch 10/16) so that we can use the 'compatible' string to probe w/ the PRP0001 device. I must admit I don't know what these functions do so I just filtered them out in the ACPI case. >> + of_clk_del_provider(dev->of_node); >> +#endif >> + return -ENOMEM; >> + } >> +
On Wed, Apr 22, 2020 at 04:51:52AM -0500, Pierre-Louis Bossart wrote: > > > + proclk->sclk_lookup = clkdev_hw_create(&proclk->hw, "sclk", NULL); > > > + if (!proclk->sclk_lookup) { > > > +#ifndef CONFIG_ACPI > > > > Is it to save code space? Otherwise the ifdefs are pretty ugly and I'd > > prefer we just call of_clk APIs and rely on the inline stubs when > > CONFIG_OF isn't enabled to be optimized out. > > CONFIG_OF was added as a dependency (see patch 10/16) so that we can use the > 'compatible' string to probe w/ the PRP0001 device. PRP0001 does not require CONFIG_OF to be set. > I must admit I don't know what these functions do so I just filtered them > out in the ACPI case. I think you have to check if one is a superposition of the other. > > > + of_clk_del_provider(dev->of_node); > > > +#endif > > > + return -ENOMEM; > > > + }
diff --git a/drivers/clk/clk-hifiberry-dacpro.c b/drivers/clk/clk-hifiberry-dacpro.c index d01a90fed51b..36210f52c624 100644 --- a/drivers/clk/clk-hifiberry-dacpro.c +++ b/drivers/clk/clk-hifiberry-dacpro.c @@ -24,10 +24,12 @@ * struct clk_hifiberry_hw - Common struct to the HiFiBerry DAC Pro * @hw: clk_hw for the common clk framework * @mode: 0 => CLK44EN, 1 => CLK48EN + * @sclk_lookup: handle for "sclk" */ struct clk_hifiberry_hw { struct clk_hw hw; u8 mode; + struct clk_lookup *sclk_lookup; }; #define to_hifiberry_clk(_hw) container_of(_hw, struct clk_hifiberry_hw, hw) @@ -121,15 +123,34 @@ static int clk_hifiberry_dacpro_probe(struct platform_device *pdev) ret = devm_clk_hw_register_clkdev(dev, &proclk->hw, init.name, NULL); #endif + if (ret) { + dev_err(dev, "Fail to add clock driver\n"); + return ret; + } + + proclk->sclk_lookup = clkdev_hw_create(&proclk->hw, "sclk", NULL); + if (!proclk->sclk_lookup) { +#ifndef CONFIG_ACPI + of_clk_del_provider(dev->of_node); +#endif + return -ENOMEM; + } + + platform_set_drvdata(pdev, proclk); return ret; } static int clk_hifiberry_dacpro_remove(struct platform_device *pdev) { + struct clk_hifiberry_hw *proclk = platform_get_drvdata(pdev); + + clkdev_drop(proclk->sclk_lookup); + #ifndef CONFIG_ACPI of_clk_del_provider(pdev->dev.of_node); #endif + return 0; }
devm_clk_get() fails on ACPI platforms when a NULL string is used. Create a "sclk" lookup to make sure codec and machine drivers can get the clock. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- drivers/clk/clk-hifiberry-dacpro.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)