On Wed, 31 Jul 2013, Kim, Milo wrote:

Thanks for the review, please see my comments.

quoted

<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?

Not really. IIRC the documentation said that LED0 (which I believe
you're calling OUT0 here) is located at pin one. So your enum above is
now incorrect isn't it? As *_OUT0 will be 0 and not 1? Or am I missing
something?

quoted

quoted

+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().

No I don't think so. Instead, you should use -EPROBE_DEFER in lieu of
messing around with initialisation orders.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog