From: Vinod Koul <vinod.koul@intel.com> To: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, LKML <linux-kernel@vger.kernel.org>, ALSA <alsa-devel@alsa-project.org>, Mark <broonie@kernel.org>, Takashi <tiwai@suse.de>, Pierre <pierre-louis.bossart@linux.intel.com>, Sanyog Kale <sanyog.r.kale@intel.com>, Shreyas NC <shreyas.nc@intel.com>, patches.audio@intel.com, alan@linux.intel.com, Charles Keepax <ckeepax@opensource.cirrus.com>, Sagar Dharia <sdharia@codeaurora.org>, plai@codeaurora.org, Sudheer Papothi <spapothi@codeaurora.org> Subject: Re: [PATCH 02/14] soundwire: Add SoundWire bus type Date: Fri, 10 Nov 2017 10:29:51 +0530 [thread overview] Message-ID: <20171110045951.GL3187@localhost> (raw) In-Reply-To: <5fee4ccb-1030-b698-e7ef-7f4390563a76@linaro.org> 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
WARNING: multiple messages have this Message-ID (diff)
From: Vinod Koul <vinod.koul@intel.com> To: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> Cc: ALSA <alsa-devel@alsa-project.org>, Charles Keepax <ckeepax@opensource.cirrus.com>, Takashi <tiwai@suse.de>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, plai@codeaurora.org, LKML <linux-kernel@vger.kernel.org>, Pierre <pierre-louis.bossart@linux.intel.com>, patches.audio@intel.com, Mark <broonie@kernel.org>, Sudheer Papothi <spapothi@codeaurora.org>, Shreyas NC <shreyas.nc@intel.com>, Sanyog Kale <sanyog.r.kale@intel.com>, Sagar Dharia <sdharia@codeaurora.org>, alan@linux.intel.com Subject: Re: [PATCH 02/14] soundwire: Add SoundWire bus type Date: Fri, 10 Nov 2017 10:29:51 +0530 [thread overview] Message-ID: <20171110045951.GL3187@localhost> (raw) In-Reply-To: <5fee4ccb-1030-b698-e7ef-7f4390563a76@linaro.org> 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
next prev parent reply other threads:[~2017-11-10 4:56 UTC|newest] Thread overview: 139+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-10-19 3:03 [PATCH 00/14] soundwire: Add a new SoundWire subsystem Vinod Koul 2017-10-19 3:03 ` Vinod Koul 2017-10-19 3:03 ` [PATCH 01/14] Documentation: Add SoundWire summary Vinod Koul 2017-10-19 3:33 ` Randy Dunlap 2017-10-19 3:33 ` Randy Dunlap 2017-10-19 4:44 ` Vinod Koul 2017-10-19 4:44 ` Vinod Koul 2017-10-20 10:39 ` Greg Kroah-Hartman 2017-10-20 10:39 ` Greg Kroah-Hartman 2017-10-20 15:49 ` Vinod Koul 2017-10-20 15:49 ` Vinod Koul 2017-10-20 16:22 ` Greg Kroah-Hartman 2017-10-20 16:22 ` Greg Kroah-Hartman 2017-10-20 17:09 ` Vinod Koul 2017-10-20 17:09 ` Vinod Koul 2017-10-21 8:57 ` Mark Brown 2017-10-21 8:57 ` Mark Brown 2017-10-21 11:28 ` Vinod Koul 2017-10-21 11:28 ` Vinod Koul 2017-10-22 10:06 ` [alsa-devel] " Pierre-Louis Bossart 2017-10-23 8:21 ` Mark Brown 2017-10-23 7:50 ` Mark Brown 2017-10-23 7:50 ` Mark Brown 2017-10-23 11:18 ` [alsa-devel] " Vinod Koul 2017-10-19 3:03 ` [PATCH 02/14] soundwire: Add SoundWire bus type Vinod Koul 2017-10-19 7:40 ` Takashi Iwai 2017-10-19 8:32 ` [alsa-devel] " Takashi Iwai 2017-10-19 8:32 ` Takashi Iwai 2017-10-20 5:11 ` Vinod Koul 2017-10-20 5:11 ` Vinod Koul 2017-10-20 6:59 ` Takashi Iwai 2017-10-20 15:46 ` Vinod Koul 2017-10-20 15:50 ` Takashi Iwai 2017-10-20 15:50 ` Takashi Iwai 2017-10-20 16:11 ` Vinod Koul 2017-10-20 10:41 ` Greg Kroah-Hartman 2017-10-20 10:41 ` Greg Kroah-Hartman 2017-10-20 15:52 ` Vinod Koul 2017-10-20 10:45 ` Greg Kroah-Hartman 2017-10-20 16:01 ` Vinod Koul 2017-10-20 16:21 ` Greg Kroah-Hartman 2017-10-20 16:21 ` Greg Kroah-Hartman 2017-10-20 17:10 ` Vinod Koul 2017-10-23 11:46 ` Alan Cox 2017-10-23 11:46 ` Alan Cox 2017-10-26 8:33 ` Vinod Koul 2017-10-26 8:33 ` Vinod Koul 2017-10-27 8:57 ` Greg Kroah-Hartman 2017-10-30 13:11 ` Vinod Koul 2017-10-20 16:03 ` Philippe Ombredanne 2017-10-20 16:20 ` Vinod Koul 2017-10-20 16:20 ` Vinod Koul 2017-10-20 16:27 ` Greg Kroah-Hartman 2017-10-20 17:13 ` Vinod Koul 2017-10-20 17:13 ` Vinod Koul 2017-10-23 11:52 ` Alan Cox 2017-10-23 11:52 ` Alan Cox 2017-10-21 9:03 ` Mark Brown 2017-10-21 11:29 ` Vinod Koul 2017-10-21 11:29 ` Vinod Koul 2017-11-09 21:14 ` Srinivas Kandagatla 2017-11-09 21:14 ` Srinivas Kandagatla 2017-11-10 4:59 ` Vinod Koul [this message] 2017-11-10 4:59 ` Vinod Koul 2017-11-10 8:55 ` Vinod Koul 2017-11-10 10:50 ` Srinivas Kandagatla 2017-11-10 10:42 ` Srinivas Kandagatla 2017-11-10 10:42 ` Srinivas Kandagatla 2017-11-10 10:58 ` Vinod Koul 2017-10-19 3:03 ` [PATCH 03/14] soundwire: Add Master registration Vinod Koul 2017-10-19 3:03 ` Vinod Koul 2017-10-19 8:54 ` [alsa-devel] " Takashi Iwai 2017-10-19 8:54 ` Takashi Iwai 2017-10-20 5:19 ` [alsa-devel] " Vinod Koul 2017-10-20 5:19 ` Vinod Koul 2017-10-20 10:47 ` Greg Kroah-Hartman 2017-10-20 16:05 ` Vinod Koul 2017-10-21 9:12 ` Mark Brown 2017-10-21 9:12 ` Mark Brown 2017-10-21 11:35 ` Vinod Koul 2017-10-21 11:35 ` Vinod Koul 2017-10-23 8:24 ` Mark Brown 2017-10-23 8:24 ` Mark Brown 2017-10-23 11:19 ` Vinod Koul 2017-11-09 21:14 ` Srinivas Kandagatla 2017-11-10 5:02 ` Vinod Koul 2017-11-10 5:02 ` Vinod Koul 2017-10-19 3:03 ` [PATCH 04/14] soundwire: Add MIPI DisCo property helpers Vinod Koul 2017-10-19 9:02 ` [alsa-devel] " Takashi Iwai 2017-10-20 5:25 ` Vinod Koul 2017-10-21 9:20 ` Mark Brown 2017-10-21 9:20 ` Mark Brown 2017-10-21 11:37 ` Vinod Koul 2017-10-22 10:14 ` [alsa-devel] " Pierre-Louis Bossart 2017-10-22 10:14 ` Pierre-Louis Bossart 2017-10-19 3:03 ` [PATCH 05/14] soundwire: Add SoundWire MIPI defined registers Vinod Koul 2017-10-19 3:03 ` [PATCH 06/14] soundwire: Add IO transfer Vinod Koul 2017-10-19 3:03 ` Vinod Koul 2017-10-19 9:13 ` [alsa-devel] " Takashi Iwai 2017-10-20 5:30 ` Vinod Koul 2017-10-20 5:30 ` Vinod Koul 2017-10-20 7:06 ` [alsa-devel] " Takashi Iwai 2017-10-20 7:06 ` Takashi Iwai 2017-10-20 15:48 ` [alsa-devel] " Vinod Koul 2017-10-21 9:29 ` Mark Brown 2017-10-21 9:29 ` Mark Brown 2017-10-21 11:40 ` Vinod Koul 2017-10-19 3:03 ` [PATCH 07/14] regmap: Add SoundWire bus support Vinod Koul 2017-10-21 9:34 ` Mark Brown 2017-10-21 9:34 ` Mark Brown 2017-10-21 11:44 ` Vinod Koul 2017-10-23 11:56 ` Alan Cox 2017-10-23 13:16 ` Mark Brown 2017-10-19 3:03 ` [PATCH 08/14] soundwire: Add Slave status handling helpers Vinod Koul 2017-10-19 13:44 ` [alsa-devel] " Takashi Iwai 2017-10-31 13:04 ` Vinod Koul 2017-10-31 13:04 ` Vinod Koul 2017-10-31 21:19 ` [alsa-devel] " Pierre-Louis Bossart 2017-10-31 21:19 ` Pierre-Louis Bossart 2017-11-01 9:08 ` [alsa-devel] " Vinod Koul 2017-11-01 9:08 ` Vinod Koul 2017-11-01 21:10 ` [alsa-devel] " Pierre-Louis Bossart 2017-11-02 3:28 ` Vinod Koul 2017-10-19 3:03 ` [PATCH 09/14] soundwire: Add slave status handling Vinod Koul 2017-10-19 3:03 ` [PATCH 10/14] soundwire: Add sysfs for SoundWire DisCo properties Vinod Koul 2017-10-21 9:42 ` Mark Brown 2017-10-21 9:42 ` Mark Brown 2017-10-21 11:53 ` Vinod Koul 2017-10-21 11:53 ` Vinod Koul 2017-11-09 21:14 ` Srinivas Kandagatla 2017-11-10 4:52 ` Vinod Koul 2017-10-19 3:03 ` [PATCH 11/14] soundwire: cdns: Add cadence module Vinod Koul 2017-10-21 9:52 ` Mark Brown 2017-10-21 9:52 ` Mark Brown 2017-10-21 11:54 ` Vinod Koul 2017-10-21 11:54 ` Vinod Koul 2017-10-19 3:03 ` [PATCH 12/14] soundwire: cdns: Add sdw_master_ops and IO transfer support Vinod Koul 2017-10-19 3:03 ` [PATCH 13/14] soundwire: intel: Add Intel Master driver Vinod Koul 2017-10-19 3:03 ` [PATCH 14/14] soundwire: intel: Add Intel init module Vinod Koul
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20171110045951.GL3187@localhost \ --to=vinod.koul@intel.com \ --cc=alan@linux.intel.com \ --cc=alsa-devel@alsa-project.org \ --cc=broonie@kernel.org \ --cc=ckeepax@opensource.cirrus.com \ --cc=gregkh@linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=patches.audio@intel.com \ --cc=pierre-louis.bossart@linux.intel.com \ --cc=plai@codeaurora.org \ --cc=sanyog.r.kale@intel.com \ --cc=sdharia@codeaurora.org \ --cc=shreyas.nc@intel.com \ --cc=spapothi@codeaurora.org \ --cc=srinivas.kandagatla@linaro.org \ --cc=tiwai@suse.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.