Re: [PATCH] ACPI: Platform driver to support App Hot Startup (PNP0C32)

[Andi Kleen <andi@firstfloor.org>]

On Thu, Aug 28, 2008 at 03:47:45PM +0200, Ângelo Miguel Arrifano wrote:
> On Thu, 28 Aug 2008 15:08:05 +0200
> Andi Kleen <andi@firstfloor.org> wrote:
> 
> > > Ok thanks, here it is.
> > 
> > Please submit it with proper description and Signed-off-by lines.
> > (see Documentation/SubmittingPatches)
> > 
> > -Andi

Quick review:

Need a title (one line description) 

Best probably you run it through checkpatch.pl and fix the warnings
and possibly also Lindent.

> index c52fca8..ed48a56 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -199,6 +199,14 @@ config ACPI_THERMAL
>  	  recommended that this option be enabled, as your processor(s)
>  	  may be damaged without it.
>  
> +config ACPI_QUICKSTART
> +	tristate "Quickstart"
> +	default m

Please remove the default m

Also I think the position is wrong, this should be further down with
other misc ACPI drivers.

> +
> +#define QUICKSTART_PF_DRIVER_NAME     "quickstart"
> +#define QUICKSTART_PF_DEVICE_NAME     "quickstart"
> +#define QUICKSTART_PF_DEVATTR_NAME    "pressed_button"

I don't see the point of these defines. Can you just expand
them please? 

> +
> +/*
> + * ACPI driver Structs
> + */
> +
> +struct quickstart_acpi {
> +	struct acpi_device *device;
> +	struct quickstart_btn *btn;
> +};

There should be a new line here.

> +static int quickstart_acpi_add(struct acpi_device *device);
> +static int quickstart_acpi_remove(struct acpi_device *device, int type);

Standard practice is to reorder the file that forward declarations
are not needed.

> +
> +static struct acpi_driver quickstart_acpi_driver = {
> +	.name = "quickstart",
> +	.class = QUICKSTART_ACPI_CLASS,
> +	.ids = quickstart_device_ids,
> +	.ops = {
> +			.add = quickstart_acpi_add,
> +			.remove = quickstart_acpi_remove,

Unusual indentation.

> +		},
> +};
> +
> +/*
> + * Platform driver structs
> + */
> +static ssize_t buttons_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf);
> +static ssize_t pressed_button_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf);
> +static ssize_t pressed_button_store(struct device *dev,
> +					struct device_attribute *attr,
> +					 const char *buf,
> +					 size_t count);

See above for forward declarations.
> + */
> +static ssize_t buttons_show(struct device *dev,
> +					 struct device_attribute *attr,
> +					 char *buf)
> +{
> +	int count = 0;
> +	struct quickstart_btn *ptr = quickstart_data.btn_lst;
> +
> +	if (!ptr)
> +		return snprintf(buf, PAGE_SIZE, "none");
> +
> +	while (ptr && (count < PAGE_SIZE)) {
> +		if (ptr->name) {
> +			count += snprintf(buf + count,
> +					PAGE_SIZE - count,
> +					"%s\n", ptr->name);
> +		}
> +		ptr = ptr->next;
> +	}

Is it guaranteed that list is always shorter than
PAGE_SIZE? PAGE_SIZE - count might go negative otherwise
and snprintf will not handle that. Should be some proper limit.


> +
> +
> +static ssize_t pressed_button_store(struct device *dev,
> +					 struct device_attribute *attr,
> +					 const char *buf, size_t count)
> +{
> +	if (count < 2)
> +		return -EINVAL;

Check is redundant.

> +
> +	if (strncasecmp(buf, "none", 4) != 0)
> +		return -EINVAL;
> +
> +	quickstart_data.pressed = NULL;
> +	return count;
> +}
> +
> +/* Hotstart Helper functions */
> +static int quickstart_btnlst_add(struct quickstart_btn **data)
> +{
> +	struct quickstart_btn **ptr = &quickstart_data.btn_lst;
> +
> +	while (*ptr)
> +		ptr = &((*ptr)->next);

Would be all clearer if you used standard list.h lists.

> +	ret = quickstart_btnlst_add(&quickstart->btn);
> +	if (ret)
> +		return ret;
> +
> +	quickstart->btn->name = kzalloc(len + 1, GFP_KERNEL);

> +	if (!quickstart->btn->name) {
> +		quickstart_btnlst_free();
> +		return -ENOMEM;
> +	}
> +	strcpy(quickstart->btn->name, bid);

Use kstrndup()

> +
> +	platform_device_unregister(pf_device);
> +
> +	platform_driver_unregister(&pf_driver);
> +
> +	acpi_bus_unregister_driver(&quickstart_acpi_driver);
> +
> +	quickstart_btnlst_free();
> +
> +	return;

You have a lot of stray such returns; Remove them all.

> Signed-off-by: Angelo Miguel Arrifano <miknix@gmail.com>

That should be at the bottom of the description.

-Andi


-- 
ak@linux.intel.com
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html