linux-kernel - 回复: [PATCH v1 2/2] ASoC: starfive: Add PDM controller support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 8 Mar 2024 08:45:45 +0000
From: Xingyu Wu <xingyu.wu@...rfivetech.com>
To: Mark Brown <broonie@...nel.org>
CC: Liam Girdwood <lgirdwood@...il.com>, Takashi Iwai <tiwai@...e.com>,
	Jaroslav Kysela <perex@...ex.cz>, Rob Herring <robh+dt@...nel.org>, Krzysztof
 Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley
	<conor.dooley@...rochip.com>, Walker Chen <walker.chen@...rfivetech.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
	"linux-sound@...r.kernel.org" <linux-sound@...r.kernel.org>
Subject:
 回复: [PATCH v1 2/2] ASoC: starfive: Add PDM controller support

> 
> On Thu, Mar 07, 2024 at 11:37:08AM +0800, Xingyu Wu wrote:
> 
> > +static const struct snd_kcontrol_new jh8100_pdm_snd_controls[] = {
> > +	SOC_SINGLE("DC compensation Control", JH8100_PDM_DMIC_CTRL0, 30,
> 1, 0),
> > +	SOC_SINGLE("High Pass Filter Control", JH8100_PDM_DMIC_CTRL0, 28, 1,
> 0),
> > +	SOC_SINGLE("Left Channel Volume Control", JH8100_PDM_DMIC_CTRL0,
> 23, 1, 0),
> > +	SOC_SINGLE("Right Channel Volume Control", JH8100_PDM_DMIC_CTRL0,
> 22, 1, 0),
> > +	SOC_SINGLE_TLV("Volume", JH8100_PDM_DMIC_CTRL0, 16, 0x3F, 1,
> volume_tlv),
> > +	SOC_SINGLE("Data MSB Shift", JH8100_PDM_DMIC_CTRL0, 1, 7, 0),
> > +	SOC_SINGLE("SCALE", JH8100_PDM_DC_SCALE0, 0, 0x3F, 0),
> > +	SOC_SINGLE("DC offset", JH8100_PDM_DC_SCALE0, 8, 0xFFFFF, 0), };
> 
> Simple on/off switches should have names ending in Switch, volumes should end
> in Volume as per control-names.rst.  Please for the next version you post show
> the output of running mixer-test on a system with this device, it will identify
> these and other issues.

Will fix. Thanks.

> 
> > +static int jh8100_pdm_component_probe(struct snd_soc_component
> > +*component) {
> > +	struct jh8100_pdm_priv *priv =
> > +snd_soc_component_get_drvdata(component);
> > +
> > +	snd_soc_component_init_regmap(component, priv->regmap);
> > +	snd_soc_add_component_controls(component, jh8100_pdm_snd_controls,
> > +				       ARRAY_SIZE(jh8100_pdm_snd_controls));
> 
> You can just specify the controls in the snd_soc_compoenent_driver.

It seem to be more convenient. Noted.

> 
> > +#ifdef CONFIG_PM
> > +static int jh8100_pdm_runtime_suspend(struct device *dev) {
> > +	struct jh8100_pdm_priv *priv = dev_get_drvdata(dev);
> > +
> > +	clk_disable_unprepare(priv->icg_clk);
> > +	return 0;
> > +}
> > +
> > +static int jh8100_pdm_runtime_resume(struct device *dev) {
> > +	struct jh8100_pdm_priv *priv = dev_get_drvdata(dev);
> > +
> > +	return jh8100_pdm_crg_enable(priv);
> > +}
> > +#endif
> 
> It's weird that the runtime suspend and resume are not symmetric - why do we
> need to bring the device out of reset but not put it into reset?

I will add the reset when suspend.

> 
> > +	if (!device_property_read_u8(&pdev->dev, "starfive,pdm-modulex",
> &using_modulex))
> > +		if (using_modulex == 1)
> > +			base += JH8100_PDM_MODULEX_SHIFT; /* Use module 1 */
> 
> This really looks like you've got one hardware block with two devices in it, either
> the address ranges registered for the devices in DT should be separate and you
> shouldn't need this property or you should have one component registering both
> PDM interfaces.

Yeah, They like two independent device and have different register to configure, but just use the same clocks and resets.
Due to the sample rate depend on the share clocks, they should be registered together as a 4-channel capture device (rarely used), or just one of them can be registered separately as a 2-channel device.
BTW, can I use the 0x12250000 about the property of reg for device 0 or 0x12250010 for device 1 to choose which device to be used in DT?

Best regards,
Xingyu Wu