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

[Srinivas Kandagatla]

Thanks for the Review Bjorn,

On 17/10/17 07:23, Bjorn Andersson wrote:
> On Fri 06 Oct 08:51 PDT 2017, srinivas.kandagatla at linaro.org wrote:
> [..]
>> diff --git a/drivers/slimbus/slim-core.c b/drivers/slimbus/slim-core.c
> [..]
>> +/**
>> + * Report callbacks(device_up, device_down) are implemented by slimbus-devices.
>> + * The calls are scheduled into a workqueue to avoid holding up controller
>> + * initialization/tear-down.
>> + */
>> +static void schedule_slim_report(struct slim_controller *ctrl,
>> +				 struct slim_device *sb, bool report)
>> +{
>> +	struct sb_report_wd *sbw;
>> +
>> +	dev_dbg(&ctrl->dev, "report:%d for slave:%s\n", report, sb->name);
> 
> This is the only place where sb->name is used in this driver. If you
> instead invoke dev_*() on &sb->dev you should get prettier output and
> can drop the double storage of the device name.

Makes sense, we could get rid of sb->name storage too.

> 
>> +
>> +	sbw = kmalloc(sizeof(*sbw), GFP_KERNEL);
>> +	if (!sbw)
>> +		return;
>> +
>> +	INIT_WORK(&sbw->wd, slim_report);
>> +	sbw->sbdev = sb;
>> +	sbw->report = report;
>> +	if (!queue_work(ctrl->wq, &sbw->wd)) {
> 
> When a controller is torn down destroy_workqueue() is called after all
> child devices has been unregistered, so this work might be scheduled
> after "sb" is gone, if I get this properly.

I agree, That is possible!
We should probably flush the workqueue before we start removing the clients.

> 
>> +		dev_err(&ctrl->dev, "failed to queue report:%d slave:%s\n",
>> +				    report, sb->name);
>> +		kfree(sbw);
>> +	}
>> +}
>> +
>> +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);
> 
> So a driver can have a probe() and device_up() or just any one of them?
> 
> And probe() is called when the controller enumerates all devices
> mentioned in DT and then device_up() is called at that point in time and
> when it's advertised on the bus?
> 
> Is there a reason for this split model?
> 
yes, Some of the devices need to be powered up before they become 
usable, so probe is used to do the initial power up of the device.


>> +
>> +	if (status)
>> +		sbdev->driver = NULL;
>> +	else if (sbdrv->device_up)
>> +		schedule_slim_report(sbdev->ctrl, sbdev, true);
>> +
>> +	return status;
>> +}
>> +
>> +static int slim_device_remove(struct device *dev)
>> +{
>> +	struct slim_device *sbdev;
>> +	struct slim_driver *sbdrv;
>> +	int status = 0;
>> +
>> +	sbdev = to_slim_device(dev);
>> +	if (!dev->driver)
>> +		return 0;
>> +
>> +	sbdrv = to_slim_driver(dev->driver);
>> +	if (sbdrv->remove)
>> +		status = sbdrv->remove(sbdev);
>> +
>> +	mutex_lock(&sbdev->report_lock);
>> +	sbdev->notified = false;
>> +	if (status == 0)
>> +		sbdev->driver = NULL;
>> +	mutex_unlock(&sbdev->report_lock);
>> +	return status;
> 
> device_unregister() will call device_del() which will end up in
> __device_release_driver() which will call this function. Upon returning
> from this function the core expect the bus to have cleaned up after the
> dev (normally by calling drv->remove(dev)).
> 
> It will completely ignore the return value and continue tearing down the
> rest of the core resources, e.g. three lines down it will
> devres_release_all().
> 
> 
> So you have the option of sleeping, while waiting for stuff to be
> aborted/finished and then you need to clean things up.
> 
> The slim_device object itself will stick around until all references are
> dropped though.

So you are suggesting that we make slim_driver remove not return anything?

> 
>> +}
>> +
>> +struct bus_type slimbus_type = {
>> +	.name		= "slimbus",
>> +	.match		= slim_device_match,
>> +	.probe		= slim_device_probe,
>> +	.remove		= slim_device_remove,
>> +};
>> +EXPORT_SYMBOL_GPL(slimbus_type);
>> +
>> +/**
>> + * slim_driver_register: Client driver registration with slimbus
>> + * @drv:Client driver to be associated with client-device.
>> + * @owner: owning module/driver
>> + * 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, struct module *owner)
>> +{
>> +	drv->driver.bus = &slimbus_type;
>> +	drv->driver.owner = owner;
>> +	return driver_register(&drv->driver);
>> +}
>> +EXPORT_SYMBOL_GPL(__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)
> 
> A driver invoking slim_driver_unregister(NULL) is broken, drop this
> check and let it oops on the dereference instead.

Yep.

> 
>> +		driver_unregister(&drv->driver);
>> +}
>> +EXPORT_SYMBOL_GPL(slim_driver_unregister);
>> +
>> +static struct slim_controller *slim_ctrl_get(struct slim_controller *ctrl)
>> +{
>> +	if (!ctrl || !get_device(&ctrl->dev))
> 
> ctrl can't be NULL here. In all code paths leading here it's
> dereferenced multiple times already.

I agree.

> 
>> +		return NULL;
>> +
>> +	return ctrl;
>> +}
>> +
>> +static void slim_ctrl_put(struct slim_controller *ctrl)
>> +{
>> +	if (ctrl)
>> +		put_device(&ctrl->dev);
>> +}
>> +
>> +static void slim_dev_release(struct device *dev)
>> +{
>> +	struct slim_device *sbdev = to_slim_device(dev);
>> +
>> +	slim_ctrl_put(sbdev->ctrl);
> 
> As far as I can see there's no case where sbdev->ctrl will ever be NULL,
> so yo can just replace this with
> 	put_device(&ctrl->dev);
> 
> And drop slim_ctrl_put().


Yes..

> 
>> +	kfree(sbdev->name);
>> +	kfree(sbdev);
>> +}
>> +
>> +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);
> 
> Unfolding slim_ctrl_get(), with ctrl != NULL, gives us a container_of(),
> so this can't fail. Which is good because then an error check would have
> been nice.
> 
> But it also means that you can replace this with just:
> 	get_device(&ctrl->dev);
> 
> And drop slim_ctrl_get()
> 

ya.

>> +	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);
> 
> This will create another copy of the same string and as noted above
> there seems to be only one consumer, which could be switched over. So
> you can drop above kasprintf() and just use dev_set_name to format the
> name.
> 
> An added benefit is that you're not leaking the device reference from
> slim_ctrl_get() on the line before.

I agree, will fix this in next version.

> 
>> +	mutex_init(&sbdev->report_lock);
>> +
>> +	/* probe slave on this controller */
>> +	return device_register(&sbdev->dev);
>> +}
>> +
>> +/* Helper to get hex Manufacturer ID and Product id from compatible */
>> +static unsigned long str2hex(unsigned char *str)
> 
> The caller of this passes char *, so you can drop the unsigned. And add
> "const" while you're at it.

I will be removing this function in next version, replacing it with 
kstrtoul()

> 
>> +{
>> +	int value = 0;
>> +
>> +	while (*str) {
>> +		char c = *str++;
>> +
>> +		value = value << 4;
>> +		if (c >= '0' && c <= '9')
>> +			value |= (c - '0');
>> +		if (c >= 'a' && c <= 'f')
>> +			value |= (c - 'a' + 10);
> 
> At the cost of one more check here you can drop the line in the
> documentation about this only working for lower-case hex digits.
> 
>> +
>> +	}
>> +
>> +	return value;
>> +}
>> +
>> +/* 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);
> 
> This is leaked in several places below.
> 
>> +		if (!slim)
>> +			continue;
>> +
>> +		slim->dev.of_node = of_node_get(node);
> 
> Dito.
> 
>> +
>> +		compat = of_get_property(node, "compatible", NULL);
>> +		if (!compat)
>> +			continue;
>> +
>> +		p = kasprintf(GFP_KERNEL, "%s", compat + strlen("slim"));
> 
> Allocating a new string using string formatting based on an offset from
> a string we don't know the size of, just to tokenize it does not seem
> like the most efficient (nor safe) way of doing this.
> 
> 
> How about:
> 		ret = sscanf(compat, "slim%x,%x", &manf_id, &prod_code);
> 		if (ret != 2)
> 			error();
> 
Looks much better!


>> +
>> +		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);
>> +		if (ret)
>> +			dev_err(dev, "of_slim device register err:%d\n", ret);
> 
> Cleanup if this fails.
Yes, we are leaking memory here, will fix this in next version.
> 
>> +	}
>> +}
>> +
>> +/**
>> + * 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);
> 
> The purpose of ctrl_idr is to generate unique ids for the name and to
> check that slim_del_controller() is only called on valid
> slim_controllers.
> 
> The latter is okay to just expect the controller drivers to do right and
> the prior is better done with an ida.
> 

I agree, IDA seems to be better fit here.

> 
> Also, the lower boundary should be 0, not ctrl->nr.
> 
>> +	mutex_unlock(&slim_lock);
>> +
>> +	if (id < 0)
>> +		return id;
>> +
>> +	ctrl->nr = id;
>> +
>> +	dev_set_name(&ctrl->dev, "sb-%d", ctrl->nr);
> 
> This name is used in a lot of debug prints, can we do better?
> 
In some of the previous discussions with Vinod, it was suggested that 
slim_register_controller should not create an additional device here, 
which is redundant to the actual controller device itself.

So this name would actually come from the controller driver in next 
version of patches.

>> +	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);
>> +	if (ret)
>> +		goto dev_reg_failed;
>> +
>> +	dev_dbg(&ctrl->dev, "Bus [%s] registered:dev:%p\n",
>> +		ctrl->name, &ctrl->dev);
> 
> This is the only place ctrl->name is used. Perhaps it would make more
> sense to base the dev_name off this string, to make all these dev_*()
> more useful.

Agreed.
> 
>> +
>> +	ctrl->wq = create_singlethread_workqueue(dev_name(&ctrl->dev));
>> +	if (!ctrl->wq)
>> +		goto err_workq_failed;
>> +
>> +	of_register_slim_devices(ctrl);
>> +
>> +	return 0;
>> +
>> +err_workq_failed:
>> +	device_unregister(&ctrl->dev);
>> +dev_reg_failed:
>> +	mutex_lock(&slim_lock);
>> +	idr_remove(&ctrl_idr, ctrl->nr);
>> +	mutex_unlock(&slim_lock);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(slim_register_controller);
>> +
>> +/* slim_remove_device: Remove the effect of slim_add_device() */
>> +static void slim_remove_device(struct slim_device *sbdev)
>> +{
>> +	device_unregister(&sbdev->dev);
>> +}
>> +
>> +static int slim_ctrl_remove_device(struct device *dev, void *null)
>> +{
>> +	slim_remove_device(to_slim_device(dev));
>> +	return 0;
>> +}
>> +
>> +/**
>> + * slim_del_controller: Controller tear-down.
>> + * @ctrl: Controller to tear-down.
>> + */
>> +int slim_del_controller(struct slim_controller *ctrl)
> 
> This is the opposite of slim_register_controller() so should it perhaps
> be called slim_unregister_controller() ?

makes sense..
> 
>> +{
>> +	struct slim_controller *found;
>> +
>> +	/* First make sure that this bus was added */
>> +	mutex_lock(&slim_lock);
>> +	found = idr_find(&ctrl_idr, ctrl->nr);
>> +	mutex_unlock(&slim_lock);
>> +	if (found != ctrl)
>> +		return -EINVAL;
> 
> Just rely on the caller doing the right thing and just
> 	ida_remove(&ctrl_ida, ctrl->nr);

Moving to ida is best

> 
>> +
>> +	/* Remove all clients */
>> +	device_for_each_child(&ctrl->dev, NULL, slim_ctrl_remove_device);
>> +
> 
> As stated above there might be work items left in flight here, after the
> slim_devices are released.
> 
We should flush the workqueue before we remove the devices.

>> 
>> +	destroy_workqueue(ctrl->wq);
>> +
>> +	/* free bus id */
>> +	mutex_lock(&slim_lock);
>> +	idr_remove(&ctrl_idr, ctrl->nr);
>> +	mutex_unlock(&slim_lock);
>> +
>> +	device_unregister(&ctrl->dev);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(slim_del_controller);
> [..]
>> +/**
>> + * slim_query_device: Query and get handle to a device.
>> + * @ctrl: Controller on which this device will be added/queried
>> + * @e_addr: Enumeration address of the device to be queried
>> + * Returns pointer to a device if it has already reported. Creates a new
>> + * device and returns pointer to it if the device has not yet enumerated.
>> + */
>> +struct slim_device *slim_query_device(struct slim_controller *ctrl,
>> +				      struct slim_eaddr *e_addr)
>> +{
>> +	struct device *dev;
>> +	struct slim_device *slim = NULL;
> 
> This will be written before read, so please don't initialize.
> 
yep.

>> +
>> +	dev = device_find_child(&ctrl->dev, e_addr, slim_match_dev);
>> +	if (dev) {
>> +		slim = to_slim_device(dev);
>> +		return slim;
>> +	}
>> +
>> +	slim = kzalloc(sizeof(struct slim_device), GFP_KERNEL);
>> +	if (IS_ERR(slim))
> 
> !slim
yep.

> 
>> +		return NULL;
>> +
>> +	slim->e_addr = *e_addr;
>> +	if (slim_add_device(ctrl, slim) != 0) {
> 
> The idiomatic way is:
> 
> ret = fn();
> if (ret) {
> 	failure...
> }
> 
okay
>> +		kfree(slim);
>> +		return NULL;
>> +	}
>> +	return slim;
>> +}
>> +EXPORT_SYMBOL_GPL(slim_query_device);
>> +
>> +static int ctrl_getaddr_entry(struct slim_controller *ctrl,
>> +			      struct slim_eaddr *eaddr, u8 *entry)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < ctrl->num_dev; i++) {
>> +		if (ctrl->addrt[i].valid &&
>> +		    slim_eaddr_equal(&ctrl->addrt[i].eaddr, eaddr)) {
>> +			*entry = i;
> 
> i will be >= 0, so it can easily be distinguished from -ENXIO. So you
> could return that directly instead of passing "i" as a reference and to
> get the value.
> 
> On the other hand the return value is only used as an offset in addrt[]
> to read out addrt[i].laddr, so perhaps you should just return that
> instead (as an int).
> 
sounds sensible!

>> +			return 0;
>> +		}
>> +	}
>> +	return -ENXIO;
>> +}
>> +
>> +/**
>> + * slim_assign_laddr: Assign logical address to a device enumerated.
> 
> So I presume this will either report a new (not seen before) e_addr
> which should cause a new device to be spawned (although it might be
> already mentioned in DT) or it might be called for an existing device to
> update the logical address.
> 
> Can you describe when the latter is the case? Or is this a side effect
> of the code, rather than slimbus?

If this function is called for an existing device with logical address, 
it would give the same logical address, if it finds a vaild one in the 
table.
Other case is when it does not find a valid on then a new address is 
assigned depending on the valid flag passed to the function.
This case is more to do with the Qualcomm B family Slim controller, 
where linux side can not assign logical address, As DSP would only be 
able to assign this address.

>> + * @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.
> 
> How do you ensure this laddr is unique?
Its index into a table which is based on unique enumeration 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.
>> + */
>> +int slim_assign_laddr(struct slim_controller *ctrl, struct slim_eaddr *e_addr,
>> +		      u8 *laddr, bool valid)
>> +{
>> +	int ret;
>> +	u8 i = 0;
>> +	bool exists = false;
>> +	struct slim_device *slim;
>> +	struct slim_addrt *temp;
>> +
>> +	mutex_lock(&ctrl->m_ctrl);
>> +	/* already assigned */
>> +	if (ctrl_getaddr_entry(ctrl, e_addr, &i) == 0) {
> 
> So I presume this updates an existing e_addr -> laddr mapping. But this
> should imply that there is an associated slim_device with this e_addr
> and laddr. Shouldn't said slim_device have its laddr updated then?

If we find a vaild an matching entry in the list, we assume that the 
slim device is already aware of this laddr.


> 
>> +		*laddr = ctrl->addrt[i].laddr;
>> +		exists = true;
>> +	} else {
>> +		if (ctrl->num_dev >= (SLIM_LA_MANAGER - 1)) {
>> +			ret = -EXFULL;
>> +			goto ret_assigned_laddr;
>> +		}
>> +		for (i = 0; i < ctrl->num_dev; i++) {
>> +			if (ctrl->addrt[i].valid == false)
>> +				break;
>> +		}
>> +		if (i == ctrl->num_dev) {
>> +			temp = krealloc(ctrl->addrt,
>> +					(ctrl->num_dev + 1) *
>> +					sizeof(struct slim_addrt),
>> +					GFP_KERNEL);
>> +			if (!temp) {
>> +				ret = -ENOMEM;
>> +				goto ret_assigned_laddr;
>> +			}
>> +			ctrl->addrt = temp;
>> +			ctrl->num_dev++;
>> +		}
> 
> This seems better handled by a list than an array that we realloc. But
> better yet, this array seems to mirror the list of registered devices.
> 
> So why doesn't this function just try to resolve the slim_device with
> e_addr and if not allocate a new slim_device with a free laddr. Using an
> ida to keep track of used logical addresses would make this much
> cleaner.
> 
> Or a idr keyed by the laddr, it would still be O(n) to scan based on
> e_addr, but this logic as well as lookups by laddr would be cleaner.
> 
I will give this a try before sending next version and see how it looks!

>> +		ctrl->addrt[i].eaddr = *e_addr;
>> +		ctrl->addrt[i].valid = true;
>> +
>> +		/* Preferred address is index into table */
>> +		if (!valid)
>> +			*laddr = i;
> 
> Is this laddr available?
this is the new index or first instance of invalid entry in the table, 
so it should be available.
> 
>> +	}
>> +
>> +	ret = ctrl->set_laddr(ctrl, &ctrl->addrt[i].eaddr, *laddr);
>> +	if (ret) {
>> +		ctrl->addrt[i].valid = false;
>> +		goto ret_assigned_laddr;
>> +	}
>> +	ctrl->addrt[i].laddr = *laddr;
>> +
>> +ret_assigned_laddr:
>> +	mutex_unlock(&ctrl->m_ctrl);
>> +	if (exists || ret)
>> +		return ret;
>> +
>> +	dev_info(&ctrl->dev, "setting slimbus l-addr:%x, ea:%x,%x,%x,%x\n",
>> +		*laddr, e_addr->manf_id, e_addr->prod_code,
>> +		e_addr->dev_index, e_addr->instance);
>> +
>> +	/**
>> +	 * Add this device to list of devices on this controller if it's
>> +	 * not already present
>> +	 */
>> +	slim = slim_query_device(ctrl, e_addr);
>> +	if (!slim) {
>> +		ret = -ENODEV;
>> +	} else {
>> +		struct slim_driver *sbdrv;
>> +
>> +		slim->laddr = *laddr;
>> +		mutex_lock(&slim->report_lock);
>> +		slim->reported = true;
>> +		if (slim->dev.driver) {
>> +			sbdrv = to_slim_driver(slim->dev.driver);
>> +			if (sbdrv->device_up)
>> +				schedule_slim_report(ctrl, slim, true);
>> +		}
>> +		mutex_unlock(&slim->report_lock);
> 
> I can't help feeling that this is the one and only point where you
> should call probe() on the slim_device. Are there some funky
> dependencies or protocol issues that makes this infeasible?

The reason probe is called earlier because the slim device needs 
power-up or reset sequence to make it discoverable on the bus.

> 
>> +	}
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(slim_assign_laddr);
>> +
>> +/**
>> + * slim_get_logical_addr: Return the logical address of a slimbus device.
>> + * @sb: client handle requesting the address.
>> + * @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 enumeration address is not found.
>> + */
>> +int slim_get_logical_addr(struct slim_device *sb, struct slim_eaddr *e_addr,
>> +			  u8 *laddr)
> 
> In what case would e_addr != sb->e_addr and why can't this function just
> be return sb->laddr?
> 
I don't think e_addr and sb->eaddr should be different in this case.

In usecase like B family Qualcomm SOCs, where linux cannot assign 
logical address, It needs to get logical address from ADSP and then use 
that. Current model of get_logical_addr provides abstraction so that 
this ADSP communication to get logical addr is hidden from client.


>> +{
>> +	int ret;
>> +	u8 entry;
>> +	struct slim_controller *ctrl = sb->ctrl;
>> +
>> +	if (!ctrl || !laddr || !e_addr)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&ctrl->m_ctrl);
>> +	ret = ctrl_getaddr_entry(ctrl, e_addr, &entry);
>> +	if (!ret)
>> +		*laddr = ctrl->addrt[entry].laddr;
>> +	mutex_unlock(&ctrl->m_ctrl);
>> +
>> +	if (ret == -ENXIO && ctrl->get_laddr) {
>> +		ret = ctrl->get_laddr(ctrl, e_addr, laddr);
>> +		if (!ret)
>> +			ret = slim_assign_laddr(ctrl, e_addr, laddr, true);
>> +	}
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(slim_get_logical_addr);
> [..]
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_VERSION("0.1");
> 
> Who will ever update this version number? It's probably better to just
> omit it.
>
Yep.

>> +MODULE_DESCRIPTION("Slimbus module");
> 
> Rather than "Slimbus module", this is actually the "Slimbus core".
> 
Yep.
>> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> [..]
>> +extern struct bus_type slimbus_type;
> 
> The device struct has a "struct device_type *type", which causes this
> name to be confusing. Please rename it slimbus_bus instead.
> 
> Why does this bus_type have to be known to the world?
May be not. can be removed.
> 
>> +
>> +/* 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
> 
> Keep the min/max here and move the rest to the patches that introduce
> consumers of them.

makes sense!

> 
> [..]
>> +/* 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
> 
> These are not currently used, move them to the patch that actually use
> them.
> 
>> +
>> +/* 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
> 
> Dito
> 
>> +
>> +/* Destination type Values */
>> +#define SLIM_MSG_DEST_LOGICALADDR	0
>> +#define SLIM_MSG_DEST_ENUMADDR		1
>> +#define	SLIM_MSG_DEST_BROADCAST		3
> 
> Dito

will do it in next version.


> 
>> +
>> +/**
>> + * 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
> 
> This mutex protects operations on the addrt array, so both name and
> documentation can be improved.
Agreed, will address this in next version.

> 
>> + * @addrt: Logical address table
> 
> Consider replacing with a idr, keyed by laddr. If there's actually a
> point in having this list...
> 
>> + * @num_dev: Number of active slimbus slaves on this bus
> 
> This is not so much "number of devices", but rather the length of @addrt.

lenght of addrt might be more than active devices as some of the entries 
in addrt are not valid after the device goes down.


> 
>> + * @wq: Workqueue per controller used to notify devices when they report present
>> + * @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.
> 
> Can you describe the use case for get_laddr() a little bit more?

Usecase is  for  B family Qualcomm SOCs, where linux cannot assign 
logical address, It needs to get logical address from ADSP and then use 
that. Current model of get_logical_addr provides abstraction so that 
this ADSP communication to get logical addr is hidden from client.

> 
>> + */
>> +struct slim_controller {
>> +	struct device		dev;
>> +	unsigned int		nr;
>> +	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 workqueue_struct *wq;
>> +	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);
> 
> If nothing else I think this should return the laddr, rather than pass
> it back into the referenced u8.

I will give it a go and see how it fits with every thing around it.

> 
>> +};
>> +
> 
> Regards,
> Bjorn
>