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

[Jacek Anaszewski <j.anaszewski@samsung.com>]

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


-- 
Best Regards,
Jacek Anaszewski