linux-kernel - RE: [PATCH v2 1/4] mfd: add LP3943 MFD driver

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Wed, 31 Jul 2013 23:29:48 +0000
From:	"Kim, Milo" <Milo.Kim@...com>
To:	Lee Jones <lee.jones@...aro.org>
CC:	"Samuel Ortiz (sameo@...ux.intel.com)" <sameo@...ux.intel.com>,
	"broonie@...nel.org" <broonie@...nel.org>,
	"linus.walleij@...aro.org" <linus.walleij@...aro.org>,
	"thierry.reding@...il.com" <thierry.reding@...il.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>
Subject: RE: [PATCH v2 1/4] mfd: add LP3943 MFD driver

Thanks for the review, please see my comments.

> <snip> * looks good up to me up to here *
> 
> Although, I think the 0 = 1, 1 = 2 ... stuff is really confusing. Is
> there nothing we can do about that?

OK, enum value of lp3943_pwm_output can be changed as below
because LP3943_PWM_INVALID is not used anymore.

enum lp3943_pwm_output {
	LP3943_PWM_OUT0,
	LP3943_PWM_OUT1,
	...
	LP3943_PWM_OUT15,
};

Then, output index will match each enum integer value.
Does it make sense?

> > +static const struct i2c_device_id lp3943_ids[] = {
> > +	{ "lp3943", 0 },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, lp3943_ids);
> 
> Are we expecting this driver to support more devices?

At this moment, this is the only one device supported.

> > +static int __init lp3943_init(void)
> > +{
> > +	return i2c_add_driver(&lp3943_driver);
> > +}
> > +subsys_initcall(lp3943_init);
> > +
> > +static void __exit lp3943_exit(void)
> > +{
> > +	i2c_del_driver(&lp3943_driver);
> > +}
> > +module_exit(lp3943_exit);
> 
> I think you want to replace:
>   lp3943_init()
>   lp3943_exit
> 
> With:
>   module_i2c_driver()

This is related with initcall sequence.
Some problem may happen if any GPIO or PWM consumer tries to request before
LP3943 MFDs are added.
For example, a GPIO is requested in _probe() of some device.
Let's assume the GPIO number is in range of what LP3943 GPIO driver provided.
Then, gpio_request() will be failed because the GPIO is invalid at this moment.
If the device request again later, it will be OK, but we can't expect this 
situation for every driver.
Some drivers request a GPIO only once in _probe(), other devices may request
a GPIO in some cases.
So, I think lp3943_init() should be defined as subsys_initcall() instead of 
module_init().

Best regards,
Milo -