Re: [PATCH RESEND 06/16] mfd: add TI LMU driver

2015-11-25 Thread Lee Jones 
On Wed, 25 Nov 2015, Kim, Milo wrote:

> On 11/24/2015 5:18 PM, Lee Jones wrote:
> >On Tue, 24 Nov 2015, Kim, Milo wrote:
> >
> >>On 11/24/2015 11:35 AM, Kim, Milo wrote:
> >>>Hi Lee,
> >>>
> >>>Thanks for all your comments. Please see my comments below.
> >>>
> >>>On 11/23/2015 7:30 PM, Lee Jones wrote:
> >+int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
> >>+{
> >>+   int ret;
> >>+   unsigned int val;
> >>+
> >>+   ret = regmap_read(lmu->regmap, reg, );
> >>+   if (ret < 0)
> >>+   return ret;
> >>+
> >>+   *read = (u8)val;
> >>+   return 0;
> >>+}
> >>+EXPORT_SYMBOL_GPL(ti_lmu_read_byte);
> It doesn't get much more simple than this.
> 
> What's the purpose of abstracting it?
> 
> >>+int ti_lmu_write_byte(struct ti_lmu *lmu, u8 reg, u8 data)
> >>+{
> >>+   return regmap_write(lmu->regmap, reg, data);
> >>+}
> >>+EXPORT_SYMBOL_GPL(ti_lmu_write_byte);
> >>+
> >>+int ti_lmu_update_bits(struct ti_lmu *lmu, u8 reg, u8 mask, u8 data)
> >>+{
> >>+   return regmap_update_bits(lmu->regmap, reg, mask, data);
> >>+}
> >>+EXPORT_SYMBOL_GPL(ti_lmu_update_bits);
> Okay, I lied, it does get more simple.
> 
> Seems like abstraction for the sake of abstraction here.
> 
> Feel free to try and convince me otherwise.
> 
> >>>
> >>>The main reason was that LMU MFD core provides consistent register
> >>>access among LMU drivers like ti-lmu-backlight, leds-lm3633,
> >>>lm363x-regulator and ti-lmu-fault-monitor('ti-lmu-hwmon' will be changed
> >>>to this in the 2nd patch).
> >>>
> >>>However, LMU register helpers are exactly same as regmap interface
> >>>except using ti_lmu data structure. So let me replace them with regmap
> >>>functions. Thanks for pointing this out.
> >>
> >>I just realized LMU MFD core helpers also provide module
> >>dependencies by using EXPORT_SYMBOL_GPL().
> >>
> >># modprobe -D ti-lmu-backlight
> >>insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
> >>insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
> >>insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
> >>insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
> >># modprobe -D ti-lmu-fault-monitor
> >>insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
> >>insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
> >>insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
> >>insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
> >># modprobe -D lm363x-regulator
> >>insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
> >>insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
> >>insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
> >>insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
> >># modprobe -D leds-lm3633
> >>insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
> >>insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
> >>insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
> >>insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko
> >>
> >>ti-lmu.ko should be loaded first because it has hardware enable pin
> >>control code. Four other drivers have dependency on this module.
> >>Without EXPORT_SYMBOL_GPL(), this dependency will be gone like
> >>below.
> >>
> >># modprobe -D ti-lmu-backlight
> >>insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
> >># modprobe -D ti-lmu-fault-monitor
> >>insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
> >># modprobe -D lm363x-regulator
> >>insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
> >># modprobe -D leds-lm3633
> >>insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko
> >>
> >>If LMU MFD core module is not loaded and other modules call regmap
> >>helpers, then loaded drivers will not work because hardware is not
> >>enabled yet.
> >>
> >>So I'd like to keep LMU MFD helpers for module dependencies.
> >>Additionally, I'll modify 'ti_lmu_read_byte()' as follows.
> >>
> >>int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
> >>{
> >>return regmap_read(lmu->regmap, reg, (unsigned int *)read);
> >>}
> >>EXPORT_SYMBOL_GPL(ti_lmu_read_byte);
> >>
> >>Please let me know if you have better idea.
> >
> >You're keeping the helpers for the wrong reasons.  This has now become
> >a hack.  If you have dependencies between modules, either use the init
> >levels or defer probe in the usual way.
> >
> 
> Yes, I was wrong. In case LMU MFD core is not loaded, other LMU
> modules like ti-lmu-backlight isn't probed because no platform
> device (mfd device) exists. So the situation which I've mentioned
> never happens.
> And if GPIO controller is not ready and HW enable GPIO request gets
> failed on LMU MFD initialization, this will be processed later
> because it returns as error code '-EPROBE_DEFER' from GPIO
> subsystem. In other words, there is no dependency issue between LMU
> modules. Those are just

Re: [PATCH RESEND 06/16] mfd: add TI LMU driver

2015-11-25 Thread Kim, Milo 

On 11/24/2015 5:18 PM, Lee Jones wrote:

On Tue, 24 Nov 2015, Kim, Milo wrote:


On 11/24/2015 11:35 AM, Kim, Milo wrote:

Hi Lee,

Thanks for all your comments. Please see my comments below.

On 11/23/2015 7:30 PM, Lee Jones wrote:

+int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)

+{
+   int ret;
+   unsigned int val;
+
+   ret = regmap_read(lmu->regmap, reg, );
+   if (ret < 0)
+   return ret;
+
+   *read = (u8)val;
+   return 0;
+}
+EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

It doesn't get much more simple than this.

What's the purpose of abstracting it?


+int ti_lmu_write_byte(struct ti_lmu *lmu, u8 reg, u8 data)
+{
+   return regmap_write(lmu->regmap, reg, data);
+}
+EXPORT_SYMBOL_GPL(ti_lmu_write_byte);
+
+int ti_lmu_update_bits(struct ti_lmu *lmu, u8 reg, u8 mask, u8 data)
+{
+   return regmap_update_bits(lmu->regmap, reg, mask, data);
+}
+EXPORT_SYMBOL_GPL(ti_lmu_update_bits);

Okay, I lied, it does get more simple.

Seems like abstraction for the sake of abstraction here.

Feel free to try and convince me otherwise.



The main reason was that LMU MFD core provides consistent register
access among LMU drivers like ti-lmu-backlight, leds-lm3633,
lm363x-regulator and ti-lmu-fault-monitor('ti-lmu-hwmon' will be changed
to this in the 2nd patch).

However, LMU register helpers are exactly same as regmap interface
except using ti_lmu data structure. So let me replace them with regmap
functions. Thanks for pointing this out.


I just realized LMU MFD core helpers also provide module
dependencies by using EXPORT_SYMBOL_GPL().

# modprobe -D ti-lmu-backlight
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
# modprobe -D ti-lmu-fault-monitor
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
# modprobe -D lm363x-regulator
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
# modprobe -D leds-lm3633
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko

ti-lmu.ko should be loaded first because it has hardware enable pin
control code. Four other drivers have dependency on this module.
Without EXPORT_SYMBOL_GPL(), this dependency will be gone like
below.

# modprobe -D ti-lmu-backlight
insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
# modprobe -D ti-lmu-fault-monitor
insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
# modprobe -D lm363x-regulator
insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
# modprobe -D leds-lm3633
insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko

If LMU MFD core module is not loaded and other modules call regmap
helpers, then loaded drivers will not work because hardware is not
enabled yet.

So I'd like to keep LMU MFD helpers for module dependencies.
Additionally, I'll modify 'ti_lmu_read_byte()' as follows.

int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
{
return regmap_read(lmu->regmap, reg, (unsigned int *)read);
}
EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

Please let me know if you have better idea.


You're keeping the helpers for the wrong reasons.  This has now become
a hack.  If you have dependencies between modules, either use the init
levels or defer probe in the usual way.



Yes, I was wrong. In case LMU MFD core is not loaded, other LMU modules 
like ti-lmu-backlight isn't probed because no platform device (mfd 
device) exists. So the situation which I've mentioned never happens.
And if GPIO controller is not ready and HW enable GPIO request gets 
failed on LMU MFD initialization, this will be processed later because 
it returns as error code '-EPROBE_DEFER' from GPIO subsystem. In other 
words, there is no dependency issue between LMU modules. Those are just 
platform device and driver.


OK, I'll remove LMU register helpers in the 2nd patch. Then, each LMU 
driver will call regmap helpers directly.

Thanks for your comments.

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

Re: [PATCH RESEND 06/16] mfd: add TI LMU driver

2015-11-25 Thread Lee Jones 
On Wed, 25 Nov 2015, Kim, Milo wrote:

> On 11/24/2015 5:18 PM, Lee Jones wrote:
> >On Tue, 24 Nov 2015, Kim, Milo wrote:
> >
> >>On 11/24/2015 11:35 AM, Kim, Milo wrote:
> >>>Hi Lee,
> >>>
> >>>Thanks for all your comments. Please see my comments below.
> >>>
> >>>On 11/23/2015 7:30 PM, Lee Jones wrote:
> >+int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
> >>+{
> >>+   int ret;
> >>+   unsigned int val;
> >>+
> >>+   ret = regmap_read(lmu->regmap, reg, );
> >>+   if (ret < 0)
> >>+   return ret;
> >>+
> >>+   *read = (u8)val;
> >>+   return 0;
> >>+}
> >>+EXPORT_SYMBOL_GPL(ti_lmu_read_byte);
> It doesn't get much more simple than this.
> 
> What's the purpose of abstracting it?
> 
> >>+int ti_lmu_write_byte(struct ti_lmu *lmu, u8 reg, u8 data)
> >>+{
> >>+   return regmap_write(lmu->regmap, reg, data);
> >>+}
> >>+EXPORT_SYMBOL_GPL(ti_lmu_write_byte);
> >>+
> >>+int ti_lmu_update_bits(struct ti_lmu *lmu, u8 reg, u8 mask, u8 data)
> >>+{
> >>+   return regmap_update_bits(lmu->regmap, reg, mask, data);
> >>+}
> >>+EXPORT_SYMBOL_GPL(ti_lmu_update_bits);
> Okay, I lied, it does get more simple.
> 
> Seems like abstraction for the sake of abstraction here.
> 
> Feel free to try and convince me otherwise.
> 
> >>>
> >>>The main reason was that LMU MFD core provides consistent register
> >>>access among LMU drivers like ti-lmu-backlight, leds-lm3633,
> >>>lm363x-regulator and ti-lmu-fault-monitor('ti-lmu-hwmon' will be changed
> >>>to this in the 2nd patch).
> >>>
> >>>However, LMU register helpers are exactly same as regmap interface
> >>>except using ti_lmu data structure. So let me replace them with regmap
> >>>functions. Thanks for pointing this out.
> >>
> >>I just realized LMU MFD core helpers also provide module
> >>dependencies by using EXPORT_SYMBOL_GPL().
> >>
> >># modprobe -D ti-lmu-backlight
> >>insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
> >>insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
> >>insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
> >>insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
> >># modprobe -D ti-lmu-fault-monitor
> >>insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
> >>insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
> >>insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
> >>insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
> >># modprobe -D lm363x-regulator
> >>insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
> >>insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
> >>insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
> >>insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
> >># modprobe -D leds-lm3633
> >>insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
> >>insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
> >>insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
> >>insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko
> >>
> >>ti-lmu.ko should be loaded first because it has hardware enable pin
> >>control code. Four other drivers have dependency on this module.
> >>Without EXPORT_SYMBOL_GPL(), this dependency will be gone like
> >>below.
> >>
> >># modprobe -D ti-lmu-backlight
> >>insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
> >># modprobe -D ti-lmu-fault-monitor
> >>insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
> >># modprobe -D lm363x-regulator
> >>insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
> >># modprobe -D leds-lm3633
> >>insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko
> >>
> >>If LMU MFD core module is not loaded and other modules call regmap
> >>helpers, then loaded drivers will not work because hardware is not
> >>enabled yet.
> >>
> >>So I'd like to keep LMU MFD helpers for module dependencies.
> >>Additionally, I'll modify 'ti_lmu_read_byte()' as follows.
> >>
> >>int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
> >>{
> >>return regmap_read(lmu->regmap, reg, (unsigned int *)read);
> >>}
> >>EXPORT_SYMBOL_GPL(ti_lmu_read_byte);
> >>
> >>Please let me know if you have better idea.
> >
> >You're keeping the helpers for the wrong reasons.  This has now become
> >a hack.  If you have dependencies between modules, either use the init
> >levels or defer probe in the usual way.
> >
> 
> Yes, I was wrong. In case LMU MFD core is not loaded, other LMU
> modules like ti-lmu-backlight isn't probed because no platform
> device (mfd device) exists. So the situation which I've mentioned
> never happens.
> And if GPIO controller is not ready and HW enable GPIO request gets
> failed on LMU MFD initialization, this will be processed later
> because it returns as error code '-EPROBE_DEFER' from GPIO
> subsystem. In other words, there is no dependency issue between LMU
> modules. Those are just

Re: [PATCH RESEND 06/16] mfd: add TI LMU driver

2015-11-25 Thread Kim, Milo 

On 11/24/2015 5:18 PM, Lee Jones wrote:

On Tue, 24 Nov 2015, Kim, Milo wrote:


On 11/24/2015 11:35 AM, Kim, Milo wrote:

Hi Lee,

Thanks for all your comments. Please see my comments below.

On 11/23/2015 7:30 PM, Lee Jones wrote:

+int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)

+{
+   int ret;
+   unsigned int val;
+
+   ret = regmap_read(lmu->regmap, reg, );
+   if (ret < 0)
+   return ret;
+
+   *read = (u8)val;
+   return 0;
+}
+EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

It doesn't get much more simple than this.

What's the purpose of abstracting it?


+int ti_lmu_write_byte(struct ti_lmu *lmu, u8 reg, u8 data)
+{
+   return regmap_write(lmu->regmap, reg, data);
+}
+EXPORT_SYMBOL_GPL(ti_lmu_write_byte);
+
+int ti_lmu_update_bits(struct ti_lmu *lmu, u8 reg, u8 mask, u8 data)
+{
+   return regmap_update_bits(lmu->regmap, reg, mask, data);
+}
+EXPORT_SYMBOL_GPL(ti_lmu_update_bits);

Okay, I lied, it does get more simple.

Seems like abstraction for the sake of abstraction here.

Feel free to try and convince me otherwise.



The main reason was that LMU MFD core provides consistent register
access among LMU drivers like ti-lmu-backlight, leds-lm3633,
lm363x-regulator and ti-lmu-fault-monitor('ti-lmu-hwmon' will be changed
to this in the 2nd patch).

However, LMU register helpers are exactly same as regmap interface
except using ti_lmu data structure. So let me replace them with regmap
functions. Thanks for pointing this out.


I just realized LMU MFD core helpers also provide module
dependencies by using EXPORT_SYMBOL_GPL().

# modprobe -D ti-lmu-backlight
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
# modprobe -D ti-lmu-fault-monitor
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
# modprobe -D lm363x-regulator
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
# modprobe -D leds-lm3633
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko

ti-lmu.ko should be loaded first because it has hardware enable pin
control code. Four other drivers have dependency on this module.
Without EXPORT_SYMBOL_GPL(), this dependency will be gone like
below.

# modprobe -D ti-lmu-backlight
insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
# modprobe -D ti-lmu-fault-monitor
insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
# modprobe -D lm363x-regulator
insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
# modprobe -D leds-lm3633
insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko

If LMU MFD core module is not loaded and other modules call regmap
helpers, then loaded drivers will not work because hardware is not
enabled yet.

So I'd like to keep LMU MFD helpers for module dependencies.
Additionally, I'll modify 'ti_lmu_read_byte()' as follows.

int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
{
return regmap_read(lmu->regmap, reg, (unsigned int *)read);
}
EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

Please let me know if you have better idea.


You're keeping the helpers for the wrong reasons.  This has now become
a hack.  If you have dependencies between modules, either use the init
levels or defer probe in the usual way.



Yes, I was wrong. In case LMU MFD core is not loaded, other LMU modules 
like ti-lmu-backlight isn't probed because no platform device (mfd 
device) exists. So the situation which I've mentioned never happens.
And if GPIO controller is not ready and HW enable GPIO request gets 
failed on LMU MFD initialization, this will be processed later because 
it returns as error code '-EPROBE_DEFER' from GPIO subsystem. In other 
words, there is no dependency issue between LMU modules. Those are just 
platform device and driver.


OK, I'll remove LMU register helpers in the 2nd patch. Then, each LMU 
driver will call regmap helpers directly.

Thanks for your comments.

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

Re: [PATCH RESEND 06/16] mfd: add TI LMU driver

2015-11-24 Thread Lee Jones 
On Tue, 24 Nov 2015, Kim, Milo wrote:

> On 11/24/2015 11:35 AM, Kim, Milo wrote:
> >Hi Lee,
> >
> >Thanks for all your comments. Please see my comments below.
> >
> >On 11/23/2015 7:30 PM, Lee Jones wrote:
> >>>+int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
> +{
> + int ret;
> + unsigned int val;
> +
> + ret = regmap_read(lmu->regmap, reg, );
> + if (ret < 0)
> + return ret;
> +
> + *read = (u8)val;
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ti_lmu_read_byte);
> >>It doesn't get much more simple than this.
> >>
> >>What's the purpose of abstracting it?
> >>
> +int ti_lmu_write_byte(struct ti_lmu *lmu, u8 reg, u8 data)
> +{
> + return regmap_write(lmu->regmap, reg, data);
> +}
> +EXPORT_SYMBOL_GPL(ti_lmu_write_byte);
> +
> +int ti_lmu_update_bits(struct ti_lmu *lmu, u8 reg, u8 mask, u8 data)
> +{
> + return regmap_update_bits(lmu->regmap, reg, mask, data);
> +}
> +EXPORT_SYMBOL_GPL(ti_lmu_update_bits);
> >>Okay, I lied, it does get more simple.
> >>
> >>Seems like abstraction for the sake of abstraction here.
> >>
> >>Feel free to try and convince me otherwise.
> >>
> >
> >The main reason was that LMU MFD core provides consistent register
> >access among LMU drivers like ti-lmu-backlight, leds-lm3633,
> >lm363x-regulator and ti-lmu-fault-monitor('ti-lmu-hwmon' will be changed
> >to this in the 2nd patch).
> >
> >However, LMU register helpers are exactly same as regmap interface
> >except using ti_lmu data structure. So let me replace them with regmap
> >functions. Thanks for pointing this out.
> 
> I just realized LMU MFD core helpers also provide module
> dependencies by using EXPORT_SYMBOL_GPL().
> 
> # modprobe -D ti-lmu-backlight
> insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
> insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
> insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
> insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
> # modprobe -D ti-lmu-fault-monitor
> insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
> insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
> insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
> insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
> # modprobe -D lm363x-regulator
> insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
> insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
> insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
> insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
> # modprobe -D leds-lm3633
> insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
> insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
> insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
> insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko
> 
> ti-lmu.ko should be loaded first because it has hardware enable pin
> control code. Four other drivers have dependency on this module.
> Without EXPORT_SYMBOL_GPL(), this dependency will be gone like
> below.
> 
> # modprobe -D ti-lmu-backlight
> insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
> # modprobe -D ti-lmu-fault-monitor
> insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
> # modprobe -D lm363x-regulator
> insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
> # modprobe -D leds-lm3633
> insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko
> 
> If LMU MFD core module is not loaded and other modules call regmap
> helpers, then loaded drivers will not work because hardware is not
> enabled yet.
> 
> So I'd like to keep LMU MFD helpers for module dependencies.
> Additionally, I'll modify 'ti_lmu_read_byte()' as follows.
> 
> int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
> {
>   return regmap_read(lmu->regmap, reg, (unsigned int *)read);
> }
> EXPORT_SYMBOL_GPL(ti_lmu_read_byte);
> 
> Please let me know if you have better idea.

You're keeping the helpers for the wrong reasons.  This has now become
a hack.  If you have dependencies between modules, either use the init
levels or defer probe in the usual way.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 06/16] mfd: add TI LMU driver

2015-11-24 Thread Lee Jones 
On Tue, 24 Nov 2015, Kim, Milo wrote:

> On 11/24/2015 11:35 AM, Kim, Milo wrote:
> >Hi Lee,
> >
> >Thanks for all your comments. Please see my comments below.
> >
> >On 11/23/2015 7:30 PM, Lee Jones wrote:
> >>>+int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
> +{
> + int ret;
> + unsigned int val;
> +
> + ret = regmap_read(lmu->regmap, reg, );
> + if (ret < 0)
> + return ret;
> +
> + *read = (u8)val;
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ti_lmu_read_byte);
> >>It doesn't get much more simple than this.
> >>
> >>What's the purpose of abstracting it?
> >>
> +int ti_lmu_write_byte(struct ti_lmu *lmu, u8 reg, u8 data)
> +{
> + return regmap_write(lmu->regmap, reg, data);
> +}
> +EXPORT_SYMBOL_GPL(ti_lmu_write_byte);
> +
> +int ti_lmu_update_bits(struct ti_lmu *lmu, u8 reg, u8 mask, u8 data)
> +{
> + return regmap_update_bits(lmu->regmap, reg, mask, data);
> +}
> +EXPORT_SYMBOL_GPL(ti_lmu_update_bits);
> >>Okay, I lied, it does get more simple.
> >>
> >>Seems like abstraction for the sake of abstraction here.
> >>
> >>Feel free to try and convince me otherwise.
> >>
> >
> >The main reason was that LMU MFD core provides consistent register
> >access among LMU drivers like ti-lmu-backlight, leds-lm3633,
> >lm363x-regulator and ti-lmu-fault-monitor('ti-lmu-hwmon' will be changed
> >to this in the 2nd patch).
> >
> >However, LMU register helpers are exactly same as regmap interface
> >except using ti_lmu data structure. So let me replace them with regmap
> >functions. Thanks for pointing this out.
> 
> I just realized LMU MFD core helpers also provide module
> dependencies by using EXPORT_SYMBOL_GPL().
> 
> # modprobe -D ti-lmu-backlight
> insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
> insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
> insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
> insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
> # modprobe -D ti-lmu-fault-monitor
> insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
> insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
> insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
> insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
> # modprobe -D lm363x-regulator
> insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
> insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
> insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
> insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
> # modprobe -D leds-lm3633
> insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
> insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
> insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
> insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko
> 
> ti-lmu.ko should be loaded first because it has hardware enable pin
> control code. Four other drivers have dependency on this module.
> Without EXPORT_SYMBOL_GPL(), this dependency will be gone like
> below.
> 
> # modprobe -D ti-lmu-backlight
> insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
> # modprobe -D ti-lmu-fault-monitor
> insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
> # modprobe -D lm363x-regulator
> insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
> # modprobe -D leds-lm3633
> insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko
> 
> If LMU MFD core module is not loaded and other modules call regmap
> helpers, then loaded drivers will not work because hardware is not
> enabled yet.
> 
> So I'd like to keep LMU MFD helpers for module dependencies.
> Additionally, I'll modify 'ti_lmu_read_byte()' as follows.
> 
> int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
> {
>   return regmap_read(lmu->regmap, reg, (unsigned int *)read);
> }
> EXPORT_SYMBOL_GPL(ti_lmu_read_byte);
> 
> Please let me know if you have better idea.

You're keeping the helpers for the wrong reasons.  This has now become
a hack.  If you have dependencies between modules, either use the init
levels or defer probe in the usual way.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND 06/16] mfd: add TI LMU driver

2015-11-23 Thread Kim, Milo 

On 11/24/2015 11:35 AM, Kim, Milo wrote:

Hi Lee,

Thanks for all your comments. Please see my comments below.

On 11/23/2015 7:30 PM, Lee Jones wrote:

+int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)

+{
+   int ret;
+   unsigned int val;
+
+   ret = regmap_read(lmu->regmap, reg, );
+   if (ret < 0)
+   return ret;
+
+   *read = (u8)val;
+   return 0;
+}
+EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

It doesn't get much more simple than this.

What's the purpose of abstracting it?


+int ti_lmu_write_byte(struct ti_lmu *lmu, u8 reg, u8 data)
+{
+   return regmap_write(lmu->regmap, reg, data);
+}
+EXPORT_SYMBOL_GPL(ti_lmu_write_byte);
+
+int ti_lmu_update_bits(struct ti_lmu *lmu, u8 reg, u8 mask, u8 data)
+{
+   return regmap_update_bits(lmu->regmap, reg, mask, data);
+}
+EXPORT_SYMBOL_GPL(ti_lmu_update_bits);

Okay, I lied, it does get more simple.

Seems like abstraction for the sake of abstraction here.

Feel free to try and convince me otherwise.



The main reason was that LMU MFD core provides consistent register
access among LMU drivers like ti-lmu-backlight, leds-lm3633,
lm363x-regulator and ti-lmu-fault-monitor('ti-lmu-hwmon' will be changed
to this in the 2nd patch).

However, LMU register helpers are exactly same as regmap interface
except using ti_lmu data structure. So let me replace them with regmap
functions. Thanks for pointing this out.


I just realized LMU MFD core helpers also provide module dependencies by 
using EXPORT_SYMBOL_GPL().


# modprobe -D ti-lmu-backlight
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
# modprobe -D ti-lmu-fault-monitor
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
# modprobe -D lm363x-regulator
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
# modprobe -D leds-lm3633
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko

ti-lmu.ko should be loaded first because it has hardware enable pin 
control code. Four other drivers have dependency on this module. Without 
EXPORT_SYMBOL_GPL(), this dependency will be gone like below.


# modprobe -D ti-lmu-backlight
insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
# modprobe -D ti-lmu-fault-monitor
insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
# modprobe -D lm363x-regulator
insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
# modprobe -D leds-lm3633
insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko

If LMU MFD core module is not loaded and other modules call regmap 
helpers, then loaded drivers will not work because hardware is not 
enabled yet.


So I'd like to keep LMU MFD helpers for module dependencies. 
Additionally, I'll modify 'ti_lmu_read_byte()' as follows.


int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
{
return regmap_read(lmu->regmap, reg, (unsigned int *)read);
}
EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

Please let me know if you have better idea.

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

Re: [PATCH RESEND 06/16] mfd: add TI LMU driver

2015-11-23 Thread Kim, Milo 

Hi Lee,

Thanks for all your comments. Please see my comments below.

On 11/23/2015 7:30 PM, Lee Jones wrote:

+int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
>+{
>+   int ret;
>+   unsigned int val;
>+
>+   ret = regmap_read(lmu->regmap, reg, );
>+   if (ret < 0)
>+   return ret;
>+
>+   *read = (u8)val;
>+   return 0;
>+}
>+EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

It doesn't get much more simple than this.

What's the purpose of abstracting it?


>+int ti_lmu_write_byte(struct ti_lmu *lmu, u8 reg, u8 data)
>+{
>+   return regmap_write(lmu->regmap, reg, data);
>+}
>+EXPORT_SYMBOL_GPL(ti_lmu_write_byte);
>+
>+int ti_lmu_update_bits(struct ti_lmu *lmu, u8 reg, u8 mask, u8 data)
>+{
>+   return regmap_update_bits(lmu->regmap, reg, mask, data);
>+}
>+EXPORT_SYMBOL_GPL(ti_lmu_update_bits);

Okay, I lied, it does get more simple.

Seems like abstraction for the sake of abstraction here.

Feel free to try and convince me otherwise.



The main reason was that LMU MFD core provides consistent register 
access among LMU drivers like ti-lmu-backlight, leds-lm3633, 
lm363x-regulator and ti-lmu-fault-monitor('ti-lmu-hwmon' will be changed 
to this in the 2nd patch).


However, LMU register helpers are exactly same as regmap interface 
except using ti_lmu data structure. So let me replace them with regmap 
functions. Thanks for pointing this out.


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

Re: [PATCH RESEND 06/16] mfd: add TI LMU driver

2015-11-23 Thread Lee Jones 
On Mon, 02 Nov 2015, Milo Kim wrote:

> TI LMU(Lighting Management Unit) driver supports lighting devices below.

Really small nit, I'd prefer a ' ' between "LMU" and "(".

>   LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697.
> 
> LMU devices have common features.
>   - I2C interface for accessing device registers
>   - Hardware enable pin control
>   - Backlight brightness control
>   - Max current conversion helper function
>   - Notifier for hardware fault monitoring
>   - Regulators for LCD display bias
> 
> It contains backlight, HWMON, LED and regulator driver.
> 
> Backlight
> -
>   It's handled by TI LMU backlight common driver and chip dependent driver.
>   Please refer to separate patches for ti-lmu-backlight.
> 
> HWMON
> -
>   LM3633 and LM3697 provide hardware monitoring feature.
>   It enables opened or shorted circuit detection.
>   After monitoring is done, each device should be re-initialized.
>   Notifier is used for this case.
>   Please refer to separate patch for ti-lmu-hwmon.
> 
> LED indicator
> -
>   LM3633 has 6 indicator LEDs. Programmable pattern is also supported.
>   Please refer to separate patch for leds-lm3633.
> 
> Regulator
> -
>   LM3631 has 5 regulators for the display bias.
>   LM3632 supports 3 regulators. One consolidated driver enables it.
>   Please refer to separate patch for lm363x-regulator.
> 
> Cc: Lee Jones 
> Cc: Jingoo Han 
> Cc: Guenter Roeck 
> Cc: Jean Delvare 
> Cc: Jacek Anaszewski 
> Cc: Mark Brown 
> Cc: lm-sens...@lm-sensors.org
> Cc: linux-l...@vger.kernel.org
> Cc: devicet...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Milo Kim 
> ---
>  drivers/mfd/Kconfig |  12 ++
>  drivers/mfd/Makefile|   1 +
>  drivers/mfd/ti-lmu.c| 324 
> 
>  include/linux/mfd/ti-lmu-register.h | 277 ++
>  include/linux/mfd/ti-lmu.h  |  81 +
>  5 files changed, 695 insertions(+)
>  create mode 100644 drivers/mfd/ti-lmu.c
>  create mode 100644 include/linux/mfd/ti-lmu-register.h
>  create mode 100644 include/linux/mfd/ti-lmu.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 99d6367..a53a38e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1037,6 +1037,18 @@ config MFD_LP8788
> TI LP8788 PMU supports regulators, battery charger, RTC,
> ADC, backlight driver and current sinks.
>  
> +config MFD_TI_LMU
> + tristate "TI Lighting Management Unit driver"
> + depends on I2C
> + select MFD_CORE
> + select REGMAP_I2C
> + help
> +   Say yes here to enable support for TI LMU chips.
> +
> +   TI LMU MFD supports LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697.
> +   It consists of backlight, hwmon, LED and regulator driver.
> +   It provides consistent device controls for lighting functions.
> +
>  config MFD_OMAP_USB_HOST
>   bool "TI OMAP USBHS core and TLL driver"
>   depends on USB_EHCI_HCD_OMAP || USB_OHCI_HCD_OMAP3
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index a59e3fc..32920f8 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -110,6 +110,7 @@ obj-$(CONFIG_MFD_AXP20X)  += axp20x.o
>  
>  obj-$(CONFIG_MFD_LP3943) += lp3943.o
>  obj-$(CONFIG_MFD_LP8788) += lp8788.o lp8788-irq.o
> +obj-$(CONFIG_MFD_TI_LMU) += ti-lmu.o
>  
>  da9055-objs  := da9055-core.o da9055-i2c.o
>  obj-$(CONFIG_MFD_DA9055) += da9055.o
> diff --git a/drivers/mfd/ti-lmu.c b/drivers/mfd/ti-lmu.c
> new file mode 100644
> index 000..e86a0ea
> --- /dev/null
> +++ b/drivers/mfd/ti-lmu.c
> @@ -0,0 +1,324 @@
> +/*
> + * TI LMU(Lighting Management Unit) Core Driver
> + *
> + * Copyright 2015 Texas Instruments
> + *
> + * Author: Milo Kim 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define LMU_IMAX_OFFSET  6
> +
> +enum ti_lmu_id {
> + LM3532,
> + LM3631,
> + LM3632,
> + LM3633,
> + LM3695,
> + LM3697,
> +};
> +
> +struct ti_lmu_data {
> + struct mfd_cell *cells;
> + int num_cells;
> + unsigned int max_register;
> +};
> +
> +int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
> +{
> + int ret;
> + unsigned int val;
> +
> + ret = regmap_read(lmu->regmap, reg, );
> + if (ret < 0)
> + return ret;
> +
> + *read = (u8)val;
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

It doesn't get much more simple than this.

What's the purpose of abstracting it?

> +int ti_lmu_write_byte(struct ti_lmu *lmu, u8 reg, u8 data)
> +{
>

Re: [PATCH RESEND 06/16] mfd: add TI LMU driver

2015-11-23 Thread Lee Jones 
On Mon, 02 Nov 2015, Milo Kim wrote:

> TI LMU(Lighting Management Unit) driver supports lighting devices below.

Really small nit, I'd prefer a ' ' between "LMU" and "(".

>   LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697.
> 
> LMU devices have common features.
>   - I2C interface for accessing device registers
>   - Hardware enable pin control
>   - Backlight brightness control
>   - Max current conversion helper function
>   - Notifier for hardware fault monitoring
>   - Regulators for LCD display bias
> 
> It contains backlight, HWMON, LED and regulator driver.
> 
> Backlight
> -
>   It's handled by TI LMU backlight common driver and chip dependent driver.
>   Please refer to separate patches for ti-lmu-backlight.
> 
> HWMON
> -
>   LM3633 and LM3697 provide hardware monitoring feature.
>   It enables opened or shorted circuit detection.
>   After monitoring is done, each device should be re-initialized.
>   Notifier is used for this case.
>   Please refer to separate patch for ti-lmu-hwmon.
> 
> LED indicator
> -
>   LM3633 has 6 indicator LEDs. Programmable pattern is also supported.
>   Please refer to separate patch for leds-lm3633.
> 
> Regulator
> -
>   LM3631 has 5 regulators for the display bias.
>   LM3632 supports 3 regulators. One consolidated driver enables it.
>   Please refer to separate patch for lm363x-regulator.
> 
> Cc: Lee Jones 
> Cc: Jingoo Han 
> Cc: Guenter Roeck 
> Cc: Jean Delvare 
> Cc: Jacek Anaszewski 
> Cc: Mark Brown 
> Cc: lm-sens...@lm-sensors.org
> Cc: linux-l...@vger.kernel.org
> Cc: devicet...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Milo Kim 
> ---
>  drivers/mfd/Kconfig |  12 ++
>  drivers/mfd/Makefile|   1 +
>  drivers/mfd/ti-lmu.c| 324 
> 
>  include/linux/mfd/ti-lmu-register.h | 277 ++
>  include/linux/mfd/ti-lmu.h  |  81 +
>  5 files changed, 695 insertions(+)
>  create mode 100644 drivers/mfd/ti-lmu.c
>  create mode 100644 include/linux/mfd/ti-lmu-register.h
>  create mode 100644 include/linux/mfd/ti-lmu.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 99d6367..a53a38e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1037,6 +1037,18 @@ config MFD_LP8788
> TI LP8788 PMU supports regulators, battery charger, RTC,
> ADC, backlight driver and current sinks.
>  
> +config MFD_TI_LMU
> + tristate "TI Lighting Management Unit driver"
> + depends on I2C
> + select MFD_CORE
> + select REGMAP_I2C
> + help
> +   Say yes here to enable support for TI LMU chips.
> +
> +   TI LMU MFD supports LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697.
> +   It consists of backlight, hwmon, LED and regulator driver.
> +   It provides consistent device controls for lighting functions.
> +
>  config MFD_OMAP_USB_HOST
>   bool "TI OMAP USBHS core and TLL driver"
>   depends on USB_EHCI_HCD_OMAP || USB_OHCI_HCD_OMAP3
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index a59e3fc..32920f8 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -110,6 +110,7 @@ obj-$(CONFIG_MFD_AXP20X)  += axp20x.o
>  
>  obj-$(CONFIG_MFD_LP3943) += lp3943.o
>  obj-$(CONFIG_MFD_LP8788) += lp8788.o lp8788-irq.o
> +obj-$(CONFIG_MFD_TI_LMU) += ti-lmu.o
>  
>  da9055-objs  := da9055-core.o da9055-i2c.o
>  obj-$(CONFIG_MFD_DA9055) += da9055.o
> diff --git a/drivers/mfd/ti-lmu.c b/drivers/mfd/ti-lmu.c
> new file mode 100644
> index 000..e86a0ea
> --- /dev/null
> +++ b/drivers/mfd/ti-lmu.c
> @@ -0,0 +1,324 @@
> +/*
> + * TI LMU(Lighting Management Unit) Core Driver
> + *
> + * Copyright 2015 Texas Instruments
> + *
> + * Author: Milo Kim 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define LMU_IMAX_OFFSET  6
> +
> +enum ti_lmu_id {
> + LM3532,
> + LM3631,
> + LM3632,
> + LM3633,
> + LM3695,
> + LM3697,
> +};
> +
> +struct ti_lmu_data {
> + struct mfd_cell *cells;
> + int num_cells;
> + unsigned int max_register;
> +};
> +
> +int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
> +{
> + int ret;
> + unsigned int val;
> +
> + ret = regmap_read(lmu->regmap, reg, );
> + if (ret < 0)
> + return ret;
> +
> + *read = (u8)val;
> + return 0;
> +}
>

Re: [PATCH RESEND 06/16] mfd: add TI LMU driver

2015-11-23 Thread Kim, Milo 

On 11/24/2015 11:35 AM, Kim, Milo wrote:

Hi Lee,

Thanks for all your comments. Please see my comments below.

On 11/23/2015 7:30 PM, Lee Jones wrote:

+int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)

+{
+   int ret;
+   unsigned int val;
+
+   ret = regmap_read(lmu->regmap, reg, );
+   if (ret < 0)
+   return ret;
+
+   *read = (u8)val;
+   return 0;
+}
+EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

It doesn't get much more simple than this.

What's the purpose of abstracting it?


+int ti_lmu_write_byte(struct ti_lmu *lmu, u8 reg, u8 data)
+{
+   return regmap_write(lmu->regmap, reg, data);
+}
+EXPORT_SYMBOL_GPL(ti_lmu_write_byte);
+
+int ti_lmu_update_bits(struct ti_lmu *lmu, u8 reg, u8 mask, u8 data)
+{
+   return regmap_update_bits(lmu->regmap, reg, mask, data);
+}
+EXPORT_SYMBOL_GPL(ti_lmu_update_bits);

Okay, I lied, it does get more simple.

Seems like abstraction for the sake of abstraction here.

Feel free to try and convince me otherwise.



The main reason was that LMU MFD core provides consistent register
access among LMU drivers like ti-lmu-backlight, leds-lm3633,
lm363x-regulator and ti-lmu-fault-monitor('ti-lmu-hwmon' will be changed
to this in the 2nd patch).

However, LMU register helpers are exactly same as regmap interface
except using ti_lmu data structure. So let me replace them with regmap
functions. Thanks for pointing this out.


I just realized LMU MFD core helpers also provide module dependencies by 
using EXPORT_SYMBOL_GPL().


# modprobe -D ti-lmu-backlight
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
# modprobe -D ti-lmu-fault-monitor
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
# modprobe -D lm363x-regulator
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
# modprobe -D leds-lm3633
insmod /lib/modules//kernel/drivers/base/regmap/regmap-i2c.ko
insmod /lib/modules//kernel/drivers/mfd/mfd-core.ko
insmod /lib/modules//kernel/drivers/mfd/ti-lmu.ko
insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko

ti-lmu.ko should be loaded first because it has hardware enable pin 
control code. Four other drivers have dependency on this module. Without 
EXPORT_SYMBOL_GPL(), this dependency will be gone like below.


# modprobe -D ti-lmu-backlight
insmod /lib/modules//kernel/drivers/video/backlight/ti-lmu-backlight.ko
# modprobe -D ti-lmu-fault-monitor
insmod /lib/modules//kernel/drivers/mfd/ti-lmu-fault-monitor.ko
# modprobe -D lm363x-regulator
insmod /lib/modules//kernel/drivers/regulator/lm363x-regulator.ko
# modprobe -D leds-lm3633
insmod /lib/modules//kernel/drivers/leds/leds-lm3633.ko

If LMU MFD core module is not loaded and other modules call regmap 
helpers, then loaded drivers will not work because hardware is not 
enabled yet.


So I'd like to keep LMU MFD helpers for module dependencies. 
Additionally, I'll modify 'ti_lmu_read_byte()' as follows.


int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
{
return regmap_read(lmu->regmap, reg, (unsigned int *)read);
}
EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

Please let me know if you have better idea.

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

Re: [PATCH RESEND 06/16] mfd: add TI LMU driver

2015-11-23 Thread Kim, Milo 

Hi Lee,

Thanks for all your comments. Please see my comments below.

On 11/23/2015 7:30 PM, Lee Jones wrote:

+int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
>+{
>+   int ret;
>+   unsigned int val;
>+
>+   ret = regmap_read(lmu->regmap, reg, );
>+   if (ret < 0)
>+   return ret;
>+
>+   *read = (u8)val;
>+   return 0;
>+}
>+EXPORT_SYMBOL_GPL(ti_lmu_read_byte);

It doesn't get much more simple than this.

What's the purpose of abstracting it?


>+int ti_lmu_write_byte(struct ti_lmu *lmu, u8 reg, u8 data)
>+{
>+   return regmap_write(lmu->regmap, reg, data);
>+}
>+EXPORT_SYMBOL_GPL(ti_lmu_write_byte);
>+
>+int ti_lmu_update_bits(struct ti_lmu *lmu, u8 reg, u8 mask, u8 data)
>+{
>+   return regmap_update_bits(lmu->regmap, reg, mask, data);
>+}
>+EXPORT_SYMBOL_GPL(ti_lmu_update_bits);

Okay, I lied, it does get more simple.

Seems like abstraction for the sake of abstraction here.

Feel free to try and convince me otherwise.



The main reason was that LMU MFD core provides consistent register 
access among LMU drivers like ti-lmu-backlight, leds-lm3633, 
lm363x-regulator and ti-lmu-fault-monitor('ti-lmu-hwmon' will be changed 
to this in the 2nd patch).


However, LMU register helpers are exactly same as regmap interface 
except using ti_lmu data structure. So let me replace them with regmap 
functions. Thanks for pointing this out.


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

[PATCH RESEND 06/16] mfd: add TI LMU driver

2015-11-01 Thread Milo Kim 
TI LMU(Lighting Management Unit) driver supports lighting devices below.

  LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697.

LMU devices have common features.
  - I2C interface for accessing device registers
  - Hardware enable pin control
  - Backlight brightness control
  - Max current conversion helper function
  - Notifier for hardware fault monitoring
  - Regulators for LCD display bias

It contains backlight, HWMON, LED and regulator driver.

Backlight
-
  It's handled by TI LMU backlight common driver and chip dependent driver.
  Please refer to separate patches for ti-lmu-backlight.

HWMON
-
  LM3633 and LM3697 provide hardware monitoring feature.
  It enables opened or shorted circuit detection.
  After monitoring is done, each device should be re-initialized.
  Notifier is used for this case.
  Please refer to separate patch for ti-lmu-hwmon.

LED indicator
-
  LM3633 has 6 indicator LEDs. Programmable pattern is also supported.
  Please refer to separate patch for leds-lm3633.

Regulator
-
  LM3631 has 5 regulators for the display bias.
  LM3632 supports 3 regulators. One consolidated driver enables it.
  Please refer to separate patch for lm363x-regulator.

Cc: Lee Jones 
Cc: Jingoo Han 
Cc: Guenter Roeck 
Cc: Jean Delvare 
Cc: Jacek Anaszewski 
Cc: Mark Brown 
Cc: lm-sens...@lm-sensors.org
Cc: linux-l...@vger.kernel.org
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Milo Kim 
---
 drivers/mfd/Kconfig |  12 ++
 drivers/mfd/Makefile|   1 +
 drivers/mfd/ti-lmu.c| 324 
 include/linux/mfd/ti-lmu-register.h | 277 ++
 include/linux/mfd/ti-lmu.h  |  81 +
 5 files changed, 695 insertions(+)
 create mode 100644 drivers/mfd/ti-lmu.c
 create mode 100644 include/linux/mfd/ti-lmu-register.h
 create mode 100644 include/linux/mfd/ti-lmu.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 99d6367..a53a38e 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1037,6 +1037,18 @@ config MFD_LP8788
  TI LP8788 PMU supports regulators, battery charger, RTC,
  ADC, backlight driver and current sinks.
 
+config MFD_TI_LMU
+   tristate "TI Lighting Management Unit driver"
+   depends on I2C
+   select MFD_CORE
+   select REGMAP_I2C
+   help
+ Say yes here to enable support for TI LMU chips.
+
+ TI LMU MFD supports LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697.
+ It consists of backlight, hwmon, LED and regulator driver.
+ It provides consistent device controls for lighting functions.
+
 config MFD_OMAP_USB_HOST
bool "TI OMAP USBHS core and TLL driver"
depends on USB_EHCI_HCD_OMAP || USB_OHCI_HCD_OMAP3
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index a59e3fc..32920f8 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -110,6 +110,7 @@ obj-$(CONFIG_MFD_AXP20X)+= axp20x.o
 
 obj-$(CONFIG_MFD_LP3943)   += lp3943.o
 obj-$(CONFIG_MFD_LP8788)   += lp8788.o lp8788-irq.o
+obj-$(CONFIG_MFD_TI_LMU)   += ti-lmu.o
 
 da9055-objs:= da9055-core.o da9055-i2c.o
 obj-$(CONFIG_MFD_DA9055)   += da9055.o
diff --git a/drivers/mfd/ti-lmu.c b/drivers/mfd/ti-lmu.c
new file mode 100644
index 000..e86a0ea
--- /dev/null
+++ b/drivers/mfd/ti-lmu.c
@@ -0,0 +1,324 @@
+/*
+ * TI LMU(Lighting Management Unit) Core Driver
+ *
+ * Copyright 2015 Texas Instruments
+ *
+ * Author: Milo Kim 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define LMU_IMAX_OFFSET6
+
+enum ti_lmu_id {
+   LM3532,
+   LM3631,
+   LM3632,
+   LM3633,
+   LM3695,
+   LM3697,
+};
+
+struct ti_lmu_data {
+   struct mfd_cell *cells;
+   int num_cells;
+   unsigned int max_register;
+};
+
+int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
+{
+   int ret;
+   unsigned int val;
+
+   ret = regmap_read(lmu->regmap, reg, );
+   if (ret < 0)
+   return ret;
+
+   *read = (u8)val;
+   return 0;
+}
+EXPORT_SYMBOL_GPL(ti_lmu_read_byte);
+
+int ti_lmu_write_byte(struct ti_lmu *lmu, u8 reg, u8 data)
+{
+   return regmap_write(lmu->regmap, reg, data);
+}
+EXPORT_SYMBOL_GPL(ti_lmu_write_byte);
+
+int ti_lmu_update_bits(struct ti_lmu *lmu, u8 reg, u8 mask, u8 data)
+{
+   return regmap_update_bits(lmu->regmap, reg, mask, data);
+}
+EXPORT_SYMBOL_GPL(ti_lmu_update_bits);
+
+/*
+ * LMU backlight and LED devices use shared max current table.
+ * This function finds appropriate register index and return it.
+ */
+enum

[PATCH RESEND 06/16] mfd: add TI LMU driver

2015-11-01 Thread Milo Kim 
TI LMU(Lighting Management Unit) driver supports lighting devices below.

  LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697.

LMU devices have common features.
  - I2C interface for accessing device registers
  - Hardware enable pin control
  - Backlight brightness control
  - Max current conversion helper function
  - Notifier for hardware fault monitoring
  - Regulators for LCD display bias

It contains backlight, HWMON, LED and regulator driver.

Backlight
-
  It's handled by TI LMU backlight common driver and chip dependent driver.
  Please refer to separate patches for ti-lmu-backlight.

HWMON
-
  LM3633 and LM3697 provide hardware monitoring feature.
  It enables opened or shorted circuit detection.
  After monitoring is done, each device should be re-initialized.
  Notifier is used for this case.
  Please refer to separate patch for ti-lmu-hwmon.

LED indicator
-
  LM3633 has 6 indicator LEDs. Programmable pattern is also supported.
  Please refer to separate patch for leds-lm3633.

Regulator
-
  LM3631 has 5 regulators for the display bias.
  LM3632 supports 3 regulators. One consolidated driver enables it.
  Please refer to separate patch for lm363x-regulator.

Cc: Lee Jones 
Cc: Jingoo Han 
Cc: Guenter Roeck 
Cc: Jean Delvare 
Cc: Jacek Anaszewski 
Cc: Mark Brown 
Cc: lm-sens...@lm-sensors.org
Cc: linux-l...@vger.kernel.org
Cc: devicet...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Milo Kim 
---
 drivers/mfd/Kconfig |  12 ++
 drivers/mfd/Makefile|   1 +
 drivers/mfd/ti-lmu.c| 324 
 include/linux/mfd/ti-lmu-register.h | 277 ++
 include/linux/mfd/ti-lmu.h  |  81 +
 5 files changed, 695 insertions(+)
 create mode 100644 drivers/mfd/ti-lmu.c
 create mode 100644 include/linux/mfd/ti-lmu-register.h
 create mode 100644 include/linux/mfd/ti-lmu.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 99d6367..a53a38e 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1037,6 +1037,18 @@ config MFD_LP8788
  TI LP8788 PMU supports regulators, battery charger, RTC,
  ADC, backlight driver and current sinks.
 
+config MFD_TI_LMU
+   tristate "TI Lighting Management Unit driver"
+   depends on I2C
+   select MFD_CORE
+   select REGMAP_I2C
+   help
+ Say yes here to enable support for TI LMU chips.
+
+ TI LMU MFD supports LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697.
+ It consists of backlight, hwmon, LED and regulator driver.
+ It provides consistent device controls for lighting functions.
+
 config MFD_OMAP_USB_HOST
bool "TI OMAP USBHS core and TLL driver"
depends on USB_EHCI_HCD_OMAP || USB_OHCI_HCD_OMAP3
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index a59e3fc..32920f8 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -110,6 +110,7 @@ obj-$(CONFIG_MFD_AXP20X)+= axp20x.o
 
 obj-$(CONFIG_MFD_LP3943)   += lp3943.o
 obj-$(CONFIG_MFD_LP8788)   += lp8788.o lp8788-irq.o
+obj-$(CONFIG_MFD_TI_LMU)   += ti-lmu.o
 
 da9055-objs:= da9055-core.o da9055-i2c.o
 obj-$(CONFIG_MFD_DA9055)   += da9055.o
diff --git a/drivers/mfd/ti-lmu.c b/drivers/mfd/ti-lmu.c
new file mode 100644
index 000..e86a0ea
--- /dev/null
+++ b/drivers/mfd/ti-lmu.c
@@ -0,0 +1,324 @@
+/*
+ * TI LMU(Lighting Management Unit) Core Driver
+ *
+ * Copyright 2015 Texas Instruments
+ *
+ * Author: Milo Kim 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define LMU_IMAX_OFFSET6
+
+enum ti_lmu_id {
+   LM3532,
+   LM3631,
+   LM3632,
+   LM3633,
+   LM3695,
+   LM3697,
+};
+
+struct ti_lmu_data {
+   struct mfd_cell *cells;
+   int num_cells;
+   unsigned int max_register;
+};
+
+int ti_lmu_read_byte(struct ti_lmu *lmu, u8 reg, u8 *read)
+{
+   int ret;
+   unsigned int val;
+
+   ret = regmap_read(lmu->regmap, reg, );
+   if (ret < 0)
+   return ret;
+
+   *read = (u8)val;
+   return 0;
+}
+EXPORT_SYMBOL_GPL(ti_lmu_read_byte);
+
+int ti_lmu_write_byte(struct ti_lmu *lmu, u8 reg, u8 data)
+{
+   return regmap_write(lmu->regmap, reg, data);
+}
+EXPORT_SYMBOL_GPL(ti_lmu_write_byte);
+
+int ti_lmu_update_bits(struct ti_lmu *lmu, u8 reg, u8 mask, u8 data)
+{
+   return regmap_update_bits(lmu->regmap, reg, mask, data);
+}