Re: [PATCH v1 1/2] ASoC: dt-bindings: tegra: Add binding for RT5631

Re: [PATCH v1 1/2] ASoC: dt-bindings: tegra: Add binding for RT5631

[Jon Hunter]

On 31/01/2021 18:41, Ion Agorria wrote:
> From: Svyatoslav Ryhel <clamor95@gmail.com>
> 
> Add device-tree binding that describes hardware integration of RT5631
> audio codec chip with NVIDIA Tegra SoCs.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Ion Agorria <ion@agorria.com>
> ---
>  .../sound/nvidia,tegra-audio-rt5631.yaml      | 134 ++++++++++++++++++
>  1 file changed, 134 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/nvidia,tegra-audio-rt5631.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/nvidia,tegra-audio-rt5631.yaml b/Documentation/devicetree/bindings/sound/nvidia,tegra-audio-rt5631.yaml
> new file mode 100644
> index 000000000000..6ee62c599518
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/nvidia,tegra-audio-rt5631.yaml
> @@ -0,0 +1,134 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: http://devicetree.org/schemas/sound/nvidia,tegra-audio-rt5631.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NVIDIA Tegra RT5631 ASoC
> +
> +description: |
> +  This binding describes integration of the Realtek ALC5631/RT5631 sound
> +  codec with the sound system of NVIDIA Tegra SoCs.
> +
> +maintainers:
> +  - Jon Hunter <jonathanh@nvidia.com>
> +  - Stephen Warren <swarren@nvidia.com>
> +  - Thierry Reding <thierry.reding@gmail.com>


Thierry and I should be sufficient and so no need to include Stephen in
the list.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - nvidia,tegra-audio-rt5631
> +
> +  clocks:
> +    minItems: 3
> +    items:
> +      - description: PLL A clock
> +      - description: PLL A OUT0 clock
> +      - description: The Tegra cdev1/extern1 clock, which feeds the card's mclk
> +
> +  clock-names:
> +    minItems: 3
> +    items:
> +      - const: pll_a
> +      - const: pll_a_out0
> +      - const: mclk
> +
> +  assigned-clocks:
> +    minItems: 1
> +    maxItems: 3
> +
> +  assigned-clock-parents:
> +    minItems: 1
> +    maxItems: 3
> +
> +  assigned-clock-rates:
> +    minItems: 1
> +    maxItems: 3
> +
> +  nvidia,model:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description: User-visible name of this sound complex.
> +
> +  nvidia,audio-routing:
> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> +    description: |
> +      A list of the connections between audio components.
> +      Each entry is a pair of strings, the first being the connection's sink,
> +      the second being the connection's source. Valid names for sources and
> +      sinks are the RT5631's pins (as documented in its binding), and the jacks
> +      on the board:
> +
> +      * Int Spk
> +      * Headphone Jack
> +      * Mic Jack
> +      * Int Mic
> +
> +  nvidia,i2s-controller:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Phandle of the Tegra I2S controller.
> +
> +  nvidia,audio-codec:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Phandle of the RT5631 audio codec.
> +
> +  nvidia,hp-mute-gpios:
> +    description: GPIO that mutes the headphones (button event).
> +    maxItems: 1
> +
> +  nvidia,hp-det-gpios:
> +    description: GPIO that detects headphones plug-in.
> +    maxItems: 1
> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - clocks
> +  - clock-names
> +  - assigned-clocks
> +  - assigned-clock-parents
> +  - nvidia,model
> +  - nvidia,audio-routing
> +  - nvidia,i2s-controller
> +  - nvidia,audio-codec
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/tegra30-car.h>
> +    #include <dt-bindings/soc/tegra-pmc.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    sound {
> +        compatible = "nvidia,tegra-audio-rt5631";
> +        nvidia,model = "NVIDIA Tegra RT5631";
> +
> +        nvidia,audio-routing =
> +            "Headphone Jack", "HPOL",
> +            "Headphone Jack", "HPOR",
> +            "Int Spk", "SPOL",
> +            "Int Spk", "SPOR",
> +            "MIC1", "MIC Bias1",
> +            "MIC Bias1", "Mic Jack",
> +            "DMIC", "Int Mic";
> +
> +        nvidia,i2s-controller = <&tegra_i2s1>;
> +        nvidia,audio-codec = <&rt5631>;
> +
> +        nvidia,hp-det-gpios = <&gpio 178 GPIO_ACTIVE_LOW>;
> +        nvidia,hp-mute-gpios = <&gpio 186 GPIO_ACTIVE_LOW>;
> +
> +        clocks = <&tegra_car TEGRA30_CLK_PLL_A>,
> +                 <&tegra_car TEGRA30_CLK_PLL_A_OUT0>,
> +                 <&tegra_pmc TEGRA_PMC_CLK_OUT_1>;
> +        clock-names = "pll_a", "pll_a_out0", "mclk";
> +
> +        assigned-clocks = <&tegra_car TEGRA30_CLK_EXTERN1>,
> +                          <&tegra_pmc TEGRA_PMC_CLK_OUT_1>;
> +
> +        assigned-clock-parents = <&tegra_car TEGRA30_CLK_PLL_A_OUT0>,
> +                                 <&tegra_car TEGRA30_CLK_EXTERN1>;
> +    };
> +
> +...
> 

Otherwise looks good to me ...

Reviewed-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

Re: [PATCH v1 2/2] ASoC: tegra: Add RT5631 machine driver

[Jon Hunter]

On 31/01/2021 18:41, Ion Agorria wrote:
> From: Svyatoslav Ryhel <clamor95@gmail.com>
> 
> Add Tegra ASoC driver for Realtek ALC5631/RT5631 codec. The RT5631
> codec is found on devices like ASUS Transformer TF201, TF700T and other
> Tegra-based Android tablets.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Ion Agorria <ion@agorria.com>
> ---
>  sound/soc/tegra/Kconfig        |   8 +
>  sound/soc/tegra/Makefile       |   2 +
>  sound/soc/tegra/tegra_rt5631.c | 261 +++++++++++++++++++++++++++++++++
>  3 files changed, 271 insertions(+)
>  create mode 100644 sound/soc/tegra/tegra_rt5631.c
> 
> diff --git a/sound/soc/tegra/Kconfig b/sound/soc/tegra/Kconfig
> index acaf7339168d..449a858f155d 100644
> --- a/sound/soc/tegra/Kconfig
> +++ b/sound/soc/tegra/Kconfig
> @@ -126,6 +126,14 @@ config SND_SOC_TEGRA_AUDIO_GRAPH_CARD
>  	  few things for Tegra audio. Most of the code is re-used from
>  	  audio graph driver and the same DT bindings are used.
>  
> +config SND_SOC_TEGRA_RT5631
> +	tristate "SoC Audio support for Tegra boards using an RT5631 codec"
> +	depends on SND_SOC_TEGRA && I2C && GPIOLIB
> +	select SND_SOC_RT5631
> +	help
> +	  Say Y or M here if you want to add support for SoC audio on Tegra
> +	  boards using the RT5631 codec, such as Transformer.
> +
>  config SND_SOC_TEGRA_RT5640
>  	tristate "SoC Audio support for Tegra boards using an RT5640 codec"
>  	depends on SND_SOC_TEGRA && I2C && GPIOLIB
> diff --git a/sound/soc/tegra/Makefile b/sound/soc/tegra/Makefile
> index af0b9889306c..11debfc03bc4 100644
> --- a/sound/soc/tegra/Makefile
> +++ b/sound/soc/tegra/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_SND_SOC_TEGRA186_DSPK) += snd-soc-tegra186-dspk.o
>  obj-$(CONFIG_SND_SOC_TEGRA210_ADMAIF) += snd-soc-tegra210-admaif.o
>  
>  # Tegra machine Support
> +snd-soc-tegra-rt5631-objs := tegra_rt5631.o
>  snd-soc-tegra-rt5640-objs := tegra_rt5640.o
>  snd-soc-tegra-rt5677-objs := tegra_rt5677.o
>  snd-soc-tegra-wm8753-objs := tegra_wm8753.o
> @@ -41,6 +42,7 @@ snd-soc-tegra-max98090-objs := tegra_max98090.o
>  snd-soc-tegra-sgtl5000-objs := tegra_sgtl5000.o
>  snd-soc-tegra-audio-graph-card-objs := tegra_audio_graph_card.o
>  
> +obj-$(CONFIG_SND_SOC_TEGRA_RT5631) += snd-soc-tegra-rt5631.o
>  obj-$(CONFIG_SND_SOC_TEGRA_RT5640) += snd-soc-tegra-rt5640.o
>  obj-$(CONFIG_SND_SOC_TEGRA_RT5677) += snd-soc-tegra-rt5677.o
>  obj-$(CONFIG_SND_SOC_TEGRA_WM8753) += snd-soc-tegra-wm8753.o
> diff --git a/sound/soc/tegra/tegra_rt5631.c b/sound/soc/tegra/tegra_rt5631.c
> new file mode 100644
> index 000000000000..9034f48bcb26
> --- /dev/null
> +++ b/sound/soc/tegra/tegra_rt5631.c
> @@ -0,0 +1,261 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * tegra_rt5631.c - Tegra machine ASoC driver for boards using RT5631 codec.
> + *
> + * Copyright (c) 2020, Svyatoslav Ryhel and Ion Agorria

2021 now :-)

> + *
> + * Based on code copyright/by:
> + *
> + * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
> + * Author: Stephen Warren <swarren@nvidia.com>
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include <sound/core.h>
> +#include <sound/jack.h>
> +#include <sound/pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +
> +#include "tegra_asoc_utils.h"
> +
> +#include "../codecs/rt5631.h"
> +
> +struct tegra_rt5631 {
> +	struct tegra_asoc_utils_data util_data;
> +	struct gpio_desc *gpiod_hp_mute;
> +	struct gpio_desc *gpiod_hp_det;
> +};
> +
> +static int tegra_rt5631_hw_params(struct snd_pcm_substream *substream,
> +				  struct snd_pcm_hw_params *params)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> +	struct snd_soc_card *card = rtd->card;
> +	struct tegra_rt5631 *machine = snd_soc_card_get_drvdata(card);
> +	unsigned int srate, mclk;
> +	int err;
> +
> +	srate = params_rate(params);
> +	switch (srate) {
> +	case 64000:
> +	case 88200:
> +	case 96000:
> +		mclk = 128 * srate;
> +		break;
> +	default:
> +		mclk = 256 * srate;
> +		break;
> +	}
> +	/* FIXME: Codec only requires >= 3MHz if OSR==0 */
> +	while (mclk < 6000000)
> +		mclk *= 2;

I don't understand that above. You say greater or equal to 3MHz, but
then compare against 6MHz. Please explain and elborate a bit more in the
comment.

> +
> +	err = tegra_asoc_utils_set_rate(&machine->util_data, srate, mclk);
> +	if (err < 0) {
> +		dev_err(card->dev, "Can't configure clocks\n");
> +		return err;
> +	}
> +
> +	err = snd_soc_dai_set_sysclk(codec_dai, 0, mclk, SND_SOC_CLOCK_IN);
> +	if (err < 0) {
> +		dev_err(card->dev, "codec_dai clock not set\n");
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct snd_soc_ops tegra_rt5631_ops = {
> +	.hw_params = tegra_rt5631_hw_params,
> +};
> +
> +static struct snd_soc_jack tegra_rt5631_hp_jack;
> +
> +static struct snd_soc_jack_pin tegra_rt5631_hp_jack_pins[] = {
> +	{
> +		.pin = "Headphone Jack",
> +		.mask = SND_JACK_HEADPHONE,
> +	},
> +};
> +
> +static struct snd_soc_jack_gpio tegra_rt5631_hp_jack_gpio = {
> +	.name = "Headphone detection",
> +	.report = SND_JACK_HEADPHONE,
> +	.debounce_time = 150,
> +};
> +
> +static int tegra_rt5631_event_hp(struct snd_soc_dapm_widget *w,
> +				 struct snd_kcontrol *k, int event)
> +{
> +	struct snd_soc_dapm_context *dapm = w->dapm;
> +	struct snd_soc_card *card = dapm->card;
> +	struct tegra_rt5631 *machine = snd_soc_card_get_drvdata(card);
> +
> +	gpiod_set_value_cansleep(machine->gpiod_hp_mute,
> +				 !SND_SOC_DAPM_EVENT_ON(event));
> +
> +	return 0;
> +}
> +
> +static const struct snd_soc_dapm_widget tegra_rt5631_dapm_widgets[] = {
> +	SND_SOC_DAPM_SPK("Int Spk", NULL),
> +	SND_SOC_DAPM_HP("Headphone Jack", tegra_rt5631_event_hp),
> +	SND_SOC_DAPM_MIC("Mic Jack", NULL),
> +	SND_SOC_DAPM_MIC("Int Mic", NULL),
> +};
> +
> +static const struct snd_kcontrol_new tegra_rt5631_controls[] = {
> +	SOC_DAPM_PIN_SWITCH("Int Spk"),
> +	SOC_DAPM_PIN_SWITCH("Int Mic"),
> +};
> +
> +static int tegra_rt5631_init(struct snd_soc_pcm_runtime *rtd)
> +{
> +	struct tegra_rt5631 *machine = snd_soc_card_get_drvdata(rtd->card);
> +	int ret;
> +
> +	ret = snd_soc_card_jack_new(rtd->card, "Headphone Jack",
> +				    SND_JACK_HEADPHONE,
> +				    &tegra_rt5631_hp_jack,
> +				    tegra_rt5631_hp_jack_pins,
> +				    ARRAY_SIZE(tegra_rt5631_hp_jack_pins));
> +	if (ret) {
> +		dev_err(rtd->dev, "Headset Jack creation failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (machine->gpiod_hp_det) {
> +		tegra_rt5631_hp_jack_gpio.desc = machine->gpiod_hp_det;
> +
> +		ret = snd_soc_jack_add_gpios(&tegra_rt5631_hp_jack, 1,
> +					     &tegra_rt5631_hp_jack_gpio);
> +		if (ret)
> +			dev_err(rtd->dev, "Jack GPIOs not added: %d\n", ret);
> +	}
> +
> +	return 0;
> +}
> +
> +SND_SOC_DAILINK_DEFS(hifi,
> +	DAILINK_COMP_ARRAY(COMP_EMPTY()),
> +	DAILINK_COMP_ARRAY(COMP_CODEC(NULL, "rt5631-hifi")),
> +	DAILINK_COMP_ARRAY(COMP_EMPTY()));
> +
> +static struct snd_soc_dai_link tegra_rt5631_dai = {
> +	.name = "RT5631",
> +	.stream_name = "RT5631 PCM",
> +	.init = tegra_rt5631_init,
> +	.ops = &tegra_rt5631_ops,
> +	.dai_fmt = SND_SOC_DAIFMT_I2S |
> +		   SND_SOC_DAIFMT_NB_NF |
> +		   SND_SOC_DAIFMT_CBS_CFS,
> +	SND_SOC_DAILINK_REG(hifi),
> +};
> +
> +static struct snd_soc_card snd_soc_tegra_rt5631 = {
> +	.name = "tegra-rt5631",
> +	.owner = THIS_MODULE,
> +	.dai_link = &tegra_rt5631_dai,
> +	.num_links = 1,
> +	.controls = tegra_rt5631_controls,
> +	.num_controls = ARRAY_SIZE(tegra_rt5631_controls),
> +	.dapm_widgets = tegra_rt5631_dapm_widgets,
> +	.num_dapm_widgets = ARRAY_SIZE(tegra_rt5631_dapm_widgets),
> +	.fully_routed = true,
> +};
> +
> +static int tegra_rt5631_probe(struct platform_device *pdev)
> +{
> +	struct snd_soc_card *card = &snd_soc_tegra_rt5631;
> +	struct device_node *np_codec, *np_i2s;
> +	struct tegra_rt5631 *machine;
> +	struct gpio_desc *gpiod;
> +	int ret;
> +
> +	machine = devm_kzalloc(&pdev->dev, sizeof(*machine), GFP_KERNEL);
> +	if (!machine)
> +		return -ENOMEM;
> +
> +	card->dev = &pdev->dev;
> +	snd_soc_card_set_drvdata(card, machine);
> +
> +	gpiod = devm_gpiod_get_optional(&pdev->dev, "nvidia,hp-mute",
> +					GPIOD_OUT_HIGH);
> +	if (IS_ERR(gpiod))
> +		return PTR_ERR(gpiod);
> +
> +	machine->gpiod_hp_mute = gpiod;
> +
> +	gpiod = devm_gpiod_get_optional(&pdev->dev, "nvidia,hp-det",
> +					GPIOD_IN);
> +	if (IS_ERR(gpiod))
> +		return PTR_ERR(gpiod);
> +
> +	machine->gpiod_hp_det = gpiod;
> +
> +	ret = snd_soc_of_parse_card_name(card, "nvidia,model");
> +	if (ret)
> +		return ret;
> +
> +	ret = snd_soc_of_parse_audio_routing(card, "nvidia,audio-routing");
> +	if (ret)
> +		return ret;
> +
> +	np_codec = of_parse_phandle(pdev->dev.of_node, "nvidia,audio-codec", 0);
> +	if (!np_codec) {
> +		dev_err(&pdev->dev,
> +			"Property 'nvidia,audio-codec' missing or invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	np_i2s = of_parse_phandle(pdev->dev.of_node, "nvidia,i2s-controller", 0);
> +	if (!np_i2s) {
> +		dev_err(&pdev->dev,
> +			"Property 'nvidia,i2s-controller' missing or invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	tegra_rt5631_dai.cpus->of_node = np_i2s;
> +	tegra_rt5631_dai.codecs->of_node = np_codec;
> +	tegra_rt5631_dai.platforms->of_node = np_i2s;
> +
> +	ret = tegra_asoc_utils_init(&machine->util_data, &pdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_snd_soc_register_card(&pdev->dev, card);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id tegra_rt5631_of_match[] = {
> +	{ .compatible = "nvidia,tegra-audio-rt5631", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, tegra_rt5631_of_match);
> +
> +static struct platform_driver tegra_rt5631_driver = {
> +	.driver = {
> +		.name = "tegra-snd-rt5631",
> +		.pm = &snd_soc_pm_ops,
> +		.of_match_table = tegra_rt5631_of_match,
> +	},
> +	.probe = tegra_rt5631_probe,
> +};
> +module_platform_driver(tegra_rt5631_driver);
> +
> +MODULE_DESCRIPTION("Tegra+RT5631 machine ASoC driver");
> +MODULE_AUTHOR("Stephen Warren <swarren@nvidia.com>");
> +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> +MODULE_AUTHOR("Ion Agorria <ion@agorria.com>");
> +MODULE_LICENSE("GPL");


So this is very similar to tegra_rt5677, is it not possible to support
both codecs with the same machine driver?

Jon

-- 
nvpublic

Re: [PATCH v1 2/2] ASoC: tegra: Add RT5631 machine driver

[Dmitry Osipenko]

02.02.2021 16:22, Jon Hunter пишет:
> So this is very similar to tegra_rt5677, is it not possible to support
> both codecs with the same machine driver?

These codecs require individual configurations and those
"../codecs/rt5631.h" and  "../codecs/rt5677.h" aren't compatible at a
quick glance.

The tegra_rt5677 also uses outdated GPIO API and etc. Hence the new
driver should be a better base anyways.

Overall it shouldn't worth time and effort trying to squeeze these
drivers into a single one, IMO.

Re: [PATCH v1 2/2] ASoC: tegra: Add RT5631 machine driver

[Jon Hunter]

On 02/02/2021 15:25, Dmitry Osipenko wrote:
> 02.02.2021 16:22, Jon Hunter пишет:
>> So this is very similar to tegra_rt5677, is it not possible to support
>> both codecs with the same machine driver?
> 
> These codecs require individual configurations and those
> "../codecs/rt5631.h" and  "../codecs/rt5677.h" aren't compatible at a
> quick glance.

Right but not all of that is needed. What is actually needed from the
header files?

> The tegra_rt5677 also uses outdated GPIO API and etc. Hence the new
> driver should be a better base anyways.

Sounds like a good time to update it :-)

> Overall it shouldn't worth time and effort trying to squeeze these
> drivers into a single one, IMO.

Not sure I agree when these drivers appear to be 90% the same.

Jon

-- 
nvpublic

Re: [PATCH v1 2/2] ASoC: tegra: Add RT5631 machine driver

[Dmitry Osipenko]

02.02.2021 19:24, Jon Hunter пишет:
> 
> On 02/02/2021 15:25, Dmitry Osipenko wrote:
>> 02.02.2021 16:22, Jon Hunter пишет:
>>> So this is very similar to tegra_rt5677, is it not possible to support
>>> both codecs with the same machine driver?
>>
>> These codecs require individual configurations and those
>> "../codecs/rt5631.h" and  "../codecs/rt5677.h" aren't compatible at a
>> quick glance.
> 
> Right but not all of that is needed. What is actually needed from the
> header files?

I recall that some downstream drivers were doing some special
programming of codecs. This is not relevant to the current upstream
drivers, but potentially it may become needed and then that single
driver could become unmanageable.

>> The tegra_rt5677 also uses outdated GPIO API and etc. Hence the new
>> driver should be a better base anyways.
> 
> Sounds like a good time to update it :-)
> 
>> Overall it shouldn't worth time and effort trying to squeeze these
>> drivers into a single one, IMO.
> 
> Not sure I agree when these drivers appear to be 90% the same.

Of course we could try, but I suggest that it should be done separately
from this series. Certainly it will take a lot of extra effort and this
series isn't about improving older drivers, it's about enabling h/w
support for the RT5631 codec.

It shouldn't be a problem to switch to the common machine driver later
on if this driver will turn out to be feasible.

Re: [PATCH v1 2/2] ASoC: tegra: Add RT5631 machine driver

[Mark Brown]

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

On Tue, Feb 02, 2021 at 04:24:31PM +0000, Jon Hunter wrote:
> On 02/02/2021 15:25, Dmitry Osipenko wrote:

> > These codecs require individual configurations and those
> > "../codecs/rt5631.h" and  "../codecs/rt5677.h" aren't compatible at a
> > quick glance.

> Right but not all of that is needed. What is actually needed from the
> header files?

Right, and if it's just a case of having a different hw_params() or
something then the majority of the driver could be shared with just a
few bits being handled differently.

> > The tegra_rt5677 also uses outdated GPIO API and etc. Hence the new
> > driver should be a better base anyways.

> Sounds like a good time to update it :-)

Yeah.

> > Overall it shouldn't worth time and effort trying to squeeze these
> > drivers into a single one, IMO.

> Not sure I agree when these drivers appear to be 90% the same.

It's certainly worth considering - given that it's the same silicon
vendor working with the same SoC vendor's reference designs it seems
likely that things will look pretty similar at the system integration
level.  It's possible that it's more trouble than it's worth but it'd be
good to have a more concrete understanding of why.

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

Re: [PATCH v1 2/2] ASoC: tegra: Add RT5631 machine driver

[Dmitry Osipenko]

31.01.2021 21:41, Ion Agorria пишет:
> +	np_codec = of_parse_phandle(pdev->dev.of_node, "nvidia,audio-codec", 0);
> +	if (!np_codec) {
> +		dev_err(&pdev->dev,
> +			"Property 'nvidia,audio-codec' missing or invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	np_i2s = of_parse_phandle(pdev->dev.of_node, "nvidia,i2s-controller", 0);
> +	if (!np_i2s) {
> +		dev_err(&pdev->dev,
> +			"Property 'nvidia,i2s-controller' missing or invalid\n");
> +		return -EINVAL;
> +	}

We missed that the np_codec and np_i2s should be put when driver is released.

https://elixir.bootlin.com/linux/v5.11-rc6/source/drivers/of/base.c#L1429

We could fix it with a devm helper in v2.

diff --git a/sound/soc/tegra/tegra_rt5631.c b/sound/soc/tegra/tegra_rt5631.c
index 9034f48bcb26..84f23915bd95 100644
--- a/sound/soc/tegra/tegra_rt5631.c
+++ b/sound/soc/tegra/tegra_rt5631.c
@@ -172,6 +172,30 @@ static struct snd_soc_card snd_soc_tegra_rt5631 = {
 	.fully_routed = true,
 };
 
+static void tegra_rt5631_node_release(void *of_node)
+{
+	of_node_put(of_node);
+}
+
+static struct device_node *
+tegra_rt5631_parse_phandle(struct device *dev, const char *name)
+{
+	struct device_node *np;
+	int err;
+
+	np = of_parse_phandle(dev->of_node, name, 0);
+	if (!np) {
+		dev_err(dev, "Property '%s' missing or invalid\n", name);
+		return ERR_PTR(-EINVAL);
+	}
+
+	err = devm_add_action_or_reset(dev, tegra_rt5631_node_release, np);
+	if (err)
+		return ERR_PTR(err);
+
+	return np;
+}
+
 static int tegra_rt5631_probe(struct platform_device *pdev)
 {
 	struct snd_soc_card *card = &snd_soc_tegra_rt5631;
@@ -209,19 +233,13 @@ static int tegra_rt5631_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	np_codec = of_parse_phandle(pdev->dev.of_node, "nvidia,audio-codec", 0);
-	if (!np_codec) {
-		dev_err(&pdev->dev,
-			"Property 'nvidia,audio-codec' missing or invalid\n");
-		return -EINVAL;
-	}
+	np_codec = tegra_rt5631_parse_phandle(&pdev->dev, "nvidia,audio-codec");
+	if (IS_ERR(np_codec))
+		return PTR_ERR(np_codec);
 
-	np_i2s = of_parse_phandle(pdev->dev.of_node, "nvidia,i2s-controller", 0);
-	if (!np_i2s) {
-		dev_err(&pdev->dev,
-			"Property 'nvidia,i2s-controller' missing or invalid\n");
-		return -EINVAL;
-	}
+	np_i2s = tegra_rt5631_parse_phandle(&pdev->dev, "nvidia,i2s-controller");
+	if (!np_i2s)
+		return PTR_ERR(np_i2s);
 
 	tegra_rt5631_dai.cpus->of_node = np_i2s;
 	tegra_rt5631_dai.codecs->of_node = np_codec;

Re: [PATCH v1 0/2] ASoC: tegra: Add RT5631 machine driver

[Jon Hunter]

On 31/01/2021 18:40, Ion Agorria wrote:
> From: Ion Agorria <ion@agorria.com>
> 
> Adds support for Tegra SoC devices with RT5631 sound codec.
> Playback to speakers, headphones and internal mic recording works.
> 
> This driver is used for ASUS Transformer TF201, TF700T and others
> Tegra based devices containing RT5631.
> 
> Svyatoslav Ryhel (2):
>   ASoC: dt-bindings: tegra: Add binding for RT5631
>   ASoC: tegra: Add RT5631 machine driver
> 
>  .../sound/nvidia,tegra-audio-rt5631.yaml      | 134 +++++++++
>  sound/soc/tegra/Kconfig                       |   8 +
>  sound/soc/tegra/Makefile                      |   2 +
>  sound/soc/tegra/tegra_rt5631.c                | 261 ++++++++++++++++++
>  4 files changed, 405 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/nvidia,tegra-audio-rt5631.yaml
>  create mode 100644 sound/soc/tegra/tegra_rt5631.c


I don't see any user of this driver included. Any reason why?

Jon

-- 
nvpublic

Re: [PATCH v1 0/2] ASoC: tegra: Add RT5631 machine driver

[Dmitry Osipenko]

02.02.2021 16:23, Jon Hunter пишет:
> 
> On 31/01/2021 18:40, Ion Agorria wrote:
>> From: Ion Agorria <ion@agorria.com>
>>
>> Adds support for Tegra SoC devices with RT5631 sound codec.
>> Playback to speakers, headphones and internal mic recording works.
>>
>> This driver is used for ASUS Transformer TF201, TF700T and others
>> Tegra based devices containing RT5631.
>>
>> Svyatoslav Ryhel (2):
>>   ASoC: dt-bindings: tegra: Add binding for RT5631
>>   ASoC: tegra: Add RT5631 machine driver
>>
>>  .../sound/nvidia,tegra-audio-rt5631.yaml      | 134 +++++++++
>>  sound/soc/tegra/Kconfig                       |   8 +
>>  sound/soc/tegra/Makefile                      |   2 +
>>  sound/soc/tegra/tegra_rt5631.c                | 261 ++++++++++++++++++
>>  4 files changed, 405 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/sound/nvidia,tegra-audio-rt5631.yaml
>>  create mode 100644 sound/soc/tegra/tegra_rt5631.c
> 
> 
> I don't see any user of this driver included. Any reason why?

They should come a bit later. The TF201/300T device-trees should be in a
good state already, we just need to finalize them for upstream and send out.

IIUC, the audio codec and dock station drivers are the most noticeable
missing parts in upsteam kernel. Ion and Svyatoslav should be able to
clarify the state of the devices in a more details.