b) Vinod commented:
"What I would like to see the end result is that sdw driver for Intel
controller here is a simple auxdev device and no additional custom setup
layer required... which implies that this handling should be moved into
auxdev or Intel code setting up auxdev..."
I was unable to figure out what this comment hinted at: the auxbus is
already handled in the intel_init.c and intel.c files and the auxbus is used
to model a set of links/managers below the PCI device, not the controller
itself. There is also no such thing as a simple auxdev device used in the
kernel today, the base layer is meant to be extended with domain-specific
structures. There is really no point in creating a simple auxbus device
without extensions.
<back from vacations>
same here :-)
I would like to see that the init_init.c removed completely, that is my
ask here
This layer was created by me to aid in creating the platform devices.
Also the mistake was not to use platform resources and instead pass a
custom structure for resources (device iomem address, irq etc)
We are 100% aligned on the ask to remove intel_init.c, this layer is
unnecessary and adds more work for developers/maintainers. We will move
all this in the SOF driver.
I would like to see is the PCI/SOF parent driver create the sdw aux
device and that should be all needed to be done. The aux device would be
probed by sdw driver. No custom resource structs for resources please.
I was following the previous paragraph but got stuck on the last
sentence 'no custom structs for resources', see below.
If that is not possible, I would like to understand technical details of
why that would be that case. If required necessary changes should be
made to aux bus to handle and not have sequencing issue which you had
trouble with platform approach.
I don't know what you are referring to with the 'sequencing issue which
you had trouble with platform approach'. We never had any technical
issues with platform devices, the solution works and has been
productized. We are only doing this iso-functionality transition because
GregKH asked us to do only use platform devices IF there is a real
platform device (controlled by DT/ACPI).
I think we are also having language/specification issues here. I don't
understand what you describe as a 'resource' - there is no interaction
with firmware - nor how we can avoid being domain-specific for something
that is Intel-specific.
Let's go back to the code to help the discussion: the auxiliary driver
which manages a SoundWire link needs to be provided with a 'custom'
structure that describes basic information provided by the PCI parent
(link masks, quirks, IO register bases) and contains internal fields
needed for the link management (mutex, ops, list, etc). This is the
structure we use:
struct sdw_intel_link_res {
void __iomem *mmio_base; /* not strictly needed, useful for debug */
void __iomem *registers;
void __iomem *shim;
void __iomem *alh;
int irq;
const struct sdw_intel_ops *ops;
struct device *dev;
struct mutex *shim_lock; /* protect shared registers */
u32 *shim_mask;
u32 clock_stop_quirks;
u32 link_mask;
struct sdw_cdns *cdns;
struct list_head list;
};
We could if it was desired for architectural clarity split this
structure in what is provided by the parent and what is used inside of
the auxiliary driver as an internal context that the parent doesn't
touch, but these definitions are again Intel-specific.
Then both types of information are included in the 'link_dev' extension
of the auxiliary device.
struct sdw_intel_link_dev {
struct auxiliary_device auxdev;
struct sdw_intel_link_res link_res;
};
That's the basic design of the auxiliary bus, domain-specific data
structures are not added inside of the auxiliary_device but are part of
an extension accessed with container_of(). That's what everyone using
the auxiliary bus is doing.
Vinod, if you can elaborate on what 'resources' refer to in your reply
that would help. We've been using the same approach as others relying on
the auxiliary bus and I am struggling to see what is wrong with the
solution we suggested, or what changes to the auxiliary bus core would
be needed. I don't mind doing something different but I just don't
understand what the suggestion is.
Thanks!
