lkml.org 
[lkml]   [2011]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRE: [PATCH 1/2] leds: add driver for LM3530 ALS


[...]
> + ret = i2c_smbus_write_byte_data(client,
> + LM3530_GEN_CONFIG, gen_config);
> + if (ret)
> + return ret;
> + ret = i2c_smbus_write_byte_data(client,
> + LM3530_ALS_CONFIG, als_config);
> + if (ret)
> + return ret;
> + ret = i2c_smbus_write_byte_data(client,
> + LM3530_BRT_RAMP_RATE, brt_ramp);
> + if (ret)
> + return ret;
> + ret = i2c_smbus_write_byte_data(client,
> + LM3530_ALS_IMP_SELECT, als_imp_sel);
> + if (ret)
> + return ret;
> + ret = i2c_smbus_write_byte_data(client,
> + LM3530_BRT_CTRL_REG, brightness);
> + if (ret)
> + return ret;
> + ret = i2c_smbus_write_byte_data(client,
> + LM3530_ALS_ZB0_REG, LM3530_DEF_ZB_0);
> + if (ret)
> + return ret;
> + ret = i2c_smbus_write_byte_data(client,
> + LM3530_ALS_ZB1_REG, LM3530_DEF_ZB_1);
> + if (ret)
> + return ret;
> + ret = i2c_smbus_write_byte_data(client,
> + LM3530_ALS_ZB2_REG, LM3530_DEF_ZB_2);
> + if (ret)
> + return ret;
> + ret = i2c_smbus_write_byte_data(client,
> + LM3530_ALS_ZB3_REG, LM3530_DEF_ZB_3);
> + if (ret)
> + return ret;
> + ret = i2c_smbus_write_byte_data(client,
> + LM3530_ALS_Z0T_REG, LM3530_DEF_ZT_0);
> + if (ret)
> + return ret;
> + ret = i2c_smbus_write_byte_data(client,
> + LM3530_ALS_Z1T_REG, LM3530_DEF_ZT_1);
> + if (ret)
> + return ret;
> + ret = i2c_smbus_write_byte_data(client,
> + LM3530_ALS_Z2T_REG, LM3530_DEF_ZT_2);
> + if (ret)
> + return ret;
> + ret = i2c_smbus_write_byte_data(client,
> + LM3530_ALS_Z3T_REG, LM3530_DEF_ZT_3);
> + if (ret)
> + return ret;
> + ret = i2c_smbus_write_byte_data(client,
> + LM3530_ALS_Z4T_REG, LM3530_DEF_ZT_4);
> + if (ret)
> + return ret;
> +
> + return ret;

I can't help wonder if a table of values iterated over would look a
little neater here. The last if is harmless but pointless too.

[Shreshtha Kumar SAHU] done

> +static ssize_t lm3530_mode_set(struct device *dev, struct device_attribute
> + *attr, const char *buf, size_t size)
> +{
> + int err;
> + struct i2c_client *client = container_of(
> + dev->parent, struct i2c_client, dev);
> + struct lm3530_data *drvdata = i2c_get_clientdata(client);
> + unsigned long mode;
> +
> + err = strict_strtoul(buf, 10, &mode);
> + if (err < 0)
> + return -EINVAL;
> +
> + if (mode == LM3530_BL_MODE_MANUAL)
> + drvdata->mode = LM3530_BL_MODE_MANUAL;
> + else if (mode == LM3530_BL_MODE_ALS)
> + drvdata->mode = LM3530_BL_MODE_ALS;
> + else if (mode == LM3530_BL_MODE_PWM) {
> + dev_err(dev, "PWM mode not supported\n");
> + return -EINVAL;
> + }
> +
> + err = lm3530_init_registers(drvdata);
> + if (err) {
> + dev_err(dev, "Setting %s Mode failed :%d\n", buf, err);
> + return err;
> + }
> +
> + return sizeof(drvdata->mode);
> +}
> +
> +static DEVICE_ATTR(mode, 0644, NULL, lm3530_mode_set);

So you poke random values of 0, 1, 2 into sysfs? I suspect that breaks
sysfs guidelines and these should be meaningful values like "pwm", "als"
and "led". The enum is ok within the kernel but not for exposure outside
of it.

[Shreshtha Kumar SAHU] done

Also, calling out PWM specifically as not supported seems like the wrong
approach
[Shreshtha Kumar SAHU] I will add support for PWM soon.

and I'm curious what happens if I poke 4 in there.
[Shreshtha Kumar SAHU] rectified with implementation of meaningful input
values


The rest looked ok at a quick glance.

Cheers,

Richard

\
 
 \ /
  Last update: 2011-01-20 11:07    [W:0.810 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site