On Thu, Oct 19, 2017 at 09:40:06AM +0200, Takashi Iwai wrote:
On Thu, 19 Oct 2017 05:03:18 +0200, Vinod Koul wrote:
+config SOUNDWIRE_BUS
- tristate
- default SOUNDWIRE
Does it make sense to be tristate? Since CONFIG_SOUNDWIRE is a bool, the above would be also only either Y or N. If it's Y and others select M, it'll be still Y.
hmmm good point. I think would make sense to make SOUNDWIRE as tristate too, just like SOUND :)
- sdw_get_device_id: find the matching SoundWire device id
- @slave: SoundWire Slave device
- @drv: SoundWire Slave Driver
Inconsistent upper/lower letters in these two lines.
thanks for spotting, will fix
- The match is done by comparing the mfg_id and part_id from the
- struct sdw_device_id. class_id is unused, as it is a placeholder
- in MIPI Spec.
- */
+static const struct sdw_device_id * +sdw_get_device_id(struct sdw_slave *slave, struct sdw_driver *drv) +{
- const struct sdw_device_id *id = drv->id_table;
- while (id && id->mfg_id) {
if (slave->id.mfg_id == id->mfg_id &&slave->id.part_id == id->part_id) {Please indentation properly.
what do you advise?
if (slave->id.mfg_id == id->mfg_id && slave->id.part_id == id->part_id) {
would mean below one is at same indent. Some people use:
if (slave->id.mfg_id == id->mfg_id && slave->id.part_id == id->part_id) {
Is it Documented anywhere...
return id;}Superfluous braces for a single-line.
That bit was intentional. Yes it is not required but given that if condition was falling to two lines, I wanted to help readability by adding these. I can remove them..
id++;- }
- return NULL;
+}
+static int sdw_bus_match(struct device *dev, struct device_driver *ddrv) +{
- struct sdw_slave *slave = dev_to_sdw_dev(dev);
- struct sdw_driver *drv = drv_to_sdw_driver(ddrv);
- return !!sdw_get_device_id(slave, drv);
+}
+int sdw_slave_modalias(struct sdw_slave *slave, char *buf, size_t size)
I'd put const to slave argument, as it won't be modified.
right...
--- a/include/linux/mod_devicetable.h +++ b/include/linux/mod_devicetable.h @@ -228,6 +228,13 @@ struct hda_device_id { unsigned long driver_data; };
+struct sdw_device_id {
- __u16 mfg_id;
- __u16 part_id;
- __u8 class_id;
- kernel_ulong_t driver_data;
Better to think of alignment.
sorry not quite clear, do you mind elaborating which ones to align?
--- /dev/null +++ b/include/linux/soundwire/sdw.h
....
+/**
- struct sdw_bus: SoundWire bus
- @dev: Master linux device
- @link_id: Link id number, can be 0 to N, unique for each Master
- @slaves: list of Slaves on this bus
- @assigned: logical addresses assigned, Index 0 (broadcast) would be unused
- @bus_lock: bus lock
- */
+struct sdw_bus {
- struct device *dev;
- unsigned int link_id;
- struct list_head slaves;
- bool assigned[SDW_MAX_DEVICES + 1];
Why not a bitmap?
That a very good suggestion, it will help in other things too :)