[PATCH v2] ASoC: da7219: read fmw property to get mclk for non-dts systems

[PATCH v2] ASoC: da7219: read fmw property to get mclk for non-dts systems

[Akshu Agrawal]

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
 include/sound/da7219.h    | 2 ++
 sound/soc/codecs/da7219.c | 7 ++++++-
 2 files changed, 8 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..aed68a4 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);
-- 
1.9.1

[PATCH v2] ASoC: da7219: read fmw property to get mclk for non-dts systems

[Akshu Agrawal]

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
 include/sound/da7219.h    | 2 ++
 sound/soc/codecs/da7219.c | 7 ++++++-
 2 files changed, 8 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..aed68a4 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);
-- 
1.9.1

Re: [alsa-devel] [PATCH v2] ASoC: da7219: read fmw property to get mclk for non-dts systems

[Pierre-Louis Bossart]

On 4/30/18 4:23 AM, 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
>   include/sound/da7219.h    | 2 ++
>   sound/soc/codecs/da7219.c | 7 ++++++-
>   2 files changed, 8 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..aed68a4 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");

this looks weird, why are you using different clk functions depending on 
the existence of a _DSD property? Why not just change the name and keep 
the same flow, e.g something like

if(!da7219->pdata->mclk_name)
	da7219->pdata->mclk_name = "mclk";
da7219->mclk = devm_clk_get(component->dev, da7219->pdata->mclk_name);


>   	if (IS_ERR(da7219->mclk)) {
>   		if (PTR_ERR(da7219->mclk) != -ENOENT) {
>   			ret = PTR_ERR(da7219->mclk);
> 

RE: [PATCH v2] ASoC: da7219: read fmw property to get mclk for non-dts systems

[Adam Thomson]

On 30 April 2018 10:23, 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.

There is already a means via DT to specify the MCLK for a device using the
generic clock DT bindings, and this driver already uses that. Should ACPI not
have something similar to that which is generic, rather than adding device
specific bindings/properties to achieve the same? There will be other drivers
that will want to do the same.

>
> Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com>
> ---
> v2: Fixed kbuild error
>  include/sound/da7219.h    | 2 ++
>  sound/soc/codecs/da7219.c | 7 ++++++-
>  2 files changed, 8 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..aed68a4 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);

By doing this you would need an associated 'clk_put()' whereas the devm call
avoids this.

> +	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);
> --
> 1.9.1

Re: [alsa-devel] [PATCH v2] ASoC: da7219: read fmw property to get mclk for non-dts systems

[Agrawal, Akshu]

On 4/30/2018 9:54 PM, Pierre-Louis Bossart wrote:
> On 4/30/18 4:23 AM, 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
>>   include/sound/da7219.h    | 2 ++
>>   sound/soc/codecs/da7219.c | 7 ++++++-
>>   2 files changed, 8 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..aed68a4 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");
> 
> this looks weird, why are you using different clk functions depending on 
> the existence of a _DSD property? Why not just change the name and keep 
> the same flow, e.g something like
> 
> if(!da7219->pdata->mclk_name)
>      da7219->pdata->mclk_name = "mclk";
> da7219->mclk = devm_clk_get(component->dev, da7219->pdata->mclk_name);
> 
> 

We can't use devm_clk_get as the value of dev argument has to be NULL, 
which can not be used with devm_clk_get.
System clock which are linked to mclk are registered by a separate ACPI 
device. And this exposing of DSD property is for all those platforms 
which are non-dts based.

>>       if (IS_ERR(da7219->mclk)) {
>>           if (PTR_ERR(da7219->mclk) != -ENOENT) {
>>               ret = PTR_ERR(da7219->mclk);
>>
> 

Re: [PATCH v2] ASoC: da7219: read fmw property to get mclk for non-dts systems

[Agrawal, Akshu]

On 5/1/2018 12:35 AM, Adam Thomson wrote:
> On 30 April 2018 10:23, 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.
> 
> There is already a means via DT to specify the MCLK for a device using the
> generic clock DT bindings, and this driver already uses that. Should ACPI not
> have something similar to that which is generic, rather than adding device
> specific bindings/properties to achieve the same? There will be other drivers
> that will want to do the same.
> 

IMO for all non-dts based ACPI systems, DSDT would be the best and 
simplest way to link system clock to mclk. Currently, machine audio 
drivers handles the clock and this can be avoided by having this property.

>>
>> Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com>
>> ---
>> v2: Fixed kbuild error
>>   include/sound/da7219.h    | 2 ++
>>   sound/soc/codecs/da7219.c | 7 ++++++-
>>   2 files changed, 8 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..aed68a4 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);
> 
> By doing this you would need an associated 'clk_put()' whereas the devm call
> avoids this.
>

Agreed, would add a corresponding clk_put call.

Thanks,
Akshu

>> +	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);
>> --
>> 1.9.1

Re: [alsa-devel] [PATCH v2] ASoC: da7219: read fmw property to get mclk for non-dts systems

[Pierre-Louis Bossart]

On 5/1/18 9:31 AM, Agrawal, Akshu wrote:
> 
> 
> On 4/30/2018 9:54 PM, Pierre-Louis Bossart wrote:
>> On 4/30/18 4:23 AM, 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
>>>   include/sound/da7219.h    | 2 ++
>>>   sound/soc/codecs/da7219.c | 7 ++++++-
>>>   2 files changed, 8 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..aed68a4 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");
>>
>> this looks weird, why are you using different clk functions depending 
>> on the existence of a _DSD property? Why not just change the name and 
>> keep the same flow, e.g something like
>>
>> if(!da7219->pdata->mclk_name)
>>      da7219->pdata->mclk_name = "mclk";
>> da7219->mclk = devm_clk_get(component->dev, da7219->pdata->mclk_name);
>>
>>
> 
> We can't use devm_clk_get as the value of dev argument has to be NULL, 
> which can not be used with devm_clk_get.
> System clock which are linked to mclk are registered by a separate ACPI 
> device. And this exposing of DSD property is for all those platforms 
> which are non-dts based.

Alternatively you could modify clk_get to use device_property instead of 
of_property.

> 
>>>       if (IS_ERR(da7219->mclk)) {
>>>           if (PTR_ERR(da7219->mclk) != -ENOENT) {
>>>               ret = PTR_ERR(da7219->mclk);
>>>
>>

Re: [PATCH v2] ASoC: da7219: read fmw property to get mclk for non-dts systems

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1067 bytes --]

On Mon, Apr 30, 2018 at 07:05:19PM +0000, Adam Thomson wrote:

> There is already a means via DT to specify the MCLK for a device using the
> generic clock DT bindings, and this driver already uses that. Should ACPI not
> have something similar to that which is generic, rather than adding device
> specific bindings/properties to achieve the same? There will be other drivers
> that will want to do the same.

There's a lot of things that ACPI *should* do but doesn't - it's a bit
of a shambles how ACPI standards get defined and what's there is not
really intended to handle systems like these semi-embedded ones.  One of
the big gaps in ACPI is that it has no handling at all of clocks, that's
supposed to be done transparently by firmware in the ACPI model.  What a
lot of the embedded Intel people have been doing is coopting the DT
bindings wholesale for ACPI systems but that has problems when you get
into areas which should be handled in some way on ACPI systems like
power and unfortunately clocks are kind of power adjacent so might be a
bit sketchy here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] ASoC: da7219: read fmw property to get mclk for non-dts systems

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1067 bytes --]

On Mon, Apr 30, 2018 at 07:05:19PM +0000, Adam Thomson wrote:

> There is already a means via DT to specify the MCLK for a device using the
> generic clock DT bindings, and this driver already uses that. Should ACPI not
> have something similar to that which is generic, rather than adding device
> specific bindings/properties to achieve the same? There will be other drivers
> that will want to do the same.

There's a lot of things that ACPI *should* do but doesn't - it's a bit
of a shambles how ACPI standards get defined and what's there is not
really intended to handle systems like these semi-embedded ones.  One of
the big gaps in ACPI is that it has no handling at all of clocks, that's
supposed to be done transparently by firmware in the ACPI model.  What a
lot of the embedded Intel people have been doing is coopting the DT
bindings wholesale for ACPI systems but that has problems when you get
into areas which should be handled in some way on ACPI systems like
power and unfortunately clocks are kind of power adjacent so might be a
bit sketchy here.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

RE: [PATCH v2] ASoC: da7219: read fmw property to get mclk for non-dts systems

[Adam Thomson]

On 01 May 2018 21:50, Mark Brown wrote:

> On Mon, Apr 30, 2018 at 07:05:19PM +0000, Adam Thomson wrote:
> 
> > There is already a means via DT to specify the MCLK for a device using the
> > generic clock DT bindings, and this driver already uses that. Should ACPI not
> > have something similar to that which is generic, rather than adding device
> > specific bindings/properties to achieve the same? There will be other drivers
> > that will want to do the same.
> 
> There's a lot of things that ACPI *should* do but doesn't - it's a bit
> of a shambles how ACPI standards get defined and what's there is not
> really intended to handle systems like these semi-embedded ones.  One of
> the big gaps in ACPI is that it has no handling at all of clocks, that's
> supposed to be done transparently by firmware in the ACPI model.  What a
> lot of the embedded Intel people have been doing is coopting the DT
> bindings wholesale for ACPI systems but that has problems when you get
> into areas which should be handled in some way on ACPI systems like
> power and unfortunately clocks are kind of power adjacent so might be a
> bit sketchy here.

Yes I was aware that previously that was the case, although have not followed
this for a while. It just feels here that we should aim for something more
generic rather than a device specific property/binding, if possible, as that
feels messy to me and I'm sure other drivers could take advantage of this as
well. I've not looked at the clock code in too much detail though, at least with
regards to this area, so not sure how feasible that is.

As a suggestion for ACPI would it be possible to re-use the 'clock-names'
property and add something in the framework to handle this?

Re: [PATCH v2] ASoC: da7219: read fmw property to get mclk for non-dts systems

[Adam Thomson]

On 01 May 2018 21:50, Mark Brown wrote:

> On Mon, Apr 30, 2018 at 07:05:19PM +0000, Adam Thomson wrote:
> 
> > There is already a means via DT to specify the MCLK for a device using the
> > generic clock DT bindings, and this driver already uses that. Should ACPI not
> > have something similar to that which is generic, rather than adding device
> > specific bindings/properties to achieve the same? There will be other drivers
> > that will want to do the same.
> 
> There's a lot of things that ACPI *should* do but doesn't - it's a bit
> of a shambles how ACPI standards get defined and what's there is not
> really intended to handle systems like these semi-embedded ones.  One of
> the big gaps in ACPI is that it has no handling at all of clocks, that's
> supposed to be done transparently by firmware in the ACPI model.  What a
> lot of the embedded Intel people have been doing is coopting the DT
> bindings wholesale for ACPI systems but that has problems when you get
> into areas which should be handled in some way on ACPI systems like
> power and unfortunately clocks are kind of power adjacent so might be a
> bit sketchy here.

Yes I was aware that previously that was the case, although have not followed
this for a while. It just feels here that we should aim for something more
generic rather than a device specific property/binding, if possible, as that
feels messy to me and I'm sure other drivers could take advantage of this as
well. I've not looked at the clock code in too much detail though, at least with
regards to this area, so not sure how feasible that is.

As a suggestion for ACPI would it be possible to re-use the 'clock-names'
property and add something in the framework to handle this?

Re: [PATCH v2] ASoC: da7219: read fmw property to get mclk for non-dts systems

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1683 bytes --]

On Wed, May 02, 2018 at 10:13:55AM +0000, Adam Thomson wrote:
> On 01 May 2018 21:50, Mark Brown wrote:

> > There's a lot of things that ACPI *should* do but doesn't - it's a bit
> > of a shambles how ACPI standards get defined and what's there is not
> > really intended to handle systems like these semi-embedded ones.  One of
> > the big gaps in ACPI is that it has no handling at all of clocks, that's
> > supposed to be done transparently by firmware in the ACPI model.  What a
> > lot of the embedded Intel people have been doing is coopting the DT
> > bindings wholesale for ACPI systems but that has problems when you get
> > into areas which should be handled in some way on ACPI systems like
> > power and unfortunately clocks are kind of power adjacent so might be a
> > bit sketchy here.

> Yes I was aware that previously that was the case, although have not followed
> this for a while. It just feels here that we should aim for something more
> generic rather than a device specific property/binding, if possible, as that
> feels messy to me and I'm sure other drivers could take advantage of this as
> well. I've not looked at the clock code in too much detail though, at least with
> regards to this area, so not sure how feasible that is.

> As a suggestion for ACPI would it be possible to re-use the 'clock-names'
> property and add something in the framework to handle this?

I completely agree that ACPI should have handling for clocks but it
really feels like something that should be done as a proper spec rather
than just ad hoc by Linux like the x86 embedded people often do - it's
too close to the power management stuff that ACPI definitely does
handle.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] ASoC: da7219: read fmw property to get mclk for non-dts systems

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1683 bytes --]

On Wed, May 02, 2018 at 10:13:55AM +0000, Adam Thomson wrote:
> On 01 May 2018 21:50, Mark Brown wrote:

> > There's a lot of things that ACPI *should* do but doesn't - it's a bit
> > of a shambles how ACPI standards get defined and what's there is not
> > really intended to handle systems like these semi-embedded ones.  One of
> > the big gaps in ACPI is that it has no handling at all of clocks, that's
> > supposed to be done transparently by firmware in the ACPI model.  What a
> > lot of the embedded Intel people have been doing is coopting the DT
> > bindings wholesale for ACPI systems but that has problems when you get
> > into areas which should be handled in some way on ACPI systems like
> > power and unfortunately clocks are kind of power adjacent so might be a
> > bit sketchy here.

> Yes I was aware that previously that was the case, although have not followed
> this for a while. It just feels here that we should aim for something more
> generic rather than a device specific property/binding, if possible, as that
> feels messy to me and I'm sure other drivers could take advantage of this as
> well. I've not looked at the clock code in too much detail though, at least with
> regards to this area, so not sure how feasible that is.

> As a suggestion for ACPI would it be possible to re-use the 'clock-names'
> property and add something in the framework to handle this?

I completely agree that ACPI should have handling for clocks but it
really feels like something that should be done as a proper spec rather
than just ad hoc by Linux like the x86 embedded people often do - it's
too close to the power management stuff that ACPI definitely does
handle.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

RE: [alsa-devel] [PATCH v2] ASoC: da7219: read fmw property to get mclk for non-dts systems

[Adam Thomson]

On 03 May 2018 02:39, Mark Brown wrote:

> On Wed, May 02, 2018 at 10:13:55AM +0000, Adam Thomson wrote:
> > On 01 May 2018 21:50, Mark Brown wrote:
> 
> > > There's a lot of things that ACPI *should* do but doesn't - it's a bit
> > > of a shambles how ACPI standards get defined and what's there is not
> > > really intended to handle systems like these semi-embedded ones.  One of
> > > the big gaps in ACPI is that it has no handling at all of clocks, that's
> > > supposed to be done transparently by firmware in the ACPI model.  What a
> > > lot of the embedded Intel people have been doing is coopting the DT
> > > bindings wholesale for ACPI systems but that has problems when you get
> > > into areas which should be handled in some way on ACPI systems like
> > > power and unfortunately clocks are kind of power adjacent so might be a
> > > bit sketchy here.
> 
> > Yes I was aware that previously that was the case, although have not followed
> > this for a while. It just feels here that we should aim for something more
> > generic rather than a device specific property/binding, if possible, as that
> > feels messy to me and I'm sure other drivers could take advantage of this as
> > well. I've not looked at the clock code in too much detail though, at least with
> > regards to this area, so not sure how feasible that is.
> 
> > As a suggestion for ACPI would it be possible to re-use the 'clock-names'
> > property and add something in the framework to handle this?
> 
> I completely agree that ACPI should have handling for clocks but it
> really feels like something that should be done as a proper spec rather
> than just ad hoc by Linux like the x86 embedded people often do - it's
> too close to the power management stuff that ACPI definitely does
> handle.

Yep, agreed. Be nice if someone could do that rather than us having workarounds.

Re: [PATCH v2] ASoC: da7219: read fmw property to get mclk for non-dts systems

[Adam Thomson]

On 03 May 2018 02:39, Mark Brown wrote:

> On Wed, May 02, 2018 at 10:13:55AM +0000, Adam Thomson wrote:
> > On 01 May 2018 21:50, Mark Brown wrote:
> 
> > > There's a lot of things that ACPI *should* do but doesn't - it's a bit
> > > of a shambles how ACPI standards get defined and what's there is not
> > > really intended to handle systems like these semi-embedded ones.  One of
> > > the big gaps in ACPI is that it has no handling at all of clocks, that's
> > > supposed to be done transparently by firmware in the ACPI model.  What a
> > > lot of the embedded Intel people have been doing is coopting the DT
> > > bindings wholesale for ACPI systems but that has problems when you get
> > > into areas which should be handled in some way on ACPI systems like
> > > power and unfortunately clocks are kind of power adjacent so might be a
> > > bit sketchy here.
> 
> > Yes I was aware that previously that was the case, although have not followed
> > this for a while. It just feels here that we should aim for something more
> > generic rather than a device specific property/binding, if possible, as that
> > feels messy to me and I'm sure other drivers could take advantage of this as
> > well. I've not looked at the clock code in too much detail though, at least with
> > regards to this area, so not sure how feasible that is.
> 
> > As a suggestion for ACPI would it be possible to re-use the 'clock-names'
> > property and add something in the framework to handle this?
> 
> I completely agree that ACPI should have handling for clocks but it
> really feels like something that should be done as a proper spec rather
> than just ad hoc by Linux like the x86 embedded people often do - it's
> too close to the power management stuff that ACPI definitely does
> handle.

Yep, agreed. Be nice if someone could do that rather than us having workarounds.