Messages in this thread | | | Date | Thu, 20 Jan 2011 16:48:16 -0800 | From | Andrew Morton <> | Subject | Re: [PATCH] backlight: new driver for the ADP8870 backlight devices |
| |
On Tue, 11 Jan 2011 20:00:48 -0500 Mike Frysinger <vapier@gentoo.org> wrote:
> > ... > > +#define FADE_VAL(in, out) ((0xF & (in)) | ((0xF & (out)) << 4)) > +#define BL_CFGR_VAL(law, blv) ((((blv) & CFGR_BLV_MASK) << CFGR_BLV_SHIFT) | ((0x3 & (law)) << 1)) > +#define ALS_CMPR_CFG_VAL(filt) ((0x7 & filt) << 1)
Missing () around `filt'.
There's no reason why these "functions" needed to be implemented as macros? They'd be better as nice lowercase-named, commented, typesafe C functions.
> > ... > > +static int adp8870_read(struct i2c_client *client, int reg, uint8_t *val) > +{ > + int ret; > + > + ret = i2c_smbus_read_byte_data(client, reg); > + if (ret < 0) { > + dev_err(&client->dev, "failed reading at 0x%02x\n", reg); > + return ret; > + } > + > + *val = (uint8_t)ret;
the cast wasn't needed.
> + return 0; > +} > + > + > > ... > > +#if defined(ADP8870_USE_LEDS) > +static void adp8870_led_work(struct work_struct *work) > +{ > + struct adp8870_led *led = container_of(work, struct adp8870_led, work); > + adp8870_write(led->client, ADP8870_ISC1 + led->id - 1, > + led->new_brightness >> 1); > +} > + > +static void adp8870_led_set(struct led_classdev *led_cdev, > + enum led_brightness value) > +{ > + struct adp8870_led *led; > + > + led = container_of(led_cdev, struct adp8870_led, cdev); > + led->new_brightness = value; > + schedule_work(&led->work); > +}
Why does it use schedule_work() instead of synchronously calling adp8870_write()?
(And if I didn't know, other readers won't know either. It needs a comment explaining this).
> > ... > > +static int __devinit adp8870_led_probe(struct i2c_client *client) > +{ > + struct adp8870_backlight_platform_data *pdata = > + client->dev.platform_data; > + struct adp8870_bl *data = i2c_get_clientdata(client); > + struct adp8870_led *led, *led_dat; > + struct led_info *cur_led; > + int ret, i; > + > + led = kzalloc(sizeof(*led) * pdata->num_leds, GFP_KERNEL);
kcalloc() is neater.
> + if (led == NULL) { > + dev_err(&client->dev, "failed to alloc memory\n"); > + return -ENOMEM; > + } > + > + ret = adp8870_write(client, ADP8870_ISCLAW, pdata->led_fade_law); > + ret = adp8870_write(client, ADP8870_ISCT1, > + (pdata->led_on_time & 0x3) << 6);
I think you intended |= here.
> + ret |= adp8870_write(client, ADP8870_ISCF, > + FADE_VAL(pdata->led_fade_in, pdata->led_fade_out)); > + > + if (ret) {
But OR-ing errnos together is a bit grubby - if two calls return different errnos, we get garbage.
> + dev_err(&client->dev, "failed to write\n"); > + goto err_free; > + } > + > + for (i = 0; i < pdata->num_leds; ++i) { > + cur_led = &pdata->leds[i]; > + led_dat = &led[i]; > + > + led_dat->id = cur_led->flags & ADP8870_FLAG_LED_MASK; > + > + if (led_dat->id > 7 || led_dat->id < 1) { > + dev_err(&client->dev, "Invalid LED ID %d\n", > + led_dat->id); > + goto err; > + } > + > + if (pdata->bl_led_assign & (1 << (led_dat->id - 1))) { > + dev_err(&client->dev, "LED %d used by Backlight\n", > + led_dat->id); > + goto err; > + } > + > + led_dat->cdev.name = cur_led->name; > + led_dat->cdev.default_trigger = cur_led->default_trigger; > + led_dat->cdev.brightness_set = adp8870_led_set; > + led_dat->cdev.brightness = LED_OFF; > + led_dat->flags = cur_led->flags >> FLAG_OFFT_SHIFT; > + led_dat->client = client; > + led_dat->new_brightness = LED_OFF; > + INIT_WORK(&led_dat->work, adp8870_led_work); > + > + ret = led_classdev_register(&client->dev, &led_dat->cdev); > + if (ret) { > + dev_err(&client->dev, "failed to register LED %d\n", > + led_dat->id); > + goto err; > + } > + > + ret = adp8870_led_setup(led_dat); > + if (ret) { > + dev_err(&client->dev, "failed to write\n"); > + i++; > + goto err; > + } > + } > + > + data->led = led; > + > + return 0; > + > + err: > + for (i = i - 1; i >= 0; --i) { > + led_classdev_unregister(&led[i].cdev); > + cancel_work_sync(&led[i].work); > + } > + > + err_free: > + kfree(led); > + > + return ret; > +} > > ... > > +static int adp8870_bl_setup(struct backlight_device *bl) > +{ > + struct adp8870_bl *data = bl_get_data(bl); > + struct i2c_client *client = data->client; > + struct adp8870_backlight_platform_data *pdata = data->pdata; > + int ret = 0; > + > + ret |= adp8870_write(client, ADP8870_BLSEL, ~pdata->bl_led_assign); > + ret |= adp8870_write(client, ADP8870_PWMLED, pdata->pwm_assign); > + ret |= adp8870_write(client, ADP8870_BLMX1, pdata->l1_daylight_max); > + ret |= adp8870_write(client, ADP8870_BLDM1, pdata->l1_daylight_dim); > + > + if (pdata->en_ambl_sens) { > + data->cached_daylight_max = pdata->l1_daylight_max; > + ret |= adp8870_write(client, ADP8870_BLMX2, > + pdata->l2_bright_max); > + ret |= adp8870_write(client, ADP8870_BLDM2, > + pdata->l2_bright_dim); > + ret |= adp8870_write(client, ADP8870_BLMX3, > + pdata->l3_office_max); > + ret |= adp8870_write(client, ADP8870_BLDM3, > + pdata->l3_office_dim); > + ret |= adp8870_write(client, ADP8870_BLMX4, > + pdata->l4_indoor_max); > + ret |= adp8870_write(client, ADP8870_BLDM4, > + pdata->l4_indor_dim); > + ret |= adp8870_write(client, ADP8870_BLMX5, > + pdata->l5_dark_max); > + ret |= adp8870_write(client, ADP8870_BLDM5, > + pdata->l5_dark_dim); > + > + ret |= adp8870_write(client, ADP8870_L2TRP, pdata->l2_trip); > + ret |= adp8870_write(client, ADP8870_L2HYS, pdata->l2_hyst); > + ret |= adp8870_write(client, ADP8870_L3TRP, pdata->l3_trip); > + ret |= adp8870_write(client, ADP8870_L3HYS, pdata->l3_hyst); > + ret |= adp8870_write(client, ADP8870_L4TRP, pdata->l4_trip); > + ret |= adp8870_write(client, ADP8870_L4HYS, pdata->l4_hyst); > + ret |= adp8870_write(client, ADP8870_L5TRP, pdata->l5_trip); > + ret |= adp8870_write(client, ADP8870_L5HYS, pdata->l5_hyst); > + ret |= adp8870_write(client, ADP8870_ALS1_EN, L5_EN | L4_EN | > + L3_EN | L2_EN); > + > + ret |= adp8870_write(client, ADP8870_CMP_CTL, > + ALS_CMPR_CFG_VAL(pdata->abml_filt)); > + > + } > + > + ret |= adp8870_write(client, ADP8870_CFGR, > + BL_CFGR_VAL(pdata->bl_fade_law, 0)); > + > + ret |= adp8870_write(client, ADP8870_BLFR, FADE_VAL(pdata->bl_fade_in, > + pdata->bl_fade_out)); > + > + /* > + * ADP8870 Rev0 requires GDWN_DIS bit set > + */ > + > + ret |= adp8870_set_bits(client, ADP8870_MDCR, BLEN | DIM_EN | NSTBY | > + (data->revid == 0 ? GDWN_DIS : 0)); > + > + return ret; > +}
Much grubbiness.
adp8870_write() can take a long time, I think. What's the worst-case value of adapter->timeout? If the interface is timing out, this code will call the timing-out function ten or twenty times. How long can all this take? How many messages will it spew on the console?
> +static ssize_t adp8870_show(struct device *dev, char *buf, int reg) > +{ > + struct adp8870_bl *data = dev_get_drvdata(dev); > + int error; > + uint8_t reg_val; > + > + mutex_lock(&data->lock); > + error = adp8870_read(data->client, reg, ®_val); > + mutex_unlock(&data->lock); > + > + if (error < 0) > + return error; > + > + return sprintf(buf, "%u\n", reg_val); > +} > + > +static ssize_t adp8870_store(struct device *dev, const char *buf, > + size_t count, int reg) > +{ > + struct adp8870_bl *data = dev_get_drvdata(dev); > + unsigned long val; > + int ret; > + > + ret = strict_strtoul(buf, 10, &val); > + if (ret) > + return ret; > + > + mutex_lock(&data->lock); > + adp8870_write(data->client, reg, val); > + mutex_unlock(&data->lock); > + > + return count; > +}
Is the sysfs API documented anywhere?
> > ... >
| |