On Tue, 05 Oct 2010 14:29:50 +0300
Onkalo Samu <samu.p.onkalo@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxx> wrote:
> >
> > > This is a driver for ROHM BH1770GLC and OSRAM SFH7770 combined
> > > ALS and proximity sensor.
> >
> > Same comment about regulators.
>
> ?
As the other review for the light sensor
> > > + 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.
I don't see a problem with a rate setting where you ask for 'at least X'
and get the nearest ? Ie swap the == for a >= ?
> > This makes no sense because you can't poll() a sysfs file
>
> ok, what should be returned when there is no valid results available?
I have the same problem with the isl29020 - I think you have to block,
there isn't any other sane response. A user given -EAGAIN simply doesn't
know what to do
> > > +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)
> >
>
> ?
If I am doing a calib store and a calib show in parallel then the show
can give a bogus answer because of this bit of code
> > > + mutex_lock(&chip->mutex);
> > > + old_calib = chip->lux_calib;
> > > + chip->lux_calib = value;
> > > + new_corr = bhsfh_get_corr_value(chip);
> > > + if (new_corr == 0) {
[Show can occur here]
> > > + chip->lux_calib = old_calib;
> > > + mutex_unlock(&chip->mutex);
> > > + return -EINVAL;
> > > + }
> > > + chip->lux_corr = new_corr;
> > > + mutex_unlock(&chip->mutex);
> > > +
> > > + return len;
--
--
"Alan, I'm getting a bit worried about you."
-- Linus Torvalds
--
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
