[RFC PATCH 00/16] ASoC/SOF/clk/gpio/dt: add Hifiberry DAC+ PRO support

[RFC PATCH 00/16] ASoC/SOF/clk/gpio/dt: add Hifiberry DAC+ PRO support

[Pierre-Louis Bossart]

The Hifiberry DAC+ / DAC+ PRO is supported in the Raspberry PI tree
but until now not in the mainline and not for ACPI platforms.

This patchset implements the recommendations suggested by Mark Brown
back in 2018: first add a gpiochip in the PCM512x codec driver, then
use these gpios from a clock driver and the machine driver.

Since this patchset relies on different subsystems, sending as RFC for
now. I chose to import the original code from the Raspberry PI tree as
is, and add my changes on top. If there is a preference to squash the
changes that'd be fine. I also don't know if I should split this
series in two, one for ASoC and one for clk changes?

This patchset does not add changes to the sound/soc/bcm machine
drivers, but that should be trivial once all the gpio/clock is
available.

Thanks to Andy Shevchenko for his help navigating the gpio subsystem
and flagging mistakes in the use of lookup tables, and to Rob Herring
for pointers on the DT bindings verification tools.

Daniel Matuschek (1):
  clk: hifiberry-dacpro: initial import

Pierre-Louis Bossart (15):
  ASoC: pcm512x: expose 6 GPIOs
  ASoC: pcm512x: use "sclk" string to retrieve clock
  ASoC: Intel: sof-pcm512x: use gpiod for LED
  ASoC: Intel: sof-pcm512x: detect Hifiberry DAC+ PRO
  ASoC: Intel: sof-pcm512x: reconfigure sclk in hw_params if needed
  ASoC: Intel: sof-pcm512x: select HIFIBERRY_DACPRO clk
  clk: hifiberry-dacpro: update SDPX/copyright
  clk: hifiberry-dacpro: style cleanups, use devm_
  clk: hifiberry-dacpro: add OF dependency
  clk: hifiberry-dacpro: transition to _hw functions
  clk: hifiberry-dacpro: add ACPI support
  clk: hifiberry-dacpro: add "sclk" lookup
  clk: hifiberry-dacpro: toggle GPIOs on prepare/unprepare
  clk: hifiberry-dacpro: add delay on clock prepare/deprepare
  ASoC: dt-bindings: add document for Hifiberry DAC+ PRO clock

 .../bindings/sound/hifiberry-dacpro.yaml      |  38 +++
 drivers/clk/Kconfig                           |   4 +
 drivers/clk/Makefile                          |   1 +
 drivers/clk/clk-hifiberry-dacpro.c            | 284 ++++++++++++++++++
 sound/soc/codecs/pcm512x.c                    | 134 ++++++++-
 sound/soc/intel/boards/Kconfig                |   2 +
 sound/soc/intel/boards/sof_pcm512x.c          | 190 +++++++++++-
 7 files changed, 635 insertions(+), 18 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/hifiberry-dacpro.yaml
 create mode 100644 drivers/clk/clk-hifiberry-dacpro.c


base-commit: dd8e871d4e560eeb8d22af82dde91457ad835a63
-- 
2.20.1

[RFC PATCH 01/16] ASoC: pcm512x: expose 6 GPIOs

[Pierre-Louis Bossart]

The GPIOs are used e.g. on HifiBerry DAC+ HATs to control the LED
(GPIO3) and the choice of the 44.1 (GPIO6) or 48 (GPIO3) kHz
oscillator (when present).

Enable basic gpio_chip to get/set values and get/set
directions. Tested with GPIO_LIB from sys/class/gpio, the LED turns
on/off as desired.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/codecs/pcm512x.c | 108 +++++++++++++++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c
index 4cbef9affffd..4f895a588c31 100644
--- a/sound/soc/codecs/pcm512x.c
+++ b/sound/soc/codecs/pcm512x.c
@@ -10,6 +10,7 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/clk.h>
+#include <linux/gpio/driver.h>
 #include <linux/kernel.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
@@ -32,6 +33,7 @@ static const char * const pcm512x_supply_names[PCM512x_NUM_SUPPLIES] = {
 struct pcm512x_priv {
 	struct regmap *regmap;
 	struct clk *sclk;
+	struct gpio_chip chip;
 	struct regulator_bulk_data supplies[PCM512x_NUM_SUPPLIES];
 	struct notifier_block supply_nb[PCM512x_NUM_SUPPLIES];
 	int fmt;
@@ -1503,6 +1505,102 @@ const struct regmap_config pcm512x_regmap = {
 };
 EXPORT_SYMBOL_GPL(pcm512x_regmap);
 
+static int pcm512x_gpio_get_direction(struct gpio_chip *chip,
+				      unsigned int offset)
+{
+	struct pcm512x_priv *pcm512x = gpiochip_get_data(chip);
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(pcm512x->regmap, PCM512x_GPIO_EN, &val);
+	if (ret < 0)
+		return ret;
+
+	val = (val >> offset) & 1;
+
+	/* val is 0 for input, 1 for output, return inverted */
+	return val ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
+}
+
+static int pcm512x_gpio_direction_input(struct gpio_chip *chip,
+					unsigned int offset)
+{
+	struct pcm512x_priv *pcm512x = gpiochip_get_data(chip);
+
+	return regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_EN,
+				  BIT(offset), 0);
+}
+
+static int pcm512x_gpio_direction_output(struct gpio_chip *chip,
+					 unsigned int offset,
+					 int value)
+{
+	struct pcm512x_priv *pcm512x = gpiochip_get_data(chip);
+	unsigned int reg;
+	int ret;
+
+	/* select Register GPIOx output for OUTPUT_x (1..6) */
+	reg = PCM512x_GPIO_OUTPUT_1 + offset;
+	ret = regmap_update_bits(pcm512x->regmap, reg, 0x0f, 0x02);
+	if (ret < 0)
+		return ret;
+
+	/* enable output x */
+	ret = regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_EN,
+				 BIT(offset), BIT(offset));
+	if (ret < 0)
+		return ret;
+
+	/* set value */
+	return regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_CONTROL_1,
+				  BIT(offset), value << offset);
+}
+
+static int pcm512x_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct pcm512x_priv *pcm512x = gpiochip_get_data(chip);
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(pcm512x->regmap, PCM512x_GPIO_CONTROL_1, &val);
+	if (ret < 0)
+		return ret;
+
+	return (val >> offset) & 1;
+}
+
+static void pcm512x_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			     int value)
+{
+	struct pcm512x_priv *pcm512x = gpiochip_get_data(chip);
+	int ret;
+
+	ret = regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_CONTROL_1,
+				 BIT(offset), value << offset);
+
+	if (ret < 0)
+		pr_debug("%s: regmap_update_bits failed: %d\n", __func__, ret);
+}
+
+/* list human-readable names, makes GPIOLIB usage straightforward */
+static const char * const pcm512x_gpio_names[] = {
+	"PCM512x-GPIO1", "PCM512x-GPIO2", "PCM512x-GPIO3",
+	"PCM512x-GPIO4", "PCM512x-GPIO5", "PCM512x-GPIO6"
+};
+
+static const struct gpio_chip template_chip = {
+	.label			= "pcm512x-gpio",
+	.names			= pcm512x_gpio_names,
+	.owner			= THIS_MODULE,
+	.get_direction		= pcm512x_gpio_get_direction,
+	.direction_input	= pcm512x_gpio_direction_input,
+	.direction_output	= pcm512x_gpio_direction_output,
+	.get			= pcm512x_gpio_get,
+	.set			= pcm512x_gpio_set,
+	.base			= -1, /* let gpiolib select the base */
+	.ngpio			= ARRAY_SIZE(pcm512x_gpio_names),
+};
+
 int pcm512x_probe(struct device *dev, struct regmap *regmap)
 {
 	struct pcm512x_priv *pcm512x;
@@ -1563,6 +1661,16 @@ int pcm512x_probe(struct device *dev, struct regmap *regmap)
 		goto err;
 	}
 
+	/* expose 6 GPIO pins, numbered from 1 to 6 */
+	pcm512x->chip = template_chip;
+	pcm512x->chip.parent = dev;
+
+	ret = devm_gpiochip_add_data(dev, &pcm512x->chip, pcm512x);
+	if (ret != 0) {
+		dev_err(dev, "Failed to register gpio chip: %d\n", ret);
+		goto err;
+	}
+
 	pcm512x->sclk = devm_clk_get(dev, NULL);
 	if (PTR_ERR(pcm512x->sclk) == -EPROBE_DEFER) {
 		ret = -EPROBE_DEFER;
-- 
2.20.1

Re: [RFC PATCH 01/16] ASoC: pcm512x: expose 6 GPIOs

[Andy Shevchenko]

On Thu, Apr 09, 2020 at 02:58:26PM -0500, Pierre-Louis Bossart wrote:
> The GPIOs are used e.g. on HifiBerry DAC+ HATs to control the LED
> (GPIO3) and the choice of the 44.1 (GPIO6) or 48 (GPIO3) kHz
> oscillator (when present).
> 
> Enable basic gpio_chip to get/set values and get/set
> directions. Tested with GPIO_LIB from sys/class/gpio, the LED turns
> on/off as desired.


One question, can this use existing GPIO infrastructure, like bgpio_init()?
Ah, I see, that one operates over MMIO, while we would need something based on
regmap API.

Bartosz, do we have plans to have bgpio_regmap_init() or alike?

...

> +static int pcm512x_gpio_get_direction(struct gpio_chip *chip,
> +				      unsigned int offset)
> +{
> +	struct pcm512x_priv *pcm512x = gpiochip_get_data(chip);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(pcm512x->regmap, PCM512x_GPIO_EN, &val);
> +	if (ret < 0)
> +		return ret;

> +	val = (val >> offset) & 1;
> +
> +	/* val is 0 for input, 1 for output, return inverted */
> +	return val ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;

This better to read as simple conditional, like

	if (val & BIT(offset))
		return ..._OUT;
	return ..._IN;

> +}

...

> +static int pcm512x_gpio_direction_output(struct gpio_chip *chip,
> +					 unsigned int offset,
> +					 int value)
> +{
> +	struct pcm512x_priv *pcm512x = gpiochip_get_data(chip);
> +	unsigned int reg;
> +	int ret;
> +
> +	/* select Register GPIOx output for OUTPUT_x (1..6) */
> +	reg = PCM512x_GPIO_OUTPUT_1 + offset;

> +	ret = regmap_update_bits(pcm512x->regmap, reg, 0x0f, 0x02);

Magic numbers detected.

> +	if (ret < 0)

Drop unnecessary ' < 0' parts where it makes sense, like here.

> +		return ret;
> +

> +	/* enable output x */

(1)

> +	ret = regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_EN,
> +				 BIT(offset), BIT(offset));
> +	if (ret < 0)
> +		return ret;
> +
> +	/* set value */

(2)

With this (1)->(2) ordering it may be a glitch. So, first set value (if
hardware allows you, otherwise it seems like a broken one), and then switch
it to output.

> +	return regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_CONTROL_1,
> +				  BIT(offset), value << offset);

You are using many times BIT(offset) mask above, perhaps
	int mask = BIT(offset);

Also, more robust is to use ternary here: 'value ? BIT(offset) : 0'.
Rationale: think what happen with value != 1 (theoretical possibility in the
future).

> +}

...

> +static int pcm512x_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{

> +	return (val >> offset) & 1;

Don't forget to use BIT() macro.

	return !!(val & BIT(offset));

> +}

...

> +static void pcm512x_gpio_set(struct gpio_chip *chip, unsigned int offset,
> +			     int value)
> +{
> +	struct pcm512x_priv *pcm512x = gpiochip_get_data(chip);
> +	int ret;
> +
> +	ret = regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_CONTROL_1,
> +				 BIT(offset), value << offset);

value ? BIT(offset) : 0

> +	if (ret < 0)

> +		pr_debug("%s: regmap_update_bits failed: %d\n", __func__, ret);

No __func__ in debug messages.
Use dev_dbg() when we have struct device available.

> +}

...

> +static const struct gpio_chip template_chip = {

Give better name, please. E.g. pcm512x_gpio_chip.

> +	.label			= "pcm512x-gpio",
> +	.names			= pcm512x_gpio_names,
> +	.owner			= THIS_MODULE,
> +	.get_direction		= pcm512x_gpio_get_direction,
> +	.direction_input	= pcm512x_gpio_direction_input,
> +	.direction_output	= pcm512x_gpio_direction_output,
> +	.get			= pcm512x_gpio_get,
> +	.set			= pcm512x_gpio_set,
> +	.base			= -1, /* let gpiolib select the base */
> +	.ngpio			= ARRAY_SIZE(pcm512x_gpio_names),
> +};

...

> +	/* expose 6 GPIO pins, numbered from 1 to 6 */
> +	pcm512x->chip = template_chip;
> +	pcm512x->chip.parent = dev;
> +
> +	ret = devm_gpiochip_add_data(dev, &pcm512x->chip, pcm512x);

> +	if (ret != 0) {

if (ret)

> +		dev_err(dev, "Failed to register gpio chip: %d\n", ret);
> +		goto err;
> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 01/16] ASoC: pcm512x: expose 6 GPIOs

[Pierre-Louis Bossart]

>> +static int pcm512x_gpio_get_direction(struct gpio_chip *chip,
>> +				      unsigned int offset)
>> +{
>> +	struct pcm512x_priv *pcm512x = gpiochip_get_data(chip);
>> +	unsigned int val;
>> +	int ret;
>> +
>> +	ret = regmap_read(pcm512x->regmap, PCM512x_GPIO_EN, &val);
>> +	if (ret < 0)
>> +		return ret;
> 
>> +	val = (val >> offset) & 1;
>> +
>> +	/* val is 0 for input, 1 for output, return inverted */
>> +	return val ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
> 
> This better to read as simple conditional, like
> 
> 	if (val & BIT(offset))
> 		return ..._OUT;
> 	return ..._IN;
> 
>> +}

ok

> 
> ...
> 
>> +static int pcm512x_gpio_direction_output(struct gpio_chip *chip,
>> +					 unsigned int offset,
>> +					 int value)
>> +{
>> +	struct pcm512x_priv *pcm512x = gpiochip_get_data(chip);
>> +	unsigned int reg;
>> +	int ret;
>> +
>> +	/* select Register GPIOx output for OUTPUT_x (1..6) */
>> +	reg = PCM512x_GPIO_OUTPUT_1 + offset;
> 
>> +	ret = regmap_update_bits(pcm512x->regmap, reg, 0x0f, 0x02);
> 
> Magic numbers detected.
> 
>> +	if (ret < 0)
> 
> Drop unnecessary ' < 0' parts where it makes sense, like here.

did you mean use  if (ret) or drop the test altogether?

There's no standard style for regmap functions so I used what was used 
in the rest of this driver.

Mark?

> 
>> +		return ret;
>> +
> 
>> +	/* enable output x */
> 
> (1)
> 
>> +	ret = regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_EN,
>> +				 BIT(offset), BIT(offset));
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* set value */
> 
> (2)
> 
> With this (1)->(2) ordering it may be a glitch. So, first set value (if
> hardware allows you, otherwise it seems like a broken one), and then switch
> it to output.

good suggestion, thanks.

> 
>> +	return regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_CONTROL_1,
>> +				  BIT(offset), value << offset);
> 
> You are using many times BIT(offset) mask above, perhaps
> 	int mask = BIT(offset);
> 
> Also, more robust is to use ternary here: 'value ? BIT(offset) : 0'.
> Rationale: think what happen with value != 1 (theoretical possibility in the
> future).

ok

> 
>> +}
> 
> ...
> 
>> +static int pcm512x_gpio_get(struct gpio_chip *chip, unsigned int offset)
>> +{
> 
>> +	return (val >> offset) & 1;
> 
> Don't forget to use BIT() macro.
> 
> 	return !!(val & BIT(offset));

There's a point where this becomes less readable IMHO, but fine.
The !! gives me a headache...

>> +static void pcm512x_gpio_set(struct gpio_chip *chip, unsigned int offset,
>> +			     int value)
>> +{
>> +	struct pcm512x_priv *pcm512x = gpiochip_get_data(chip);
>> +	int ret;
>> +
>> +	ret = regmap_update_bits(pcm512x->regmap, PCM512x_GPIO_CONTROL_1,
>> +				 BIT(offset), value << offset);
> 
> value ? BIT(offset) : 0

ok

> 
>> +	if (ret < 0)
> 
>> +		pr_debug("%s: regmap_update_bits failed: %d\n", __func__, ret);
> 
> No __func__ in debug messages.
> Use dev_dbg() when we have struct device available.

Not sure we do, will look into this.

>> +static const struct gpio_chip template_chip = {
> 
> Give better name, please. E.g. pcm512x_gpio_chip.

ok

>> +	/* expose 6 GPIO pins, numbered from 1 to 6 */
>> +	pcm512x->chip = template_chip;
>> +	pcm512x->chip.parent = dev;
>> +
>> +	ret = devm_gpiochip_add_data(dev, &pcm512x->chip, pcm512x);
> 
>> +	if (ret != 0) {
> 
> if (ret)

ok

Re: [RFC PATCH 01/16] ASoC: pcm512x: expose 6 GPIOs

[Andy Shevchenko]

On Tue, Apr 14, 2020 at 12:52:07PM -0500, Pierre-Louis Bossart wrote:

...

> > > +static int pcm512x_gpio_direction_output(struct gpio_chip *chip,
> > > +					 unsigned int offset,
> > > +					 int value)
> > > +{
> > > +	struct pcm512x_priv *pcm512x = gpiochip_get_data(chip);
> > > +	unsigned int reg;
> > > +	int ret;
> > > +
> > > +	/* select Register GPIOx output for OUTPUT_x (1..6) */
> > > +	reg = PCM512x_GPIO_OUTPUT_1 + offset;
> > 
> > > +	ret = regmap_update_bits(pcm512x->regmap, reg, 0x0f, 0x02);
> > 
> > Magic numbers detected.
> > 
> > > +	if (ret < 0)
> > 
> > Drop unnecessary ' < 0' parts where it makes sense, like here.
> 
> did you mean use  if (ret) or drop the test altogether?

Do you see 'ret' part in my phrase above?

> There's no standard style for regmap functions so I used what was used in
> the rest of this driver.

I see. May be than to drop it from the rest and do not add more?

> > > +		return ret;

...

> > > +	return (val >> offset) & 1;
> > 
> > Don't forget to use BIT() macro.
> > 
> > 	return !!(val & BIT(offset));
> 
> There's a point where this becomes less readable IMHO, but fine.
> The !! gives me a headache...

You can check assembly if it gives better result in code generation.
But at least new drivers in GPIO are using it.

...

> > > +		pr_debug("%s: regmap_update_bits failed: %d\n", __func__, ret);
> > 
> > No __func__ in debug messages.
> > Use dev_dbg() when we have struct device available.
> 
> Not sure we do, will look into this.

I didn't get you in the first part. Are you talking about struct device?
It's there, just needs to be de-referenced from GPIO chip.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 01/16] ASoC: pcm512x: expose 6 GPIOs

[Linus Walleij]

On Tue, Apr 14, 2020 at 7:09 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Thu, Apr 09, 2020 at 02:58:26PM -0500, Pierre-Louis Bossart wrote:
> > The GPIOs are used e.g. on HifiBerry DAC+ HATs to control the LED
> > (GPIO3) and the choice of the 44.1 (GPIO6) or 48 (GPIO3) kHz
> > oscillator (when present).
> >
> > Enable basic gpio_chip to get/set values and get/set
> > directions. Tested with GPIO_LIB from sys/class/gpio, the LED turns
> > on/off as desired.
>
>
> One question, can this use existing GPIO infrastructure, like bgpio_init()?
> Ah, I see, that one operates over MMIO, while we would need something based on
> regmap API.
>
> Bartosz, do we have plans to have bgpio_regmap_init() or alike?

Michael Walle is working on that:
https://lore.kernel.org/linux-gpio/20200402203656.27047-11-michael@walle.cc/

I think we should try to merge it sooner rather than later.
I can provide an ib-* branch for ASoC whenever we agreed
on a basic generic driver.

Yours,
Linus Walleij

Re: [RFC PATCH 01/16] ASoC: pcm512x: expose 6 GPIOs

[Pierre-Louis Bossart]

On 4/16/20 6:42 AM, Linus Walleij wrote:
> On Tue, Apr 14, 2020 at 7:09 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> On Thu, Apr 09, 2020 at 02:58:26PM -0500, Pierre-Louis Bossart wrote:
>>> The GPIOs are used e.g. on HifiBerry DAC+ HATs to control the LED
>>> (GPIO3) and the choice of the 44.1 (GPIO6) or 48 (GPIO3) kHz
>>> oscillator (when present).
>>>
>>> Enable basic gpio_chip to get/set values and get/set
>>> directions. Tested with GPIO_LIB from sys/class/gpio, the LED turns
>>> on/off as desired.
>>
>>
>> One question, can this use existing GPIO infrastructure, like bgpio_init()?
>> Ah, I see, that one operates over MMIO, while we would need something based on
>> regmap API.
>>
>> Bartosz, do we have plans to have bgpio_regmap_init() or alike?
> 
> Michael Walle is working on that:
> https://lore.kernel.org/linux-gpio/20200402203656.27047-11-michael@walle.cc/
> 
> I think we should try to merge it sooner rather than later.
> I can provide an ib-* branch for ASoC whenever we agreed
> on a basic generic driver.

Thanks for the pointer, I will give it a try.

[RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Pierre-Louis Bossart]

Using devm_clk_get() with a NULL string fails on ACPI platforms, use
the "sclk" string as a fallback.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/codecs/pcm512x.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c
index 4f895a588c31..1df291b84925 100644
--- a/sound/soc/codecs/pcm512x.c
+++ b/sound/soc/codecs/pcm512x.c
@@ -1603,6 +1603,7 @@ static const struct gpio_chip template_chip = {
 
 int pcm512x_probe(struct device *dev, struct regmap *regmap)
 {
+	const char * const clk_name[] = {NULL, "sclk"};
 	struct pcm512x_priv *pcm512x;
 	int i, ret;
 
@@ -1671,17 +1672,28 @@ int pcm512x_probe(struct device *dev, struct regmap *regmap)
 		goto err;
 	}
 
-	pcm512x->sclk = devm_clk_get(dev, NULL);
-	if (PTR_ERR(pcm512x->sclk) == -EPROBE_DEFER) {
-		ret = -EPROBE_DEFER;
-		goto err;
-	}
-	if (!IS_ERR(pcm512x->sclk)) {
-		ret = clk_prepare_enable(pcm512x->sclk);
-		if (ret != 0) {
-			dev_err(dev, "Failed to enable SCLK: %d\n", ret);
+	for (i = 0; i < ARRAY_SIZE(clk_name); i++) {
+		pcm512x->sclk = devm_clk_get(dev, clk_name[i]);
+		if (PTR_ERR(pcm512x->sclk) == -EPROBE_DEFER) {
+			ret = -EPROBE_DEFER;
 			goto err;
 		}
+		if (!IS_ERR(pcm512x->sclk)) {
+			dev_dbg(dev, "SCLK detected by devm_clk_get\n");
+			ret = clk_prepare_enable(pcm512x->sclk);
+			if (ret != 0) {
+				dev_err(dev, "Failed to enable SCLK: %d\n",
+					ret);
+				goto err;
+			}
+			break;
+		}
+
+		if (!clk_name[i])
+			dev_dbg(dev, "no SCLK detected with NULL string\n");
+		else
+			dev_dbg(dev, "no SCLK detected for %s string\n",
+				clk_name[i]);
 	}
 
 	/* Default to standby mode */
-- 
2.20.1

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Andy Shevchenko]

On Thu, Apr 09, 2020 at 02:58:27PM -0500, Pierre-Louis Bossart wrote:
> Using devm_clk_get() with a NULL string fails on ACPI platforms, use
> the "sclk" string as a fallback.

This is fishy a bit.
Do we have this name in the bindings?

If no, why not simple switch to devm_clk_get_optional()?

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Pierre-Louis Bossart]

On 4/14/20 12:11 PM, Andy Shevchenko wrote:
> On Thu, Apr 09, 2020 at 02:58:27PM -0500, Pierre-Louis Bossart wrote:
>> Using devm_clk_get() with a NULL string fails on ACPI platforms, use
>> the "sclk" string as a fallback.
> 
> This is fishy a bit.

I didn't find a single example where we use a NULL string in ACPI cases?

> Do we have this name in the bindings?

No, that's probably a miss

> If no, why not simple switch to devm_clk_get_optional()?

Not sure what that would change?

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Andy Shevchenko]

On Tue, Apr 14, 2020 at 12:54:25PM -0500, Pierre-Louis Bossart wrote:
> On 4/14/20 12:11 PM, Andy Shevchenko wrote:
> > On Thu, Apr 09, 2020 at 02:58:27PM -0500, Pierre-Louis Bossart wrote:
> > > Using devm_clk_get() with a NULL string fails on ACPI platforms, use
> > > the "sclk" string as a fallback.
> > 
> > This is fishy a bit.
> 
> I didn't find a single example where we use a NULL string in ACPI cases?

...

> > If no, why not simple switch to devm_clk_get_optional()?
> 
> Not sure what that would change?

Hmm... Who is the provider of this clock?

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Pierre-Louis Bossart]

On 4/15/20 4:52 AM, Andy Shevchenko wrote:
> On Tue, Apr 14, 2020 at 12:54:25PM -0500, Pierre-Louis Bossart wrote:
>> On 4/14/20 12:11 PM, Andy Shevchenko wrote:
>>> On Thu, Apr 09, 2020 at 02:58:27PM -0500, Pierre-Louis Bossart wrote:
>>>> Using devm_clk_get() with a NULL string fails on ACPI platforms, use
>>>> the "sclk" string as a fallback.
>>>
>>> This is fishy a bit.
>>
>> I didn't find a single example where we use a NULL string in ACPI cases?
> 
> ...
> 
>>> If no, why not simple switch to devm_clk_get_optional()?
>>
>> Not sure what that would change?
> 
> Hmm... Who is the provider of this clock?

Well, at the hardware level, the clock is provided by two local 
oscillators controlled by the codec GPIOs. So you could consider that 
the codec is both the provider and consumer of the clock.

In the Linux world, the PCM512x codec driver creates a gpiochip. And the 
clk driver uses the gpios to expose a clk used by the PCM512x codec driver.

I am not fully happy with this design because it creates a double 
dependency which makes it impossible to remove modules. But I don't know 
how to model it otherwise.

But to go back to your question, the two parts are really joined at the 
hip since the same gpios exposed by one is used by the other.

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Andy Shevchenko]

On Wed, Apr 15, 2020 at 09:19:10AM -0500, Pierre-Louis Bossart wrote:
> On 4/15/20 4:52 AM, Andy Shevchenko wrote:
> > On Tue, Apr 14, 2020 at 12:54:25PM -0500, Pierre-Louis Bossart wrote:
> > > On 4/14/20 12:11 PM, Andy Shevchenko wrote:
> > > > On Thu, Apr 09, 2020 at 02:58:27PM -0500, Pierre-Louis Bossart wrote:
> > > > > Using devm_clk_get() with a NULL string fails on ACPI platforms, use
> > > > > the "sclk" string as a fallback.
> > > > 
> > > > This is fishy a bit.
> > > 
> > > I didn't find a single example where we use a NULL string in ACPI cases?
> > 
> > ...
> > 
> > > > If no, why not simple switch to devm_clk_get_optional()?
> > > 
> > > Not sure what that would change?
> > 
> > Hmm... Who is the provider of this clock?
> 
> Well, at the hardware level, the clock is provided by two local oscillators
> controlled by the codec GPIOs. So you could consider that the codec is both
> the provider and consumer of the clock.

Is it internal component to the codec or discrete (outside of codec chip)?
I bet it is the latter. Thus, codec is not provider. Board, where this
configuration is used, *is* provider of the clock(s).

> In the Linux world, the PCM512x codec driver creates a gpiochip. And the clk
> driver uses the gpios to expose a clk used by the PCM512x codec driver.

Yeah, hardware cyclic dependency :-)

> I am not fully happy with this design because it creates a double dependency
> which makes it impossible to remove modules. But I don't know how to model
> it otherwise.

I guess it should be clock provider which uses GPIO as a parameter to switch
clock rates, and codec driver, which provides GPIO chip and instantiates
(after) the clock provider instance. One module or several, is an
implementation detail.

> But to go back to your question, the two parts are really joined at the hip
> since the same gpios exposed by one is used by the other.

I got it, thanks for explanation.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Mark Brown]

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

On Thu, Apr 09, 2020 at 02:58:27PM -0500, Pierre-Louis Bossart wrote:
> Using devm_clk_get() with a NULL string fails on ACPI platforms, use
> the "sclk" string as a fallback.

Is this something that could be fixed at the ACPI level?

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

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Pierre-Louis Bossart]

On 4/14/20 12:45 PM, Mark Brown wrote:
> On Thu, Apr 09, 2020 at 02:58:27PM -0500, Pierre-Louis Bossart wrote:
>> Using devm_clk_get() with a NULL string fails on ACPI platforms, use
>> the "sclk" string as a fallback.
> 
> Is this something that could be fixed at the ACPI level?

I guess to fix this we'd need some sort of ACPI-level connection or 
description of the clock, and I've never seen such a description?

All the examples I've seen use an explicit 'mclk' string (that's e.g. 
what we did for the PMC clocks for Baytrail/Cherrytrail machine drivers, 
we added a lookup). Here I used 'sclk' since it's what TI refers to in 
their documentation.

I vaguely recall AMD had similar issues with another codec.

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Mark Brown]

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

On Tue, Apr 14, 2020 at 01:14:41PM -0500, Pierre-Louis Bossart wrote:
> On 4/14/20 12:45 PM, Mark Brown wrote:
> > On Thu, Apr 09, 2020 at 02:58:27PM -0500, Pierre-Louis Bossart wrote:

> > > Using devm_clk_get() with a NULL string fails on ACPI platforms, use
> > > the "sclk" string as a fallback.

> > Is this something that could be fixed at the ACPI level?

> I guess to fix this we'd need some sort of ACPI-level connection or
> description of the clock, and I've never seen such a description?

Wait, so SCLK is in the *global* namespace and the provider has to
register the same name?  That doesn't sound clever.  It might be better
to have the board register the connection from the clock provider to the
device rather than hard code global namespace strings like this, that
sounds like a recipie for misery.

It is really sad that nobody involved in producing these systems that
don't work with the current limitations in ACPI has been able to make
progress on improving ACPI so it can cope with modern hardware and we're
having to deal with this stuff.

> All the examples I've seen use an explicit 'mclk' string (that's e.g. what
> we did for the PMC clocks for Baytrail/Cherrytrail machine drivers, we added
> a lookup). Here I used 'sclk' since it's what TI refers to in their
> documentation.

They appear to call it SCK not SCLK.

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

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Pierre-Louis Bossart]

On 4/14/20 1:27 PM, Mark Brown wrote:
> On Tue, Apr 14, 2020 at 01:14:41PM -0500, Pierre-Louis Bossart wrote:
>> On 4/14/20 12:45 PM, Mark Brown wrote:
>>> On Thu, Apr 09, 2020 at 02:58:27PM -0500, Pierre-Louis Bossart wrote:
> 
>>>> Using devm_clk_get() with a NULL string fails on ACPI platforms, use
>>>> the "sclk" string as a fallback.
> 
>>> Is this something that could be fixed at the ACPI level?
> 
>> I guess to fix this we'd need some sort of ACPI-level connection or
>> description of the clock, and I've never seen such a description?
> 
> Wait, so SCLK is in the *global* namespace and the provider has to
> register the same name?  That doesn't sound clever.  It might be better
> to have the board register the connection from the clock provider to the
> device rather than hard code global namespace strings like this, that
> sounds like a recipie for misery.

I believe this change has zero impact on DT platforms.

The 'sclk' is a fallback here. If you find a clock with the NULL string, 
it's what gets used. Likewise for the clock provider, the 'sclk' is a 
lookup - an alias in other words. The use of the references and phandles 
should work just fine for Device Tree.

> It is really sad that nobody involved in producing these systems that
> don't work with the current limitations in ACPI has been able to make
> progress on improving ACPI so it can cope with modern hardware and we're
> having to deal with this stuff.

I can't disagree but I have to live with what's available to me as an 
audio guy...I had a solution two years ago where I could set the clock 
directly from the machine driver. The recommendation at the time was to 
use the clk framework, but that clk framework is limited for ACPI 
platforms, so we can only use it with these global names.

We had the same problem on Baytrail/Cherrytrail devices some 4 years ago 
and we had to use an 'mclk' alias. We are going to have the same problem 
when we expose the SSP MCLK, BLCK and FSYNC clocks - and that's also 
what the Skylake driver did - we don't have a solution without global names.

>> All the examples I've seen use an explicit 'mclk' string (that's e.g. what
>> we did for the PMC clocks for Baytrail/Cherrytrail machine drivers, we added
>> a lookup). Here I used 'sclk' since it's what TI refers to in their
>> documentation.
> 
> They appear to call it SCK not SCLK.

Yes indeed, will change.

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Mark Brown]

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

On Tue, Apr 14, 2020 at 02:15:16PM -0500, Pierre-Louis Bossart wrote:
> On 4/14/20 1:27 PM, Mark Brown wrote:

> > Wait, so SCLK is in the *global* namespace and the provider has to
> > register the same name?  That doesn't sound clever.  It might be better
> > to have the board register the connection from the clock provider to the
> > device rather than hard code global namespace strings like this, that
> > sounds like a recipie for misery.

> I believe this change has zero impact on DT platforms.

> The 'sclk' is a fallback here. If you find a clock with the NULL string,
> it's what gets used. Likewise for the clock provider, the 'sclk' is a lookup
> - an alias in other words. The use of the references and phandles should
> work just fine for Device Tree.

It's not just DT platforms that I'm worried about here, it's also ACPI
systems - all it takes is for a system to have a second device and a
name collision could happen, especially with such generic names.  We
tried to avoid doing this for board files for the same reason.

> > It is really sad that nobody involved in producing these systems that
> > don't work with the current limitations in ACPI has been able to make
> > progress on improving ACPI so it can cope with modern hardware and we're
> > having to deal with this stuff.

> I can't disagree but I have to live with what's available to me as an audio
> guy...I had a solution two years ago where I could set the clock directly
> from the machine driver. The recommendation at the time was to use the clk
> framework, but that clk framework is limited for ACPI platforms, so we can
> only use it with these global names.

My understanding is that ACPI just doesn't have clock bindings (or audio
bindings or...) so you're basically using board files here and board
files can definitely do more than we're seeing here.

> We had the same problem on Baytrail/Cherrytrail devices some 4 years ago and
> we had to use an 'mclk' alias. We are going to have the same problem when we
> expose the SSP MCLK, BLCK and FSYNC clocks - and that's also what the
> Skylake driver did - we don't have a solution without global names.

You should be able to register links between devices using the clock
API, or add that functionality if it's not there but AFAIK clkdev still
works.

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

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Pierre-Louis Bossart]

On 4/14/20 2:50 PM, Mark Brown wrote:
> On Tue, Apr 14, 2020 at 02:15:16PM -0500, Pierre-Louis Bossart wrote:
>> On 4/14/20 1:27 PM, Mark Brown wrote:
> 
>>> Wait, so SCLK is in the *global* namespace and the provider has to
>>> register the same name?  That doesn't sound clever.  It might be better
>>> to have the board register the connection from the clock provider to the
>>> device rather than hard code global namespace strings like this, that
>>> sounds like a recipie for misery.
> 
>> I believe this change has zero impact on DT platforms.
> 
>> The 'sclk' is a fallback here. If you find a clock with the NULL string,
>> it's what gets used. Likewise for the clock provider, the 'sclk' is a lookup
>> - an alias in other words. The use of the references and phandles should
>> work just fine for Device Tree.
> 
> It's not just DT platforms that I'm worried about here, it's also ACPI
> systems - all it takes is for a system to have a second device and a
> name collision could happen, especially with such generic names.  We
> tried to avoid doing this for board files for the same reason.

I am on the paranoid side but here I don't see much potential for conflicts:

a) this only works for the Up2 board with a HAT connector
b) this only work with the Hifiberry DAC+ PRO board.

This codec is not used in any traditional client devices.

> 
>>> It is really sad that nobody involved in producing these systems that
>>> don't work with the current limitations in ACPI has been able to make
>>> progress on improving ACPI so it can cope with modern hardware and we're
>>> having to deal with this stuff.
> 
>> I can't disagree but I have to live with what's available to me as an audio
>> guy...I had a solution two years ago where I could set the clock directly
>> from the machine driver. The recommendation at the time was to use the clk
>> framework, but that clk framework is limited for ACPI platforms, so we can
>> only use it with these global names.
> 
> My understanding is that ACPI just doesn't have clock bindings (or audio
> bindings or...) so you're basically using board files here and board
> files can definitely do more than we're seeing here.

I don't understand your definition of board file, sorry. We've never had 
one, the only thing that's board-specific is the machine driver.

>> We had the same problem on Baytrail/Cherrytrail devices some 4 years ago and
>> we had to use an 'mclk' alias. We are going to have the same problem when we
>> expose the SSP MCLK, BLCK and FSYNC clocks - and that's also what the
>> Skylake driver did - we don't have a solution without global names.
> 
> You should be able to register links between devices using the clock
> API, or add that functionality if it's not there but AFAIK clkdev still
> works.

The machine driver has no information whatsoever on who provides the 
clock. I just don't see how I might link stuff without at least some 
amount of information?

All I needed was to toggle 2 gpios to select 44.1 or 48kHz...Looks like 
it's going to take two more years, oh well.

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Pierre-Louis Bossart]

>>>> Wait, so SCLK is in the *global* namespace and the provider has to
>>>> register the same name?  That doesn't sound clever.  It might be better
>>>> to have the board register the connection from the clock provider to 
>>>> the
>>>> device rather than hard code global namespace strings like this, that
>>>> sounds like a recipie for misery.

Thinking a bit more on this, is the objection on the notion of using a 
fixed string, on the way it's registered or the lack of support for 
clocks in ACPI?

 From a quick look, the use of a fixed string is rather prevalent, see 
below. Less than 10% of codec drivers rely on a NULL string, so is it 
really a dangerous precedent so use "sclk" in this case? It seems to me 
that all clk providers need to use a unique string - what am I missing here?

adau17x1.c:	adau->mclk = devm_clk_get(dev, "mclk");
cs42l51.c:	cs42l51->mclk_handle = devm_clk_get(dev, "MCLK");
cs42xx8.c:	cs42xx8->clk = devm_clk_get(dev, "mclk");
cs53l30.c:	cs53l30->mclk = devm_clk_get(dev, "mclk");
cx2072x.c:	cx2072x->mclk = devm_clk_get(cx2072x->dev, "mclk");
da7213.c:	da7213->mclk = devm_clk_get(component->dev, "mclk");
da7218.c:	da7218->mclk = devm_clk_get(component->dev, "mclk");
da7219.c:	da7219->mclk = devm_clk_get(component->dev, "mclk");
es8316.c:	es8316->mclk = devm_clk_get_optional(component->dev, "mclk");
es8328.c:	es8328->clk = devm_clk_get(component->dev, NULL);
inno_rk3036.c:	priv->pclk = devm_clk_get(&pdev->dev, "acodec_pclk");
jz4725b.c:	icdc->clk = devm_clk_get(&pdev->dev, "aic");
jz4770.c:	codec->clk = devm_clk_get(dev, "aic");
lochnagar-sc.c:	priv->mclk = devm_clk_get(&pdev->dev, "mclk");
max98088.c:	max98088->mclk = devm_clk_get(&i2c->dev, "mclk");
max98090.c:	max98090->mclk = devm_clk_get(component->dev, "mclk");
max98095.c:	max98095->mclk = devm_clk_get(component->dev, "mclk");
max9860.c:	mclk = clk_get(dev, "mclk");
msm8916-wcd-analog.c:	priv->mclk = devm_clk_get(dev, "mclk");
msm8916-wcd-digital.c:	priv->ahbclk = devm_clk_get(dev, "ahbix-clk");
msm8916-wcd-digital.c:	priv->mclk = devm_clk_get(dev, "mclk");
msm8916-wcd-digital.c:	mclk_rate = clk_get_rate(msm8916_wcd->mclk);
nau8825.c:	nau8825->mclk = devm_clk_get(nau8825->dev, "mclk");
nau8825.c:	nau8825->mclk = devm_clk_get(dev, "mclk");
pcm3168a.c:	pcm3168a->scki = devm_clk_get(dev, "scki");
pcm512x.c:	pcm512x->sclk = devm_clk_get(dev, NULL);
rk3328_codec.c:	rk3328->mclk = devm_clk_get(&pdev->dev, "mclk");
rk3328_codec.c:	rk3328->pclk = devm_clk_get(&pdev->dev, "pclk");
rt5514.c:	rt5514->mclk = devm_clk_get(component->dev, "mclk");
rt5616.c:	rt5616->mclk = devm_clk_get(component->dev, "mclk");
rt5640.c:	rt5640->mclk = devm_clk_get(component->dev, "mclk");
rt5659.c:	rt5659->mclk = devm_clk_get(&i2c->dev, "mclk");
rt5660.c:	rt5660->mclk = devm_clk_get(&i2c->dev, "mclk");
rt5682.c:		rt5682->mclk = devm_clk_get(component->dev, "mclk");
sirf-audio-codec.c:	sirf_audio_codec->clk = devm_clk_get(&pdev->dev, NULL);
sta32x.c:	sta32x->xti_clk = devm_clk_get(dev, "xti");
tas571x.c:	priv->mclk = devm_clk_get(dev, "mclk");
tlv320aic32x4.c:	pll = devm_clk_get(component->dev, "pll");
tscs42xx.c:		tscs42xx->sysclk = devm_clk_get(&i2c->dev, src_names[src]);
tscs454.c:		freq = clk_get_rate(tscs454->sysclk);
tscs454.c:		tscs454->sysclk = devm_clk_get(&i2c->dev, src_names[src]);
wcd9335.c:	wcd->mclk = devm_clk_get(dev, "mclk");
wcd9335.c:	wcd->native_clk = devm_clk_get(dev, "slimbus");
wm2000.c:	wm2000->mclk = devm_clk_get(&i2c->dev, "MCLK");
wm8731.c:	wm8731->mclk = devm_clk_get(&spi->dev, "mclk");
wm8731.c:	wm8731->mclk = devm_clk_get(&i2c->dev, "mclk");
wm8904.c:	wm8904->mclk = devm_clk_get(&i2c->dev, "mclk");
wm8960.c:	wm8960->mclk = devm_clk_get(&i2c->dev, "mclk");
wm8962.c:	pdata->mclk = devm_clk_get(&i2c->dev, NULL);

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Mark Brown]

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

On Tue, Apr 14, 2020 at 04:02:00PM -0500, Pierre-Louis Bossart wrote:

> Thinking a bit more on this, is the objection on the notion of using a fixed
> string, on the way it's registered or the lack of support for clocks in
> ACPI?

The issue is using a clock named in the global namespace.  Like I keep
saying you're not using ACPI here, you're using board files and board
files can do better.

> From a quick look, the use of a fixed string is rather prevalent, see below.
> Less than 10% of codec drivers rely on a NULL string, so is it really a
> dangerous precedent so use "sclk" in this case? It seems to me that all clk
> providers need to use a unique string - what am I missing here?

> adau17x1.c:	adau->mclk = devm_clk_get(dev, "mclk");

Notice how all the clock lookup functions take both a device and a
string - the device is important here, the string is namespaced with the
device in most usage (including board file usage) so if two different
devices ask for the same name they might get different clocks.

> wm8962.c:	pdata->mclk = devm_clk_get(&i2c->dev, NULL);

This is how lookups that don't even specify a name can work.  You seem
to want to rely on the name only which is very much not good practice,
even on board files.

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

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Mark Brown]

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

On Tue, Apr 14, 2020 at 03:13:01PM -0500, Pierre-Louis Bossart wrote:
> On 4/14/20 2:50 PM, Mark Brown wrote:

> > It's not just DT platforms that I'm worried about here, it's also ACPI
> > systems - all it takes is for a system to have a second device and a
> > name collision could happen, especially with such generic names.  We
> > tried to avoid doing this for board files for the same reason.

> I am on the paranoid side but here I don't see much potential for conflicts:

> a) this only works for the Up2 board with a HAT connector
> b) this only work with the Hifiberry DAC+ PRO board.

> This codec is not used in any traditional client devices.

That's what you're doing right now but someone else can use the same
devices, or adopt the same approaches on something like a Chromebook.

> > My understanding is that ACPI just doesn't have clock bindings (or audio
> > bindings or...) so you're basically using board files here and board
> > files can definitely do more than we're seeing here.

> I don't understand your definition of board file, sorry. We've never had
> one, the only thing that's board-specific is the machine driver.

Architectures that don't have firmware bindings use straight C code to
register and set things up.  Machine drivers are essentially board
files, they're just audio specific bits of board file that use audio
APIs and so are in the sound directory.

> > You should be able to register links between devices using the clock
> > API, or add that functionality if it's not there but AFAIK clkdev still
> > works.

> The machine driver has no information whatsoever on who provides the clock.
> I just don't see how I might link stuff without at least some amount of
> information?

The machine driver must have this information, it knows exactly what
hardware it runs on.  The whole point of a machine driver is that it's
board specific.

> All I needed was to toggle 2 gpios to select 44.1 or 48kHz...Looks like it's
> going to take two more years, oh well.

I think you're giving up way too easily here.  The kernel has really
good support for systems that don't have any firmware description at
all, this shouldn't be complex or breaking new ground.

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

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Pierre-Louis Bossart]

On 4/15/20 6:36 AM, Mark Brown wrote:
> On Tue, Apr 14, 2020 at 03:13:01PM -0500, Pierre-Louis Bossart wrote:
>> On 4/14/20 2:50 PM, Mark Brown wrote:
> 
>>> It's not just DT platforms that I'm worried about here, it's also ACPI
>>> systems - all it takes is for a system to have a second device and a
>>> name collision could happen, especially with such generic names.  We
>>> tried to avoid doing this for board files for the same reason.
> 
>> I am on the paranoid side but here I don't see much potential for conflicts:
> 
>> a) this only works for the Up2 board with a HAT connector
>> b) this only work with the Hifiberry DAC+ PRO board.
> 
>> This codec is not used in any traditional client devices.
> 
> That's what you're doing right now but someone else can use the same
> devices, or adopt the same approaches on something like a Chromebook.
> 
>>> My understanding is that ACPI just doesn't have clock bindings (or audio
>>> bindings or...) so you're basically using board files here and board
>>> files can definitely do more than we're seeing here.
> 
>> I don't understand your definition of board file, sorry. We've never had
>> one, the only thing that's board-specific is the machine driver.
> 
> Architectures that don't have firmware bindings use straight C code to
> register and set things up.  Machine drivers are essentially board
> files, they're just audio specific bits of board file that use audio
> APIs and so are in the sound directory.

Humm, we may have a conceptual disconnect here. In the ACPI world, there 
is no support for the machine driver - unlike Device Tree. It is probed 
when the SST/SOF driver creates a platform device using the codec _HID 
as a key to hard-coded lookup tables in 
sound/soc/intel/common/soc-acpi*.c - it will be probed *after* the codec 
driver probes. I really don't see how to use the machine driver as 
currently implemented to establish board-level connections that would 
influence the codec driver probe and its use of a clock.

> 
>>> You should be able to register links between devices using the clock
>>> API, or add that functionality if it's not there but AFAIK clkdev still
>>> works.
> 
>> The machine driver has no information whatsoever on who provides the clock.
>> I just don't see how I might link stuff without at least some amount of
>> information?
> 
> The machine driver must have this information, it knows exactly what
> hardware it runs on.  The whole point of a machine driver is that it's
> board specific.
> 
>> All I needed was to toggle 2 gpios to select 44.1 or 48kHz...Looks like it's
>> going to take two more years, oh well.
> 
> I think you're giving up way too easily here.  The kernel has really
> good support for systems that don't have any firmware description at
> all, this shouldn't be complex or breaking new ground.

See above, I don't think the machine driver can do what you had in mind?

I don't see how to proceed unless we remove all support for ACPI, both 
for codec and clock driver, and trigger their probe "manually" with a 
board-level initialization.

And btw there's already a precedent for using global names, it's what 
the Skylake driver does for the mclk and ssp clocks. To the best of my 
knowledge the device specific namespacing does not exist on any ACPI 
platform. We have a request from Dialog to implement the same thing for 
SOF to solve dependencies on the clock being stable before turning on 
the codec, so if global names are not acceptable we have a real problem.

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Mark Brown]

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

On Wed, Apr 15, 2020 at 09:44:12AM -0500, Pierre-Louis Bossart wrote:
> On 4/15/20 6:36 AM, Mark Brown wrote:

> > Architectures that don't have firmware bindings use straight C code to
> > register and set things up.  Machine drivers are essentially board
> > files, they're just audio specific bits of board file that use audio
> > APIs and so are in the sound directory.

> Humm, we may have a conceptual disconnect here. In the ACPI world, there is
> no support for the machine driver - unlike Device Tree. It is probed when

This is nothing to do with device tree except in that it has useful
firmware descriptions of the hardware.

> the SST/SOF driver creates a platform device using the codec _HID as a key
> to hard-coded lookup tables in sound/soc/intel/common/soc-acpi*.c - it will
> be probed *after* the codec driver probes. I really don't see how to use the
> machine driver as currently implemented to establish board-level connections
> that would influence the codec driver probe and its use of a clock.

You have the opportunity to run whatever code you want to run at the
point where you're registering your drivers with the system on module
init, things like DMI quirk tables (which is what you're going to need
to do here AFAICT) should work just as well there as they do later on
when the driver loads.

> > I think you're giving up way too easily here.  The kernel has really
> > good support for systems that don't have any firmware description at
> > all, this shouldn't be complex or breaking new ground.

> See above, I don't think the machine driver can do what you had in mind?

> I don't see how to proceed unless we remove all support for ACPI, both for
> codec and clock driver, and trigger their probe "manually" with a
> board-level initialization.

The clkdev stuff can use dev_name() so so long as the devices appear
with predictable names you should be fine.  If not IIRC everything in
ACPI is named in the AML so clkdev could be extended to be able to find
things based on the names it gives.

> And btw there's already a precedent for using global names, it's what the
> Skylake driver does for the mclk and ssp clocks. To the best of my knowledge
> the device specific namespacing does not exist on any ACPI platform. We have

No machine description at all exists on board file systems other than
what we write in C and they manage to cope with this, I'm sure we can
find a way to do it with ACPI.  I mentioned clkdev before, that is
something that's done entirely at the Linux level.

> a request from Dialog to implement the same thing for SOF to solve
> dependencies on the clock being stable before turning on the codec, so if
> global names are not acceptable we have a real problem.

If existing usages that have ended up getting merged are going to be
used to push for additional adoption then that's not encouraging.

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

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Pierre-Louis Bossart]

>> the SST/SOF driver creates a platform device using the codec _HID as a key
>> to hard-coded lookup tables in sound/soc/intel/common/soc-acpi*.c - it will
>> be probed *after* the codec driver probes. I really don't see how to use the
>> machine driver as currently implemented to establish board-level connections
>> that would influence the codec driver probe and its use of a clock.
> 
> You have the opportunity to run whatever code you want to run at the
> point where you're registering your drivers with the system on module
> init, things like DMI quirk tables (which is what you're going to need
> to do here AFAICT) should work just as well there as they do later on
> when the driver loads.

The idea here was to have one single build, and then rely on what the 
user configured with initrd override to probe the right I2C codec driver 
and indirectly the machine driver. It's similar to device tree overlays.

With the same up2 board, I change the .aml file in 
/lib/firmware/acpi-upgrades, swap one HAT board for another and the new 
board is handled automagically.

I don't see how I can use hard-coded DMI tables or board-specific things 
without losing the single build?

>>> I think you're giving up way too easily here.  The kernel has really
>>> good support for systems that don't have any firmware description at
>>> all, this shouldn't be complex or breaking new ground.
> 
>> See above, I don't think the machine driver can do what you had in mind?
> 
>> I don't see how to proceed unless we remove all support for ACPI, both for
>> codec and clock driver, and trigger their probe "manually" with a
>> board-level initialization.
> 
> The clkdev stuff can use dev_name() so so long as the devices appear
> with predictable names you should be fine.  If not IIRC everything in
> ACPI is named in the AML so clkdev could be extended to be able to find
> things based on the names it gives.

I had a discussion with Andy Shevchenko that we should precisely not be 
using dev_name() since we don't control the names that ACPI selects for 
us. And since I was using the generic PRP0001 thing for the clock device 
to probe using the 'compatible' string it's actually even less reliable 
and unique...

>> And btw there's already a precedent for using global names, it's what the
>> Skylake driver does for the mclk and ssp clocks. To the best of my knowledge
>> the device specific namespacing does not exist on any ACPI platform. We have
> 
> No machine description at all exists on board file systems other than
> what we write in C and they manage to cope with this, I'm sure we can
> find a way to do it with ACPI.  I mentioned clkdev before, that is
> something that's done entirely at the Linux level.
> 
>> a request from Dialog to implement the same thing for SOF to solve
>> dependencies on the clock being stable before turning on the codec, so if
>> global names are not acceptable we have a real problem.
> 
> If existing usages that have ended up getting merged are going to be
> used to push for additional adoption then that's not encouraging.

I wasn't trying to push this against your will, rather I wanted to 
highlight that we should be clear on the direction for all these uses of 
the clk API in an ACPI context. If what I suggested here is not the 
right path, then how do we deal with all the existing cases? This 
PCM512x use is not a mainstream usage, we use this board mainly for 
validation and for community support, the other cases with 'mclk' and 
'sspX_fsync' are critical and impact devices shipping in large volumes.

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Mark Brown]

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

On Wed, Apr 15, 2020 at 12:26:57PM -0500, Pierre-Louis Bossart wrote:

> > You have the opportunity to run whatever code you want to run at the
> > point where you're registering your drivers with the system on module
> > init, things like DMI quirk tables (which is what you're going to need
> > to do here AFAICT) should work just as well there as they do later on
> > when the driver loads.

> The idea here was to have one single build, and then rely on what the user
> configured with initrd override to probe the right I2C codec driver and
> indirectly the machine driver. It's similar to device tree overlays.

> With the same up2 board, I change the .aml file in
> /lib/firmware/acpi-upgrades, swap one HAT board for another and the new
> board is handled automagically.

> I don't see how I can use hard-coded DMI tables or board-specific things
> without losing the single build?

Ugh, so you change for another machine description and don't update any
of the DMI information?  Perhaps you can't...  That doesn't seem very
ACPI given how reliant it is on doing quirks based on DMI data for
modern systems :/

> > The clkdev stuff can use dev_name() so so long as the devices appear
> > with predictable names you should be fine.  If not IIRC everything in
> > ACPI is named in the AML so clkdev could be extended to be able to find
> > things based on the names it gives.

> I had a discussion with Andy Shevchenko that we should precisely not be
> using dev_name() since we don't control the names that ACPI selects for us.

It's not ideal and you should definitely use something better if it's
available but if it's all you've got...  You could also search for the
device by binding string at runtime, that'd blow up if you've got more
than one of the same device but it's a bit less likely.

> And since I was using the generic PRP0001 thing for the clock device to
> probe using the 'compatible' string it's actually even less reliable and
> unique...

I see, though actually that might be a place to set up links from now I
think about it - if you know what the I2C device is going to be called
you could have the platform device for the clock source register the
links too.

> > If existing usages that have ended up getting merged are going to be
> > used to push for additional adoption then that's not encouraging.

> I wasn't trying to push this against your will, rather I wanted to highlight
> that we should be clear on the direction for all these uses of the clk API
> in an ACPI context. If what I suggested here is not the right path, then how
> do we deal with all the existing cases? This PCM512x use is not a mainstream
> usage, we use this board mainly for validation and for community support,
> the other cases with 'mclk' and 'sspX_fsync' are critical and impact devices
> shipping in large volumes.

The ideal thing would of course be to extend ACPI to encode things like
this in the firmware description so that it is able to reflect the
reality of modern systems, the graph bits of this were already specified
so most of the work has been done.  However it's a bit late for
the shipping systems.

Normal shipping systems are in a lot better position here in that they 
do have some hope of being able to patch up the links after the fact
with DMI based quirks.  It really would be good to see a way of doing
that deployed, especially in cases where you might otherwise have to
modify the CODEC driver.  I *think* that should be doable, assuming
there's some stable or runtime discoverable naming for the ACPI devices.

In the case of this driver could you look at registering the link from
the device for the clocks?  Have it say "I supply SCK on device X" as it
registers.  That should be fairly straightforward I think, we do that
for one of the regulators.

The main thing I want to avoid is having to have the CODEC drivers know
platform specific strings that they're supposed to look up, or general
approaches where that ends up being a thing that looks idiomatic.  That
was something board files did for a while, it didn't work very well and
we did something better with clkdev instead.  I'm a lot less worried
about this for cases where it's two devices that are part of the SoC
talking to each other, that's relatively well controled and doesn't
affect non-x86 platforms.  When it starts touching the CODECs it's a lot
more worrying.

I think by now there's ample evidence that it's worth investing in
better firmware descriptions :(

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

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Pierre-Louis Bossart]

> In the case of this driver could you look at registering the link from
> the device for the clocks?  Have it say "I supply SCK on device X" as it
> registers.  That should be fairly straightforward I think, we do that
> for one of the regulators.

When you wrote 'in the case of this driver', were you referring to the 
clock provider, saying 'I support SCK on device i2c-104C5122:00' ?

If you have a pointer on the regulator example, I'd appreciate it, I am 
really way beyond my comfort zone.

> The main thing I want to avoid is having to have the CODEC drivers know
> platform specific strings that they're supposed to look up, or general
> approaches where that ends up being a thing that looks idiomatic.  That
> was something board files did for a while, it didn't work very well and
> we did something better with clkdev instead.  I'm a lot less worried
> about this for cases where it's two devices that are part of the SoC
> talking to each other, that's relatively well controled and doesn't
> affect non-x86 platforms.  When it starts touching the CODECs it's a lot
> more worrying.

I see the nuance, thanks for the clarification.

Maybe an alternate suggestion if you want to avoid hard-coded strings in 
the kernel: what if we added optional properties for the clock lookup 
name in both the codec and clock driver, and set the name in a _DSD 
blob. That would move the platform-specific names to platform firmware, 
and avoid the links described above that are probably ACPI-only.

In this case we can add whatever we want, the DSDT table contains 
absolutely nothing for audio so we can add things as needed, and in case 
another usage of this codec happens in a future device they'd have to 
define their own clock name and store it in platform firmware.

> I think by now there's ample evidence that it's worth investing in
> better firmware descriptions :(

Indeed, and tools to check they are correct! Most of the stuff we 
defined for SoundWire ends-up wrong or undefined, still an uphill battle...

Re: [RFC PATCH 02/16] ASoC: pcm512x: use "sclk" string to retrieve clock

[Mark Brown]

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

On Wed, Apr 15, 2020 at 03:22:50PM -0500, Pierre-Louis Bossart wrote:

> > In the case of this driver could you look at registering the link from
> > the device for the clocks?  Have it say "I supply SCK on device X" as it
> > registers.  That should be fairly straightforward I think, we do that
> > for one of the regulators.

> When you wrote 'in the case of this driver', were you referring to the clock
> provider, saying 'I support SCK on device i2c-104C5122:00' ?

Yes.

> If you have a pointer on the regulator example, I'd appreciate it, I am
> really way beyond my comfort zone.

The arizona driver is what I was thinking of, that's more complex than
you proabably need as it's a MFD.

> Maybe an alternate suggestion if you want to avoid hard-coded strings in the
> kernel: what if we added optional properties for the clock lookup name in
> both the codec and clock driver, and set the name in a _DSD blob. That would
> move the platform-specific names to platform firmware, and avoid the links
> described above that are probably ACPI-only.

If you read the lookup information from firmware somehow that's probably
fine I think.  It's not nice but if it's baked into firmware...

> > I think by now there's ample evidence that it's worth investing in
> > better firmware descriptions :(

> Indeed, and tools to check they are correct! Most of the stuff we defined
> for SoundWire ends-up wrong or undefined, still an uphill battle...

The audio-graph-card appears to be working really well FWIW,
Morimoto-san did an excellent job there.

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

[RFC PATCH 03/16] ASoC: Intel: sof-pcm512x: use gpiod for LED

[Pierre-Louis Bossart]

Remove direct regmap access, use gpios exposed by PCM512x codec
Keep the codec_init function, this will be used in following patches

The gpios handling is done with an explicit lookup table. We cannot
use ACPI-based mappings since we don't have an ACPI device for the
machine driver, and the gpiochip is created during the probe of the
PCM512x driver.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/boards/sof_pcm512x.c | 45 ++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/sound/soc/intel/boards/sof_pcm512x.c b/sound/soc/intel/boards/sof_pcm512x.c
index fb7811899999..dcd769b352fa 100644
--- a/sound/soc/intel/boards/sof_pcm512x.c
+++ b/sound/soc/intel/boards/sof_pcm512x.c
@@ -10,6 +10,8 @@
 #include <linux/i2c.h>
 #include <linux/input.h>
 #include <linux/module.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/machine.h>
 #include <linux/platform_device.h>
 #include <linux/types.h>
 #include <sound/core.h>
@@ -43,6 +45,7 @@ struct sof_hdmi_pcm {
 struct sof_card_private {
 	struct list_head hdmi_pcm_list;
 	bool idisp_codec;
+	struct gpio_desc *gpio_4;
 };
 
 static int sof_pcm512x_quirk_cb(const struct dmi_system_id *id)
@@ -84,23 +87,16 @@ static int sof_hdmi_init(struct snd_soc_pcm_runtime *rtd)
 
 static int sof_pcm512x_codec_init(struct snd_soc_pcm_runtime *rtd)
 {
-	struct snd_soc_component *codec = asoc_rtd_to_codec(rtd, 0)->component;
-
-	snd_soc_component_update_bits(codec, PCM512x_GPIO_EN, 0x08, 0x08);
-	snd_soc_component_update_bits(codec, PCM512x_GPIO_OUTPUT_4, 0x0f, 0x02);
-	snd_soc_component_update_bits(codec, PCM512x_GPIO_CONTROL_1,
-				      0x08, 0x08);
-
 	return 0;
 }
 
 static int aif1_startup(struct snd_pcm_substream *substream)
 {
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
-	struct snd_soc_component *codec = asoc_rtd_to_codec(rtd, 0)->component;
+	struct sof_card_private *ctx = snd_soc_card_get_drvdata(rtd->card);
 
-	snd_soc_component_update_bits(codec, PCM512x_GPIO_CONTROL_1,
-				      0x08, 0x08);
+	/* Turn LED on */
+	gpiod_set_value(ctx->gpio_4, 1);
 
 	return 0;
 }
@@ -108,10 +104,10 @@ static int aif1_startup(struct snd_pcm_substream *substream)
 static void aif1_shutdown(struct snd_pcm_substream *substream)
 {
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
-	struct snd_soc_component *codec = asoc_rtd_to_codec(rtd, 0)->component;
+	struct sof_card_private *ctx = snd_soc_card_get_drvdata(rtd->card);
 
-	snd_soc_component_update_bits(codec, PCM512x_GPIO_CONTROL_1,
-				      0x08, 0x00);
+	/* Turn LED off */
+	gpiod_set_value(ctx->gpio_4, 0);
 }
 
 static const struct snd_soc_ops sof_pcm512x_ops = {
@@ -354,6 +350,14 @@ static struct snd_soc_dai_link *sof_card_dai_links_create(struct device *dev,
 	return NULL;
 }
 
+static struct gpiod_lookup_table pcm512x_gpios_table = {
+	/* .dev_id set during probe */
+	.table = {
+		GPIO_LOOKUP("pcm512x-gpio", 3, "PCM512x-GPIO4", GPIO_ACTIVE_HIGH),
+		{ },
+	},
+};
+
 static int sof_audio_probe(struct platform_device *pdev)
 {
 	struct snd_soc_acpi_mach *mach = pdev->dev.platform_data;
@@ -413,6 +417,21 @@ static int sof_audio_probe(struct platform_device *pdev)
 
 	snd_soc_card_set_drvdata(&sof_audio_card_pcm512x, ctx);
 
+	/*
+	 * Enable GPIO4 for LED
+	 */
+	pcm512x_gpios_table.dev_id = dev_name(&pdev->dev);
+	gpiod_add_lookup_table(&pcm512x_gpios_table);
+
+	ctx->gpio_4 = devm_gpiod_get(&pdev->dev, "PCM512x-GPIO4",
+				     GPIOD_OUT_LOW);
+
+	if (IS_ERR(ctx->gpio_4)) {
+		dev_err(&pdev->dev, "gpio4 not found\n");
+		ret = PTR_ERR(ctx->gpio_4);
+		return ret;
+	}
+
 	return devm_snd_soc_register_card(&pdev->dev,
 					  &sof_audio_card_pcm512x);
 }
-- 
2.20.1

Re: [RFC PATCH 03/16] ASoC: Intel: sof-pcm512x: use gpiod for LED

[Andy Shevchenko]

On Thu, Apr 09, 2020 at 02:58:28PM -0500, Pierre-Louis Bossart wrote:
> Remove direct regmap access, use gpios exposed by PCM512x codec
> Keep the codec_init function, this will be used in following patches
> 
> The gpios handling is done with an explicit lookup table. We cannot
> use ACPI-based mappings since we don't have an ACPI device for the
> machine driver, and the gpiochip is created during the probe of the
> PCM512x driver.

...

> +#include <linux/gpio/machine.h>

Okay, it's a board code.

...

> +static struct gpiod_lookup_table pcm512x_gpios_table = {
> +	/* .dev_id set during probe */
> +	.table = {

> +		GPIO_LOOKUP("pcm512x-gpio", 3, "PCM512x-GPIO4", GPIO_ACTIVE_HIGH),

It says GPIO 4 and here is number 3.
Does this 4 come from hardware documentation?

> +		{ },

No comma for terminator entries.

> +	},
> +};

...

> +	gpiod_add_lookup_table(&pcm512x_gpios_table);

Where is the counterpart gpiod_remove_lookup_table() call?

> +	ctx->gpio_4 = devm_gpiod_get(&pdev->dev, "PCM512x-GPIO4",
> +				     GPIOD_OUT_LOW);

Can driver work without this GPIO? If so, perhaps devm_gpiod_get_optional().

> +	if (IS_ERR(ctx->gpio_4)) {
> +		dev_err(&pdev->dev, "gpio4 not found\n");
> +		ret = PTR_ERR(ctx->gpio_4);
> +		return ret;
> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 03/16] ASoC: Intel: sof-pcm512x: use gpiod for LED

[Mark Brown]

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

On Tue, Apr 14, 2020 at 08:17:52PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 09, 2020 at 02:58:28PM -0500, Pierre-Louis Bossart wrote:

> > +		GPIO_LOOKUP("pcm512x-gpio", 3, "PCM512x-GPIO4", GPIO_ACTIVE_HIGH),

> It says GPIO 4 and here is number 3.
> Does this 4 come from hardware documentation?

Yes, the first GPIO in the device is GPIO1.

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

Re: [RFC PATCH 03/16] ASoC: Intel: sof-pcm512x: use gpiod for LED

[Pierre-Louis Bossart]

>> +static struct gpiod_lookup_table pcm512x_gpios_table = {
>> +	/* .dev_id set during probe */
>> +	.table = {
> 
>> +		GPIO_LOOKUP("pcm512x-gpio", 3, "PCM512x-GPIO4", GPIO_ACTIVE_HIGH),
> 
> It says GPIO 4 and here is number 3.
> Does this 4 come from hardware documentation?

Yes TI count from 1 to 6 in their documentation. The initial HifiBerry 
DAC+ also counts from 1 to 6. I can add a comment here.

> 
>> +		{ },
> 
> No comma for terminator entries.

ok

> 
>> +	},
>> +};
> 
> ...
> 
>> +	gpiod_add_lookup_table(&pcm512x_gpios_table);
> 
> Where is the counterpart gpiod_remove_lookup_table() call?

Ah that's a miss, thanks for flagging this. I remember looking but 
obviously needed a coffee at the time.

>> +	ctx->gpio_4 = devm_gpiod_get(&pdev->dev, "PCM512x-GPIO4",
>> +				     GPIOD_OUT_LOW);
> 
> Can driver work without this GPIO? If so, perhaps devm_gpiod_get_optional().

that part yes, it's only for the LED, but if this fails then probably 
the rest of the code will also fail.
> 
>> +	if (IS_ERR(ctx->gpio_4)) {
>> +		dev_err(&pdev->dev, "gpio4 not found\n");
>> +		ret = PTR_ERR(ctx->gpio_4);
>> +		return ret;
>> +	}
> 

Re: [RFC PATCH 03/16] ASoC: Intel: sof-pcm512x: use gpiod for LED

[Andy Shevchenko]

On Tue, Apr 14, 2020 at 12:57:35PM -0500, Pierre-Louis Bossart wrote:

...

> > > +		GPIO_LOOKUP("pcm512x-gpio", 3, "PCM512x-GPIO4", GPIO_ACTIVE_HIGH),
> > 
> > It says GPIO 4 and here is number 3.
> > Does this 4 come from hardware documentation?
> 
> Yes TI count from 1 to 6 in their documentation. The initial HifiBerry DAC+
> also counts from 1 to 6. I can add a comment here.

Okay!

...

> > > +	ctx->gpio_4 = devm_gpiod_get(&pdev->dev, "PCM512x-GPIO4",
> > > +				     GPIOD_OUT_LOW);
> > 
> > Can driver work without this GPIO? If so, perhaps devm_gpiod_get_optional().
> 
> that part yes, it's only for the LED, but if this fails then probably the
> rest of the code will also fail.

The problem with above code that it's setting the hard dependency to a LED
gpio. Is it crucial to get codec working? I bet no. In case
gpiod_get_optional() fails, it will be correct to bail out, because it will
mean other kind of errors not related to optionality of the GPIO (rather it's
present, but something went wrong).

> > > +	if (IS_ERR(ctx->gpio_4)) {
> > > +		dev_err(&pdev->dev, "gpio4 not found\n");
> > > +		ret = PTR_ERR(ctx->gpio_4);
> > > +		return ret;
> > > +	}

-- 
With Best Regards,
Andy Shevchenko

[RFC PATCH 04/16] ASoC: Intel: sof-pcm512x: detect Hifiberry DAC+ PRO

[Pierre-Louis Bossart]

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(+)

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;
 }
 
-- 
2.20.1

Re: [RFC PATCH 04/16] ASoC: Intel: sof-pcm512x: detect Hifiberry DAC+ PRO

[Andy Shevchenko]

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;
> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 04/16] ASoC: Intel: sof-pcm512x: detect Hifiberry DAC+ PRO

[Pierre-Louis Bossart]

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;
>> +	}
> 

Re: [RFC PATCH 04/16] ASoC: Intel: sof-pcm512x: detect Hifiberry DAC+ PRO

[Andy Shevchenko]

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;
> > > +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 04/16] ASoC: Intel: sof-pcm512x: detect Hifiberry DAC+ PRO

[Pierre-Louis Bossart]

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;
>>>> +	}
> 

Re: [RFC PATCH 04/16] ASoC: Intel: sof-pcm512x: detect Hifiberry DAC+ PRO

[Andy Shevchenko]

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;
> > > > > +	}

-- 
With Best Regards,
Andy Shevchenko

[RFC PATCH 05/16] ASoC: Intel: sof-pcm512x: reconfigure sclk in hw_params if needed

[Pierre-Louis Bossart]

The SCLK is resumed by the codec driver. In case the rate specified in
hw_params does not match the current configuration, disable, set the
new rate and restart the clock.

There is no operation on hw_free, the codec suspend routine will
disable/deprepare the clock.

Note that we don't change the DAI configuration when the DAC+ PRO is
detected. All changes for the codec master mode are handled in the
topology file (DAI configuration change and scheduling change)

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/boards/sof_pcm512x.c | 94 ++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/sound/soc/intel/boards/sof_pcm512x.c b/sound/soc/intel/boards/sof_pcm512x.c
index c1d2a53c1ec8..b5153ce954c7 100644
--- a/sound/soc/intel/boards/sof_pcm512x.c
+++ b/sound/soc/intel/boards/sof_pcm512x.c
@@ -145,6 +145,31 @@ static int sof_pcm512x_codec_init(struct snd_soc_pcm_runtime *rtd)
 	return 0;
 }
 
+static int aif1_update_rate_den(struct snd_pcm_substream *substream,
+				struct snd_pcm_hw_params *params)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct sof_card_private *ctx = snd_soc_card_get_drvdata(rtd->card);
+	struct snd_ratnum rats_no_pll;
+	unsigned int num = 0, den = 0;
+	int err;
+
+	rats_no_pll.num = clk_get_rate(ctx->sclk) / 64;
+	rats_no_pll.den_min = 1;
+	rats_no_pll.den_max = 128;
+	rats_no_pll.den_step = 1;
+
+	err = snd_interval_ratnum(hw_param_interval(params,
+						    SNDRV_PCM_HW_PARAM_RATE),
+				  1, &rats_no_pll, &num, &den);
+	if (err >= 0 && den) {
+		params->rate_num = num;
+		params->rate_den = den;
+	}
+
+	return 0;
+}
+
 static int aif1_startup(struct snd_pcm_substream *substream)
 {
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
@@ -156,6 +181,74 @@ static int aif1_startup(struct snd_pcm_substream *substream)
 	return 0;
 }
 
+static int aif1_hw_params(struct snd_pcm_substream *substream,
+			  struct snd_pcm_hw_params *params)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct sof_card_private *ctx = snd_soc_card_get_drvdata(rtd->card);
+	struct device *dev = rtd->card->dev;
+	int current_rate;
+	int sclk_rate;
+	int channels;
+	int width;
+	int rate;
+	int ret = 0;
+
+	if (ctx->is_dac_pro) {
+		rate = params_rate(params);
+		channels = params_channels(params);
+		width = snd_pcm_format_physical_width(params_format(params));
+
+		if (rate % 24000)
+			sclk_rate = 22579200;
+		else
+			sclk_rate = 24576000;
+
+		current_rate = clk_get_rate(ctx->sclk);
+		if (current_rate != sclk_rate) {
+			/*
+			 * The sclk clock is started and stopped by the codec
+			 * resume/suspend functions. If the rate isn't correct,
+			 * stop, set the new rate and restart the clock
+			 */
+
+			dev_dbg(dev, "reconfiguring SCLK to rate %d\n",
+				sclk_rate);
+
+			clk_disable_unprepare(ctx->sclk);
+
+			ret = clk_set_rate(ctx->sclk, sclk_rate);
+			if (ret) {
+				dev_err(dev, "Could not set SCLK rate %d\n",
+					sclk_rate);
+				return ret;
+			}
+
+			ret = clk_prepare_enable(ctx->sclk);
+			if (ret) {
+				dev_err(dev, "Failed to enable SCLK: %d\n",
+					ret);
+				return ret;
+			}
+		}
+
+		ret = aif1_update_rate_den(substream, params);
+		if (ret) {
+			dev_err(dev, "Failed to update rate denominator: %d\n", ret);
+			return ret;
+		}
+
+		ret = snd_soc_dai_set_bclk_ratio(rtd->codec_dai,
+						 channels * width);
+		if (ret) {
+			dev_err(dev, "Failed to set bclk ratio : %d\n", ret);
+			return ret;
+		}
+	}
+
+	return ret;
+}
+
 static void aif1_shutdown(struct snd_pcm_substream *substream)
 {
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
@@ -167,6 +260,7 @@ static void aif1_shutdown(struct snd_pcm_substream *substream)
 
 static const struct snd_soc_ops sof_pcm512x_ops = {
 	.startup = aif1_startup,
+	.hw_params = aif1_hw_params,
 	.shutdown = aif1_shutdown,
 };
 
-- 
2.20.1

Re: [RFC PATCH 05/16] ASoC: Intel: sof-pcm512x: reconfigure sclk in hw_params if needed

[Andy Shevchenko]

On Thu, Apr 09, 2020 at 02:58:30PM -0500, Pierre-Louis Bossart wrote:
> The SCLK is resumed by the codec driver. In case the rate specified in
> hw_params does not match the current configuration, disable, set the
> new rate and restart the clock.
> 
> There is no operation on hw_free, the codec suspend routine will
> disable/deprepare the clock.
> 
> Note that we don't change the DAI configuration when the DAC+ PRO is
> detected. All changes for the codec master mode are handled in the
> topology file (DAI configuration change and scheduling change)

...

> +	err = snd_interval_ratnum(hw_param_interval(params,
> +						    SNDRV_PCM_HW_PARAM_RATE),
> +				  1, &rats_no_pll, &num, &den);

> +	if (err >= 0 && den) {

Perhaps usual pattern, i.e.

	if (err < 0 || !den)
		return 0;
(so, above seems optional configuration)

	params...;
	return 0;

> +		params->rate_num = num;
> +		params->rate_den = den;
> +	}
> +
> +	return 0;

...

> +static int aif1_hw_params(struct snd_pcm_substream *substream,
> +			  struct snd_pcm_hw_params *params)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct sof_card_private *ctx = snd_soc_card_get_drvdata(rtd->card);
> +	struct device *dev = rtd->card->dev;
> +	int current_rate;
> +	int sclk_rate;
> +	int channels;
> +	int width;
> +	int rate;
> +	int ret = 0;
> +

> +	if (ctx->is_dac_pro) {

	if (!...)
		return 0;

...and drop the redundant ret assignment above.

> +		rate = params_rate(params);
> +		channels = params_channels(params);
> +		width = snd_pcm_format_physical_width(params_format(params));
> +
> +		if (rate % 24000)
> +			sclk_rate = 22579200;
> +		else
> +			sclk_rate = 24576000;
> +
> +		current_rate = clk_get_rate(ctx->sclk);
> +		if (current_rate != sclk_rate) {
> +			/*
> +			 * The sclk clock is started and stopped by the codec
> +			 * resume/suspend functions. If the rate isn't correct,
> +			 * stop, set the new rate and restart the clock
> +			 */
> +
> +			dev_dbg(dev, "reconfiguring SCLK to rate %d\n",
> +				sclk_rate);
> +
> +			clk_disable_unprepare(ctx->sclk);
> +
> +			ret = clk_set_rate(ctx->sclk, sclk_rate);
> +			if (ret) {
> +				dev_err(dev, "Could not set SCLK rate %d\n",
> +					sclk_rate);
> +				return ret;
> +			}
> +
> +			ret = clk_prepare_enable(ctx->sclk);
> +			if (ret) {
> +				dev_err(dev, "Failed to enable SCLK: %d\n",
> +					ret);
> +				return ret;
> +			}
> +		}
> +
> +		ret = aif1_update_rate_den(substream, params);
> +		if (ret) {
> +			dev_err(dev, "Failed to update rate denominator: %d\n", ret);
> +			return ret;
> +		}

Do you still need below steps when current_rate == sclk_rate?

> +		ret = snd_soc_dai_set_bclk_ratio(rtd->codec_dai,
> +						 channels * width);
> +		if (ret) {
> +			dev_err(dev, "Failed to set bclk ratio : %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 05/16] ASoC: Intel: sof-pcm512x: reconfigure sclk in hw_params if needed

[Pierre-Louis Bossart]

>> +	err = snd_interval_ratnum(hw_param_interval(params,
>> +						    SNDRV_PCM_HW_PARAM_RATE),
>> +				  1, &rats_no_pll, &num, &den);
> 
>> +	if (err >= 0 && den) {
> 
> Perhaps usual pattern, i.e.
> 
> 	if (err < 0 || !den)
> 		return 0;
> (so, above seems optional configuration)
> 
> 	params...;
> 	return 0;

ok

> 
>> +	if (ctx->is_dac_pro) {
> 
> 	if (!...)
> 		return 0;
> 
> ...and drop the redundant ret assignment above.

yes, this was suggested by Guennadi today as well.


>> +		ret = aif1_update_rate_den(substream, params);
>> +		if (ret) {
>> +			dev_err(dev, "Failed to update rate denominator: %d\n", ret);
>> +			return ret;
>> +		}
> 
> Do you still need below steps when current_rate == sclk_rate?

Good question. I assume the values are properly stored by the regmap 
cache, but if these channel and width do change (something we don't 
support for now) then yes we should move this out of the if case.

I'll give it a try, thanks for flagging this.

>> +		ret = snd_soc_dai_set_bclk_ratio(rtd->codec_dai,
>> +						 channels * width);
>> +		if (ret) {
>> +			dev_err(dev, "Failed to set bclk ratio : %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
> 

[RFC PATCH 06/16] ASoC: Intel: sof-pcm512x: select HIFIBERRY_DACPRO clk

[Pierre-Louis Bossart]

This configuration is needed to get the GPIO-controller clocks.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/boards/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig
index 556c3104e641..cad0af5b8b70 100644
--- a/sound/soc/intel/boards/Kconfig
+++ b/sound/soc/intel/boards/Kconfig
@@ -464,10 +464,12 @@ config SND_SOC_INTEL_SOF_RT5682_MACH
 config SND_SOC_INTEL_SOF_PCM512x_MACH
 	tristate "SOF with TI PCM512x codec"
 	depends on I2C && ACPI
+	depends on COMMON_CLK
 	depends on (SND_SOC_SOF_HDA_AUDIO_CODEC && (MFD_INTEL_LPSS || COMPILE_TEST)) ||\
 		   (SND_SOC_SOF_BAYTRAIL && (X86_INTEL_LPSS || COMPILE_TEST))
 	depends on SND_HDA_CODEC_HDMI
 	select SND_SOC_PCM512x_I2C
+	select COMMON_CLK_HIFIBERRY_DACPRO
 	help
 	  This adds support for ASoC machine driver for SOF platforms
 	  with TI PCM512x I2S audio codec.
-- 
2.20.1

[RFC PATCH 07/16] clk: hifiberry-dacpro: initial import

[Pierre-Louis Bossart]

From: Daniel Matuschek <daniel@hifiberry.com>

This patch imports the clock code from the Raspberry v5.5-y tree. The
ASoC machine driver initially present in this patch was dropped. The
comments are also dropped but all sign-offs are kept below. The patch
authorship was modified with explicit permission from Daniel Matuschek
to make sure it matches the Signed-off tag.

This patch generates a lot of checkpatch.pl warnings that are
corrected in follow-up patches.

Signed-off-by: DigitalDreamtime <clive.messer@digitaldreamtime.co.uk>
Signed-off-by: Daniel Matuschek <daniel@hifiberry.com>
Signed-off-by: Matthias Reichl <hias@horus.com>
Signed-off-by: Hui Wang <hui.wang@canonical.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/clk/Kconfig                |   3 +
 drivers/clk/Makefile               |   1 +
 drivers/clk/clk-hifiberry-dacpro.c | 160 +++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+)
 create mode 100644 drivers/clk/clk-hifiberry-dacpro.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index bcb257baed06..6bfffc99e3fd 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -70,6 +70,9 @@ config COMMON_CLK_HI655X
 	  multi-function device has one fixed-rate oscillator, clocked
 	  at 32KHz.
 
+config COMMON_CLK_HIFIBERRY_DACPRO
+	tristate
+
 config COMMON_CLK_SCMI
 	tristate "Clock driver controlled via SCMI interface"
 	depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index f4169cc2fd31..43ae7596de7b 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_MACH_ASPEED_G6)		+= clk-ast2600.o
 obj-$(CONFIG_ARCH_HIGHBANK)		+= clk-highbank.o
 obj-$(CONFIG_CLK_HSDK)			+= clk-hsdk-pll.o
 obj-$(CONFIG_COMMON_CLK_LOCHNAGAR)	+= clk-lochnagar.o
+obj-$(CONFIG_COMMON_CLK_HIFIBERRY_DACPRO)	+= clk-hifiberry-dacpro.o
 obj-$(CONFIG_COMMON_CLK_MAX77686)	+= clk-max77686.o
 obj-$(CONFIG_COMMON_CLK_MAX9485)	+= clk-max9485.o
 obj-$(CONFIG_ARCH_MILBEAUT_M10V)	+= clk-milbeaut.o
diff --git a/drivers/clk/clk-hifiberry-dacpro.c b/drivers/clk/clk-hifiberry-dacpro.c
new file mode 100644
index 000000000000..9e2634465823
--- /dev/null
+++ b/drivers/clk/clk-hifiberry-dacpro.c
@@ -0,0 +1,160 @@
+/*
+ * Clock Driver for HiFiBerry DAC Pro
+ *
+ * Author: Stuart MacLean
+ *         Copyright 2015
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+
+/* Clock rate of CLK44EN attached to GPIO6 pin */
+#define CLK_44EN_RATE 22579200UL
+/* Clock rate of CLK48EN attached to GPIO3 pin */
+#define CLK_48EN_RATE 24576000UL
+
+/**
+ * struct hifiberry_dacpro_clk - Common struct to the HiFiBerry DAC Pro
+ * @hw: clk_hw for the common clk framework
+ * @mode: 0 => CLK44EN, 1 => CLK48EN
+ */
+struct clk_hifiberry_hw {
+	struct clk_hw hw;
+	uint8_t mode;
+};
+
+#define to_hifiberry_clk(_hw) container_of(_hw, struct clk_hifiberry_hw, hw)
+
+static const struct of_device_id clk_hifiberry_dacpro_dt_ids[] = {
+	{ .compatible = "hifiberry,dacpro-clk",},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, clk_hifiberry_dacpro_dt_ids);
+
+static unsigned long clk_hifiberry_dacpro_recalc_rate(struct clk_hw *hw,
+	unsigned long parent_rate)
+{
+	return (to_hifiberry_clk(hw)->mode == 0) ? CLK_44EN_RATE :
+		CLK_48EN_RATE;
+}
+
+static long clk_hifiberry_dacpro_round_rate(struct clk_hw *hw,
+	unsigned long rate, unsigned long *parent_rate)
+{
+	long actual_rate;
+
+	if (rate <= CLK_44EN_RATE) {
+		actual_rate = (long)CLK_44EN_RATE;
+	} else if (rate >= CLK_48EN_RATE) {
+		actual_rate = (long)CLK_48EN_RATE;
+	} else {
+		long diff44Rate = (long)(rate - CLK_44EN_RATE);
+		long diff48Rate = (long)(CLK_48EN_RATE - rate);
+
+		if (diff44Rate < diff48Rate)
+			actual_rate = (long)CLK_44EN_RATE;
+		else
+			actual_rate = (long)CLK_48EN_RATE;
+	}
+	return actual_rate;
+}
+
+
+static int clk_hifiberry_dacpro_set_rate(struct clk_hw *hw,
+	unsigned long rate, unsigned long parent_rate)
+{
+	unsigned long actual_rate;
+	struct clk_hifiberry_hw *clk = to_hifiberry_clk(hw);
+
+	actual_rate = (unsigned long)clk_hifiberry_dacpro_round_rate(hw, rate,
+		&parent_rate);
+	clk->mode = (actual_rate == CLK_44EN_RATE) ? 0 : 1;
+	return 0;
+}
+
+
+const struct clk_ops clk_hifiberry_dacpro_rate_ops = {
+	.recalc_rate = clk_hifiberry_dacpro_recalc_rate,
+	.round_rate = clk_hifiberry_dacpro_round_rate,
+	.set_rate = clk_hifiberry_dacpro_set_rate,
+};
+
+static int clk_hifiberry_dacpro_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct clk_hifiberry_hw *proclk;
+	struct clk *clk;
+	struct device *dev;
+	struct clk_init_data init;
+
+	dev = &pdev->dev;
+
+	proclk = kzalloc(sizeof(struct clk_hifiberry_hw), GFP_KERNEL);
+	if (!proclk)
+		return -ENOMEM;
+
+	init.name = "clk-hifiberry-dacpro";
+	init.ops = &clk_hifiberry_dacpro_rate_ops;
+	init.flags = 0;
+	init.parent_names = NULL;
+	init.num_parents = 0;
+
+	proclk->mode = 0;
+	proclk->hw.init = &init;
+
+	clk = devm_clk_register(dev, &proclk->hw);
+	if (!IS_ERR(clk)) {
+		ret = of_clk_add_provider(dev->of_node, of_clk_src_simple_get,
+			clk);
+	} else {
+		dev_err(dev, "Fail to register clock driver\n");
+		kfree(proclk);
+		ret = PTR_ERR(clk);
+	}
+	return ret;
+}
+
+static int clk_hifiberry_dacpro_remove(struct platform_device *pdev)
+{
+	of_clk_del_provider(pdev->dev.of_node);
+	return 0;
+}
+
+static struct platform_driver clk_hifiberry_dacpro_driver = {
+	.probe = clk_hifiberry_dacpro_probe,
+	.remove = clk_hifiberry_dacpro_remove,
+	.driver = {
+		.name = "clk-hifiberry-dacpro",
+		.of_match_table = clk_hifiberry_dacpro_dt_ids,
+	},
+};
+
+static int __init clk_hifiberry_dacpro_init(void)
+{
+	return platform_driver_register(&clk_hifiberry_dacpro_driver);
+}
+core_initcall(clk_hifiberry_dacpro_init);
+
+static void __exit clk_hifiberry_dacpro_exit(void)
+{
+	platform_driver_unregister(&clk_hifiberry_dacpro_driver);
+}
+module_exit(clk_hifiberry_dacpro_exit);
+
+MODULE_DESCRIPTION("HiFiBerry DAC Pro clock driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:clk-hifiberry-dacpro");
-- 
2.20.1

Re: [RFC PATCH 07/16] clk: hifiberry-dacpro: initial import

[Andy Shevchenko]

On Thu, Apr 09, 2020 at 02:58:32PM -0500, Pierre-Louis Bossart wrote:
> From: Daniel Matuschek <daniel@hifiberry.com>
> 
> This patch imports the clock code from the Raspberry v5.5-y tree. The
> ASoC machine driver initially present in this patch was dropped. The
> comments are also dropped but all sign-offs are kept below. The patch
> authorship was modified with explicit permission from Daniel Matuschek
> to make sure it matches the Signed-off tag.
> 
> This patch generates a lot of checkpatch.pl warnings that are
> corrected in follow-up patches.

I guess it will be waste of time to review this part without squashing it first.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 07/16] clk: hifiberry-dacpro: initial import

[Pierre-Louis Bossart]

On 4/14/20 12:31 PM, Andy Shevchenko wrote:
> On Thu, Apr 09, 2020 at 02:58:32PM -0500, Pierre-Louis Bossart wrote:
>> From: Daniel Matuschek <daniel@hifiberry.com>
>>
>> This patch imports the clock code from the Raspberry v5.5-y tree. The
>> ASoC machine driver initially present in this patch was dropped. The
>> comments are also dropped but all sign-offs are kept below. The patch
>> authorship was modified with explicit permission from Daniel Matuschek
>> to make sure it matches the Signed-off tag.
>>
>> This patch generates a lot of checkpatch.pl warnings that are
>> corrected in follow-up patches.
> 
> I guess it will be waste of time to review this part without squashing it first.

I wasn't sure if squashing was desired, so kept all my changes separate. 
There are some changes from the legacy clk to the clk_hw parts plus 
introduction of ACPI support that are easier to review if kept separate. 
Maybe I should have squashed the cosmetic parts in patch8, and kept the 
functional changes as separate patches.

Re: [RFC PATCH 07/16] clk: hifiberry-dacpro: initial import

[Andy Shevchenko]

On Tue, Apr 14, 2020 at 01:09:28PM -0500, Pierre-Louis Bossart wrote:
> On 4/14/20 12:31 PM, Andy Shevchenko wrote:
> > On Thu, Apr 09, 2020 at 02:58:32PM -0500, Pierre-Louis Bossart wrote:
> > > From: Daniel Matuschek <daniel@hifiberry.com>
> > > 
> > > This patch imports the clock code from the Raspberry v5.5-y tree. The
> > > ASoC machine driver initially present in this patch was dropped. The
> > > comments are also dropped but all sign-offs are kept below. The patch
> > > authorship was modified with explicit permission from Daniel Matuschek
> > > to make sure it matches the Signed-off tag.
> > > 
> > > This patch generates a lot of checkpatch.pl warnings that are
> > > corrected in follow-up patches.
> > 
> > I guess it will be waste of time to review this part without squashing it first.
> 
> I wasn't sure if squashing was desired, so kept all my changes separate.

At the end, yes. It's a new contribution that can be at least cleaned before
hands to more-or-less acceptable point. With so many subtle issues it's not
good that we dump an ugly code outside of drivers/staging.

> There are some changes from the legacy clk to the clk_hw parts plus
> introduction of ACPI support that are easier to review if kept separate.

Yes, for review purposes it's okay.

You always can put your name as a SoB + Co-developed-by tag or give credits to
other people in the commit message differently (depending to amount of work
they do vs. yours).

> Maybe I should have squashed the cosmetic parts in patch8, and kept the
> functional changes as separate patches.

Use a common sense, you know your work better than me :-)
Just explain this in cover letter (like you do for this version).

-- 
With Best Regards,
Andy Shevchenko

[RFC PATCH 08/16] clk: hifiberry-dacpro: update SDPX/copyright

[Pierre-Louis Bossart]

Reformat license information and add Intel copyright

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/clk/clk-hifiberry-dacpro.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/clk-hifiberry-dacpro.c b/drivers/clk/clk-hifiberry-dacpro.c
index 9e2634465823..eb67a8c47c49 100644
--- a/drivers/clk/clk-hifiberry-dacpro.c
+++ b/drivers/clk/clk-hifiberry-dacpro.c
@@ -1,17 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
- * Clock Driver for HiFiBerry DAC Pro
- *
- * Author: Stuart MacLean
- *         Copyright 2015
+ * Copyright (c) 2015 Stuart MacLean
+ * Copyright (c) 2020 Intel Corporation
  *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * version 2 as published by the Free Software Foundation.
+ * Clock Driver for HiFiBerry DAC Pro
  *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * General Public License for more details.
  */
 
 #include <linux/clk-provider.h>
-- 
2.20.1

[RFC PATCH 09/16] clk: hifiberry-dacpro: style cleanups, use devm_

[Pierre-Louis Bossart]

Lots of small issues, xmas style, alignment, wrong comments, memory leak

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/clk/clk-hifiberry-dacpro.c | 42 +++++++++++-------------------
 1 file changed, 15 insertions(+), 27 deletions(-)

diff --git a/drivers/clk/clk-hifiberry-dacpro.c b/drivers/clk/clk-hifiberry-dacpro.c
index eb67a8c47c49..78ede325d237 100644
--- a/drivers/clk/clk-hifiberry-dacpro.c
+++ b/drivers/clk/clk-hifiberry-dacpro.c
@@ -21,13 +21,13 @@
 #define CLK_48EN_RATE 24576000UL
 
 /**
- * struct hifiberry_dacpro_clk - Common struct to the HiFiBerry DAC Pro
+ * struct clk_hifiberry_hw - Common struct to the HiFiBerry DAC Pro
  * @hw: clk_hw for the common clk framework
  * @mode: 0 => CLK44EN, 1 => CLK48EN
  */
 struct clk_hifiberry_hw {
 	struct clk_hw hw;
-	uint8_t mode;
+	u8 mode;
 };
 
 #define to_hifiberry_clk(_hw) container_of(_hw, struct clk_hifiberry_hw, hw)
@@ -39,14 +39,15 @@ static const struct of_device_id clk_hifiberry_dacpro_dt_ids[] = {
 MODULE_DEVICE_TABLE(of, clk_hifiberry_dacpro_dt_ids);
 
 static unsigned long clk_hifiberry_dacpro_recalc_rate(struct clk_hw *hw,
-	unsigned long parent_rate)
+						      unsigned long parent_rate)
 {
 	return (to_hifiberry_clk(hw)->mode == 0) ? CLK_44EN_RATE :
 		CLK_48EN_RATE;
 }
 
 static long clk_hifiberry_dacpro_round_rate(struct clk_hw *hw,
-	unsigned long rate, unsigned long *parent_rate)
+					    unsigned long rate,
+					    unsigned long *parent_rate)
 {
 	long actual_rate;
 
@@ -66,21 +67,20 @@ static long clk_hifiberry_dacpro_round_rate(struct clk_hw *hw,
 	return actual_rate;
 }
 
-
 static int clk_hifiberry_dacpro_set_rate(struct clk_hw *hw,
-	unsigned long rate, unsigned long parent_rate)
+					 unsigned long rate,
+					 unsigned long parent_rate)
 {
-	unsigned long actual_rate;
 	struct clk_hifiberry_hw *clk = to_hifiberry_clk(hw);
+	unsigned long actual_rate;
 
 	actual_rate = (unsigned long)clk_hifiberry_dacpro_round_rate(hw, rate,
-		&parent_rate);
+								&parent_rate);
 	clk->mode = (actual_rate == CLK_44EN_RATE) ? 0 : 1;
 	return 0;
 }
 
-
-const struct clk_ops clk_hifiberry_dacpro_rate_ops = {
+static const struct clk_ops clk_hifiberry_dacpro_rate_ops = {
 	.recalc_rate = clk_hifiberry_dacpro_recalc_rate,
 	.round_rate = clk_hifiberry_dacpro_round_rate,
 	.set_rate = clk_hifiberry_dacpro_set_rate,
@@ -88,15 +88,15 @@ const struct clk_ops clk_hifiberry_dacpro_rate_ops = {
 
 static int clk_hifiberry_dacpro_probe(struct platform_device *pdev)
 {
-	int ret;
 	struct clk_hifiberry_hw *proclk;
-	struct clk *clk;
-	struct device *dev;
 	struct clk_init_data init;
+	struct device *dev;
+	struct clk *clk;
+	int ret;
 
 	dev = &pdev->dev;
 
-	proclk = kzalloc(sizeof(struct clk_hifiberry_hw), GFP_KERNEL);
+	proclk = devm_kzalloc(dev, sizeof(*proclk), GFP_KERNEL);
 	if (!proclk)
 		return -ENOMEM;
 
@@ -115,7 +115,6 @@ static int clk_hifiberry_dacpro_probe(struct platform_device *pdev)
 			clk);
 	} else {
 		dev_err(dev, "Fail to register clock driver\n");
-		kfree(proclk);
 		ret = PTR_ERR(clk);
 	}
 	return ret;
@@ -135,18 +134,7 @@ static struct platform_driver clk_hifiberry_dacpro_driver = {
 		.of_match_table = clk_hifiberry_dacpro_dt_ids,
 	},
 };
-
-static int __init clk_hifiberry_dacpro_init(void)
-{
-	return platform_driver_register(&clk_hifiberry_dacpro_driver);
-}
-core_initcall(clk_hifiberry_dacpro_init);
-
-static void __exit clk_hifiberry_dacpro_exit(void)
-{
-	platform_driver_unregister(&clk_hifiberry_dacpro_driver);
-}
-module_exit(clk_hifiberry_dacpro_exit);
+module_platform_driver(clk_hifiberry_dacpro_driver);
 
 MODULE_DESCRIPTION("HiFiBerry DAC Pro clock driver");
 MODULE_LICENSE("GPL v2");
-- 
2.20.1

[RFC PATCH 10/16] clk: hifiberry-dacpro: add OF dependency

[Pierre-Louis Bossart]

Make sure OF is enabled, in case ACPI platforms use OF matching with
PRP0001 and .compatible string

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/clk/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 6bfffc99e3fd..5b9f829d84fe 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -72,6 +72,7 @@ config COMMON_CLK_HI655X
 
 config COMMON_CLK_HIFIBERRY_DACPRO
 	tristate
+	depends on OF
 
 config COMMON_CLK_SCMI
 	tristate "Clock driver controlled via SCMI interface"
-- 
2.20.1

[RFC PATCH 11/16] clk: hifiberry-dacpro: transition to _hw functions

[Pierre-Louis Bossart]

devm_clk_register() and of_clk_add_provider() are deprecated, use the
recommended functions.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/clk/clk-hifiberry-dacpro.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk-hifiberry-dacpro.c b/drivers/clk/clk-hifiberry-dacpro.c
index 78ede325d237..bf0616c959da 100644
--- a/drivers/clk/clk-hifiberry-dacpro.c
+++ b/drivers/clk/clk-hifiberry-dacpro.c
@@ -91,7 +91,6 @@ static int clk_hifiberry_dacpro_probe(struct platform_device *pdev)
 	struct clk_hifiberry_hw *proclk;
 	struct clk_init_data init;
 	struct device *dev;
-	struct clk *clk;
 	int ret;
 
 	dev = &pdev->dev;
@@ -109,14 +108,15 @@ static int clk_hifiberry_dacpro_probe(struct platform_device *pdev)
 	proclk->mode = 0;
 	proclk->hw.init = &init;
 
-	clk = devm_clk_register(dev, &proclk->hw);
-	if (!IS_ERR(clk)) {
-		ret = of_clk_add_provider(dev->of_node, of_clk_src_simple_get,
-			clk);
-	} else {
+	ret = devm_clk_hw_register(dev, &proclk->hw);
+	if (ret) {
 		dev_err(dev, "Fail to register clock driver\n");
-		ret = PTR_ERR(clk);
+		return ret;
 	}
+
+	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get,
+				     &proclk->hw);
+
 	return ret;
 }
 
-- 
2.20.1

[RFC PATCH 12/16] clk: hifiberry-dacpro: add ACPI support

[Pierre-Louis Bossart]

On ACPI platforms the of_ functions are irrelevant, conditionally
compile them out and add devm_clk_hw_register_clkdev() call instead.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/clk/clk-hifiberry-dacpro.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/clk/clk-hifiberry-dacpro.c b/drivers/clk/clk-hifiberry-dacpro.c
index bf0616c959da..d01a90fed51b 100644
--- a/drivers/clk/clk-hifiberry-dacpro.c
+++ b/drivers/clk/clk-hifiberry-dacpro.c
@@ -114,15 +114,22 @@ static int clk_hifiberry_dacpro_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+#ifndef CONFIG_ACPI
 	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get,
 				     &proclk->hw);
+#else
+	ret = devm_clk_hw_register_clkdev(dev, &proclk->hw,
+					  init.name, NULL);
+#endif
 
 	return ret;
 }
 
 static int clk_hifiberry_dacpro_remove(struct platform_device *pdev)
 {
+#ifndef CONFIG_ACPI
 	of_clk_del_provider(pdev->dev.of_node);
+#endif
 	return 0;
 }
 
-- 
2.20.1

Re: [RFC PATCH 12/16] clk: hifiberry-dacpro: add ACPI support

[Stephen Boyd]

Quoting Pierre-Louis Bossart (2020-04-09 12:58:37)
> On ACPI platforms the of_ functions are irrelevant, conditionally
> compile them out and add devm_clk_hw_register_clkdev() call instead.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/clk/clk-hifiberry-dacpro.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/clk/clk-hifiberry-dacpro.c b/drivers/clk/clk-hifiberry-dacpro.c
> index bf0616c959da..d01a90fed51b 100644
> --- a/drivers/clk/clk-hifiberry-dacpro.c
> +++ b/drivers/clk/clk-hifiberry-dacpro.c
> @@ -114,15 +114,22 @@ static int clk_hifiberry_dacpro_probe(struct platform_device *pdev)
>                 return ret;
>         }
>  
> +#ifndef CONFIG_ACPI

Use if (!IS_ENABLED(CONFIG_ACPI)) instead?

>         ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get,
>                                      &proclk->hw);
> +#else
> +       ret = devm_clk_hw_register_clkdev(dev, &proclk->hw,
> +                                         init.name, NULL);
> +#endif
>  
>         return ret;
>  }
>  
>  static int clk_hifiberry_dacpro_remove(struct platform_device *pdev)
>  {
> +#ifndef CONFIG_ACPI
>         of_clk_del_provider(pdev->dev.of_node);
> +#endif

Re: [RFC PATCH 12/16] clk: hifiberry-dacpro: add ACPI support

[Andy Shevchenko]

On Wed, Apr 22, 2020 at 02:32:15AM -0700, Stephen Boyd wrote:
> Quoting Pierre-Louis Bossart (2020-04-09 12:58:37)
> > On ACPI platforms the of_ functions are irrelevant, conditionally
> > compile them out and add devm_clk_hw_register_clkdev() call instead.

...

> Use if (!IS_ENABLED(CONFIG_ACPI)) instead?
> 
> >         ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get,
> >                                      &proclk->hw);

I'm rather wondering if we have OF stuff integrated properly to CLK framework
to avoid first branch completely.

> > +#else
> > +       ret = devm_clk_hw_register_clkdev(dev, &proclk->hw,
> > +                                         init.name, NULL);
> > +#endif

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 12/16] clk: hifiberry-dacpro: add ACPI support

[Pierre-Louis Bossart]

On 4/22/20 4:32 AM, Stephen Boyd wrote:
> Quoting Pierre-Louis Bossart (2020-04-09 12:58:37)
>> On ACPI platforms the of_ functions are irrelevant, conditionally
>> compile them out and add devm_clk_hw_register_clkdev() call instead.
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   drivers/clk/clk-hifiberry-dacpro.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/clk/clk-hifiberry-dacpro.c b/drivers/clk/clk-hifiberry-dacpro.c
>> index bf0616c959da..d01a90fed51b 100644
>> --- a/drivers/clk/clk-hifiberry-dacpro.c
>> +++ b/drivers/clk/clk-hifiberry-dacpro.c
>> @@ -114,15 +114,22 @@ static int clk_hifiberry_dacpro_probe(struct platform_device *pdev)
>>                  return ret;
>>          }
>>   
>> +#ifndef CONFIG_ACPI
> 
> Use if (!IS_ENABLED(CONFIG_ACPI)) instead?

git grep CONFIG_ACPI shows most of the kernel code uses #if(n)def 
CONFIG_ACPI. It's equivalent, it's a boolean.

Re: [RFC PATCH 12/16] clk: hifiberry-dacpro: add ACPI support

[Stephen Boyd]

Quoting Pierre-Louis Bossart (2020-04-22 02:54:38)
> 
> 
> On 4/22/20 4:32 AM, Stephen Boyd wrote:
> > Quoting Pierre-Louis Bossart (2020-04-09 12:58:37)
> >> On ACPI platforms the of_ functions are irrelevant, conditionally
> >> compile them out and add devm_clk_hw_register_clkdev() call instead.
> >>
> >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >> ---
> >>   drivers/clk/clk-hifiberry-dacpro.c | 7 +++++++
> >>   1 file changed, 7 insertions(+)
> >>
> >> diff --git a/drivers/clk/clk-hifiberry-dacpro.c b/drivers/clk/clk-hifiberry-dacpro.c
> >> index bf0616c959da..d01a90fed51b 100644
> >> --- a/drivers/clk/clk-hifiberry-dacpro.c
> >> +++ b/drivers/clk/clk-hifiberry-dacpro.c
> >> @@ -114,15 +114,22 @@ static int clk_hifiberry_dacpro_probe(struct platform_device *pdev)
> >>                  return ret;
> >>          }
> >>   
> >> +#ifndef CONFIG_ACPI
> > 
> > Use if (!IS_ENABLED(CONFIG_ACPI)) instead?
> 
> git grep CONFIG_ACPI shows most of the kernel code uses #if(n)def 
> CONFIG_ACPI. It's equivalent, it's a boolean.

It's not equivalent. It is a pre-processor directive vs. an if statement
that evaluates to 0 or 1 and lets the compiler see both sides of the
code to check types.

Re: [RFC PATCH 12/16] clk: hifiberry-dacpro: add ACPI support

[Pierre-Louis Bossart]

On 4/22/20 3:52 PM, Stephen Boyd wrote:
> Quoting Pierre-Louis Bossart (2020-04-22 02:54:38)
>>
>>
>> On 4/22/20 4:32 AM, Stephen Boyd wrote:
>>> Quoting Pierre-Louis Bossart (2020-04-09 12:58:37)
>>>> On ACPI platforms the of_ functions are irrelevant, conditionally
>>>> compile them out and add devm_clk_hw_register_clkdev() call instead.
>>>>
>>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>>> ---
>>>>    drivers/clk/clk-hifiberry-dacpro.c | 7 +++++++
>>>>    1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/clk-hifiberry-dacpro.c b/drivers/clk/clk-hifiberry-dacpro.c
>>>> index bf0616c959da..d01a90fed51b 100644
>>>> --- a/drivers/clk/clk-hifiberry-dacpro.c
>>>> +++ b/drivers/clk/clk-hifiberry-dacpro.c
>>>> @@ -114,15 +114,22 @@ static int clk_hifiberry_dacpro_probe(struct platform_device *pdev)
>>>>                   return ret;
>>>>           }
>>>>    
>>>> +#ifndef CONFIG_ACPI
>>>
>>> Use if (!IS_ENABLED(CONFIG_ACPI)) instead?
>>
>> git grep CONFIG_ACPI shows most of the kernel code uses #if(n)def
>> CONFIG_ACPI. It's equivalent, it's a boolean.
> 
> It's not equivalent. It is a pre-processor directive vs. an if statement
> that evaluates to 0 or 1 and lets the compiler see both sides of the
> code to check types.

Ah, yes I misunderstood your point.

[RFC PATCH 13/16] clk: hifiberry-dacpro: add "sclk" lookup

[Pierre-Louis Bossart]

devm_clk_get() fails on ACPI platforms when a NULL string is used.
Create a "sclk" lookup to make sure codec and machine drivers can get
the clock.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/clk/clk-hifiberry-dacpro.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/clk/clk-hifiberry-dacpro.c b/drivers/clk/clk-hifiberry-dacpro.c
index d01a90fed51b..36210f52c624 100644
--- a/drivers/clk/clk-hifiberry-dacpro.c
+++ b/drivers/clk/clk-hifiberry-dacpro.c
@@ -24,10 +24,12 @@
  * struct clk_hifiberry_hw - Common struct to the HiFiBerry DAC Pro
  * @hw: clk_hw for the common clk framework
  * @mode: 0 => CLK44EN, 1 => CLK48EN
+ * @sclk_lookup: handle for "sclk"
  */
 struct clk_hifiberry_hw {
 	struct clk_hw hw;
 	u8 mode;
+	struct clk_lookup *sclk_lookup;
 };
 
 #define to_hifiberry_clk(_hw) container_of(_hw, struct clk_hifiberry_hw, hw)
@@ -121,15 +123,34 @@ static int clk_hifiberry_dacpro_probe(struct platform_device *pdev)
 	ret = devm_clk_hw_register_clkdev(dev, &proclk->hw,
 					  init.name, NULL);
 #endif
+	if (ret) {
+		dev_err(dev, "Fail to add clock driver\n");
+		return ret;
+	}
+
+	proclk->sclk_lookup = clkdev_hw_create(&proclk->hw, "sclk", NULL);
+	if (!proclk->sclk_lookup) {
+#ifndef CONFIG_ACPI
+		of_clk_del_provider(dev->of_node);
+#endif
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(pdev, proclk);
 
 	return ret;
 }
 
 static int clk_hifiberry_dacpro_remove(struct platform_device *pdev)
 {
+	struct clk_hifiberry_hw *proclk = platform_get_drvdata(pdev);
+
+	clkdev_drop(proclk->sclk_lookup);
+
 #ifndef CONFIG_ACPI
 	of_clk_del_provider(pdev->dev.of_node);
 #endif
+
 	return 0;
 }
 
-- 
2.20.1

Re: [RFC PATCH 13/16] clk: hifiberry-dacpro: add "sclk" lookup

[Stephen Boyd]

Quoting Pierre-Louis Bossart (2020-04-09 12:58:38)
> devm_clk_get() fails on ACPI platforms when a NULL string is used.
> Create a "sclk" lookup to make sure codec and machine drivers can get
> the clock.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/clk/clk-hifiberry-dacpro.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/clk/clk-hifiberry-dacpro.c b/drivers/clk/clk-hifiberry-dacpro.c
> index d01a90fed51b..36210f52c624 100644
> --- a/drivers/clk/clk-hifiberry-dacpro.c
> +++ b/drivers/clk/clk-hifiberry-dacpro.c
> @@ -24,10 +24,12 @@
>   * struct clk_hifiberry_hw - Common struct to the HiFiBerry DAC Pro
>   * @hw: clk_hw for the common clk framework
>   * @mode: 0 => CLK44EN, 1 => CLK48EN
> + * @sclk_lookup: handle for "sclk"
>   */
>  struct clk_hifiberry_hw {
>         struct clk_hw hw;
>         u8 mode;
> +       struct clk_lookup *sclk_lookup;
>  };
>  
>  #define to_hifiberry_clk(_hw) container_of(_hw, struct clk_hifiberry_hw, hw)
> @@ -121,15 +123,34 @@ static int clk_hifiberry_dacpro_probe(struct platform_device *pdev)
>         ret = devm_clk_hw_register_clkdev(dev, &proclk->hw,
>                                           init.name, NULL);
>  #endif
> +       if (ret) {
> +               dev_err(dev, "Fail to add clock driver\n");
> +               return ret;
> +       }
> +
> +       proclk->sclk_lookup = clkdev_hw_create(&proclk->hw, "sclk", NULL);
> +       if (!proclk->sclk_lookup) {
> +#ifndef CONFIG_ACPI

Is it to save code space? Otherwise the ifdefs are pretty ugly and I'd
prefer we just call of_clk APIs and rely on the inline stubs when
CONFIG_OF isn't enabled to be optimized out.

> +               of_clk_del_provider(dev->of_node);
> +#endif
> +               return -ENOMEM;
> +       }
> +

Re: [RFC PATCH 13/16] clk: hifiberry-dacpro: add "sclk" lookup

[Pierre-Louis Bossart]

>> +       proclk->sclk_lookup = clkdev_hw_create(&proclk->hw, "sclk", NULL);
>> +       if (!proclk->sclk_lookup) {
>> +#ifndef CONFIG_ACPI
> 
> Is it to save code space? Otherwise the ifdefs are pretty ugly and I'd
> prefer we just call of_clk APIs and rely on the inline stubs when
> CONFIG_OF isn't enabled to be optimized out.

CONFIG_OF was added as a dependency (see patch 10/16) so that we can use 
the 'compatible' string to probe w/ the PRP0001 device.

I must admit I don't know what these functions do so I just filtered 
them out in the ACPI case.

>> +               of_clk_del_provider(dev->of_node);
>> +#endif
>> +               return -ENOMEM;
>> +       }
>> +

Re: [RFC PATCH 13/16] clk: hifiberry-dacpro: add "sclk" lookup

[Andy Shevchenko]

On Wed, Apr 22, 2020 at 04:51:52AM -0500, Pierre-Louis Bossart wrote:
> > > +       proclk->sclk_lookup = clkdev_hw_create(&proclk->hw, "sclk", NULL);
> > > +       if (!proclk->sclk_lookup) {
> > > +#ifndef CONFIG_ACPI
> > 
> > Is it to save code space? Otherwise the ifdefs are pretty ugly and I'd
> > prefer we just call of_clk APIs and rely on the inline stubs when
> > CONFIG_OF isn't enabled to be optimized out.
> 
> CONFIG_OF was added as a dependency (see patch 10/16) so that we can use the
> 'compatible' string to probe w/ the PRP0001 device.

PRP0001 does not require CONFIG_OF to be set.

> I must admit I don't know what these functions do so I just filtered them
> out in the ACPI case.

I think you have to check if one is a superposition of the other.

> > > +               of_clk_del_provider(dev->of_node);
> > > +#endif
> > > +               return -ENOMEM;
> > > +       }

-- 
With Best Regards,
Andy Shevchenko

[RFC PATCH 14/16] clk: hifiberry-dacpro: toggle GPIOs on prepare/unprepare

[Pierre-Louis Bossart]

Now that the PCM512x driver exposes GPIOs, we can set their values as
needed in this clk driver (instead of doing nothing).

This clk driver does not have access to the codec regmap, so it only
toggles GPIOs. The user (typically a machine driver) should verify
that the clocks are present by testing the PCM512x_RATE4_DET register
(reports if the sclk is seen by the codec).

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/clk/clk-hifiberry-dacpro.c | 110 +++++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/drivers/clk/clk-hifiberry-dacpro.c b/drivers/clk/clk-hifiberry-dacpro.c
index 36210f52c624..f1f5af260083 100644
--- a/drivers/clk/clk-hifiberry-dacpro.c
+++ b/drivers/clk/clk-hifiberry-dacpro.c
@@ -9,6 +9,8 @@
 
 #include <linux/clk-provider.h>
 #include <linux/clkdev.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/machine.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -20,16 +22,35 @@
 /* Clock rate of CLK48EN attached to GPIO3 pin */
 #define CLK_48EN_RATE 24576000UL
 
+static struct gpiod_lookup_table pcm512x_gpios_table = {
+	/* .dev_id set during probe */
+	.table = {
+		GPIO_LOOKUP("pcm512x-gpio", 2, "PCM512x-GPIO3", GPIO_ACTIVE_HIGH),
+		GPIO_LOOKUP("pcm512x-gpio", 5, "PCM512x-GPIO6", GPIO_ACTIVE_HIGH),
+		{ },
+	},
+};
+
 /**
  * struct clk_hifiberry_hw - Common struct to the HiFiBerry DAC Pro
+ * @dev: device
  * @hw: clk_hw for the common clk framework
  * @mode: 0 => CLK44EN, 1 => CLK48EN
  * @sclk_lookup: handle for "sclk"
+ * @gpio_44: gpiod desc for 44.1kHz support
+ * @gpio_48: gpiod desc for 48 kHz support
+ * @prepared: boolean caching clock state
+ * @gpio_initialized: boolean flag used to take gpio references.
  */
 struct clk_hifiberry_hw {
+	struct device *dev;
 	struct clk_hw hw;
 	u8 mode;
 	struct clk_lookup *sclk_lookup;
+	struct gpio_desc *gpio_44;
+	struct gpio_desc *gpio_48;
+	bool prepared;
+	bool gpio_initialized;
 };
 
 #define to_hifiberry_clk(_hw) container_of(_hw, struct clk_hifiberry_hw, hw)
@@ -69,6 +90,88 @@ static long clk_hifiberry_dacpro_round_rate(struct clk_hw *hw,
 	return actual_rate;
 }
 
+static int clk_hifiberry_dacpro_is_prepared(struct clk_hw *hw)
+{
+	struct clk_hifiberry_hw *clk = to_hifiberry_clk(hw);
+
+	return clk->prepared;
+}
+
+static int clk_hifiberry_dacpro_prepare(struct clk_hw *hw)
+{
+	struct clk_hifiberry_hw *clk = to_hifiberry_clk(hw);
+
+	/*
+	 * The gpios are handled here to avoid any dependencies on
+	 * probe.
+	 *
+	 * The user of the clock should verify with the PCM512
+	 * registers that the clock are actually present and stable.
+	 * This driver only toggles the relevant GPIOs.
+	 */
+	if (!clk->gpio_initialized) {
+
+		clk->gpio_44 = devm_gpiod_get(clk->dev,
+					      "PCM512x-GPIO6",
+					      GPIOD_OUT_LOW);
+		if (IS_ERR(clk->gpio_44)) {
+			dev_err(clk->dev, "gpio44 not found\n");
+			return PTR_ERR(clk->gpio_44);
+		}
+
+		clk->gpio_48 = devm_gpiod_get(clk->dev,
+					      "PCM512x-GPIO3",
+					      GPIOD_OUT_LOW);
+		if (IS_ERR(clk->gpio_48)) {
+			dev_err(clk->dev, "gpio48 not found\n");
+			return PTR_ERR(clk->gpio_48);
+		}
+
+		clk->gpio_initialized = true;
+	}
+
+	if (clk->prepared)
+		return 0;
+
+	switch (clk->mode) {
+	case 0:
+		/* 44.1 kHz */
+		gpiod_set_value(clk->gpio_44, 1);
+		break;
+	case 1:
+		/* 48 kHz */
+		gpiod_set_value(clk->gpio_48, 1);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	clk->prepared = 1;
+
+	return 0;
+}
+
+static void clk_hifiberry_dacpro_unprepare(struct clk_hw *hw)
+{
+	struct clk_hifiberry_hw *clk = to_hifiberry_clk(hw);
+
+	if (!clk->prepared)
+		return;
+
+	switch (clk->mode) {
+	case 0:
+		gpiod_set_value(clk->gpio_44, 0);
+		break;
+	case 1:
+		gpiod_set_value(clk->gpio_48, 0);
+		break;
+	default:
+		return;
+	}
+
+	clk->prepared = false;
+}
+
 static int clk_hifiberry_dacpro_set_rate(struct clk_hw *hw,
 					 unsigned long rate,
 					 unsigned long parent_rate)
@@ -83,6 +186,9 @@ static int clk_hifiberry_dacpro_set_rate(struct clk_hw *hw,
 }
 
 static const struct clk_ops clk_hifiberry_dacpro_rate_ops = {
+	.is_prepared = clk_hifiberry_dacpro_is_prepared,
+	.prepare = clk_hifiberry_dacpro_prepare,
+	.unprepare = clk_hifiberry_dacpro_unprepare,
 	.recalc_rate = clk_hifiberry_dacpro_recalc_rate,
 	.round_rate = clk_hifiberry_dacpro_round_rate,
 	.set_rate = clk_hifiberry_dacpro_set_rate,
@@ -97,6 +203,9 @@ static int clk_hifiberry_dacpro_probe(struct platform_device *pdev)
 
 	dev = &pdev->dev;
 
+	pcm512x_gpios_table.dev_id = dev_name(dev);
+	gpiod_add_lookup_table(&pcm512x_gpios_table);
+
 	proclk = devm_kzalloc(dev, sizeof(*proclk), GFP_KERNEL);
 	if (!proclk)
 		return -ENOMEM;
@@ -107,6 +216,7 @@ static int clk_hifiberry_dacpro_probe(struct platform_device *pdev)
 	init.parent_names = NULL;
 	init.num_parents = 0;
 
+	proclk->dev = dev;
 	proclk->mode = 0;
 	proclk->hw.init = &init;
 
-- 
2.20.1

[RFC PATCH 15/16] clk: hifiberry-dacpro: add delay on clock prepare/deprepare

[Pierre-Louis Bossart]

Add a delay to make sure the PCM512x codec can detect SCLK presence.

The initial code from the Raspberry tree used msleep(2), which can be
up to 20ms. A delay of 5-10ms seems fine in practice.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/clk/clk-hifiberry-dacpro.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/clk/clk-hifiberry-dacpro.c b/drivers/clk/clk-hifiberry-dacpro.c
index f1f5af260083..2907b203fcf2 100644
--- a/drivers/clk/clk-hifiberry-dacpro.c
+++ b/drivers/clk/clk-hifiberry-dacpro.c
@@ -9,6 +9,7 @@
 
 #include <linux/clk-provider.h>
 #include <linux/clkdev.h>
+#include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/machine.h>
 #include <linux/kernel.h>
@@ -145,6 +146,8 @@ static int clk_hifiberry_dacpro_prepare(struct clk_hw *hw)
 	default:
 		return -EINVAL;
 	}
+	/* wait for SCLK update to be detected by PCM512x codec */
+	usleep_range(5000, 10000);
 
 	clk->prepared = 1;
 
@@ -168,6 +171,8 @@ static void clk_hifiberry_dacpro_unprepare(struct clk_hw *hw)
 	default:
 		return;
 	}
+	/* wait for SCLK update to be detected by PCM512x codec */
+	usleep_range(5000, 10000);
 
 	clk->prepared = false;
 }
-- 
2.20.1

[RFC PATCH 16/16] ASoC: dt-bindings: add document for Hifiberry DAC+ PRO clock

[Pierre-Louis Bossart]

The Hifiberry DAC+ PRO relies on two local audio oscillators exposed
with the clock framework.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 .../bindings/sound/hifiberry-dacpro.yaml      | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/hifiberry-dacpro.yaml

diff --git a/Documentation/devicetree/bindings/sound/hifiberry-dacpro.yaml b/Documentation/devicetree/bindings/sound/hifiberry-dacpro.yaml
new file mode 100644
index 000000000000..9305a1a0ccd7
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/hifiberry-dacpro.yaml
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/hifiberry-dacpro.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Hifiberry DAC+ Pro clock driver
+
+maintainers:
+  - Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
+
+description: |
+  The Hifiberry DAC+ PRO provides two oscillators for enhanced audio
+  quality. The clk driver allow for select and configuration of the
+  clock source.
+
+properties:
+  "#clock-cells":
+    const: 0
+
+  compatible:
+    items:
+      - const: hifiberry,dacpro-clk
+  reg:
+    maxItems: 1
+
+required:
+  - "#clock-cells"
+  - compatible
+
+examples:
+  - |
+    dacpro_osc: dacpro_osc {
+        compatible = "hifiberry,dacpro-clk";
+        #clock-cells = <0>;
+    };
+
+...
-- 
2.20.1

Re: [RFC PATCH 16/16] ASoC: dt-bindings: add document for Hifiberry DAC+ PRO clock

[Andy Shevchenko]

On Thu, Apr 09, 2020 at 02:58:41PM -0500, Pierre-Louis Bossart wrote:
> The Hifiberry DAC+ PRO relies on two local audio oscillators exposed
> with the clock framework.

> +# SPDX-License-Identifier: GPL-2.0

Dual license requirement nowadays.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 16/16] ASoC: dt-bindings: add document for Hifiberry DAC+ PRO clock

[Pierre-Louis Bossart]

On 4/14/20 12:27 PM, Andy Shevchenko wrote:
> On Thu, Apr 09, 2020 at 02:58:41PM -0500, Pierre-Louis Bossart wrote:
>> The Hifiberry DAC+ PRO relies on two local audio oscillators exposed
>> with the clock framework.
> 
>> +# SPDX-License-Identifier: GPL-2.0
> 
> Dual license requirement nowadays.

Darn, yes, I should know better. thanks for flagging this.

Re: [RFC PATCH 00/16] ASoC/SOF/clk/gpio/dt: add Hifiberry DAC+ PRO support

[Andy Shevchenko]

On Thu, Apr 09, 2020 at 02:58:25PM -0500, Pierre-Louis Bossart wrote:
> The Hifiberry DAC+ / DAC+ PRO is supported in the Raspberry PI tree
> but until now not in the mainline and not for ACPI platforms.
> 
> This patchset implements the recommendations suggested by Mark Brown
> back in 2018: first add a gpiochip in the PCM512x codec driver, then
> use these gpios from a clock driver and the machine driver.
> 
> Since this patchset relies on different subsystems, sending as RFC for
> now. I chose to import the original code from the Raspberry PI tree as
> is,

I don't see briefly what they are, any pointers like patch numbers in the
series?

>	and add my changes on top. If there is a preference to squash the
> changes that'd be fine.

I guess it would be good to have.

> I also don't know if I should split this
> series in two, one for ASoC and one for clk changes?
> 
> This patchset does not add changes to the sound/soc/bcm machine
> drivers, but that should be trivial once all the gpio/clock is
> available.
> 
> Thanks to Andy Shevchenko for his help navigating the gpio subsystem
> and flagging mistakes in the use of lookup tables, and to Rob Herring
> for pointers on the DT bindings verification tools.

You are welcome!
I'm going to review them (where I understand something) as they are presented.

> Daniel Matuschek (1):
>   clk: hifiberry-dacpro: initial import
> 
> Pierre-Louis Bossart (15):
>   ASoC: pcm512x: expose 6 GPIOs
>   ASoC: pcm512x: use "sclk" string to retrieve clock
>   ASoC: Intel: sof-pcm512x: use gpiod for LED
>   ASoC: Intel: sof-pcm512x: detect Hifiberry DAC+ PRO
>   ASoC: Intel: sof-pcm512x: reconfigure sclk in hw_params if needed
>   ASoC: Intel: sof-pcm512x: select HIFIBERRY_DACPRO clk
>   clk: hifiberry-dacpro: update SDPX/copyright
>   clk: hifiberry-dacpro: style cleanups, use devm_
>   clk: hifiberry-dacpro: add OF dependency
>   clk: hifiberry-dacpro: transition to _hw functions
>   clk: hifiberry-dacpro: add ACPI support
>   clk: hifiberry-dacpro: add "sclk" lookup
>   clk: hifiberry-dacpro: toggle GPIOs on prepare/unprepare
>   clk: hifiberry-dacpro: add delay on clock prepare/deprepare
>   ASoC: dt-bindings: add document for Hifiberry DAC+ PRO clock
> 
>  .../bindings/sound/hifiberry-dacpro.yaml      |  38 +++
>  drivers/clk/Kconfig                           |   4 +
>  drivers/clk/Makefile                          |   1 +
>  drivers/clk/clk-hifiberry-dacpro.c            | 284 ++++++++++++++++++
>  sound/soc/codecs/pcm512x.c                    | 134 ++++++++-
>  sound/soc/intel/boards/Kconfig                |   2 +
>  sound/soc/intel/boards/sof_pcm512x.c          | 190 +++++++++++-
>  7 files changed, 635 insertions(+), 18 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/sound/hifiberry-dacpro.yaml
>  create mode 100644 drivers/clk/clk-hifiberry-dacpro.c
> 
> 
> base-commit: dd8e871d4e560eeb8d22af82dde91457ad835a63
> -- 
> 2.20.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 00/16] ASoC/SOF/clk/gpio/dt: add Hifiberry DAC+ PRO support

[Pierre-Louis Bossart]

>> Since this patchset relies on different subsystems, sending as RFC for
>> now. I chose to import the original code from the Raspberry PI tree as
>> is,
> 
> I don't see briefly what they are, any pointers like patch numbers in the
> series?

The prefix is the key, I should have added the following in the cover 
letter:

ASoC: pcm512x -> gpiochip creation

ASoC: Intel: sof-pcm512x -> use of gpio and clk API in the 
board-specific machine driver.

clk: hifiberry-dacpro -> use of gpio and creation of clock.

ASoC: dtbindings -> yaml description