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
