Re: [PATCH 1/5] leds: Add Intel Cherry Trail Whiskey Cove PMIC LED driver

[Hans de Goede <hdegoede@redhat.com>]

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@gmail.com>
>>
>> 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@gmail.com>
>> + * Copyright 2023 Hans de Goede <hansg@kernel.org>
>> + *
>> + * 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