Re: [PATCH 4/4] ASoC: codecs: Add support for Richtek rt9123p

[ChiYuan Huang]

On Mon, Apr 07, 2025 at 08:53:04AM +0800, ChiYuan Huang wrote:
> On Fri, Apr 04, 2025 at 03:05:19PM -0500, Rob Herring wrote:
> > On Fri, Apr 04, 2025 at 10:22:14PM +0800, cy_huang@xxxxxxxxxxx wrote:
> > > From: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> > > 
> > > Add codec driver for Richtek rt9123p.
> > > 
> > > Signed-off-by: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> > > ---
> > >  sound/soc/codecs/Kconfig   |   6 ++
> > >  sound/soc/codecs/Makefile  |   2 +
> > >  sound/soc/codecs/rt9123p.c | 171 +++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 179 insertions(+)
> > >  create mode 100644 sound/soc/codecs/rt9123p.c
> > > 
> > > diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> > > index c61b2d3cf284..b0fa935846c0 100644
> > > --- a/sound/soc/codecs/Kconfig
> > > +++ b/sound/soc/codecs/Kconfig
> > > @@ -1832,6 +1832,12 @@ config SND_SOC_RT9123
> > >  	  Enable support for the I2C control mode of Richtek RT9123 3.2W mono
> > >  	  Class-D audio amplifier.
> > >  
> > > +config SND_SOC_RT9123P
> > > +	tristate "Richtek RT9123P Mono Class-D Amplifier"
> > > +	help
> > > +	  Enable support for the HW control mode of Richtek RT9123P 3.2W mono
> > > +	  Class-D audio amplifier.
> > > +
> > >  config SND_SOC_RTQ9128
> > >  	tristate "Richtek RTQ9128 45W Digital Input Amplifier"
> > >  	depends on I2C
> > > diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
> > > index d8d0bc367af8..fba699701804 100644
> > > --- a/sound/soc/codecs/Makefile
> > > +++ b/sound/soc/codecs/Makefile
> > > @@ -271,6 +271,7 @@ snd-soc-rt721-sdca-y := rt721-sdca.o rt721-sdca-sdw.o
> > >  snd-soc-rt722-sdca-y := rt722-sdca.o rt722-sdca-sdw.o
> > >  snd-soc-rt9120-y := rt9120.o
> > >  snd-soc-rt9123-y := rt9123.o
> > > +snd-soc-rt9123p-y := rt9123p.o
> > >  snd-soc-rtq9128-y := rtq9128.o
> > >  snd-soc-sdw-mockup-y := sdw-mockup.o
> > >  snd-soc-sgtl5000-y := sgtl5000.o
> > > @@ -686,6 +687,7 @@ obj-$(CONFIG_SND_SOC_RT721_SDCA_SDW)     += snd-soc-rt721-sdca.o
> > >  obj-$(CONFIG_SND_SOC_RT722_SDCA_SDW)     += snd-soc-rt722-sdca.o
> > >  obj-$(CONFIG_SND_SOC_RT9120)	+= snd-soc-rt9120.o
> > >  obj-$(CONFIG_SND_SOC_RT9123)	+= snd-soc-rt9123.o
> > > +obj-$(CONFIG_SND_SOC_RT9123P)	+= snd-soc-rt9123p.o
> > >  obj-$(CONFIG_SND_SOC_RTQ9128)	+= snd-soc-rtq9128.o
> > >  obj-$(CONFIG_SND_SOC_SDW_MOCKUP)     += snd-soc-sdw-mockup.o
> > >  obj-$(CONFIG_SND_SOC_SGTL5000)  += snd-soc-sgtl5000.o
> > > diff --git a/sound/soc/codecs/rt9123p.c b/sound/soc/codecs/rt9123p.c
> > > new file mode 100644
> > > index 000000000000..b0ff5f856e4c
> > > --- /dev/null
> > > +++ b/sound/soc/codecs/rt9123p.c
> > > @@ -0,0 +1,171 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +//
> > > +// rt9123p.c -- RT9123 (HW Mode) ALSA SoC Codec driver
> > > +//
> > > +// Author: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> > > +
> > > +#include <linux/acpi.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/err.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/property.h>
> > > +#include <sound/pcm.h>
> > > +#include <sound/soc.h>
> > > +#include <sound/soc-dai.h>
> > > +#include <sound/soc-dapm.h>
> > > +
> > > +struct rt9123p_priv {
> > > +	struct gpio_desc *enable;
> > > +	unsigned int enable_delay;
> > > +	int enable_switch;
> > > +};
> > > +
> > > +static int rt9123p_daiops_trigger(struct snd_pcm_substream *substream, int cmd,
> > > +				  struct snd_soc_dai *dai)
> > > +{
> > > +	struct snd_soc_component *comp = dai->component;
> > > +	struct rt9123p_priv *rt9123p = snd_soc_component_get_drvdata(comp);
> > > +
> > > +	if (!rt9123p->enable)
> > > +		return 0;
> > > +
> > > +	switch (cmd) {
> > > +	case SNDRV_PCM_TRIGGER_START:
> > > +	case SNDRV_PCM_TRIGGER_RESUME:
> > > +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> > > +		mdelay(rt9123p->enable_delay);
> > > +		if (rt9123p->enable_switch) {
> > > +			gpiod_set_value(rt9123p->enable, 1);
> > > +			dev_dbg(comp->dev, "set enable to 1");
> > > +		}
> > > +		break;
> > > +	case SNDRV_PCM_TRIGGER_STOP:
> > > +	case SNDRV_PCM_TRIGGER_SUSPEND:
> > > +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> > > +		gpiod_set_value(rt9123p->enable, 0);
> > > +		dev_dbg(comp->dev, "set enable to 0");
> > > +		break;
> > > +	default:
> > > +		break;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int rt9123p_enable_event(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kcontrol,
> > > +				int event)
> > > +{
> > > +	struct snd_soc_component *comp = snd_soc_dapm_to_component(w->dapm);
> > > +	struct rt9123p_priv *rt9123p = snd_soc_component_get_drvdata(comp);
> > > +
> > > +	if (event & SND_SOC_DAPM_POST_PMU)
> > > +		rt9123p->enable_switch = 1;
> > > +	else if (event & SND_SOC_DAPM_POST_PMD)
> > > +		rt9123p->enable_switch = 0;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct snd_soc_dapm_widget rt9123p_dapm_widgets[] = {
> > > +	SND_SOC_DAPM_OUTPUT("SPK"),
> > > +	SND_SOC_DAPM_OUT_DRV_E("Amp Drv", SND_SOC_NOPM, 0, 0, NULL, 0, rt9123p_enable_event,
> > > +			       SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD),
> > > +};
> > > +
> > > +static const struct snd_soc_dapm_route rt9123p_dapm_routes[] = {
> > > +	{"Amp Drv", NULL, "HiFi Playback"},
> > > +	{"SPK", NULL, "Amp Drv"},
> > > +};
> > > +
> > > +static const struct snd_soc_component_driver rt9123p_comp_driver = {
> > > +	.dapm_widgets		= rt9123p_dapm_widgets,
> > > +	.num_dapm_widgets	= ARRAY_SIZE(rt9123p_dapm_widgets),
> > > +	.dapm_routes		= rt9123p_dapm_routes,
> > > +	.num_dapm_routes	= ARRAY_SIZE(rt9123p_dapm_routes),
> > > +	.idle_bias_on		= 1,
> > > +	.use_pmdown_time	= 1,
> > > +	.endianness		= 1,
> > > +};
> > > +
> > > +static const struct snd_soc_dai_ops rt9123p_dai_ops = {
> > > +	.trigger        = rt9123p_daiops_trigger,
> > > +};
> > > +
> > > +static struct snd_soc_dai_driver rt9123p_dai_driver = {
> > > +	.name = "HiFi",
> > > +	.playback = {
> > > +		.stream_name	= "HiFi Playback",
> > > +		.formats	= SNDRV_PCM_FMTBIT_S16 | SNDRV_PCM_FMTBIT_S24 |
> > > +				  SNDRV_PCM_FMTBIT_S32,
> > > +		.rates		= SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 |
> > > +				  SNDRV_PCM_RATE_22050 | SNDRV_PCM_RATE_24000 |
> > > +				  SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |
> > > +				  SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |
> > > +				  SNDRV_PCM_RATE_96000,
> > > +		.rate_min	= 8000,
> > > +		.rate_max	= 96000,
> > > +		.channels_min	= 1,
> > > +		.channels_max	= 2,
> > > +	},
> > > +	.ops    = &rt9123p_dai_ops,
> > > +};
> > > +
> > > +static int rt9123p_platform_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct rt9123p_priv *rt9123p;
> > > +	int ret;
> > > +
> > > +	rt9123p = devm_kzalloc(dev, sizeof(*rt9123p), GFP_KERNEL);
> > > +	if (!rt9123p)
> > > +		return -ENOMEM;
> > > +
> > > +	rt9123p->enable = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW);
> > > +	if (IS_ERR(rt9123p->enable))
> > > +		return PTR_ERR(rt9123p->enable);
> > > +
> > > +	ret = device_property_read_u32(dev, "enable-delay", &rt9123p->enable_delay);
> > 
> > Not documented. You have a single GPIO for any sort of control. What is 
> > this delay relative to? Why does it need to be tuned per board? 
> > Properties with units have unit suffix.
> > 
> Like as the property desciption in the document, to wait clock or data
> to be ready. Sometimes, there's unknown jitter signal while clock or
> data starts. It'll be better to have a delay for this. That's why it's
> optional.
> 
Hi, Rob:
  I'm sorting out all the changes needed for v2.

In last mail, I have already described what the delay for.

Another question is for this property name 'enable-delay'. Should I add a
suffix like as 'enable-delay-ms'?
> Thanks.
> > Rob