[v2,19/21] leds: leds-pasic3: Add support for PASIC3 LED controller

Add support for LEDs inside the PASIC3 chip.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 drivers/leds/Kconfig       |   9 ++
 drivers/leds/Makefile      |   1 +
 drivers/leds/leds-pasic3.c | 232 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 242 insertions(+)
 create mode 100644 drivers/leds/leds-pasic3.c

Dne 18.8.2015 v 00:06 Petr Cvek napsal(a):
> Add support for LEDs inside the PASIC3 chip.

BTW During research I have found there was driver in 

	https://github.com/hackndev/linux-hnd 

but it did not make up to vanilla, so I wrote mine from scratch (actually I
have tested the PASIC3 registers mostly by trial and error - and found some
for case open reset).

Hi Petr,

On 08/18/2015 12:06 AM, Petr Cvek wrote:
> Add support for LEDs inside the PASIC3 chip.
>
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
> ---
>   drivers/leds/Kconfig       |   9 ++
>   drivers/leds/Makefile      |   1 +
>   drivers/leds/leds-pasic3.c | 232 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 242 insertions(+)
>   create mode 100644 drivers/leds/leds-pasic3.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 9ad35f7..516ba65 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -586,6 +586,15 @@ config LEDS_PM8941_WLED
>   	  This option enables support for the 'White' LED block
>   	  on Qualcomm PM8941 PMICs.
>
> +config LEDS_PASIC3
> +	tristate "LED support for the HTC Magician/Alpine PASIC3"
> +	depends on LEDS_CLASS
> +	depends on HTC_PASIC3
> +	select REGMAP
> +	help
> +	  This option enables support for the PASIC3 chip (different chip
> +	  than Compaq ASIC3).
> +
>   comment "LED Triggers"
>   source "drivers/leds/trigger/Kconfig"
>
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 8d6a24a..b1c659c 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -65,6 +65,7 @@ obj-$(CONFIG_LEDS_VERSATILE)		+= leds-versatile.o
>   obj-$(CONFIG_LEDS_MENF21BMC)		+= leds-menf21bmc.o
>   obj-$(CONFIG_LEDS_PM8941_WLED)		+= leds-pm8941-wled.o
>   obj-$(CONFIG_LEDS_KTD2692)		+= leds-ktd2692.o
> +obj-$(CONFIG_LEDS_PASIC3)		+= leds-pasic3.o
>
>   # LED SPI Drivers
>   obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
> diff --git a/drivers/leds/leds-pasic3.c b/drivers/leds/leds-pasic3.c
> new file mode 100644
> index 0000000..0f95230
> --- /dev/null
> +++ b/drivers/leds/leds-pasic3.c
> @@ -0,0 +1,232 @@
> +/*
> + * AIC3 (or PASIC3) LED driver for HTC Magician/Alpine/.. (not Compaq ASIC3)

s/AIC3/ASIC3/

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

> + *
> + * Copyright (c) 2015 Petr Cvek <petr.cvek@tul.cz>
> + *
> + * 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 <petr.cvek@tul.cz>");
> +MODULE_DESCRIPTION("HTC Magician PASIC3 LED driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:leds-pasic3");
>

Dne 18.8.2015 v 12:27 Jacek Anaszewski napsal(a):
> Hi Petr,
> 
> On 08/18/2015 12:06 AM, Petr Cvek wrote:
>> +/*
>> + * AIC3 (or PASIC3) LED driver for HTC Magician/Alpine/.. (not Compaq ASIC3)
> 
> s/AIC3/ASIC3/
> 
> I am a bit confused. What's the relation between ASIC3 and PASIC3?

Actually both drivers should be renamed to AIC3 (or AIC only, as there is version 2). "HTC_AIC3" is written on the chip and "pasic3" is confusing with compaq asic3 driver, which claims in Kconfig help to be compatible with HTC machines. Compaq ASIC3 has much more functionality and from brief look it seems it has 16 bit registers (HTC_AIC3 has only LEDs, RESET and DS1WM and has 8 bit registers).

>> +spinlock_t lock;
> 
> Please move it to the struct pasic3_leds_data.

OK

> 
>> +
>> +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.

> 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++;

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


Can the same brightness_set() function be called multiple times with same value (like LED_ON)?

I think I will change it to a different code altogether, power_gpio is shared variable and power can be shut off when two of three leds are disabled (green LED does not use power_gpio). Something like:

enabled_leds |= BIT(hw_num);

or

enabled_leds &= ~BIT(hw_num);

>> +        ret = gpio_request(pdata->power_gpio,
>> +            "Red-Blue LED Power");
> 
> Please use devm_gpio_request. Then you will not need pasic3_led_remove.
> 

OK

Am Dienstag, den 18.08.2015, 23:48 +0200 schrieb Petr Cvek:
> Dne 18.8.2015 v 12:27 Jacek Anaszewski napsal(a):
> > Hi Petr,
> > 
> > On 08/18/2015 12:06 AM, Petr Cvek wrote:
> > > +/*
> > > + * AIC3 (or PASIC3) LED driver for HTC Magician/Alpine/.. (not 
> > > Compaq ASIC3)
> > 
> > s/AIC3/ASIC3/
> > 
> > I am a bit confused. What's the relation between ASIC3 and PASIC3?
> 
> Actually both drivers should be renamed to AIC3 (or AIC only, as 
> there is version 2). "HTC_AIC3" is written on the chip and "pasic3" 
> is confusing with compaq asic3 driver, which claims in Kconfig help 
> to be compatible with HTC machines. Compaq ASIC3 has much more 
> functionality and from brief look it seems it has 16 bit registers 
> (HTC_AIC3 has only LEDs, RESET and DS1WM and has 8 bit registers).

I think I've got the PASIC3 name from debug strings present in the HTC
bootloader or WinCE binary. This chip (and its predecessor PASIC2 in
HTC Blueangel) are different from the ASIC3 found in hx4700 iPAQs.

regards
Philipp

On 08/18/2015 11:48 PM, Petr Cvek wrote:
> Dne 18.8.2015 v 12:27 Jacek Anaszewski napsal(a):
>> Hi Petr,
>>
>> On 08/18/2015 12:06 AM, Petr Cvek wrote:
>>> +/*
>>> + * AIC3 (or PASIC3) LED driver for HTC Magician/Alpine/.. (not Compaq ASIC3)
>>
>> s/AIC3/ASIC3/
>>
>> I am a bit confused. What's the relation between ASIC3 and PASIC3?
>
> Actually both drivers should be renamed to AIC3 (or AIC only, as there is version 2). "HTC_AIC3" is written on the chip and "pasic3" is confusing with compaq asic3 driver, which claims in Kconfig help to be compatible with HTC machines. Compaq ASIC3 has much more functionality and from brief look it seems it has 16 bit registers (HTC_AIC3 has only LEDs, RESET and DS1WM and has 8 bit registers).

I see, thanks for the explanation.

>>> +spinlock_t lock;
>>
>> Please move it to the struct pasic3_leds_data.
>
> OK
>
>>
>>> +
>>> +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.
>
>> 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++;
>
>>
>> Shouldn't you check if also power_gpio state doesn't need to be altered
>> here?
>>
>
>
> Can the same brightness_set() function be called multiple times with same value (like LED_ON)?

Yes it can. LED core doesn't check if the value isn't already set.

> I think I will change it to a different code altogether, power_gpio is shared variable and power can be shut off when two of three leds are disabled (green LED does not use power_gpio). Something like:
>
> enabled_leds |= BIT(hw_num);
>
> or
>
> enabled_leds &= ~BIT(hw_num);

OK, we'll discuss that after you'll submit the next version.

>
>>> +        ret = gpio_request(pdata->power_gpio,
>>> +            "Red-Blue LED Power");
>>
>> Please use devm_gpio_request. Then you will not need pasic3_led_remove.
>>
>
> OK
>
>