[PATCH v8 09/12] leds: Add support for Palmas LEDs
Ian Lartey
ian at slimlogic.co.uk
Mon Mar 11 11:16:32 EDT 2013
On 08/03/13 01:13, Kim, Milo wrote:
>> 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.
Thanks for your comments Kim.
>
>> 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?
The led still needs to be turned off later on in the procedure.
led_blink = 0;
so we can't return early.
>
>> +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.
is_palmas_charger is simply a macro which checks a value so it
is as simple as it gets.
>
>> +
>> + 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,
> ...
> };
Your right it does need some bounds checking here, I'll put some in.
>
>> +
>> + 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().
Yes, thanks for that.
>
>> +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().
agreed
>
>> 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
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
More information about the linux-arm-kernel
mailing list