On Wed, 3 Dec 2008, Matthew Garrett wrote:
> oqo-wmi provides a WMI-based interface to backlight and rfkill control on
> model 2 OQO devices. It was originally written by Brian Julin
> (<bri@xxxxxxxxx>) - I've ported it to the rfkill layer and tidied it up a
> little.
Some general comments below.
> +/* Some of this code is left like in acer-wmi so we can add the older
> + Model 01 and any future models more easily, but we should not expect
> + it to be as complicated as Acer given each model is a leap rather than
> + a subtle variant on the last, so we aren't using "quirks" perse. Not
> + sure if there is any real difference for our purposes between the o2
> + and e2.
> +*/
> +struct oqo_model {
> + const char *model;
> + u16 model_subs;
> +};
> +#define MODEL_SUB_OQO_O2_SMB0 3
> +
> +static struct oqo_model oqo_models[] = {
> + {
> + .model = "Model 2",
> + .model_subs = MODEL_SUB_OQO_O2_SMB0,
> + },
> + {}
> +};
> +
> +static struct oqo_model *model;
With "like in acer-wmi" you mean that this whole model stuff currently
serves no purpose? If we don't access the model variable yet, I think it
shouldn't be there.
> +static int dmi_matched(const struct dmi_system_id *dmi)
__init
> +{
> + model = dmi->driver_data;
> + /*
> + * Detect which ACPI-WMI interface we're using.
> + */
> + if (wmi_has_guid(OQO_O2_AMW0_GUID))
> + interface = &AMW0_interface;
> +
> + return 0;
> +}
> +
> +static struct dmi_system_id oqo_dmis[] = {
__initdata
And i think const is also possible.
> +static acpi_status oqo_read_kine(int *good, s16 *x, s16 *y, s16 *z,
> + u16 *lumin)
> +{
> + u8 hiregs[4] = { OQO_O2_SMB0_ACCEL_XHI,
> + OQO_O2_SMB0_ACCEL_YHI,
> + OQO_O2_SMB0_ACCEL_ZHI,
> + OQO_O2_SMB0_LUMIN_HI
> + };
> + u8 loregs[4] = { OQO_O2_SMB0_ACCEL_XLO,
> + OQO_O2_SMB0_ACCEL_YLO,
> + OQO_O2_SMB0_ACCEL_ZLO,
> + OQO_O2_SMB0_LUMIN_LO
> + };
> +
> + short ax[4] = { ABS_X, ABS_Y, ABS_Z, ABS_MISC };
Uhm, I think these three variables could be static and const.
> + *x = (u16) res[0];
> + *y = (u16) res[1];
> + *z = (u16) res[2];
> + *lumin = (u16) res[3];
> + return status;
This return could be dropped, just let it fall-through.
> +leave:
> + return status;
> +}
> +static int __devinit oqo_kine_init(void)
> +{
> + int err;
> +
> + oqo_kine = input_allocate_device();
> + if (!oqo_kine)
> + return -ENOMEM;
> +
> + oqo_kine->name = "OQO embedded accelerometer";
> + oqo_kine->phys = "platform:oqo-wmi:kine";
> + oqo_kine->id.bustype = 0;
> + oqo_kine->id.vendor = 0;
> + oqo_kine->id.product = 2;
> + oqo_kine->id.version = 0;
> + oqo_kine->evbit[0] = BIT_MASK(EV_ABS);
> + set_bit(ABS_X, oqo_kine->absbit);
> + set_bit(ABS_Y, oqo_kine->absbit);
> + set_bit(ABS_Z, oqo_kine->absbit);
> + set_bit(ABS_MISC, oqo_kine->absbit);
> + oqo_kine->absmin[ABS_X] =
> + oqo_kine->absmin[ABS_Y] =
> + oqo_kine->absmin[ABS_Z] = oqo_kine->absmin[ABS_MISC] = -32768;
> + oqo_kine->absmax[ABS_X] =
> + oqo_kine->absmax[ABS_Y] =
> + oqo_kine->absmax[ABS_Z] = oqo_kine->absmax[ABS_MISC] = 32767;
> +
> + memcpy(oqo_kine->dev.bus_id, "kine", 4);
> +
> + oqo_kine_polled = input_allocate_polled_device();
> + if (!oqo_kine_polled) {
> + err = -ENOMEM;
> + goto bail0;
> + }
> +
> + oqo_kine_polled->poll = oqo_kine_poll;
> + oqo_kine_polled->poll_interval = 250;
> + oqo_kine_polled->input = oqo_kine;
input_allocate_polled_device() allocates ->input for us, you must remove
the input_allocate_device() above and shift the code around.
> +static void __devexit oqo_kine_fini(void)
> +{
> + smwrite_u8(OQO_O2_SMB0_ACCEL_POLL_ITVL, orig.kine_itvl);
> + input_unregister_polled_device(oqo_kine_polled);
> + input_free_polled_device(oqo_kine_polled);
Uhm, I wonder if this is the right order. First unregister and then do
the smwrite_u8() to prevent the device from reverting our write?
> +static int __devinit oqo_platform_probe(struct platform_device *device)
> +{
> + int err;
> + int i;
> + char *troubleok = "trouble, but continuing.\n";
This variable seems needless, just append the message to the printk()
calls.
> +
> + memset(oqo_sn, 0, OQO_O2_SMB0_SERIAL_LEN + 1);
> + for (i = 0; i < OQO_O2_SMB0_SERIAL_LEN; i++) {
> + err = oqo_smbus_getb(OQO_O2_SMB0_SERIAL_START + i, oqo_sn + i);
> + if (err) {
> + printk(OQO_ERR "Serial number check failed.\n");
> + return err;
> + }
> + }
> + printk(OQO_INFO "Found OQO with serial number %s.\n", oqo_sn);
> +
> + err = oqo_backlight_init(&device->dev);
> + if (err)
> + printk(OQO_ERR "Backlight init %s", troubleok);
> +
> + err = oqo_rfkill_init(&device->dev);
> + if (err)
> + printk(OQO_ERR "RFKill init %s", troubleok);
> +
> + /* LID does not work at all yet, and this may be taken
> + care of by ACPI.
> + */
> + orig.lid_wakes = smread_u8(OQO_O2_SMB0_LID_WAKES_ADDR);
> + orig.lid_wakes &= OQO_O2_SMB0_LID_WAKES_MASK;
> + orig.lid_wakes = curr.lid_wakes = !!orig.lid_wakes;
> + if (orig.lid_wakes < 0) {
> + printk(OQO_ERR "Wake on LID event %s", troubleok);
> + } else {
> + printk(OQO_INFO "Wake on LID is %s.\n",
> + (orig.lid_wakes ? "on" : "off"));
> + }
> +
> + err = oqo_kine_init();
> + return err;
Uhm, if oqo_kine_init() fails, does our caller take care of
unregistering the backlight and rfkill devices?
> +}
> +
> +static int oqo_platform_remove(struct platform_device *device)
> +{
> + oqo_backlight_fini();
> + oqo_rfkill_fini();
> + oqo_kine_fini();
These three functions are marked __devexit, I suspect this will cause
section mismatches.
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +
> +static int oqo_platform_suspend(struct platform_device *dev, pm_message_t state)
> +{
> + if (!interface)
> + return -ENOMEM;
Interface should be always set once the module has been loaded
successfully.
> +
> + /* This sticks during boot so do not turn it entirely off */
> + if (oqo_backlight_device) {
> + curr.bl_bright = read_brightness(oqo_backlight_device);
> + smwrite_s16(OQO_O2_SMB0_BL_HI, OQO_O2_SMB0_BL_LO, 256);
> + }
> + return 0;
> +}
> +
> +static int oqo_platform_resume(struct platform_device *device)
> +{
> + if (!interface)
> + return -ENOMEM;
Likewise.
> +
> + if (oqo_backlight_device) {
> + smwrite_s16(OQO_O2_SMB0_BL_HI,
> + OQO_O2_SMB0_BL_LO, curr.bl_bright);
> + }
> +
> + return 0;
> +}
> + err = platform_device_add(oqo_platform_device);
> + if (err) {
> + printk(OQO_ERR "platform_device_add gave %d.\n", err);
> + platform_device_put(oqo_platform_device);
> + goto bail1;
Why not add a bail2 and move platform_device_put() there?
> + }
> +
> + return 0;
> +
> +bail1:
> + platform_driver_unregister(&oqo_platform_driver);
> +bail0:
> + return err;
> +}
> +
> +static void __exit oqo_wmi_fini(void)
> +{
> + platform_device_del(oqo_platform_device);
> + platform_driver_unregister(&oqo_platform_driver);
> +
> + return;
> +}
Needless return at end of function.
Sven
--
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
