[v4] ASoC: AMD: Enable/Disable auxiliary clock via common clock framework
diff mbox

Message ID 1522054560-31088-1-git-send-email-akshu.agrawal@amd.com
State New
Headers show

Commit Message

Agrawal, Akshu March 26, 2018, 8:56 a.m. UTC
This enables/disables and sets auxiliary clock at 25Mhz. It uses
common clock framework for proper ref counting. This approach will
save power in comparison to keeping it always On in firmware.

V2: Correcting the pin to OSCOUT1 from OSCOUT2
V3: Fix error/warnings from kbuild test
V4: Fix build errors for platform which do not support CONFIG_COMMON_CLK

TEST= aplay -vv <file>
check register to see clock enabled
kill aplay
check register to see clock disabled

Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com>
Acked-by: Alex Deucher <alexander.deucher@amd.com>
---
 sound/soc/amd/acp-da7219-max98357a.c | 144 ++++++++++++++++++++++++++++++++++-
 1 file changed, 142 insertions(+), 2 deletions(-)

Comments

Daniel Kurtz March 27, 2018, 4:22 a.m. UTC | #1
Hi Akshu

On Mon, Mar 26, 2018 at 3:12 AM Akshu Agrawal <akshu.agrawal@amd.com> wrote:

> This enables/disables and sets auxiliary clock at 25Mhz. It uses
> common clock framework for proper ref counting. This approach will
> save power in comparison to keeping it always On in firmware.

> V2: Correcting the pin to OSCOUT1 from OSCOUT2
> V3: Fix error/warnings from kbuild test
> V4: Fix build errors for platform which do not support CONFIG_COMMON_CLK

> TEST= aplay -vv <file>
> check register to see clock enabled
> kill aplay
> check register to see clock disabled

> Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com>
> Acked-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>   sound/soc/amd/acp-da7219-max98357a.c | 144
++++++++++++++++++++++++++++++++++-
>   1 file changed, 142 insertions(+), 2 deletions(-)

> diff --git a/sound/soc/amd/acp-da7219-max98357a.c
b/sound/soc/amd/acp-da7219-max98357a.c
> index 99c6b5c..3c77803 100644
> --- a/sound/soc/amd/acp-da7219-max98357a.c
> +++ b/sound/soc/amd/acp-da7219-max98357a.c
> @@ -30,10 +30,13 @@
>   #include <sound/soc-dapm.h>
>   #include <sound/jack.h>
>   #include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
>   #include <linux/gpio.h>
>   #include <linux/module.h>
>   #include <linux/i2c.h>
>   #include <linux/acpi.h>
> +#include <linux/types.h>

>   #include "../codecs/da7219.h"
>   #include "../codecs/da7219-aad.h"
> @@ -41,6 +44,20 @@
>   #define CZ_PLAT_CLK 24000000
>   #define MCLK_RATE 24576000
>   #define DUAL_CHANNEL           2
> +#define CLKDRVSTR2     0x28
> +#define MISCCLKCNTL1   0x40
> +#define OSCCLKENB      2
> +#define OSCOUT1CLK25MHZ        16

These defines do not belong with the clocks above them, so please separate
them.
It might be helpful to add a comment describing what these addresses are,
ie that they are registers of the SoC.

> +
> +struct cz_clock {
> +       const char* acp_clks_name;

Why store this in the struct?

> +#ifdef CONFIG_COMMON_CLK
> +       struct clk_hw acp_clks_hw;
> +#endif
> +       struct clk_lookup *acp_clks_lookup;

Why store this in the struct?

> +       struct clk *acp_clks;

Why store this in the struct?

> +       void __iomem *res_base;
> +};

>   static struct snd_soc_jack cz_jack;
>   struct clk *da7219_dai_clk;
> @@ -91,6 +108,8 @@ static int cz_da7219_hw_params(struct
snd_pcm_substream *substream,
>   {
>          int ret = 0;
>          struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +       struct snd_soc_card *card = rtd->card;
> +       struct clk *acpd7219_clk;

>          ret = clk_prepare_enable(da7219_dai_clk);
>          if (ret < 0) {
> @@ -98,13 +117,27 @@ static int cz_da7219_hw_params(struct
snd_pcm_substream *substream,
>                  return ret;
>          }

> +       acpd7219_clk = clk_get(card->dev, "acpd7219-clks");
> +       ret = clk_prepare_enable(acpd7219_clk);
> +       if (ret < 0) {
> +               dev_err(rtd->dev, "can't enable oscillator clock %d\n",
ret);
> +               return ret;
> +       }
> +
>          return ret;
>   }

>   static int cz_da7219_hw_free(struct snd_pcm_substream *substream)
>   {
> +       struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +       struct snd_soc_card *card = rtd->card;
> +       struct clk *acpd7219_clk;
> +
>          clk_disable_unprepare(da7219_dai_clk);

> +       acpd7219_clk = clk_get(card->dev, "acpd7219-clks");
> +       clk_disable_unprepare(acpd7219_clk);
> +
>          return 0;
>   }

> @@ -237,9 +270,113 @@ static int cz_fe_startup(struct snd_pcm_substream
*substream)
>          .num_controls = ARRAY_SIZE(cz_mc_controls),
>   };

> +#ifdef CONFIG_COMMON_CLK
> +static int acpd7219_clks_prepare(struct clk_hw *hw)

Should setting OSCCLKENB be the clock enable rather than prepare?

> +{
> +       u32 reg_val;
> +       struct cz_clock *cz_clock_obj =
> +               container_of(hw, struct cz_clock, acp_clks_hw);

nit: just "cz_clock" should be enough for all of these.

> +       void __iomem *base = cz_clock_obj->res_base;
> +
> +       reg_val = readl(base + MISCCLKCNTL1);
> +       reg_val &= ~(0x1 << OSCCLKENB);
> +       writel(reg_val, base + MISCCLKCNTL1);
> +       reg_val = readl(base + CLKDRVSTR2);
> +       reg_val |= (0x1 << OSCOUT1CLK25MHZ);
> +       writel(reg_val, base + CLKDRVSTR2);

Writing OSCOUT1CLK25MHZ sets the clock to 25 MHz (ie, instead of 48 MHz).
So, technically this part should probably be in a set_rate() rather than
prepare() [and reject other frequencies]?

> +
> +       return 0;
> +}
> +
> +static void acpd7219_clks_unprepare(struct clk_hw *hw)

Similarly, can this be disable()?

> +{
> +       u32 reg_val;
> +       struct cz_clock *cz_clock_obj =
> +               container_of(hw, struct cz_clock, acp_clks_hw);
> +       void __iomem *base = cz_clock_obj->res_base;
> +
> +       reg_val = readl(base + MISCCLKCNTL1);
> +       reg_val |= (0x1 << OSCCLKENB);
> +       writel(reg_val, base + MISCCLKCNTL1);
> +}
> +
> +static int acpd7219_clks_is_enabled(struct clk_hw *hw)
> +{
> +       u32 reg_val;
> +       struct cz_clock *cz_clock_obj =
> +               container_of(hw, struct cz_clock, acp_clks_hw);
> +       void __iomem *base = cz_clock_obj->res_base;
> +
> +       reg_val = readl(base + MISCCLKCNTL1);
> +
> +       return !(reg_val & OSCCLKENB);
> +}
> +
> +const struct clk_ops acpd7219_clks_ops = {

static const ...

> +       .prepare = acpd7219_clks_prepare,
> +       .unprepare = acpd7219_clks_unprepare,
> +       .is_enabled = acpd7219_clks_is_enabled,
> +};
> +
> +static int register_acpd7219_clocks(struct platform_device *pdev)

Can this be:
__init

> +{
> +       struct clk_init_data init = {};
> +       struct clk *acp_clks;
> +       struct clk_lookup *acp_clks_lookup;
> +       struct cz_clock *cz_clock_obj;
> +       struct resource *res;
> +       struct device dev = pdev->dev;
> +
> +       cz_clock_obj = kzalloc(sizeof(struct cz_clock), GFP_KERNEL);
> +       if (!cz_clock_obj)
> +               return -ENOMEM;
> +
> +       cz_clock_obj->acp_clks_name = "acpd7219-clks";
> +
> +       init.parent_names = NULL;
> +       init.num_parents = 0;
> +       init.name = cz_clock_obj->acp_clks_name;
> +       init.ops = &acpd7219_clks_ops;

I think you can just do this all during: "struct clk_init_data init = { }".
In fact, can't this done like this instead:

static const struct clk_init_data __initconst = {
   .name = "acpd7219-clks";
   .ops = &acpd7219_clks_ops;
};

> +       cz_clock_obj->acp_clks_hw.init = &init;
> +
> +       acp_clks = devm_clk_register(&dev, &cz_clock_obj->acp_clks_hw);
> +       if (IS_ERR(acp_clks))
> +       {
> +               dev_err(&dev, "Failed to register DAI clocks: %ld\n",
> +                        PTR_ERR(acp_clks));
> +               return -EINVAL;

propagate PTR_ERR(acp_clks), instead?

> +       }
> +       cz_clock_obj->acp_clks = acp_clks;
> +
> +       acp_clks_lookup = clkdev_create(acp_clks,
cz_clock_obj->acp_clks_name,
> +                                       "%s", dev_name(&dev));
> +       if (!acp_clks_lookup)
> +               dev_warn(&dev, "Failed to create DAI clkdev");
> +       else
> +               cz_clock_obj->acp_clks_lookup =
acp_clks_lookup;clkdev_create
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res) {
> +               dev_err(&pdev->dev, "Failed to get misc io resource.\n");
> +               return -EINVAL;
> +       }
> +       cz_clock_obj->res_base = devm_ioremap_nocache(&pdev->dev,
res->start,
> +                                       resource_size(res));
> +       if (!cz_clock_obj->res_base)
> +               return -ENOMEM;
> +
> +       return 0;
> +}
> +#else
> +static int register_acpd7219_clocks(struct platform_device *pdev)
> +{
> +       return 0;
> +}
> +#endif /* CONFIG_COMMON_CLK */
> +
>   static int cz_probe(struct platform_device *pdev)
>   {
> -       int ret;
> +       int ret = 0;

Do not initialize this before it is used.

>          struct snd_soc_card *card;

>          card = &cz_card;
> @@ -252,7 +389,10 @@ static int cz_probe(struct platform_device *pdev)
>                                  cz_card.name, ret);
>                  return ret;
>          }
> -       return 0;
> +
> +       ret = register_acpd7219_clocks(pdev);
> +
> +       return ret;

Just:
   return register_acpd7219_clocks(pdev);

Thanks,
-Dan

>   }

>   static const struct acpi_device_id cz_audio_acpi_match[] = {
> --
> 1.9.1
Sriram Periyasamy March 27, 2018, 6:46 p.m. UTC | #2
On Mon, Mar 26, 2018 at 02:26:00PM +0530, Akshu Agrawal wrote:
> This enables/disables and sets auxiliary clock at 25Mhz. It uses
> common clock framework for proper ref counting. This approach will
> save power in comparison to keeping it always On in firmware.
> 
> V2: Correcting the pin to OSCOUT1 from OSCOUT2
> V3: Fix error/warnings from kbuild test
> V4: Fix build errors for platform which do not support CONFIG_COMMON_CLK
> 

Please consider adding the change history across versions below
the marker --- which is the standard way of doing.

> TEST= aplay -vv <file>
> check register to see clock enabled
> kill aplay
> check register to see clock disabled
> 
> Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com>
> Acked-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  sound/soc/amd/acp-da7219-max98357a.c | 144 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 142 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/amd/acp-da7219-max98357a.c b/sound/soc/amd/acp-da7219-max98357a.c
> index 99c6b5c..3c77803 100644
> --- a/sound/soc/amd/acp-da7219-max98357a.c
> +++ b/sound/soc/amd/acp-da7219-max98357a.c

<snip>

> @@ -98,13 +117,27 @@ static int cz_da7219_hw_params(struct snd_pcm_substream *substream,
>  		return ret;
>  	}
>  
> +	acpd7219_clk = clk_get(card->dev, "acpd7219-clks");

Consider adding the error handling, for example if CONFIG_COMMON_CLK is not
defined for this platform, then clk_get may return error.

> +	ret = clk_prepare_enable(acpd7219_clk);
> +	if (ret < 0) {
> +		dev_err(rtd->dev, "can't enable oscillator clock %d\n", ret);
> +		return ret;

One return statement is enough here.

> +	}
> +
>  	return ret;
>  }
>  
>  static int cz_da7219_hw_free(struct snd_pcm_substream *substream)
>  {
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_card *card = rtd->card;
> +	struct clk *acpd7219_clk;
> +
>  	clk_disable_unprepare(da7219_dai_clk);
>  
> +	acpd7219_clk = clk_get(card->dev, "acpd7219-clks");

Error handling here as well.

> +	clk_disable_unprepare(acpd7219_clk);
> +
>  	return 0;
>  }
>  
> @@ -237,9 +270,113 @@ static int cz_fe_startup(struct snd_pcm_substream *substream)
>  	.num_controls = ARRAY_SIZE(cz_mc_controls),
>  };
>

<snip>
  
> +
> +static int register_acpd7219_clocks(struct platform_device *pdev)
> +{
> +	struct clk_init_data init = {};
> +	struct clk *acp_clks;
> +	struct clk_lookup *acp_clks_lookup;
> +	struct cz_clock *cz_clock_obj;
> +	struct resource *res;
> +	struct device dev = pdev->dev;
> +
> +	cz_clock_obj = kzalloc(sizeof(struct cz_clock), GFP_KERNEL);
> +	if (!cz_clock_obj)
> +		return -ENOMEM;
> +
> +	cz_clock_obj->acp_clks_name = "acpd7219-clks";
> +
> +	init.parent_names = NULL;
> +	init.num_parents = 0;
> +	init.name = cz_clock_obj->acp_clks_name;
> +	init.ops = &acpd7219_clks_ops;
> +	cz_clock_obj->acp_clks_hw.init = &init;
> +
> +	acp_clks = devm_clk_register(&dev, &cz_clock_obj->acp_clks_hw);
> +	if (IS_ERR(acp_clks))
> +	{
> +		dev_err(&dev, "Failed to register DAI clocks: %ld\n",
> +			 PTR_ERR(acp_clks));
> +		return -EINVAL;
> +	}
> +	cz_clock_obj->acp_clks = acp_clks;
> +
> +	acp_clks_lookup = clkdev_create(acp_clks, cz_clock_obj->acp_clks_name,
> +					"%s", dev_name(&dev));
> +	if (!acp_clks_lookup)
> +		dev_warn(&dev, "Failed to create DAI clkdev");

Error not handled on purpose?

> +	else
> +		cz_clock_obj->acp_clks_lookup = acp_clks_lookup;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Failed to get misc io resource.\n");

Shouldn't be clkdev_drop used here to free up?

> +		return -EINVAL;
> +	}
> +	cz_clock_obj->res_base = devm_ioremap_nocache(&pdev->dev, res->start,
> +					resource_size(res));
> +	if (!cz_clock_obj->res_base)

Here as well.
clkdev_drop is not called when the driver is removed?

Thanks,
Sriram.
Agrawal, Akshu March 30, 2018, 10:02 a.m. UTC | #3
On 3/27/2018 9:52 AM, Daniel Kurtz wrote:
> Hi Akshu
> 
> On Mon, Mar 26, 2018 at 3:12 AM Akshu Agrawal <akshu.agrawal@amd.com> wrote:
> 
>> This enables/disables and sets auxiliary clock at 25Mhz. It uses
>> common clock framework for proper ref counting. This approach will
>> save power in comparison to keeping it always On in firmware.
> 
>> V2: Correcting the pin to OSCOUT1 from OSCOUT2
>> V3: Fix error/warnings from kbuild test
>> V4: Fix build errors for platform which do not support CONFIG_COMMON_CLK
> 
>> TEST= aplay -vv <file>
>> check register to see clock enabled
>> kill aplay
>> check register to see clock disabled
> 
>> Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com>
>> Acked-by: Alex Deucher <alexander.deucher@amd.com>
>> ---
>>    sound/soc/amd/acp-da7219-max98357a.c | 144
> ++++++++++++++++++++++++++++++++++-
>>    1 file changed, 142 insertions(+), 2 deletions(-)
> 
>> diff --git a/sound/soc/amd/acp-da7219-max98357a.c
> b/sound/soc/amd/acp-da7219-max98357a.c
>> index 99c6b5c..3c77803 100644
>> --- a/sound/soc/amd/acp-da7219-max98357a.c
>> +++ b/sound/soc/amd/acp-da7219-max98357a.c
>> @@ -30,10 +30,13 @@
>>    #include <sound/soc-dapm.h>
>>    #include <sound/jack.h>
>>    #include <linux/clk.h>
>> +#include <linux/clkdev.h>
>> +#include <linux/clk-provider.h>
>>    #include <linux/gpio.h>
>>    #include <linux/module.h>
>>    #include <linux/i2c.h>
>>    #include <linux/acpi.h>
>> +#include <linux/types.h>
> 
>>    #include "../codecs/da7219.h"
>>    #include "../codecs/da7219-aad.h"
>> @@ -41,6 +44,20 @@
>>    #define CZ_PLAT_CLK 24000000
>>    #define MCLK_RATE 24576000
>>    #define DUAL_CHANNEL           2
>> +#define CLKDRVSTR2     0x28
>> +#define MISCCLKCNTL1   0x40
>> +#define OSCCLKENB      2
>> +#define OSCOUT1CLK25MHZ        16
> 
> These defines do not belong with the clocks above them, so please separate
> them.
> It might be helpful to add a comment describing what these addresses are,
> ie that they are registers of the SoC.
> 

Accepted.

>> +
>> +struct cz_clock {
>> +       const char* acp_clks_name;
> 
> Why store this in the struct?
> 

need to get res_base in clk_enable. Hence, better to keep them
in one struct

>> +#ifdef CONFIG_COMMON_CLK
>> +       struct clk_hw acp_clks_hw;
>> +#endif
>> +       struct clk_lookup *acp_clks_lookup;
> 
> Why store this in the struct?
> 
>> +       struct clk *acp_clks;
> 
> Why store this in the struct?
> 
>> +       void __iomem *res_base;
>> +};
> 
>>    static struct snd_soc_jack cz_jack;
>>    struct clk *da7219_dai_clk;
>> @@ -91,6 +108,8 @@ static int cz_da7219_hw_params(struct
> snd_pcm_substream *substream,
>>    {
>>           int ret = 0;
>>           struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> +       struct snd_soc_card *card = rtd->card;
>> +       struct clk *acpd7219_clk;
> 
>>           ret = clk_prepare_enable(da7219_dai_clk);
>>           if (ret < 0) {
>> @@ -98,13 +117,27 @@ static int cz_da7219_hw_params(struct
> snd_pcm_substream *substream,
>>                   return ret;
>>           }
> 
>> +       acpd7219_clk = clk_get(card->dev, "acpd7219-clks");
>> +       ret = clk_prepare_enable(acpd7219_clk);
>> +       if (ret < 0) {
>> +               dev_err(rtd->dev, "can't enable oscillator clock %d\n",
> ret);
>> +               return ret;
>> +       }
>> +
>>           return ret;
>>    }
> 
>>    static int cz_da7219_hw_free(struct snd_pcm_substream *substream)
>>    {
>> +       struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> +       struct snd_soc_card *card = rtd->card;
>> +       struct clk *acpd7219_clk;
>> +
>>           clk_disable_unprepare(da7219_dai_clk);
> 
>> +       acpd7219_clk = clk_get(card->dev, "acpd7219-clks");
>> +       clk_disable_unprepare(acpd7219_clk);
>> +
>>           return 0;
>>    }
> 
>> @@ -237,9 +270,113 @@ static int cz_fe_startup(struct snd_pcm_substream
> *substream)
>>           .num_controls = ARRAY_SIZE(cz_mc_controls),
>>    };
> 
>> +#ifdef CONFIG_COMMON_CLK
>> +static int acpd7219_clks_prepare(struct clk_hw *hw)
> 
> Should setting OSCCLKENB be the clock enable rather than prepare?
> 

Accepted.

>> +{
>> +       u32 reg_val;
>> +       struct cz_clock *cz_clock_obj =
>> +               container_of(hw, struct cz_clock, acp_clks_hw);
> 
> nit: just "cz_clock" should be enough for all of these.
>

build failure with just cz_clock

>> +       void __iomem *base = cz_clock_obj->res_base;
>> +
>> +       reg_val = readl(base + MISCCLKCNTL1);
>> +       reg_val &= ~(0x1 << OSCCLKENB);
>> +       writel(reg_val, base + MISCCLKCNTL1);
>> +       reg_val = readl(base + CLKDRVSTR2);
>> +       reg_val |= (0x1 << OSCOUT1CLK25MHZ);
>> +       writel(reg_val, base + CLKDRVSTR2);
> 
> Writing OSCOUT1CLK25MHZ sets the clock to 25 MHz (ie, instead of 48 MHz).
> So, technically this part should probably be in a set_rate() rather than
> prepare() [and reject other frequencies]?
>

We need to set it always at 25Mhz. We initialize sysclk and set pll on 
that frequency. Though we are setting rate of clock but parameters rate 
and parent_rate will be of no use. Its more like enabling a clock at as 
fixed rate.

>> +
>> +       return 0;
>> +}
>> +
>> +static void acpd7219_clks_unprepare(struct clk_hw *hw)
> 
> Similarly, can this be disable()?
> 

Accepted.

>> +{
>> +       u32 reg_val;
>> +       struct cz_clock *cz_clock_obj =
>> +               container_of(hw, struct cz_clock, acp_clks_hw);
>> +       void __iomem *base = cz_clock_obj->res_base;
>> +
>> +       reg_val = readl(base + MISCCLKCNTL1);
>> +       reg_val |= (0x1 << OSCCLKENB);
>> +       writel(reg_val, base + MISCCLKCNTL1);
>> +}
>> +
>> +static int acpd7219_clks_is_enabled(struct clk_hw *hw)
>> +{
>> +       u32 reg_val;
>> +       struct cz_clock *cz_clock_obj =
>> +               container_of(hw, struct cz_clock, acp_clks_hw);
>> +       void __iomem *base = cz_clock_obj->res_base;
>> +
>> +       reg_val = readl(base + MISCCLKCNTL1);
>> +
>> +       return !(reg_val & OSCCLKENB);
>> +}
>> +
>> +const struct clk_ops acpd7219_clks_ops = {
> 
> static const ...
> 

Accepted.

>> +       .prepare = acpd7219_clks_prepare,
>> +       .unprepare = acpd7219_clks_unprepare,
>> +       .is_enabled = acpd7219_clks_is_enabled,
>> +};
>> +
>> +static int register_acpd7219_clocks(struct platform_device *pdev)
> 
> Can this be:
> __init
> 

No, seems there is code calling code in this function.

>> +{
>> +       struct clk_init_data init = {};
>> +       struct clk *acp_clks;
>> +       struct clk_lookup *acp_clks_lookup;
>> +       struct cz_clock *cz_clock_obj;
>> +       struct resource *res;
>> +       struct device dev = pdev->dev;
>> +
>> +       cz_clock_obj = kzalloc(sizeof(struct cz_clock), GFP_KERNEL);
>> +       if (!cz_clock_obj)
>> +               return -ENOMEM;
>> +
>> +       cz_clock_obj->acp_clks_name = "acpd7219-clks";
>> +
>> +       init.parent_names = NULL;
>> +       init.num_parents = 0;
>> +       init.name = cz_clock_obj->acp_clks_name;
>> +       init.ops = &acpd7219_clks_ops;
> 
> I think you can just do this all during: "struct clk_init_data init = { }".
> In fact, can't this done like this instead:
> 
> static const struct clk_init_data __initconst = {
>     .name = "acpd7219-clks";
>     .ops = &acpd7219_clks_ops;
> };
>

Accepted.

>> +       cz_clock_obj->acp_clks_hw.init = &init;
>> +
>> +       acp_clks = devm_clk_register(&dev, &cz_clock_obj->acp_clks_hw);
>> +       if (IS_ERR(acp_clks))
>> +       {
>> +               dev_err(&dev, "Failed to register DAI clocks: %ld\n",
>> +                        PTR_ERR(acp_clks));
>> +               return -EINVAL;
> 
> propagate PTR_ERR(acp_clks), instead?
> 

Accepted.

>> +       }
>> +       cz_clock_obj->acp_clks = acp_clks;
>> +
>> +       acp_clks_lookup = clkdev_create(acp_clks,
> cz_clock_obj->acp_clks_name,
>> +                                       "%s", dev_name(&dev));
>> +       if (!acp_clks_lookup)
>> +               dev_warn(&dev, "Failed to create DAI clkdev");
>> +       else
>> +               cz_clock_obj->acp_clks_lookup =
> acp_clks_lookup;clkdev_create
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       if (!res) {
>> +               dev_err(&pdev->dev, "Failed to get misc io resource.\n");
>> +               return -EINVAL;
>> +       }
>> +       cz_clock_obj->res_base = devm_ioremap_nocache(&pdev->dev,
> res->start,
>> +                                       resource_size(res));
>> +       if (!cz_clock_obj->res_base)
>> +               return -ENOMEM;
>> +
>> +       return 0;
>> +}
>> +#else
>> +static int register_acpd7219_clocks(struct platform_device *pdev)
>> +{
>> +       return 0;
>> +}
>> +#endif /* CONFIG_COMMON_CLK */
>> +
>>    static int cz_probe(struct platform_device *pdev)
>>    {
>> -       int ret;
>> +       int ret = 0;
> 
> Do not initialize this before it is used.
>

Accepted.

>>           struct snd_soc_card *card;
> 
>>           card = &cz_card;
>> @@ -252,7 +389,10 @@ static int cz_probe(struct platform_device *pdev)
>>                                   cz_card.name, ret);
>>                   return ret;
>>           }
>> -       return 0;
>> +
>> +       ret = register_acpd7219_clocks(pdev);
>> +
>> +       return ret;
> 
> Just:
>     return register_acpd7219_clocks(pdev);
> 

Accepted.

> Thanks,
> -Dan
> 
>>    }
> 
>>    static const struct acpi_device_id cz_audio_acpi_match[] = {
>> --
>> 1.9.1
Agrawal, Akshu March 30, 2018, 10:02 a.m. UTC | #4
On 3/28/2018 12:16 AM, Sriram Periyasamy wrote:
> On Mon, Mar 26, 2018 at 02:26:00PM +0530, Akshu Agrawal wrote:
>> This enables/disables and sets auxiliary clock at 25Mhz. It uses
>> common clock framework for proper ref counting. This approach will
>> save power in comparison to keeping it always On in firmware.
>>
>> V2: Correcting the pin to OSCOUT1 from OSCOUT2
>> V3: Fix error/warnings from kbuild test
>> V4: Fix build errors for platform which do not support CONFIG_COMMON_CLK
>>
> 
> Please consider adding the change history across versions below
> the marker --- which is the standard way of doing.
>

Accepted.

>> TEST= aplay -vv <file>
>> check register to see clock enabled
>> kill aplay
>> check register to see clock disabled
>>
>> Signed-off-by: Akshu Agrawal <akshu.agrawal@amd.com>
>> Acked-by: Alex Deucher <alexander.deucher@amd.com>
>> ---
>>   sound/soc/amd/acp-da7219-max98357a.c | 144 ++++++++++++++++++++++++++++++++++-
>>   1 file changed, 142 insertions(+), 2 deletions(-)
>>
>> diff --git a/sound/soc/amd/acp-da7219-max98357a.c b/sound/soc/amd/acp-da7219-max98357a.c
>> index 99c6b5c..3c77803 100644
>> --- a/sound/soc/amd/acp-da7219-max98357a.c
>> +++ b/sound/soc/amd/acp-da7219-max98357a.c
> 
> <snip>
> 
>> @@ -98,13 +117,27 @@ static int cz_da7219_hw_params(struct snd_pcm_substream *substream,
>>   		return ret;
>>   	}
>>   
>> +	acpd7219_clk = clk_get(card->dev, "acpd7219-clks");
> 
> Consider adding the error handling, for example if CONFIG_COMMON_CLK is not
> defined for this platform, then clk_get may return error.
> 

Accepted.

>> +	ret = clk_prepare_enable(acpd7219_clk);
>> +	if (ret < 0) {
>> +		dev_err(rtd->dev, "can't enable oscillator clock %d\n", ret);
>> +		return ret;
> 
> One return statement is enough here.
> 

Accepted.

>> +	}
>> +
>>   	return ret;
>>   }
>>   
>>   static int cz_da7219_hw_free(struct snd_pcm_substream *substream)
>>   {
>> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> +	struct snd_soc_card *card = rtd->card;
>> +	struct clk *acpd7219_clk;
>> +
>>   	clk_disable_unprepare(da7219_dai_clk);
>>   
>> +	acpd7219_clk = clk_get(card->dev, "acpd7219-clks");
> 
> Error handling here as well.
> 

Accepted.

>> +	clk_disable_unprepare(acpd7219_clk);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -237,9 +270,113 @@ static int cz_fe_startup(struct snd_pcm_substream *substream)
>>   	.num_controls = ARRAY_SIZE(cz_mc_controls),
>>   };
>>
> 
> <snip>
>    
>> +
>> +static int register_acpd7219_clocks(struct platform_device *pdev)
>> +{
>> +	struct clk_init_data init = {};
>> +	struct clk *acp_clks;
>> +	struct clk_lookup *acp_clks_lookup;
>> +	struct cz_clock *cz_clock_obj;
>> +	struct resource *res;
>> +	struct device dev = pdev->dev;
>> +
>> +	cz_clock_obj = kzalloc(sizeof(struct cz_clock), GFP_KERNEL);
>> +	if (!cz_clock_obj)
>> +		return -ENOMEM;
>> +
>> +	cz_clock_obj->acp_clks_name = "acpd7219-clks";
>> +
>> +	init.parent_names = NULL;
>> +	init.num_parents = 0;
>> +	init.name = cz_clock_obj->acp_clks_name;
>> +	init.ops = &acpd7219_clks_ops;
>> +	cz_clock_obj->acp_clks_hw.init = &init;
>> +
>> +	acp_clks = devm_clk_register(&dev, &cz_clock_obj->acp_clks_hw);
>> +	if (IS_ERR(acp_clks))
>> +	{
>> +		dev_err(&dev, "Failed to register DAI clocks: %ld\n",
>> +			 PTR_ERR(acp_clks));
>> +		return -EINVAL;
>> +	}
>> +	cz_clock_obj->acp_clks = acp_clks;
>> +
>> +	acp_clks_lookup = clkdev_create(acp_clks, cz_clock_obj->acp_clks_name,
>> +					"%s", dev_name(&dev));
>> +	if (!acp_clks_lookup)
>> +		dev_warn(&dev, "Failed to create DAI clkdev");
> 
> Error not handled on purpose?
>

Yes, just warning.

>> +	else
>> +		cz_clock_obj->acp_clks_lookup = acp_clks_lookup;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res) {
>> +		dev_err(&pdev->dev, "Failed to get misc io resource.\n");
> 
> Shouldn't be clkdev_drop used here to free up?
> 

Accepted.

>> +		return -EINVAL;
>> +	}
>> +	cz_clock_obj->res_base = devm_ioremap_nocache(&pdev->dev, res->start,
>> +					resource_size(res));
>> +	if (!cz_clock_obj->res_base)
> 
> Here as well.
> clkdev_drop is not called when the driver is removed?
>

Accepted.

> Thanks,
> Sriram.
>

Patch
diff mbox

diff --git a/sound/soc/amd/acp-da7219-max98357a.c b/sound/soc/amd/acp-da7219-max98357a.c
index 99c6b5c..3c77803 100644
--- a/sound/soc/amd/acp-da7219-max98357a.c
+++ b/sound/soc/amd/acp-da7219-max98357a.c
@@ -30,10 +30,13 @@ 
 #include <sound/soc-dapm.h>
 #include <sound/jack.h>
 #include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
 #include <linux/gpio.h>
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/acpi.h>
+#include <linux/types.h>
 
 #include "../codecs/da7219.h"
 #include "../codecs/da7219-aad.h"
@@ -41,6 +44,20 @@ 
 #define CZ_PLAT_CLK 24000000
 #define MCLK_RATE 24576000
 #define DUAL_CHANNEL		2
+#define CLKDRVSTR2	0x28
+#define MISCCLKCNTL1	0x40
+#define OSCCLKENB	2
+#define OSCOUT1CLK25MHZ	16
+
+struct cz_clock {
+	const char* acp_clks_name;
+#ifdef CONFIG_COMMON_CLK
+	struct clk_hw acp_clks_hw;
+#endif
+	struct clk_lookup *acp_clks_lookup;
+	struct clk *acp_clks;
+	void __iomem *res_base;
+};
 
 static struct snd_soc_jack cz_jack;
 struct clk *da7219_dai_clk;
@@ -91,6 +108,8 @@  static int cz_da7219_hw_params(struct snd_pcm_substream *substream,
 {
 	int ret = 0;
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_card *card = rtd->card;
+	struct clk *acpd7219_clk;
 
 	ret = clk_prepare_enable(da7219_dai_clk);
 	if (ret < 0) {
@@ -98,13 +117,27 @@  static int cz_da7219_hw_params(struct snd_pcm_substream *substream,
 		return ret;
 	}
 
+	acpd7219_clk = clk_get(card->dev, "acpd7219-clks");
+	ret = clk_prepare_enable(acpd7219_clk);
+	if (ret < 0) {
+		dev_err(rtd->dev, "can't enable oscillator clock %d\n", ret);
+		return ret;
+	}
+
 	return ret;
 }
 
 static int cz_da7219_hw_free(struct snd_pcm_substream *substream)
 {
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_card *card = rtd->card;
+	struct clk *acpd7219_clk;
+
 	clk_disable_unprepare(da7219_dai_clk);
 
+	acpd7219_clk = clk_get(card->dev, "acpd7219-clks");
+	clk_disable_unprepare(acpd7219_clk);
+
 	return 0;
 }
 
@@ -237,9 +270,113 @@  static int cz_fe_startup(struct snd_pcm_substream *substream)
 	.num_controls = ARRAY_SIZE(cz_mc_controls),
 };
 
+#ifdef CONFIG_COMMON_CLK
+static int acpd7219_clks_prepare(struct clk_hw *hw)
+{
+	u32 reg_val;
+	struct cz_clock *cz_clock_obj =
+		container_of(hw, struct cz_clock, acp_clks_hw);
+	void __iomem *base = cz_clock_obj->res_base;
+
+	reg_val = readl(base + MISCCLKCNTL1);
+	reg_val &= ~(0x1 << OSCCLKENB);
+	writel(reg_val, base + MISCCLKCNTL1);
+	reg_val = readl(base + CLKDRVSTR2);
+	reg_val |= (0x1 << OSCOUT1CLK25MHZ);
+	writel(reg_val, base + CLKDRVSTR2);
+
+	return 0;
+}
+
+static void acpd7219_clks_unprepare(struct clk_hw *hw)
+{
+	u32 reg_val;
+	struct cz_clock *cz_clock_obj =
+		container_of(hw, struct cz_clock, acp_clks_hw);
+	void __iomem *base = cz_clock_obj->res_base;
+
+	reg_val = readl(base + MISCCLKCNTL1);
+	reg_val |= (0x1 << OSCCLKENB);
+	writel(reg_val, base + MISCCLKCNTL1);
+}
+
+static int acpd7219_clks_is_enabled(struct clk_hw *hw)
+{
+	u32 reg_val;
+	struct cz_clock *cz_clock_obj =
+		container_of(hw, struct cz_clock, acp_clks_hw);
+	void __iomem *base = cz_clock_obj->res_base;
+
+	reg_val = readl(base + MISCCLKCNTL1);
+
+	return !(reg_val & OSCCLKENB);
+}
+
+const struct clk_ops acpd7219_clks_ops = {
+	.prepare = acpd7219_clks_prepare,
+	.unprepare = acpd7219_clks_unprepare,
+	.is_enabled = acpd7219_clks_is_enabled,
+};
+
+static int register_acpd7219_clocks(struct platform_device *pdev)
+{
+	struct clk_init_data init = {};
+	struct clk *acp_clks;
+	struct clk_lookup *acp_clks_lookup;
+	struct cz_clock *cz_clock_obj;
+	struct resource *res;
+	struct device dev = pdev->dev;
+
+	cz_clock_obj = kzalloc(sizeof(struct cz_clock), GFP_KERNEL);
+	if (!cz_clock_obj)
+		return -ENOMEM;
+
+	cz_clock_obj->acp_clks_name = "acpd7219-clks";
+
+	init.parent_names = NULL;
+	init.num_parents = 0;
+	init.name = cz_clock_obj->acp_clks_name;
+	init.ops = &acpd7219_clks_ops;
+	cz_clock_obj->acp_clks_hw.init = &init;
+
+	acp_clks = devm_clk_register(&dev, &cz_clock_obj->acp_clks_hw);
+	if (IS_ERR(acp_clks))
+	{
+		dev_err(&dev, "Failed to register DAI clocks: %ld\n",
+			 PTR_ERR(acp_clks));
+		return -EINVAL;
+	}
+	cz_clock_obj->acp_clks = acp_clks;
+
+	acp_clks_lookup = clkdev_create(acp_clks, cz_clock_obj->acp_clks_name,
+					"%s", dev_name(&dev));
+	if (!acp_clks_lookup)
+		dev_warn(&dev, "Failed to create DAI clkdev");
+	else
+		cz_clock_obj->acp_clks_lookup = acp_clks_lookup;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "Failed to get misc io resource.\n");
+		return -EINVAL;
+	}
+	cz_clock_obj->res_base = devm_ioremap_nocache(&pdev->dev, res->start,
+					resource_size(res));
+	if (!cz_clock_obj->res_base)
+		return -ENOMEM;
+
+	return 0;
+}
+#else
+static int register_acpd7219_clocks(struct platform_device *pdev)
+{
+	return 0;
+}
+#endif /* CONFIG_COMMON_CLK */
+
 static int cz_probe(struct platform_device *pdev)
 {
-	int ret;
+	int ret = 0;
 	struct snd_soc_card *card;
 
 	card = &cz_card;
@@ -252,7 +389,10 @@  static int cz_probe(struct platform_device *pdev)
 				cz_card.name, ret);
 		return ret;
 	}
-	return 0;
+
+	ret = register_acpd7219_clocks(pdev);
+
+	return ret;
 }
 
 static const struct acpi_device_id cz_audio_acpi_match[] = {