diff mbox series

[3/3] ASoC: maxim,max9867: add "mclk" support

Message ID 20230302-max9867-v1-3-aa9f7f25db5e@skidata.com
State New
Headers
Series Add "mclk" support for maxim,max9867 |

Commit Message

Richard Leitner March 2, 2023, 11:55 a.m. UTC 
  From: Benjamin Bara <benjamin.bara@skidata.com>

Add basic support for the codecs mclk by enabling it during probing.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 sound/soc/codecs/max9867.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Comments

Mark Brown March 2, 2023, 12:20 p.m. UTC | #1 
On Thu, Mar 02, 2023 at 12:55:03PM +0100, richard.leitner@linux.dev wrote:

> +	max9867->mclk = devm_clk_get(&i2c->dev, "mclk");
> +	if (IS_ERR(max9867->mclk))
> +		return PTR_ERR(max9867->mclk);
> +	ret = clk_prepare_enable(max9867->mclk);
> +	if (ret < 0)
> +		dev_err(&i2c->dev, "Failed to enable MCLK: %d\n", ret);
> +

Nothing ever disables the clock - we need a disable in the remove path
at least.
Claudiu Beznea March 2, 2023, 12:45 p.m. UTC | #2 
On 02.03.2023 14:20, Mark Brown wrote:
>> +	max9867->mclk = devm_clk_get(&i2c->dev, "mclk");
>> +	if (IS_ERR(max9867->mclk))
>> +		return PTR_ERR(max9867->mclk);
>> +	ret = clk_prepare_enable(max9867->mclk);
>> +	if (ret < 0)
>> +		dev_err(&i2c->dev, "Failed to enable MCLK: %d\n", ret);
>> +
> Nothing ever disables the clock - we need a disable in the remove path
> at least.

I don't have the full context of this patch but this diff seems a good
candidate for devm_clk_get_enabled().
Richard Leitner March 2, 2023, 12:45 p.m. UTC | #3 
On Thu, Mar 02, 2023 at 12:20:18PM +0000, Mark Brown wrote:
> On Thu, Mar 02, 2023 at 12:55:03PM +0100, richard.leitner@linux.dev wrote:
> 
> > +	max9867->mclk = devm_clk_get(&i2c->dev, "mclk");
> > +	if (IS_ERR(max9867->mclk))
> > +		return PTR_ERR(max9867->mclk);
> > +	ret = clk_prepare_enable(max9867->mclk);
> > +	if (ret < 0)
> > +		dev_err(&i2c->dev, "Failed to enable MCLK: %d\n", ret);
> > +
> 
> Nothing ever disables the clock - we need a disable in the remove path
> at least.

Sure. Sorry for missing that. I will send a v2 later today.

regards;rl
Richard Leitner March 2, 2023, 2:46 p.m. UTC | #4 
Hi Claudiu,

On Thu, Mar 02, 2023 at 12:45:50PM +0000, Claudiu.Beznea@microchip.com wrote:
> On 02.03.2023 14:20, Mark Brown wrote:
> >> +	max9867->mclk = devm_clk_get(&i2c->dev, "mclk");
> >> +	if (IS_ERR(max9867->mclk))
> >> +		return PTR_ERR(max9867->mclk);
> >> +	ret = clk_prepare_enable(max9867->mclk);
> >> +	if (ret < 0)
> >> +		dev_err(&i2c->dev, "Failed to enable MCLK: %d\n", ret);
> >> +
> > Nothing ever disables the clock - we need a disable in the remove path
> > at least.
> 
> I don't have the full context of this patch but this diff seems a good
> candidate for devm_clk_get_enabled().

Thanks for that pointer, but currently we are thinking of prepare_enable
the clock in SND_SOC_BIAS_ON and disable_unprepare it in SND_SOC_BIAS_OFF
(similar to wm8731.c).
Therefore probe() will only do a devm_clk_get().

Claudiu, Rob: Will this be an acceptable solution?

regards;rl
Claudiu Beznea March 3, 2023, 10 a.m. UTC | #5 
On 02.03.2023 16:46, Richard Leitner wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Claudiu,
> 
> On Thu, Mar 02, 2023 at 12:45:50PM +0000, Claudiu.Beznea@microchip.com wrote:
>> On 02.03.2023 14:20, Mark Brown wrote:
>>>> +  max9867->mclk = devm_clk_get(&i2c->dev, "mclk");
>>>> +  if (IS_ERR(max9867->mclk))
>>>> +          return PTR_ERR(max9867->mclk);
>>>> +  ret = clk_prepare_enable(max9867->mclk);
>>>> +  if (ret < 0)
>>>> +          dev_err(&i2c->dev, "Failed to enable MCLK: %d\n", ret);
>>>> +
>>> Nothing ever disables the clock - we need a disable in the remove path
>>> at least.
>>
>> I don't have the full context of this patch but this diff seems a good
>> candidate for devm_clk_get_enabled().
> 
> Thanks for that pointer, but currently we are thinking of prepare_enable
> the clock in SND_SOC_BIAS_ON and disable_unprepare it in SND_SOC_BIAS_OFF
> (similar to wm8731.c).
> Therefore probe() will only do a devm_clk_get().

Sounds good for me.

> 
> Claudiu, Rob: Will this be an acceptable solution?
> 
> regards;rl
diff mbox series

Patch

diff --git a/sound/soc/codecs/max9867.c b/sound/soc/codecs/max9867.c
index e161ab037bf7..b92dd61bb2b2 100644
--- a/sound/soc/codecs/max9867.c
+++ b/sound/soc/codecs/max9867.c
@@ -6,6 +6,7 @@ 
 // Copyright 2018 Ladislav Michl <ladis@linux-mips.org>
 //
 
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
@@ -16,6 +17,7 @@ 
 #include "max9867.h"
 
 struct max9867_priv {
+	struct clk *mclk;
 	struct regmap *regmap;
 	const struct snd_pcm_hw_constraint_list *constraints;
 	unsigned int sysclk, pclk;
@@ -663,8 +665,18 @@  static int max9867_i2c_probe(struct i2c_client *i2c)
 	dev_info(&i2c->dev, "device revision: %x\n", reg);
 	ret = devm_snd_soc_register_component(&i2c->dev, &max9867_component,
 			max9867_dai, ARRAY_SIZE(max9867_dai));
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(&i2c->dev, "Failed to register component: %d\n", ret);
+		return ret;
+	}
+
+	max9867->mclk = devm_clk_get(&i2c->dev, "mclk");
+	if (IS_ERR(max9867->mclk))
+		return PTR_ERR(max9867->mclk);
+	ret = clk_prepare_enable(max9867->mclk);
+	if (ret < 0)
+		dev_err(&i2c->dev, "Failed to enable MCLK: %d\n", ret);
+
 	return ret;
 }