| Message ID | 20190504010030.29233-2-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: add sysfs and debugfs support | expand |
On Fri, May 03, 2019 at 08:00:24PM -0500, Pierre-Louis Bossart wrote: > For each master N, add a device sdw-master:N and add the > master properties as attributes. > > Credits: this patch is based on an earlier internal contribution by > Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/Makefile | 3 +- > drivers/soundwire/bus.c | 6 ++ > drivers/soundwire/sysfs.c | 162 ++++++++++++++++++++++++++++++++++ > include/linux/soundwire/sdw.h | 10 +++ > 4 files changed, 180 insertions(+), 1 deletion(-) > create mode 100644 drivers/soundwire/sysfs.c > > diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile > index 5817beaca0e1..787f1cbf342c 100644 > --- a/drivers/soundwire/Makefile > +++ b/drivers/soundwire/Makefile > @@ -3,7 +3,8 @@ > # > > #Bus Objs > -soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o > +soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \ > + sysfs.o > obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o > > #Cadence Objs > diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c > index fe745830a261..38de7071e135 100644 > --- a/drivers/soundwire/bus.c > +++ b/drivers/soundwire/bus.c > @@ -49,6 +49,10 @@ int sdw_add_bus_master(struct sdw_bus *bus) > } > } > > + ret = sdw_sysfs_bus_init(bus); > + if (ret < 0) > + dev_warn(bus->dev, "Bus sysfs init failed:%d\n", ret); > + > /* > * Device numbers in SoundWire are 0 through 15. Enumeration device > * number (0), Broadcast device number (15), Group numbers (12 and > @@ -129,6 +133,8 @@ static int sdw_delete_slave(struct device *dev, void *data) > */ > void sdw_delete_bus_master(struct sdw_bus *bus) > { > + sdw_sysfs_bus_exit(bus); > + > device_for_each_child(bus->dev, NULL, sdw_delete_slave); > } > EXPORT_SYMBOL(sdw_delete_bus_master); > diff --git a/drivers/soundwire/sysfs.c b/drivers/soundwire/sysfs.c > new file mode 100644 > index 000000000000..7b6c3826a73a > --- /dev/null > +++ b/drivers/soundwire/sysfs.c > @@ -0,0 +1,162 @@ > +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) > +// Copyright(c) 2015-19 Intel Corporation. > + > +#include <linux/device.h> > +#include <linux/mod_devicetable.h> > +#include <linux/slab.h> > +#include <linux/sysfs.h> > +#include <linux/soundwire/sdw.h> > +#include <linux/soundwire/sdw_type.h> > +#include "bus.h" > + > +struct sdw_master_sysfs { > + struct device dev; > + struct sdw_bus *bus; > +}; > + > +#define to_sdw_device(_dev) \ > + container_of(_dev, struct sdw_master_sysfs, dev) > + > +/* > + * The sysfs for properties reflects the MIPI description as given > + * in the MIPI DisCo spec > + * > + * Base file is: > + * sdw-master-N > + * |---- revision > + * |---- clk_stop_modes > + * |---- max_clk_freq > + * |---- clk_freq > + * |---- clk_gears > + * |---- default_row > + * |---- default_col > + * |---- dynamic_shape > + * |---- err_threshold > + */ > + > +#define sdw_master_attr(field, format_string) \ > +static ssize_t field##_show(struct device *dev, \ > + struct device_attribute *attr, \ > + char *buf) \ > +{ \ > + struct sdw_master_sysfs *master = to_sdw_device(dev); \ > + return sprintf(buf, format_string, master->bus->prop.field); \ > +} \ > +static DEVICE_ATTR_RO(field) > + > +sdw_master_attr(revision, "0x%x\n"); > +sdw_master_attr(clk_stop_modes, "0x%x\n"); > +sdw_master_attr(max_clk_freq, "%d\n"); > +sdw_master_attr(default_row, "%d\n"); > +sdw_master_attr(default_col, "%d\n"); > +sdw_master_attr(default_frame_rate, "%d\n"); > +sdw_master_attr(dynamic_frame, "%d\n"); > +sdw_master_attr(err_threshold, "%d\n"); > + > +static ssize_t clock_frequencies_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct sdw_master_sysfs *master = to_sdw_device(dev); > + ssize_t size = 0; > + int i; > + > + for (i = 0; i < master->bus->prop.num_clk_freq; i++) > + size += sprintf(buf + size, "%8d ", > + master->bus->prop.clk_freq[i]); > + size += sprintf(buf + size, "\n"); > + > + return size; > +} > +static DEVICE_ATTR_RO(clock_frequencies); > + > +static ssize_t clock_gears_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct sdw_master_sysfs *master = to_sdw_device(dev); > + ssize_t size = 0; > + int i; > + > + for (i = 0; i < master->bus->prop.num_clk_gears; i++) > + size += sprintf(buf + size, "%8d ", > + master->bus->prop.clk_gears[i]); > + size += sprintf(buf + size, "\n"); > + > + return size; > +} > +static DEVICE_ATTR_RO(clock_gears); > + > +static struct attribute *master_node_attrs[] = { > + &dev_attr_revision.attr, > + &dev_attr_clk_stop_modes.attr, > + &dev_attr_max_clk_freq.attr, > + &dev_attr_default_row.attr, > + &dev_attr_default_col.attr, > + &dev_attr_default_frame_rate.attr, > + &dev_attr_dynamic_frame.attr, > + &dev_attr_err_threshold.attr, > + &dev_attr_clock_frequencies.attr, > + &dev_attr_clock_gears.attr, > + NULL, > +}; > + > +static const struct attribute_group sdw_master_node_group = { > + .attrs = master_node_attrs, > +}; > + > +static const struct attribute_group *sdw_master_node_groups[] = { > + &sdw_master_node_group, > + NULL > +}; Minor nit, you can use the ATTRIBUTE_GROUPS() macro here to save you a few lines. > + > +static void sdw_device_release(struct device *dev) > +{ > + struct sdw_master_sysfs *master = to_sdw_device(dev); > + > + kfree(master); > +} > + > +static struct device_type sdw_device_type = { > + .name = "sdw_device", > + .release = sdw_device_release, > +}; > + > +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? thanks, greg k-h
Thanks for the quick feedback Greg! >> +static const struct attribute_group sdw_master_node_group = { >> + .attrs = master_node_attrs, >> +}; >> + >> +static const struct attribute_group *sdw_master_node_groups[] = { >> + &sdw_master_node_group, >> + NULL >> +}; > > Minor nit, you can use the ATTRIBUTE_GROUPS() macro here to save you a > few lines. will do. >> + >> +static void sdw_device_release(struct device *dev) >> +{ >> + struct sdw_master_sysfs *master = to_sdw_device(dev); >> + >> + kfree(master); >> +} >> + >> +static struct device_type sdw_device_type = { >> + .name = "sdw_device", >> + .release = sdw_device_release, >> +}; >> + >> +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? This is indeed my main question on this code (see cover letter) and why I tagged the series as RFC. I find it odd to create an int-sdw.0 platform device to model the SoundWire master, and a sdw-master:0 device whose purpose is only to expose the properties of that master. it'd be simpler if all the properties were exposed one level up. Vinod and Sanyog might be able to shed some light on this?
>> +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?
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? Thanks
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
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. Thanks
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
>> 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?
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. Thanks
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. thanks, greg k-h
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 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: 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; I really don't see what you are hinting at, sorry, unless we are talking about major surgery in the code.
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
On 5/8/19 11:59 AM, Greg KH wrote: > 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? Allow me to provide a bit of background. I am not trying to be pedantic but make sure we are on the same page. The SoundWire spec only defines a Master and Slaves attached to that Master. In real applications, there is a need to have multiple links, which can possibly operate in synchronized ways, so Intel came up with the concept of Controller, which expose multiple Master interfaces that are in sync (two streams can start at exactly the same clock edge of different links). The Controller is exposed in ACPI as a child of the HDAudio controller (ACPI companion of a PCI device). The controller exposes a 'master-count' and a set of link-specific properties needed for bandwidth/clock scaling. For some reason, our Windows friends did not want to have a device for each Master interface, likely because they did not want to load a driver per Master interface or have 'yellow bangs'. So the net result is that we have the following hierarchy in ACPI Device(HDAS) // HDaudio controller Device(SNDW) // SoundWire Controller Device(SDW0) { // Slave0 _ADR(link0, vendorX, partY...) } Device(SDW1) { // Slave0 _ADR(link0, vendorX, partY...) } Device(SDW2) { // Slave0 _ADR(link1, vendorX, partY...) } Device(SDWM) { // Slave0 _ADR(linkM, vendorX, partY...) } There is no master device represented in ACPI and the only way by which we know to which Master a Slave device is attached by looking up the _ADR which contains the link information. So, coming back to the plot, when we parse the Controller properties, we find out how many Master interfaces we have, create a platform_device for each of them, then initialize all the bus stuff. > > 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. The learning curve is not small here... >> >> 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? Looks like we are doing things wrong at multiple levels. It might be better to have a more 'self-contained' solution where the bus initialization creates/registers a master device instead of having this proxy platform_device. That would avoid all these refcount issues and make the translation from device to bus straightforward. Am I on the right track or still in the weeds?
On 08-05-19, 15:57, Pierre-Louis Bossart wrote: > > > On 5/8/19 11:59 AM, Greg KH wrote: > > 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? Correct, I am revisiting this and I think I have a fair idea of expectations here (looking at usb and greybus model), will hack something up > Allow me to provide a bit of background. I am not trying to be pedantic but > make sure we are on the same page. > > The SoundWire spec only defines a Master and Slaves attached to that Master. > > In real applications, there is a need to have multiple links, which can > possibly operate in synchronized ways, so Intel came up with the concept of > Controller, which expose multiple Master interfaces that are in sync (two > streams can start at exactly the same clock edge of different links). > > The Controller is exposed in ACPI as a child of the HDAudio controller (ACPI > companion of a PCI device). The controller exposes a 'master-count' and a > set of link-specific properties needed for bandwidth/clock scaling. > > For some reason, our Windows friends did not want to have a device for each > Master interface, likely because they did not want to load a driver per > Master interface or have 'yellow bangs'. > > So the net result is that we have the following hierarchy in ACPI > > Device(HDAS) // HDaudio controller > Device(SNDW) // SoundWire Controller > Device(SDW0) { // Slave0 > _ADR(link0, vendorX, partY...) > } > Device(SDW1) { // Slave0 > _ADR(link0, vendorX, partY...) > } > Device(SDW2) { // Slave0 > _ADR(link1, vendorX, partY...) > } > Device(SDWM) { // Slave0 > _ADR(linkM, vendorX, partY...) > } > > There is no master device represented in ACPI and the only way by which we > know to which Master a Slave device is attached by looking up the _ADR which > contains the link information. > > So, coming back to the plot, when we parse the Controller properties, we > find out how many Master interfaces we have, create a platform_device for > each of them, then initialize all the bus stuff. So the idea here would be to go back and create a sdw_master device and use that in the bus instance. I think it should be doable.. > > I thought I asked that a long time ago when first reviewing these > > patches... Sorry my fault, I should have fixed it back then. > > > > > 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. > > The learning curve is not small here... > > > > > > > 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. Not really we have object here which should contain a real device for master and need plumbing for it.. > > 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? > > Looks like we are doing things wrong at multiple levels. > > It might be better to have a more 'self-contained' solution where the bus > initialization creates/registers a master device instead of having this > proxy platform_device. That would avoid all these refcount issues and make > the translation from device to bus straightforward. yes that is my thinking as well. We still need to link to platform/pci/whatever device you have and grab a refcount to that one. > Am I on the right track or still in the weeds?
On Wed, May 08, 2019 at 03:57:49PM -0500, Pierre-Louis Bossart wrote: > > > On 5/8/19 11:59 AM, Greg KH wrote: > > 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? > > Allow me to provide a bit of background. I am not trying to be pedantic but > make sure we are on the same page. > > The SoundWire spec only defines a Master and Slaves attached to that Master. > > In real applications, there is a need to have multiple links, which can > possibly operate in synchronized ways, so Intel came up with the concept of > Controller, which expose multiple Master interfaces that are in sync (two > streams can start at exactly the same clock edge of different links). > > The Controller is exposed in ACPI as a child of the HDAudio controller (ACPI > companion of a PCI device). The controller exposes a 'master-count' and a > set of link-specific properties needed for bandwidth/clock scaling. > > For some reason, our Windows friends did not want to have a device for each > Master interface, likely because they did not want to load a driver per > Master interface or have 'yellow bangs'. > > So the net result is that we have the following hierarchy in ACPI > > Device(HDAS) // HDaudio controller > Device(SNDW) // SoundWire Controller > Device(SDW0) { // Slave0 > _ADR(link0, vendorX, partY...) > } > Device(SDW1) { // Slave0 > _ADR(link0, vendorX, partY...) > } > Device(SDW2) { // Slave0 > _ADR(link1, vendorX, partY...) > } > Device(SDWM) { // Slave0 > _ADR(linkM, vendorX, partY...) > } > > There is no master device represented in ACPI and the only way by which we > know to which Master a Slave device is attached by looking up the _ADR which > contains the link information. > > So, coming back to the plot, when we parse the Controller properties, we > find out how many Master interfaces we have, create a platform_device for > each of them, then initialize all the bus stuff. So soundwire is creating platform devices? Ick, no! Don't do that, create a "real" device that is the root of your bus and attach that to the platform/pci/whatever device that is the parent of that device's resources. > > 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. > > The learning curve is not small here... kernel programming is hard. Writing a new bus subsystem is even harder :( > > > 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? > > Looks like we are doing things wrong at multiple levels. > > It might be better to have a more 'self-contained' solution where the bus > initialization creates/registers a master device instead of having this > proxy platform_device. That would avoid all these refcount issues and make > the translation from device to bus straightforward. Yes, that sounds better. Don't mess with a platform device unless you really have no other possible choice. And even then, don't do it and try to do something else. Platform devices are really abused, don't perpetuate it. thanks, greg k-h
diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile index 5817beaca0e1..787f1cbf342c 100644 --- a/drivers/soundwire/Makefile +++ b/drivers/soundwire/Makefile @@ -3,7 +3,8 @@ # #Bus Objs -soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o +soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \ + sysfs.o obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o #Cadence Objs diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index fe745830a261..38de7071e135 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -49,6 +49,10 @@ int sdw_add_bus_master(struct sdw_bus *bus) } } + ret = sdw_sysfs_bus_init(bus); + if (ret < 0) + dev_warn(bus->dev, "Bus sysfs init failed:%d\n", ret); + /* * Device numbers in SoundWire are 0 through 15. Enumeration device * number (0), Broadcast device number (15), Group numbers (12 and @@ -129,6 +133,8 @@ static int sdw_delete_slave(struct device *dev, void *data) */ void sdw_delete_bus_master(struct sdw_bus *bus) { + sdw_sysfs_bus_exit(bus); + device_for_each_child(bus->dev, NULL, sdw_delete_slave); } EXPORT_SYMBOL(sdw_delete_bus_master); diff --git a/drivers/soundwire/sysfs.c b/drivers/soundwire/sysfs.c new file mode 100644 index 000000000000..7b6c3826a73a --- /dev/null +++ b/drivers/soundwire/sysfs.c @@ -0,0 +1,162 @@ +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) +// Copyright(c) 2015-19 Intel Corporation. + +#include <linux/device.h> +#include <linux/mod_devicetable.h> +#include <linux/slab.h> +#include <linux/sysfs.h> +#include <linux/soundwire/sdw.h> +#include <linux/soundwire/sdw_type.h> +#include "bus.h" + +struct sdw_master_sysfs { + struct device dev; + struct sdw_bus *bus; +}; + +#define to_sdw_device(_dev) \ + container_of(_dev, struct sdw_master_sysfs, dev) + +/* + * The sysfs for properties reflects the MIPI description as given + * in the MIPI DisCo spec + * + * Base file is: + * sdw-master-N + * |---- revision + * |---- clk_stop_modes + * |---- max_clk_freq + * |---- clk_freq + * |---- clk_gears + * |---- default_row + * |---- default_col + * |---- dynamic_shape + * |---- err_threshold + */ + +#define sdw_master_attr(field, format_string) \ +static ssize_t field##_show(struct device *dev, \ + struct device_attribute *attr, \ + char *buf) \ +{ \ + struct sdw_master_sysfs *master = to_sdw_device(dev); \ + return sprintf(buf, format_string, master->bus->prop.field); \ +} \ +static DEVICE_ATTR_RO(field) + +sdw_master_attr(revision, "0x%x\n"); +sdw_master_attr(clk_stop_modes, "0x%x\n"); +sdw_master_attr(max_clk_freq, "%d\n"); +sdw_master_attr(default_row, "%d\n"); +sdw_master_attr(default_col, "%d\n"); +sdw_master_attr(default_frame_rate, "%d\n"); +sdw_master_attr(dynamic_frame, "%d\n"); +sdw_master_attr(err_threshold, "%d\n"); + +static ssize_t clock_frequencies_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct sdw_master_sysfs *master = to_sdw_device(dev); + ssize_t size = 0; + int i; + + for (i = 0; i < master->bus->prop.num_clk_freq; i++) + size += sprintf(buf + size, "%8d ", + master->bus->prop.clk_freq[i]); + size += sprintf(buf + size, "\n"); + + return size; +} +static DEVICE_ATTR_RO(clock_frequencies); + +static ssize_t clock_gears_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct sdw_master_sysfs *master = to_sdw_device(dev); + ssize_t size = 0; + int i; + + for (i = 0; i < master->bus->prop.num_clk_gears; i++) + size += sprintf(buf + size, "%8d ", + master->bus->prop.clk_gears[i]); + size += sprintf(buf + size, "\n"); + + return size; +} +static DEVICE_ATTR_RO(clock_gears); + +static struct attribute *master_node_attrs[] = { + &dev_attr_revision.attr, + &dev_attr_clk_stop_modes.attr, + &dev_attr_max_clk_freq.attr, + &dev_attr_default_row.attr, + &dev_attr_default_col.attr, + &dev_attr_default_frame_rate.attr, + &dev_attr_dynamic_frame.attr, + &dev_attr_err_threshold.attr, + &dev_attr_clock_frequencies.attr, + &dev_attr_clock_gears.attr, + NULL, +}; + +static const struct attribute_group sdw_master_node_group = { + .attrs = master_node_attrs, +}; + +static const struct attribute_group *sdw_master_node_groups[] = { + &sdw_master_node_group, + NULL +}; + +static void sdw_device_release(struct device *dev) +{ + struct sdw_master_sysfs *master = to_sdw_device(dev); + + kfree(master); +} + +static struct device_type sdw_device_type = { + .name = "sdw_device", + .release = sdw_device_release, +}; + +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; + + bus->sysfs = master; + master->bus = bus; + master->dev.type = &sdw_device_type; + master->dev.bus = &sdw_bus_type; + master->dev.parent = bus->dev; + master->dev.groups = sdw_master_node_groups; + dev_set_name(&master->dev, "sdw-master:%x", bus->link_id); + + err = device_register(&master->dev); + if (err) + put_device(&master->dev); + + return err; +} + +void sdw_sysfs_bus_exit(struct sdw_bus *bus) +{ + struct sdw_master_sysfs *master = bus->sysfs; + + if (!master) + return; + + master->bus = NULL; + put_device(&master->dev); + bus->sysfs = NULL; +} diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 3b231472464a..b64d46fed0c8 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -402,6 +402,14 @@ int sdw_slave_read_dp0(struct sdw_slave *slave, struct fwnode_handle *port, struct sdw_dp0_prop *dp0); +/* + * SDW sysfs APIs + */ +struct sdw_master_sysfs; + +int sdw_sysfs_bus_init(struct sdw_bus *bus); +void sdw_sysfs_bus_exit(struct sdw_bus *bus); + /* * SDW Slave Structures and APIs */ @@ -731,6 +739,7 @@ struct sdw_master_ops { * @m_rt_list: List of Master instance of all stream(s) running on Bus. This * is used to compute and program bus bandwidth, clock, frame shape, * transport and port parameters + * @sysfs: Bus sysfs * @defer_msg: Defer message * @clk_stop_timeout: Clock stop timeout computed * @bank_switch_timeout: Bank switch timeout computed @@ -750,6 +759,7 @@ struct sdw_bus { struct sdw_bus_params params; struct sdw_master_prop prop; struct list_head m_rt_list; + struct sdw_master_sysfs *sysfs; struct sdw_defer defer_msg; unsigned int clk_stop_timeout; u32 bank_switch_timeout;
For each master N, add a device sdw-master:N and add the master properties as attributes. Credits: this patch is based on an earlier internal contribution by Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- drivers/soundwire/Makefile | 3 +- drivers/soundwire/bus.c | 6 ++ drivers/soundwire/sysfs.c | 162 ++++++++++++++++++++++++++++++++++ include/linux/soundwire/sdw.h | 10 +++ 4 files changed, 180 insertions(+), 1 deletion(-) create mode 100644 drivers/soundwire/sysfs.c