Re: [PATCH 6/6] ASoC: codecs: Add NeoFidelity NTP8835 codec

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On Fri, Sep 20, 2024 at 08:33:12PM +0300, Igor Prusov wrote:
 > 
> > 
> > > +
> > > +static void ntp8835_reset_gpio(struct ntp8835_priv *ntp8835, bool active)
> > > +{
> > > +	if (active) {
> > > +		/*
> > > +		 * According to NTP8835 datasheet, 6.2 Timing Sequence (recommended):
> > > +		 * Deassert for T2 >= 1ms...
> > > +		 */
> > > +		reset_control_deassert(ntp8835->reset);
> > 
> > Explain in comment why do you need to power up device to perform
> > reset... This sounds odd.
> > 
> 
> This sequence comes from device datasheet, for some reason vendor
> recommends to drive /RESET low for 0.1us during initialization.
> Datasheet also describes (section 6.3) init sequence with simple reset
> deassert, but it's called legacy, though it works fine on my board. Do
> you mean to add more verbose comment than linking to a datasheet?

I think verbose comment would be better.

> 
> > > +		fsleep(1000);
> > > +
> > > +		/* ...Assert for T3 >= 0.1us... */
> > > +		reset_control_assert(ntp8835->reset);
> > > +		fsleep(1);
> > > +
> > > +		/* ...Deassert, and wait for T4 >= 0.5ms before sound on sequence. */
> > > +		reset_control_deassert(ntp8835->reset);
> > > +		fsleep(500);
> > > +	} else {
> > > +		reset_control_assert(ntp8835->reset);
> > 
> > This function is confusing. It is supposed to perform reset and leave
> > the device in active state, but here it leaves the device in reset.
> > 
> > 
> > 
> > > +
> > > +static struct snd_soc_dai_driver ntp8835_dai = {
> > 
> > Not const?
> > 
> 
> ntp8835_dai is passed to devm_snd_soc_register_component(), which takes
> non-const parameter.

Right, indeed.

> 
> > > +	.name = "ntp8835-amplifier",
> > > +	.playback = {
> > > +		.stream_name = "Playback",
> > > +		.channels_min = 1,
> > > +		.channels_max = 3,
> > > +		.rates = SNDRV_PCM_RATE_8000_192000,
> > > +		.formats = NTP8835_FORMATS,
> > > +	},
> > > +	.ops = &ntp8835_dai_ops,
> > > +};
> > > +
> > > +static struct regmap_config ntp8835_regmap = {
> > 
> > Not const?
> > 
> > Judging by weird includes and such simple issues, it looks like you try
> > to upstream downstream or old code. That's not how you are supposed to
> > bring new devices. You expect us to perform review on the same issues we
> > fixed already. Work on newest drivers - take them as template - so you
> > will not repeat the same issues we already fixed.
> > 
> > > +	.reg_bits = 8,
> > > +	.val_bits = 8,
> > > +	.max_register = REG_MAX,
> > > +	.cache_type = REGCACHE_MAPLE,
> > > +};
> > > +
> > > +static int ntp8835_i2c_probe(struct i2c_client *i2c)
> > > +{
> > > +	struct ntp8835_priv *ntp8835;
> > > +	struct regmap *regmap;
> > > +	int ret;
> > > +
> > > +	ntp8835 = devm_kzalloc(&i2c->dev, sizeof(struct ntp8835_priv), GFP_KERNEL);
> > 
> > sizeof(*)
> > 
> > > +	if (!ntp8835)
> > > +		return -ENOMEM;
> > > +
> > > +	ntp8835->i2c = i2c;
> > > +
> > > +	ntp8835->reset = devm_reset_control_get_shared(&i2c->dev, NULL);
> > 
> > shared is on purpose?
> > 
> 
> Yes, we have a board with two amplifiers sharing same reset line, so
> shared allows to work around this hardware issue. Is it the wrong
> approach?

No, it's ok, I just want to be sure you added this consciously.

> 
> > > +	if (IS_ERR(ntp8835->reset))
> > > +		return dev_err_probe(&i2c->dev, PTR_ERR(ntp8835->reset),
> > > +				     "Failed to get reset\n");
> > > +
> > > +	ret = reset_control_deassert(ntp8835->reset);
> > > +	if (ret)
> > > +		return dev_err_probe(&i2c->dev, PTR_ERR(ntp8835->reset),
> > > +				     "Failed to deassert reset\n");
> > > +
> > > +	dev_set_drvdata(&i2c->dev, ntp8835);
> > > +
> > > +	ntp8835_reset_gpio(ntp8835, true);
> > > +
> > > +	regmap = devm_regmap_init_i2c(i2c, &ntp8835_regmap);
> > > +	if (IS_ERR(regmap))
> > > +		return dev_err_probe(&i2c->dev, PTR_ERR(regmap),
> > > +				     "Failed to allocate regmap\n");
> > > +
> > > +	ret = devm_snd_soc_register_component(&i2c->dev, &soc_component_ntp8835,
> > > +					      &ntp8835_dai, 1);
> > > +	if (ret)
> > > +		return dev_err_probe(&i2c->dev, ret,
> > > +				     "Failed to register component\n");
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct i2c_device_id ntp8835_i2c_id[] = {
> > > +	{ "ntp8835", 0 },
> > > +	{}
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, ntp8835_i2c_id);
> > > +
> > > +static const struct of_device_id ntp8835_of_match[] = {
> > > +	{.compatible = "neofidelity,ntp8835",},
> > > +	{.compatible = "neofidelity,ntp8835c",},
> > 
> > This does not match your i2c IDs, which leads to troubles when matching
> > variants.
> > 
> > Anyway, aren't they compatible?
> > 
> > 
> 
> They have identical programming interface and only differ in some output
> signal characteristics. Is it OK use single compatible string in such
> case?

Driver should have one compatible (and one entry in i2c device ID). You
add the second in the bindings as one followed by fallback.
Like this:
https://elixir.bootlin.com/linux/v6.3-rc6/source/Documentation/devicetree/bindings/sound/nvidia,tegra210-ope.yaml#L31

Best regards,
Krzysztof