Messages in this thread | | | From | Shreshtha Kumar SAHU <> | Date | Thu, 20 Jan 2011 11:03:29 +0100 | Subject | RE: [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
| |