| Message ID | 20200409195841.18901-5-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC/SOF/clk/gpio/dt: add Hifiberry DAC+ PRO support | expand |
On Thu, Apr 09, 2020 at 02:58:29PM -0500, Pierre-Louis Bossart wrote: > Try to detect HifiBerry 44.1 and 48kHz oscillators on codec init ... > + ctx->sclk = devm_clk_get(rtd->card->dev, "sclk"); Is this in the bindings? > + if (IS_ERR(ctx->sclk)) { > + dev_info(dev, "Could not get SCLK, will operate in SOC master mode\n"); Sounds like devm_clk_get_optional(). > + goto skip_dacpro; > + }
On 4/14/20 12:20 PM, Andy Shevchenko wrote: > On Thu, Apr 09, 2020 at 02:58:29PM -0500, Pierre-Louis Bossart wrote: >> Try to detect HifiBerry 44.1 and 48kHz oscillators on codec init > > ... > >> + ctx->sclk = devm_clk_get(rtd->card->dev, "sclk"); > > Is this in the bindings? Not for now. the 'sclk' part is only used by me myself and I in an ACPI context. I can add this description if desired. > >> + if (IS_ERR(ctx->sclk)) { > >> + dev_info(dev, "Could not get SCLK, will operate in SOC master mode\n"); > > Sounds like devm_clk_get_optional(). I am not sure about the semantic here. This driver selects the one which implements this clock, so if we get a -ENOENT return it's a very bad sign. Not sure what suppressing the error and converting to NULL would do? > >> + goto skip_dacpro; >> + } >
On Tue, Apr 14, 2020 at 01:02:12PM -0500, Pierre-Louis Bossart wrote: > On 4/14/20 12:20 PM, Andy Shevchenko wrote: > > On Thu, Apr 09, 2020 at 02:58:29PM -0500, Pierre-Louis Bossart wrote: ... > > > + ctx->sclk = devm_clk_get(rtd->card->dev, "sclk"); > > > > Is this in the bindings? > > Not for now. the 'sclk' part is only used by me myself and I in an ACPI > context. I can add this description if desired. Unfortunately you need to add this to the bindings, because it's a part of it and somebody may use it outside of your scope. > > > + if (IS_ERR(ctx->sclk)) { > > > > > + dev_info(dev, "Could not get SCLK, will operate in SOC master mode\n"); > > > > Sounds like devm_clk_get_optional(). > > I am not sure about the semantic here. This driver selects the one which > implements this clock, so if we get a -ENOENT return it's a very bad sign. > Not sure what suppressing the error and converting to NULL would do? Same as per GPIO. Can it work without this clock? How did it work before your change? When you add any hard dependency always ask yourself above questions. > > > + goto skip_dacpro; > > > + }
On 4/15/20 4:55 AM, Andy Shevchenko wrote: > On Tue, Apr 14, 2020 at 01:02:12PM -0500, Pierre-Louis Bossart wrote: >> On 4/14/20 12:20 PM, Andy Shevchenko wrote: >>> On Thu, Apr 09, 2020 at 02:58:29PM -0500, Pierre-Louis Bossart wrote: > > ... > >>>> + ctx->sclk = devm_clk_get(rtd->card->dev, "sclk"); >>> >>> Is this in the bindings? >> >> Not for now. the 'sclk' part is only used by me myself and I in an ACPI >> context. I can add this description if desired. > > Unfortunately you need to add this to the bindings, because it's a part of it > and somebody may use it outside of your scope. ok >>>> + if (IS_ERR(ctx->sclk)) { >>> >>>> + dev_info(dev, "Could not get SCLK, will operate in SOC master mode\n"); >>> >>> Sounds like devm_clk_get_optional(). >> >> I am not sure about the semantic here. This driver selects the one which >> implements this clock, so if we get a -ENOENT return it's a very bad sign. >> Not sure what suppressing the error and converting to NULL would do? > > Same as per GPIO. > Can it work without this clock? How did it work before your change? > > When you add any hard dependency always ask yourself above questions. The clock is not required in codec slave mode, it's provided by the SOC. The clock is required when the codec is configured as master. Without these GPIO selection there will be no audio. So yes we could move this to devm_clk_get_optional() and change the test to IS_ERR_OR_NULL. >>>> + goto skip_dacpro; >>>> + } >
On Wed, Apr 15, 2020 at 09:07:20AM -0500, Pierre-Louis Bossart wrote: > On 4/15/20 4:55 AM, Andy Shevchenko wrote: > > On Tue, Apr 14, 2020 at 01:02:12PM -0500, Pierre-Louis Bossart wrote: > > > On 4/14/20 12:20 PM, Andy Shevchenko wrote: > > > > On Thu, Apr 09, 2020 at 02:58:29PM -0500, Pierre-Louis Bossart wrote: ... > > > > > + if (IS_ERR(ctx->sclk)) { > > > > > > > > > + dev_info(dev, "Could not get SCLK, will operate in SOC master mode\n"); > > > > > > > > Sounds like devm_clk_get_optional(). > > > > > > I am not sure about the semantic here. This driver selects the one which > > > implements this clock, so if we get a -ENOENT return it's a very bad sign. > > > Not sure what suppressing the error and converting to NULL would do? > > > > Same as per GPIO. > > Can it work without this clock? How did it work before your change? > > > > When you add any hard dependency always ask yourself above questions. > > The clock is not required in codec slave mode, it's provided by the SOC. > > The clock is required when the codec is configured as master. Without these > GPIO selection there will be no audio. So yes we could move this to > devm_clk_get_optional() and change the test to IS_ERR_OR_NULL. Hmm... I do not understand. If it's optional, why to check for NULL? Perhaps you need to split code to show explicitly master / slave paths and for the first one call everything w/o _optinal() suffix? > > > > > + goto skip_dacpro; > > > > > + }
diff --git a/sound/soc/intel/boards/sof_pcm512x.c b/sound/soc/intel/boards/sof_pcm512x.c index dcd769b352fa..c1d2a53c1ec8 100644 --- a/sound/soc/intel/boards/sof_pcm512x.c +++ b/sound/soc/intel/boards/sof_pcm512x.c @@ -46,6 +46,8 @@ struct sof_card_private { struct list_head hdmi_pcm_list; bool idisp_codec; struct gpio_desc *gpio_4; + struct clk *sclk; + bool is_dac_pro; }; static int sof_pcm512x_quirk_cb(const struct dmi_system_id *id) @@ -87,6 +89,59 @@ static int sof_hdmi_init(struct snd_soc_pcm_runtime *rtd) static int sof_pcm512x_codec_init(struct snd_soc_pcm_runtime *rtd) { + struct sof_card_private *ctx = snd_soc_card_get_drvdata(rtd->card); + struct device *dev = rtd->card->dev; + unsigned int sck; + int ret; + + ctx->sclk = devm_clk_get(rtd->card->dev, "sclk"); + if (IS_ERR(ctx->sclk)) { + dev_info(dev, "Could not get SCLK, will operate in SOC master mode\n"); + goto skip_dacpro; + } + + /* + * now we have a clk, see if it's really present or if we are on + * plain vanilla DAC+ + */ + + /* Try 48 kHz */ + clk_set_rate(ctx->sclk, 24576000UL); + ret = clk_prepare_enable(ctx->sclk); + if (ret) { + dev_info(dev, "Failed to enable SCLK for DAC+ PRO 48 kHz: %d\n", ret); + goto skip_dacpro; + } + + snd_soc_component_read(rtd->codec_dai->component, + PCM512x_RATE_DET_4, &sck); + clk_disable_unprepare(ctx->sclk); + if (sck & 0x40) { + dev_info(dev, "No SCLK detected for DAC+ PRO 48 kHz\n"); + goto skip_dacpro; + } + + /* Try 44.1 kHz */ + clk_set_rate(ctx->sclk, 22579200UL); + ret = clk_prepare_enable(ctx->sclk); + if (ret) { + dev_info(dev, "Failed to enable SCLK for DAC+ PRO 44.1 kHz: %d\n", ret); + goto skip_dacpro; + } + + snd_soc_component_read(rtd->codec_dai->component, + PCM512x_RATE_DET_4, &sck); + clk_disable_unprepare(ctx->sclk); + + if (sck & 0x40) { + dev_info(dev, "No SCLK detected for DAC+ PRO 44.1 kHz\n"); + goto skip_dacpro; + } + + dev_info(dev, "DAC+ PRO detected\n"); + ctx->is_dac_pro = true; + +skip_dacpro: return 0; }
Try to detect HifiBerry 44.1 and 48kHz oscillators on codec init Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- sound/soc/intel/boards/sof_pcm512x.c | 55 ++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)