On Tue, May 07, 2019 at 04:33:31PM +0530, Vinod Koul wrote:
On 07-05-19, 07:54, Greg KH wrote:
On Tue, May 07, 2019 at 10:57:32AM +0530, Vinod Koul wrote:
On 06-05-19, 21:24, Pierre-Louis Bossart wrote:
+int sdw_sysfs_bus_init(struct sdw_bus *bus) +{
- struct sdw_master_sysfs *master;
- int err;
- if (bus->sysfs) {
dev_err(bus->dev, "SDW sysfs is already initialized\n");return -EIO;- }
- master = kzalloc(sizeof(*master), GFP_KERNEL);
- if (!master)
return -ENOMEM;Why are you creating a whole new device to put all of this under? Is this needed? What will the sysfs tree look like when you do this? Why can't the "bus" device just get all of these attributes and no second device be created?
I tried a quick hack and indeed we could simplify the code with something as simple as:
[attributes omitted]
static const struct attribute_group sdw_master_node_group = { .attrs = master_node_attrs, .name = "mipi-disco" };
int sdw_sysfs_bus_init(struct sdw_bus *bus) { return sysfs_create_group(&bus->dev->kobj, &sdw_master_node_group); }
void sdw_sysfs_bus_exit(struct sdw_bus *bus) { sysfs_remove_group(&bus->dev->kobj, &sdw_master_node_group); }
which gives me a simpler structure and doesn't require additional pretend-devices:
/sys/bus/acpi/devices/PRP00001:00/int-sdw.0/mipi-disco# ls clock_gears /sys/bus/acpi/devices/PRP00001:00/int-sdw.0/mipi-disco# more clock_gears 8086
The issue I have is that for the _show() functions, I don't see a way to go from the device argument to bus. In the example above I forced the output but would need a helper.
static ssize_t clock_gears_show(struct device *dev, struct device_attribute *attr, char *buf) { struct sdw_bus *bus; // this is what I need to find from dev ssize_t size = 0; int i;
return sprintf(buf, "%d \n", 8086); }
my brain is starting to fry, but I don't see how container_of() would work here since the bus structure contains a pointer to the device. I don't also see a way to check for all devices for the bus_type soundwire. For the slaves we do have a macro based on container_of(), so wondering if we made a mistake in the bus definition? Vinod, any thoughts?
yeah I dont recall a way to get bus fed into create_group, I did look at the other examples back then and IIRC and most of them were using a global to do the trick (I didn't want to go down that route).
I think that was the reason I wrote it this way...
BTW if you do use psedo-device you can create your own struct foo which embeds device and then then you can use container approach to get foo (and foo contains bus as a member).
Greg, any thoughts?
Why would you have "bus" attributes on a device? I don't think you are using "bus" here like the driver model uses the term "bus", right?
What are you really trying to show here?
And if you need to know the bus pointer from the device, why don't you have a pointer to it in your device-specific structure?
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.
thanks,
greg k-h