* Re: [PATCH v2 2/4] iio: add LM3533 ambient light sensor driver
From: Jonathan Cameron @ 2012-05-19 8:04 UTC (permalink / raw)
To: Johan Hovold
Cc: Jonathan Cameron, Rob Landley, Richard Purdie, Samuel Ortiz,
Greg Kroah-Hartman, Florian Tobias Schandinat, Arnd Bergmann,
Andrew Morton, Mark Brown, linux-doc, linux-kernel, linux-iio,
devel, linux-fbdev
In-Reply-To: <20120518175742.GA12894@localhost>
On 05/18/2012 06:57 PM, Johan Hovold wrote:
> On Fri, May 18, 2012 at 06:34:01PM +0100, Jonathan Cameron wrote:
>> On 05/18/2012 01:27 PM, Johan Hovold wrote:
>
> [...]
>
>>> I really think that this should remain a device specific attribute as I
>>> originally suggested. It's an integration parameter that needs to be set
>>> precisely depending on the actual hardware setup (which analog light
>>> sensor and other external components).
>> Then it shouldn't be exposed to userspace. If there is reason to vary
>> it from userspace then it is a calibration parameter and should be
>> treated like the other ones we have, if not it should be done from
>> dt or platform data.
>>>
>>> The lm3533 also supports two types of light sensors: pwm- and analog-
>>> output ones. The resistor select settings only applies when in analog
>>> mode as the input is always high impedance otherwise. Thus a generic
>>> attribute (such as calibscale or hardware gain) shouldn't be used as it
>>> will have no effect whatsoever in PWM-mode.
>>>
>>> I'm thus back at my original proposal, albeit with a different name (I
>>> think a lot of this discussion could have been avoided had I not
>>> misnamed the parameter "gain"):
>>>
>>> What: /sys/bus/iio/devices/iio:deviceX/r_select
>>> 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).
>>>
>>> I don't think much is gained from using ohm as the unit: it just adds
>>> complexity and the selected resistor setting will likely not match the
>>> input value anyway. It's better that the chip integrators have full
>>> control over which resistor setting is actually used so that it matches
>>> external components.
>> This smacks of something that should never be exposed to users.
>> I'd hide it away in platform data.
>
> Fair enough. I'll drop the sysfs param and submit a patch for mfd-next
> which adds r_select to the platform data.
>
cool. I'll review the rest of the patch with the assumption you'll do this.
Jonathan
^ permalink raw reply
* Re: [PATCH v2 2/4] iio: add LM3533 ambient light sensor driver
From: Johan Hovold @ 2012-05-18 17:57 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Johan Hovold, Jonathan Cameron, Rob Landley, Richard Purdie,
Samuel Ortiz, Greg Kroah-Hartman, Florian Tobias Schandinat,
Arnd Bergmann, Andrew Morton, Mark Brown, linux-doc, linux-kernel,
linux-iio, devel, linux-fbdev
In-Reply-To: <4FB68809.2010009@kernel.org>
On Fri, May 18, 2012 at 06:34:01PM +0100, Jonathan Cameron wrote:
> On 05/18/2012 01:27 PM, Johan Hovold wrote:
[...]
> > I really think that this should remain a device specific attribute as I
> > originally suggested. It's an integration parameter that needs to be set
> > precisely depending on the actual hardware setup (which analog light
> > sensor and other external components).
> Then it shouldn't be exposed to userspace. If there is reason to vary
> it from userspace then it is a calibration parameter and should be
> treated like the other ones we have, if not it should be done from
> dt or platform data.
> >
> > The lm3533 also supports two types of light sensors: pwm- and analog-
> > output ones. The resistor select settings only applies when in analog
> > mode as the input is always high impedance otherwise. Thus a generic
> > attribute (such as calibscale or hardware gain) shouldn't be used as it
> > will have no effect whatsoever in PWM-mode.
> >
> > I'm thus back at my original proposal, albeit with a different name (I
> > think a lot of this discussion could have been avoided had I not
> > misnamed the parameter "gain"):
> >
> > What: /sys/bus/iio/devices/iio:deviceX/r_select
> > 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).
> >
> > I don't think much is gained from using ohm as the unit: it just adds
> > complexity and the selected resistor setting will likely not match the
> > input value anyway. It's better that the chip integrators have full
> > control over which resistor setting is actually used so that it matches
> > external components.
> This smacks of something that should never be exposed to users.
> I'd hide it away in platform data.
Fair enough. I'll drop the sysfs param and submit a patch for mfd-next
which adds r_select to the platform data.
Thanks,
Johan
^ permalink raw reply
* Re: [PATCH v2 2/4] iio: add LM3533 ambient light sensor driver
From: Jonathan Cameron @ 2012-05-18 17:34 UTC (permalink / raw)
To: Johan Hovold
Cc: Jonathan Cameron, Rob Landley, Richard Purdie, Samuel Ortiz,
Greg Kroah-Hartman, Florian Tobias Schandinat, Arnd Bergmann,
Andrew Morton, Mark Brown, linux-doc, linux-kernel, linux-iio,
devel, linux-fbdev
In-Reply-To: <20120518122725.GB20467@localhost>
On 05/18/2012 01:27 PM, Johan Hovold wrote:
> On Wed, May 16, 2012 at 03:21:14PM +0100, Jonathan Cameron wrote:
>> On 5/16/2012 2:05 PM, Johan Hovold wrote:
>>> On Tue, May 15, 2012 at 09:00:46PM +0100, Jonathan Cameron wrote:
>>>> On 05/15/2012 05:44 PM, Johan Hovold wrote:
>>>>> On Tue, May 08, 2012 at 02:47:19PM +0100, Jonathan Cameron wrote:
>>>>>> On 5/3/2012 5:36 PM, Johan Hovold wrote:
>>>>>>> On Thu, May 03, 2012 at 12:40:10PM +0100, Jonathan Cameron wrote:
>>>>>>>> On 5/3/2012 11:26 AM, 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.
>>>>>>>> Code is fine. Pretty much all my comments are to do with the interface.
>>>>>>> [...]
>>>>>>>
>>>>>>>>> diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als
>>>>>>>>> new file mode 100644
>>>>>>>>> index 0000000..9849d14
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als
>>>>>>>>> @@ -0,0 +1,62 @@
>>>>>>>>> +What: /sys/bus/iio/devices/iio:deviceX/gain
>>>>>>>>> +Date: April 2012
>>>>>>>>> +KernelVersion: 3.5
>>>>>>>>> +Contact: Johan Hovold<jhovold@gmail.com>
>>>>>>>>> +Description:
>>>>>>>>> + Set the ALS gain-resistor setting (0..127) for analog input
>>>>>>>>> + mode, where
>>>>>>>>> +
>>>>>>>>> + 0000000 - ALS input is high impedance
>>>>>>>>> + 0000001 - 200kOhm (10uA at 2V full-scale)
>>>>>>>>> + 0000010 - 100kOhm (20uA at 2V full-scale)
>>>>>>>>> + ...
>>>>>>>>> + 1111110 - 1.587kOhm (1.26mA at 2V full-scale)
>>>>>>>>> + 1111111 - 1.575kOhm (1.27mA at 2V full-scale)
>>>>>>>>> +
>>>>>>>>> + R_als = 2V / (10uA * gain) (gain> 0)
>>>>>>>> Firstly, no magic numbers. These are definitely magic.
>>>>>>> Not that magic as they're clearly documented (in code and public
>>>>>>> datasheets), right? What would you prefer instead?
>>>>>> The numbers on the right of the - look good to me though then this isn't
>>>>>> a gain. (200kohm) and the infinite element is annoying. Why not
>>>>>> compute the actual gains?
>>>>>> Gain = (Rals*10e-6)/2 and use those values? Yes you will have to do
>>>>>> a bit of fixed point maths in the driver but the advantage is you'll
>>>>>> have real values that are standardizable across multiple devices
>>>>>> and hence allow your device to be operated by generic userspace
>>>>>> code. Welcome to standardising interfaces - my favourite occupation ;)
>>>>>>
>>>>>>>> Secondly see in_illuminance0_scale for a suitable existing attribute.
>>>>>>> I didn't consider scale to be appropriate given the following
>>>>>>> documentation (e.g, for in_voltageY_scale):
>>>>>> sorry I just did this to someone else in another review (so I'm
>>>>>> consistently wrong!)
>>>>>>
>>>>>> in_voltageY_calibscale is what I should have said. That one applies a
>>>>>> scaling before the raw reading is generated (so in hardware).
>>>>>
>>>>> Ok, then calibscale is the appropriate attribute for the resistor
>>>>> setting. But as this is a device-specific hardware-calibration setting
>>>>> I would suggest using the following interface:
>>>>>
>>>>> What: /sys/bus/iio/devices/iio:deviceX/in_illuminance_calibscale
>>>>> Description:
>>>>> Set the ALS calibration scale (internal resistors) for
>>>>> analog input mode, where the scale factor is the current in uA
>>>>> at 2V full-scale (10..1270, 10uA step), that is,
>>>>>
>>>>> R_als = 2V / in_illuminance_calibscale
>>>>>
>>>>> This setting is ignored in PWM mode.
>>>> This is a generic element that really ought to just fit in with the
>>>> equivalent in sysfs-bus-iio for calibscan. It's a ratio, so it should
>>>> be unit free for starters.
>>>
>>> I'm starting to doubt that calibscale is really appropriate in this case.
>>>
>>> For starters, the description in sysfs-bus-iio doesn't really apply:
>>>
>>> "Hardware applied calibration scale factor. (assumed to fix
>>> production inaccuracies)."
>> Hmm.. if you really don't like this, Michael Hennerich had a case
>> where this made even less sense, so we now have hardwaregain.
>> Use that if you like...
>
> I really think that this should remain a device specific attribute as I
> originally suggested. It's an integration parameter that needs to be set
> precisely depending on the actual hardware setup (which analog light
> sensor and other external components).
Then it shouldn't be exposed to userspace. If there is reason to vary
it from userspace then it is a calibration parameter and should be
treated like the other ones we have, if not it should be done from
dt or platform data.
>
> The lm3533 also supports two types of light sensors: pwm- and analog-
> output ones. The resistor select settings only applies when in analog
> mode as the input is always high impedance otherwise. Thus a generic
> attribute (such as calibscale or hardware gain) shouldn't be used as it
> will have no effect whatsoever in PWM-mode.
>
> I'm thus back at my original proposal, albeit with a different name (I
> think a lot of this discussion could have been avoided had I not
> misnamed the parameter "gain"):
>
> What: /sys/bus/iio/devices/iio:deviceX/r_select
> 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).
>
> I don't think much is gained from using ohm as the unit: it just adds
> complexity and the selected resistor setting will likely not match the
> input value anyway. It's better that the chip integrators have full
> control over which resistor setting is actually used so that it matches
> external components.
This smacks of something that should never be exposed to users.
I'd hide it away in platform data.
>
>
>>> The resistor setting of the lm3533 is about fitting an external analog
>>> light sensor to the lm3533 als interface (which is basically just an adc
>>> with some extra logic), that is, it is used to match the output current
>>> of the chosen sensor so that the ADC measures 2V at full LUX.
>>>
>>> It's not a setting to calibrate "inaccuracies", but rather an
>>> integration parameter that is set once when the characteristics of the
>>> light sensor is known. (Sure, it could be used later to increase
>>> sensitivity as well, but the main purpose is to fit a new light sensor
>>> to a generic input interface.)
>>>
>>>>> [...]
>>>>>
>>>>>>>>> +What: /sys/bus/iio/devices/iio:deviceX/target[m]_[n]
>>>>>>>>> +Date: April 2012
>>>>>>>>> +KernelVersion: 3.5
>>>>>>>>> +Contact: Johan Hovold<jhovold@gmail.com>
>>>>>>>>> +Description:
>>>>>>>>> + Set the target brightness for ALS-mapper m in light zone n
>>>>>>>>> + (0..255), where m in 1..3 and n in 0..4.
>>>>>>>> Don't suppose you could do a quick summary of what these zones are
>>>>>>>> and why there are 3 ALS-mappers? I'm not getting terribly far on a
>>>>>>>> quick look at the datasheet!
>>>>>>> Of course. The average adc readings are mapped to five light zones using
>>>>>>> eight zone boundary registers (4 boundaries with hysteresis) and a set
>>>>>>> of rules.
>>>>>> This is going to be fun. We'll need the boundaries and attached
>>>>>> hysteresis attributes to fully specify these (nothing would indicate
>>>>>> that hysterisis is involved otherwise).
>>>>>
>>>>> You can't define the hysteresis explicitly with the lm3533 register
>>>>> interface, rather it's is defined implicitly in case threshY_falling is
>>>>> less than threshY_rasising.
>>>>>
>>>>> So the raising/falling attributes should be enough, right?
>>>> Nope, because they don't tell a general userspace application what is
>>>> going on. Without hysterisis attributes it has no way of knowing there
>>>> is hysterisis present.
>>>
>>> Well an application could simply look at the difference between raising
>>> and falling to determine the hysteresis?
>> Only if it knows it has your sensor. For other sensors it could be
>> completely separate or not present. If the parameter is missing
>> assumption is that there is no hysterisis.
>>>
>>> It gets more complicated as the lm3533 allow the raising threshold to
>>> be lower than the falling. It appears the device is using whichever
>>> register is lower for the falling threshold. I guess I should compensate
>>> for this in the driver.
>> That's nasty.
>>>
>>> Furthermore, you can define threshold 1 to be lower than threshold 0,
>>> effectively preventing zone 1 to be reached. In this case, dropping
>>> below thres1_falling gives zone 0, and raising above thres1_raising gives
>>> zone 2. In particular, no threshold event is generated when
>>> thres0_{falling/raising} is passed in either direction. But perhaps this
>>> should just be documented as a feature/quirk of the device.
>> Seems sensible...
>>>
>>>> Feel free to make them read only though.
>>>
>>> So you're suggesting something like:
>>>
>>> events/in_illuminance0_threshY_falling_value
>>> events/in_illuminance0_threshY_raising_value
>>> events/in_illuminance0_threshY_hysteresis
>>>
>>> where hysteresis is a read-only attribute whose value is
>>>
>>> threshY_raising_value - threshY_falling_value
>> yes. Annoying it may be but it matches existing interface.
>
> I'm posting a v4 which includes the above proposal for resistor select.
> I've also added the hysteresis attributes as requested and fixed the
> device threshold quirkiness mentioned above (the device is using
> whichever register value is smaller as the falling threshold).
cool. I'll take a look soon.
>
> Thanks,
> Johan
^ permalink raw reply
* Re: [PATCH] OMAPDSS: DSI: Support command mode interleaving during video mode blanking periods
From: Tomi Valkeinen @ 2012-05-18 13:39 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap
In-Reply-To: <1337061738-32488-1-git-send-email-archit@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2381 bytes --]
Hi,
On Tue, 2012-05-15 at 11:32 +0530, Archit Taneja wrote:
> DSI supports interleaving of command mode packets during the HSA, HFP, HBP and
> BLLP blanking intervals in a video mode stream. This is useful as a user may
> want to read or change the configuration of a panel without stopping the video
> stream.
>
> On OMAP DSI, we can queue HS or LP command mode packets in the TX FIFO, and
> the DSI HW takes care of interleaving this data during the one of the blanking
> intervals. The DSI HW needs to be programmed with the maximum amount of data
> that can be interleaved in a particular blanking period. A blanking period
> cannot be used to send command mode data for it's complete duration, there is
> some amount of time required for the DSI data and clock lanes to transition
> to the desired LP or HS state.
>
> Based on the state of the lanes at the beginning and end of the blanking period,
> we have different scenarios, with each scenario having a different value of time
> required to transition to HS or LP. Refer to the section 'Interleaving Mode' in
> OMAP TRM for more info on the scenarios and the equations to calculate the time
> required for HS or LP transitions.
>
> We use the scenarios which takes the maximum time for HS or LP transition, this
> gives us the minimum amount of time that can be used to interleave command mode
> data. The amount of data that can be sent during this minimum time is calculated
> for command mode packets both in LP and HS. These are written to the registers
> DSI_VM_TIMING4 to DSI_VM_TIMING6.
>
> The calculations don't take into account the time required of transmitting BTA
> when doing a DSI read, or verifying if a DSI write went through correctly. Until
> these latencies aren't considered, the behaviour of DSI is unpredictable when
> a BTA is interleaved during a blanking period. Enhancement of these calculations
> is a TODO item.
The code in dsi_config_cmd_mode_interleaving() looks a bit long and
confusing, but I don't think it's trivial to simplify it. There's just
so many variables to consider.
In the future I think we should store the DSI configurations into
memory, so that we don't need to parse the hardware registers to find
out things like DSI timings.
I'll apply this, as it's very nice to have at least minimal interleaving
support.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/4] iio: add LM3533 ambient light sensor driver
From: Johan Hovold @ 2012-05-18 12:27 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Johan Hovold, Jonathan Cameron, Rob Landley, Richard Purdie,
Samuel Ortiz, Greg Kroah-Hartman, Florian Tobias Schandinat,
Arnd Bergmann, Andrew Morton, Mark Brown, linux-doc, linux-kernel,
linux-iio, devel, linux-fbdev
In-Reply-To: <4FB3B7DA.5050306@cam.ac.uk>
On Wed, May 16, 2012 at 03:21:14PM +0100, Jonathan Cameron wrote:
> On 5/16/2012 2:05 PM, Johan Hovold wrote:
> > On Tue, May 15, 2012 at 09:00:46PM +0100, Jonathan Cameron wrote:
> >> On 05/15/2012 05:44 PM, Johan Hovold wrote:
> >>> On Tue, May 08, 2012 at 02:47:19PM +0100, Jonathan Cameron wrote:
> >>>> On 5/3/2012 5:36 PM, Johan Hovold wrote:
> >>>>> On Thu, May 03, 2012 at 12:40:10PM +0100, Jonathan Cameron wrote:
> >>>>>> On 5/3/2012 11:26 AM, 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.
> >>>>>> Code is fine. Pretty much all my comments are to do with the interface.
> >>>>> [...]
> >>>>>
> >>>>>>> diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als
> >>>>>>> new file mode 100644
> >>>>>>> index 0000000..9849d14
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als
> >>>>>>> @@ -0,0 +1,62 @@
> >>>>>>> +What: /sys/bus/iio/devices/iio:deviceX/gain
> >>>>>>> +Date: April 2012
> >>>>>>> +KernelVersion: 3.5
> >>>>>>> +Contact: Johan Hovold<jhovold@gmail.com>
> >>>>>>> +Description:
> >>>>>>> + Set the ALS gain-resistor setting (0..127) for analog input
> >>>>>>> + mode, where
> >>>>>>> +
> >>>>>>> + 0000000 - ALS input is high impedance
> >>>>>>> + 0000001 - 200kOhm (10uA at 2V full-scale)
> >>>>>>> + 0000010 - 100kOhm (20uA at 2V full-scale)
> >>>>>>> + ...
> >>>>>>> + 1111110 - 1.587kOhm (1.26mA at 2V full-scale)
> >>>>>>> + 1111111 - 1.575kOhm (1.27mA at 2V full-scale)
> >>>>>>> +
> >>>>>>> + R_als = 2V / (10uA * gain) (gain> 0)
> >>>>>> Firstly, no magic numbers. These are definitely magic.
> >>>>> Not that magic as they're clearly documented (in code and public
> >>>>> datasheets), right? What would you prefer instead?
> >>>> The numbers on the right of the - look good to me though then this isn't
> >>>> a gain. (200kohm) and the infinite element is annoying. Why not
> >>>> compute the actual gains?
> >>>> Gain = (Rals*10e-6)/2 and use those values? Yes you will have to do
> >>>> a bit of fixed point maths in the driver but the advantage is you'll
> >>>> have real values that are standardizable across multiple devices
> >>>> and hence allow your device to be operated by generic userspace
> >>>> code. Welcome to standardising interfaces - my favourite occupation ;)
> >>>>
> >>>>>> Secondly see in_illuminance0_scale for a suitable existing attribute.
> >>>>> I didn't consider scale to be appropriate given the following
> >>>>> documentation (e.g, for in_voltageY_scale):
> >>>> sorry I just did this to someone else in another review (so I'm
> >>>> consistently wrong!)
> >>>>
> >>>> in_voltageY_calibscale is what I should have said. That one applies a
> >>>> scaling before the raw reading is generated (so in hardware).
> >>>
> >>> Ok, then calibscale is the appropriate attribute for the resistor
> >>> setting. But as this is a device-specific hardware-calibration setting
> >>> I would suggest using the following interface:
> >>>
> >>> What: /sys/bus/iio/devices/iio:deviceX/in_illuminance_calibscale
> >>> Description:
> >>> Set the ALS calibration scale (internal resistors) for
> >>> analog input mode, where the scale factor is the current in uA
> >>> at 2V full-scale (10..1270, 10uA step), that is,
> >>>
> >>> R_als = 2V / in_illuminance_calibscale
> >>>
> >>> This setting is ignored in PWM mode.
> >> This is a generic element that really ought to just fit in with the
> >> equivalent in sysfs-bus-iio for calibscan. It's a ratio, so it should
> >> be unit free for starters.
> >
> > I'm starting to doubt that calibscale is really appropriate in this case.
> >
> > For starters, the description in sysfs-bus-iio doesn't really apply:
> >
> > "Hardware applied calibration scale factor. (assumed to fix
> > production inaccuracies)."
> Hmm.. if you really don't like this, Michael Hennerich had a case
> where this made even less sense, so we now have hardwaregain.
> Use that if you like...
I really think that this should remain a device specific attribute as I
originally suggested. It's an integration parameter that needs to be set
precisely depending on the actual hardware setup (which analog light
sensor and other external components).
The lm3533 also supports two types of light sensors: pwm- and analog-
output ones. The resistor select settings only applies when in analog
mode as the input is always high impedance otherwise. Thus a generic
attribute (such as calibscale or hardware gain) shouldn't be used as it
will have no effect whatsoever in PWM-mode.
I'm thus back at my original proposal, albeit with a different name (I
think a lot of this discussion could have been avoided had I not
misnamed the parameter "gain"):
What: /sys/bus/iio/devices/iio:deviceX/r_select
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).
I don't think much is gained from using ohm as the unit: it just adds
complexity and the selected resistor setting will likely not match the
input value anyway. It's better that the chip integrators have full
control over which resistor setting is actually used so that it matches
external components.
> > The resistor setting of the lm3533 is about fitting an external analog
> > light sensor to the lm3533 als interface (which is basically just an adc
> > with some extra logic), that is, it is used to match the output current
> > of the chosen sensor so that the ADC measures 2V at full LUX.
> >
> > It's not a setting to calibrate "inaccuracies", but rather an
> > integration parameter that is set once when the characteristics of the
> > light sensor is known. (Sure, it could be used later to increase
> > sensitivity as well, but the main purpose is to fit a new light sensor
> > to a generic input interface.)
> >
> >>> [...]
> >>>
> >>>>>>> +What: /sys/bus/iio/devices/iio:deviceX/target[m]_[n]
> >>>>>>> +Date: April 2012
> >>>>>>> +KernelVersion: 3.5
> >>>>>>> +Contact: Johan Hovold<jhovold@gmail.com>
> >>>>>>> +Description:
> >>>>>>> + Set the target brightness for ALS-mapper m in light zone n
> >>>>>>> + (0..255), where m in 1..3 and n in 0..4.
> >>>>>> Don't suppose you could do a quick summary of what these zones are
> >>>>>> and why there are 3 ALS-mappers? I'm not getting terribly far on a
> >>>>>> quick look at the datasheet!
> >>>>> Of course. The average adc readings are mapped to five light zones using
> >>>>> eight zone boundary registers (4 boundaries with hysteresis) and a set
> >>>>> of rules.
> >>>> This is going to be fun. We'll need the boundaries and attached
> >>>> hysteresis attributes to fully specify these (nothing would indicate
> >>>> that hysterisis is involved otherwise).
> >>>
> >>> You can't define the hysteresis explicitly with the lm3533 register
> >>> interface, rather it's is defined implicitly in case threshY_falling is
> >>> less than threshY_rasising.
> >>>
> >>> So the raising/falling attributes should be enough, right?
> >> Nope, because they don't tell a general userspace application what is
> >> going on. Without hysterisis attributes it has no way of knowing there
> >> is hysterisis present.
> >
> > Well an application could simply look at the difference between raising
> > and falling to determine the hysteresis?
> Only if it knows it has your sensor. For other sensors it could be
> completely separate or not present. If the parameter is missing
> assumption is that there is no hysterisis.
> >
> > It gets more complicated as the lm3533 allow the raising threshold to
> > be lower than the falling. It appears the device is using whichever
> > register is lower for the falling threshold. I guess I should compensate
> > for this in the driver.
> That's nasty.
> >
> > Furthermore, you can define threshold 1 to be lower than threshold 0,
> > effectively preventing zone 1 to be reached. In this case, dropping
> > below thres1_falling gives zone 0, and raising above thres1_raising gives
> > zone 2. In particular, no threshold event is generated when
> > thres0_{falling/raising} is passed in either direction. But perhaps this
> > should just be documented as a feature/quirk of the device.
> Seems sensible...
> >
> >> Feel free to make them read only though.
> >
> > So you're suggesting something like:
> >
> > events/in_illuminance0_threshY_falling_value
> > events/in_illuminance0_threshY_raising_value
> > events/in_illuminance0_threshY_hysteresis
> >
> > where hysteresis is a read-only attribute whose value is
> >
> > threshY_raising_value - threshY_falling_value
> yes. Annoying it may be but it matches existing interface.
I'm posting a v4 which includes the above proposal for resistor select.
I've also added the hysteresis attributes as requested and fixed the
device threshold quirkiness mentioned above (the device is using
whichever register value is smaller as the falling threshold).
Thanks,
Johan
^ permalink raw reply
* [PATCH for v3.4] fbdev: sh_mobile_lcdc: Don't confuse line size with pitch
From: Laurent Pinchart @ 2012-05-18 8:58 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Florian Tobias Schandinat, LKML, linux-fbdev
When using the MERAM the LCDC line size needs to be programmed with a
MERAM-specific value different than the real frame buffer pitch. Fix it.
Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
---
drivers/video/sh_mobile_lcdcfb.c | 5 +++--
drivers/video/sh_mobile_lcdcfb.h | 1 +
2 files changed, 4 insertions(+), 2 deletions(-)
Hi Linus,
Could you please apply this patch to v3.4-rc ? I apologize for pushing it this
late, but this is a fix for a 3.4 regression that has only been discovered now
and breaks an important use case (fbcon on SH Mobile).
Due to the little time left before v3.4 Florian has asked me to send you the
patch directly instead of pushing it through his tree.
diff --git a/drivers/video/sh_mobile_lcdcfb.c b/drivers/video/sh_mobile_lcdcfb.c
index 7a0b301..e672698 100644
--- a/drivers/video/sh_mobile_lcdcfb.c
+++ b/drivers/video/sh_mobile_lcdcfb.c
@@ -758,7 +758,7 @@ static void __sh_mobile_lcdc_start(struct sh_mobile_lcdc_priv *priv)
}
lcdc_write_chan(ch, LDDFR, tmp);
- lcdc_write_chan(ch, LDMLSR, ch->pitch);
+ lcdc_write_chan(ch, LDMLSR, ch->line_size);
lcdc_write_chan(ch, LDSA1R, ch->base_addr_y);
if (ch->format->yuv)
lcdc_write_chan(ch, LDSA2R, ch->base_addr_c);
@@ -847,6 +847,7 @@ static int sh_mobile_lcdc_start(struct sh_mobile_lcdc_priv *priv)
ch->base_addr_y = ch->dma_handle;
ch->base_addr_c = ch->base_addr_y + ch->xres * ch->yres_virtual;
+ ch->line_size = ch->pitch;
/* Enable MERAM if possible. */
if (mdev = NULL || mdev->ops = NULL ||
@@ -882,7 +883,7 @@ static int sh_mobile_lcdc_start(struct sh_mobile_lcdc_priv *priv)
meram = mdev->ops->meram_register(mdev, ch->cfg->meram_cfg,
ch->pitch, ch->yres, pixelformat,
- &ch->pitch);
+ &ch->line_size);
if (!IS_ERR(meram)) {
mdev->ops->meram_update(mdev, meram,
ch->base_addr_y, ch->base_addr_c,
diff --git a/drivers/video/sh_mobile_lcdcfb.h b/drivers/video/sh_mobile_lcdcfb.h
index da1c26e..5c3bddd 100644
--- a/drivers/video/sh_mobile_lcdcfb.h
+++ b/drivers/video/sh_mobile_lcdcfb.h
@@ -84,6 +84,7 @@ struct sh_mobile_lcdc_chan {
unsigned long base_addr_y;
unsigned long base_addr_c;
+ unsigned int line_size;
int (*notify)(struct sh_mobile_lcdc_chan *ch,
enum sh_mobile_lcdc_entity_event event,
--
Regards,
Laurent Pinchart
^ permalink raw reply related
* Re: [PULL for v3.4] SH Mobile LCDC fix
From: Laurent Pinchart @ 2012-05-18 8:57 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <26224047.6PR9YZjWoc@avalon>
Hi Florian,
On Friday 18 May 2012 01:39:19 Florian Tobias Schandinat wrote:
> On 05/17/2012 10:23 PM, Laurent Pinchart wrote:
> > Hi Florian,
> >
> > Is it still possible for the following patch to go to v3.4 ? I've only
> > realized now that MERAM support was broken in some use cases (one of them
> > being fbcon) due to a v3.4-rc regression - thanks to Guennadi Liakhovetski
> > for reporting this.
>
> Umm, I don't like last minute stuff, but this is important enough. BUT...
I don't like it either, I'll add fbcon to my test cases for the future.
> > If this is too late for v3.4 please let me know and I'll CC stable on the
> > patch and resend a pull request for v3.5.
> >
> > The following changes since commit
5e7b911f9a3e582635801675b7fe935b16cd4af5:
> > drivers/video: fsl-diu-fb: don't initialize the THRESHOLDS registers
> >
> > (2012-05-13 13:08:56 +0000)
>
> ...how am I supposed to merge this? It's based on top of all the stuff
> that I plan to ask Linus to pull during the merge window and AFAIK there
> is no way to merge only upper parts of branches. I could cherry-pick it
> but that wouldn't look like a merge or a patch I applied.
Oops, my bad, sorry.
> As this is only a single patch and not much time left, I'd suggest you
> send it directly to Linus (as a patch) with my Ack added to it. Oh and
> make sure to mention that this fixes a regression.
Sure, I'll do that.
Thank you.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PULL for v3.4] SH Mobile LCDC fix
From: Florian Tobias Schandinat @ 2012-05-18 1:39 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <26224047.6PR9YZjWoc@avalon>
Hi Laurent,
On 05/17/2012 10:23 PM, Laurent Pinchart wrote:
> Hi Florian,
>
> Is it still possible for the following patch to go to v3.4 ? I've only
> realized now that MERAM support was broken in some use cases (one of them
> being fbcon) due to a v3.4-rc regression - thanks to Guennadi Liakhovetski for
> reporting this.
Umm, I don't like last minute stuff, but this is important enough. BUT...
>
> If this is too late for v3.4 please let me know and I'll CC stable on the
> patch and resend a pull request for v3.5.
>
> The following changes since commit 5e7b911f9a3e582635801675b7fe935b16cd4af5:
>
> drivers/video: fsl-diu-fb: don't initialize the THRESHOLDS registers
> (2012-05-13 13:08:56 +0000)
...how am I supposed to merge this? It's based on top of all the stuff
that I plan to ask Linus to pull during the merge window and AFAIK there
is no way to merge only upper parts of branches. I could cherry-pick it
but that wouldn't look like a merge or a patch I applied.
As this is only a single patch and not much time left, I'd suggest you
send it directly to Linus (as a patch) with my Ack added to it. Oh and
make sure to mention that this fixes a regression.
Thanks,
Florian Tobias Schandinat
>
> are available in the git repository at:
> git://linuxtv.org/pinchartl/fbdev.git fixes
>
> Laurent Pinchart (1):
> fbdev: sh_mobile_lcdc: Don't confuse line size with pitch
>
> drivers/video/sh_mobile_lcdcfb.c | 5 +++--
> drivers/video/sh_mobile_lcdcfb.h | 1 +
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
^ permalink raw reply
* [PULL for v3.4] SH Mobile LCDC fix
From: Laurent Pinchart @ 2012-05-17 22:23 UTC (permalink / raw)
To: linux-fbdev
Hi Florian,
Is it still possible for the following patch to go to v3.4 ? I've only
realized now that MERAM support was broken in some use cases (one of them
being fbcon) due to a v3.4-rc regression - thanks to Guennadi Liakhovetski for
reporting this.
If this is too late for v3.4 please let me know and I'll CC stable on the
patch and resend a pull request for v3.5.
The following changes since commit 5e7b911f9a3e582635801675b7fe935b16cd4af5:
drivers/video: fsl-diu-fb: don't initialize the THRESHOLDS registers
(2012-05-13 13:08:56 +0000)
are available in the git repository at:
git://linuxtv.org/pinchartl/fbdev.git fixes
Laurent Pinchart (1):
fbdev: sh_mobile_lcdc: Don't confuse line size with pitch
drivers/video/sh_mobile_lcdcfb.c | 5 +++--
drivers/video/sh_mobile_lcdcfb.h | 1 +
2 files changed, 4 insertions(+), 2 deletions(-)
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: Kernel Display and Video API Consolidation mini-summit at ELC 2012 - Notes
From: Hans Verkuil @ 2012-05-17 7:53 UTC (permalink / raw)
To: Jun Nie
Cc: Guennadi Liakhovetski, Laurent Pinchart, Daniel Vetter,
Tomasz Stanislawski, linux-fbdev, Sakari Ailus, Pawel Osciak,
Magnus Damm, Marcus Lorentzon, dri-devel, Alexander Deucher,
Rob Clark, linux-media, Marek Szyprowski
In-Reply-To: <CAGA24M+OEwi-ayBrXcMPg5PzndRF4mSr2dOOQAxhDCu6ShZLjQ@mail.gmail.com>
On Thu May 17 2012 04:46:37 Jun Nie wrote:
> Is there any discussion on HDCP on the summit? It is tightly
> coupled with HDMI and DVI and should be managed together with the
> transmitter. But there is not code to handle HDCP in DRM/FB/V4L in
> latest kernel. Any thoughts on HDCP? Or you guys think there is risk
> to support it in kernel? Thanks for your comments!
There is no risk to support it in the kernel, the risk is all for the
implementer (usually by having to lock down the system preventing access
to the box). You'd better read the HDCP license very carefully before deciding
to use HDCP under linux!
I'm working on V4L HDMI receivers and transmitters myself, but not on HDCP.
But I'd be happy to review/comment on proposals for adding HDCP support.
Note that there is very little work to be done to add this for simple
receivers and transmitters. The hard part will be supporting repeaters.
For simple receivers all you need in V4L2 is a flag telling you that the
received video was encrypted and for a transmitter I think you just need a
control to turn encryption on or off (AFAIK, I'd have to verify that statement
regarding the transmitter to be 100% certain). All the actual encryption and
decryption is handled by the receiver/transmitter hardware, at least on the
hardware that I have seen.
Repeaters are a lot harder as you have to handle key exchanges. I don't know
off-hand what that would involve API-wise in V4L2.
Regards,
Hans
^ permalink raw reply
* Re: Kernel Display and Video API Consolidation mini-summit at ELC 2012 - Notes
From: Jun Nie @ 2012-05-17 2:46 UTC (permalink / raw)
To: Guennadi Liakhovetski, Laurent Pinchart
Cc: Daniel Vetter, Tomasz Stanislawski, linux-fbdev, Sakari Ailus,
Pawel Osciak, Magnus Damm, Marcus Lorentzon, dri-devel,
Alexander Deucher, Rob Clark, linux-media, Marek Szyprowski
In-Reply-To: <Pine.LNX.4.64.1202201633100.2836@axis700.grange>
Is there any discussion on HDCP on the summit? It is tightly
coupled with HDMI and DVI and should be managed together with the
transmitter. But there is not code to handle HDCP in DRM/FB/V4L in
latest kernel. Any thoughts on HDCP? Or you guys think there is risk
to support it in kernel? Thanks for your comments!
Jun
^ permalink raw reply
* Re: [PATCH v2 2/4] iio: add LM3533 ambient light sensor driver
From: Jonathan Cameron @ 2012-05-16 14:21 UTC (permalink / raw)
To: Johan Hovold
Cc: Jonathan Cameron, Rob Landley, Richard Purdie, Samuel Ortiz,
Greg Kroah-Hartman, Florian Tobias Schandinat, Arnd Bergmann,
Andrew Morton, Mark Brown, linux-doc, linux-kernel, linux-iio,
devel, linux-fbdev
In-Reply-To: <20120516130507.GA10774@localhost>
On 5/16/2012 2:05 PM, Johan Hovold wrote:
> On Tue, May 15, 2012 at 09:00:46PM +0100, Jonathan Cameron wrote:
>> On 05/15/2012 05:44 PM, Johan Hovold wrote:
>>> On Tue, May 08, 2012 at 02:47:19PM +0100, Jonathan Cameron wrote:
>>>> On 5/3/2012 5:36 PM, Johan Hovold wrote:
>>>>> On Thu, May 03, 2012 at 12:40:10PM +0100, Jonathan Cameron wrote:
>>>>>> On 5/3/2012 11:26 AM, 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.
>>>>>> Code is fine. Pretty much all my comments are to do with the interface.
>>>>> [...]
>>>>>
>>>>>>> diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als
>>>>>>> new file mode 100644
>>>>>>> index 0000000..9849d14
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als
>>>>>>> @@ -0,0 +1,62 @@
>>>>>>> +What: /sys/bus/iio/devices/iio:deviceX/gain
>>>>>>> +Date: April 2012
>>>>>>> +KernelVersion: 3.5
>>>>>>> +Contact: Johan Hovold<jhovold@gmail.com>
>>>>>>> +Description:
>>>>>>> + Set the ALS gain-resistor setting (0..127) for analog input
>>>>>>> + mode, where
>>>>>>> +
>>>>>>> + 0000000 - ALS input is high impedance
>>>>>>> + 0000001 - 200kOhm (10uA at 2V full-scale)
>>>>>>> + 0000010 - 100kOhm (20uA at 2V full-scale)
>>>>>>> + ...
>>>>>>> + 1111110 - 1.587kOhm (1.26mA at 2V full-scale)
>>>>>>> + 1111111 - 1.575kOhm (1.27mA at 2V full-scale)
>>>>>>> +
>>>>>>> + R_als = 2V / (10uA * gain) (gain> 0)
>>>>>> Firstly, no magic numbers. These are definitely magic.
>>>>> Not that magic as they're clearly documented (in code and public
>>>>> datasheets), right? What would you prefer instead?
>>>> The numbers on the right of the - look good to me though then this isn't
>>>> a gain. (200kohm) and the infinite element is annoying. Why not
>>>> compute the actual gains?
>>>> Gain = (Rals*10e-6)/2 and use those values? Yes you will have to do
>>>> a bit of fixed point maths in the driver but the advantage is you'll
>>>> have real values that are standardizable across multiple devices
>>>> and hence allow your device to be operated by generic userspace
>>>> code. Welcome to standardising interfaces - my favourite occupation ;)
>>>>
>>>>>> Secondly see in_illuminance0_scale for a suitable existing attribute.
>>>>> I didn't consider scale to be appropriate given the following
>>>>> documentation (e.g, for in_voltageY_scale):
>>>> sorry I just did this to someone else in another review (so I'm
>>>> consistently wrong!)
>>>>
>>>> in_voltageY_calibscale is what I should have said. That one applies a
>>>> scaling before the raw reading is generated (so in hardware).
>>>
>>> Ok, then calibscale is the appropriate attribute for the resistor
>>> setting. But as this is a device-specific hardware-calibration setting
>>> I would suggest using the following interface:
>>>
>>> What: /sys/bus/iio/devices/iio:deviceX/in_illuminance_calibscale
>>> Description:
>>> Set the ALS calibration scale (internal resistors) for
>>> analog input mode, where the scale factor is the current in uA
>>> at 2V full-scale (10..1270, 10uA step), that is,
>>>
>>> R_als = 2V / in_illuminance_calibscale
>>>
>>> This setting is ignored in PWM mode.
>> This is a generic element that really ought to just fit in with the
>> equivalent in sysfs-bus-iio for calibscan. It's a ratio, so it should
>> be unit free for starters.
>
> I'm starting to doubt that calibscale is really appropriate in this case.
>
> For starters, the description in sysfs-bus-iio doesn't really apply:
>
> "Hardware applied calibration scale factor. (assumed to fix
> production inaccuracies)."
Hmm.. if you really don't like this, Michael Hennerich had a case
where this made even less sense, so we now have hardwaregain.
Use that if you like...
>
> The resistor setting of the lm3533 is about fitting an external analog
> light sensor to the lm3533 als interface (which is basically just an adc
> with some extra logic), that is, it is used to match the output current
> of the chosen sensor so that the ADC measures 2V at full LUX.
>
> It's not a setting to calibrate "inaccuracies", but rather an
> integration parameter that is set once when the characteristics of the
> light sensor is known. (Sure, it could be used later to increase
> sensitivity as well, but the main purpose is to fit a new light sensor
> to a generic input interface.)
>
>>> [...]
>>>
>>>>>>> +What: /sys/bus/iio/devices/iio:deviceX/target[m]_[n]
>>>>>>> +Date: April 2012
>>>>>>> +KernelVersion: 3.5
>>>>>>> +Contact: Johan Hovold<jhovold@gmail.com>
>>>>>>> +Description:
>>>>>>> + Set the target brightness for ALS-mapper m in light zone n
>>>>>>> + (0..255), where m in 1..3 and n in 0..4.
>>>>>> Don't suppose you could do a quick summary of what these zones are
>>>>>> and why there are 3 ALS-mappers? I'm not getting terribly far on a
>>>>>> quick look at the datasheet!
>>>>> Of course. The average adc readings are mapped to five light zones using
>>>>> eight zone boundary registers (4 boundaries with hysteresis) and a set
>>>>> of rules.
>>>> This is going to be fun. We'll need the boundaries and attached
>>>> hysteresis attributes to fully specify these (nothing would indicate
>>>> that hysterisis is involved otherwise).
>>>
>>> You can't define the hysteresis explicitly with the lm3533 register
>>> interface, rather it's is defined implicitly in case threshY_falling is
>>> less than threshY_rasising.
>>>
>>> So the raising/falling attributes should be enough, right?
>> Nope, because they don't tell a general userspace application what is
>> going on. Without hysterisis attributes it has no way of knowing there
>> is hysterisis present.
>
> Well an application could simply look at the difference between raising
> and falling to determine the hysteresis?
Only if it knows it has your sensor. For other sensors it could be
completely separate or not present. If the parameter is missing
assumption is that there is no hysterisis.
>
> It gets more complicated as the lm3533 allow the raising threshold to
> be lower than the falling. It appears the device is using whichever
> register is lower for the falling threshold. I guess I should compensate
> for this in the driver.
That's nasty.
>
> Furthermore, you can define threshold 1 to be lower than threshold 0,
> effectively preventing zone 1 to be reached. In this case, dropping
> below thres1_falling gives zone 0, and raising above thres1_raising gives
> zone 2. In particular, no threshold event is generated when
> thres0_{falling/raising} is passed in either direction. But perhaps this
> should just be documented as a feature/quirk of the device.
Seems sensible...
>
>> Feel free to make them read only though.
>
> So you're suggesting something like:
>
> events/in_illuminance0_threshY_falling_value
> events/in_illuminance0_threshY_raising_value
> events/in_illuminance0_threshY_hysteresis
>
> where hysteresis is a read-only attribute whose value is
>
> threshY_raising_value - threshY_falling_value
yes. Annoying it may be but it matches existing interface.
>
>>>>> To simplify somewhat (by ignoring some of the rules): If the average
>>>>> adc input drops below boundary0_low, the zone register reads 0; if it
>>>>> drops below boundary1_low, it reads 1, and so on. If the input it
>>>>> increases over boundary3_high, the zone register return 4; if it
>>>>> increases passed boundary2_high, it returns zone 3, etc.
>>>>>
>>>>> That is, roughly something like (we get 8-bits of input from the ADC):
>>>>>
>>>>> zone 0
>>>>>
>>>>> boundary0_low 51
>>>>> boundary0_high 53
>>>>>
>>>>> zone 1
>>>>>
>>>>> boundary1_low 102
>>>>> boundary1_high 106
>>>>>
>>>>> zone 2
>>>>>
>>>>> boundary2_low 153
>>>>> boundary2_high 161
>>>>>
>>>>> zone 3
>>>>>
>>>>> boundary3_low 204
>>>>> boundary3_high 220
>>>>>
>>>>> zone 4
>>>>>
>>>>> [ Figure 6 on page 20 in the datasheets should make it clear. ]
>>>>>
>>>>> The ALS interface and it's zone concept can then be used to control the
>>>>> LEDs and backlights of the chip, by determining the target brightness for
>>>>> each zone, e.g., set brightness to 52 when in zone 0.
>>>>>
>>>>> To complicate things further (and it is complicated), there are three
>>>>> such sets of target brightness values: ALSM1, ALSM2, ALSM3.
>>>>>
>>>>> So for each LED or backlight you can set ALS-input control mode, by
>>>>> saying that the device should get it's brightness levels from target set
>>>>> 1, 2, or 3.
>>>>>
>>>>> [ And it gets even more complicated, as ALSM1 can only control
>>>>> backlight0, where as ALSM2 and ALSM3 can control any of the remaining
>>>>> devices, but that's irrelevant here. ]
>>>>>
>>>>> Initially, I thought this interface to be too esoteric to be worth
>>>>> generalising, but it sort of fits with event thresholds so I gave it a
>>>>> try.
>>>> Glad you did and it pretty much fits, be it with a few extensions being
>>>> necessary.
>>>>> The biggest conceptual problem, I think, is that the zone
>>>>> boundaries can be used to control the other devices, even when the event
>>>>> is not enabled (or even an irq line not configured). That is, I find it
>>>>> a bit awkward that the event thresholds also defines the zones (a sort of
>>>>> discrete scaling factor).
>>>> That is indeed awkward. I'm not sure how we handle this either. If we
>>>> need to control these from the other devices (e.g. the back light
>>>> driver) then we'll have to get them into chan_spec and use the
>>>> inkernel interfaces to do it. Not infeasible but I was hoping to
>>>> avoid that until we have had a few months to see what similar devices
>>>> show up (on basis nothing in this world is a one off for long ;)
>>>
>>> I don't think the control bits can or should be generalised at this
>>> point. The same ALS-target values may be used to control more than one
>>> device, so they need to be set from the als rather from the controlled
>>> device (otherwise, changing the target value of led1 could change that
>>> of the other three leds without the user realising that this can be a
>>> side effect).
>> Good point. Nasty little device to write an interface for :)
>
> Indeed. Thanks for appreciating that. ;)
>
>>>>> Perhaps simply keeping the attributes outside of events (e.g. named
>>>>> boundary[n]_{low,high}) and having a custom event enabled (e.g.
>>>>> in_illuminance_zone_change_en) is the best solution?
>>>> Maybe, but it's ugly and as you have said, they do correspond pretty well to
>>>> thresholds so I'd rather you went with that.
>>>> The core stuff for registering events clearly needs a rethink.... For
>>>> now doing it as you describe above (with the addition fo hysteresis
>>>> attributes) should be fine. Just document the 'quirks'.
>>>
>>> Ok, I'll keep the event/zone interface as it stands for now and we'll
>>> see if it can be generalised later. [ See my comment on the hysteresis
>>> above: there are only the rising/falling thresholds (low/high
>>> boundaries) and no boundary or hysteresis settings. ]
>> On that, just to reiterate, to have anything generalizable, userspace
>> needs to know that hysterisis exists on the individual thresholds
>> (though it is clearly a function of the neighbouring one).
>
> See my comments above.
>
> Thanks,
> Johan
^ permalink raw reply
* Re: [PATCH v2 2/4] iio: add LM3533 ambient light sensor driver
From: Johan Hovold @ 2012-05-16 13:05 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Johan Hovold, Jonathan Cameron, Rob Landley, Richard Purdie,
Samuel Ortiz, Greg Kroah-Hartman, Florian Tobias Schandinat,
Arnd Bergmann, Andrew Morton, Mark Brown, linux-doc, linux-kernel,
linux-iio, devel, linux-fbdev
In-Reply-To: <4FB2B5EE.6080009@kernel.org>
On Tue, May 15, 2012 at 09:00:46PM +0100, Jonathan Cameron wrote:
> On 05/15/2012 05:44 PM, Johan Hovold wrote:
> > On Tue, May 08, 2012 at 02:47:19PM +0100, Jonathan Cameron wrote:
> >> On 5/3/2012 5:36 PM, Johan Hovold wrote:
> >>> On Thu, May 03, 2012 at 12:40:10PM +0100, Jonathan Cameron wrote:
> >>>> On 5/3/2012 11:26 AM, 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.
> >>>> Code is fine. Pretty much all my comments are to do with the interface.
> >>> [...]
> >>>
> >>>>> diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als
> >>>>> new file mode 100644
> >>>>> index 0000000..9849d14
> >>>>> --- /dev/null
> >>>>> +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als
> >>>>> @@ -0,0 +1,62 @@
> >>>>> +What: /sys/bus/iio/devices/iio:deviceX/gain
> >>>>> +Date: April 2012
> >>>>> +KernelVersion: 3.5
> >>>>> +Contact: Johan Hovold<jhovold@gmail.com>
> >>>>> +Description:
> >>>>> + Set the ALS gain-resistor setting (0..127) for analog input
> >>>>> + mode, where
> >>>>> +
> >>>>> + 0000000 - ALS input is high impedance
> >>>>> + 0000001 - 200kOhm (10uA at 2V full-scale)
> >>>>> + 0000010 - 100kOhm (20uA at 2V full-scale)
> >>>>> + ...
> >>>>> + 1111110 - 1.587kOhm (1.26mA at 2V full-scale)
> >>>>> + 1111111 - 1.575kOhm (1.27mA at 2V full-scale)
> >>>>> +
> >>>>> + R_als = 2V / (10uA * gain) (gain> 0)
> >>>> Firstly, no magic numbers. These are definitely magic.
> >>> Not that magic as they're clearly documented (in code and public
> >>> datasheets), right? What would you prefer instead?
> >> The numbers on the right of the - look good to me though then this isn't
> >> a gain. (200kohm) and the infinite element is annoying. Why not
> >> compute the actual gains?
> >> Gain = (Rals*10e-6)/2 and use those values? Yes you will have to do
> >> a bit of fixed point maths in the driver but the advantage is you'll
> >> have real values that are standardizable across multiple devices
> >> and hence allow your device to be operated by generic userspace
> >> code. Welcome to standardising interfaces - my favourite occupation ;)
> >>
> >>>> Secondly see in_illuminance0_scale for a suitable existing attribute.
> >>> I didn't consider scale to be appropriate given the following
> >>> documentation (e.g, for in_voltageY_scale):
> >> sorry I just did this to someone else in another review (so I'm
> >> consistently wrong!)
> >>
> >> in_voltageY_calibscale is what I should have said. That one applies a
> >> scaling before the raw reading is generated (so in hardware).
> >
> > Ok, then calibscale is the appropriate attribute for the resistor
> > setting. But as this is a device-specific hardware-calibration setting
> > I would suggest using the following interface:
> >
> > What: /sys/bus/iio/devices/iio:deviceX/in_illuminance_calibscale
> > Description:
> > Set the ALS calibration scale (internal resistors) for
> > analog input mode, where the scale factor is the current in uA
> > at 2V full-scale (10..1270, 10uA step), that is,
> >
> > R_als = 2V / in_illuminance_calibscale
> >
> > This setting is ignored in PWM mode.
> This is a generic element that really ought to just fit in with the
> equivalent in sysfs-bus-iio for calibscan. It's a ratio, so it should
> be unit free for starters.
I'm starting to doubt that calibscale is really appropriate in this case.
For starters, the description in sysfs-bus-iio doesn't really apply:
"Hardware applied calibration scale factor. (assumed to fix
production inaccuracies)."
The resistor setting of the lm3533 is about fitting an external analog
light sensor to the lm3533 als interface (which is basically just an adc
with some extra logic), that is, it is used to match the output current
of the chosen sensor so that the ADC measures 2V at full LUX.
It's not a setting to calibrate "inaccuracies", but rather an
integration parameter that is set once when the characteristics of the
light sensor is known. (Sure, it could be used later to increase
sensitivity as well, but the main purpose is to fit a new light sensor
to a generic input interface.)
> > [...]
> >
> >>>>> +What: /sys/bus/iio/devices/iio:deviceX/target[m]_[n]
> >>>>> +Date: April 2012
> >>>>> +KernelVersion: 3.5
> >>>>> +Contact: Johan Hovold<jhovold@gmail.com>
> >>>>> +Description:
> >>>>> + Set the target brightness for ALS-mapper m in light zone n
> >>>>> + (0..255), where m in 1..3 and n in 0..4.
> >>>> Don't suppose you could do a quick summary of what these zones are
> >>>> and why there are 3 ALS-mappers? I'm not getting terribly far on a
> >>>> quick look at the datasheet!
> >>> Of course. The average adc readings are mapped to five light zones using
> >>> eight zone boundary registers (4 boundaries with hysteresis) and a set
> >>> of rules.
> >> This is going to be fun. We'll need the boundaries and attached
> >> hysteresis attributes to fully specify these (nothing would indicate
> >> that hysterisis is involved otherwise).
> >
> > You can't define the hysteresis explicitly with the lm3533 register
> > interface, rather it's is defined implicitly in case threshY_falling is
> > less than threshY_rasising.
> >
> > So the raising/falling attributes should be enough, right?
> Nope, because they don't tell a general userspace application what is
> going on. Without hysterisis attributes it has no way of knowing there
> is hysterisis present.
Well an application could simply look at the difference between raising
and falling to determine the hysteresis?
It gets more complicated as the lm3533 allow the raising threshold to
be lower than the falling. It appears the device is using whichever
register is lower for the falling threshold. I guess I should compensate
for this in the driver.
Furthermore, you can define threshold 1 to be lower than threshold 0,
effectively preventing zone 1 to be reached. In this case, dropping
below thres1_falling gives zone 0, and raising above thres1_raising gives
zone 2. In particular, no threshold event is generated when
thres0_{falling/raising} is passed in either direction. But perhaps this
should just be documented as a feature/quirk of the device.
> Feel free to make them read only though.
So you're suggesting something like:
events/in_illuminance0_threshY_falling_value
events/in_illuminance0_threshY_raising_value
events/in_illuminance0_threshY_hysteresis
where hysteresis is a read-only attribute whose value is
threshY_raising_value - threshY_falling_value
> >>> To simplify somewhat (by ignoring some of the rules): If the average
> >>> adc input drops below boundary0_low, the zone register reads 0; if it
> >>> drops below boundary1_low, it reads 1, and so on. If the input it
> >>> increases over boundary3_high, the zone register return 4; if it
> >>> increases passed boundary2_high, it returns zone 3, etc.
> >>>
> >>> That is, roughly something like (we get 8-bits of input from the ADC):
> >>>
> >>> zone 0
> >>>
> >>> boundary0_low 51
> >>> boundary0_high 53
> >>>
> >>> zone 1
> >>>
> >>> boundary1_low 102
> >>> boundary1_high 106
> >>>
> >>> zone 2
> >>>
> >>> boundary2_low 153
> >>> boundary2_high 161
> >>>
> >>> zone 3
> >>>
> >>> boundary3_low 204
> >>> boundary3_high 220
> >>>
> >>> zone 4
> >>>
> >>> [ Figure 6 on page 20 in the datasheets should make it clear. ]
> >>>
> >>> The ALS interface and it's zone concept can then be used to control the
> >>> LEDs and backlights of the chip, by determining the target brightness for
> >>> each zone, e.g., set brightness to 52 when in zone 0.
> >>>
> >>> To complicate things further (and it is complicated), there are three
> >>> such sets of target brightness values: ALSM1, ALSM2, ALSM3.
> >>>
> >>> So for each LED or backlight you can set ALS-input control mode, by
> >>> saying that the device should get it's brightness levels from target set
> >>> 1, 2, or 3.
> >>>
> >>> [ And it gets even more complicated, as ALSM1 can only control
> >>> backlight0, where as ALSM2 and ALSM3 can control any of the remaining
> >>> devices, but that's irrelevant here. ]
> >>>
> >>> Initially, I thought this interface to be too esoteric to be worth
> >>> generalising, but it sort of fits with event thresholds so I gave it a
> >>> try.
> >> Glad you did and it pretty much fits, be it with a few extensions being
> >> necessary.
> >>> The biggest conceptual problem, I think, is that the zone
> >>> boundaries can be used to control the other devices, even when the event
> >>> is not enabled (or even an irq line not configured). That is, I find it
> >>> a bit awkward that the event thresholds also defines the zones (a sort of
> >>> discrete scaling factor).
> >> That is indeed awkward. I'm not sure how we handle this either. If we
> >> need to control these from the other devices (e.g. the back light
> >> driver) then we'll have to get them into chan_spec and use the
> >> inkernel interfaces to do it. Not infeasible but I was hoping to
> >> avoid that until we have had a few months to see what similar devices
> >> show up (on basis nothing in this world is a one off for long ;)
> >
> > I don't think the control bits can or should be generalised at this
> > point. The same ALS-target values may be used to control more than one
> > device, so they need to be set from the als rather from the controlled
> > device (otherwise, changing the target value of led1 could change that
> > of the other three leds without the user realising that this can be a
> > side effect).
> Good point. Nasty little device to write an interface for :)
Indeed. Thanks for appreciating that. ;)
> >>> Perhaps simply keeping the attributes outside of events (e.g. named
> >>> boundary[n]_{low,high}) and having a custom event enabled (e.g.
> >>> in_illuminance_zone_change_en) is the best solution?
> >> Maybe, but it's ugly and as you have said, they do correspond pretty well to
> >> thresholds so I'd rather you went with that.
> >> The core stuff for registering events clearly needs a rethink.... For
> >> now doing it as you describe above (with the addition fo hysteresis
> >> attributes) should be fine. Just document the 'quirks'.
> >
> > Ok, I'll keep the event/zone interface as it stands for now and we'll
> > see if it can be generalised later. [ See my comment on the hysteresis
> > above: there are only the rising/falling thresholds (low/high
> > boundaries) and no boundary or hysteresis settings. ]
> On that, just to reiterate, to have anything generalizable, userspace
> needs to know that hysterisis exists on the individual thresholds
> (though it is clearly a function of the neighbouring one).
See my comments above.
Thanks,
Johan
^ permalink raw reply
* Re: [PATCH] mbxfb: unbreak compilation with CONFIG_FB_MBX_DEBUG
From: Eric Miao @ 2012-05-16 0:20 UTC (permalink / raw)
To: tartler
Cc: Florian Tobias Schandinat, Arnd Bergmann, linux-fbdev,
linux-kernel, vamos-dev, Reinhard Tartler
In-Reply-To: <1337013111-7732-1-git-send-email-tartler@cs.fau.de>
On Tue, May 15, 2012 at 12:31 AM, <tartler@cs.fau.de> wrote:
> From: Reinhard Tartler <siretart@tauware.de>
>
> This patch adds missing function prototypes.
>
> Signed-off-by: Reinhard Tartler <tartler@cs.fau.de>
> ---
> drivers/video/mbx/mbxfb.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> This patch was found with tools developed in the VAMOS project:
> http://www4.cs.fau.de/Research/VAMOS/
>
> TBH, I'm not sure if this is the correct solution. However, I'd
> appreciate if someone could confirm that this is a real bug.
This looks to be a correct solution to me.
Acked-by: Eric Miao <eric.y.miao@gmail.com>
>
>
> diff --git a/drivers/video/mbx/mbxfb.c b/drivers/video/mbx/mbxfb.c
> index 6ce3416..c2200ec 100644
> --- a/drivers/video/mbx/mbxfb.c
> +++ b/drivers/video/mbx/mbxfb.c
> @@ -878,6 +878,9 @@ static int mbxfb_resume(struct platform_device *dev)
> #ifndef CONFIG_FB_MBX_DEBUG
> #define mbxfb_debugfs_init(x) do {} while(0)
> #define mbxfb_debugfs_remove(x) do {} while(0)
> +#else
> +void mbxfb_debugfs_init(struct fb_info *fbi);
> +void mbxfb_debugfs_remove(struct fb_info *fbi);
> #endif
>
> #define res_size(_r) (((_r)->end - (_r)->start) + 1)
> --
> 1.7.9.5
>
^ permalink raw reply
* [PATCH 5/5] drivers/tosa: driver needs I2C and SPI to compile
From: mathieu.poirier @ 2012-05-15 21:03 UTC (permalink / raw)
To: linux-fbdev; +Cc: linux-kernel, arnd, mathieu.poirier
In-Reply-To: <1337115781-27979-1-git-send-email-mathieu.poirier@linaro.org>
From: Arnd Bergmann <arnd@arndb.de>
This driver requires both SPI and I2C to build.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
drivers/video/backlight/Kconfig | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index af16884..d7487d2 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -88,7 +88,7 @@ config LCD_PLATFORM
config LCD_TOSA
tristate "Sharp SL-6000 LCD Driver"
- depends on SPI && MACH_TOSA
+ depends on I2C && SPI && MACH_TOSA
help
If you have an Sharp SL-6000 Zaurus say Y to enable a driver
for its LCD.
--
1.7.5.4
^ permalink raw reply related
* [PATCH 4/5] drivers/savagefb: use mdelay instead of udelay
From: mathieu.poirier @ 2012-05-15 21:03 UTC (permalink / raw)
To: linux-fbdev; +Cc: linux-kernel, arnd, mathieu.poirier
In-Reply-To: <1337115781-27979-1-git-send-email-mathieu.poirier@linaro.org>
From: Arnd Bergmann <arnd@arndb.de>
Long delays need to use mdelay on architectures such as ARM
that cannot compute long timeouts in udelay.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
drivers/video/savage/savagefb_driver.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/video/savage/savagefb_driver.c b/drivers/video/savage/savagefb_driver.c
index cee7803..f3d3b9c 100644
--- a/drivers/video/savage/savagefb_driver.c
+++ b/drivers/video/savage/savagefb_driver.c
@@ -1351,7 +1351,7 @@ static void savagefb_set_par_int(struct savagefb_par *par, struct savage_reg *r
/* following part not present in X11 driver */
cr67 = vga_in8(0x3d5, par) & 0xf;
vga_out8(0x3d5, 0x50 | cr67, par);
- udelay(10000);
+ mdelay(10);
vga_out8(0x3d4, 0x67, par);
/* end of part */
vga_out8(0x3d5, reg->CR67 & ~0x0c, par);
@@ -1904,11 +1904,11 @@ static int savage_init_hw(struct savagefb_par *par)
vga_out8(0x3d4, 0x66, par);
cr66 = vga_in8(0x3d5, par);
vga_out8(0x3d5, cr66 | 0x02, par);
- udelay(10000);
+ mdelay(10);
vga_out8(0x3d4, 0x66, par);
vga_out8(0x3d5, cr66 & ~0x02, par); /* clear reset flag */
- udelay(10000);
+ mdelay(10);
/*
@@ -1918,11 +1918,11 @@ static int savage_init_hw(struct savagefb_par *par)
vga_out8(0x3d4, 0x3f, par);
cr3f = vga_in8(0x3d5, par);
vga_out8(0x3d5, cr3f | 0x08, par);
- udelay(10000);
+ mdelay(10);
vga_out8(0x3d4, 0x3f, par);
vga_out8(0x3d5, cr3f & ~0x08, par); /* clear reset flags */
- udelay(10000);
+ mdelay(10);
/* Savage ramdac speeds */
par->numClocks = 4;
--
1.7.5.4
^ permalink raw reply related
* [PATCH 3/5] video/console: automatically select a font
From: mathieu.poirier @ 2012-05-15 21:02 UTC (permalink / raw)
To: linux-fbdev; +Cc: linux-kernel, arnd, mathieu.poirier
In-Reply-To: <1337115781-27979-1-git-send-email-mathieu.poirier@linaro.org>
From: Arnd Bergmann <arnd@arndb.de>
The frame buffer console needs at least one font to be built into
the kernel, so add the necessary Kconfig magic to guarantee that
one of the available font is always on. If a user accidentally
disables all fonts manually, the 8x16 font will be selected
anyway.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
drivers/video/console/Kconfig | 14 ++++++++++++++
1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
index c2d11fe..e2c96d0 100644
--- a/drivers/video/console/Kconfig
+++ b/drivers/video/console/Kconfig
@@ -224,5 +224,19 @@ config FONT_10x18
big letters. It fits between the sun 12x22 and the normal 8x16 font.
If other fonts are too big or too small for you, say Y, otherwise say N.
+config FONT_AUTOSELECT
+ def_bool y
+ depends on FRAMEBUFFER_CONSOLE || SGI_NEWPORT_CONSOLE || STI_CONSOLE || USB_SISUSBVGA_CON
+ depends on !FONT_8x8
+ depends on !FONT_6x11
+ depends on !FONT_7x14
+ depends on !FONT_PEARL_8x8
+ depends on !FONT_ACORN_8x8
+ depends on !FONT_MINI_4x6
+ depends on !FONT_SUN8x16
+ depends on !FONT_SUN12x22
+ depends on !FONT_10x18
+ select FONT_8x16
+
endmenu
--
1.7.5.4
^ permalink raw reply related
* [PATCH 2/5] video/ili9320: do not mark exported functions __devexit
From: mathieu.poirier @ 2012-05-15 21:02 UTC (permalink / raw)
To: linux-fbdev; +Cc: linux-kernel, arnd, mathieu.poirier
In-Reply-To: <1337115781-27979-1-git-send-email-mathieu.poirier@linaro.org>
From: Arnd Bergmann <arnd@arndb.de>
No symbol can be exported when the section is discarded - the only
solution I could think of is not to mark symbols as __devexit
when they are exported.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
drivers/video/backlight/ili9320.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/video/backlight/ili9320.c b/drivers/video/backlight/ili9320.c
index 5118a9f..b67485a 100644
--- a/drivers/video/backlight/ili9320.c
+++ b/drivers/video/backlight/ili9320.c
@@ -267,7 +267,7 @@ int __devinit ili9320_probe_spi(struct spi_device *spi,
EXPORT_SYMBOL_GPL(ili9320_probe_spi);
-int __devexit ili9320_remove(struct ili9320 *ili)
+int ili9320_remove(struct ili9320 *ili)
{
ili9320_power(ili, FB_BLANK_POWERDOWN);
--
1.7.5.4
^ permalink raw reply related
* [PATCH 1/5] drivers/video: use correct __devexit_p annotation
From: mathieu.poirier @ 2012-05-15 21:02 UTC (permalink / raw)
To: linux-fbdev; +Cc: linux-kernel, arnd, mathieu.poirier
In-Reply-To: <1337115781-27979-1-git-send-email-mathieu.poirier@linaro.org>
From: Arnd Bergmann <arnd@arndb.de>
__devexit functions are discarded when CONFIG_HOTPLUG
is not set, so the symbol needs to be referenced carefully.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
drivers/video/broadsheetfb.c | 2 +-
drivers/video/mbx/mbxfb.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/broadsheetfb.c b/drivers/video/broadsheetfb.c
index 377dde3..c95b417 100644
--- a/drivers/video/broadsheetfb.c
+++ b/drivers/video/broadsheetfb.c
@@ -1211,7 +1211,7 @@ static int __devexit broadsheetfb_remove(struct platform_device *dev)
static struct platform_driver broadsheetfb_driver = {
.probe = broadsheetfb_probe,
- .remove = broadsheetfb_remove,
+ .remove = __devexit_p(broadsheetfb_remove),
.driver = {
.owner = THIS_MODULE,
.name = "broadsheetfb",
diff --git a/drivers/video/mbx/mbxfb.c b/drivers/video/mbx/mbxfb.c
index 55bf619..56550ec 100644
--- a/drivers/video/mbx/mbxfb.c
+++ b/drivers/video/mbx/mbxfb.c
@@ -1045,7 +1045,7 @@ static int __devexit mbxfb_remove(struct platform_device *dev)
static struct platform_driver mbxfb_driver = {
.probe = mbxfb_probe,
- .remove = mbxfb_remove,
+ .remove = __devexit_p(mbxfb_remove),
.suspend = mbxfb_suspend,
.resume = mbxfb_resume,
.driver = {
--
1.7.5.4
^ permalink raw reply related
* [PATCH 0/5] drivers/video: randconfig patches for kernel 3.4.patch
From: mathieu.poirier @ 2012-05-15 21:02 UTC (permalink / raw)
To: linux-fbdev; +Cc: linux-kernel, arnd, mathieu.poirier
From: Arnd Bergmann <arnd@arndb.de>
These patches fix miscellaneous problems when working
with make randconfig. They were discovered on kernel
3.1-rc4 and have been reformatted for 3.4.
Arnd Bergmann (5):
drivers/video: use correct __devexit_p annotation
video/ili9320: do not mark exported functions __devexit
video/console: automatically select a font
drivers/savagefb: use mdelay instead of udelay
drivers/tosa: driver needs I2C and SPI to compile
drivers/video/backlight/Kconfig | 2 +-
drivers/video/backlight/ili9320.c | 2 +-
drivers/video/broadsheetfb.c | 2 +-
drivers/video/console/Kconfig | 14 ++++++++++++++
drivers/video/mbx/mbxfb.c | 2 +-
drivers/video/savage/savagefb_driver.c | 10 +++++-----
6 files changed, 23 insertions(+), 9 deletions(-)
--
1.7.5.4
^ permalink raw reply
* Re: [PATCH v2 2/4] iio: add LM3533 ambient light sensor driver
From: Jonathan Cameron @ 2012-05-15 20:00 UTC (permalink / raw)
To: Johan Hovold
Cc: Jonathan Cameron, Rob Landley, Richard Purdie, Samuel Ortiz,
Greg Kroah-Hartman, Florian Tobias Schandinat, Arnd Bergmann,
Andrew Morton, Mark Brown, linux-doc, linux-kernel, linux-iio,
devel, linux-fbdev
In-Reply-To: <20120515164456.GB15632@localhost>
On 05/15/2012 05:44 PM, Johan Hovold wrote:
> On Tue, May 08, 2012 at 02:47:19PM +0100, Jonathan Cameron wrote:
>> On 5/3/2012 5:36 PM, Johan Hovold wrote:
>>> On Thu, May 03, 2012 at 12:40:10PM +0100, Jonathan Cameron wrote:
>>>> On 5/3/2012 11:26 AM, 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.
>>>> Code is fine. Pretty much all my comments are to do with the interface.
>>> [...]
>>>
>>>>> diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als
>>>>> new file mode 100644
>>>>> index 0000000..9849d14
>>>>> --- /dev/null
>>>>> +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als
>>>>> @@ -0,0 +1,62 @@
>>>>> +What: /sys/bus/iio/devices/iio:deviceX/gain
>>>>> +Date: April 2012
>>>>> +KernelVersion: 3.5
>>>>> +Contact: Johan Hovold<jhovold@gmail.com>
>>>>> +Description:
>>>>> + Set the ALS gain-resistor setting (0..127) for analog input
>>>>> + mode, where
>>>>> +
>>>>> + 0000000 - ALS input is high impedance
>>>>> + 0000001 - 200kOhm (10uA at 2V full-scale)
>>>>> + 0000010 - 100kOhm (20uA at 2V full-scale)
>>>>> + ...
>>>>> + 1111110 - 1.587kOhm (1.26mA at 2V full-scale)
>>>>> + 1111111 - 1.575kOhm (1.27mA at 2V full-scale)
>>>>> +
>>>>> + R_als = 2V / (10uA * gain) (gain> 0)
>>>> Firstly, no magic numbers. These are definitely magic.
>>> Not that magic as they're clearly documented (in code and public
>>> datasheets), right? What would you prefer instead?
>> The numbers on the right of the - look good to me though then this isn't
>> a gain. (200kohm) and the infinite element is annoying. Why not
>> compute the actual gains?
>> Gain = (Rals*10e-6)/2 and use those values? Yes you will have to do
>> a bit of fixed point maths in the driver but the advantage is you'll
>> have real values that are standardizable across multiple devices
>> and hence allow your device to be operated by generic userspace
>> code. Welcome to standardising interfaces - my favourite occupation ;)
>>
>>>> Secondly see in_illuminance0_scale for a suitable existing attribute.
>>> I didn't consider scale to be appropriate given the following
>>> documentation (e.g, for in_voltageY_scale):
>> sorry I just did this to someone else in another review (so I'm
>> consistently wrong!)
>>
>> in_voltageY_calibscale is what I should have said. That one applies a
>> scaling before the raw reading is generated (so in hardware).
>
> Ok, then calibscale is the appropriate attribute for the resistor
> setting. But as this is a device-specific hardware-calibration setting
> I would suggest using the following interface:
>
> What: /sys/bus/iio/devices/iio:deviceX/in_illuminance_calibscale
> Description:
> Set the ALS calibration scale (internal resistors) for
> analog input mode, where the scale factor is the current in uA
> at 2V full-scale (10..1270, 10uA step), that is,
>
> R_als = 2V / in_illuminance_calibscale
>
> This setting is ignored in PWM mode.
This is a generic element that really ought to just fit in with the
equivalent in sysfs-bus-iio for calibscan. It's a ratio, so it should
be unit free for starters.
>
> [...]
>
>>>>> +What: /sys/bus/iio/devices/iio:deviceX/target[m]_[n]
>>>>> +Date: April 2012
>>>>> +KernelVersion: 3.5
>>>>> +Contact: Johan Hovold<jhovold@gmail.com>
>>>>> +Description:
>>>>> + Set the target brightness for ALS-mapper m in light zone n
>>>>> + (0..255), where m in 1..3 and n in 0..4.
>>>> Don't suppose you could do a quick summary of what these zones are
>>>> and why there are 3 ALS-mappers? I'm not getting terribly far on a
>>>> quick look at the datasheet!
>>> Of course. The average adc readings are mapped to five light zones using
>>> eight zone boundary registers (4 boundaries with hysteresis) and a set
>>> of rules.
>> This is going to be fun. We'll need the boundaries and attached
>> hysteresis attributes to fully specify these (nothing would indicate
>> that hysterisis is involved otherwise).
>
> You can't define the hysteresis explicitly with the lm3533 register
> interface, rather it's is defined implicitly in case threshY_falling is
> less than threshY_rasising.
>
> So the raising/falling attributes should be enough, right?
Nope, because they don't tell a general userspace application what is
going on. Without hysterisis attributes it has no way of knowing there
is hysterisis present. Feel free to make them read only though.
>
>>> To simplify somewhat (by ignoring some of the rules): If the average
>>> adc input drops below boundary0_low, the zone register reads 0; if it
>>> drops below boundary1_low, it reads 1, and so on. If the input it
>>> increases over boundary3_high, the zone register return 4; if it
>>> increases passed boundary2_high, it returns zone 3, etc.
>>>
>>> That is, roughly something like (we get 8-bits of input from the ADC):
>>>
>>> zone 0
>>>
>>> boundary0_low 51
>>> boundary0_high 53
>>>
>>> zone 1
>>>
>>> boundary1_low 102
>>> boundary1_high 106
>>>
>>> zone 2
>>>
>>> boundary2_low 153
>>> boundary2_high 161
>>>
>>> zone 3
>>>
>>> boundary3_low 204
>>> boundary3_high 220
>>>
>>> zone 4
>>>
>>> [ Figure 6 on page 20 in the datasheets should make it clear. ]
>>>
>>> The ALS interface and it's zone concept can then be used to control the
>>> LEDs and backlights of the chip, by determining the target brightness for
>>> each zone, e.g., set brightness to 52 when in zone 0.
>>>
>>> To complicate things further (and it is complicated), there are three
>>> such sets of target brightness values: ALSM1, ALSM2, ALSM3.
>>>
>>> So for each LED or backlight you can set ALS-input control mode, by
>>> saying that the device should get it's brightness levels from target set
>>> 1, 2, or 3.
>>>
>>> [ And it gets even more complicated, as ALSM1 can only control
>>> backlight0, where as ALSM2 and ALSM3 can control any of the remaining
>>> devices, but that's irrelevant here. ]
>>>
>>> Initially, I thought this interface to be too esoteric to be worth
>>> generalising, but it sort of fits with event thresholds so I gave it a
>>> try.
>> Glad you did and it pretty much fits, be it with a few extensions being
>> necessary.
>>> The biggest conceptual problem, I think, is that the zone
>>> boundaries can be used to control the other devices, even when the event
>>> is not enabled (or even an irq line not configured). That is, I find it
>>> a bit awkward that the event thresholds also defines the zones (a sort of
>>> discrete scaling factor).
>> That is indeed awkward. I'm not sure how we handle this either. If we
>> need to control these from the other devices (e.g. the back light
>> driver) then we'll have to get them into chan_spec and use the
>> inkernel interfaces to do it. Not infeasible but I was hoping to
>> avoid that until we have had a few months to see what similar devices
>> show up (on basis nothing in this world is a one off for long ;)
>
> I don't think the control bits can or should be generalised at this
> point. The same ALS-target values may be used to control more than one
> device, so they need to be set from the als rather from the controlled
> device (otherwise, changing the target value of led1 could change that
> of the other three leds without the user realising that this can be a
> side effect).
Good point. Nasty little device to write an interface for :)
>
>>> Perhaps simply keeping the attributes outside of events (e.g. named
>>> boundary[n]_{low,high}) and having a custom event enabled (e.g.
>>> in_illuminance_zone_change_en) is the best solution?
>> Maybe, but it's ugly and as you have said, they do correspond pretty well to
>> thresholds so I'd rather you went with that.
>> The core stuff for registering events clearly needs a rethink.... For
>> now doing it as you describe above (with the addition fo hysteresis
>> attributes) should be fine. Just document the 'quirks'.
>
> Ok, I'll keep the event/zone interface as it stands for now and we'll
> see if it can be generalised later. [ See my comment on the hysteresis
> above: there are only the rising/falling thresholds (low/high
> boundaries) and no boundary or hysteresis settings. ]
On that, just to reiterate, to have anything generalizable, userspace
needs to know that hysterisis exists on the individual thresholds
(though it is clearly a function of the neighbouring one).
>
> Thanks,
> Johan
^ permalink raw reply
* Re: [PULL for v3.5] SH Mobile LCDC overlay support
From: Laurent Pinchart @ 2012-05-15 19:19 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <5022250.LOHRC1DPLO@avalon>
Hi Florian,
On Sunday 13 May 2012 20:36:29 Florian Tobias Schandinat wrote:
> On 04/24/2012 11:31 AM, Laurent Pinchart wrote:
> > Hi Florian,
> >
> > Could you please pull the following patches that add overlay support for
> > the SH Mobile LCDC ? They have been posted for review as part of a bigger
> > series some time ago.
>
> Well, besides that I dislike everyone inventing his/her own overlay API
> and just not having time to propose something that fits everyone your
> patch looks mostly okay.
This is a topic that we started to discuss at the ELC a couple of months ago.
My goal was to come up with a single API shared between DRM/KMS, FB and V4L2
to handle graphics pipeline configuration. It quickly turned out that there
was not enough shared interest between DRM and V4L2 developers to make this
happen. Should we resume the discussion for FB, or would you rather want a
separate FB-specific API ?
> Two things:
>
> "That's fine, but you need a Documentation/ABI entry for any new sysfs
> file you create." [Greg K-H]
Thank you for reminding me.
> So as far as I understand Gregs statement and your code correct you
> should add such thing.
>
> I'm not convinced that you don't leak resources. Is the memory you
> allocate with dma_alloc_coherent for the overlays really freed after
> normal use and in error paths?
I'm not sure where the cleanup part of the patch went to, but definitely not
in that branch :-/ I'll fix that and resubmit.
--
Best regards,
Laurent Pinchart
^ permalink raw reply
* [PATCH v4] backlight: add LM3533 backlight driver
From: Johan Hovold @ 2012-05-15 19:13 UTC (permalink / raw)
To: Richard Purdie, Florian Tobias Schandinat, Andrew Morton
Cc: Rob Landley, Samuel Ortiz, Jonathan Cameron, Greg Kroah-Hartman,
Arnd Bergmann, Mark Brown, linux-doc, linux-kernel, linux-fbdev,
Johan Hovold
In-Reply-To: <1336674568-24319-1-git-send-email-jhovold@gmail.com>
Add sub-driver for the backlights on National Semiconductor / TI LM3533
lighting power chips.
The chip provides 256 brightness levels and ambient-light-sensor and pwm
input control.
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
v2
- add sysfs-ABI documentation
- open code max_current/pwm macros
v3
- remove max_current attribute
v4
- fix return type of attribute is_visible
- fix sysfs-ABI-documentation typo
.../testing/sysfs-class-backlight-driver-lm3533 | 41 ++
drivers/video/backlight/Kconfig | 12 +
drivers/video/backlight/Makefile | 1 +
drivers/video/backlight/lm3533_bl.c | 423 ++++++++++++++++++++
4 files changed, 477 insertions(+), 0 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-class-backlight-driver-lm3533
create mode 100644 drivers/video/backlight/lm3533_bl.c
diff --git a/Documentation/ABI/testing/sysfs-class-backlight-driver-lm3533 b/Documentation/ABI/testing/sysfs-class-backlight-driver-lm3533
new file mode 100644
index 0000000..ea91f71
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-backlight-driver-lm3533
@@ -0,0 +1,41 @@
+What: /sys/class/backlight/<backlight>/als
+Date: April 2012
+KernelVersion: 3.5
+Contact: Johan Hovold <jhovold@gmail.com>
+Description:
+ Set the ALS-control mode (0..2), where
+
+ 0 - disabled
+ 1 - ALS-mapper 1 (backlight 0)
+ 2 - ALS-mapper 2 (backlight 1)
+
+What: /sys/class/backlight/<backlight>/id
+Date: April 2012
+KernelVersion: 3.5
+Contact: Johan Hovold <jhovold@gmail.com>
+Description:
+ Get the id of this backlight (0, 1).
+
+What: /sys/class/backlight/<backlight>/linear
+Date: April 2012
+KernelVersion: 3.5
+Contact: Johan Hovold <jhovold@gmail.com>
+Description:
+ Set the brightness-mapping mode (0, 1), where
+
+ 0 - exponential mode
+ 1 - linear mode
+
+What: /sys/class/backlight/<backlight>/pwm
+Date: April 2012
+KernelVersion: 3.5
+Contact: Johan Hovold <jhovold@gmail.com>
+Description:
+ Set the PWM-input control mask (5 bits), where
+
+ bit 5 - PWM-input enabled in Zone 4
+ bit 4 - PWM-input enabled in Zone 3
+ bit 3 - PWM-input enabled in Zone 2
+ bit 2 - PWM-input enabled in Zone 1
+ bit 1 - PWM-input enabled in Zone 0
+ bit 0 - PWM-input enabled
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index af16884..fa2b037 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -184,6 +184,18 @@ config BACKLIGHT_GENERIC
known as the Corgi backlight driver. If you have a Sharp Zaurus
SL-C7xx, SL-Cxx00 or SL-6000x say y.
+config BACKLIGHT_LM3533
+ tristate "Backlight Driver for LM3533"
+ depends on BACKLIGHT_CLASS_DEVICE
+ depends on MFD_LM3533
+ help
+ Say Y to enable the backlight driver for National Semiconductor / TI
+ LM3533 Lighting Power chips.
+
+ The backlights can be controlled directly, through PWM input, or by
+ the ambient-light-sensor interface. The chip supports 256 brightness
+ levels.
+
config BACKLIGHT_LOCOMO
tristate "Sharp LOCOMO LCD/Backlight Driver"
depends on SHARP_LOCOMO
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index 36855ae..a2ac9cf 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_BACKLIGHT_EP93XX) += ep93xx_bl.o
obj-$(CONFIG_BACKLIGHT_GENERIC) += generic_bl.o
obj-$(CONFIG_BACKLIGHT_HP700) += jornada720_bl.o
obj-$(CONFIG_BACKLIGHT_HP680) += hp680_bl.o
+obj-$(CONFIG_BACKLIGHT_LM3533) += lm3533_bl.o
obj-$(CONFIG_BACKLIGHT_LOCOMO) += locomolcd.o
obj-$(CONFIG_BACKLIGHT_LP855X) += lp855x_bl.o
obj-$(CONFIG_BACKLIGHT_OMAP1) += omap1_bl.o
diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
new file mode 100644
index 0000000..0148227
--- /dev/null
+++ b/drivers/video/backlight/lm3533_bl.c
@@ -0,0 +1,423 @@
+/*
+ * lm3533-bl.c -- LM3533 Backlight driver
+ *
+ * Copyright (C) 2011-2012 Texas Instruments
+ *
+ * Author: Johan Hovold <jhovold@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/backlight.h>
+#include <linux/fb.h>
+#include <linux/slab.h>
+
+#include <linux/mfd/lm3533.h>
+
+
+#define LM3533_HVCTRLBANK_COUNT 2
+#define LM3533_BL_MAX_BRIGHTNESS 255
+
+#define LM3533_REG_CTRLBANK_AB_BCONF 0x1a
+
+
+struct lm3533_bl {
+ struct lm3533 *lm3533;
+ struct lm3533_ctrlbank cb;
+ struct backlight_device *bd;
+ int id;
+};
+
+
+static inline int lm3533_bl_get_ctrlbank_id(struct lm3533_bl *bl)
+{
+ return bl->id;
+}
+
+static int lm3533_bl_update_status(struct backlight_device *bd)
+{
+ struct lm3533_bl *bl = bl_get_data(bd);
+ int brightness = bd->props.brightness;
+
+ if (bd->props.power != FB_BLANK_UNBLANK)
+ brightness = 0;
+ if (bd->props.fb_blank != FB_BLANK_UNBLANK)
+ brightness = 0;
+
+ return lm3533_ctrlbank_set_brightness(&bl->cb, (u8)brightness);
+}
+
+static int lm3533_bl_get_brightness(struct backlight_device *bd)
+{
+ struct lm3533_bl *bl = bl_get_data(bd);
+ u8 val;
+ int ret;
+
+ ret = lm3533_ctrlbank_get_brightness(&bl->cb, &val);
+ if (ret)
+ return ret;
+
+ return val;
+}
+
+static const struct backlight_ops lm3533_bl_ops = {
+ .get_brightness = lm3533_bl_get_brightness,
+ .update_status = lm3533_bl_update_status,
+};
+
+static ssize_t show_id(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct lm3533_bl *bl = dev_get_drvdata(dev);
+
+ return scnprintf(buf, PAGE_SIZE, "%d\n", bl->id);
+}
+
+/*
+ * ALS-control setting:
+ *
+ * 0 - ALS disabled
+ * 1 - ALS-mapper 1 (backlight 0)
+ * 2 - ALS-mapper 2 (backlight 1)
+ */
+static ssize_t show_als(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct lm3533_bl *bl = dev_get_drvdata(dev);
+ int ctrlbank = lm3533_bl_get_ctrlbank_id(bl);
+ u8 val;
+ u8 mask;
+ int als;
+ int ret;
+
+ ret = lm3533_read(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF, &val);
+ if (ret)
+ return ret;
+
+ mask = 2 * ctrlbank;
+ als = val & mask;
+ if (als)
+ als = ctrlbank + 1;
+
+ return scnprintf(buf, PAGE_SIZE, "%d\n", als);
+}
+
+static ssize_t store_als(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ struct lm3533_bl *bl = dev_get_drvdata(dev);
+ int ctrlbank = lm3533_bl_get_ctrlbank_id(bl);
+ int als;
+ u8 val;
+ u8 mask;
+ int ret;
+
+ if (kstrtoint(buf, 0, &als))
+ return -EINVAL;
+
+ if (als != 0 && (als != ctrlbank + 1))
+ return -EINVAL;
+
+ mask = 1 << (2 * ctrlbank);
+
+ if (als)
+ val = mask;
+ else
+ val = 0;
+
+ ret = lm3533_update(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF, val,
+ mask);
+ if (ret)
+ return ret;
+
+ return len;
+}
+
+static ssize_t show_linear(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct lm3533_bl *bl = dev_get_drvdata(dev);
+ u8 val;
+ u8 mask;
+ int linear;
+ int ret;
+
+ ret = lm3533_read(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF, &val);
+ if (ret)
+ return ret;
+
+ mask = 1 << (2 * lm3533_bl_get_ctrlbank_id(bl) + 1);
+
+ if (val & mask)
+ linear = 1;
+ else
+ linear = 0;
+
+ return scnprintf(buf, PAGE_SIZE, "%x\n", linear);
+}
+
+static ssize_t store_linear(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ struct lm3533_bl *bl = dev_get_drvdata(dev);
+ unsigned long linear;
+ u8 mask;
+ u8 val;
+ int ret;
+
+ if (kstrtoul(buf, 0, &linear))
+ return -EINVAL;
+
+ mask = 1 << (2 * lm3533_bl_get_ctrlbank_id(bl) + 1);
+
+ if (linear)
+ val = mask;
+ else
+ val = 0;
+
+ ret = lm3533_update(bl->lm3533, LM3533_REG_CTRLBANK_AB_BCONF, val,
+ mask);
+ if (ret)
+ return ret;
+
+ return len;
+}
+
+static ssize_t show_pwm(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct lm3533_bl *bl = dev_get_drvdata(dev);
+ u8 val;
+ int ret;
+
+ ret = lm3533_ctrlbank_get_pwm(&bl->cb, &val);
+ if (ret)
+ return ret;
+
+ return scnprintf(buf, PAGE_SIZE, "%u\n", val);
+}
+
+static ssize_t store_pwm(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ struct lm3533_bl *bl = dev_get_drvdata(dev);
+ u8 val;
+ int ret;
+
+ if (kstrtou8(buf, 0, &val))
+ return -EINVAL;
+
+ ret = lm3533_ctrlbank_set_pwm(&bl->cb, val);
+ if (ret)
+ return ret;
+
+ return len;
+}
+
+static LM3533_ATTR_RW(als);
+static LM3533_ATTR_RO(id);
+static LM3533_ATTR_RW(linear);
+static LM3533_ATTR_RW(pwm);
+
+static struct attribute *lm3533_bl_attributes[] = {
+ &dev_attr_als.attr,
+ &dev_attr_id.attr,
+ &dev_attr_linear.attr,
+ &dev_attr_pwm.attr,
+ NULL,
+};
+
+static umode_t lm3533_bl_attr_is_visible(struct kobject *kobj,
+ struct attribute *attr, int n)
+{
+ struct device *dev = container_of(kobj, struct device, kobj);
+ struct lm3533_bl *bl = dev_get_drvdata(dev);
+ umode_t mode = attr->mode;
+
+ if (attr = &dev_attr_als.attr) {
+ if (!bl->lm3533->have_als)
+ mode = 0;
+ }
+
+ return mode;
+};
+
+static struct attribute_group lm3533_bl_attribute_group = {
+ .is_visible = lm3533_bl_attr_is_visible,
+ .attrs = lm3533_bl_attributes
+};
+
+static int __devinit lm3533_bl_setup(struct lm3533_bl *bl,
+ struct lm3533_bl_platform_data *pdata)
+{
+ int ret;
+
+ ret = lm3533_ctrlbank_set_max_current(&bl->cb, pdata->max_current);
+ if (ret)
+ return ret;
+
+ return lm3533_ctrlbank_set_pwm(&bl->cb, pdata->pwm);
+}
+
+static int __devinit lm3533_bl_probe(struct platform_device *pdev)
+{
+ struct lm3533 *lm3533;
+ struct lm3533_bl_platform_data *pdata;
+ struct lm3533_bl *bl;
+ struct backlight_device *bd;
+ struct backlight_properties props;
+ int ret;
+
+ dev_dbg(&pdev->dev, "%s\n", __func__);
+
+ lm3533 = dev_get_drvdata(pdev->dev.parent);
+ if (!lm3533)
+ return -EINVAL;
+
+ pdata = pdev->dev.platform_data;
+ if (!pdata) {
+ dev_err(&pdev->dev, "no platform data\n");
+ return -EINVAL;
+ }
+
+ if (pdev->id < 0 || pdev->id >= LM3533_HVCTRLBANK_COUNT) {
+ dev_err(&pdev->dev, "illegal backlight id %d\n", pdev->id);
+ return -EINVAL;
+ }
+
+ bl = kzalloc(sizeof(*bl), GFP_KERNEL);
+ if (!bl) {
+ dev_err(&pdev->dev,
+ "failed to allocate memory for backlight\n");
+ return -ENOMEM;
+ }
+
+ bl->lm3533 = lm3533;
+ bl->id = pdev->id;
+
+ bl->cb.lm3533 = lm3533;
+ bl->cb.id = lm3533_bl_get_ctrlbank_id(bl);
+ bl->cb.dev = NULL; /* until registered */
+
+ memset(&props, 0, sizeof(props));
+ props.type = BACKLIGHT_RAW;
+ props.max_brightness = LM3533_BL_MAX_BRIGHTNESS;
+ props.brightness = pdata->default_brightness;
+ bd = backlight_device_register(pdata->name, pdev->dev.parent, bl,
+ &lm3533_bl_ops, &props);
+ if (IS_ERR(bd)) {
+ dev_err(&pdev->dev, "failed to register backlight device\n");
+ ret = PTR_ERR(bd);
+ goto err_free;
+ }
+
+ bl->bd = bd;
+ bl->cb.dev = &bl->bd->dev;
+
+ platform_set_drvdata(pdev, bl);
+
+ ret = sysfs_create_group(&bd->dev.kobj, &lm3533_bl_attribute_group);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to create sysfs attributes\n");
+ goto err_unregister;
+ }
+
+ backlight_update_status(bd);
+
+ ret = lm3533_bl_setup(bl, pdata);
+ if (ret)
+ goto err_sysfs_remove;
+
+ ret = lm3533_ctrlbank_enable(&bl->cb);
+ if (ret)
+ goto err_sysfs_remove;
+
+ return 0;
+
+err_sysfs_remove:
+ sysfs_remove_group(&bd->dev.kobj, &lm3533_bl_attribute_group);
+err_unregister:
+ backlight_device_unregister(bd);
+err_free:
+ kfree(bl);
+
+ return ret;
+}
+
+static int __devexit lm3533_bl_remove(struct platform_device *pdev)
+{
+ struct lm3533_bl *bl = platform_get_drvdata(pdev);
+ struct backlight_device *bd = bl->bd;
+
+ dev_dbg(&bd->dev, "%s\n", __func__);
+
+ bd->props.power = FB_BLANK_POWERDOWN;
+ bd->props.brightness = 0;
+
+ lm3533_ctrlbank_disable(&bl->cb);
+ sysfs_remove_group(&bd->dev.kobj, &lm3533_bl_attribute_group);
+ backlight_device_unregister(bd);
+ kfree(bl);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+static int lm3533_bl_suspend(struct platform_device *pdev, pm_message_t state)
+{
+ struct lm3533_bl *bl = platform_get_drvdata(pdev);
+
+ dev_dbg(&pdev->dev, "%s\n", __func__);
+
+ return lm3533_ctrlbank_disable(&bl->cb);
+}
+
+static int lm3533_bl_resume(struct platform_device *pdev)
+{
+ struct lm3533_bl *bl = platform_get_drvdata(pdev);
+
+ dev_dbg(&pdev->dev, "%s\n", __func__);
+
+ return lm3533_ctrlbank_enable(&bl->cb);
+}
+#else
+#define lm3533_bl_suspend NULL
+#define lm3533_bl_resume NULL
+#endif
+
+static void lm3533_bl_shutdown(struct platform_device *pdev)
+{
+ struct lm3533_bl *bl = platform_get_drvdata(pdev);
+
+ dev_dbg(&pdev->dev, "%s\n", __func__);
+
+ lm3533_ctrlbank_disable(&bl->cb);
+}
+
+static struct platform_driver lm3533_bl_driver = {
+ .driver = {
+ .name = "lm3533-backlight",
+ .owner = THIS_MODULE,
+ },
+ .probe = lm3533_bl_probe,
+ .remove = __devexit_p(lm3533_bl_remove),
+ .shutdown = lm3533_bl_shutdown,
+ .suspend = lm3533_bl_suspend,
+ .resume = lm3533_bl_resume,
+};
+module_platform_driver(lm3533_bl_driver);
+
+MODULE_AUTHOR("Johan Hovold <jhovold@gmail.com>");
+MODULE_DESCRIPTION("LM3533 Backlight driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:lm3533-backlight");
--
1.7.8.5
^ permalink raw reply related
* Re: ASoC: wm9712: Microphone doesn't work: Mux handling broken?
From: Mark Brown @ 2012-05-15 17:15 UTC (permalink / raw)
To: Christoph Fritz
Cc: alsa-devel, Hans J. Koch, Sascha Hauer, Liam Girdwood,
linux-fbdev
In-Reply-To: <20120515091541.GA4433@lovely.krouter>
[-- Attachment #1: Type: text/plain, Size: 992 bytes --]
On Tue, May 15, 2012 at 11:15:41AM +0200, Christoph Fritz wrote:
> On Sun, May 13, 2012 at 05:56:53AM +0200, Christoph Fritz wrote:
> > > Just filling in the appropriate mux value in the relevant route should
> > > do the trick.
> Do you mean filling in to wm9712_audio_map or wm9712_enum?
There will need to be routes in the audio map. Don't know if there are
any fixes needed in the enum, it was the routing I noticed was missing.
> > > Looking at the code it looks like the widget isn't hooked
> > > into the audio routing map at all so I'm a little surprised.
> Does that mean that wm9712_dapm_widgets should be referred by a
> struct snd_kcontrol_new ?
No, that should never happen.
> > >I'm out of
> > > the office at the minute and so can't readily set up a test system
> > > myself.
> I'm not that into alsa and would greatly appreciate if you could have
> a look with your test system.
For personal reasons I don't really have an ETA for when I'll be in the
office, sorry.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/4] iio: add LM3533 ambient light sensor driver
From: Johan Hovold @ 2012-05-15 16:44 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Johan Hovold, Rob Landley, Richard Purdie, Samuel Ortiz,
Greg Kroah-Hartman, Florian Tobias Schandinat, Arnd Bergmann,
Andrew Morton, Mark Brown, linux-doc, linux-kernel, linux-iio,
devel, linux-fbdev
In-Reply-To: <4FA923E7.6050707@cam.ac.uk>
On Tue, May 08, 2012 at 02:47:19PM +0100, Jonathan Cameron wrote:
> On 5/3/2012 5:36 PM, Johan Hovold wrote:
> > On Thu, May 03, 2012 at 12:40:10PM +0100, Jonathan Cameron wrote:
> >> On 5/3/2012 11:26 AM, 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.
> >> Code is fine. Pretty much all my comments are to do with the interface.
> > [...]
> >
> >>> diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als
> >>> new file mode 100644
> >>> index 0000000..9849d14
> >>> --- /dev/null
> >>> +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als
> >>> @@ -0,0 +1,62 @@
> >>> +What: /sys/bus/iio/devices/iio:deviceX/gain
> >>> +Date: April 2012
> >>> +KernelVersion: 3.5
> >>> +Contact: Johan Hovold<jhovold@gmail.com>
> >>> +Description:
> >>> + Set the ALS gain-resistor setting (0..127) for analog input
> >>> + mode, where
> >>> +
> >>> + 0000000 - ALS input is high impedance
> >>> + 0000001 - 200kOhm (10uA at 2V full-scale)
> >>> + 0000010 - 100kOhm (20uA at 2V full-scale)
> >>> + ...
> >>> + 1111110 - 1.587kOhm (1.26mA at 2V full-scale)
> >>> + 1111111 - 1.575kOhm (1.27mA at 2V full-scale)
> >>> +
> >>> + R_als = 2V / (10uA * gain) (gain> 0)
> >> Firstly, no magic numbers. These are definitely magic.
> > Not that magic as they're clearly documented (in code and public
> > datasheets), right? What would you prefer instead?
> The numbers on the right of the - look good to me though then this isn't
> a gain. (200kohm) and the infinite element is annoying. Why not
> compute the actual gains?
> Gain = (Rals*10e-6)/2 and use those values? Yes you will have to do
> a bit of fixed point maths in the driver but the advantage is you'll
> have real values that are standardizable across multiple devices
> and hence allow your device to be operated by generic userspace
> code. Welcome to standardising interfaces - my favourite occupation ;)
>
> >> Secondly see in_illuminance0_scale for a suitable existing attribute.
> > I didn't consider scale to be appropriate given the following
> > documentation (e.g, for in_voltageY_scale):
> sorry I just did this to someone else in another review (so I'm
> consistently wrong!)
>
> in_voltageY_calibscale is what I should have said. That one applies a
> scaling before the raw reading is generated (so in hardware).
Ok, then calibscale is the appropriate attribute for the resistor
setting. But as this is a device-specific hardware-calibration setting
I would suggest using the following interface:
What: /sys/bus/iio/devices/iio:deviceX/in_illuminance_calibscale
Description:
Set the ALS calibration scale (internal resistors) for
analog input mode, where the scale factor is the current in uA
at 2V full-scale (10..1270, 10uA step), that is,
R_als = 2V / in_illuminance_calibscale
This setting is ignored in PWM mode.
[...]
> >>> +What: /sys/bus/iio/devices/iio:deviceX/target[m]_[n]
> >>> +Date: April 2012
> >>> +KernelVersion: 3.5
> >>> +Contact: Johan Hovold<jhovold@gmail.com>
> >>> +Description:
> >>> + Set the target brightness for ALS-mapper m in light zone n
> >>> + (0..255), where m in 1..3 and n in 0..4.
> >> Don't suppose you could do a quick summary of what these zones are
> >> and why there are 3 ALS-mappers? I'm not getting terribly far on a
> >> quick look at the datasheet!
> > Of course. The average adc readings are mapped to five light zones using
> > eight zone boundary registers (4 boundaries with hysteresis) and a set
> > of rules.
> This is going to be fun. We'll need the boundaries and attached
> hysteresis attributes to fully specify these (nothing would indicate
> that hysterisis is involved otherwise).
You can't define the hysteresis explicitly with the lm3533 register
interface, rather it's is defined implicitly in case threshY_falling is
less than threshY_rasising.
So the raising/falling attributes should be enough, right?
> > To simplify somewhat (by ignoring some of the rules): If the average
> > adc input drops below boundary0_low, the zone register reads 0; if it
> > drops below boundary1_low, it reads 1, and so on. If the input it
> > increases over boundary3_high, the zone register return 4; if it
> > increases passed boundary2_high, it returns zone 3, etc.
> >
> > That is, roughly something like (we get 8-bits of input from the ADC):
> >
> > zone 0
> >
> > boundary0_low 51
> > boundary0_high 53
> >
> > zone 1
> >
> > boundary1_low 102
> > boundary1_high 106
> >
> > zone 2
> >
> > boundary2_low 153
> > boundary2_high 161
> >
> > zone 3
> >
> > boundary3_low 204
> > boundary3_high 220
> >
> > zone 4
> >
> > [ Figure 6 on page 20 in the datasheets should make it clear. ]
> >
> > The ALS interface and it's zone concept can then be used to control the
> > LEDs and backlights of the chip, by determining the target brightness for
> > each zone, e.g., set brightness to 52 when in zone 0.
> >
> > To complicate things further (and it is complicated), there are three
> > such sets of target brightness values: ALSM1, ALSM2, ALSM3.
> >
> > So for each LED or backlight you can set ALS-input control mode, by
> > saying that the device should get it's brightness levels from target set
> > 1, 2, or 3.
> >
> > [ And it gets even more complicated, as ALSM1 can only control
> > backlight0, where as ALSM2 and ALSM3 can control any of the remaining
> > devices, but that's irrelevant here. ]
> >
> > Initially, I thought this interface to be too esoteric to be worth
> > generalising, but it sort of fits with event thresholds so I gave it a
> > try.
> Glad you did and it pretty much fits, be it with a few extensions being
> necessary.
> > The biggest conceptual problem, I think, is that the zone
> > boundaries can be used to control the other devices, even when the event
> > is not enabled (or even an irq line not configured). That is, I find it
> > a bit awkward that the event thresholds also defines the zones (a sort of
> > discrete scaling factor).
> That is indeed awkward. I'm not sure how we handle this either. If we
> need to control these from the other devices (e.g. the back light
> driver) then we'll have to get them into chan_spec and use the
> inkernel interfaces to do it. Not infeasible but I was hoping to
> avoid that until we have had a few months to see what similar devices
> show up (on basis nothing in this world is a one off for long ;)
I don't think the control bits can or should be generalised at this
point. The same ALS-target values may be used to control more than one
device, so they need to be set from the als rather from the controlled
device (otherwise, changing the target value of led1 could change that
of the other three leds without the user realising that this can be a
side effect).
> > Perhaps simply keeping the attributes outside of events (e.g. named
> > boundary[n]_{low,high}) and having a custom event enabled (e.g.
> > in_illuminance_zone_change_en) is the best solution?
> Maybe, but it's ugly and as you have said, they do correspond pretty well to
> thresholds so I'd rather you went with that.
> The core stuff for registering events clearly needs a rethink.... For
> now doing it as you describe above (with the addition fo hysteresis
> attributes) should be fine. Just document the 'quirks'.
Ok, I'll keep the event/zone interface as it stands for now and we'll
see if it can be generalised later. [ See my comment on the hysteresis
above: there are only the rising/falling thresholds (low/high
boundaries) and no boundary or hysteresis settings. ]
Thanks,
Johan
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox