| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z/XILJ8YRt905whu@git-send.richtek.com>
Date: Wed, 9 Apr 2025 09:06:52 +0800
From: ChiYuan Huang <cy_huang@...htek.com>
To: Rob Herring <robh@...nel.org>
CC: Mark Brown <broonie@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Liam Girdwood <lgirdwood@...il.com>,
Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>, Otto lin
<otto_lin@...htek.com>, Allen Lin <allen_lin@...htek.com>,
<devicetree@...r.kernel.org>, <linux-sound@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/4] ASoC: codecs: Add support for Richtek rt9123p
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@...htek.com wrote:
> > > From: ChiYuan Huang <cy_huang@...htek.com>
> > >
> > > Add codec driver for Richtek rt9123p.
> > >
> > > Signed-off-by: ChiYuan Huang <cy_huang@...htek.com>
> > > ---
> > > 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@...htek.com>
> > > +
> > > +#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
Powered by blists - more mailing lists