Re: [PATCH 1/2] leds: cpcap: new driver

[Sebastian Reichel]

Hi,

On Mon, Mar 06, 2017 at 11:11:47PM +0100, Pavel Machek wrote:
> > Motorola CPCAP is a PMIC found in multiple smartphones.
> > This driver adds support for the chip's LED controllers.
> > It has explicit support for all controllers used by the
> > Droid 4. Since no datasheets are available the other
> > available controllers are not supported until somebody
> > verified, that the register layout matches.
> 
> This of course leads me to two questions:
> 
> 1) Where can I get Droid 4?

I got a used one on Ebay for 42â incl. shipping & customs. The trick
is clicking the worldwide option, since they are pretty expensive in
EU (they only exist with US LTE modem, so they were not sold here
officially).

> 2) How well is it supported?

UART + WLAN works with mainline master branch. As written by Tony
we have a couple of patches ready for 4.12. Big open tasks are the
cameras and the modems. Cameras are handled via co-processor in the
stock system (that's about all I know about them so far) and modems are
connected via USB + GPIOs (and for the 2G/3G modem an additional UART).
LTE modem support seems simple (USB-CDC based), but does not work
in EU and 2G/3G looks like much work. Modem voice support will be
simpler than on N900, though (data goes directly to the audio codec).
Speaking about audio codec: I'm currently working on that.

> > index 000000000000..d523f8c3c358
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/cpcap-leds.txt
> > @@ -0,0 +1,29 @@
> > +Motorola CPCAP PMIC LEDs
> > +------------------------
> > +
> > +This module is part of the CPCAP. For more details about the whole
> > +chip see Documentation/devicetree/bindings/mfd/motorola-cpcap.txt.
> > +
> > +Requires node properties:
> > +- compatible: should be one of
> > +   * "motorola,cpcap-led-mdl"		(Main Display Lighting)
> > +   * "motorola,cpcap-led-kl"		(Keyboard Lighting)
> > +   * "motorola,cpcap-led-adl"		(Aux Display Lighting)
> > +   * "motorola,cpcap-led-red"		(Red Triode)
> > +   * "motorola,cpcap-led-green"		(Green Triode)
> > +   * "motorola,cpcap-led-blue"		(Blue Triode)
> > +   * "motorola,cpcap-led-cf"		(Camera Flash)
> > +   * "motorola,cpcap-led-bt"		(Bluetooth)
> > +   * "motorola,cpcap-led-cp"		(Camera Privacy LED)
> 
> BTW. Does the RGB controller support any kind of "patterns" similar
> to what n900 can do?

No. Motorola CPCAP has simple blink support for the RGB leds, though.
It can potentially save some CPU cycles, but I did not yet add support
for that. CPCAP also has a few more LED interfaces, that are unused
on Droid 4.

> > +&cpcap {
> > +	cpcap_led_red: red-led {
> > +		compatible = "motorola,cpcap-led-red";
> > +		label = "cpcap:red";
> > +		vdd-supply = <&sw5>;
> > +	};
> > +};
> 
> This should be copied to the device tree people.

They are already in CC.

> > index 275f467956ee..043f02a4fe73 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -76,6 +76,15 @@ config LEDS_BCM6358
> >  	  This option enables support for LEDs connected to the BCM6358
> >  	  LED HW controller accessed via MMIO registers.
> >  
> > +config LEDS_CPCAP
> > +	tristate "LED Support for Motorola CPCAP"
> > +	depends on LEDS_CLASS
> > +	depends on MFD_CPCAP
> > +	depends on OF
> > +	help
> > +	  This option enables support for LEDs offered by Motorola's
> > +	  CPCAP PMIC.
> > +
> 
> Umm. That help explains exactly what I oculd tell from the name. Can
> you spell out "CPCAP" and "PMIC"... and maybe mention that it is used
> on the Droid 4 phone?

PMIC = power management integrated circuit

https://en.wikipedia.org/wiki/Power_management_integrated_circuit

CPCAP = a chip similar to TWL6040.

CPCAP it used on multiple motorola smartphones, just like TWL6040
is often used as PMIC. Usually we do not add a list of boards using
some feature to config description (except when the option is only
useful for a single one).

> > +#define DEBUG
> 
> Remove for production?

Yes, thanks.

> > +	err = device_property_read_string(&pdev->dev, "label", &led->led.name);
> > +	if (err) {
> > +		dev_err(led->dev, "Couldn't read led label: %d", err);
> 
> s/led/LED/.

ok.

> 
> > +	if (err) {
> > +		dev_err(led->dev, "Couldn't register led: %d", err);
> > +		return err;
> 
> And here.

ok.

> Acked-by: Pavel Machek <pavel@xxxxxx>

Thanks.

-- Sebastian

Attachment: signature.asc
Description: PGP signature