On Wed, May 08, 2019 at 11:42:15AM -0500, Pierre-Louis Bossart wrote:
On 5/8/19 4:16 AM, Greg KH wrote:
On Wed, May 08, 2019 at 01:16:06PM +0530, Vinod Koul wrote:
On 07-05-19, 17:49, Pierre-Louis Bossart wrote:
The model here is that Master device is PCI or Platform device and then creates a bus instance which has soundwire slave devices.
So for any attribute on Master device (which has properties as well and representation in sysfs), device specfic struct (PCI/platfrom doesn't help). For slave that is not a problem as sdw_slave structure takes care if that.
So, the solution was to create the psedo sdw_master device for the representation and have device-specific structure.
Ok, much like the "USB host controller" type device. That's fine, make such a device, add it to your bus, and set the type correctly. And keep a pointer to that structure in your device-specific structure if you really need to get to anything in it.
humm, you lost me on the last sentence. Did you mean using set_drv/platform_data during the init and retrieving the bus information with get_drv/platform_data as needed later? Or something else I badly need to learn?
IIUC Greg meant we should represent a soundwire master device type and use that here. Just like we have soundwire slave device type. Something like:
struct sdw_master { struct device dev; struct sdw_master_prop *prop; ... };
In show function you get master from dev (container of) and then use that to access the master properties. So int.sdw.0 can be of this type.
Yes, you need to represent the master device type if you are going to be having an internal representation of it.
Humm, confused...In the existing code bus and master are synonyms, see e.g. following code excerpts:
- sdw_add_bus_master() - add a bus Master instance
- @bus: bus instance
- Initializes the bus instance, read properties and create child
- devices.
struct sdw_bus { struct device *dev; <<< pointer here
That's the pointer to what? The device that the bus is "attached to" (i.e. parent, like a platform device or a pci device)?
Why isn't this a "real" device in itself?
I thought I asked that a long time ago when first reviewing these patches...
unsigned int link_id; struct list_head slaves; DECLARE_BITMAP(assigned, SDW_MAX_DEVICES); struct mutex bus_lock; struct mutex msg_lock; const struct sdw_master_ops *ops; const struct sdw_master_port_ops *port_ops; struct sdw_bus_params params; struct sdw_master_prop prop;
The existing code creates a platform_device in drivers/soundwire/intel_init.c, and it's assigned by the following code:
The core creates a platform device, don't assume you can "take it over" :)
That platform device lives on the platform bus, you need a "master" device that lives on your soundbus bus.
Again, look at how USB does this. Or better yet, greybus, as that code is a lot smaller and simpler.
static int intel_probe(struct platform_device *pdev) { struct sdw_cdns_stream_config config; struct sdw_intel *sdw; int ret;
sdw = devm_kzalloc(&pdev->dev, sizeof(*sdw), GFP_KERNEL); [snip] sdw->cdns.dev = &pdev->dev; sdw->cdns.bus.dev = &pdev->dev;
Gotta love the lack of reference counting :(
I really don't see what you are hinting at, sorry, unless we are talking about major surgery in the code.
It sounds like you need a device on your bus that represents the master, as you have attributes associated with it, and other things. You can't put attributes on a random pci or platform device, as you do not "own" that device.
does that help?
greg k-h