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

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

On 10/10/17 17:49, Vinod Koul wrote:
> On Tue, Oct 10, 2017 at 01:34:55PM +0100, Srinivas Kandagatla wrote:
>>>>   9 files changed, 1192 insertions(+)
>>>
>>> thats a lot of code for review, consider splitting it up further for better
>>> reviews
>>
>> Its was suggested that parts of dtbindings and of_* wrapper merged into this
>> patch.  In V5 review comments. https://lkml.org/lkml/2016/4/28/175
> 
> yes but it can still be split :)
> 
Will give it a go in next version!!
> 
>>>> +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
>> Yes, we need this to match the compatible string from device tree vs driver
>> itself, most of the bus driver do this in bus match functions.
>>
>>
>>> DT so pardon my ignorance). Since we have devices with ids can we use that
>>> alone for matching?
>>
>> Two cases to consider here,
>> 1> If the device is up and discoverable.
>> 2> Device is not discoverable yet, as it needs some power up sequence.
>>
>>
>> In first case comparing with ID table makes sense.
>>
>> But second case we would want to probe the device(for power sequencing)
>> before we can discover the device on bus.
>>
>>
>> This code as it is supports both DT and id_table.
> 
> Why not only id_table, see below:
> 


Yes, we make id_table as mandatory field for all slimbus drivers.


>>>> +	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?
>>>
>> up would be called when a device is discovered on the bus, and down on when
>> the device disappeared on slimbus.
>>
>>>> +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?
>>
>> It is scheduling a device_up() callback in workqueue for reporting
>> discovered device.
> 
> any reason for that? Would the device not announce itself on the bus and
> then you can synchronously update the device.
You are correct,  Device should announce itself in all cases. core 
should only call this callback only when device is announced, it does 
not make sense for this call in slim_device_probe(). Will remove it from 
here in next version.


> 
>>>> +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?
>>
>> This is made inline with __platfrom_driver_register() suggested in v5
>> review.
> 
> I guess Greg is best person to make this call :)
> 

Forgot to put original comment in v5 by Arnd: 
https://lkml.org/lkml/2016/4/28/179


>>>> +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?
>>
>> I partly agree with you, as all the devices on slimbus might not be in a
>> discoverable state. Such devices would need some sort of power up sequence
>> which what the of_wrapper and the match function are trying to achieve.
>> Driver probe will be called based on the compatible match which would then
>> power up/reset the device so that it can announce itself and the device_up()
>> would be called at that point.
>>
>> Your comment is 100% true, If the devices are in discoverable state, in such
>> case we would not need any DT entires as you said.
> 
> Yes you are right. Since the device need to be powered up thru side band we
> cannot live without using firmware (acpi/dt)
> 
> Now consider the below scheme:
>   - The Bus scans device node of the controller (acpi/dt), as
>     above and find the slim devices and adds them. The ID needs to be
>     extracted from firmware (acpi/dt), we sure need to set the right firmware
>     node for the device
>   - Drivers register based on ID, no need to make it DT/ACPI aware
>   - Match function is invoked and driver probed
>   - Sideband mechanism kick in and power up device and it announces on the
>     bus
> 

I will make id_table mandatory in next version, which should remove the 
of_matching function and code should look as you suggested.

> In case it is already powered up, it can announce being up earlier.
> 
> FWIW i am using above method with ACPI on SoundWire.
Ah..okay

> 
>>
>>>
>>>> +		if (ret)
>>>> +			dev_err(dev, "of_slim device register err:%d\n", ret);
>>>> +	}
>>>> +}
>>>> +
>>>> +/**
>>>> + * slim_register_controller: Controller bring-up and registration.
...
>>>> +
>>>> +	mutex_init(&ctrl->m_ctrl);
>>>> +	ret = device_register(&ctrl->dev);
>>>
>>> one more device_register?? Can you explain why
>>>
>>
>> This is a device for each controller.
> 
> wont the controller have its own platform_device?

Reason could be that slim_register controller can be called from any 
code not just platform devices..


>