Re: [alsa-devel] [RFC PATCH 1/7] soundwire: Add sysfs support for master(s)

[Greg KH]

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?

thanks,

greg k-h