Re: [PATCH 02/14] soundwire: Add SoundWire bus type

[Vinod Koul <vinod.koul@intel.com>]

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@intel.com>
> >Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> >---
> 
> >+++ 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.

> 
> >+	---help---
> >+	  SoundWire is a 2-Pin interface with data and clock line ratified
> >+	  by the MIPI Alliance. SoundWire is used for transporting data
> >+	  typically related to audio functions. SoundWire interface is
> 
> >+#ifndef __SDW_BUS_H
> >+#define __SDW_BUS_H
> >+
> >+#include <linux/init.h>
> >+#include <linux/device.h>
> >+#include <linux/module.h>
> >+#include <linux/mod_devicetable.h>
> >+#include <linux/acpi.h>
> Do you need these headers here?

Yes :) I will double check though


> 
> >+#include <linux/soundwire/sdw.h>
> >+
> >+int sdw_slave_modalias(struct sdw_slave *slave, char *buf, size_t size);
> >+
> >+#endif /* __SDW_BUS_H */
> >diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
> >new file mode 100644
> >index 000000000000..a14d1de80afa
> >--- /dev/null
> >+++ b/drivers/soundwire/bus_type.c
> >
> >+#include <linux/acpi.h>
> >+#include <linux/device.h>
> >+#include <linux/init.h>
> >+#include <linux/module.h>
> >+#include <linux/mod_devicetable.h>
> >+#include <linux/pm_domain.h>
> >+#include <linux/pm_runtime.h>
> >+#include <linux/soundwire/sdw.h>
> >+#include "bus.h"
> >+
> >+/**
> >+ * 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 :)

> 
> >+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) {
> >+			return id;
> >+		}
> >+		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)
> >+{
> >+	/* modalias is sdw:m<mfg_id>p<part_id> */
> >+
> >+	return snprintf(buf, size, "sdw:m%04Xp%04X\n",
> >+			slave->id.mfg_id, slave->id.part_id);
> >+}
> >+
> >+static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env)
> >+{
> >+	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> >+	char modalias[32];
> >+
> >+	sdw_slave_modalias(slave, modalias, sizeof(modalias));
> >+
> >+	if (add_uevent_var(env, "MODALIAS=%s", modalias))
> >+		return -ENOMEM;
> >+
> >+	return 0;
> >+}
> >+
> >+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

> >+	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?

> 
> 
> >+		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.

-- 
~Vinod