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@xxxxxxxxxxxxxx> 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@xxxxxxxxx>
That should be at the bottom of the description.
-Andi
--
ak@xxxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
