LED driver

[networkimprov]

Manual: http://www.nxp.com/documents/data_sheet/PCA9632.pdf Source: https://github.com/networkimprov/linux/blob/anvl-v4.7/drivers/leds/leds-pca963x.c

Restore state after suspend/resume (not necessary if we implement alt-suspend #42)

Reset hw on reboot so led state is cleared

Set blink state based on measured timing (~78x/min) instead of spec'd 60/min.

Why does LED light green on power-off restart, but not on warm restart?

protect LED from kernel misconfiguration drivers/leds/leds-pca963x.c has the following comment: /* default to open-drain unless totem pole (push-pull) is specified */

Add a device tree binding to the driver. (Have this, possibly incomplete.)

@mranostay mentioned some other issues seen on first review of module - mutex locking issue, others?

[mranostay]

Does "protect LED from kernel misconfiguration" mean making sure they can't change it to hw-totempole? I can sense the bad things that would happen if you pushed current in totem-pole mode :)....

[networkimprov]

Braden (original EE): The CPU GPIO LED can be used concurrently and safely on the Anvl with the leds-pca963x driver if the platform data / device tree leaves the PCA9632 in open-drain mode (which it currently does). The Anvl uses the pca9632 in open-drain output mode, But if “outdrv” type is eventually added to the device tree to support different platform uses of the PCA963x, then the Anvl device tree should set it as open drain, and not totem-pole/push-pull mode. When the led platform device structure is present, I think the default configuration is pca9633_platform_data.outdrv = PCA9633_OPEN_DRAIN, which is okay. If the platform structure is not present, then the default PCA963x mode will be used, which is also okay. The chip defaults to open-drain output mode on power up. In the leds-pca963x driver, pca9633_dt_init() doesn’t appear to explicitly set the pdata->outdrv, so again I assume it’s set to its default of PCA9633_OPEN_DRAIN (which I think enumerates as 0).

The remote paranoia behind this: The PCA9632 in the Anvl is powered at 2.8V (to provide compatibility with the 1.8V I2C I/O signaling). If the outputs are configured as platform_data->outdrv = PCA9633_TOTEM_POLE AND the CPU GPIO “BOOT_LED” output is active, then the PCA9632 output pins, if also active (driven to 2.8V), could source all of its output current to GND through the “BOOT_LED” N-FETS, potentially damaging the PCA9632 and/or the 200mA N-FETs.

[mranostay]

Also "Set blink state based on measured timing (~78x/min) instead of spec'd 60/min." what is measured timing in this context?

[networkimprov]

The PCA chip blinks faster than it's supposed to. I verified that it's not a driver bug by using an I2C tool to write the blink registers directly. We need to adjust the driver's calculation of blink values for the measured timing. I've added a link to manual at the top of the page.

[mranostay]

Ok yeah will look into the calculation today.

[networkimprov]

The charger driver is the highest-priority BTW.

[mranostay]

Calculation in the driver is correct, and validated that it is those settings are being set on the device. Albeit there is a slight rounding error but nothing that should make it blink at more than 63 times a minute.

@networkimprov Was this issue was measured by stopwatch, or having a scope verify?

[networkimprov]

Hey you didn't believe me that I checked that? :-) We should adjust the user inputs to produce the expected blink speed; multiply the values by 0.75(?) before translating them to chip inputs via existing algorithm.

I measured by stopwatch. I asked EE to verify the circuit, I think the clock is internal to chip.

Correction... 78 / 60 = 1.3

[mranostay]

Ok just surprising since this is a very common LED driver chip. Shame there isn't a test pad on the chip.. Of course I assume this has been tested on multiple board right? To verify the PCA6932 chips doesn't have a bad crystal (seems 400 khz from the docs)?

[networkimprov]

Let's make the timing correction a dts parameter, in case the chip behaves differently in some other electrical configuration.

Every board I've timed (admittedly a handful) has the same issue.

[networkimprov]

Point me to location of anode side of R151.

See pandrw.pdf in MFG zip.

Why does Q4B & Q5A connection to red LED have diff numbered gates?

The Q's come 2 per chip with pins 1-6. (Those two are connected to diff LEDs, btw.)

Pls don't reply direct to me by email on issue threads; I may not see it!

[mranostay]

Ok had to check this out with the logic analyzer myself :), and yeah I can confirm it is blinking fast. Updated the values to make the period length longer, and confirmed with LA that is the closest to 1 Hz as a we can get with the frequency steps.

Please review -> https://github.com/networkimprov/arch-packages/commit/ef483c8b70c95286c8630d49a203619e980379ea

[networkimprov]

Erm, I think that was more investigation than nec! I timed a range of on/off values when I discovered the fast blinking, they were all 1.3x too fast.

When you tackle the other LED driver issues, I think we should fix this in the driver, with the ratio set by dts field. I'd have changed the script values already if I liked that solution :-)

[mranostay]

On Mon, Sep 12, 2016 at 4:24 PM, Liam notifications@github.com wrote:

Erm, I think that was more investigation than nec! I timed a range of on/off values when I discovered the fast blinking, they were all 1.3x too fast.

When you tackle the other LED driver issues, I think we should fix this in the driver, with the ratio set by dts field. I'd have changed the script values already if I liked that solution :-)

Ok makes sense :). Should email the maintainer of that driver to see if he has seen weird behavior as well. Are we really sure this weirdness is linear enough to have a simple scaling value?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/networkimprov/arch-packages/issues/34#issuecomment-246526761, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwXOfTkgZ7V60Cn6godlGLGNnv8noa-ks5qpd8agaJpZM4DSn2s .

[networkimprov]

I don't think there's a maintainer. We wrote the patches to use the blink feature in 2013, and it hasn't seen many since. (The log is truncated because the file was renamed.) Maybe no one else is blinking it. Let's start with a single ratio, and revise if nec.

But later, charger and gauge are higher priority.

[mranostay]

@networkimprov Was wondering are we sure we don't have a batch of counterfeit ICs? Won't be the first time I've seen it...

[networkimprov]

Two diff builds with diff sourcing had this issue. Both sources were reputable...

[mranostay]

Changes ready for review @tmlind @networkimprov

https://github.com/networkimprov/linux/commits/anvl-v4.7-leds

[tmlind]

Looks OK to me for the scaling

[networkimprov]

Let's use a DTS param instead of a module constant.

Shouldn't it scale both time_on & time_off wherever they're set? Perhaps with a macro. As is, this calculation has only one scaled input: gdc = (time_on * 256) / period;

[mranostay]

On Mon, Sep 26, 2016 at 3:33 PM, Liam notifications@github.com wrote:

Let's use a DTS param instead of a module constant.

Ok no issue with that.

Shouldn't it scale both time_on & time_off wherever they're set? Perhaps with a macro.

Yeah that is going to confuse anyone in userspace why the value is going to be different than they wrote, and frankly they shouldn't care since it will be corrected.

As is, this calculation has only one scaled input: gdc = (time_on * 256) / period;

Ok time_on should be scaled. Although duty_cycle seems to be sane from stopwatching it. No gfrq = (period * 24 / 1000) - 1; is scaled because the step increase is roughy 30%

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/networkimprov/arch-packages/issues/34#issuecomment-249717240, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwXOQaPJtS7e9HjTEqlTE8BTI123WwHks5quEgqgaJpZM4DSn2s .

[networkimprov]

that is going to confuse anyone in userspace why the value is going to be different

Good point. So how bout a macro to scale period in the equations instead of changing period?

gdc = (SCALE(time_on) * 256) / SCALE(period);
gfrq = (SCALE(period) * 24 / 1000) - 1;

[mranostay]

On Mon, Sep 26, 2016 at 4:05 PM, Liam notifications@github.com wrote:

that is going to confuse anyone in userspace why the value is going to be different

Good point. So how bout a macro to scale period in the equations instead of changing period?

Well it is cleaner to change the period imho. and the period value doesn't leave the function scope.

gdc = (SCALE(time_on) * 256) / SCALE(period); gfrq = (SCALE(period) * 24 / 1000) - 1;

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/networkimprov/arch-packages/issues/34#issuecomment-249723119, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwXOUDVPOcR3fmvqQf_wiDx1HwGnmUfks5quE-xgaJpZM4DSn2s .

[networkimprov]

OK

[mranostay]

Reworked with DT property, and scaling helper function @tmlind @networkimprov

https://github.com/networkimprov/linux/tree/anvl-v4.7-leds-dt

[networkimprov]

This one sets time_on which you avoided previously to protect userspace values. Maybe inline the helper? { return scale ? ... : val; }

Re docs, "some chips gives incorrect group blinking..."

In some configurations, the chip blinks faster than expected. This parameter
provides a scaling ratio (x1000) to compensate, e.g. 1300=1.3x and 750=0.75x.

[mranostay]

@networkimprov Ah good catch.... not sure how I missed that. Will fix the time_on... Inlining sometimes gets shot down on the list because "the compiler should be smart enough", but the tenary logic makes sense to do in this case

[networkimprov]

Maybe p..._scale(pca963x_led* p, unsigned int val) and have it do p->chip->chipdef->scaling?

[mranostay]

Version 2 ready for review -> https://github.com/networkimprov/linux/commit/21c56b62a81ab90d8061f754cd11224ecb9efdbd

[networkimprov]

I was thinking pass pca963x from blink_set vs led_cdev :-) Hope I'm not nitpicking...

[mranostay]

On Tue, Sep 27, 2016 at 3:53 PM, Liam notifications@github.com wrote:

I was thinking pass pca963x from blink_set vs led_cdev :-) Hope I'm not nitpicking...

Yeah was trying to keep it similar to the other functions in the driver. But I guess pca963x_blink() is doing it as well.. will fix in v3

EDIT: version 3 https://github.com/networkimprov/linux/commit/937bceb904e2e97e58d41480ddb1b21c0e3fcb7c

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/networkimprov/arch-packages/issues/34#issuecomment-250022680, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwXOeYeISZam-LdCTVmjoqF_fF7NdoNks5quZ5xgaJpZM4DSn2s .

[networkimprov]

k, looks good.

[mranostay]

Should we attempt to upstream these changes?

[networkimprov]

There's more stuff to do on this driver first.

[mranostay]

@networkimprov @tmlind About the suspend/resume does the i2c rail get power down on suspend (VAUX2)? I suspect not if we need proper handling.

@tmlind Do you think we may get any push back on the mailing list... since in theory somebody may want to program a LED sequence on suspend, and this maybe should handle in userspace?

[networkimprov]

If vaux2 doesn't turn off in suspend; we need to change that. To stay on in suspend, you could set keep-power-in-suspend in dts, which we do for wifi.

probe() clears the chip, and remove() should too.

Something else in kernel must turn on green during cold boot.

[mranostay]

U-boot is actually setting it IIRC. Doubt the power on reset value enables the green LED only.

@networkimprov Actually keep-power-in-suspend is sdio/mmc only thing. Will check the PMIC drivers suspend flow to confirm VAUX2

[mranostay]

@networkimprov @tmlind confirmed that VAUX2 doesn't turn off on suspend which is probably by design. I confirmed this by configuring the LED blinking rate, then suspending and resuming.

Although we actually don't have to save the state we can put the PCA9632 into a low power state by setting the respective bit in MODE2 register. Then it will use less than a microamp according to datasheet

[networkimprov]

That sounds good if it turns off the LEDs?

[mranostay]

@networkimprov @tmlind Tested... of course really hard to test if it the power savings are real but should be (tm).

Maybe will need to add a device tree property for upstream to avoid breaking other systems that expect no suspend.

EDIT: https://github.com/networkimprov/linux/commit/11795438f254226667f187281539ee39941421fc

[networkimprov]

When you post anything to mailing list on our behalf, pls drop a link here. I like http://lkml.org/ but it's been down for weeks. Best alternative?

I plan to set the LED to amber on reboot via systemd service, so we shouldn't clear it during reboot.

[networkimprov]

Can also upstream time scaling patch!

[mranostay]

On Fri, Sep 30, 2016 at 12:42 PM, Liam notifications@github.com wrote:

When you post anything to mailing list on our behalf, pls drop a link here. I like http://lkml.org/ but it's been down for weeks. Best alternative?

I usually use spinics:

http://www.spinics.net/lists/kernel/msg2354085.html

I plan to set the LED to amber on reboot via systemd service, so we shouldn't clear it during reboot.

Ok need to check that.. I think the LED subsystem sets the LED brightness to zero on an unregister..

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/networkimprov/arch-packages/issues/34#issuecomment-250834756, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwXOWwsGQ5LEEbTbYh_H75h_w4lOe_aks5qvWYfgaJpZM4DSn2s .

[networkimprov]

Previously the chip kept its state during reboot, but the behavior you describe is a more reasonable default; albeit not for us.

[mranostay]

On Fri, Sep 30, 2016 at 12:58 PM, Matt Ranostay mranostay@gmail.com wrote:

On Fri, Sep 30, 2016 at 12:42 PM, Liam notifications@github.com wrote:

When you post anything to mailing list on our behalf, pls drop a link here. I like http://lkml.org/ but it's been down for weeks. Best alternative?

I usually use spinics:

http://www.spinics.net/lists/kernel/msg2354085.html

I plan to set the LED to amber on reboot via systemd service, so we shouldn't clear it during reboot.

So we got feedback on this from the list. So the LED_CORE_SUSPENDRESUME makes sense.

But was wondering if we care for me to add Runtime PM to put the device in low power state. LED subsystem is already turning the LEDS off with the LED_CORE_SUSPENDRESUME flag.

But low power state turn off the oscillator so the power goes to 1 uA versus the ~38 uA with it on. So do we care about such small power consumption?

Ok need to check that.. I think the LED subsystem sets the LED brightness to zero on an unregister..

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/networkimprov/arch-packages/issues/34#issuecomment-250834756, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwXOWwsGQ5LEEbTbYh_H75h_w4lOe_aks5qvWYfgaJpZM4DSn2s .

[networkimprov]

We don't care about that much power at this point.

Did you try suspend/resume before you applied patch? Sounds like it should have had correct behavior then...

[mranostay]

On Fri, Sep 30, 2016 at 4:50 PM, Liam notifications@github.com wrote:

We don't care about that much power at this point.

Did you try suspend/resume before you applied patch? Sounds like it should have had correct behavior then...

Yes with the LED_CORE_SUSPENDRESUME flag being set it does turn off the LEDs on suspend, and sets them again on resume. However without LED_CORE_SUSPENDRESUME it keeps blinking as expected, and this is all from the led subsystem handling it.

The suspend/resume function only turns off the oscillator which really isn't much of a power usage.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/networkimprov/arch-packages/issues/34#issuecomment-250877137, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwXOVu3HEr4QLzU3wVnYnUE0CODvg2Tks5qvaBFgaJpZM4DSn2s .

[networkimprov]

Well that's progress, all that work replaced by one flag :-) Guess we can defer further work on this feature, and push a flag patch to anvl-v4.7-leds-dt

[networkimprov]

Let's make led_core_suspendresume optional via dts param. Other flags should be settable by dts params as well: https://github.com/networkimprov/linux/blob/anvl-v4.7/include/linux/leds.h#L43

[mranostay]

On Sun, Oct 2, 2016 at 12:14 PM, Liam notifications@github.com wrote:

Let's make led_core_suspendresume optional via dts param. Other flags should be settable by dts params as well: https://github.com/networkimprov/linux/blob/anvl- v4.7/include/linux/leds.h#L43

Which other flags would we need to be configurable?

EDIT: seems the power management is only one I can see far... almost worth it to have it in the led subsystem.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/networkimprov/arch-packages/issues/34#issuecomment-250989275, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwXOb3GkaS7zGMDSt1no0HxM0OHAI4wks5qwAJ-gaJpZM4DSn2s .