Re: [PATCH 1/3] leds: add aw2013 driver

[Pavel Machek]

Hi!

> +#define AW2013_NAME "aw2013"

That's.... not really useful define. Make it NAME? Drop it?

> +#define AW2013_TIME_STEP 130

I'd add comment with /* units */.

> +#define STATE_OFF 0
> +#define STATE_KEEP 1
> +#define STATE_ON 2

We should add enum into core for this...

> +static int aw2013_chip_init(struct aw2013 *chip)
> +{
> +	int i, ret;
> +
> +	ret = regmap_write(chip->regmap, AW2013_GCR, AW2013_GCR_ENABLE);
> +	if (ret) {
> +		dev_err(&chip->client->dev, "Failed to enable the chip: %d\n",
> +			ret);
> +		goto error;
> +	}
> +
> +	for (i = 0; i < chip->num_leds; i++) {
> +		ret = regmap_update_bits(chip->regmap,
> +					 AW2013_LCFG(chip->leds[i].num),
> +					 AW2013_LCFG_IMAX_MASK,
> +					 chip->leds[i].imax);
> +		if (ret) {
> +			dev_err(&chip->client->dev,
> +				"Failed to set maximum current for led %d: %d\n",
> +				chip->leds[i].num, ret);
> +			goto error;
> +		}
> +	}
> +
> +error:
> +	return ret;
> +}

No need for goto if you are just returning.

> +static bool aw2013_chip_in_use(struct aw2013 *chip)
> +{
> +	int i;
> +
> +	for (i = 0; i < chip->num_leds; i++)
> +		if (chip->leds[i].cdev.brightness)
> +			return true;
> +
> +	return false;
> +}

How is this going to interact with ledstate == KEEP?

> +static int aw2013_brightness_set(struct led_classdev *cdev,
> +				 enum led_brightness brightness)
> +{
> +	struct aw2013_led *led = container_of(cdev, struct aw2013_led, cdev);
> +	int ret, num;
> +
> +	mutex_lock(&led->chip->mutex);
> +
> +	if (aw2013_chip_in_use(led->chip)) {
> +		ret = aw2013_chip_enable(led->chip);
> +		if (ret)
> +			return ret;
> +	}

You are returning with mutex held.

> +	/* Never on - just set to off */
> +	if (!*delay_on)
> +		return aw2013_brightness_set(&led->cdev, LED_OFF);
> +
> +	/* Never off - just set to brightness */
> +	if (!*delay_off)
> +		return aw2013_brightness_set(&led->cdev, led->cdev.brightness);

Is this dance neccessary? Should we do it in the core somewhere?

> +		} else {
> +			led->imax = 1; // 5mA
> +			dev_info(&client->dev,
> +				 "DT property led-max-microamp is missing!\n");
> +		}

Lets remove the exclamation mark.

> +		led->num = source;
> +		led->chip = chip;
> +		led->fwnode = of_fwnode_handle(child);
> +
> +		if (!of_property_read_string(child, "default-state", &str)) {
> +			if (!strcmp(str, "on"))
> +				led->default_state = STATE_ON;
> +			else if (!strcmp(str, "keep"))
> +				led->default_state = STATE_KEEP;
> +			else
> +				led->default_state = STATE_OFF;
> +		}

We should really have something in core for this. Should we support
arbitrary brightness there?

> +static void aw2013_read_current_state(struct aw2013 *chip)
> +{
> +	int i, led_on;
> +
> +	regmap_read(chip->regmap, AW2013_LCTR, &led_on);
> +
> +	for (i = 0; i < chip->num_leds; i++) {
> +		if (!(led_on & AW2013_LCTR_LE(chip->leds[i].num))) {
> +			chip->leds[i].cdev.brightness = LED_OFF;
> +			continue;
> +		}
> +		regmap_read(chip->regmap, AW2013_REG_PWM(chip->leds[i].num),
> +			    &chip->leds[i].cdev.brightness);
> +	}
> +}
> +
> +static void aw2013_init_default_state(struct aw2013_led *led)
> +{
> +	switch (led->default_state) {
> +	case STATE_ON:
> +		led->cdev.brightness = LED_FULL;
> +		break;
> +	case STATE_OFF:
> +		led->cdev.brightness = LED_OFF;
> +	} /* On keep - just set brightness that was retrieved previously */
> +
> +	aw2013_brightness_set(&led->cdev, led->cdev.brightness);
> +}

Aha; I guess this makes "keeping" the state to work. Do you really
need that functionality?

Pretty nice driver, thanks.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachment: signature.asc
Description: PGP signature