Re: [alsa-devel] [PATCH v5 09/17] soundwire: intel: remove platform devices and use 'Master Devices' instead

[Vinod Koul]

On 10-01-20, 10:08, Pierre-Louis Bossart wrote:
> 
> > > > The "big" difference is that probe is called by core (asoc) and not by
> > > > driver onto themselves.. IMO that needs to go away.
> > > 
> > > What I did is not different from what existed already with platform devices.
> > > They were manually created, weren't they?
> > 
> > Manual creation of device based on a requirement is different, did I ask
> > you why you are creating device :)
> > 
> > I am simple asking you not to call probe in the driver. If you need
> > that, move it to core! We do not want these kind of things in the
> > drivers...
> 
> What core are you talking about?

soundwire core ofcourse! IMO All that which goes into soundwire-bus-objs is
considered as soundwire core part and rest are drivers intel, qc, so on!
> 
> The SOF intel driver needs to create a device, which will then be bound with
> a SoundWire master driver.
> 
> What I am doing is no different from what your team did with
> platform_register_device, I am really lost on what you are asking.

Again repeating myself, you call an API to do that is absolutely fine,
but we don't do that in drivers or open code these things

> > > > > FWIW, the implementation here follows what was suggested for Greybus 'Host
> > > > > Devices' [1] [2], so it's not like I am creating any sort of dangerous
> > > > > precedent.
> > > > > 
> > > > > [1]
> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/greybus/es2.c#L1275
> > > > > [2] https://elixir.bootlin.com/linux/latest/source/drivers/greybus/hd.c#L124
> > > > 
> > > > And if you look closely all this work is done by core not by drivers!
> > > > Drivers _should_ never do all this, it is the job of core to do that for
> > > > you.
> > > 
> > > Please look at the code again, you have a USB probe that will manually call
> > > the GreyBus device creation.
> > > 
> > > static int ap_probe(struct usb_interface *interface,
> > > 		    const struct usb_device_id *id)
> > > {
> > > 	hd = gb_hd_create(&es2_driver, &udev->dev, 	
> > > 
> > > 
> > > static struct usb_driver es2_ap_driver = {
> > > 	.name =		"es2_ap_driver",
> > > 	.probe =	ap_probe, <<< code above
> > > 	.disconnect =	ap_disconnect,
> > > 	.id_table =	id_table,
> > > 	.soft_unbind =	1,
> > > };
> > 
> > Look closely the driver es2 calls into greybus core hd.c and gets the
> > work done, subtle but a big differances in the approaches..
> 
> I am sorry, I have absolutely no idea what you are referring to.
> 
> The code I copy/pasted here makes no call to the greybus core, it's ap_probe
> -> gb_hd_create. No core involved. If I am mistaken, please show me what I
> got wrong.

1. es2_ap_driver is host controller driver

2. gb_hd_create() is an API provided by greybus core!

es2 driver doesn't open code creation like you are doing in intel driver,
it doesn't call probe on its own, greybus does that

This is very common pattern in linux kernel subsytems, drivers dont do
these things, the respective subsystem core does that... see about es2
driver and implementation of gb_hd_create(). See callers of
platform_register_device() and its implementation.

I don't know how else I can explain this to you, is something wrong in
how I conveyed this info or you... or something else, I dont know!!!

-- 
~Vinod