Re: [PATCH 2/8] DSPBRIDGE: sysfs entry for global driver state

[Nishanth Menon <nm@ti.com>]

Ramirez Luna, Omar had written, on 01/08/2010 10:19 AM, the following:
>> From: Menon, Nishanth
>>
>> Ramirez Luna, Omar had written, on 01/07/2010 07:00 PM, the following:
>>> This patch creates a new sysfs entry to query the driver
>>> state without making any bridge direct call, this is
>>> useful to monitor driver state without creating any handle
>>> to bridge driver.
>> why sysfs and not debugfs considering that this is a debug feature?
> 
> It was placed under sysfs because one application we use for recovery relies on this driver state value, however the design of recovery is changing and this might be used only for debugging purposes; so agree to debugfs.
Thanks.

> 
>> this in my mind is a useful feature, no question, but I wonder if this
>> is a feature we might want to expand upon and allow for user space
>> applications to show beyond the basic run levels - show loading level(as
>> an example) thru the same interface?? just my 2 cents..
> 
> I'm not sure I catch the loading level part... but the states shown in the current patch are the ones available. I was thinking in future to show the amount of available DMM...
yes, I agree, but bridge also knows what frequency DSP is running on -> 
how about adding that info to running state? it is a little more useful 
(albeit in a non-standard way - that standard userspace app plugins can 
use - but a step in the right direction anyways..). here is why I am 
mentioning this:
N900 has a plugin to show me the cpu load - it has two bars - one 
showing the arm frequency, I would have loved to see the second bar show 
me the dsp status - loaded/idle/cpu load etc.. ;) might be a cool thing 
to have but I guess I am just a geek wishing to see more into my system :D..

> 
>>> Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
>>> ---
>>>  drivers/dsp/bridge/rmgr/drv_interface.c |   87 +++++++++++++++++++++++++++++++
>>>  1 files changed, 87 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c b/drivers/dsp/bridge/rmgr/drv_interface.c
>>> index c853854..3a4d058 100644
>>> --- a/drivers/dsp/bridge/rmgr/drv_interface.c
>>> +++ b/drivers/dsp/bridge/rmgr/drv_interface.c
>>> @@ -58,11 +58,13 @@
>>>  #include <linux/init.h>
>>>  #include <linux/moduleparam.h>
>>>  #include <linux/cdev.h>
>>> +#include <linux/kobject.h>
>>>
>>>  /*  ----------------------------------- DSP/BIOS Bridge */
>>>  #include <dspbridge/std.h>
>>>  #include <dspbridge/dbdefs.h>
>>>  #include <dspbridge/errbase.h>
>>> +#include <_tiomap.h>
>>>
>>>  /*  ----------------------------------- Trace & Debug */
>>>  #include <dspbridge/gt.h>
>>> @@ -148,6 +150,10 @@ static int omap34xxbridge_suspend_lockout(
>>>  }
>>>  #endif
>>>
>>> +/* Sysfs interface */
>>> +static void bridge_create_sysfs(struct platform_device *pdev);
>>> +static void bridge_destroy_sysfs(struct platform_device *pdev);
>>> +
>>>  #ifdef DEBUG
>>>  module_param(GT_str, charp, 0);
>>>  MODULE_PARM_DESC(GT_str, "GT string, default = NULL");
>>> @@ -297,6 +303,9 @@ static int __devinit omap34xx_bridge_probe(struct platform_device *pdev)
>>>  	device_create(bridge_class, NULL, MKDEV(driver_major, driver_minor),
>>>  			NULL, "DspBridge");
>>>
>>> +	/* Create sysfs interface */
>>> +	bridge_create_sysfs(pdev);
>>> +
>>>  	GT_init();
>>>  	GT_create(&driverTrace, "LD");
>>>
>>> @@ -454,6 +463,9 @@ func_cont:
>>>  	SERVICES_Exit();
>>>  	GT_exit();
>>>
>>> +	/* Remove driver sysfs entries */
>>> +	bridge_destroy_sysfs(pdev);
>>> +
>>>  	devno = MKDEV(driver_major, driver_minor);
>>>  	if (bridge_device) {
>>>  		cdev_del(&bridge_device->cdev);
>>> @@ -677,6 +689,81 @@ DSP_STATUS DRV_RemoveAllResources(HANDLE hPCtxt)
>>>  }
>>>  #endif
>>>
>>> +/*
>>> + * Sysfs
>> huh?
> 
> It marks beginning of sysfs functions, might be a bit ambiguous tough...
> 
>>> + */
>>> +static ssize_t drv_state_show(struct device *dev, struct device_attribute *attr,
>>> +				char *buf)
>>> +{
>>> +	struct WMD_DEV_CONTEXT *dev_ctxt;
>>> +	struct DEV_OBJECT *dev_obj = NULL;
>>> +	char *drv_state = "unknown";
>>> +
>>> +	dev_obj = (struct DEV_OBJECT *)DRV_GetFirstDevObject();
>>> +	DEV_GetWMDContext(dev_obj, &dev_ctxt);
>>> +
>>> +	if (!dev_ctxt)
>>> +		goto err;
>>> +
>>> +	switch (dev_ctxt->dwBrdState) {
>>> +	case BRD_STOPPED:
>>> +		drv_state = "stopped";
>>> +		break;
>>> +	case BRD_IDLE:
>>> +		drv_state = "idle";
>>> +		break;
>>> +	case BRD_RUNNING:
>>> +		drv_state = "running";
>>> +		break;
>>> +	case BRD_LOADED:
>>> +		drv_state = "loaded";
>>> +		break;
>>> +	case BRD_SLEEP_TRANSITION:
>>> +		drv_state = "sleep transition";
>>> +		break;
>>> +	case BRD_HIBERNATION:
>>> +		drv_state = "hibernation";
>>> +		break;
>>> +	case BRD_RETENTION:
>>> +		drv_state = "retention";
>>> +		break;
>>> +	case BRD_DSP_HIBERNATION:
>>> +		drv_state = "self initiated hibernation";
>>> +		break;
>>> +	case BRD_ERROR:
>>> +		drv_state = "error";
>>> +		break;
>> default?
>>
> 
> Agree, better to have it.
> 
>>> +	}
>>> +
>>> +err:
>>> +	return sprintf(buf, "%s\n", drv_state);
>>> +}
>>> +static DEVICE_ATTR(drv_state, S_IRUGO, drv_state_show, NULL);
>>> +
>>> +static struct attribute *attrs[] = {
>>> +	&dev_attr_drv_state.attr,
>>> +	NULL,
>>> +};
>>> +
>>> +static struct attribute_group attr_group = {
>>> +	.attrs = attrs,
>>> +};
>>> +
>>> +static void bridge_create_sysfs(struct platform_device *pdev)
>>> +{
>>> +	int error;
>>> +
>>> +	error = sysfs_create_group(&pdev->dev.kobj, &attr_group);
>>> +
>>> +	if (error)
>>> +		kobject_put(&pdev->dev.kobj);
>>> +}
>>> +
>>> +static void bridge_destroy_sysfs(struct platform_device *pdev)
>>> +{
>>> +	sysfs_remove_group(&pdev->dev.kobj, &attr_group);
>>> +}
>>> +
>>>  /* Bridge driver initialization and de-initialization functions */
>>>  module_init(bridge_init);
>>>  module_exit(bridge_exit);
>>
>> --
>> Regards,
>> Nishanth Menon


-- 
Regards,
Nishanth Menon