RE: [PATCH] ASoC/HDA cs35l56: Remove redundant IRQ handling

[Simon Trimmer <simont@xxxxxxxxxxxxxxxxxxxxx>]

On Wed, Sep 04, 2024 at 18:27:00PM +0000, Mark Brown wrote:
> On Wed, Sep 04, 2024 at 02:46:30PM +0100, Simon Trimmer wrote:
> > On Wed, Sep 04, 2024 at 12:25:00PM +0000, Mark Brown wrote:
> > > On Wed, Sep 04, 2024 at 12:07:00PM +0000, Simon Trimmer wrote:
> 
> > > > The IRQ handling in the cs35l56 driver was purely informational. It
was
> > > > not necessary to support the HDA or ASoC driver functionality and
added
> > > > unnecessary complexity to the drivers.
> 
> > > Given that the code is there now and has been since the driver was
> > > introduced about 18 months ago what's the ongoing cost of having it?
> > > The information it's providing is notification of hardware faults,
> > > reporting those does seem useful.
> 
> > Originally we were expecting to use the IRQ mechanism for an event
logging
> > stream that would function in a similar manner to compressed streams to
be
> > able to get an information feed for debug and tuning tools, but those
were
> > never created and the logging infrastructure not implemented.
> 
> Right.  Though ideally we might actually do something about some of
> the errors that are reported.
>
> > It's quite a spread of code and a lot of complexity in the regular
execution
> > paths managing them / synchronizing the contexts, there is more going on
in
> > the SoundWire bus variant compared to the conventional i2c/spi that it
is
> > hard to justify maintaining it all for a couple of log messages - in the
> > event that someone did encounter the two situations being reported the
> > regmap dump would point us to the cause pretty quickly.
> 
> I'm not sure how many end users are going to get as far as talking to
> you in the event that they have issues - people often won't get as far
> as trying to contact their distros or upstreams.  Even errors in dmesg
> are pretty obscure but they're more discoverable than interpreting the
> regmap, people would need to identify that they need to look at the chip
> first and actually be experiencing the problem when they figure that
> out.  Ideally we'd hae better handling for this (I did note that the
> latest Iron Devices driver will back off speaker volume during a thermal
> warning which isn't a terrible idea, though there's potential issues).

I'll have a chat to people when they are back at work - the IRQ signals we
were logging were from the firmware rather than the hardware as it entered a
protection state, this isn't fatal and can clear. Over the last few years I
don't think I've encountered them in testing except for when I deliberately
abused the hardware which I don't think anyone would recommend :)

How people behave when encountering trouble is certainly true - as more PCs
adopt our devices we have a steadily increasing stream of direct support
messages and have been getting tagged in distro issue reports. These have
been universally around missing support in particular kernel versions or the
linux-firmware package not containing the necessary tuning files, so far
I've only asked our field/partner engineers for a dump of a regmap and I've
not seen a report of a hardware warning yet but I too suspect these are the
tip of the iceberg for people encountering troubles.

A general Linux tangent - we had been debating how many of our end users
multiboot their hardware vs those who just run Linux on them - has anyone
seen any reports or statistics in this area?

> It sounds like the only real concern is the Soundwire stuff (I2C and SPI
> interrupt stuff should generally be trivial?) - if that's the case I'd
> be more inclined to only pull out the Soundwire bits and leave the
> support there for the simpler buses.

That is a good idea and would certainly make things better - I'll see what I
can do

-Simon