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

[Xingyu Wu]

> 
> 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