| Message ID | CAOLZvyE6W8-X2_M+-KQNry2VPwLzwbBLuxr5Z5EdBQo7Skv2RQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 03, 2015 at 08:54:57AM +0100, Manuel Lauss wrote: > + wm8731->mclk = devm_clk_get(&spi->dev, "mclk"); > + if (IS_ERR(wm8731->mclk)) { > + wm8731->mclk = NULL; > + dev_warn(&spi->dev, "assuming static MCLK\n"); > + } This is broken for both deferred probe and in the case where the clock API genuinely returns a NULL clock. Other than that it's the kind of thing that we've done for some other drivers, though it's not good to have to do this. Check them for correct behaviour. The coding style is also not right for the whole patch and there's a lot of missing error checking.
On Tue, Feb 3, 2015 at 1:44 PM, Mark Brown <broonie@kernel.org> wrote: > On Tue, Feb 03, 2015 at 08:54:57AM +0100, Manuel Lauss wrote: > >> + wm8731->mclk = devm_clk_get(&spi->dev, "mclk"); >> + if (IS_ERR(wm8731->mclk)) { >> + wm8731->mclk = NULL; >> + dev_warn(&spi->dev, "assuming static MCLK\n"); >> + } > > This is broken for both deferred probe and in the case where the clock > API genuinely returns a NULL clock. Other than that it's the kind of > thing that we've done for some other drivers, though it's not good to > have to do this. Check them for correct behaviour. Hm, so the only option is to create the simples possible 12MHz clk object? Manuel
On Tue, Feb 03, 2015 at 03:40:45PM +0100, Manuel Lauss wrote: > On Tue, Feb 3, 2015 at 1:44 PM, Mark Brown <broonie@kernel.org> wrote: > >> + wm8731->mclk = devm_clk_get(&spi->dev, "mclk"); > >> + if (IS_ERR(wm8731->mclk)) { > >> + wm8731->mclk = NULL; > >> + dev_warn(&spi->dev, "assuming static MCLK\n"); > >> + } > > This is broken for both deferred probe and in the case where the clock > > API genuinely returns a NULL clock. Other than that it's the kind of > > thing that we've done for some other drivers, though it's not good to > > have to do this. Check them for correct behaviour. > Hm, so the only option is to create the simples possible 12MHz clk object? Well, that's the best option in general. You can get away with just making sure that -EPROBE_DEFER is handled and that IS_ERR() is used to check for an invalid clock but if you can define a clock that's even better (and should be pretty painless), we're going to want to do that transition at some point.
On 02/03/2015 01:44 PM, Mark Brown wrote: > On Tue, Feb 03, 2015 at 08:54:57AM +0100, Manuel Lauss wrote: > >> + wm8731->mclk = devm_clk_get(&spi->dev, "mclk"); >> + if (IS_ERR(wm8731->mclk)) { >> + wm8731->mclk = NULL; >> + dev_warn(&spi->dev, "assuming static MCLK\n"); >> + } > > This is broken for both deferred probe and in the case where the clock > API genuinely returns a NULL clock. Other than that it's the kind of > thing that we've done for some other drivers, though it's not good to > have to do this. Check them for correct behaviour. Ideally we'd introduce a {devm_}clk_get_optional(), with the same semantics as gpiod_get_optional(), which handles the finer details of differentiating between clock specified, but not yet probed, clock specified, but incorrectly and no clock specified, so this doesn't have to be done over and over by each driver.
On Tue, Feb 03, 2015 at 05:53:48PM +0100, Lars-Peter Clausen wrote: > On 02/03/2015 01:44 PM, Mark Brown wrote: > >On Tue, Feb 03, 2015 at 08:54:57AM +0100, Manuel Lauss wrote: > > > >>+ wm8731->mclk = devm_clk_get(&spi->dev, "mclk"); > >>+ if (IS_ERR(wm8731->mclk)) { > >>+ wm8731->mclk = NULL; > >>+ dev_warn(&spi->dev, "assuming static MCLK\n"); > >>+ } > > > >This is broken for both deferred probe and in the case where the clock > >API genuinely returns a NULL clock. Other than that it's the kind of > >thing that we've done for some other drivers, though it's not good to > >have to do this. Check them for correct behaviour. > > Ideally we'd introduce a {devm_}clk_get_optional(), with the same semantics > as gpiod_get_optional(), which handles the finer details of differentiating > between clock specified, but not yet probed, clock specified, but > incorrectly and no clock specified, so this doesn't have to be done over and > over by each driver. No, we don't need to. It clk_get() already knows this distinction, and it appropriately returns -ENOENT vs -EPROBE_DEFER according to whether there's a clock specified in DT or not.
On 02/03/2015 06:17 PM, Russell King - ARM Linux wrote: > On Tue, Feb 03, 2015 at 05:53:48PM +0100, Lars-Peter Clausen wrote: >> On 02/03/2015 01:44 PM, Mark Brown wrote: >>> On Tue, Feb 03, 2015 at 08:54:57AM +0100, Manuel Lauss wrote: >>> >>>> + wm8731->mclk = devm_clk_get(&spi->dev, "mclk"); >>>> + if (IS_ERR(wm8731->mclk)) { >>>> + wm8731->mclk = NULL; >>>> + dev_warn(&spi->dev, "assuming static MCLK\n"); >>>> + } >>> >>> This is broken for both deferred probe and in the case where the clock >>> API genuinely returns a NULL clock. Other than that it's the kind of >>> thing that we've done for some other drivers, though it's not good to >>> have to do this. Check them for correct behaviour. >> >> Ideally we'd introduce a {devm_}clk_get_optional(), with the same semantics >> as gpiod_get_optional(), which handles the finer details of differentiating >> between clock specified, but not yet probed, clock specified, but >> incorrectly and no clock specified, so this doesn't have to be done over and >> over by each driver. > > No, we don't need to. It clk_get() already knows this distinction, and > it appropriately returns -ENOENT vs -EPROBE_DEFER according to whether > there's a clock specified in DT or not. I know, but it returns a error when no clock is specified (-ENOENT), whereas gpiod_get_optional()-like semantics mean, it would return no error.
On 02/03/2015 06:26 PM, Lars-Peter Clausen wrote: > On 02/03/2015 06:17 PM, Russell King - ARM Linux wrote: >> On Tue, Feb 03, 2015 at 05:53:48PM +0100, Lars-Peter Clausen wrote: >>> On 02/03/2015 01:44 PM, Mark Brown wrote: >>>> On Tue, Feb 03, 2015 at 08:54:57AM +0100, Manuel Lauss wrote: >>>> >>>>> + wm8731->mclk = devm_clk_get(&spi->dev, "mclk"); >>>>> + if (IS_ERR(wm8731->mclk)) { >>>>> + wm8731->mclk = NULL; >>>>> + dev_warn(&spi->dev, "assuming static MCLK\n"); >>>>> + } >>>> >>>> This is broken for both deferred probe and in the case where the clock >>>> API genuinely returns a NULL clock. Other than that it's the kind of >>>> thing that we've done for some other drivers, though it's not good to >>>> have to do this. Check them for correct behaviour. >>> >>> Ideally we'd introduce a {devm_}clk_get_optional(), with the same semantics >>> as gpiod_get_optional(), which handles the finer details of differentiating >>> between clock specified, but not yet probed, clock specified, but >>> incorrectly and no clock specified, so this doesn't have to be done over and >>> over by each driver. >> >> No, we don't need to. It clk_get() already knows this distinction, and >> it appropriately returns -ENOENT vs -EPROBE_DEFER according to whether >> there's a clock specified in DT or not. > > I know, but it returns a error when no clock is specified (-ENOENT), whereas > gpiod_get_optional()-like semantics mean, it would return no error. What I wanted to say is that pretty much every user of clk_get() that wants a optional clock gets the handling wrong. E.g. they check for PTR_ERR(clk) == -EPROBE_DEFER rather than checking for PTR_ERR(clk) != -ENOENT. Which causes errors when the clock is specified, but incorrectly specified (e.g. invalid phandle or specifier) to be silently ignored. My hope is that having a explicit API for requesting a optional clock might make it easier for users to gets things right. If you have coccinelle you can use the following script to find good and bad users: @@ expression clk; @@ clk = ( devm_clk_get | clk_get ) (...); <+... ( *PTR_ERR(clk) == -EPROBE_DEFER | *PTR_ERR(clk) != -ENOENT ) ...+>
Hi Mark, On 02/04/2015 12:21 AM, Mark Brown wrote: > On Tue, Feb 03, 2015 at 03:40:45PM +0100, Manuel Lauss wrote: >> On Tue, Feb 3, 2015 at 1:44 PM, Mark Brown <broonie@kernel.org> wrote: > >>>> + wm8731->mclk = devm_clk_get(&spi->dev, "mclk"); >>>> + if (IS_ERR(wm8731->mclk)) { >>>> + wm8731->mclk = NULL; >>>> + dev_warn(&spi->dev, "assuming static MCLK\n"); >>>> + } > >>> This is broken for both deferred probe and in the case where the clock >>> API genuinely returns a NULL clock. Other than that it's the kind of >>> thing that we've done for some other drivers, though it's not good to >>> have to do this. Check them for correct behaviour. > >> Hm, so the only option is to create the simples possible 12MHz clk object? > > Well, that's the best option in general. You can get away with just > making sure that -EPROBE_DEFER is handled and that IS_ERR() is used to > check for an invalid clock but if you can define a clock that's even > better (and should be pretty painless), we're going to want to do that > transition at some point. Do you mean I send my RFC patch as the formal patch, and let other boards which use the wm8731 to add clk object, am I right? Best Regards, Bo Shen
On Wed, Feb 04, 2015 at 11:45:29AM +0800, Bo Shen wrote: > Do you mean I send my RFC patch as the formal patch, and let other boards > which use the wm8731 to add clk object, am I right? No, we need to keep the boards working so we either need patches adding the fixed clocks for existing boards or we need to have the handling of missing clocks that Manuel suggested.
diff --git a/sound/soc/codecs/wm8731.c b/sound/soc/codecs/wm8731.c index b115ed8..648b8cd 100644 --- a/sound/soc/codecs/wm8731.c +++ b/sound/soc/codecs/wm8731.c @@ -13,6 +13,7 @@ * published by the Free Software Foundation. */ +#include <linux/clk.h> #include <linux/module.h> #include <linux/moduleparam.h> #include <linux/init.h> @@ -45,6 +46,7 @@ static const char *wm8731_supply_names[WM8731_NUM_SUPPLIES] = { /* codec private data */ struct wm8731_priv { struct regmap *regmap; + struct clk *mclk; struct regulator_bulk_data supplies[WM8731_NUM_SUPPLIES]; const struct snd_pcm_hw_constraint_list *constraints;