| Message ID | 1525334313-28661-1-git-send-email-akshu.agrawal@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03 May 2018 08:59, Akshu Agrawal wrote: > Non-dts based systems can use ACPI DSDT to pass on the mclk > to da7219. > This enables da7219 mclk to be linked to system clock. > Enable/Disable of the mclk is already handled in the codec so > platform drivers don't have to explicitly do handling of mclk. > > Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com> > --- > v2: Fixed kbuild error > v3: Add corresponding clk_put for clk_get > include/sound/da7219.h | 2 ++ > sound/soc/codecs/da7219.c | 10 +++++++++- > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/include/sound/da7219.h b/include/sound/da7219.h > index 1bfcb16..df7ddf4 100644 > --- a/include/sound/da7219.h > +++ b/include/sound/da7219.h > @@ -38,6 +38,8 @@ struct da7219_pdata { > > const char *dai_clks_name; > > + const char *mclk_name; > + > /* Mic */ > enum da7219_micbias_voltage micbias_lvl; > enum da7219_mic_amp_in_sel mic_amp_in_sel; > diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c > index 980a6a8..ecd46fc 100644 > --- a/sound/soc/codecs/da7219.c > +++ b/sound/soc/codecs/da7219.c > @@ -1624,6 +1624,8 @@ static struct da7219_pdata *da7219_fw_to_pdata(struct > snd_soc_component *compone > dev_warn(dev, "Using default clk name: %s\n", > pdata->dai_clks_name); > > + device_property_read_string(dev, "dlg,mclk-name", &pdata->mclk_name); > + Personally am still not keen on this. To me the use of a device_property_* function suggests the same property resides in both DT and ACPI, but here we're only using this for the ACPI case. DT has no want or need for this. I still feel we should look at something more generic in the clock framework, although I do agree with Mark that this should be properly specced. > if (device_property_read_u32(dev, "dlg,micbias-lvl", &of_val32) >= 0) > pdata->micbias_lvl = da7219_fw_micbias_lvl(dev, of_val32); > else > @@ -1905,7 +1907,10 @@ static int da7219_probe(struct snd_soc_component > *component) > da7219_handle_pdata(component); > > /* Check if MCLK provided */ > - da7219->mclk = devm_clk_get(component->dev, "mclk"); > + if (da7219->pdata->mclk_name) > + da7219->mclk = clk_get(NULL, da7219->pdata->mclk_name); > + if (!da7219->mclk) > + da7219->mclk = devm_clk_get(component->dev, "mclk"); > if (IS_ERR(da7219->mclk)) { > if (PTR_ERR(da7219->mclk) != -ENOENT) { > ret = PTR_ERR(da7219->mclk); > @@ -1971,6 +1976,9 @@ static void da7219_remove(struct snd_soc_component > *component) > clkdev_drop(da7219->dai_clks_lookup); > #endif > > + if (da7219->pdata->mclk_name) > + clk_put(da7219->mclk); > + > /* Supplies */ > regulator_bulk_disable(DA7219_NUM_SUPPLIES, da7219->supplies); > } > -- > 1.9.1
On 5/4/2018 2:45 PM, Adam Thomson wrote: > On 03 May 2018 08:59, Akshu Agrawal wrote: > >> Non-dts based systems can use ACPI DSDT to pass on the mclk >> to da7219. >> This enables da7219 mclk to be linked to system clock. >> Enable/Disable of the mclk is already handled in the codec so >> platform drivers don't have to explicitly do handling of mclk. >> >> Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com> >> --- >> v2: Fixed kbuild error >> v3: Add corresponding clk_put for clk_get >> include/sound/da7219.h | 2 ++ >> sound/soc/codecs/da7219.c | 10 +++++++++- >> 2 files changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/include/sound/da7219.h b/include/sound/da7219.h >> index 1bfcb16..df7ddf4 100644 >> --- a/include/sound/da7219.h >> +++ b/include/sound/da7219.h >> @@ -38,6 +38,8 @@ struct da7219_pdata { >> >> const char *dai_clks_name; >> >> + const char *mclk_name; >> + >> /* Mic */ >> enum da7219_micbias_voltage micbias_lvl; >> enum da7219_mic_amp_in_sel mic_amp_in_sel; >> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c >> index 980a6a8..ecd46fc 100644 >> --- a/sound/soc/codecs/da7219.c >> +++ b/sound/soc/codecs/da7219.c >> @@ -1624,6 +1624,8 @@ static struct da7219_pdata *da7219_fw_to_pdata(struct >> snd_soc_component *compone >> dev_warn(dev, "Using default clk name: %s\n", >> pdata->dai_clks_name); >> >> + device_property_read_string(dev, "dlg,mclk-name", &pdata->mclk_name); >> + > > Personally am still not keen on this. To me the use of a device_property_* > function suggests the same property resides in both DT and ACPI, but here we're > only using this for the ACPI case. DT has no want or need for this. I still feel > we should look at something more generic in the clock framework, although I do > agree with Mark that this should be properly specced. > I am not an expert in field of ACPI, IMO forming a Spec and changing ACPI to have DT like clock framework is good to have but a bigger change which should be taken up later. The current code of handling of mclk in the driver is usable only by DT. The device_property (though ACPI specific) makes this code, a common code for DT and ACPI based devices. https://www.kernel.org/doc/Documentation/acpi/DSD-properties-rules.txt "....Still, for the sake of code re-use, it may make sense to provide as much of the configuration data as possible in the form of device properties and complement that with an ACPI-specific mechanism suitable for the use case at hand......" Thanks, Akshu
On Sun, May 6, 2018 at 10:50 PM Agrawal, Akshu <Akshu.Agrawal@amd.com> wrote: > On 5/4/2018 2:45 PM, Adam Thomson wrote: > > On 03 May 2018 08:59, Akshu Agrawal wrote: > > > >> Non-dts based systems can use ACPI DSDT to pass on the mclk > >> to da7219. > >> This enables da7219 mclk to be linked to system clock. > >> Enable/Disable of the mclk is already handled in the codec so > >> platform drivers don't have to explicitly do handling of mclk. > >> > >> Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com> > >> --- > >> v2: Fixed kbuild error > >> v3: Add corresponding clk_put for clk_get > >> include/sound/da7219.h | 2 ++ > >> sound/soc/codecs/da7219.c | 10 +++++++++- > >> 2 files changed, 11 insertions(+), 1 deletion(-) > >> > >> diff --git a/include/sound/da7219.h b/include/sound/da7219.h > >> index 1bfcb16..df7ddf4 100644 > >> --- a/include/sound/da7219.h > >> +++ b/include/sound/da7219.h > >> @@ -38,6 +38,8 @@ struct da7219_pdata { > >> > >> const char *dai_clks_name; > >> > >> + const char *mclk_name; > >> + > >> /* Mic */ > >> enum da7219_micbias_voltage micbias_lvl; > >> enum da7219_mic_amp_in_sel mic_amp_in_sel; > >> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c > >> index 980a6a8..ecd46fc 100644 > >> --- a/sound/soc/codecs/da7219.c > >> +++ b/sound/soc/codecs/da7219.c > >> @@ -1624,6 +1624,8 @@ static struct da7219_pdata *da7219_fw_to_pdata(struct > >> snd_soc_component *compone > >> dev_warn(dev, "Using default clk name: %s\n", > >> pdata->dai_clks_name); > >> > >> + device_property_read_string(dev, "dlg,mclk-name", &pdata->mclk_name); > >> + > > > > Personally am still not keen on this. To me the use of a device_property_* > > function suggests the same property resides in both DT and ACPI, but here we're > > only using this for the ACPI case. DT has no want or need for this. I still feel > > we should look at something more generic in the clock framework, although I do > > agree with Mark that this should be properly specced. > > > I am not an expert in field of ACPI, IMO forming a Spec and changing > ACPI to have DT like clock framework is good to have but a bigger change > which should be taken up later. > The current code of handling of mclk in the driver is usable only by DT. > The device_property (though ACPI specific) makes this code, a common > code for DT and ACPI based devices. > https://www.kernel.org/doc/Documentation/acpi/DSD-properties-rules.txt > "....Still, for the sake of code re-use, it may make sense to provide as > much of the configuration data as possible in the form of device > properties and complement that with an ACPI-specific mechanism suitable > for the use case at hand......" This sounds like a pretty reasonable justification for addressing the issue using DSD to me. For what its worth, you can add: Reviewed-by: Daniel Kurtz <djkurtz@chromium.org> > Thanks, > Akshu
On 5/7/2018 12:09 PM, Daniel Kurtz wrote: > On Sun, May 6, 2018 at 10:50 PM Agrawal, Akshu <Akshu.Agrawal@amd.com> > wrote: > > > >> On 5/4/2018 2:45 PM, Adam Thomson wrote: >>> On 03 May 2018 08:59, Akshu Agrawal wrote: >>> >>>> Non-dts based systems can use ACPI DSDT to pass on the mclk >>>> to da7219. >>>> This enables da7219 mclk to be linked to system clock. >>>> Enable/Disable of the mclk is already handled in the codec so >>>> platform drivers don't have to explicitly do handling of mclk. >>>> >>>> Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com> >>>> --- >>>> v2: Fixed kbuild error >>>> v3: Add corresponding clk_put for clk_get >>>> include/sound/da7219.h | 2 ++ >>>> sound/soc/codecs/da7219.c | 10 +++++++++- >>>> 2 files changed, 11 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/include/sound/da7219.h b/include/sound/da7219.h >>>> index 1bfcb16..df7ddf4 100644 >>>> --- a/include/sound/da7219.h >>>> +++ b/include/sound/da7219.h >>>> @@ -38,6 +38,8 @@ struct da7219_pdata { >>>> >>>> const char *dai_clks_name; >>>> >>>> + const char *mclk_name; >>>> + >>>> /* Mic */ >>>> enum da7219_micbias_voltage micbias_lvl; >>>> enum da7219_mic_amp_in_sel mic_amp_in_sel; >>>> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c >>>> index 980a6a8..ecd46fc 100644 >>>> --- a/sound/soc/codecs/da7219.c >>>> +++ b/sound/soc/codecs/da7219.c >>>> @@ -1624,6 +1624,8 @@ static struct da7219_pdata > *da7219_fw_to_pdata(struct >>>> snd_soc_component *compone >>>> dev_warn(dev, "Using default clk name: %s\n", >>>> pdata->dai_clks_name); >>>> >>>> + device_property_read_string(dev, "dlg,mclk-name", > &pdata->mclk_name); >>>> + >>> >>> Personally am still not keen on this. To me the use of a > device_property_* >>> function suggests the same property resides in both DT and ACPI, but > here we're >>> only using this for the ACPI case. DT has no want or need for this. I > still feel >>> we should look at something more generic in the clock framework, > although I do >>> agree with Mark that this should be properly specced. >>> > >> I am not an expert in field of ACPI, IMO forming a Spec and changing >> ACPI to have DT like clock framework is good to have but a bigger change >> which should be taken up later. > >> The current code of handling of mclk in the driver is usable only by DT. >> The device_property (though ACPI specific) makes this code, a common >> code for DT and ACPI based devices. > >> https://www.kernel.org/doc/Documentation/acpi/DSD-properties-rules.txt >> "....Still, for the sake of code re-use, it may make sense to provide as >> much of the configuration data as possible in the form of device >> properties and complement that with an ACPI-specific mechanism suitable >> for the use case at hand......" > > This sounds like a pretty reasonable justification for addressing the issue > using DSD to me. > For what its worth, you can add: > > Reviewed-by: Daniel Kurtz <djkurtz@chromium.org> > > Hi Mark, If you are Ok with this patch can you please take this. Thanks, Akshu
On 07 May 2018 05:50, Agrawal, Akshu wrote: Apologies for the delay in response. Please see comments below. > On 5/4/2018 2:45 PM, Adam Thomson wrote: > > On 03 May 2018 08:59, Akshu Agrawal wrote: > > > >> Non-dts based systems can use ACPI DSDT to pass on the mclk > >> to da7219. > >> This enables da7219 mclk to be linked to system clock. > >> Enable/Disable of the mclk is already handled in the codec so > >> platform drivers don't have to explicitly do handling of mclk. > >> > >> Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com> > >> --- > >> v2: Fixed kbuild error > >> v3: Add corresponding clk_put for clk_get > >> include/sound/da7219.h | 2 ++ > >> sound/soc/codecs/da7219.c | 10 +++++++++- > >> 2 files changed, 11 insertions(+), 1 deletion(-) > >> > >> diff --git a/include/sound/da7219.h b/include/sound/da7219.h > >> index 1bfcb16..df7ddf4 100644 > >> --- a/include/sound/da7219.h > >> +++ b/include/sound/da7219.h > >> @@ -38,6 +38,8 @@ struct da7219_pdata { > >> > >> const char *dai_clks_name; > >> > >> + const char *mclk_name; > >> + > >> /* Mic */ > >> enum da7219_micbias_voltage micbias_lvl; > >> enum da7219_mic_amp_in_sel mic_amp_in_sel; > >> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c > >> index 980a6a8..ecd46fc 100644 > >> --- a/sound/soc/codecs/da7219.c > >> +++ b/sound/soc/codecs/da7219.c > >> @@ -1624,6 +1624,8 @@ static struct da7219_pdata > *da7219_fw_to_pdata(struct > >> snd_soc_component *compone > >> dev_warn(dev, "Using default clk name: %s\n", > >> pdata->dai_clks_name); > >> > >> + device_property_read_string(dev, "dlg,mclk-name", &pdata->mclk_name); > >> + > > > > Personally am still not keen on this. To me the use of a device_property_* > > function suggests the same property resides in both DT and ACPI, but here we're > > only using this for the ACPI case. DT has no want or need for this. I still feel > > we should look at something more generic in the clock framework, although I do > > agree with Mark that this should be properly specced. > > > > I am not an expert in field of ACPI, IMO forming a Spec and changing > ACPI to have DT like clock framework is good to have but a bigger change > which should be taken up later. That's fair enough, but I wonder who will take that on. Really would be beneficial to have something generic on the ACPI side. > The current code of handling of mclk in the driver is usable only by DT. > The device_property (though ACPI specific) makes this code, a common > code for DT and ACPI based devices. > > https://www.kernel.org/doc/Documentation/acpi/DSD-properties-rules.txt > "....Still, for the sake of code re-use, it may make sense to provide as > much of the configuration data as possible in the form of device > properties and complement that with an ACPI-specific mechanism suitable > for the use case at hand......" Given that on the DT side there's the expectation that the generic 'clock-names' property might be provided to the driver, could we not reuse that rather than introducing 'dlg,mclk-name' and diverging DT and ACPI properties? > > Thanks, > Akshu
On Tue, May 15, 2018 at 03:13:47PM +0530, Agrawal, Akshu wrote:
> If you are Ok with this patch can you please take this.
Please don't send content free pings and please allow a reasonable time
for review. People get busy, go on holiday, attend conferences and so
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review. If there have been
review comments then people may be waiting for those to be addressed.
Sending content free pings just adds to the mail volume (if they are
seen at all) and if something has gone wrong you'll have to resend the
patches anyway.
diff --git a/include/sound/da7219.h b/include/sound/da7219.h index 1bfcb16..df7ddf4 100644 --- a/include/sound/da7219.h +++ b/include/sound/da7219.h @@ -38,6 +38,8 @@ struct da7219_pdata { const char *dai_clks_name; + const char *mclk_name; + /* Mic */ enum da7219_micbias_voltage micbias_lvl; enum da7219_mic_amp_in_sel mic_amp_in_sel; diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c index 980a6a8..ecd46fc 100644 --- a/sound/soc/codecs/da7219.c +++ b/sound/soc/codecs/da7219.c @@ -1624,6 +1624,8 @@ static struct da7219_pdata *da7219_fw_to_pdata(struct snd_soc_component *compone dev_warn(dev, "Using default clk name: %s\n", pdata->dai_clks_name); + device_property_read_string(dev, "dlg,mclk-name", &pdata->mclk_name); + if (device_property_read_u32(dev, "dlg,micbias-lvl", &of_val32) >= 0) pdata->micbias_lvl = da7219_fw_micbias_lvl(dev, of_val32); else @@ -1905,7 +1907,10 @@ static int da7219_probe(struct snd_soc_component *component) da7219_handle_pdata(component); /* Check if MCLK provided */ - da7219->mclk = devm_clk_get(component->dev, "mclk"); + if (da7219->pdata->mclk_name) + da7219->mclk = clk_get(NULL, da7219->pdata->mclk_name); + if (!da7219->mclk) + da7219->mclk = devm_clk_get(component->dev, "mclk"); if (IS_ERR(da7219->mclk)) { if (PTR_ERR(da7219->mclk) != -ENOENT) { ret = PTR_ERR(da7219->mclk); @@ -1971,6 +1976,9 @@ static void da7219_remove(struct snd_soc_component *component) clkdev_drop(da7219->dai_clks_lookup); #endif + if (da7219->pdata->mclk_name) + clk_put(da7219->mclk); + /* Supplies */ regulator_bulk_disable(DA7219_NUM_SUPPLIES, da7219->supplies); }
Non-dts based systems can use ACPI DSDT to pass on the mclk to da7219. This enables da7219 mclk to be linked to system clock. Enable/Disable of the mclk is already handled in the codec so platform drivers don't have to explicitly do handling of mclk. Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com> --- v2: Fixed kbuild error v3: Add corresponding clk_put for clk_get include/sound/da7219.h | 2 ++ sound/soc/codecs/da7219.c | 10 +++++++++- 2 files changed, 11 insertions(+), 1 deletion(-)