Hi Petr,

On 08/18/2015 12:06 AM, Petr Cvek wrote:

s/AIC3/ASIC3/

I am a bit confused. What's the relation between ASIC3 and PASIC3?

+ *
+ * Copyright (c) 2015 Petr Cvek [off-list ref]
+ *
+ * CLK to MS calculation based on leds-asic3.c
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under  the terms of the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ */
+
+#include <linux/gpio.h>
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/mfd/htc-pasic3.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/*
+ * 1 tick is around 62-63 ms, blinking settings (on+off):
+ *	Min: 1+1 ticks ~125ms
+ *	Max: 127+127 ticks ~15 875ms
+ * HW sometimes takes time to change (counter overflow after write?)
+ */
+
+#define MS_TO_CLK(ms)	DIV_ROUND_CLOSEST(((ms)*1024), 64000)
+#define CLK_TO_MS(clk)	(((clk)*64000)/1024)
+#define MAX_CLK		254		/* 127 + 127 (2x 7 bit reg) */
+#define MAX_MS		CLK_TO_MS(MAX_CLK)
+
+spinlock_t lock;

Please move it to the struct pasic3_leds_data.

+
+static void brightness_set(struct led_classdev *cdev,
+	enum led_brightness value)
+{
+	struct platform_device *pdev = to_platform_device(cdev->dev->parent);
+	struct pasic3_led *led = dev_get_platdata(cdev->dev);
+	struct pasic3_leds_pdata *pdata = dev_get_platdata(&pdev->dev);
+	unsigned long flags;
+	u8 val;
+
+	spin_lock_irqsave(&lock, flags);
+
+	if (value == LED_OFF) {
+		val = pasic3_read_register(pdata->mfd_dev, PASIC3_CH_CONTROL);
+		pasic3_write_register(pdata->mfd_dev, PASIC3_CH_CONTROL,
+			val & ~(led->bit_blink_en | led->bit_force_on));
+
+		val = pasic3_read_register(pdata->mfd_dev, PASIC3_MASK_A);
+		pasic3_write_register(pdata->mfd_dev, PASIC3_MASK_A,
+			val & ~led->bit_mask);
+
+		if (pdata->power_gpio) {
+			pdata->power_gpio_users--;

Now it is possible that power_gpio_users will have negative value.
You should check the value before decrementing it.

+
+			if (pdata->power_gpio_users == 0)
+				gpio_set_value(pdata->power_gpio, 0);
+		}
+	} else {
+		if (pdata->power_gpio) {
+			pdata->power_gpio_users++;
+
+			if (pdata->power_gpio_users != 0)
+				gpio_set_value(pdata->power_gpio, 1);
+		}

Now you are setting power_gpio_users to 1 each time brightness is set,
whereas it should be set to 1 only if it was 0.

This could be changed to:

if (pdata->power_gpio_users == 0)
	gpio_set_value(pdata->power_gpio, 1);

pdata->power_gpio_users++;

+
+		val = pasic3_read_register(pdata->mfd_dev, PASIC3_CH_CONTROL);
+		pasic3_write_register(pdata->mfd_dev, PASIC3_CH_CONTROL,
+			val | led->bit_force_on);
+	}
+
+	spin_unlock_irqrestore(&lock, flags);
+}
+
+static int blink_set(struct led_classdev *cdev,
+	unsigned long *delay_on, unsigned long *delay_off)
+{
+	struct platform_device *pdev = to_platform_device(cdev->dev->parent);
+	struct pasic3_led *led = dev_get_platdata(cdev->dev);
+	struct pasic3_leds_pdata *pdata = dev_get_platdata(&pdev->dev);
+	unsigned long flags;
+	u32 on, off;
+	u8 val;
+	if (*delay_on > MAX_MS || *delay_off > MAX_MS)
+		return -EINVAL;
+
+	if (*delay_on == 0 && *delay_off == 0) {
+		/* Choose sensible default delays */
+		on = MS_TO_CLK(500);
+		off = MS_TO_CLK(500);
+	} else {
+		on = MS_TO_CLK(*delay_on);
+		off = MS_TO_CLK(*delay_off);
+		if ((on + off) > MAX_CLK)
+			return -EINVAL;
+		/* Minimal on/off value must be 1 */
+		on = on ? on : 1;
+		off = off ? off : 1;
+	}
+
+	spin_lock_irqsave(&lock, flags);

Shouldn't you check if also power_gpio state doesn't need to be altered
here?

+
+	pasic3_write_register(pdata->mfd_dev, led->reg_delay_on, on);
+	pasic3_write_register(pdata->mfd_dev, led->reg_delay_off, off);
+
+	val = pasic3_read_register(pdata->mfd_dev, PASIC3_CH_CONTROL);
+	pasic3_write_register(pdata->mfd_dev, PASIC3_CH_CONTROL,
+		val | led->bit_blink_en);
+
+	spin_unlock_irqrestore(&lock, flags);
+
+	*delay_on = CLK_TO_MS(on);
+	*delay_off = CLK_TO_MS(off);
+
+	return 0;
+}
+
+static int pasic3_led_probe(struct platform_device *pdev)
+{
+	struct pasic3_leds_pdata *pdata = dev_get_platdata(&pdev->dev);
+	struct led_classdev *cdev;
+	int ret;
+	int i;
+
+	if (!pdata) {
+		dev_err(&pdev->dev, "no platform data\n");
+		return -EINVAL;
+	}
+
+	if (!pdata->leds) {
+		dev_err(&pdev->dev, "no leds data\n");
+		return -EINVAL;
+	}
+
+	if (pdata->power_gpio) {
+		pdata->power_gpio_users = 0;
+
+		ret = gpio_request(pdata->power_gpio,
+			"Red-Blue LED Power");

Please use devm_gpio_request. Then you will not need pasic3_led_remove.

+		if (ret < 0) {
+			dev_warn(&pdev->dev, "failed to request power GPIO\n");
+			goto err;
+		}
+	}
+
+	spin_lock_init(&lock);
+
+	cdev = devm_kzalloc(&pdev->dev,
+		sizeof(struct led_classdev) * pdata->num_leds, GFP_KERNEL);
+	if (!cdev) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	for (i = 0; i < pdata->num_leds; i++) {
+		cdev[i].name = pdata->leds[i].name;
+		cdev[i].default_trigger = pdata->leds[i].default_trigger;
+		cdev[i].flags = LED_CORE_SUSPENDRESUME;
+		cdev[i].brightness_set = brightness_set;
+		cdev[i].blink_set = blink_set;
+
+		ret = devm_led_classdev_register(&pdev->dev, &cdev[i]);
+		if (ret < 0)
+			goto err;
+
+		cdev[i].dev->platform_data = &pdata->leds[i];
+	}
+
+	return 0;
+
+err:
+	if (pdata->power_gpio)
+		gpio_free(pdata->power_gpio);
+
+	return ret;
+}
+
+static int pasic3_led_remove(struct platform_device *pdev)
+{
+	struct pasic3_leds_pdata *pdata = dev_get_platdata(&pdev->dev);
+
+	if (pdata->power_gpio)
+		gpio_free(pdata->power_gpio);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int pasic3_led_suspend(struct device *dev)
+{
+	struct pasic3_leds_pdata *pdata = dev_get_platdata(dev);
+
+	if (pdata->power_gpio)
+		gpio_set_value(pdata->power_gpio, 0);
+
+	return 0;
+}
+
+static int pasic3_led_resume(struct device *dev)
+{
+	struct pasic3_leds_pdata *pdata = dev_get_platdata(dev);
+
+	if (pdata->power_gpio)
+		gpio_set_value(pdata->power_gpio, 1);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(pasic3_led_pm_ops, pasic3_led_suspend,
+	pasic3_led_resume);
+
+static struct platform_driver pasic3_led_driver = {
+	.probe		= pasic3_led_probe,
+	.remove		= pasic3_led_remove,
+	.driver		= {
+		.name	= "leds-pasic3",
+		.pm	= &pasic3_led_pm_ops,
+	},
+};
+
+module_platform_driver(pasic3_led_driver);
+
+MODULE_AUTHOR("Petr Cvek [off-list ref]");
+MODULE_DESCRIPTION("HTC Magician PASIC3 LED driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:leds-pasic3");

-- 
Best Regards,
Jacek Anaszewski