linux-kernel - Re: [PATCH] platform-driver-x86: ACPI EC Extra driver for Oaktrail

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AANLkTin-W0-hj_jguZvbLNML_4VpV64qBN=bwEaf5RW3@mail.gmail.com>
Date:	Thu, 6 Jan 2011 11:21:49 +0100
From:	Corentin Chary <corentin.chary@...il.com>
To:	Yin Kangkai <kangkai.yin@...ux.intel.com>
Cc:	platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
	"Wang, Yong Y" <yong.y.wang@...el.com>,
	"Liu, Bing Wei" <bing.wei.liu@...el.com>
Subject: Re: [PATCH] platform-driver-x86: ACPI EC Extra driver for Oaktrail

On Thu, Jan 6, 2011 at 11:11 AM, Yin Kangkai
<kangkai.yin@...ux.intel.com> 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@...el.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
>



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