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.
>
> >+ ---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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
