Message-ID: <Y21gwGDb5CFft0kp@sirena.org.uk>
Date:   Thu, 10 Nov 2022 20:36:16 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Marc Zyngier <maz@...nel.org>
Cc:     Richard Fitzgerald <rf@...nsource.cirrus.com>, lee@...nel.org,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        linus.walleij@...aro.org, tglx@...utronix.de,
        alsa-devel@...a-project.org, devicetree@...r.kernel.org,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        patches@...nsource.cirrus.com
Subject: Re: [PATCH 09/12] irqchip: cirrus: Add driver for Cirrus Logic
 CS48L31/32/33 codecs

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.  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...

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists