Re: [PATCH 02/14] soundwire: Add SoundWire bus type
From: Srinivas Kandagatla
Date: Fri Nov 10 2017 - 05:42:15 EST
On 10/11/17 04:59, Vinod Koul wrote:
On Thu, Nov 09, 2017 at 09:14:07PM +0000, Srinivas Kandagatla wrote:
On 19/10/17 04:03, Vinod Koul wrote:
This adds the base SoundWire bus type, bus and driver registration.
along with changes to module device table for new SoundWire
device type.
Signed-off-by: Sanyog Kale <sanyog.r.kale@xxxxxxxxx>
Signed-off-by: Vinod Koul <vinod.koul@xxxxxxxxx>
---
+++ b/drivers/soundwire/Kconfig
@@ -0,0 +1,22 @@
+#
+# SoundWire subsystem configuration
+#
+
+menuconfig SOUNDWIRE
+ bool "SoundWire support"
Any reason why this subsystem can not be build as module?
This is not subsystem symbol but the menu. The SOUNDWIRE_BUS can be module.
I Noticed that.
Are you able to be build SOUNDWIRE_BUS as moudles?
I think the issue is that SOUNDWIRE_BUS default is set to SOUNDWIRE
This would never allow SOUNDWIRE_BUS to set as module if SOUNDWIRE is bool.
May be that is the issue.
config SOUNDWIRE_BUS
tristate
default SOUNDWIRE
select REGMAP_SOUNDWIRE
+/**
+ * sdw_get_device_id: find the matching SoundWire device id
+ *
function name should end with () - according to kernel doc.
ah thanks for pointing will add
+ * @slave: SoundWire Slave device
+ * @drv: SoundWire Slave Driver
+ *
+ * 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.
+ */
BTW, This is a static private function, why are we adding kernel doc for
this?
the match is an important routine and helps people understand the logic
hence documentation. More doc is better right :)
I agree, more doc is better.
+struct bus_type sdw_bus_type = {
+ .name = "soundwire",
+ .match = sdw_bus_match,
+ .uevent = sdw_uevent,
+};
+EXPORT_SYMBOL(sdw_bus_type);
+
+static int sdw_drv_probe(struct device *dev)
+{
+ struct sdw_slave *slave = dev_to_sdw_dev(dev);
+ struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);
+ const struct sdw_device_id *id;
+ int ret;
+
+ id = sdw_get_device_id(slave, drv);
By this time we must have already matched dev and driver by the ID,
shouldn't it be just slave->id here?
I don't think so we do not have slave->id, we pass the id in probe as an
argument
Which probe function are you referening too ?
Not sure I get it, Only way to get to this probe is that id_table from
driver matches slave id which is done as part of sdw_bus_match().
So the id should be valid and calling sdw_get_device_id() is redundant here?
+ if (!id)
+ return -ENODEV;
+
+ /*
+ * attach to power domain but don't turn on (last arg)
+ */
+ ret = dev_pm_domain_attach(dev, false);
+ if (ret) {
Shouldn't it just handle the EPROBE_DEFER case and ignore it for other
errors.
why should we ignore other errors and continue?
If you are making power domain as mandatory for all the devices, then it
makes sense to err out. But not all the devices might have pm domains
associated, so continuing on other errors makes sense.. All of the bus
drivers in the kernel do that ex: ./drivers/base/platform.c,
./drivers/mmc/core/sdio_bus.c, ./drivers/spi/spi.c...
+ dev_err(dev, "Failed to attach PM domain: %d\n", ret);
+ return ret;
+ }
+
+ ret = drv->probe(slave, id);
+ if (ret) {
+ dev_err(dev, "Probe of %s failed: %d\n", drv->name, ret);
+ return ret;
+ }
What happens if the slave driver is built as module and loaded after the
slave device is attached to the bus. How does the slave driver get updated
status in this case?
We have similar usecase in slimbus too.
So we create devices based on firmware description, then the Slave may
report as present and we mark it as present. Once a driver is loaded, the
driver is probed here, the slave->status clearly tells the driver that slave
has already reported present.
Yep, that solution makes sense, Looks like I can do the same for slimbus
too.