lkml.org 
[lkml]   [2011]   [Jan]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] platform-driver-x86: ACPI EC Extra driver for Oaktrail
    On 2011-01-06, 08:29 +0100, Corentin Chary wrote:
    > Hi,

    Thanks for the review and comments.

    > > @@ -0,0 +1,349 @@
    > > +/*-*-linux-c-*-*/
    >
    > I don't know what's our general policy about that, but I don't think
    > each text editor should be allowed to add its own header on each
    > files. Most of the time you can configure your editor to set the
    > right indent style based on the path of the file or something like that.

    Yes, I agree with you, will remove that.

    > > + * gps - GPS subsystem enabled: contains either 0 or 1. (rw)
    > > + * wifi - WiFi subsystem enabled: contains either 0 or 1. (rw)
    > > + * wwan - WWAN (3G) subsystem enabled: contains either 0 or 1. (rw)
    >
    > Is there a reason do add these files in /sys/devices/platform while the
    > functionality is already provided by rfkill ?

    Provide a alternative way to enable/disable these devices, and also
    for debugging. Does that make any sense?

    > > + * camera - Camera subsystem enabled: contains either 0 or 1. (rw)
    > > + * bluetooth - Bluetooth subsystem enabled: contains either 0 or 1. (rw)
    > > + * touchscreen - Touchscreen subsystem enabled: contains either 0 or 1. (ro)
    >
    > This should be in Documentation/ABI/testing/

    Should I prepare the document now and submit also?

    > > +
    > > +static struct platform_device *oaktrail_device;
    > > +static struct rfkill *bt_rfkill;
    > > +static struct rfkill *gps_rfkill;
    > > +static struct rfkill *wifi_rfkill;
    > > +static struct rfkill *wwan_rfkill;
    >
    > Here you could create two (four ?) helpers that contains the logic,
    > and craft dummy functions which only call the helpers with the right
    > parameters in your macros.
    >
    > These helpers could also be used by later functions.
    >

    I will try to define some micros..

    > > +static int setup_rfkill(void)
    >
    > oaktrail_rfkill_init() ?

    Sure.

    > > +       rfkill_destroy(wwan_rfkill);
    > > +err_allocate_wwan:
    > > +       rfkill_unregister(gps_rfkill);
    > > +err_register_gps:
    > > +       rfkill_destroy(gps_rfkill);
    > > +err_allocate_gps:
    > > +       rfkill_unregister(bt_rfkill);
    > > +err_register_bt:
    > > +       rfkill_destroy(bt_rfkill);
    > > +err_allocate_bt:
    > > +       rfkill_unregister(wifi_rfkill);
    > > +err_register_wifi:
    > > +       rfkill_destroy(wifi_rfkill);
    > > +
    > > +       return ret;
    > > +}
    >
    > Here I'd write an helper function to call rfkill_alloc,
    > rfkill_register and handle
    > rfkill_register failure. And then, if any of the helper calls fail, just call
    > oaktrail_rfkill_exit (which which rfkill if the rfkill pointer is NULL or not).
    >
    > oaktrail_rfkill_exit could also be used in the module exit function.

    Yes, will try to do that.

    > > +static int __devinit oaktrail_probe(struct platform_device *pdev)
    > > +{
    > > +       int err;
    > > +
    > > +       err = sysfs_create_group(&pdev->dev.kobj, &oaktrail_attribute_group);
    > > +       return err;
    > > +}
    >
    > return sysfs_create_group(&pdev->dev.kobj, &oaktrail_attribute_group);
    >
    > we don't really need err right now, do we ?

    Will change this.

    > > +MODULE_AUTHOR("Yin Kangkai (kangkai.yin@intel.com)");
    > > +MODULE_DESCRIPTION("Intel Oaktrail Platform ACPI Extras");
    > > +MODULE_VERSION(DRIVER_VERSION);
    > > +MODULE_LICENSE("GPL");
    >
    > Maybe you could add some MODULE_ALIAS("dmi:xxxxx") lines
    > to enable module autoloading ?

    Will try to.

    Thanks for the review.

    Regards,
    Kangkai
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2011-01-06 11:13    [W:0.027 / U:1.096 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site