Re: [PATCH 1/3] SLIMbus: Device management on SLIMbus

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

Hi Sagar,

On 14/06/15 06:49, Sagar Dharia wrote:
> 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). So probe is called
> if the device is added to board-info list for a controller.
> 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@codeaurora.org>
> ---
>   drivers/Kconfig                 |   2 +
>   drivers/Makefile                |   1 +
>   drivers/slimbus/Kconfig         |   9 +
>   drivers/slimbus/Makefile        |   4 +
>   drivers/slimbus/slimbus.c       | 767 ++++++++++++++++++++++++++++++++++++++++
>   include/linux/mod_devicetable.h |  13 +
>   include/linux/slimbus.h         | 393 ++++++++++++++++++++
>   7 files changed, 1189 insertions(+)
>   create mode 100644 drivers/slimbus/Kconfig
>   create mode 100644 drivers/slimbus/Makefile
>   create mode 100644 drivers/slimbus/slimbus.c
>   create mode 100644 include/linux/slimbus.h
>

Good to see the slimbus patches :-)

Can you also add patch to add MAINTAINERS to this?

I like to try these patches on APQ8064 or any upstream Qcom platform, Do 
you have other patches to test this on APQ8064 or any A family SOCs/ B 
family SOCs which have upstream support?

Also I keep getting lost as I start looking at code, Am missing 
understanding of how these exported functions are going to be used by 
consumers/clients/controllers?
It would help if
1> document which explains how these apis are supposed to be used.
2> split up this patch into small patches, so that you can get good 
review comments.

Pl ignore if i have repeated the same comments someone else commented on.

> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index c0cc96b..e39c969 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -182,4 +182,6 @@ source "drivers/thunderbolt/Kconfig"
>
>   source "drivers/android/Kconfig"
>
> +source "drivers/slimbus/Kconfig"
> +
>   endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 46d2554..37c1c88 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -74,6 +74,7 @@ obj-$(CONFIG_TARGET_CORE)	+= target/
>   obj-$(CONFIG_MTD)		+= mtd/
>   obj-$(CONFIG_SPI)		+= spi/
>   obj-$(CONFIG_SPMI)		+= spmi/
> +obj-$(CONFIG_SLIMBUS)		+= slimbus/
>   obj-y				+= hsi/
>   obj-y				+= net/
>   obj-$(CONFIG_ATM)		+= atm/
> diff --git a/drivers/slimbus/Kconfig b/drivers/slimbus/Kconfig
> new file mode 100644
> index 0000000..fb30497
> --- /dev/null
> +++ b/drivers/slimbus/Kconfig
> @@ -0,0 +1,9 @@
> +#
> +# SLIMBUS driver configuration
> +#
> +menuconfig SLIMBUS
> +	tristate "Slimbus support"
> +	help
> +	  Slimbus is standard interface between baseband and audio codec,
> +	  and other peripheral components in mobile terminals.
> +
For consistency reasons could you fix on single style of SLIMbus vs 
Slimbus string in comments or description.

> diff --git a/drivers/slimbus/Makefile b/drivers/slimbus/Makefile
> new file mode 100644
> index 0000000..05f53bc
> --- /dev/null
> +++ b/drivers/slimbus/Makefile
> @@ -0,0 +1,4 @@
> +#
> +# Makefile for kernel slimbus framework.
> +#
> +obj-$(CONFIG_SLIMBUS)			+= slimbus.o
> diff --git a/drivers/slimbus/slimbus.c b/drivers/slimbus/slimbus.c
> new file mode 100644
> index 0000000..be4f2c7
> --- /dev/null
> +++ b/drivers/slimbus/slimbus.c
> @@ -0,0 +1,767 @@
> +/* Copyright (c) 2011-2015, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/gcd.h>
Do you need this?
> +#include <linux/completion.h>
> +#include <linux/idr.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slimbus.h>
> +
> +static DEFINE_MUTEX(slim_lock);
> +static DEFINE_IDR(ctrl_idr);
> +static struct device_type slim_dev_type;
> +static struct device_type slim_ctrl_type;
> +
> +static int slim_compare_eaddr(struct slim_eaddr *a, struct slim_eaddr *b)
> +{
> +	if (a->manf_id == b->manf_id && a->prod_code == b->prod_code &&
> +			a->dev_index == b->dev_index &&
> +			a->instance == b->instance)
> +		return 0;
> +	return -EIO;
A new line before return would make the code more readable, pl fix it in 
other instances too.

> +}
> +
> +static const struct slim_device_id *slim_match(const struct slim_device_id *id,
> +					const struct slim_device *slim_dev)
> +{
> +	while (id->manf_id != 0 || id->prod_code != 0) {
> +		if (id->manf_id == slim_dev->e_addr.manf_id &&
> +				id->prod_code == slim_dev->e_addr.prod_code &&
> +				id->dev_index == slim_dev->e_addr.dev_index)
> +			return id;
> +		id++;
> +	}
> +	return NULL;
> +}
> +
> +static int slim_device_match(struct device *dev, struct device_driver *driver)
> +{
> +	struct slim_device *slim_dev;
> +	struct slim_driver *drv = to_slim_driver(driver);
> +
> +	if (dev->type == &slim_dev_type)
> +		slim_dev = to_slim_device(dev);
> +	else
> +		return 0;
> +	if (drv->id_table)
Please use new lines where it makes things clear, not having new lines 
in instances like this makes the code look odd.

> +		return slim_match(drv->id_table, slim_dev) != NULL;
> +	return 0;
> +}
> +
...
> +
> +static void slim_device_shutdown(struct device *dev)
> +{
> +	struct slim_device *slim_dev;
> +	struct slim_driver *driver;
> +
> +	if (dev->type == &slim_dev_type)
> +		slim_dev = to_slim_device(dev);
> +	else
> +		return;
> +
> +	if (!dev->driver)
> +		return;
> +	driver = to_slim_driver(dev->driver);
> +	if (driver->shutdown)
> +		driver->shutdown(slim_dev);
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +
> +static int slim_pm_suspend(struct device *dev)
> +{
> +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +
Not sure why you need to do this, this is already in 
pm_generic_suspend/resume
> +	if (pm)
> +		return pm_generic_suspend(dev);
> +	else
> +		return 0;
You could just use return pm_generic_suspend(dev);
Or use directly.

	SET_SYSTEM_SLEEP_PM_OPS(pm_generic_suspend, pm_generic_resume)

> +}
> +
> +static int slim_pm_resume(struct device *dev)
> +{
> +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +
> +	if (pm)
> +		return pm_generic_resume(dev);
> +	else
> +		return 0;
> +}
> +
> +#else
> +#define slim_pm_suspend		NULL
> +#define slim_pm_resume		NULL
> +#endif
> +
> +static const struct dev_pm_ops slimbus_pm = {
> +	.suspend = slim_pm_suspend,
> +	.resume = slim_pm_resume,
> +	SET_RUNTIME_PM_OPS(
> +		pm_generic_suspend,
> +		pm_generic_resume,
> +		NULL
> +		)
> +};
> +struct bus_type slimbus_type = {
> +	.name		= "slimbus",
> +	.match		= slim_device_match,
> +	.probe		= slim_device_probe,
> +	.remove		= slim_device_remove,
> +	.shutdown	= slim_device_shutdown,
> +	.pm		= &slimbus_pm,
> +};
> +EXPORT_SYMBOL(slimbus_type);
> +
This module is GPL v2. why the symbols are not marked as GPL?
> +static void __exit slimbus_exit(void)
> +{
> +	bus_unregister(&slimbus_type);
> +}
> +
> +static int __init slimbus_init(void)
> +{
> +	return bus_register(&slimbus_type);
> +}
> +postcore_initcall(slimbus_init);
> +module_exit(slimbus_exit);
> +

Looks like this file is appended with multiple files..
Ideally init calls stay at the end of the file..

> +/*
> + * slim_driver_register: Client driver registration with slimbus
> + * @drv:Client driver to be associated with client-device.
> + * This API will register the client driver with the slimbus
> + * It is called from the driver's module-init function.
> + */
> +int slim_driver_register(struct slim_driver *drv)
> +{
> +	drv->driver.bus = &slimbus_type;
> +
> +	return driver_register(&drv->driver);
> +}
> +EXPORT_SYMBOL(slim_driver_register);
> +
> +/*
> + * slim_driver_unregister: Undo effect of slim_driver_register
> + * @drv: Client driver to be unregistered
> + */
> +void slim_driver_unregister(struct slim_driver *drv)
> +{
> +	if (drv)
> +		driver_unregister(&drv->driver);
> +}
> +
> +#define slim_ctrl_attr_gr NULL
Would prefer to see all the defines on top of the file.
> +
> +static void slim_ctrl_release(struct device *dev)
> +{
> +	struct slim_controller *ctrl = to_slim_controller(dev);
> +
> +	complete(&ctrl->dev_released);
> +}
> +
> +static struct device_type slim_ctrl_type = {
> +	.groups		= slim_ctrl_attr_gr,
> +	.release	= slim_ctrl_release,
> +};
> +
> +static struct slim_controller *slim_ctrl_get(struct slim_controller *ctrl)
> +{
> +	if (!ctrl || !get_device(&ctrl->dev))
> +		return NULL;
> +
> +	return ctrl;
> +}
> +
> +static void slim_ctrl_put(struct slim_controller *ctrl)
> +{
> +	if (ctrl)
> +		put_device(&ctrl->dev);
> +}
> +
> +#define slim_device_attr_gr NULL
> +#define slim_device_uevent NULL
Defines go on top.

I get a feeling that this file should be split into more files, so its 
easy to navigate.

> +static void slim_dev_release(struct device *dev)
> +{
> +	struct slim_device *sbdev = to_slim_device(dev);
> +
> +	slim_ctrl_put(sbdev->ctrl);
> +}
> +
> +static struct device_type slim_dev_type = {
> +	.groups		= slim_device_attr_gr,
> +	.uevent		= slim_device_uevent,
> +	.release	= slim_dev_release,
> +};
> +
> +static void slim_report(struct work_struct *work)
> +{
> +	struct slim_driver *sbdrv;
> +	struct slim_device *sbdev =
> +			container_of(work, struct slim_device, wd);
> +	if (!sbdev->dev.driver)
> +		return;
> +	/* check if device-up or down needs to be called */
> +	if ((!sbdev->reported && !sbdev->notified) ||
> +			(sbdev->reported && sbdev->notified))
> +		return;
> +
> +	sbdrv = to_slim_driver(sbdev->dev.driver);
> +	/*
> +	 * address no longer valid, means device reported absent, whereas
> +	 * address valid, means device reported present
> +	 */
> +	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);
> +	}
> +}
> +
> +/*
> + * slim_add_device: Add a new device without register board info.
> + * @ctrl: Controller to which this device is to be added to.
> + * Called when device doesn't have an explicit client-driver to be probed, or
> + * the client-driver is a module installed dynamically.
> + */
> +int slim_add_device(struct slim_controller *ctrl, struct slim_device *sbdev)
> +{
> +	sbdev->dev.bus = &slimbus_type;
> +	sbdev->dev.parent = ctrl->dev.parent;
> +	sbdev->dev.type = &slim_dev_type;
> +	sbdev->dev.driver = NULL;
> +	sbdev->ctrl = ctrl;
> +	slim_ctrl_get(ctrl);
> +	if (!sbdev->name) {
> +		sbdev->name = kcalloc(SLIMBUS_NAME_SIZE, sizeof(char),
> +					GFP_KERNEL);
> +		if (!sbdev->name)
> +			return -ENOMEM;
> +		snprintf(sbdev->name, SLIMBUS_NAME_SIZE, "0x%x:0x%x:0x%x:0x%x",
> +				sbdev->e_addr.manf_id, sbdev->e_addr.prod_code,
> +				sbdev->e_addr.dev_index,
> +				sbdev->e_addr.instance);
> +	}

kasprintf ?

> +	dev_dbg(&ctrl->dev, "adding device:%s", sbdev->name);
> +	dev_set_name(&sbdev->dev, "%s", sbdev->name);
> +	INIT_WORK(&sbdev->wd, slim_report);
> +	mutex_lock(&ctrl->m_ctrl);
> +	list_add_tail(&sbdev->dev_list, &ctrl->devs);
> +	mutex_unlock(&ctrl->m_ctrl);
> +	/* probe slave on this controller */
> +	return device_register(&sbdev->dev);
> +}
> +EXPORT_SYMBOL(slim_add_device);
> +
> +struct sbi_boardinfo {
> +	struct list_head	list;
> +	struct slim_boardinfo	board_info;
> +};
> +
> +static LIST_HEAD(board_list);
> +static LIST_HEAD(slim_ctrl_list);
> +static DEFINE_MUTEX(board_lock);
> +
> +/* If controller is not present, only add to boards list */
> +static void slim_match_ctrl_to_boardinfo(struct slim_controller *ctrl,
> +				struct slim_boardinfo *bi)
> +{
> +	int ret;
> +
> +	if (ctrl->nr != bi->bus_num)
> +		return;
> +
> +	ret = slim_add_device(ctrl, bi->slim_slave);
> +	if (ret != 0)
> +		dev_err(ctrl->dev.parent, "can't create new device %s, ret:%d",
> +			bi->slim_slave->name, ret);
> +}
> +
> +
> +/* slim_remove_device: Remove the effect of slim_add_device() */
> +void slim_remove_device(struct slim_device *sbdev)
> +{
> +	struct slim_controller *ctrl = sbdev->ctrl;
> +
> +	mutex_lock(&ctrl->m_ctrl);
> +	list_del_init(&sbdev->dev_list);
> +	mutex_unlock(&ctrl->m_ctrl);
> +	device_unregister(&sbdev->dev);
> +}
> +EXPORT_SYMBOL(slim_remove_device);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_VERSION("0.1");
> +MODULE_DESCRIPTION("Slimbus module");
> +MODULE_ALIAS("platform:slimbus");

> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 3bfd567..94abc09 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -427,6 +427,19 @@ struct spi_device_id {
>   	kernel_ulong_t driver_data;	/* Data private to the driver */
>   };
>
> +/* SLIMbus */
> +
> +#define SLIMBUS_NAME_SIZE	32
> +#define SLIMBUS_MODULE_PREFIX	"slim:"
> +
> +struct slim_device_id {
> +	__u16 manf_id, prod_code;
> +	__u8 dev_index, instance;
> +
> +	/* Data private to the driver */
> +	kernel_ulong_t driver_data;
> +};
> +
>   #define SPMI_NAME_SIZE	32
>   #define SPMI_MODULE_PREFIX "spmi:"
>
> diff --git a/include/linux/slimbus.h b/include/linux/slimbus.h
> new file mode 100644
> index 0000000..05b7594
> --- /dev/null
> +++ b/include/linux/slimbus.h
> @@ -0,0 +1,393 @@
> +/* Copyright (c) 2011-2015, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef _LINUX_SLIMBUS_H
> +#define _LINUX_SLIMBUS_H
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/mutex.h>
> +#include <linux/mod_devicetable.h>
> +
> +/*
> + * Interfaces between SLIMbus manager drivers, SLIMbus client drivers, and
> + * SLIMbus infrastructure.
> + */
> +
> +extern struct bus_type slimbus_type;
> +
> +/* Standard values per SLIMbus spec needed by controllers and devices */
> +#define SLIM_CL_PER_SUPERFRAME		6144
> +#define SLIM_CL_PER_SUPERFRAME_DIV8	(SLIM_CL_PER_SUPERFRAME >> 3)
> +#define SLIM_MAX_CLK_GEAR		10
> +#define SLIM_MIN_CLK_GEAR		1
> +#define SLIM_CL_PER_SL			4
> +#define SLIM_SL_PER_SUPERFRAME		(SLIM_CL_PER_SUPERFRAME >> 2)
> +#define SLIM_FRM_SLOTS_PER_SUPERFRAME	16
> +#define SLIM_GDE_SLOTS_PER_SUPERFRAME	2
> +
> +struct slim_controller;
> +struct slim_device;
> +
> +/*
> + * struct slim_eaddr: Enumeration address for a slimbus device
> + * @manf_id: Manufacturer Id for the device
> + * @prod_code: Product code
> + * @dev_index: Device index
> + * @instance: Instance value
> + */
> +struct slim_eaddr {
> +	u16 manf_id;
> +	u16 prod_code;
> +	u8 dev_index;
> +	u8 instance;
> +};
> +
> +/*
> + * struct slim_framer - Represents Slimbus framer.
> + * Every controller may have multiple framers. There is 1 active framer device
> + * responsible for clocking the bus.
> + * Manager is responsible for framer hand-over.
> + * @e_addr: Enumeration address of the framer.
> + * @rootfreq: Root Frequency at which the framer can run. This is maximum
> + *	frequency ('clock gear 10') at which the bus can operate.
> + * @superfreq: Superframes per root frequency. Every frame is 6144 bits.
> + */
> +struct slim_framer {
> +	struct slim_eaddr	e_addr;
> +	int			rootfreq;
> +	int			superfreq;
> +};
> +#define to_slim_framer(d) container_of(d, struct slim_framer, dev)
?? there is no dev in slim_framer ?
> +
> +/*
> + * 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 {
> +	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
> +
> +/* SLIMbus core type Message Codes. */
> +/* Device management messages used by this framework */
> +#define SLIM_MSG_MC_REPORT_PRESENT               0x1
> +#define SLIM_MSG_MC_ASSIGN_LOGICAL_ADDRESS       0x2
> +#define SLIM_MSG_MC_REPORT_ABSENT                0xF
> +
> +/* Destination type Values */
> +#define SLIM_MSG_DEST_LOGICALADDR	0
> +#define SLIM_MSG_DEST_ENUMADDR		1
> +#define	SLIM_MSG_DEST_BROADCAST		3
> +
> +/*
> + * struct slim_controller: Controls every instance of SLIMbus
> + *				(similar to 'master' on SPI)
> + *	'Manager device' is responsible for  device management, bandwidth
> + *	allocation, channel setup, and port associations per channel.
> + *	Device management means Logical address assignment/removal based on
> + *	enumeration (report-present, report-absent) if a device.
> + *	Bandwidth allocation is done dynamically by the manager based on active
> + *	channels on the bus, message-bandwidth requests made by slimbus devices.
> + *	Based on current bandwidth usage, manager chooses a frequency to run
> + *	the bus at (in steps of 'clock-gear', 1 through 10, each clock gear
> + *	representing twice the frequency than the previous gear).
> + *	Manager is also responsible for entering (and exiting) low-power-mode
> + *	(known as 'clock pause').
> + *	Manager can do handover of framer if there are multiple framers on the
> + *	bus and a certain usecase warrants using certain framer to avoid keeping
> + *	previous framer being powered-on.
> + *
> + *	Controller here performs duties of the manager device, and 'interface
> + *	device'. Interface device is responsible for monitoring the bus and
> + *	reporting information such as loss-of-synchronization, data
> + *	slot-collision.
> + * @dev: Device interface to this driver
> + * @nr: Board-specific number identifier for this controller/bus
> + * @list: Link with other slimbus controllers
> + * @name: Name for this controller
> + * @min_cg: Minimum clock gear supported by this controller (default value: 1)
> + * @max_cg: Maximum clock gear supported by this controller (default value: 10)
> + * @clkgear: Current clock gear in which this bus is running
> + * @a_framer: Active framer which is clocking the bus managed by this controller
> + * @m_ctrl: Mutex protecting controller data structures
> + * @addrt: Logical address table
> + * @num_dev: Number of active slimbus slaves on this bus
> + * @devs: List of devices on this controller
> + * @wq: Workqueue per controller used to notify devices when they report present
> + * @dev_released: completion used to signal when sysfs has released this
> + *	controller so that it can be deleted during shutdown
> + * @xfer_msg: Transfer a message on this controller (this can be a broadcast
> + *	control/status message like data channel setup, or a unicast message
> + *	like value element read/write.
> + * @set_laddr: Setup logical address at laddr for the slave with elemental
> + *	address e_addr. Drivers implementing controller will be expected to
> + *	send unicast message to this device with its logical address.
> + * @get_laddr: It is possible that controller needs to set fixed logical
> + *	address table and get_laddr can be used in that case so that controller
> + *	can do this assignment.
> + */
> +struct slim_controller {
> +	struct device		dev;
> +	unsigned int		nr;
> +	struct list_head	list;
> +	char			name[SLIMBUS_NAME_SIZE];
> +	int			min_cg;
> +	int			max_cg;
> +	int			clkgear;
> +	struct slim_framer	*a_framer;
> +	struct mutex		m_ctrl;
> +	struct slim_addrt	*addrt;
> +	u8			num_dev;
> +	struct list_head	devs;
> +	struct workqueue_struct *wq;
> +	struct completion	dev_released;
> +	int			(*set_laddr)(struct slim_controller *ctrl,
> +				struct slim_eaddr *ea, u8 laddr);
> +	int			(*get_laddr)(struct slim_controller *ctrl,
> +				struct slim_eaddr *ea, u8 *laddr);
> +};
> +#define to_slim_controller(d) container_of(d, struct slim_controller, dev)
> +
> +/*
> + * 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
> + * @reset_device: This callback is called after framer is booted.
> + *		Driver should do the needful to reset the device,
> + *		so that device acquires sync and be operational.
> + * @driver: Slimbus device drivers should initialize name and owner field of
> + *	this structure
> + * @id_table: List of slimbus devices supported by this driver
> + */
> +struct slim_driver {
> +	int				(*probe)(struct slim_device *sl);
> +	int				(*remove)(struct slim_device *sl);
> +	void				(*shutdown)(struct slim_device *sl);
> +	int				(*suspend)(struct slim_device *sl,
> +					pm_message_t pmesg);
> +	int				(*resume)(struct slim_device *sl);
> +	int				(*device_up)(struct slim_device *sl);
> +	int				(*device_down)(struct slim_device *sl);
> +	int				(*reset_device)(struct slim_device *sl);
> +
> +	struct device_driver		driver;
> +	const struct slim_device_id	*id_table;
> +};
> +#define to_slim_driver(d) container_of(d, struct slim_driver, driver)
> +
> +/*
> + * Client/device handle (struct slim_device):
> + * ------------------------------------------
> + *  This is the client/device handle returned when a slimbus
> + *  device is registered with a controller. This structure can be provided
> + *  during register_board_info, or can be allocated using slim_add_device API.
> + *  Pointer to this structure is used by client-driver as a handle.
> + *  @dev: Driver model representation of the device.
> + *  @name: Name of driver to use with this device.
> + *  @e_addr: Enumeration address of this device.
> + *  @driver: Device's driver. Pointer to access routines.
> + *  @ctrl: Slimbus controller managing the bus hosting this device.
> + *  @laddr: 1-byte Logical address of this device.
> + *  @reported: Flag to indicate whether this device reported present. The flag
> + *	is set when device reports present, and is reset when it reports
> + *	absent. This flag alongwith notified flag below is used to call
> + *	device_up, or device_down callbacks for driver of this device.
> + *  @notified: Flag to indicate whether this device has been notified. The
> + *	device may report present multiple times, but should be notified only
> + *	first time it has reported present.
> + *  @dev_list: List of devices on a controller
> + *  @wd: Work structure associated with workqueue for presence notification
> + */
> +struct slim_device {
> +	struct device		dev;
> +	char		*name;
> +	struct slim_eaddr	e_addr;
> +	struct slim_driver	*driver;
> +	struct slim_controller	*ctrl;
> +	u8			laddr;
> +	bool			reported;
> +	bool			notified;
> +	struct list_head	dev_list;
> +	struct work_struct	wd;
> +};
> +#define to_slim_device(d) container_of(d, struct slim_device, dev)
> +
> +/*
> + * struct slim_boardinfo: Declare board info for Slimbus device bringup.
> + * @bus_num: Controller number (bus) on which this device will sit.
> + * @slim_slave: Device to be registered with slimbus.
> + */
> +struct slim_boardinfo {
> +	int			bus_num;
> +	struct slim_device	*slim_slave;
> +};
> +
> +/* Manager's logical address is set to 0xFF per spec */
> +#define SLIM_LA_MANAGER 0xFF
> +/*
> + * slim_get_logical_addr: Return the logical address of a slimbus device.
> + * @sb: client handle requesting the adddress.
> + * @e_addr: Enumeration address of the device.
> + * @laddr: output buffer to store the address
> + * context: can sleep
> + * -EINVAL is returned in case of invalid parameters, and -ENXIO is returned if
> + *  the device with this elemental address is not found.
> + */

documentation should go to c-file..
> +
> +extern int slim_get_logical_addr(struct slim_device *sb,
> +				struct slim_eaddr *e_addr, u8 *laddr);
> +
> +/*
> + * slim_driver_register: Client driver registration with slimbus
> + * @drv:Client driver to be associated with client-device.
> + * This API will register the client driver with the slimbus
> + * It is called from the driver's module-init function.
> + */
> +extern int slim_driver_register(struct slim_driver *drv);
> +
> +/*
> + * slim_driver_unregister: Undo effects of slim_driver_register
> + * @drv: Client driver to be unregistered
> + */
> +extern void slim_driver_unregister(struct slim_driver *drv);
> +
> +/*
> + * slim_add_numbered_controller: Controller bring-up.
> + * @ctrl: Controller to be registered.
> + * A controller is registered with the framework using this API. ctrl->nr is the
> + * desired number with which slimbus framework registers the controller.
> + * Function will return -EBUSY if the number is in use.
> + */
> +extern int slim_add_numbered_controller(struct slim_controller *ctrl);
> +
> +/*
> + * slim_del_controller: Controller tear-down.
> + * Controller added with the above API is teared down using this API.
> + */
> +extern int slim_del_controller(struct slim_controller *ctrl);
> +
> +/*
> + * slim_add_device: Add a new device without register board info.
> + * @ctrl: Controller to which this device is to be added to.
> + * Called when device doesn't have an explicit client-driver to be probed, or
> + * the client-driver is a module installed dynamically.
> + */
> +extern int slim_add_device(struct slim_controller *ctrl,
> +			struct slim_device *sbdev);
> +
> +/* slim_remove_device: Remove the effect of slim_add_device() */
> +extern void slim_remove_device(struct slim_device *sbdev);
> +
> +/*
> + * slim_assign_laddr: Assign logical address to a device enumerated.
> + * @ctrl: Controller with which device is enumerated.
> + * @e_addr: Enumeration address of the device.
> + * @laddr: Return logical address (if valid flag is false)
> + * @valid: true if laddr holds a valid address that controller wants to
> + *	set for this enumeration address. Otherwise framework sets index into
> + *	address table as logical address.
> + * Called by controller in response to REPORT_PRESENT. Framework will assign
> + * a logical address to this enumeration address.
> + * Function returns -EXFULL to indicate that all logical addresses are already
> + * taken.
> + */
> +extern int slim_assign_laddr(struct slim_controller *ctrl,
> +			struct slim_eaddr *e_addr, u8 *laddr, bool valid);
> +
> +/*
> + * slim_report_absent: Controller calls this function when a device
> + *	reports absent, OR when the device cannot be communicated with
> + * @sbdev: Device that cannot be reached, or that sent report absent
> + */
> +void slim_report_absent(struct slim_device *sbdev);
> +
> +/*
> + * slim_framer_booted: This function is called by controller after the active
> + * framer has booted (using Bus Reset sequence, or after it has shutdown and has
> + * come back up). Components, devices on the bus may be in undefined state,
> + * and this function triggers their drivers to do the needful
> + * to bring them back in Reset state so that they can acquire sync, report
> + * present and be operational again.
> + */
> +void slim_framer_booted(struct slim_controller *ctrl);
> +
> +/*
> + * slim_ctrl_add_boarddevs: Add devices registered by board-info
> + * @ctrl: Controller to which these devices are to be added to.
> + * This API is called by controller when it is up and running.
> + * If devices on a controller were registered before controller,
> + * this will make sure that they get probed when controller is up
> + */
> +extern void slim_ctrl_add_boarddevs(struct slim_controller *ctrl);
> +
> +/*
> + * slim_register_board_info: Board-initialization routine.
> + * @info: List of all devices on all controllers present on the board.
> + * @n: number of entries.
> + * API enumerates respective devices on corresponding controller.
> + * Called from board-init function.
> + */
> +#ifdef CONFIG_SLIMBUS
> +extern int slim_register_board_info(struct slim_boardinfo const *info,
> +					unsigned n);
> +#else
> +static inline int slim_register_board_info(struct slim_boardinfo const *info,
> +					unsigned n)
> +{
> +	return 0;
Should'nt it return error code?

Also don't you want to add dummy functions for other functions?
--srini
> +}
> +#endif
> +
> +static inline void *slim_get_ctrldata(const struct slim_controller *dev)
> +{
> +	return dev_get_drvdata(&dev->dev);
> +}
> +
> +static inline void slim_set_ctrldata(struct slim_controller *dev, void *data)
> +{
> +	dev_set_drvdata(&dev->dev, data);
> +}
> +
> +static inline void *slim_get_devicedata(const struct slim_device *dev)
> +{
> +	return dev_get_drvdata(&dev->dev);
> +}
> +
> +static inline void slim_set_clientdata(struct slim_device *dev, void *data)
> +{
> +	dev_set_drvdata(&dev->dev, data);
> +}
> +
> +#endif /* _LINUX_SLIMBUS_H */
>