On Fri, Nov 10, 2017 at 10:42:06AM +0000, Srinivas Kandagatla wrote:
>
>
> 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
removing this makes it build as module, sorry I assumed you have already
seen Takashi's comment. I have fixed it in v2. Posting anytime now :)
> >
> >>
> >>>+ * @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?
we dont store in id so we have to lookup again. I see the point
in doing so, let me check that
> >>>+ 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...
Ah thanks for pointing, let me check that
> >>>+ 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.
yup!
--
~Vinod
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
