Re: [PATCH 1/2] leds: cpcap: new driver
From: Jacek Anaszewski
Date: Mon Mar 06 2017 - 16:42:18 EST
Hi Sebastian,
Thanks for the patch. I have few comments in the code below.
On 03/05/2017 06:22 PM, Sebastian Reichel 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.
>
> Signed-off-by: Sebastian Reichel <sre@xxxxxxxxxx>
> ---
> .../devicetree/bindings/leds/cpcap-leds.txt | 29 +++
> drivers/leds/Kconfig | 9 +
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-cpcap.c | 248 +++++++++++++++++++++
> 4 files changed, 287 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/leds/cpcap-leds.txt
> create mode 100644 drivers/leds/leds-cpcap.c
>
> diff --git a/Documentation/devicetree/bindings/leds/cpcap-leds.txt b/Documentation/devicetree/bindings/leds/cpcap-leds.txt
> new file mode 100644
> 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)
> +- label: The label for this LED
Please change the property description to the following:
see Documentation/devicetree/bindings/leds/common.txt
> +- vdd-supply: A phandle to the regulator powering the LED
> +
> +Example:
> +
> +&cpcap {
> + cpcap_led_red: red-led {
> + compatible = "motorola,cpcap-led-red";
> + label = "cpcap:red";
> + vdd-supply = <&sw5>;
> + };
> +};
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> 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.
> +
> config LEDS_LM3530
> tristate "LCD Backlight driver for LM3530"
> depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 6b8273736478..333e84ce5d3b 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_LEDS_AAT1290) += leds-aat1290.o
> obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o
> obj-$(CONFIG_LEDS_BCM6358) += leds-bcm6358.o
> obj-$(CONFIG_LEDS_BD2802) += leds-bd2802.o
> +obj-$(CONFIG_LEDS_CPCAP) += leds-cpcap.o
> obj-$(CONFIG_LEDS_LOCOMO) += leds-locomo.o
> obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o
> obj-$(CONFIG_LEDS_LM3533) += leds-lm3533.o
> diff --git a/drivers/leds/leds-cpcap.c b/drivers/leds/leds-cpcap.c
> new file mode 100644
> index 000000000000..e2fe13aae2f1
> --- /dev/null
> +++ b/drivers/leds/leds-cpcap.c
> @@ -0,0 +1,248 @@
> +/*
> + * Copyright (c) 2017 Sebastian Reichel <sre@xxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#define DEBUG
This is stray debug macro I believe.
> +
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/mfd/motorola-cpcap.h>
Please sort these directives alphabetically. I.e. the last entry
should go after linux/leds.h.
> +
> +#define CPCAP_LED_NO_CURRENT 0x0001
> +#define CPCAP_LED_OFF 0x0000
> +
> +struct cpcap_led_info {
> + u16 reg;
> + u16 mask;
> + u16 limit;
> + u16 init_mask;
> + u16 init_val;
> +};
> +
> +static const struct cpcap_led_info cpcap_led_red = {
> + .reg = CPCAP_REG_REDC,
> + .mask = 0x03FF,
> + .limit = 31,
> +};
> +
> +static const struct cpcap_led_info cpcap_led_green = {
> + .reg = CPCAP_REG_GREENC,
> + .mask = 0x03FF,
> + .limit = 31,
> +};
> +
> +static const struct cpcap_led_info cpcap_led_blue = {
> + .reg = CPCAP_REG_BLUEC,
> + .mask = 0x03FF,
> + .limit = 31,
> +};
> +
> +/* aux display light */
> +static const struct cpcap_led_info cpcap_led_adl = {
> + .reg = CPCAP_REG_ADLC,
> + .mask = 0x000F,
> + .limit = 1,
> + .init_mask = 0x7FFF,
> + .init_val = 0x5FF0,
> +};
> +
> +/* camera privacy led */
> +static const struct cpcap_led_info cpcap_led_cp = {
> + .reg = CPCAP_REG_CLEDC,
> + .mask = 0x0007,
> + .limit = 1,
> + .init_mask = 0x03FF,
> + .init_val = 0x0008,
> +};
> +
> +/* unsupported led */
> +static const struct cpcap_led_info cpcap_led_unsup = {
> + .reg = 0x0000,
> +};
> +
> +struct cpcap_led {
> + struct led_classdev led;
> + const struct cpcap_led_info *info;
> + struct device *dev;
> + struct regmap *regmap;
> + struct regulator *vdd;
> + bool powered;
> +
> + u32 current_limit;
> +};
> +
> +static u16 cpcap_led_val(u8 current_limit, u8 duty_cycle)
> +{
> + current_limit &= 0x1f; /* 5 bit */
> + duty_cycle &= 0x0f; /* 4 bit */
> +
> + return current_limit << 4 | duty_cycle;
> +}
> +
> +static int cpcap_led_set_power(struct cpcap_led *led, bool status)
> +{
> + int err;
> +
> + if (status == led->powered)
> + return 0;
> +
> + if (status)
> + err = regulator_enable(led->vdd);
> + else
> + err = regulator_disable(led->vdd);
> +
> + if (err) {
> + dev_err(led->dev, "regulator failure: %d", err);
> + return err;
> + }
> +
> + led->powered = status;
> +
> + return 0;
> +}
> +
> +static int cpcap_led_set(struct led_classdev *ledc, enum led_brightness value)
> +{
> + struct cpcap_led *led = container_of(ledc, struct cpcap_led, led);
> + int brightness;
> + int err;
You need mutex protection here so as not to leave the device in an
inconsistent state when concurrent access occurs.
The operation of brightness setting is not atomic here.
> + if (value > LED_OFF) {
> + err = cpcap_led_set_power(led, true);
> + if (err)
> + return err;
> + }
> +
> + if (value == LED_OFF) {
> + /* Avoid HW issue by turning off current before duty cycle */
> + err = regmap_update_bits(led->regmap,
> + led->info->reg, led->info->mask, CPCAP_LED_NO_CURRENT);
> + if (err) {
> + dev_err(led->dev, "regmap failed: %d", err);
> + return err;
> + }
> +
> + brightness = CPCAP_LED_OFF;
> + } else {
> + brightness = cpcap_led_val(value, 0x01);
Please add a macro for 0x01.
> + }
> +
> + dev_dbg(led->dev, "regmap: reg=%04x mask=%04x val=%04x",
> + led->info->reg, led->info->mask, brightness);
Please remove this debug logging. You can always enable CONFIG_DEBUG_FS.
> + err = regmap_update_bits(led->regmap, led->info->reg, led->info->mask,
> + brightness);
> + if (err) {
> + dev_err(led->dev, "regmap failed: %d", err);
> + return err;
> + }
> +
> + if (value == LED_OFF) {
Why can't we accomplish this in the second condition?
> + err = cpcap_led_set_power(led, false);
> + if (err)
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id cpcap_led_of_match[] = {
> + { .compatible = "motorola,cpcap-led-red", .data = &cpcap_led_red },
> + { .compatible = "motorola,cpcap-led-green", .data = &cpcap_led_green },
> + { .compatible = "motorola,cpcap-led-blue", .data = &cpcap_led_blue },
> + { .compatible = "motorola,cpcap-led-adl", .data = &cpcap_led_adl },
> + { .compatible = "motorola,cpcap-led-cp", .data = &cpcap_led_cp },
> +
> + { .compatible = "motorola,cpcap-led-cf", .data = &cpcap_led_unsup },
> + { .compatible = "motorola,cpcap-led-mdl", .data = &cpcap_led_unsup },
> + { .compatible = "motorola,cpcap-led-kl", .data = &cpcap_led_unsup },
> + { .compatible = "motorola,cpcap-led-bt", .data = &cpcap_led_unsup },
Does it make sense to provide entries for unsupported LEDs?
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, cpcap_led_of_match);
> +
> +static int cpcap_led_probe(struct platform_device *pdev)
> +{
> + const struct of_device_id *match;
> + struct cpcap_led *led;
> + int err;
> +
> + match = of_match_device(of_match_ptr(cpcap_led_of_match), &pdev->dev);
> + if (!match || !match->data)
> + return -EINVAL;
> +
> + led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
> + if (!led)
> + return -ENOMEM;
> + platform_set_drvdata(pdev, led);
> + led->info = match->data;
> + led->dev = &pdev->dev;
> +
> + if (led->info->reg == 0x0000) {
> + dev_err(led->dev, "Unsupported LED");
> + return -ENODEV;
> + }
> +
> + led->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> + if (!led->regmap)
> + return -ENODEV;
> +
> + led->vdd = devm_regulator_get(&pdev->dev, "vdd");
> + if (IS_ERR(led->vdd)) {
> + err = PTR_ERR(led->vdd);
> + dev_err(led->dev, "Couldn't get regulator: %d", err);
> + return err;
> + }
> +
> + err = device_property_read_string(&pdev->dev, "label", &led->led.name);
> + if (err) {
> + dev_err(led->dev, "Couldn't read led label: %d", err);
> + return err;
> + }
> +
> + led->led.max_brightness = led->info->limit;
> +
> + led->led.brightness_set_blocking = cpcap_led_set;
> + err = devm_led_classdev_register(&pdev->dev, &led->led);
> + if (err) {
> + dev_err(led->dev, "Couldn't register led: %d", err);
> + return err;
> + }
> +
> + if (led->info->init_mask) {
> + err = regmap_update_bits(led->regmap, led->info->reg,
> + led->info->init_mask, led->info->init_val);
> + if (err) {
> + dev_err(led->dev, "regmap failed: %d", err);
> + return err;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static struct platform_driver cpcap_led_driver = {
> + .probe = cpcap_led_probe,
> + .driver = {
> + .name = "cpcap-led",
> + .of_match_table = cpcap_led_of_match,
> + },
> +};
> +module_platform_driver(cpcap_led_driver);
> +
> +MODULE_DESCRIPTION("CPCAP LED driver");
> +MODULE_AUTHOR("Sebastian Reichel <sre@xxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
Your copyright note claims "GPL v2".
--
Best regards,
Jacek Anaszewski