Re: [PATCH] soundwire: intel: move to auxiliary bus

[Vinod Koul]

On 23-03-21, 12:29, Pierre-Louis Bossart wrote:
> Thanks Greg and Vinod for the reviews
> 
> > > > -static int intel_master_probe(struct platform_device *pdev)
> > > > +static int intel_link_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)
> > > >   {
> > > > -	struct device *dev = &pdev->dev;
> > > > +	struct device *dev = &auxdev->dev;
> > > > +	struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev);
> > > 
> > > Do we need another abstractions for resources here, why not aux dev
> > > creation set the resources required and we skip this step...
> 
> Not sure what resources you are referring to?

Resources in the sdw_intel_link_dev which are sdw_intel_link_res. They
should be resources for auxdev and if you do that lets you get rid of
this abstraction.

> 
> this is just a container_of() and the documented way of extending the auxbus
> (see https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#example-usage)
> 
> 
> struct sdw_intel_link_dev {
> 	struct auxiliary_device auxdev;
> 	struct sdw_intel_link_res link_res;
> };
> 
> #define auxiliary_dev_to_sdw_intel_link_dev(auxiliary_dev) \
> 	container_of(auxiliary_dev, struct sdw_intel_link_dev, auxdev)
> 
> > > >   	struct sdw_intel *sdw;
> > > >   	struct sdw_cdns *cdns;
> > > >   	struct sdw_bus *bus;
> > > > @@ -1346,14 +1347,14 @@ static int intel_master_probe(struct platform_device *pdev)
> > > >   	cdns = &sdw->cdns;
> > > >   	bus = &cdns->bus;
> > > > -	sdw->instance = pdev->id;
> > > > -	sdw->link_res = dev_get_platdata(dev);
> > > > +	sdw->instance = auxdev->id;
> > > 
> > > so auxdev has id and still we pass id as argument :( Not sure if folks
> > > can fix this now
> > 
> > That's odd, yeah, it should be fixed.
> 
> I think we are talking about different things?
> 
> this is defined in mod_devicetable.h:
> 
> struct auxiliary_device_id {
> 	char name[AUXILIARY_NAME_SIZE];
> 	kernel_ulong_t driver_data;
> };
> 
> and used for the driver probe:
> 
> 	ret = auxdrv->probe(auxdev, auxiliary_match_id(auxdrv->id_table, auxdev));
> 
> but there is a separate id:
> 
> struct auxiliary_device {
> 	struct device dev;
> 	const char *name;
> 	u32 id;
> };
> 
> which is set during the device initialization in intel_init.c
> 
> 	/* we don't use an IDA since we already have a link ID */
> 	auxdev->id = link_id;
> 
> In the auxiliary bus design, the parent has to take care of managing this
> id, be it with an IDA or as we do here with a physical link ID that is
> unique.

Aha, maybe both of them should not be 'id' to avoid this confusion!

That also reminds me that we have duplicate info:

+       sdw->instance = auxdev->id;
+       bus->link_id = auxdev->id;

drop the local driver instance and use bus->link_id please

> in short, I don't see how I could change the code given the differences in
> concepts?

-- 
~Vinod