[RFC PATCH 00/16] ASoC/SOF/clk/gpio/dt: add Hifiberry DAC+ PRO support
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
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;
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;- }
+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
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.
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
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.
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 */
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()?
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?
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?
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.
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.
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?
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.
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.
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.
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.
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.
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);
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.
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.
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.
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.
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.
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 :(
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...
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.
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); }
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;- }
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.
+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;- }
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;- }
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; }
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;- }
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;- }
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;- }
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;- }
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;- }
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, };
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;elsesclk_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;
+}
- 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;
+}
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.
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");
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.
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.
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).
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>
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");
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"
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; }
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; }
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
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
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.
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.
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.
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; }
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;}
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;}
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;}
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;
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; }
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>; + }; + +...
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.
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.
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
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
participants (5)
-
Andy Shevchenko -
Linus Walleij -
Mark Brown -
Pierre-Louis Bossart -
Stephen Boyd