On Thu, Jan 6, 2011 at 11:11 AM, Yin Kangkai <kangkai.yin@xxxxxxxxxxxxxxx> wrote: > 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? Maybe using debugfs then ? > >> > + * 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? Yep, there is already one or two platform drivers documented here, you can use them as a template. >> > + >> > +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@xxxxxxxxx)"); >> > +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 > -- Corentin Chary http://xf.iksaif.net -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html