Re: [PATCH 4/5] ideapad: add debugfs support

[Ike Panhc <ike.pan@canonical.com>]

Some modification.

On 09/06/2011 02:32 AM, Ike Panhc wrote:
>  
>  static acpi_handle ideapad_handle;
> +static struct ideapad_private *ideapad_priv;

This is not necessary.

>  static bool no_bt_rfkill;
>  module_param(no_bt_rfkill, bool, 0444);
>  MODULE_PARM_DESC(no_bt_rfkill, "No rfkill for bluetooth.");
> @@ -188,6 +192,146 @@ static int write_ec_cmd(acpi_handle handle, int cmd, unsigned long data)
>  }
>  
>  /*
> + * debugfs
> + */
> +#define DEBUGFS_EVENT_LEN (4096)

No one use this.

> +static int debugfs_cfg_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, debugfs_cfg_show, NULL);

priv structure can be the third argument, so global variable is not necessary.

> +}
> +
> +static const struct file_operations debugfs_cfg_fops = {
> +	.owner = THIS_MODULE,
> +	.open = debugfs_cfg_open,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = single_release,
> +};
> +
> +static int __devinit ideapad_debugfs_init(struct ideapad_private *priv)
> +{
> +	struct dentry *node;
> +
> +	priv->debug = debugfs_create_dir("ideapad", NULL);
> +	if (priv->debug == NULL) {

Use IS_ERR_OR_NULL instead.
When CONFIG_DEBUG_FS is not set, debugfs_create_dir will return ERR_PTR(-ENODEV)

> +		pr_err("failed to create debugfs directory");
> +		goto errout;
> +	}
> +
> +	node = debugfs_create_file("cfg", S_IRUGO, priv->debug, NULL,
> +				   &debugfs_cfg_fops);

priv structure can be the third argument.

> +	if (!node) {
> +		pr_err("failed to create cfg in debugfs");
> +		goto errout;
> +	}
> +
> +	node = debugfs_create_file("status", S_IRUGO, priv->debug, NULL,
> +				   &debugfs_status_fops);
> +	if (!node) {
> +		pr_err("failed to create event in debugfs");
> +		goto errout;
> +	}
> +
> +	return 0;
> +
> +errout:

call ideapad_debugfs_exit when failed.

> +	return -ENOMEM;
> +}
> +

> @@ -573,6 +717,7 @@ static int __devinit ideapad_acpi_add(struct acpi_device *adevice)
>  	if (!priv)
>  		return -ENOMEM;
>  	dev_set_drvdata(&adevice->dev, priv);
> +	ideapad_priv = priv;

Removed

>  	ideapad_handle = adevice->handle;
>  	priv->cfg = cfg;
>  
> @@ -580,6 +725,10 @@ static int __devinit ideapad_acpi_add(struct acpi_device *adevice)
>  	if (ret)
>  		goto platform_failed;
>  
> +	ret = ideapad_debugfs_init(priv);
> +	if (ret)
> +		goto debugfs_failed;
> +

Not to fail out when debugfs init failed. debugfs is for developers, not for users.