On Tue, 5 Oct 2010 12:42:55 +0300
Samu Onkalo <samu.p.onkalo@xxxxxxxxx> wrote:
> This is a driver for ROHM BH1770GLC and OSRAM SFH7770 combined
> ALS and proximity sensor.
Same comment about regulators.
> +/* Supported stand alone rates in ms from chip data sheet */
> +static s16 prox_rates[] = {10, 20, 30, 40, 70, 100, 200, 500, 1000, 2000};
> +
> +/* Supported IR-led currents in mA */
> +static const u8 prox_curr_ma[] = {5, 10, 20, 50, 100, 150, 200};
> +
> +/* Supported stand alone rates in ms from chip data sheet */
> +static s16 lux_rates[] = {100, 200, 500, 1000, 2000};
Any reason only one of the three is const ?
> +static int bhsfh_prox_rates(struct bhsfh_chip *chip, int rate,
> + int rate_threshold)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(prox_rates); i++)
> + if (prox_rates[i] == rate) {
> + chip->prox_rate = i;
> + break;
> + }
> +
> + if (i == ARRAY_SIZE(prox_rates))
> + return -EINVAL;
> +
> + for (i = 0; i < ARRAY_SIZE(prox_rates); i++)
> + if (prox_rates[i] == rate_threshold) {
> + chip->prox_rate_threshold = i;
> + return 0;
> + }
> +
> + return -EINVAL;
This makes it hard for generic code. Wouldn't picking the best (first at
least as good as required) be a bit more polite to user space ?
> +static ssize_t bhsfh_lux_result_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct bhsfh_chip *chip = dev_get_drvdata(dev);
> + ssize_t ret;
> +
> + mutex_lock(&chip->mutex);
> + if (pm_runtime_suspended(dev))
> + ret = -EIO; /* Chip is not enabled at all */
> + else if (chip->lux_wait_result)
> + ret = -EAGAIN; /* Waiting for result */
This makes no sense because you can't poll() a sysfs file
> +static ssize_t bhsfh_lux_calib_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct bhsfh_chip *chip = dev_get_drvdata(dev);
> + return snprintf(buf, PAGE_SIZE, "%u\n", chip->lux_calib);
> +}
This is short chip->mutex locks as you sometimes temporarily change the
value (error path below)
> +
> +static ssize_t bhsfh_lux_calib_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct bhsfh_chip *chip = dev_get_drvdata(dev);
> + unsigned long value;
> + u32 old_calib;
> + u32 new_corr;
> +
> + if (strict_strtoul(buf, 0, &value))
> + return -EINVAL;
> +
> + mutex_lock(&chip->mutex);
> + old_calib = chip->lux_calib;
> + chip->lux_calib = value;
> + new_corr = bhsfh_get_corr_value(chip);
> + if (new_corr == 0) {
> + chip->lux_calib = old_calib;
> + mutex_unlock(&chip->mutex);
> + return -EINVAL;
> + }
> + chip->lux_corr = new_corr;
> + mutex_unlock(&chip->mutex);
> +
> + return len;
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
