Re: [PATCH 09/12] irqchip: cirrus: Add driver for Cirrus Logic CS48L31/32/33 codecs

[Marc Zyngier <maz@xxxxxxxxxx>]

On Thu, 10 Nov 2022 20:36:16 +0000,
Mark Brown <broonie@xxxxxxxxxx> wrote:
> 
> On Thu, Nov 10, 2022 at 06:47:20PM +0000, Marc Zyngier wrote:
> 
> > Read again what I have written. Having to expose a device-specific API
> > for endpoint drivers to obtain their interrupts, and requiring them to
> > know about some magic values that describe the interrupts source are
> > not a acceptable constructs.
> 
> > We have firmware descriptions to expose interrupt linkages, and your
> > HW is not special enough to deserve its own top level API. Yes, we
> > accepted such drivers in the past, but it has to stop.
> 
> > Either you describe the internal structure of your device in DT or
> > ACPI, and make all client drivers use the standard API, or you make
> > this a codec library, purely specific to your device and only used by
> > it. But the current shape is not something I'm prepared to accept.
> 
> ACPI gets to be a lot of fun here, it's just not idiomatic to describe
> the internals of these devices in firmware there and a lot of the
> systems shipping this stuff are targeted at other OSs and system
> integrators are therefore not in the least worried about Linux
> preferences.

Let me reassure the vendors that I do not care about them either. By
this standard, we'd all run Windows on x86.

> You'd need to look at having the MFD add additional
> description via swnode or something to try to get things going.  MFD
> does have support for that, though it's currently mainly used with
> devices that only have ACPI use (axp20x looks like the only potentially
> DT user, from the git history the swnode bits are apparently for use on
> ACPI systems).  That might get fragile in the DT case since you could
> have multiple sources for description of the same thing unless you do
> something like suppress the swnode stuff on DT systems.
> 
> Given that swnode is basically DT written out in C code I'm not actually
> convinced it's that much of a win, unless someone writes some tooling to
> generate swnode data from DT files you're not getting the benefit of any
> of the schema validation work that's being done.  We'd also need to do
> some work for regulators to make sure that if we are parsing DT
> properties on ACPI systems we don't do so from _DSD since ACPI has
> strong ideas about how power works and we don't want to end up with
> systems with firmware providing mixed ACPI/DT models without a clear
> understanding of what we're geting into.
> 
> I do also have other concerns in the purely DT case, especially with
> chip functions like the CODEC where there's a very poor mapping between
> physical IPs and how Linux is tending to describe things internally at
> the minute.  In particular these devices often have a clock tree
> portions of which can be visible and useful off chip but which tends to
> get lumped in with the audio IPs in our current code.  Ideally we'd
> describe that as a clock subdevice (or subdevices if that fits the
> hardware) using the clock bindings but then that has a bunch of knock on
> effects the way the code currently is which probably it's probably
> disproportionate to force an individual driver author to work through.
> OTOH the DT bindings should be OS neutral ABI so...

I don't think this is a reason to continue on the current path that
pretends to have something generic, but instead is literally a board
file fragment with baked-in magic numbers.

An irqchip is supposed to offer services to arbitrary clients
(endpoint drivers) that are oblivious of the irqchip itself, of the
hwirq mapping, and use the standard APIs to obtain a virtual interrupt
number. None of that here. This is a monolithic driver, only split
across multiple subsystem to satisfy a "not in my backyard"
requirement.

If the vendors/authors want to keep the shape of the code as is, they
can do it outside of the irqchip code and have some library code with
an internal API. At least they will stop pretending that this is a
general purpose driver. And the existing madera code can also go in
the process.

	M.

-- 
Without deviation from the norm, progress is not possible.