[PATCH v8 09/12] leds: Add support for Palmas LEDs

[Kim, Milo]

> The Palmas familly of chips has LED support. This is not always muxed
> to output pins so depending on the setting of the mux this driver
> will create the appropriate LED class devices.

It looks good and I've added few comments.

> diff --git a/drivers/leds/leds-palmas.c b/drivers/leds/leds-palmas.c

> +static int palmas_leds_blink_set(struct led_classdev *led_cdev,
> +				   unsigned long *delay_on,
> +				   unsigned long *delay_off)
> +{
> +	struct palmas_led *led = to_palmas_led(led_cdev);
> +	unsigned long flags;
> +	int ret = 0;
> +	int period;
> +
> +	/* Pick some defaults if we've not been given times */
> +	if (*delay_on == 0 && *delay_off == 0) {
> +		*delay_on = 250;
> +		*delay_off = 250;
> +	}
> +
> +	spin_lock_irqsave(&led->value_lock, flags);
> +
> +	/*
> +	 * We only have a limited selection of settings, see if we can
> +	 * support the configuration we're being given
> +	 */
> +	switch (*delay_on) {
> +	case 500:
> +		led->on_time = LED_ON_500MS;
> +		break;
> +	case 250:
> +		led->on_time = LED_ON_250MS;
> +		break;
> +	case 125:
> +		led->on_time = LED_ON_125MS;
> +		break;
> +	case 62:
> +	case 63:
> +		/* Actually 62.5ms */
> +		led->on_time = LED_ON_62_5MS;
> +		break;
> +	default:
> +		ret = -EINVAL;

Invalid delay on time,
Can it return here quickly if updating the period is not mandatory?

> +static int palmas_leds_probe(struct platform_device *pdev)
> +{
> +	struct palmas *palmas = dev_get_drvdata(pdev->dev.parent);
> +	struct palmas_leds_platform_data *pdata = pdev->dev.platform_data;
> +	struct palmas_leds_data *leds_data;
> +	struct device_node *node = pdev->dev.of_node;
> +	int ret, i;
> +	int offset = 0;
> +
> +	if (!palmas->led_muxed && !is_palmas_charger(palmas->product_id))
> {
> +		dev_err(&pdev->dev, "there are no LEDs muxed\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Palmas charger requires platform data */
> +	if (is_palmas_charger(palmas->product_id) && node && !pdata) {
> +
> +		if (!pdata)
> +			return -ENOMEM;
> +
> +		ret = palmas_dt_to_pdata(&pdev->dev, node, pdata);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (is_palmas_charger(palmas->product_id) && !pdata)
> +		return -EINVAL;

In _probe(), is_palmas_charger() is called several times.
Any reason to get the charger at each time?
If it's not, just get once and store the value in the variable.
Then many calls can be replaced with it simply.

> +
> +	leds_data = devm_kzalloc(&pdev->dev, sizeof(*leds_data),
> GFP_KERNEL);
> +	if (!leds_data)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, leds_data);
> +
> +	leds_data->palmas = palmas;
> +
> +	switch (palmas->led_muxed) {
> +	case PALMAS_LED1_MUXED | PALMAS_LED2_MUXED:
> +		leds_data->no_leds = 2;
> +		break;
> +	case PALMAS_LED1_MUXED:
> +	case PALMAS_LED2_MUXED:
> +		leds_data->no_leds = 1;
> +		break;
> +	default:
> +		leds_data->no_leds = 0;
> +		break;
> +	}
> +
> +	if (is_palmas_charger(palmas->product_id)) {
> +		if (pdata->chrg_led_mode)
> +			leds_data->no_leds += 2;
> +		else
> +			leds_data->no_leds++;
> +	}
> +
> +	if (leds_data->no_leds == 0)
> +		leds_data->palmas_led = NULL;
> +	else
> +		leds_data = devm_kzalloc(&pdev->dev,
> +				leds_data->no_leds * sizeof(*leds_data),
> +				GFP_KERNEL);
> +
> +	/* Initialise LED1 */
> +	if (palmas->led_muxed & PALMAS_LED1_MUXED) {
> +		palmas_init_led(leds_data, offset, 1);
> +		offset++;
> +	}
> +
> +	/* Initialise LED2 */
> +	if (palmas->led_muxed & PALMAS_LED2_MUXED) {
> +		palmas_init_led(leds_data, offset, 2);
> +		offset++;
> +	}
> +
> +	if (is_palmas_charger(palmas->product_id)) {
> +		palmas_init_led(leds_data, offset, 3);
> +		offset++;
> +
> +		if (pdata->chrg_led_mode) {
> +			palmas_led_update_bits(leds_data,
> PALMAS_CHRG_LED_CTRL,
> +					PALMAS_CHRG_LED_CTRL_CHRG_LED_MODE,
> +					PALMAS_CHRG_LED_CTRL_CHRG_LED_MODE);
> +
> +			palmas_init_led(leds_data, offset, 4);
> +		}
> +	}
> +
> +	for (i = 0; i < leds_data->no_leds; i++) {
> +		ret = led_classdev_register(leds_data->dev,
> +				&leds_data->palmas_led[i].cdev);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev,
> +				"Failed to register LED no: %d err: %d\n",
> +				i, ret);
> +			goto err_led;
> +		}
> +	}
> +
> +	if (!is_palmas_charger(palmas->product_id))
> +		return 0;
> +
> +	/* Set the LED current registers if set in platform data */
> +	if (pdata->led1_current)
> +		palmas_led_update_bits(leds_data, PALMAS_LED_CURRENT_CTRL1,
> +				PALMAS_LED_CURRENT_CTRL1_LED_1_CURRENT_MASK,
> +				pdata->led1_current);

It looks buggy when 'led1_current' is negative value or out-of-range.
Why don't you define ledN_current as enum type?
Then it guarantees exact range of configurable value.

For example,
enum palmas_led_current {
	PALMAS_ILED_1mA,
	PALMAS_ILED_2mA,
...
};

> +
> +	if (pdata->led2_current)
> +		palmas_led_update_bits(leds_data, PALMAS_LED_CURRENT_CTRL1,
> +				PALMAS_LED_CURRENT_CTRL1_LED_2_CURRENT_MASK,
> +				pdata->led2_current <<
> +				PALMAS_LED_CURRENT_CTRL1_LED_2_CURRENT_SHIFT);
> +
> +	if (pdata->led3_current)
> +		palmas_led_update_bits(leds_data, PALMAS_LED_CURRENT_CTRL2,
> +				PALMAS_LED_CURRENT_CTRL2_LED_3_CURRENT_MASK,
> +				pdata->led3_current);
> +
> +	if (pdata->led3_current)
> +		palmas_led_update_bits(leds_data, PALMAS_LED_CURRENT_CTRL2,
> +				PALMAS_LED_CURRENT_CTRL2_LED_3_CURRENT_MASK,
> +				pdata->led3_current);
> +
> +	if (pdata->led4_current)
> +		palmas_led_update_bits(leds_data, PALMAS_CHRG_LED_CTRL,
> +				PALMAS_CHRG_LED_CTRL_CHRG_LED_CURRENT_MASK,
> +				pdata->led4_current <<
> +				PALMAS_CHRG_LED_CTRL_CHRG_LED_CURRENT_SHIFT);
> +
> +	if (pdata->chrg_led_vbat_low)
> +		palmas_led_update_bits(leds_data, PALMAS_CHRG_LED_CTRL,
> +				PALMAS_CHRG_LED_CTRL_CHRG_LOWBAT_BLK_DIS,
> +				PALMAS_CHRG_LED_CTRL_CHRG_LOWBAT_BLK_DIS);
> +
> +	return 0;
> +
> +err_led:
> +	for (i = 0; i < leds_data->no_leds; i++)
> +		led_classdev_unregister(&leds_data->palmas_led[i].cdev);
> +	kfree(leds_data->palmas_led);
> +	kfree(leds_data);

The kfree()s can be ignored because of memory allocation with devm_zalloc().

> +static int palmas_leds_remove(struct platform_device *pdev)
> +{
> +	struct palmas_leds_data *leds_data = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < leds_data->no_leds; i++)
> +		led_classdev_unregister(&leds_data->palmas_led[i].cdev);
> +	if (i)
> +		kfree(leds_data->palmas_led);
> +	kfree(leds_data);

Ditto. The kfree()s are not required with devm_zalloc().

> diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h
> index 5661f2d..7399c71 100644
> --- a/include/linux/mfd/palmas.h
> +++ b/include/linux/mfd/palmas.h
> @@ -232,6 +232,16 @@ struct palmas_clk_platform_data {
>  	int clk32kgaudio_mode_sleep;
>  };
> 
> +struct palmas_leds_platform_data {
> +	int led1_current;
> +	int led2_current;
> +	int led3_current;
> +	int led4_current;

Ditto, replaceable with enum type ?

Thanks.

Best Regards,
Milo