On Wed, 24 Feb 2016 13:49:46 -0600
Rob Herring <robh+dt@xxxxxxxxxx> wrote:
> On Wed, Feb 24, 2016 at 1:34 PM, David Rivshin (Allworx)
> <drivshin.allworx@xxxxxxxxx> wrote:
> > On Wed, 24 Feb 2016 17:04:49 +0100
> > Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx> wrote:
> >
> >> Hi David,
> >>
> >> Thanks for the patch.
> >>
> >> On 02/23/2016 07:17 PM, David Rivshin (Allworx) wrote:
> >> > From: David Rivshin <drivshin@xxxxxxxxxxx>
> >> >
> >> > This adds a binding description for the is31fl3236/35/18/16 I2C LED
> >> > drivers.
> >> >
> >> > Signed-off-by: David Rivshin <drivshin@xxxxxxxxxxx>
> >> > ---
> >> > .../devicetree/bindings/leds/leds-is31fl32xx.txt | 51 ++++++++++++++++++++++
> >> > 1 file changed, 51 insertions(+)
> >> > create mode 100644 Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
> >> > new file mode 100644
> >> > index 0000000..0a05a1d
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
> >> > @@ -0,0 +1,51 @@
> >> > +Binding for ISSI IS31FL32xx LED Drivers
> >> > +
> >> > +The IS31FL32xx family of LED drivers are I2C devices with multiple
> >> > +constant-current channels, each with independent 256-level PWM control.
> >> > +Each LED is represented as a sub-node of the device.
> >> > +
> >> > +Required properties:
> >> > +- compatible: one of
> >> > + issi,is31fl3236
> >> > + issi,is31fl3235
> >> > + issi,is31fl3218
> >> > + issi,is31fl3216
> >> > +- reg: I2C slave address
> >> > +- address-cells : must be 1
> >> > +- size-cells : must be 0
> >> > +
> >> > +LED sub-node properties:
> >> > +- reg : LED channel number (1..N)
> >> > +- max-brightness : (optional) Maximum brightness possible for the LED.
> >>
> >> Please use led-max-microamp instead. You can refer to
> >> Documentation/devicetree/bindings/leds/common.txt for detailed
> >> description.
> >
> > FYI, I think I got that property from leds-pwm, since these use PWMs
> > internally, although I don't actually use it myself.
> >
> > I can see how led-max-microamp could be a useful property, however
> > I'm uncertain about computing cdev->max_brightness from it as these
> > devices look to control their max current via an external resistor.
> > I suppose either the resistor value, or the resultant max-current
> > would need to be given in the devicetree, and then that used along
> > with led-max-microamp to compute cdev->max_brightness. Although I'd
> > want it to be optional as I suspect most users don't need any
> > restriction. Also, I don't think I can properly test the result,
> > which always makes me nervous.
> >
> > Would it be acceptable to simply remove the max-brightness property
> > without adding led-max-microamp? It can always be added if a user
> > turns out to need it in the future. Or do you feel strongly that
> > led-max-microamp should be in the binding from the start?
>
> You should put in DT whatever makes sense for the controls the h/w
> has. If you only have a PWM, then only max-brightness makes sense and
> led-max-microamp does not.
There are multiple ways you can look at it, and I'm not sure which
is the overriding one. The brightness of the LED is determined by the
current, which is in turn a combination of the following:
- The max per-LED current is set with an external resistor.
- Some devices have a scaling factor, which can be changed dynamically:
- One of the devices has either a global scaling factor in a register,
that can adjust max-current from 0.25x to 2x.
- Two of the devices have a per-LED current divisor (1, 2, 3, or 4).
- The main control is a 0-255 "PWM duty cycle" register, which
effectively scales the current.
IOW, the final current is:
<resistor-setting> * <scaling> * <PWM-duty-cycle>
Currently the binding and driver do not support setting either type of
scaling (always unity) and only use the PWM duty cycle to dim the LED.
So if the purpose is to say "never allow this LED channel to go above
X current", the leds-max-microamp probably makes sense.
If the purpose is to say "never allow the PWM duty cycle register
for this channel to go above value X", then max-brightness perhaps
makes sense.
If the scaling is fixed by the DT (i.e won't be changed dynamically
by the OS), then those two are basically equivalent in functionality.
But if scaling can be changed dynamically, then they have different
implications.
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
