Re: [PATCH 1/5] misc: Driver for bh1770glc / sfh7770 ALS and proximity sensor

[Onkalo Samu <samu.p.onkalo@nokia.com>]

Alan, thanks for comments

-Samu

On Tue, 2010-10-05 at 13:21 +0200, ext Alan Cox wrote:
> On Tue,  5 Oct 2010 12:42:55 +0300
> Samu Onkalo <samu.p.onkalo@nokia.com> 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 ?

No real reason.

> 
> 
> > +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 ?

Well, perhaps it is better to have hardcoded (or platform specific)
rates instead of control interface. I'll change that.

> 
> 
> > +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

ok, what should be returned when there is no valid results available?

> 
> 
> 
> > +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;