Re: [alsa-devel] [PATCH 4/9] ASoC: tegra: add Tegra210 based I2S driver

[Sameer Pujar]

On 1/21/2020 9:33 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 21.01.2020 17:21, Sameer Pujar ÐÐÑÐÑ:
>
> [snip]
>
> > > > +static int tegra210_i2s_put_control(struct snd_kcontrol *kcontrol,
> > > > +     struct snd_ctl_elem_value *ucontrol)
> > > Checkpatch should complain about the wrong indentation here.
> > I had run checkpatch before sending the patch, below is the result.
> > -----
> > WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> > #70:
> > new file mode 100644
> >
> > total: 0 errors, 1 warnings, 1103 lines checked
> >
> > NOTE: For some of the reported defects, checkpatch may be able to
> >        mechanically convert to the typical style using --fix or
> > --fix-inplace.
> > -----
> Using 'checkpatch --strict':
>
> CHECK: Alignment should match open parenthesis
> #2693: FILE: sound/soc/tegra/tegra210_i2s.c:362:
> +static int tegra210_i2s_put_control(struct snd_kcontrol *kcontrol,
> +       struct snd_ctl_elem_value *ucontrol
>
> [snip]

will fix
> > > > +
> > > > +     } else if (strstr(kcontrol->id.name, "Playback Audio Bit Format"))
> > > > +             i2s->audio_fmt_override[I2S_RX_PATH] = value;
> > > > +     else if (strstr(kcontrol->id.name, "Capture Audio Bit Format"))
> > > > +             i2s->audio_fmt_override[I2S_TX_PATH] = value;
> > > > +     else if (strstr(kcontrol->id.name, "Client Bit Format"))
> > > > +             i2s->client_fmt_override = value;
> > > > +     else if (strstr(kcontrol->id.name, "Playback Audio Channels"))
> > > > +             i2s->audio_ch_override[I2S_RX_PATH] = value;
> > > > +     else if (strstr(kcontrol->id.name, "Capture Audio Channels"))
> > > > +             i2s->audio_ch_override[I2S_TX_PATH] = value;
> > > > +     else if (strstr(kcontrol->id.name, "Client Channels"))
> > > > +             i2s->client_ch_override = value;
> > > > +     else if (strstr(kcontrol->id.name, "Capture Stereo To Mono"))
> > > > +             i2s->stereo_to_mono[I2S_TX_PATH] = value;
> > > > +     else if (strstr(kcontrol->id.name, "Capture Mono To Stereo"))
> > > > +             i2s->mono_to_stereo[I2S_TX_PATH] = value;
> > > > +     else if (strstr(kcontrol->id.name, "Playback Stereo To Mono"))
> > > > +             i2s->stereo_to_mono[I2S_RX_PATH] = value;
> > > > +     else if (strstr(kcontrol->id.name, "Playback Mono To Stereo"))
> > > > +             i2s->mono_to_stereo[I2S_RX_PATH] = value;
> > > > +     else if (strstr(kcontrol->id.name, "Playback FIFO Threshold"))
> > > > +             i2s->rx_fifo_th = value;
> > > > +     else if (strstr(kcontrol->id.name, "BCLK Ratio"))
> > > > +             i2s->bclk_ratio = value;
> > > I'm pretty sure that checkpatch should complain about the missing
> > > brackets, they should make code's indentation uniform and thus easier to
> > > read. Same for all other occurrences in the code.
> > same as above
> CHECK: braces {} should be used on all arms of this statement
> #2699: FILE: sound/soc/tegra/tegra210_i2s.c:368:
> +       if (strstr(kcontrol->id.name, "Loopback")) {
> [...]
> +       } else if (strstr(kcontrol->id.name, "Sample Rate"))
> [...]
> +       else if (strstr(kcontrol->id.name, "FSYNC Width")) {
> [...]
> +       } else if (strstr(kcontrol->id.name, "Playback Audio Bit Format"))
> [...]
> +       else if (strstr(kcontrol->id.name, "Capture Audio Bit Format"))
> [...]
> +       else if (strstr(kcontrol->id.name, "Client Bit Format"))
> [...]
> +       else if (strstr(kcontrol->id.name, "Playback Audio Channels"))
> [...]
> +       else if (strstr(kcontrol->id.name, "Capture Audio Channels"))
> [...]
> +       else if (strstr(kcontrol->id.name, "Client Channels"))
> [...]
> +       else if (strstr(kcontrol->id.name, "Capture Stereo To Mono"))
> [...]
> +       else if (strstr(kcontrol->id.name, "Capture Mono To Stereo"))
> [...]
> +       else if (strstr(kcontrol->id.name, "Playback Stereo To Mono"))
> [...]
> +       else if (strstr(kcontrol->id.name, "Playback Mono To Stereo"))
> [...]
> +       else if (strstr(kcontrol->id.name, "Playback FIFO Threshold"))
> [...]
> +       else if (strstr(kcontrol->id.name, "BCLK Ratio"))
> [...]
>
> [snip]

will fix
> > > > +     pm_runtime_enable(dev);
> > > Error checking?
> > return type for above is void()
> Ok
>
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int tegra210_i2s_remove(struct platform_device *pdev)
> > > > +{
> > > > +     pm_runtime_disable(&pdev->dev);
> > > > +     if (!pm_runtime_status_suspended(&pdev->dev))
> > > > +             tegra210_i2s_runtime_suspend(&pdev->dev);
> > > This breaks device's RPM refcounting if it was disabled in the active
> > > state. This code should be removed. At most you could warn about the
> > > unxpected RPM state here, but it shouldn't be necessary.
> > I guess this was added for safety and explicit suspend keeps clock
> > disabled.
> > Not sure if ref-counting of the device matters when runtime PM is
> > disabled and device is removed.
> > I see few drivers using this way.
> It should matter (if I'm not missing something) because RPM should be in
> a wrecked state once you'll try to re-load the driver's module. Likely
> that those few other drivers are wrong.
>
> [snip]

Once the driver is re-loaded and RPM is enabled, I don't think it would use
the same 'dev' and the corresponding ref count. Doesn't it use the new 
counters?
If RPM is not working for some reason, most likely it would be the case 
for other
devices. What best driver can do is probably do a force suspend during 
removal if
already not done. I would prefer to keep, since multiple drivers still 
have it,
unless there is a real harm in doing so.
> > > > +     int rx_fifo_th;
> > > Could rx_fifo_th be negative?
> > rx_fifo_th itself does not take negative values, explicit typecasting> is avoided in "if" condition by declaring this as "int"
> Explicit typecasting isn't needed for integers.

What I meant was, rx_fifo_th is checked against a 'int' variable in an 
"if" condition.