| Message ID | 1512575231-4154-11-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 06, 2017 at 09:17:06PM +0530, Vinod Koul wrote: > It helps to read the properties for understanding and debugging > systems, so add sysfs files for SoundWire DisCo properties. > > TODO: Add ABI files for sysfs Is this TODO done? > +/* > + * The sysfs for properties reflects the MIPI description as given > + * in the MIPI DisCo spec > + * > + * Base file is: > + * properties > + * |---- interface-revision > + * |---- master-count > + * |---- link-N > + * |---- clock-stop-modes > + * |---- max-clock-frequency > + * |---- clock-frequencies > + * |---- default-frame-rows > + * |---- default-frame-cols > + * |---- dynamic-frame-shape > + * |---- command-error-threshold > + */ Why nest them so deep? Anyway, that's not really an issue I guess, it's your ABI, not mine :) > + > +struct sdw_master_sysfs { > + struct kobject kobj; > + struct sdw_bus *bus; Huh? Why do you need to use kobjects? When you switch from using a 'struct device' and hang a kobject off of it, that's a huge signal that something is wrong here. That kobject will now no longer be part of the device "chain" in the system, uevents will get odd, and other strange things can happen. Why can't you just use "normal" attributes attached to the device? You shouldn't need a kobject here. What am I missing? > +/* > + * Slave sysfs > + */ > + > +/* > + * The sysfs for Slave reflects the MIPI description as given > + * in the MIPI DisCo spec > + * > + * Base file is device > + * |---- mipi_revision > + * |---- wake_capable > + * |---- test_mode_capable > + * |---- simple_clk_stop_capable > + * |---- clk_stop_timeout > + * |---- ch_prep_timeout > + * |---- reset_behave > + * |---- high_PHY_capable > + * |---- paging_support > + * |---- bank_delay_support > + * |---- p15_behave > + * |---- master_count > + * |---- source_ports > + * |---- sink_ports > + * |---- dp0 > + * |---- max_word > + * |---- min_word > + * |---- words > + * |---- flow_controlled > + * |---- simple_ch_prep_sm > + * |---- device_interrupts > + * |---- dpN > + * |---- max_word > + * |---- min_word > + * |---- words > + * |---- type > + * |---- max_grouping > + * |---- simple_ch_prep_sm > + * |---- ch_prep_timeout > + * |---- device_interrupts > + * |---- max_ch > + * |---- min_ch > + * |---- ch > + * |---- ch_combinations > + * |---- modes > + * |---- max_async_buffer > + * |---- block_pack_mode > + * |---- port_encoding > + * |---- bus_min_freq > + * |---- bus_max_freq > + * |---- bus_freq > + * |---- max_freq > + * |---- min_freq > + * |---- freq > + * |---- prep_ch_behave > + * |---- glitchless > + * > + */ > +struct sdw_slave_sysfs { > + struct sdw_slave *slave; > +}; Same here, why are you using kobjects for this "device"? Shouldn't you use a real struct device for dpX? > + > +#define SLAVE_ATTR(type) \ > +static ssize_t type##_show(struct device *dev, \ > + struct device_attribute *attr, char *buf) \ > +{ \ > + struct sdw_slave *slave = dev_to_sdw_dev(dev); \ > + return sprintf(buf, "0x%x\n", slave->prop.type); \ > +} \ > +static DEVICE_ATTR_RO(type) Oh wait, you are? I'm confused, is this a real device or not? > + > +SLAVE_ATTR(mipi_revision); > +SLAVE_ATTR(wake_capable); > +SLAVE_ATTR(test_mode_capable); > +SLAVE_ATTR(clk_stop_mode1); > +SLAVE_ATTR(simple_clk_stop_capable); > +SLAVE_ATTR(clk_stop_timeout); > +SLAVE_ATTR(ch_prep_timeout); > +SLAVE_ATTR(reset_behave); > +SLAVE_ATTR(high_PHY_capable); > +SLAVE_ATTR(paging_support); > +SLAVE_ATTR(bank_delay_support); > +SLAVE_ATTR(p15_behave); > +SLAVE_ATTR(master_count); > +SLAVE_ATTR(source_ports); > + > +static ssize_t modalias_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct sdw_slave *slave = dev_to_sdw_dev(dev); > + > + return sdw_slave_modalias(slave, buf, 256); > +} > +static DEVICE_ATTR_RO(modalias); > + > +static struct attribute *slave_dev_attrs[] = { > + &dev_attr_mipi_revision.attr, > + &dev_attr_wake_capable.attr, > + &dev_attr_test_mode_capable.attr, > + &dev_attr_clk_stop_mode1.attr, > + &dev_attr_simple_clk_stop_capable.attr, > + &dev_attr_clk_stop_timeout.attr, > + &dev_attr_ch_prep_timeout.attr, > + &dev_attr_reset_behave.attr, > + &dev_attr_high_PHY_capable.attr, > + &dev_attr_paging_support.attr, > + &dev_attr_bank_delay_support.attr, > + &dev_attr_p15_behave.attr, > + &dev_attr_master_count.attr, > + &dev_attr_source_ports.attr, > + &dev_attr_modalias.attr, > + NULL, > +}; > + > +static struct attribute_group sdw_slave_dev_attr_group = { > + .attrs = slave_dev_attrs, > +}; > + > +const struct attribute_group *sdw_slave_dev_attr_groups[] = { > + &sdw_slave_dev_attr_group, > + NULL > +}; > diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h > index e752db72a045..fce502d08fef 100644 > --- a/include/linux/soundwire/sdw.h > +++ b/include/linux/soundwire/sdw.h > @@ -308,6 +308,15 @@ int sdw_master_read_prop(struct sdw_bus *bus); > int sdw_slave_read_prop(struct sdw_slave *slave); > > /* > + * SDW sysfs APIs > + */ > +struct sdw_slave_sysfs; > +struct sdw_master_sysfs; > + > +int sdw_sysfs_bus_init(struct sdw_bus *bus); How are you handling the sysfs files showing up "after" the device is registered and userspace not seeing them? Have you tried using libudev to get the attributes? Does this all work properly? I think with the use of a kobject that breaks, and that's not good. > +void sdw_sysfs_bus_exit(struct sdw_bus *bus); > + > +/* > * SDW Slave Structures and APIs > */ > > @@ -363,6 +372,7 @@ struct sdw_slave_ops { > * @bus: Bus handle > * @ops: Slave callback ops > * @prop: Slave properties > + * @sysfs: Sysfs interface > * @node: node for bus list > * @port_ready: Port ready completion flag for each Slave port > * @dev_num: Device Number assigned by Bus > @@ -374,6 +384,7 @@ struct sdw_slave { > struct sdw_bus *bus; > const struct sdw_slave_ops *ops; > struct sdw_slave_prop prop; > + struct sdw_slave_sysfs *sysfs; So a differently reference counted device is hanging off of this? That's the big objection to using a kobject here and should have been a clue that this isn't ok. > struct list_head node; > struct completion *port_ready; > u16 dev_num; > @@ -453,6 +464,7 @@ struct sdw_master_ops { > * @msg_lock: message lock > * @ops: Master callback ops > * @prop: Master properties > + * @sysfs: Bus sysfs > * @defer_msg: Defer message > * @clk_stop_timeout: Clock stop timeout computed > */ > @@ -465,6 +477,7 @@ struct sdw_bus { > struct mutex msg_lock; > const struct sdw_master_ops *ops; > struct sdw_master_prop prop; > + struct sdw_master_sysfs *sysfs; Same here. thanks, greg k-h
On Wed, Dec 13, 2017 at 10:15:37AM +0100, Greg Kroah-Hartman wrote: > On Wed, Dec 06, 2017 at 09:17:06PM +0530, Vinod Koul wrote: > > It helps to read the properties for understanding and debugging > > systems, so add sysfs files for SoundWire DisCo properties. > > > > TODO: Add ABI files for sysfs > > Is this TODO done? Nope sorry not yet. But before this get merged will add > > + * Base file is: > > + * properties > > + * |---- interface-revision > > + * |---- master-count > > + * |---- link-N > > + * |---- clock-stop-modes > > + * |---- max-clock-frequency > > + * |---- clock-frequencies > > + * |---- default-frame-rows > > + * |---- default-frame-cols > > + * |---- dynamic-frame-shape > > + * |---- command-error-threshold > > + */ > > Why nest them so deep? Anyway, that's not really an issue I guess, it's > your ABI, not mine :) well it gives us a hierarchical view. We have N links... > > > + > > +struct sdw_master_sysfs { > > + struct kobject kobj; > > + struct sdw_bus *bus; > > Huh? Why do you need to use kobjects? > > When you switch from using a 'struct device' and hang a kobject off of > it, that's a huge signal that something is wrong here. That kobject > will now no longer be part of the device "chain" in the system, uevents > will get odd, and other strange things can happen. > > Why can't you just use "normal" attributes attached to the device? You > shouldn't need a kobject here. What am I missing? Okay my understanding might be incorrect then. So we can have N links in the system and each link would have a kobject for "link-N". Not sure how device attributes would do link-N/clock-stop-modes and so on, if they can let me know how and I will surely change that. > > > +/* > > + * Slave sysfs > > + */ > > + > > +/* > > + * The sysfs for Slave reflects the MIPI description as given > > + * in the MIPI DisCo spec > > + * > > + * Base file is device > > + * |---- mipi_revision > > + * |---- wake_capable > > + * |---- test_mode_capable > > + * |---- simple_clk_stop_capable > > + * |---- clk_stop_timeout > > + * |---- ch_prep_timeout > > + * |---- reset_behave > > + * |---- high_PHY_capable > > + * |---- paging_support > > + * |---- bank_delay_support > > + * |---- p15_behave > > + * |---- master_count > > + * |---- source_ports > > + * |---- sink_ports > > + * |---- dp0 > > + * |---- max_word > > + * |---- min_word > > + * |---- words > > + * |---- flow_controlled > > + * |---- simple_ch_prep_sm > > + * |---- device_interrupts > > + * |---- dpN > > + * |---- max_word > > + * |---- min_word > > + * |---- words > > + * |---- type > > + * |---- max_grouping > > + * |---- simple_ch_prep_sm > > + * |---- ch_prep_timeout > > + * |---- device_interrupts > > + * |---- max_ch > > + * |---- min_ch > > + * |---- ch > > + * |---- ch_combinations > > + * |---- modes > > + * |---- max_async_buffer > > + * |---- block_pack_mode > > + * |---- port_encoding > > + * |---- bus_min_freq > > + * |---- bus_max_freq > > + * |---- bus_freq > > + * |---- max_freq > > + * |---- min_freq > > + * |---- freq > > + * |---- prep_ch_behave > > + * |---- glitchless > > + * > > + */ > > +struct sdw_slave_sysfs { > > + struct sdw_slave *slave; > > +}; > > Same here, why are you using kobjects for this "device"? Shouldn't you > use a real struct device for dpX? > > > + > > +#define SLAVE_ATTR(type) \ > > +static ssize_t type##_show(struct device *dev, \ > > + struct device_attribute *attr, char *buf) \ > > +{ \ > > + struct sdw_slave *slave = dev_to_sdw_dev(dev); \ > > + return sprintf(buf, "0x%x\n", slave->prop.type); \ > > +} \ > > +static DEVICE_ATTR_RO(type) > > Oh wait, you are? I'm confused, is this a real device or not? Yes it real device :) We have attributes and a device can have N data ports which have their own attributes, so added kobject for each dpN and attributes for those. So it would appear as: dpN/max_word and so in sysfs If there a better way to do this without kobject, please do let me know and I will change that > > > + > > +SLAVE_ATTR(mipi_revision); > > +SLAVE_ATTR(wake_capable); > > +SLAVE_ATTR(test_mode_capable); > > +SLAVE_ATTR(clk_stop_mode1); > > +SLAVE_ATTR(simple_clk_stop_capable); > > +SLAVE_ATTR(clk_stop_timeout); > > +SLAVE_ATTR(ch_prep_timeout); > > +SLAVE_ATTR(reset_behave); > > +SLAVE_ATTR(high_PHY_capable); > > +SLAVE_ATTR(paging_support); > > +SLAVE_ATTR(bank_delay_support); > > +SLAVE_ATTR(p15_behave); > > +SLAVE_ATTR(master_count); > > +SLAVE_ATTR(source_ports); > > + > > +static ssize_t modalias_show(struct device *dev, struct device_attribute *attr, > > + char *buf) > > +{ > > + struct sdw_slave *slave = dev_to_sdw_dev(dev); > > + > > + return sdw_slave_modalias(slave, buf, 256); > > +} > > +static DEVICE_ATTR_RO(modalias); > > + > > +static struct attribute *slave_dev_attrs[] = { > > + &dev_attr_mipi_revision.attr, > > + &dev_attr_wake_capable.attr, > > + &dev_attr_test_mode_capable.attr, > > + &dev_attr_clk_stop_mode1.attr, > > + &dev_attr_simple_clk_stop_capable.attr, > > + &dev_attr_clk_stop_timeout.attr, > > + &dev_attr_ch_prep_timeout.attr, > > + &dev_attr_reset_behave.attr, > > + &dev_attr_high_PHY_capable.attr, > > + &dev_attr_paging_support.attr, > > + &dev_attr_bank_delay_support.attr, > > + &dev_attr_p15_behave.attr, > > + &dev_attr_master_count.attr, > > + &dev_attr_source_ports.attr, > > + &dev_attr_modalias.attr, > > + NULL, > > +}; > > + > > +static struct attribute_group sdw_slave_dev_attr_group = { > > + .attrs = slave_dev_attrs, > > +}; > > + > > +const struct attribute_group *sdw_slave_dev_attr_groups[] = { > > + &sdw_slave_dev_attr_group, > > + NULL > > +}; > > diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h > > index e752db72a045..fce502d08fef 100644 > > --- a/include/linux/soundwire/sdw.h > > +++ b/include/linux/soundwire/sdw.h > > @@ -308,6 +308,15 @@ int sdw_master_read_prop(struct sdw_bus *bus); > > int sdw_slave_read_prop(struct sdw_slave *slave); > > > > /* > > + * SDW sysfs APIs > > + */ > > +struct sdw_slave_sysfs; > > +struct sdw_master_sysfs; > > + > > +int sdw_sysfs_bus_init(struct sdw_bus *bus); > > How are you handling the sysfs files showing up "after" the device is > registered and userspace not seeing them? Have you tried using libudev > to get the attributes? Does this all work properly? I think with the > use of a kobject that breaks, and that's not good. Ah i am missing an uevent here, will add. I will check the libudev too. > > +void sdw_sysfs_bus_exit(struct sdw_bus *bus); > > + > > +/* > > * SDW Slave Structures and APIs > > */ > > > > @@ -363,6 +372,7 @@ struct sdw_slave_ops { > > * @bus: Bus handle > > * @ops: Slave callback ops > > * @prop: Slave properties > > + * @sysfs: Sysfs interface > > * @node: node for bus list > > * @port_ready: Port ready completion flag for each Slave port > > * @dev_num: Device Number assigned by Bus > > @@ -374,6 +384,7 @@ struct sdw_slave { > > struct sdw_bus *bus; > > const struct sdw_slave_ops *ops; > > struct sdw_slave_prop prop; > > + struct sdw_slave_sysfs *sysfs; > > So a differently reference counted device is hanging off of this? > That's the big objection to using a kobject here and should have been a > clue that this isn't ok. sdw_slave embeds the struct device and sysfs is for this device, shouldn't that be okay? > > > struct list_head node; > > struct completion *port_ready; > > u16 dev_num; > > @@ -453,6 +464,7 @@ struct sdw_master_ops { > > * @msg_lock: message lock > > * @ops: Master callback ops > > * @prop: Master properties > > + * @sysfs: Bus sysfs > > * @defer_msg: Defer message > > * @clk_stop_timeout: Clock stop timeout computed > > */ > > @@ -465,6 +477,7 @@ struct sdw_bus { > > struct mutex msg_lock; > > const struct sdw_master_ops *ops; > > struct sdw_master_prop prop; > > + struct sdw_master_sysfs *sysfs; > > Same here. sdw_bus represent master device and is registered by the driver. is there a better way to handle this.
On 12/13/2017 03:54 AM, Vinod Koul wrote: > >>> + * Base file is: >>> + * properties >>> + * |---- interface-revision >>> + * |---- master-count >>> + * |---- link-N >>> + * |---- clock-stop-modes >>> + * |---- max-clock-frequency >>> + * |---- clock-frequencies >>> + * |---- default-frame-rows >>> + * |---- default-frame-cols >>> + * |---- dynamic-frame-shape >>> + * |---- command-error-threshold >>> + */ >> Why nest them so deep? Anyway, that's not really an issue I guess, it's >> your ABI, not mine :) > well it gives us a hierarchical view. We have N links... The property definitions follows the MIPI DisCo spec, there's no real creativity here. That said, Vinod you need to remove the master-count - as discussed on the other patch where this property is read this is not relevant for a master if you don't have a representation of a controller.
On Wed, Dec 13, 2017 at 03:24:30PM +0530, Vinod Koul wrote: > On Wed, Dec 13, 2017 at 10:15:37AM +0100, Greg Kroah-Hartman wrote: > > On Wed, Dec 06, 2017 at 09:17:06PM +0530, Vinod Koul wrote: > > > It helps to read the properties for understanding and debugging > > > systems, so add sysfs files for SoundWire DisCo properties. > > > > > > TODO: Add ABI files for sysfs > > > > Is this TODO done? > > Nope sorry not yet. But before this get merged will add > > > > + * Base file is: > > > + * properties > > > + * |---- interface-revision > > > + * |---- master-count > > > + * |---- link-N > > > + * |---- clock-stop-modes > > > + * |---- max-clock-frequency > > > + * |---- clock-frequencies > > > + * |---- default-frame-rows > > > + * |---- default-frame-cols > > > + * |---- dynamic-frame-shape > > > + * |---- command-error-threshold > > > + */ > > > > Why nest them so deep? Anyway, that's not really an issue I guess, it's > > your ABI, not mine :) > > well it gives us a hierarchical view. We have N links... That's fine, then make it a real 'struct device' if you want to have a reference counted object. Tie it to your bus, and you are good to go. Don't use a raw kobject as that totaly breaks the device heirachy in the kernel as well as preventing any of these attributes from being accessed by userspace libraries (i.e. libudev.) > > > + > > > +struct sdw_master_sysfs { > > > + struct kobject kobj; > > > + struct sdw_bus *bus; > > > > Huh? Why do you need to use kobjects? > > > > When you switch from using a 'struct device' and hang a kobject off of > > it, that's a huge signal that something is wrong here. That kobject > > will now no longer be part of the device "chain" in the system, uevents > > will get odd, and other strange things can happen. > > > > Why can't you just use "normal" attributes attached to the device? You > > shouldn't need a kobject here. What am I missing? > > Okay my understanding might be incorrect then. So we can have N links in > the system and each link would have a kobject for "link-N". Not sure how > device attributes would do link-N/clock-stop-modes and so on, if they can > let me know how and I will surely change that. You can create a subdirectory for attributes quite easily. If you don't want to make it a "full" object, and all you care about is the subdirectory, then do it that way. Otherwise use a 'struct device' please. thanks, greg k-h
On Wed, Dec 13, 2017 at 05:28:21PM +0100, Greg Kroah-Hartman wrote: > On Wed, Dec 13, 2017 at 03:24:30PM +0530, Vinod Koul wrote: > > On Wed, Dec 13, 2017 at 10:15:37AM +0100, Greg Kroah-Hartman wrote: > > > On Wed, Dec 06, 2017 at 09:17:06PM +0530, Vinod Koul wrote: > > > > It helps to read the properties for understanding and debugging > > > > systems, so add sysfs files for SoundWire DisCo properties. > > > > > > > > TODO: Add ABI files for sysfs > > > > > > Is this TODO done? > > > > Nope sorry not yet. But before this get merged will add > > > > > > + * Base file is: > > > > + * properties > > > > + * |---- interface-revision > > > > + * |---- master-count > > > > + * |---- link-N > > > > + * |---- clock-stop-modes > > > > + * |---- max-clock-frequency > > > > + * |---- clock-frequencies > > > > + * |---- default-frame-rows > > > > + * |---- default-frame-cols > > > > + * |---- dynamic-frame-shape > > > > + * |---- command-error-threshold > > > > + */ > > > > > > Why nest them so deep? Anyway, that's not really an issue I guess, it's > > > your ABI, not mine :) > > > > well it gives us a hierarchical view. We have N links... > > That's fine, then make it a real 'struct device' if you want to have a > reference counted object. Tie it to your bus, and you are good to go. > Don't use a raw kobject as that totaly breaks the device heirachy in the > kernel as well as preventing any of these attributes from being accessed > by userspace libraries (i.e. libudev.) > > > > > + > > > > +struct sdw_master_sysfs { > > > > + struct kobject kobj; > > > > + struct sdw_bus *bus; > > > > > > Huh? Why do you need to use kobjects? > > > > > > When you switch from using a 'struct device' and hang a kobject off of > > > it, that's a huge signal that something is wrong here. That kobject > > > will now no longer be part of the device "chain" in the system, uevents > > > will get odd, and other strange things can happen. > > > > > > Why can't you just use "normal" attributes attached to the device? You > > > shouldn't need a kobject here. What am I missing? > > > > Okay my understanding might be incorrect then. So we can have N links in > > the system and each link would have a kobject for "link-N". Not sure how > > device attributes would do link-N/clock-stop-modes and so on, if they can > > let me know how and I will surely change that. > > You can create a subdirectory for attributes quite easily. If you don't > want to make it a "full" object, and all you care about is the > subdirectory, then do it that way. Otherwise use a 'struct device' > please. Okay thanks this makes sense, yes all we do care about is creating subdirectories and attributes under them. So in that sense we don't care much about adding kobjects, it was means to the end. So do you mind pointing an example, I though the way for that was kobjects by looking at few examples I saw.
On Wed, Dec 13, 2017 at 10:22:46PM +0530, Vinod Koul wrote: > On Wed, Dec 13, 2017 at 05:28:21PM +0100, Greg Kroah-Hartman wrote: > > On Wed, Dec 13, 2017 at 03:24:30PM +0530, Vinod Koul wrote: > > > On Wed, Dec 13, 2017 at 10:15:37AM +0100, Greg Kroah-Hartman wrote: > > > > On Wed, Dec 06, 2017 at 09:17:06PM +0530, Vinod Koul wrote: > > > > > It helps to read the properties for understanding and debugging > > > > > systems, so add sysfs files for SoundWire DisCo properties. > > > > > > > > > > TODO: Add ABI files for sysfs > > > > > > > > Is this TODO done? > > > > > > Nope sorry not yet. But before this get merged will add > > > > > > > > + * Base file is: > > > > > + * properties > > > > > + * |---- interface-revision > > > > > + * |---- master-count > > > > > + * |---- link-N > > > > > + * |---- clock-stop-modes > > > > > + * |---- max-clock-frequency > > > > > + * |---- clock-frequencies > > > > > + * |---- default-frame-rows > > > > > + * |---- default-frame-cols > > > > > + * |---- dynamic-frame-shape > > > > > + * |---- command-error-threshold > > > > > + */ > > > > > > > > Why nest them so deep? Anyway, that's not really an issue I guess, it's > > > > your ABI, not mine :) > > > > > > well it gives us a hierarchical view. We have N links... > > > > That's fine, then make it a real 'struct device' if you want to have a > > reference counted object. Tie it to your bus, and you are good to go. > > Don't use a raw kobject as that totaly breaks the device heirachy in the > > kernel as well as preventing any of these attributes from being accessed > > by userspace libraries (i.e. libudev.) > > > > > > > + > > > > > +struct sdw_master_sysfs { > > > > > + struct kobject kobj; > > > > > + struct sdw_bus *bus; > > > > > > > > Huh? Why do you need to use kobjects? > > > > > > > > When you switch from using a 'struct device' and hang a kobject off of > > > > it, that's a huge signal that something is wrong here. That kobject > > > > will now no longer be part of the device "chain" in the system, uevents > > > > will get odd, and other strange things can happen. > > > > > > > > Why can't you just use "normal" attributes attached to the device? You > > > > shouldn't need a kobject here. What am I missing? > > > > > > Okay my understanding might be incorrect then. So we can have N links in > > > the system and each link would have a kobject for "link-N". Not sure how > > > device attributes would do link-N/clock-stop-modes and so on, if they can > > > let me know how and I will surely change that. > > > > You can create a subdirectory for attributes quite easily. If you don't > > want to make it a "full" object, and all you care about is the > > subdirectory, then do it that way. Otherwise use a 'struct device' > > please. > > Okay thanks this makes sense, yes all we do care about is creating > subdirectories and attributes under them. So in that sense we don't care > much about adding kobjects, it was means to the end. > > So do you mind pointing an example, I though the way for that was kobjects > by looking at few examples I saw. Create a dynamic attribute group on the fly and initialize it and set the name to the name you want for the subdirectory. I know it's done in the kernel in some places, dig around :) good luck! greg k-h
On Wed, Dec 13, 2017 at 05:59:22PM +0100, Greg Kroah-Hartman wrote: > On Wed, Dec 13, 2017 at 10:22:46PM +0530, Vinod Koul wrote: > > On Wed, Dec 13, 2017 at 05:28:21PM +0100, Greg Kroah-Hartman wrote: > > > On Wed, Dec 13, 2017 at 03:24:30PM +0530, Vinod Koul wrote: > > > > On Wed, Dec 13, 2017 at 10:15:37AM +0100, Greg Kroah-Hartman wrote: > > > > > On Wed, Dec 06, 2017 at 09:17:06PM +0530, Vinod Koul wrote: > > > > > > It helps to read the properties for understanding and debugging > > > > > > systems, so add sysfs files for SoundWire DisCo properties. > > > > > > > > > > > > TODO: Add ABI files for sysfs > > > > > > > > > > Is this TODO done? > > > > > > > > Nope sorry not yet. But before this get merged will add > > > > > > > > > > + * Base file is: > > > > > > + * properties > > > > > > + * |---- interface-revision > > > > > > + * |---- master-count > > > > > > + * |---- link-N > > > > > > + * |---- clock-stop-modes > > > > > > + * |---- max-clock-frequency > > > > > > + * |---- clock-frequencies > > > > > > + * |---- default-frame-rows > > > > > > + * |---- default-frame-cols > > > > > > + * |---- dynamic-frame-shape > > > > > > + * |---- command-error-threshold > > > > > > + */ > > > > > > > > > > Why nest them so deep? Anyway, that's not really an issue I guess, it's > > > > > your ABI, not mine :) > > > > > > > > well it gives us a hierarchical view. We have N links... > > > > > > That's fine, then make it a real 'struct device' if you want to have a > > > reference counted object. Tie it to your bus, and you are good to go. > > > Don't use a raw kobject as that totaly breaks the device heirachy in the > > > kernel as well as preventing any of these attributes from being accessed > > > by userspace libraries (i.e. libudev.) > > > > > > > > > + > > > > > > +struct sdw_master_sysfs { > > > > > > + struct kobject kobj; > > > > > > + struct sdw_bus *bus; > > > > > > > > > > Huh? Why do you need to use kobjects? > > > > > > > > > > When you switch from using a 'struct device' and hang a kobject off of > > > > > it, that's a huge signal that something is wrong here. That kobject > > > > > will now no longer be part of the device "chain" in the system, uevents > > > > > will get odd, and other strange things can happen. > > > > > > > > > > Why can't you just use "normal" attributes attached to the device? You > > > > > shouldn't need a kobject here. What am I missing? > > > > > > > > Okay my understanding might be incorrect then. So we can have N links in > > > > the system and each link would have a kobject for "link-N". Not sure how > > > > device attributes would do link-N/clock-stop-modes and so on, if they can > > > > let me know how and I will surely change that. > > > > > > You can create a subdirectory for attributes quite easily. If you don't > > > want to make it a "full" object, and all you care about is the > > > subdirectory, then do it that way. Otherwise use a 'struct device' > > > please. > > > > Okay thanks this makes sense, yes all we do care about is creating > > subdirectories and attributes under them. So in that sense we don't care > > much about adding kobjects, it was means to the end. > > > > So do you mind pointing an example, I though the way for that was kobjects > > by looking at few examples I saw. > > Create a dynamic attribute group on the fly and initialize it and set > the name to the name you want for the subdirectory. I know it's done in > the kernel in some places, dig around :) Hey Greg, So I have spent couple of days on this but don't have a solution. I can create a subdirectory using dynamic attribute group by giving it a name. Works as advertised:). But I am unable to create subdirectory under newly created subdir. The properties defined by MIPI follow a certain hierarchy so we need subdirectories to represent that. Second issue am facing with these is getting the sdw_bus context. I tried adding it but nothing looks elegant. I tried making sdw_attributes on top of struct attribute and pass around the bus but doesn't seem to work for me. On the contrary, the current design of creating kobjects looked more cleaner. It has been used in bunch of other places, I double checked ref counting and cleanup looks okay to me. Btw I tested with libudev as well, its works well as we do send the event using kobject_uevent(). > good luck! Didn't help though :( So let me know if you have any other suggestion, if not I will send this once I add the Documentation for ABI.
On Fri, Dec 22, 2017 at 01:56:56PM +0530, Vinod Koul wrote: > On Wed, Dec 13, 2017 at 05:59:22PM +0100, Greg Kroah-Hartman wrote: > > On Wed, Dec 13, 2017 at 10:22:46PM +0530, Vinod Koul wrote: > > > On Wed, Dec 13, 2017 at 05:28:21PM +0100, Greg Kroah-Hartman wrote: > > > > On Wed, Dec 13, 2017 at 03:24:30PM +0530, Vinod Koul wrote: > > > > > On Wed, Dec 13, 2017 at 10:15:37AM +0100, Greg Kroah-Hartman wrote: > > > > > > On Wed, Dec 06, 2017 at 09:17:06PM +0530, Vinod Koul wrote: > > > > > > > It helps to read the properties for understanding and debugging > > > > > > > systems, so add sysfs files for SoundWire DisCo properties. > > > > > > > > > > > > > > TODO: Add ABI files for sysfs > > > > > > > > > > > > Is this TODO done? > > > > > > > > > > Nope sorry not yet. But before this get merged will add > > > > > > > > > > > > + * Base file is: > > > > > > > + * properties > > > > > > > + * |---- interface-revision > > > > > > > + * |---- master-count > > > > > > > + * |---- link-N > > > > > > > + * |---- clock-stop-modes > > > > > > > + * |---- max-clock-frequency > > > > > > > + * |---- clock-frequencies > > > > > > > + * |---- default-frame-rows > > > > > > > + * |---- default-frame-cols > > > > > > > + * |---- dynamic-frame-shape > > > > > > > + * |---- command-error-threshold > > > > > > > + */ > > > > > > > > > > > > Why nest them so deep? Anyway, that's not really an issue I guess, it's > > > > > > your ABI, not mine :) > > > > > > > > > > well it gives us a hierarchical view. We have N links... > > > > > > > > That's fine, then make it a real 'struct device' if you want to have a > > > > reference counted object. Tie it to your bus, and you are good to go. > > > > Don't use a raw kobject as that totaly breaks the device heirachy in the > > > > kernel as well as preventing any of these attributes from being accessed > > > > by userspace libraries (i.e. libudev.) > > > > > > > > > > > + > > > > > > > +struct sdw_master_sysfs { > > > > > > > + struct kobject kobj; > > > > > > > + struct sdw_bus *bus; > > > > > > > > > > > > Huh? Why do you need to use kobjects? > > > > > > > > > > > > When you switch from using a 'struct device' and hang a kobject off of > > > > > > it, that's a huge signal that something is wrong here. That kobject > > > > > > will now no longer be part of the device "chain" in the system, uevents > > > > > > will get odd, and other strange things can happen. > > > > > > > > > > > > Why can't you just use "normal" attributes attached to the device? You > > > > > > shouldn't need a kobject here. What am I missing? > > > > > > > > > > Okay my understanding might be incorrect then. So we can have N links in > > > > > the system and each link would have a kobject for "link-N". Not sure how > > > > > device attributes would do link-N/clock-stop-modes and so on, if they can > > > > > let me know how and I will surely change that. > > > > > > > > You can create a subdirectory for attributes quite easily. If you don't > > > > want to make it a "full" object, and all you care about is the > > > > subdirectory, then do it that way. Otherwise use a 'struct device' > > > > please. > > > > > > Okay thanks this makes sense, yes all we do care about is creating > > > subdirectories and attributes under them. So in that sense we don't care > > > much about adding kobjects, it was means to the end. > > > > > > So do you mind pointing an example, I though the way for that was kobjects > > > by looking at few examples I saw. > > > > Create a dynamic attribute group on the fly and initialize it and set > > the name to the name you want for the subdirectory. I know it's done in > > the kernel in some places, dig around :) > > Hey Greg, > > So I have spent couple of days on this but don't have a solution. > > I can create a subdirectory using dynamic attribute group by giving it a > name. Works as advertised:). But I am unable to create subdirectory under > newly created subdir. The properties defined by MIPI follow a certain > hierarchy so we need subdirectories to represent that. Yes, you can't create directories 2 levels deep, sorry, didn't realize you wanted to do that. > Second issue am facing with these is getting the sdw_bus context. I tried > adding it but nothing looks elegant. I tried making sdw_attributes on top of > struct attribute and pass around the bus but doesn't seem to work for me. > > On the contrary, the current design of creating kobjects looked more > cleaner. It has been used in bunch of other places, I double checked ref > counting and cleanup looks okay to me. Btw I tested with libudev as well, > its works well as we do send the event using kobject_uevent(). Where has it been used in other parts of the kernel like this? Time to go yell at people :) And the issue isn't that you will not catch the uevent, but that this is an attribute of the "parent" device, which is now not a device, but a "raw" kobject on no bus at all. So, just create these as real struct devices, with a different "type" and have them all live on the bus properly. Look at how the greybus code does it as one example (USB is another one, but it's much more complex.) thanks, greg k-h
On Fri, Dec 22, 2017 at 09:33:44AM +0100, Greg Kroah-Hartman wrote: > On Fri, Dec 22, 2017 at 01:56:56PM +0530, Vinod Koul wrote: > > Hey Greg, > > > > So I have spent couple of days on this but don't have a solution. > > > > I can create a subdirectory using dynamic attribute group by giving it a > > name. Works as advertised:). But I am unable to create subdirectory under > > newly created subdir. The properties defined by MIPI follow a certain > > hierarchy so we need subdirectories to represent that. > > Yes, you can't create directories 2 levels deep, sorry, didn't realize > you wanted to do that. > > > Second issue am facing with these is getting the sdw_bus context. I tried > > adding it but nothing looks elegant. I tried making sdw_attributes on top of > > struct attribute and pass around the bus but doesn't seem to work for me. > > > > On the contrary, the current design of creating kobjects looked more > > cleaner. It has been used in bunch of other places, I double checked ref > > counting and cleanup looks okay to me. Btw I tested with libudev as well, > > its works well as we do send the event using kobject_uevent(). > > Where has it been used in other parts of the kernel like this? Time to > go yell at people :) > > And the issue isn't that you will not catch the uevent, but that this is > an attribute of the "parent" device, which is now not a device, but a > "raw" kobject on no bus at all. > > So, just create these as real struct devices, with a different "type" > and have them all live on the bus properly. Look at how the greybus > code does it as one example (USB is another one, but it's much more > complex.) Thanks for the quick reply. Are you sure greybus is a right example? Looking at audio_manager.c we seem to create kset thus kobject without a device. It lives off global sysfs! Probing further I think you wanted to point me to gb_interface_create() which I think would be similar to sdw_add_bus_master(). So we should create a device without a driver for this and make kobjects live off it. So IIUC it is okay to create kbojects but make sure we they live with a 'device' right? Thanks
On Fri, Dec 22, 2017 at 05:13:44PM +0530, Vinod Koul wrote: > On Fri, Dec 22, 2017 at 09:33:44AM +0100, Greg Kroah-Hartman wrote: > > On Fri, Dec 22, 2017 at 01:56:56PM +0530, Vinod Koul wrote: > > > > Hey Greg, > > > > > > So I have spent couple of days on this but don't have a solution. > > > > > > I can create a subdirectory using dynamic attribute group by giving it a > > > name. Works as advertised:). But I am unable to create subdirectory under > > > newly created subdir. The properties defined by MIPI follow a certain > > > hierarchy so we need subdirectories to represent that. > > > > Yes, you can't create directories 2 levels deep, sorry, didn't realize > > you wanted to do that. > > > > > Second issue am facing with these is getting the sdw_bus context. I tried > > > adding it but nothing looks elegant. I tried making sdw_attributes on top of > > > struct attribute and pass around the bus but doesn't seem to work for me. > > > > > > On the contrary, the current design of creating kobjects looked more > > > cleaner. It has been used in bunch of other places, I double checked ref > > > counting and cleanup looks okay to me. Btw I tested with libudev as well, > > > its works well as we do send the event using kobject_uevent(). > > > > Where has it been used in other parts of the kernel like this? Time to > > go yell at people :) > > > > And the issue isn't that you will not catch the uevent, but that this is > > an attribute of the "parent" device, which is now not a device, but a > > "raw" kobject on no bus at all. > > > > So, just create these as real struct devices, with a different "type" > > and have them all live on the bus properly. Look at how the greybus > > code does it as one example (USB is another one, but it's much more > > complex.) > > Thanks for the quick reply. > > Are you sure greybus is a right example? Yes, but: > Looking at audio_manager.c we seem to create kset thus kobject without a > device. It lives off global sysfs! Not the audio_manager.c code :) That code never really got reviewed all that well before it was merged. There's a reason it is in staging... > Probing further I think you wanted to point me to gb_interface_create() > which I think would be similar to sdw_add_bus_master(). So we should > create a device without a driver for this and make kobjects live off it. > > So IIUC it is okay to create kbojects but make sure we they live with a > 'device' right? No, again, no driver or bus, should ever be creating "raw" kobjects. Create a new structure, that has a struct device in it, and give it a type that is unique to what ever this thing is and point the parent at the device you are managing. I don't have the time right now to dig through this sorry. Look at usb_create_ep_devs() maybe for a better example. thanks, greg k-h
diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile index bcde0d26524c..67dc7b546258 100644 --- a/drivers/soundwire/Makefile +++ b/drivers/soundwire/Makefile @@ -3,5 +3,5 @@ # #Bus Objs -soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o +soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o sysfs.o obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index bf68faface75..94245d70c913 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -41,6 +41,8 @@ int sdw_add_bus_master(struct sdw_bus *bus) } } + sdw_sysfs_bus_init(bus); + /* * Device numbers in SoundWire are 0 thru 15. Enumeration device * number (0), Broadcast device number (15), Group numbers (12 and @@ -105,6 +107,7 @@ 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/bus.h b/drivers/soundwire/bus.h index 345c34d697e9..d9fbb9a991ae 100644 --- a/drivers/soundwire/bus.h +++ b/drivers/soundwire/bus.h @@ -16,6 +16,8 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus) void sdw_extract_slave_id(struct sdw_bus *bus, u64 addr, struct sdw_slave_id *id); +extern const struct attribute_group *sdw_slave_dev_attr_groups[]; + enum { SDW_MSG_FLAG_READ = 0, SDW_MSG_FLAG_WRITE, diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c index 2b88396bec66..ce7aa5ec609e 100644 --- a/drivers/soundwire/slave.c +++ b/drivers/soundwire/slave.c @@ -34,6 +34,7 @@ static int sdw_slave_add(struct sdw_bus *bus, id->class_id, id->unique_id); slave->dev.release = sdw_slave_release; + slave->dev.groups = sdw_slave_dev_attr_groups; slave->dev.bus = &sdw_bus_type; slave->bus = bus; slave->status = SDW_SLAVE_UNATTACHED; diff --git a/drivers/soundwire/sysfs.c b/drivers/soundwire/sysfs.c new file mode 100644 index 000000000000..9e9093c17dcf --- /dev/null +++ b/drivers/soundwire/sysfs.c @@ -0,0 +1,343 @@ +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) +// Copyright(c) 2015-17 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" + +/* + * The sysfs for properties reflects the MIPI description as given + * in the MIPI DisCo spec + * + * Base file is: + * properties + * |---- interface-revision + * |---- master-count + * |---- link-N + * |---- clock-stop-modes + * |---- max-clock-frequency + * |---- clock-frequencies + * |---- default-frame-rows + * |---- default-frame-cols + * |---- dynamic-frame-shape + * |---- command-error-threshold + */ + +struct sdw_master_sysfs { + struct kobject kobj; + struct sdw_bus *bus; +}; + +struct sdw_prop_attribute { + struct attribute attr; + ssize_t (*show)(struct sdw_bus *bus, + struct sdw_prop_attribute *attr, char *buf); +}; + +static ssize_t sdw_prop_attr_show(struct kobject *kobj, + struct attribute *attr, char *buf) +{ + struct sdw_prop_attribute *prop_attr = + container_of(attr, struct sdw_prop_attribute, attr); + struct sdw_master_sysfs *master = + container_of(kobj, struct sdw_master_sysfs, kobj); + + if (!prop_attr->show) + return -EIO; + + return prop_attr->show(master->bus, prop_attr, buf); +} + +static const struct sysfs_ops sdw_prop_sysfs_ops = { + .show = sdw_prop_attr_show, +}; + +static void prop_release(struct kobject *kobj) +{ + struct sdw_master_sysfs *master = + container_of(kobj, struct sdw_master_sysfs, kobj); + + kfree(master); +} + +static struct kobj_type sdw_master_prop_ktype = { + .release = prop_release, + .sysfs_ops = &sdw_prop_sysfs_ops, +}; + + +#define MASTER_ATTR(_name) \ + struct sdw_prop_attribute master_attr_##_name = __ATTR_RO(_name) + +static ssize_t revision_show(struct sdw_bus *bus, + struct sdw_prop_attribute *attr, char *buf) +{ + return sprintf(buf, "0x%08x\n", bus->prop.revision); +} + +static ssize_t count_show(struct sdw_bus *bus, + struct sdw_prop_attribute *attr, char *buf) +{ + return sprintf(buf, "0x%08x\n", bus->prop.master_count); +} + +static ssize_t clock_stop_modes_show(struct sdw_bus *bus, + struct sdw_prop_attribute *attr, char *buf) +{ + return sprintf(buf, "0x%08x\n", bus->prop.clk_stop_mode); +} + +static ssize_t max_clock_frequency_show(struct sdw_bus *bus, + struct sdw_prop_attribute *attr, char *buf) +{ + return sprintf(buf, "0x%08x\n", bus->prop.max_freq); +} + +static ssize_t clock_frequencies_show(struct sdw_bus *bus, + struct sdw_prop_attribute *attr, char *buf) +{ + ssize_t size = 0; + int i; + + for (i = 0; i < bus->prop.num_freq; i++) + size += sprintf(buf + size, "%8d\n", bus->prop.freq[i]); + + return size; +} + +static ssize_t clock_gears_show(struct sdw_bus *bus, + struct sdw_prop_attribute *attr, char *buf) +{ + ssize_t size = 0; + int i; + + for (i = 0; i < bus->prop.num_clk_gears; i++) + size += sprintf(buf + size, "%8d\n", bus->prop.clk_gears[i]); + + return size; +} + +static ssize_t default_frame_rows_show(struct sdw_bus *bus, + struct sdw_prop_attribute *attr, char *buf) +{ + return sprintf(buf, "0x%08x\n", bus->prop.default_row); +} + +static ssize_t default_frame_cols_show(struct sdw_bus *bus, + struct sdw_prop_attribute *attr, char *buf) +{ + return sprintf(buf, "0x%08x\n", bus->prop.default_col); +} + +static ssize_t dynamic_frame_shape_show(struct sdw_bus *bus, + struct sdw_prop_attribute *attr, char *buf) +{ + return sprintf(buf, "0x%08x\n", bus->prop.dynamic_frame); +} + +static ssize_t command_error_threshold_show(struct sdw_bus *bus, + struct sdw_prop_attribute *attr, char *buf) +{ + return sprintf(buf, "0x%08x\n", bus->prop.err_threshold); +} + +static MASTER_ATTR(revision); +static MASTER_ATTR(count); +static MASTER_ATTR(clock_stop_modes); +static MASTER_ATTR(max_clock_frequency); +static MASTER_ATTR(clock_frequencies); +static MASTER_ATTR(clock_gears); +static MASTER_ATTR(default_frame_rows); +static MASTER_ATTR(default_frame_cols); +static MASTER_ATTR(dynamic_frame_shape); +static MASTER_ATTR(command_error_threshold); + +static struct attribute *master_node_attrs[] = { + &master_attr_revision.attr, + &master_attr_count.attr, + &master_attr_clock_stop_modes.attr, + &master_attr_max_clock_frequency.attr, + &master_attr_clock_frequencies.attr, + &master_attr_clock_gears.attr, + &master_attr_default_frame_rows.attr, + &master_attr_default_frame_cols.attr, + &master_attr_dynamic_frame_shape.attr, + &master_attr_command_error_threshold.attr, + NULL, +}; + +static const struct attribute_group sdw_master_node_group = { + .attrs = master_node_attrs, +}; + +int sdw_sysfs_bus_init(struct sdw_bus *bus) +{ + struct kset *sdw_bus_kset; + struct sdw_master_sysfs *master; + int err; + + if (bus->sysfs) { + dev_err(bus->dev, "SDW sysfs is already initialized\n"); + return -EIO; + } + + sdw_bus_kset = bus_get_kset(&sdw_bus_type); + + master = bus->sysfs = kzalloc(sizeof(*master), GFP_KERNEL); + if (!master) + return -ENOMEM; + + err = kobject_init_and_add(&master->kobj, + &sdw_master_prop_ktype, &sdw_bus_kset->kobj, + "mipi-properties-link%d", bus->link_id); + if (err < 0) + return err; + + master->bus = bus; + + err = sysfs_create_group(&master->kobj, &sdw_master_node_group); + if (err < 0) { + kobject_put(&master->kobj); + return err; + } + + kobject_uevent(&master->kobj, KOBJ_CHANGE); + return 0; +} + +void sdw_sysfs_bus_exit(struct sdw_bus *bus) +{ + struct sdw_master_sysfs *master = bus->sysfs; + + if (!master) + return; + + kobject_put(&master->kobj); + bus->sysfs = NULL; +} + +/* + * Slave sysfs + */ + +/* + * The sysfs for Slave reflects the MIPI description as given + * in the MIPI DisCo spec + * + * Base file is device + * |---- mipi_revision + * |---- wake_capable + * |---- test_mode_capable + * |---- simple_clk_stop_capable + * |---- clk_stop_timeout + * |---- ch_prep_timeout + * |---- reset_behave + * |---- high_PHY_capable + * |---- paging_support + * |---- bank_delay_support + * |---- p15_behave + * |---- master_count + * |---- source_ports + * |---- sink_ports + * |---- dp0 + * |---- max_word + * |---- min_word + * |---- words + * |---- flow_controlled + * |---- simple_ch_prep_sm + * |---- device_interrupts + * |---- dpN + * |---- max_word + * |---- min_word + * |---- words + * |---- type + * |---- max_grouping + * |---- simple_ch_prep_sm + * |---- ch_prep_timeout + * |---- device_interrupts + * |---- max_ch + * |---- min_ch + * |---- ch + * |---- ch_combinations + * |---- modes + * |---- max_async_buffer + * |---- block_pack_mode + * |---- port_encoding + * |---- bus_min_freq + * |---- bus_max_freq + * |---- bus_freq + * |---- max_freq + * |---- min_freq + * |---- freq + * |---- prep_ch_behave + * |---- glitchless + * + */ +struct sdw_slave_sysfs { + struct sdw_slave *slave; +}; + +#define SLAVE_ATTR(type) \ +static ssize_t type##_show(struct device *dev, \ + struct device_attribute *attr, char *buf) \ +{ \ + struct sdw_slave *slave = dev_to_sdw_dev(dev); \ + return sprintf(buf, "0x%x\n", slave->prop.type); \ +} \ +static DEVICE_ATTR_RO(type) + +SLAVE_ATTR(mipi_revision); +SLAVE_ATTR(wake_capable); +SLAVE_ATTR(test_mode_capable); +SLAVE_ATTR(clk_stop_mode1); +SLAVE_ATTR(simple_clk_stop_capable); +SLAVE_ATTR(clk_stop_timeout); +SLAVE_ATTR(ch_prep_timeout); +SLAVE_ATTR(reset_behave); +SLAVE_ATTR(high_PHY_capable); +SLAVE_ATTR(paging_support); +SLAVE_ATTR(bank_delay_support); +SLAVE_ATTR(p15_behave); +SLAVE_ATTR(master_count); +SLAVE_ATTR(source_ports); + +static ssize_t modalias_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct sdw_slave *slave = dev_to_sdw_dev(dev); + + return sdw_slave_modalias(slave, buf, 256); +} +static DEVICE_ATTR_RO(modalias); + +static struct attribute *slave_dev_attrs[] = { + &dev_attr_mipi_revision.attr, + &dev_attr_wake_capable.attr, + &dev_attr_test_mode_capable.attr, + &dev_attr_clk_stop_mode1.attr, + &dev_attr_simple_clk_stop_capable.attr, + &dev_attr_clk_stop_timeout.attr, + &dev_attr_ch_prep_timeout.attr, + &dev_attr_reset_behave.attr, + &dev_attr_high_PHY_capable.attr, + &dev_attr_paging_support.attr, + &dev_attr_bank_delay_support.attr, + &dev_attr_p15_behave.attr, + &dev_attr_master_count.attr, + &dev_attr_source_ports.attr, + &dev_attr_modalias.attr, + NULL, +}; + +static struct attribute_group sdw_slave_dev_attr_group = { + .attrs = slave_dev_attrs, +}; + +const struct attribute_group *sdw_slave_dev_attr_groups[] = { + &sdw_slave_dev_attr_group, + NULL +}; diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index e752db72a045..fce502d08fef 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -308,6 +308,15 @@ int sdw_master_read_prop(struct sdw_bus *bus); int sdw_slave_read_prop(struct sdw_slave *slave); /* + * SDW sysfs APIs + */ +struct sdw_slave_sysfs; +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 */ @@ -363,6 +372,7 @@ struct sdw_slave_ops { * @bus: Bus handle * @ops: Slave callback ops * @prop: Slave properties + * @sysfs: Sysfs interface * @node: node for bus list * @port_ready: Port ready completion flag for each Slave port * @dev_num: Device Number assigned by Bus @@ -374,6 +384,7 @@ struct sdw_slave { struct sdw_bus *bus; const struct sdw_slave_ops *ops; struct sdw_slave_prop prop; + struct sdw_slave_sysfs *sysfs; struct list_head node; struct completion *port_ready; u16 dev_num; @@ -453,6 +464,7 @@ struct sdw_master_ops { * @msg_lock: message lock * @ops: Master callback ops * @prop: Master properties + * @sysfs: Bus sysfs * @defer_msg: Defer message * @clk_stop_timeout: Clock stop timeout computed */ @@ -465,6 +477,7 @@ struct sdw_bus { struct mutex msg_lock; const struct sdw_master_ops *ops; struct sdw_master_prop prop; + struct sdw_master_sysfs *sysfs; struct sdw_defer defer_msg; unsigned int clk_stop_timeout; };