| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20110120164816.5afa0561.akpm@linux-foundation.org>
Date: Thu, 20 Jan 2011 16:48:16 -0800
From: Andrew Morton <akpm@...ux-foundation.org>
To: Mike Frysinger <vapier@...too.org>
Cc: device-drivers-devel@...ckfin.uclinux.org,
linux-kernel@...r.kernel.org, Richard Purdie <rpurdie@...ys.net>,
Michael Hennerich <michael.hennerich@...log.com>
Subject: Re: [PATCH] backlight: new driver for the ADP8870 backlight devices
On Tue, 11 Jan 2011 20:00:48 -0500
Mike Frysinger <vapier@...too.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?
>
> ...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists