Re: [PATCH v2 1/4] mfd: add LM3533 lighting-power core driver

On Thu, May 03, 2012 at 11:38:48AM +0100, Mark Brown wrote:

On Thu, May 03, 2012 at 12:26:36PM +0200, Johan Hovold wrote:

quoted

Add support for National Semiconductor / TI LM3533 lighting power chips.

This is the core driver which provides register access over I2C and
registers the ambient-light-sensor, LED and backlight sub-drivers.

Reviwed-by: Mark Brown [off-list ref]

though

quoted

+	dev_dbg(lm3533->dev, "read [%02x]: %02x\n", reg, *val);

I'd expect you can drop these log messages, if there's stuff like this
missing we should add it to regmap.  At the minute the regmap logging is
via trace points rather than debug logs as you can leave them enabled
all the time.

If such debugging is added to regmap we still need a way to enable them
per driver (or rather regmap) to not clutter the logs.

These three dev_dbg statements are extremely useful during debugging /
development especially in combination with the other dynamic printks in
these drivers.

I'd actually prefer just keeping them for now.
 

Might also be worth moving some of the sysfs stuff to live with the
relevant drivers.

Which attributes do you have in mind?

The boost_freq and boost_ovp affect both backlight devices (i.e. cannot
be set separately) and as such belong in the parent driver IMO.

Same with the output_hvled{1..2}, output_lvled{1..5} attributes. The
chip has four logical LEDs ("control banks") but five low-voltage output
sinks. The five output_lvled attributes determine the mapping and as
such belong in the parent driver. The two logical backlight devices can
likewise be used to control either or both high-voltage outputs and
belong in the parent driver for the same reasons.

Thanks,
Johan