Re: [PATCH 1/3] ALSA: hda/tegra: Skip reset on BPMP devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 12/7/2021 2:06 PM, Takashi Iwai wrote:
On Tue, 07 Dec 2021 09:16:43 +0100,
Thierry Reding wrote:
I suppose this could also be a bool. Not sure if we need to care about
packing optimizations at this point.

It may also be useful to rename this to something less generic to avoid
potential clashes with other data structures in the future. We've often
used the _soc suffix in other drivers to mark this kind of SoC-specific
data. In this case it would be struct hda_tegra_soc.

If Takashi is fine with this as-is, I don't have any strong objections,
though.
Indeed, a bit more prefix would be better for avoiding the possible
conflict in future, but the struct name is local, so I don't mind to
use the simple name for now.  We can change it later once when needed,
too.

[...]


One other thing we've done in the past is to explicitly pass these
structures for each compatible string. That simplifies things a bit
because we don't have to keep checking for non-NULL pointers and instead
rely on the fact that there's always a valid pointer.

To do so, you'd basically add:

       static const struct hda_data tegra186_data = {
               .do_reset = 0,
       };

And reference that for both the Tegra186 and Tegra194 entries. Again,
not strictly necessary and since we have only one occurrence where we
need to check this, it seems fine as-is, so:

Acked-by: Thierry Reding <treding@xxxxxxxxxx>
That's true, too.  OTOH, completely without a NULL check would be also
unsafe, so some sanity check would be still required.

That said, the current patch is good enough for taking as a regression
fix, but I'm fine to wait for a while for v2 to address those, too :)


Let me send a v2 for above.



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux