On Mon, 28 Sep 2009 22:38:00 -0300
Thadeu Lima de Souza Cascardo <cascardo@xxxxxxxxxxxxxx> wrote:
> This add supports for devices like keyboard, backlight, tablet and
> accelerometer.
>
> This work is supported by International Syst S/A.
>
I need to have a big whine about the coding style.
> +static acpi_status cmpc_start_accel(acpi_handle handle)
> +{
> + union acpi_object param[2];
> + struct acpi_object_list input;
> + acpi_status status;
> + param[0].type = ACPI_TYPE_INTEGER;
> + param[0].integer.value = 0x3;
> + param[1].type = ACPI_TYPE_INTEGER;
> + input.count = 2;
> + input.pointer = param;
> + status = acpi_evaluate_object(handle, "ACMD", &input, NULL);
> + return status;
> +}
To the jaded kernel developer's eye, this is quite hard to read. Every
function here squishes the declarations of the locals up against the
start of the code. It's a small thing, but this:
static acpi_status cmpc_start_accel(acpi_handle handle)
{
union acpi_object param[2];
struct acpi_object_list input;
acpi_status status;
param[0].type = ACPI_TYPE_INTEGER;
param[0].integer.value = 0x3;
param[1].type = ACPI_TYPE_INTEGER;
input.count = 2;
input.pointer = param;
status = acpi_evaluate_object(handle, "ACMD", &input, NULL);
return status;
}
puts a smile on our faces.
>
> ...
>
> +static ssize_t cmpc_accel_sense_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct acpi_device *acpi;
> + int sense;
> + acpi = to_acpi_device(dev);
> + if (sscanf(buf, "%d", &sense) <= 0)
> + return -EINVAL;
> + cmpc_accel_set_sense(acpi->handle, sense);
> + return strnlen(buf, count);
> +}
This will treat input of the form "42foo" as a valid decimal number.
That's a bit sloppy. Can we use strict_strtoul() here?
--
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
