Message-ID: <20120521220754.GA6659@localhost>
Date:	Tue, 22 May 2012 00:07:54 +0200
From:	Johan Hovold <jhovold@...il.com>
To:	Jonathan Cameron <jic23@...nel.org>
Cc:	Johan Hovold <jhovold@...il.com>,
	Jonathan Cameron <jic23@....ac.uk>,
	Rob Landley <rob@...dley.net>,
	Richard Purdie <rpurdie@...ys.net>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Florian Tobias Schandinat <FlorianSchandinat@....de>,
	Arnd Bergmann <arnd@...db.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-iio@...r.kernel.org,
	"Hennerich, Michael" <Michael.Hennerich@...log.com>
Subject: Re: [PATCH v4] iio: add LM3533 ambient-light-sensor driver

On Mon, May 21, 2012 at 05:37:11PM +0100, Jonathan Cameron wrote:
> Michael cc'd for comments on core support of some stuff that is also
> in frequency drivers down the end of the email.
> 
> On 05/21/2012 10:50 AM, Johan Hovold wrote:
> > On Sat, May 19, 2012 at 09:48:23AM +0100, Jonathan Cameron wrote:
> >> On 05/18/2012 02:07 PM, Johan Hovold wrote:
> >>> Add sub-driver for the ambient-light-sensor interface on National
> >>> Semiconductor / TI LM3533 lighting power chips.
> >>>
> >>> The sensor interface can be used to control the LEDs and backlights of
> >>> the chip through defining five light zones and three sets of
> >>> corresponding brightness target levels.
> >>>
> >>> The driver provides raw and mean adc readings along with the current
> >>> light zone through sysfs. A threshold event can be generated on zone
> >>> changes.
> >>
> >> Hi Johan,
> >>
> >> I hate to be a pain with this one, but it's a complex beast and I'd
> >> really like to get the interface right first time - particularly as
> >> you are going in after the move out of staging.
> >>
> >>
> >> Queries for you.
> >> 1) Ordering in the probe function. Normally expect iio_device_register
> >> to be the last call. Why not here?
> >> 2) Worth combining enable / disable into one as very similar functions?
> >> 3) Suspicious code in als_set_input_mode
> >>
> >> Naming of the target values.  I think we can make the naming of these
> >> fit in much better with the normal abi which is going to be all for the
> >> good in the long run.  They are basically current output channels
> >> with a controllable set of steps (where we don't have direct control
> >> of which one we are in).  This is very similar to the frequency controls
> >> on some of Analog's dds that we have a well defined interface for.
> >>
> >> More detail below, but in summary.
> >>
> >> out_currentX_currentY_raw for channel X value for zone Y.
> >>
> >> Jonathan
> >>>
> >>> Signed-off-by: Johan Hovold <jhovold@...il.com>
> >>> ---
> >>>
> >>> Note that addition of r_select to the platform data probably needs to go
> >>> in via mfd.
> >>>
> >>>
> >>> v2:
> >>>  - reimplement using iio
> >>>  - add sysfs-ABI documentation
> >>> v3
> >>>  - use indexed channel
> >>>  - fix sysfs-ABI documentation typo and style
> >>>  - replace gain attribute with in_illuminance0_calibscale
> >>>  - add calibscale to platform data
> >>>  - fix adc register definitions
> >>>  - replace to_lm3533_dev_attr macro with inline function
> >>>  - fix device used for error reporting at irq allocation
> >>>  - use iio device for error reporting during setup/enable
> >>>  - rebase on staging-next
> >>>    - fix header include paths
> >>>    - use dev_to_iio_dev
> >>>    - add IIO_CHAN_INFO_RAW to info mask
> >>>    - use iio_device_{alloc,free}
> >>> v4
> >>>  - move to driver/iio/light
> >>>  - add events/in_illuminance0_threshY_hysteresis attributes
> >>>  - fix device zone-boundary quirkiness
> >>>  - clean up attribute handling
> >>>  - replace calibscale with device-specific r_select attribute
> >>>
> >>>
> >>>  .../ABI/testing/sysfs-bus-iio-light-lm3533-als     |   64 ++
> >>>  drivers/iio/Kconfig                                |    1 +
> >>>  drivers/iio/Makefile                               |    1 +
> >>>  drivers/iio/light/Kconfig                          |   22 +
> >>>  drivers/iio/light/Makefile                         |    5 +
> >>>  drivers/iio/light/lm3533-als.c                     |  941 ++++++++++++++++++++
> >>>  include/linux/mfd/lm3533.h                         |    1 +
> >>>  7 files changed, 1035 insertions(+), 0 deletions(-)
> >>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-light-lm3533-als
> >>>  create mode 100644 drivers/iio/light/Kconfig
> >>>  create mode 100644 drivers/iio/light/Makefile
> >>>  create mode 100644 drivers/iio/light/lm3533-als.c
> >>>
> >>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-light-lm3533-als b/Documentation/ABI/testing/sysfs-bus-iio-light-lm3533-als
> >>> new file mode 100644
> >>> index 0000000..7ea1770
> >>> --- /dev/null
> >>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-light-lm3533-als
> >>> @@ -0,0 +1,64 @@
> >>> +What:		/sys/bus/iio/devices/iio:deviceX/r_select
> >>> +Date:		April 2012
> >>> +KernelVersion:	3.5
> >>> +Contact:	Johan Hovold <jhovold@...il.com>
> >>> +Description:
> >>> +		Set the ALS internal pull-down resistor for analog input mode
> >>> +		(1..127), such that,
> >>> +
> >>> +		R_als = 200000 / r_select	(ohm)
> >>> +
> >>> +		This setting is ignored in PWM-mode (input is always high
> >>> +		impedance in PWM-mode).
> >>> +
> >>> +What:		/sys/.../events/in_illuminance0_thresh_either_en
> >>> +Date:		April 2012
> >>> +KernelVersion:	3.5
> >>> +Contact:	Johan Hovold <jhovold@...il.com>
> >>> +Description:
> >>> +		Event generated when channel passes one of the four thresholds
> >>> +		in each direction (rising|falling) and a zone change occurs.
> >>> +		The corresponding light zone can be read from
> >>> +		in_illuminance0_zone.
> >>> +
> >>> +What:		/sys/.../events/in_illuminance0_threshY_hysteresis
> >>> +Date:		May 2012
> >>> +KernelVersion:	3.5
> >>> +Contact:	Johan Hovold <jhovold@...il.com>
> >>> +Description:
> >>> +		Get the hysteresis for thresholds Y, that is,
> >>> +
> >>> +		threshY_hysteresis = threshY_raising - threshY_falling
> >>> +
> >>> +What:		/sys/.../events/illuminance_threshY_falling_value
> >>> +What:		/sys/.../events/illuminance_threshY_raising_value
> >>> +Date:		April 2012
> >>> +KernelVersion:	3.5
> >>> +Contact:	Johan Hovold <jhovold@...il.com>
> >>> +Description:
> >>> +		Specifies the value of threshold that the device is
> >>> +		comparing against for the events enabled by
> >>> +		in_illuminance0_thresh_either_en, where Y in 0..3.
> >>> +
> >>> +		Note that threshY_falling must be less than or equal to
> >>> +		threshY_raising.
> >>> +
> >>> +		These thresholds correspond to the eight zone-boundary
> >>> +		registers (boundaryY_{low,high}) and defines the five light
> >>> +		zones.
> >>> +
> >>> +What:		/sys/bus/iio/devices/iio:deviceX/in_illuminance0_zone
> >>> +Date:		April 2012
> >>> +KernelVersion:	3.5
> >>> +Contact:	Johan Hovold <jhovold@...il.com>
> >>> +Description:
> >>> +		Get the current light zone (0..4) as defined by the
> >>> +		in_illuminance0_threshY_{falling,rising} thresholds.
> >>> +
> >>> +What:		/sys/bus/iio/devices/iio:deviceX/targetY_Z
> >>> +Date:		April 2012
> >>> +KernelVersion:	3.5
> >>> +Contact:	Johan Hovold <jhovold@...il.com>
> >>> +Description:
> >>> +		Set the target brightness for ALS-mapper Y in light zone Z
> >>> +		(0..255), where Y in 1..3 and Z in 0..4.
> >>
> >> What are the units of this?
> > 
> > The datasheet reads "percent of the full-scale current" (actually depends
> > somewhat on whether the als is in linear or exponential mode). When the
> > leds or backlights are in PWM-mode (not the ALS necessarily), these
> > values are interpreted as a scale factor which is applied to the output
> > current determined by the PWM-signal.
> > 
> > Either way it could indeed be considered a raw output current (which
> > could be manipulated later by various factors).
> Fine.... Theoretically if at all possible we'd want the conversion
> factors to get it to an actual current (in amps) to be available though.
> (tend to relax that if there are unknowable elements or they aren't
> specified by board file etc).

Hmmm. I'm starting to get a feeling that we're over-doing this. The ALS
on the lm3533 isn't a general purpose sensor. It's simply a way to
control the leds and backlights of that device. So what you do is to 
determine the full-scale current (max current at maximum brightness 0xff
in this case -- set in board file). Then the ALS input range is divided
in 5 zones, and for each zone you set a brightness as a percent of the
full-scale current. You relly don't care about amps (except for the
maximum determined by the setup).

The equation's for determining the current are available in the
datasheets however, but they depend on which mapping mode (linear or
exponential) and can also be effected by PWM-input duty cycle etc. For
this particular device, I really don't see the point in trying to
determine actual current in amps in all these settings.

Note also that the actual output current cannot be determined in the
ALS as the required factors are only set/known in the led/backlight
devices (mapping mode, pwm-mode).

> >> Also arguably is it not the als that this is related to, but rather
> >> the light source?
> > 
> > Well, it would be a raw output, mapping the measured LUX.
> Fair enough, though I wonder if we are stepping on led / backlight
> classes stuff with this.

Keeping the target sets and the mapper terminology could still be an
option. 

ALS input mode is a special mode of the LEDs and backlights which
overrides the direct current control. It's indeed a special-purpose
device.

> >> A quick datasheet browse says that these are current targets? If so I
> >> wonder if we can make that explicit...  Could treat them as 3 output
> >> channels and have indexed values like we do for frequencies in dds
> >> devices (where external hardware is controlling them.
> > 
> > I think I like this.
> > 
> >> Hmm. lets see.
> >>
> >> out_currentX_currentY_raw
> >> (the double naming is a bit clunky but corresponds to frequency devices
> >> where we have
> >> out_altvoltageX_frequencyY_raw
> >>
> >> Hence we'd treat you 3 mapers as indexed channels 0,1,2 and the zones
> >> as indexed values they can take 0,1,2,3,4
> >> out_currentX_raw is not read only and gives you the current for whichever
> >> zone the device is currently in.
> > 
> > I take it you mean "out_currentX_raw is read only".
> yes. I do indeed. oops.
> > 
> >> This may seem convoluted but I'd really rather have something generalizable
> >> for this if we possibly can.  We'd still need documentation to say what is
> >> controlling these, but at least they'd fit within our more general abi.
> >>
> >> What do you think?
> > 
> > I like it. From a user perspective it's mainly a change of names (and
> > indexes). But conceptually it's perhaps more clear: the als maps it's
> > input to an output current, which, just like a PWM-signal, could be used
> > as an input to the LEDs and backlights to determine their outputs.
> > 
> > I'd have to modify the LED and backlight interfaces somewhat to reflect
> > the changed indexes and terminology (e.g. "output channel" rather
> > than "ALS mapper"). Something like:
> > 
> > 	als_en		-- enable als input mode (0,1)
> > 	als_channel	-- which out_currentX to use as input (0..2) in
> > 			   als input mode
> Not entirely sure I'm happy with this. Would rather it was done
> on a per channel basis, so in_illuminance0_ *
> From point of view of sensors I don't really care if it is an als or
> measuring something active (hence inherently not ambient!)

Not sure I understood that. What is it you don't like about it? You need
to keep in mind what is actually there; three sets of target values per
zone of which one set is dedicated to the first backlight device. That
means, that the ALS mapper (or channel if we want to use that
terminology) needs to be set in the actual devices and not the other way
round. [ The als_en and als_channel attributes would belong to the
led/backlight devices. ]

What did you mean by "per channel basis, so in_illuminance0_ *"?

Again it's a special purpose device -- the lm3533 leds and backlights
are controlled in hw by the on-chip als interface. We can't just use any
generic iio device for this.

> Silly question but how is the out_current related to the input in als
> mode?

1. raw adc input is averaged
2. mean adc input is mapped to zone using zone registers ("thresholds")
3. zone is mapped to a percentage using ALS mapper registers, that is, 
   three sets of 8-bit values per zone. Which set is used can be set on
   a per-device (led, backlight) basis
4. the percentage is applied to the per-device full-scale current to
   determine the actual output. How this mapping is done depends on if
   linear or exponential mapping mode is set (also per-device).

[ And things can get even more complicated if the devices are in
  PWM-mode, but this is roughly the full picture. ]

And all of the above is done in hw.

So, we're only using out_current channels because it maybe fits iio
better. For anyone familiar with the lm3533 this may even just confuse
things. 

There are only the three tables of values that maps a zone to an output
percentage (e.g. half brightness in zone 2).

> > So to summarise, we get the following new sysfs-entries for the ALS
> > (where the first set replace targetX_Y):
> > 
> > 	out_currentX_currentY_raw	r/w, (0..255), X in 0..2, Y in 0..4
> > 	out_currentX_raw		ro (0..255), X in 0..2
> > 
> > Is there any support in core for the first set or should I simply
> > rename my target attributes?
> No support in the core yet for this sort of thing..
> Michael, any thoughts on this? In a sense it's very similar to
> out_altvoltageX_frequencyY_raw etc...
> > 
> > Thanks,
> > Johan
> 
--
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/

Powered by blists - more mailing lists