Re: [PATCH v6 6/9] leds: multicolor: Introduce a multicolor class definition

[Dan Murphy]

Jacek

On 9/19/19 4:32 PM, Jacek Anaszewski wrote:
> Dan,
>
> On 9/19/19 3:07 AM, Dan Murphy wrote:
> > Jacek
> >
> > On 9/18/19 4:27 PM, Jacek Anaszewski wrote:
> > > Hi Dan,
> > >
> > > I think Greg's guidance clarified everything nicely -
> > > we will avoid <color> sub-dirs in favour of prefixes
> > > to *intensity and *max_intensity.
> > Yes I will make the change accordingly. It will simplify the code.
> > > Before you will send an update I have some improvement
> > > ideas regarding the remnants after the previous approach,
> > > where single color intensity update resulted in updating
> > > hardware state. Now the update will happen only on write to
> > > brightness file, so we will not need color_set/color_get ops
> > > anymore.
> > I left those call backs in specifically for the LP50xx. Otherwise the
> > LEDs are only updated when the brightness file is written.
> > The LP50xx has an engine that performs the intensity computation for the
> > specific LED. So there is no call back to the MC FW for calculating the
> > intensity.
> >
> > The brightness and intensity are written directly to the device and the
> > MCU in the device does all the computations so you have real time update.
> You can still handle that in brightness_set op. You need to compare
> which color channels have changed and update them in hardware in
> addition to setting LEDn_BRIGHTNESS register.
>
> And yes - even updating a single color will need two operations:

If we kept the ops then the LP50xx device would only need one operation 
to the led intensity file to update the color.

> echo 231 >  colors/red_intensity // only cache the color in MC core
> echo 100 > brightness // do the actual hw update
And this is the way the LP55xx device works now.
> Note that brightness value doesn't have to be necessarily different
> from the previous one here, but writing brightness file will be needed
> to trigger the hw update.
>
> > For the LP55xx device the LEDs are only updated when the brightness file
> > is written.
> >
> > I think we can leave those call backs in if device driver or product
> > development teams would like to use them.
> I'd not do that - it will be confusing. We can accomplish everything
> in brightness_set{_blocking} op. It will have also the advantage of
> same ABI semantics across all devices. Otherwise we would need separate
> documentation for devices like LP50xx.

OK I am not going to argue this I will just remove the ops even though I 
don't agree.

Removing the ops will just make the LP50xx driver more complex then what 
it needs to be.

I will post v8 later today.

> I have also another question - what with linear vs logarithmic
> LP50xx brightness scale? I think we should make both options available
> to the userspace.

I have no requirements from customers to provide this scaling.

It can be an enhancement to the driver later if we get the request.

Dan