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

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

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?

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

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

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

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


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

> +
> +	return 0;
> +}
> +




>