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
