Hi Pavel,
Thank you for reviewing this.
On 4/14/23 14:44, Pavel Machek wrote:
> On Thu 2023-04-13 17:18:04, Hans de Goede wrote:
>> From: Yauhen Kharuzhy <jekhor@xxxxxxxxx>
>>
>> Add support for LEDs connected to the Intel Cherry Trail Whiskey Cove
>> PMIC. Charger and general-purpose leds are supported. Hardware blinking
>> is implemented, breathing is not.
>
> leds->LEDs.
>
>> diff --git a/drivers/leds/leds-cht-wcove.c b/drivers/leds/leds-cht-wcove.c
>> new file mode 100644
>> index 000000000000..06447804d050
>> --- /dev/null
>> +++ b/drivers/leds/leds-cht-wcove.c
>> @@ -0,0 +1,368 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Driver for LEDs connected to the Intel Cherry Trail Whiskey Cove PMIC
>> + *
>> + * Copyright 2019 Yauhen Kharuzhy <jekhor@xxxxxxxxx>
>> + * Copyright 2023 Hans de Goede <hansg@xxxxxxxxxx>
>> + *
>> + * Based on Lenovo Yoga Book Android kernel sources
>> + */
>
> "sources." Should copyrights from Android be copied here?
>
>> +static const char * const cht_wc_leds_names[CHT_WC_LED_COUNT] = {
>> + "cht-wc::" LED_FUNCTION_CHARGING,
>> + "cht-wc::" LED_FUNCTION_INDICATOR,
>> +};
>
> No need for "cht-wc".
I should put something there right, iow not just
"::" LED_FUNCTION_CHARGING" if it should not be
cht-wc should I put "platform::" there then, or
maybe "pmic::" ?
> Mention color.
This PMIC is used in multiple designs at
least both white and red LEDs are used in
2 designs I know about. AFAIK the norm is
to leave out the color if it is different
per design?
> At least charging LED is
> going to be common - document in Documentation/leds/well-known-leds.txt.
That already has:
* Power management
Good: "platform:*:charging" (allwinner sun50i)
So you want me to extend the (allwinner sun50i) bit with ", cht-wc" ?
Ack to all the other comments, will fix for v2.
Regards,
Hans
>
>> +static int cht_wc_leds_brightness_set(struct led_classdev *cdev,
>> + enum led_brightness value)
>> +{
>> + struct cht_wc_led *led = container_of(cdev, struct cht_wc_led, cdev);
>> + int ret;
>> +
>> + mutex_lock(&led->mutex);
>> +
>> + if (!value) {
>> + ret = regmap_update_bits(led->regmap, led->regs->ctrl,
>> + led->regs->on_off_mask, led->regs->off_val);
>> + if (ret)
>> + dev_err(cdev->dev, "Failed to turn off: %d\n", ret);
>> +
>> + /* Disable HW blinking */
>> + ret = regmap_update_bits(led->regmap, led->regs->fsm,
>> + CHT_WC_LED_EFF_MASK, CHT_WC_LED_EFF_ON);
>> + if (ret < 0)
>> + dev_err(cdev->dev, "Failed to update LED FSM reg: %d\n", ret);
>
> This overwrites previous error. Not sure if it is big deal.
>
>> +static int cht_wc_leds_blink_set(struct led_classdev *cdev,
>> + unsigned long *delay_on,
>> + unsigned long *delay_off)
>> +{
>> + struct cht_wc_led *led = container_of(cdev, struct cht_wc_led, cdev);
>> + unsigned int ctrl;
>> + int ret;
>> +
>> + mutex_lock(&led->mutex);
>> +
>> + if (!*delay_on && !*delay_off)
>> + *delay_on = *delay_off = 1000;
>
> That's really slow blink; I'd do something faster by default.
>
>> + /*
>> + * LED1 might be in hw-controlled mode when this driver gets loaded; and
>> + * since the PMIC is always powered by the battery any changes made are
>> + * permanent. Save LED1 regs to restore them on remove() or shutdown().
>> + */
>
> Fun :-).
>
> Thank you.
>
> Best regards,
> Pavel
