Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus

[Vinod Koul]

On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandagatla@xxxxxxxxxx wrote:
> From: Sagar Dharia <sdharia@xxxxxxxxxxxxxx>
> 
> SLIMbus (Serial Low Power Interchip Media Bus) is a specification
> developed by MIPI (Mobile Industry Processor Interface) alliance.
> SLIMbus is a 2-wire implementation, which is used to communicate with
> peripheral components like audio-codec.
> SLIMbus uses Time-Division-Multiplexing to accommodate multiple data
> channels, and control channel. Control channel has messages to do
> device-enumeration, messages to send/receive control-data to/from
> slimbus devices, messages for port/channel management, and messages to
> do bandwidth allocation.
> The framework supports multiple instances of the bus (1 controller per
> bus), and multiple slave devices per controller.
> 
> This patch does device enumeration, logical address assignment,
> informing device when the device reports present/absent etc.
> Reporting present may need the driver to do the needful (e.g. turning
> on voltage regulators powering the device). Additionally device is
> probed when it reports present if that device doesn't need any such
> steps mentioned above.
> 
> Signed-off-by: Sagar Dharia <sdharia@xxxxxxxxxxxxxx>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> ---
>  Documentation/devicetree/bindings/slimbus/bus.txt |  57 ++
>  Documentation/slimbus/summary                     | 109 ++++
>  drivers/Kconfig                                   |   2 +
>  drivers/Makefile                                  |   1 +
>  drivers/slimbus/Kconfig                           |  11 +
>  drivers/slimbus/Makefile                          |   5 +
>  drivers/slimbus/slim-core.c                       | 695 ++++++++++++++++++++++
>  include/linux/mod_devicetable.h                   |  13 +
>  include/linux/slimbus.h                           | 299 ++++++++++
>  9 files changed, 1192 insertions(+)

thats a lot of code for review, consider splitting it up further for better
reviews

>  create mode 100644 Documentation/devicetree/bindings/slimbus/bus.txt
>  create mode 100644 Documentation/slimbus/summary
>  create mode 100644 drivers/slimbus/Kconfig
>  create mode 100644 drivers/slimbus/Makefile
>  create mode 100644 drivers/slimbus/slim-core.c

how about core.c (https://lkml.org/lkml/2017/7/12/430)

> +static const struct slim_device_id *slim_match(const struct slim_device_id *id,
> +					       const struct slim_device *sbdev)
> +{
> +	while (id->manf_id != 0 || id->prod_code != 0) {
> +		if (id->manf_id == sbdev->e_addr.manf_id &&
> +		    id->prod_code == sbdev->e_addr.prod_code &&
> +		    id->dev_index == sbdev->e_addr.dev_index)
> +			return id;
> +		id++;
> +	}
> +	return NULL;
> +}
> +
> +static int slim_device_match(struct device *dev, struct device_driver *drv)
> +{
> +	struct slim_device *sbdev = to_slim_device(dev);
> +	struct slim_driver *sbdrv = to_slim_driver(drv);
> +
> +	/* Attempt an OF style match first */
> +	if (of_driver_match_device(dev, drv))
> +		return 1;

is of_driver_match_device() a must have here? (I dont completely understand
DT so pardon my ignorance). Since we have devices with ids can we use that
alone for matching?

> +
> +	/* Then try to match against the id table */
> +	if (sbdrv->id_table)
> +		return slim_match(sbdrv->id_table, sbdev) != NULL;
> +
> +	return 0;
> +}
> +

rather than jumping now to reporting APIs, can we club all bus_type parts to
one place (patch) so that it is easier to review logically

> +struct sb_report_wd {
> +	struct work_struct wd;
> +	struct slim_device *sbdev;
> +	bool report;
> +};
> +
> +static void slim_report(struct work_struct *work)
> +{
> +	struct slim_driver *sbdrv;
> +	struct sb_report_wd *sbw = container_of(work, struct sb_report_wd, wd);
> +	struct slim_device *sbdev = sbw->sbdev;
> +
> +	mutex_lock(&sbdev->report_lock);
> +	if (!sbdev->dev.driver)
> +		goto report_exit;
> +
> +	/* check if device-up or down needs to be called */
> +	if ((!sbdev->reported && !sbdev->notified) ||
> +	    (sbdev->reported && sbdev->notified))
> +		goto report_exit;
> +
> +	sbdrv = to_slim_driver(sbdev->dev.driver);
> +
> +	/**
> +	 * address no longer valid, means device reported absent, whereas
> +	 * address valid, means device reported present
> +	 */

I think ppl commented about this style, so lets fix those issues

> +	if (sbdev->notified && !sbdev->reported) {
> +		sbdev->notified = false;
> +		if (sbdrv->device_down)
> +			sbdrv->device_down(sbdev);
> +	} else if (!sbdev->notified && sbdev->reported) {
> +		sbdev->notified = true;
> +		if (sbdrv->device_up)
> +			sbdrv->device_up(sbdev);

what do the device_up/down calls signify here?

> +static int slim_device_probe(struct device *dev)
> +{
> +	struct slim_device	*sbdev;
> +	struct slim_driver	*sbdrv;
> +	int status = 0;
> +
> +	sbdev = to_slim_device(dev);
> +	sbdrv = to_slim_driver(dev->driver);
> +
> +	sbdev->driver = sbdrv;
> +
> +	if (sbdrv->probe)
> +		status = sbdrv->probe(sbdev);
> +
> +	if (status)
> +		sbdev->driver = NULL;
> +	else if (sbdrv->device_up)
> +		schedule_slim_report(sbdev->ctrl, sbdev, true);

can you please explain what this is trying to do?

> +int __slim_driver_register(struct slim_driver *drv, struct module *owner)
> +{
> +	drv->driver.bus = &slimbus_type;
> +	drv->driver.owner = owner;
> +	return driver_register(&drv->driver);
> +}
> +EXPORT_SYMBOL_GPL(__slim_driver_register);

any reason to use __ for this API?

> +static int slim_add_device(struct slim_controller *ctrl,
> +			   struct slim_device *sbdev)
> +{
> +	sbdev->dev.bus = &slimbus_type;
> +	sbdev->dev.parent = &ctrl->dev;
> +	sbdev->dev.release = slim_dev_release;
> +	sbdev->dev.driver = NULL;
> +	sbdev->ctrl = ctrl;
> +
> +	slim_ctrl_get(ctrl);
> +	sbdev->name = kasprintf(GFP_KERNEL, "%x:%x:%x:%x",
> +					sbdev->e_addr.manf_id,
> +					sbdev->e_addr.prod_code,
> +					sbdev->e_addr.dev_index,
> +					sbdev->e_addr.instance);
> +	if (!sbdev->name)
> +		return -ENOMEM;
> +
> +	dev_set_name(&sbdev->dev, "%s", sbdev->name);
> +	mutex_init(&sbdev->report_lock);
> +
> +	/* probe slave on this controller */
> +	return device_register(&sbdev->dev);

I dont think the comment is quite correct, you register a device not probe!

> +/* OF helpers for SLIMbus */
> +static void of_register_slim_devices(struct slim_controller *ctrl)
> +{
> +	struct device *dev = &ctrl->dev;
> +	struct device_node *node;
> +
> +	if (!ctrl->dev.of_node)
> +		return;
> +
> +	for_each_child_of_node(ctrl->dev.of_node, node) {
> +		struct slim_device *slim;
> +		const char *compat = NULL;
> +		char *p, *tok;
> +		int reg[2], ret;
> +
> +		slim = kzalloc(sizeof(*slim), GFP_KERNEL);
> +		if (!slim)
> +			continue;
> +
> +		slim->dev.of_node = of_node_get(node);
> +
> +		compat = of_get_property(node, "compatible", NULL);
> +		if (!compat)
> +			continue;
> +
> +		p = kasprintf(GFP_KERNEL, "%s", compat + strlen("slim"));
> +
> +		tok = strsep(&p, ",");
> +		if (!tok) {
> +			dev_err(dev, "No valid Manufacturer ID found\n");
> +			kfree(p);
> +			continue;
> +		}
> +		slim->e_addr.manf_id = str2hex(tok);
> +
> +		tok = strsep(&p, ",");
> +		if (!tok) {
> +			dev_err(dev, "No valid Product ID found\n");
> +			kfree(p);
> +			continue;
> +		}
> +		slim->e_addr.prod_code = str2hex(tok);
> +		kfree(p);
> +
> +		ret = of_property_read_u32_array(node, "reg", reg, 2);
> +		if (ret) {
> +			dev_err(dev, "Device and Instance id not found:%d\n",
> +				ret);
> +			continue;
> +		}
> +		slim->e_addr.dev_index = reg[0];
> +		slim->e_addr.instance = reg[1];
> +
> +		ret = slim_add_device(ctrl, slim);

okay this is good stuff. So we scan the DT for slimbus devices and register
them here. Same stuff we can do with ACPI :)

then why do we need the of register stuff I commented earlier. A Slimbus
device can work irrespective of firmware type and registers using various
ids. The platform will scan firmware (dt/acpi) create devices and load
drivers against them generically.  Apart from this code we ideally should
not have any DT parts in the bus, do you agree?

> +		if (ret)
> +			dev_err(dev, "of_slim device register err:%d\n", ret);
> +	}
> +}
> +
> +/**
> + * slim_register_controller: Controller bring-up and registration.
> + * @ctrl: Controller to be registered.
> + * A controller is registered with the framework using this API.
> + * If devices on a controller were registered before controller,
> + * this will make sure that they get probed when controller is up
> + */
> +int slim_register_controller(struct slim_controller *ctrl)
> +{
> +	int id, ret = 0;
> +
> +	mutex_lock(&slim_lock);
> +	id = idr_alloc(&ctrl_idr, ctrl, ctrl->nr, -1, GFP_KERNEL);

what are these ids used for?

> +	mutex_unlock(&slim_lock);
> +
> +	if (id < 0)
> +		return id;
> +
> +	ctrl->nr = id;
> +
> +	dev_set_name(&ctrl->dev, "sb-%d", ctrl->nr);
> +	ctrl->num_dev = 0;
> +
> +	if (!ctrl->min_cg)
> +		ctrl->min_cg = SLIM_MIN_CLK_GEAR;
> +	if (!ctrl->max_cg)
> +		ctrl->max_cg = SLIM_MAX_CLK_GEAR;
> +
> +	mutex_init(&ctrl->m_ctrl);
> +	ret = device_register(&ctrl->dev);

one more device_register?? Can you explain why

> +/**
> + * struct slim_addrt: slimbus address used internally by the slimbus framework.
> + * @valid: If the device is present. Valid is set to false when device reports
> + *	absent.
> + * @eaddr: Enumeration address
> + * @laddr: It is possible that controller will set a predefined logical address
> + *	rather than the one assigned by framework. (i.e. logical address may
> + *	not be same as index into this table). This entry will store the
> + *	logical address value for this enumeration address.
> + */
> +struct slim_addrt {

addrt? why not just addr?

> +	bool			valid;
> +	struct slim_eaddr	eaddr;
> +	u8			laddr;
> +};
> +
> +/* SLIMbus message types. Related to interpretation of message code. */
> +#define SLIM_MSG_MT_CORE			0x0
> +#define SLIM_MSG_MT_DEST_REFERRED_CLASS		0x1
> +#define SLIM_MSG_MT_DEST_REFERRED_USER		0x2
> +#define SLIM_MSG_MT_SRC_REFERRED_CLASS		0x5
> +#define SLIM_MSG_MT_SRC_REFERRED_USER		0x6

BIT() GENMASK() please here and other places where they define bits in spec

> +/**
> + * struct slim_driver: Slimbus 'generic device' (slave) device driver
> + *				(similar to 'spi_device' on SPI)
> + * @probe: Binds this driver to a slimbus device.
> + * @remove: Unbinds this driver from the slimbus device.
> + * @shutdown: Standard shutdown callback used during powerdown/halt.
> + * @suspend: Standard suspend callback used during system suspend
> + * @resume: Standard resume callback used during system resume
> + * @device_up: This callback is called when the device reports present and
> + *		gets a logical address assigned to it
> + * @device_down: This callback is called when device reports absent, or the
> + *		bus goes down. Device will report present when bus is up and
> + *		device_up callback will be called again when that happens

do we need two callback, why not a status or notify callback with argument
for up/down?

-- 
~Vinod