by Zhang, Rui

[permalink] [raw]

Subject: Re: [PATCH] ALS: TSL2550 driver move from i2c/chips

update Pavel's email address.

On Fri, 2009-10-16 at 09:37 +0800, Zhang Rui wrote:
> On Tue, 2009-10-13 at 01:02 +0800, Jonathan Cameron wrote:
> > Jean Delvare wrote:
> > > Hi Jonathan,
> > >
> > > On Mon, 12 Oct 2009 15:19:07 +0100, Jonathan Cameron wrote:
> > >>>> @@ -60,9 +65,41 @@ static const u8 TSL2550_MODE_RANGE[2] = {
> > >>>> };
> > >>>>
> > >>>> /*
> > >>>> + * IDR to assign each registered device a unique id
> > >>>> + */
> > >>>> +static DEFINE_IDR(tsl2550_idr);
> > >>>> +static DEFINE_SPINLOCK(tsl2550_idr_lock);
> > >>>> +#define TSL2550_DEV_MAX 9
> > >>> Such an arbitrary limit is simply not acceptable.
> > >> Fair enough, but it is based on restricting the size
> > >> of the char array needed for the name when registering
> > >> with als. Hence single digit decimal maximum.
> > >> Do you suggest leaving it unrestricted (which makes code
> > >> a little fiddly given variable size of max idr) or some other
> > >> limit?
> > >
> > > The name size really isn't an issue. You won't notice 16 bytes instead
> > > of 9. And it's not like we'll have millions of these devices.
> > I agree, it's merely a question of picking some suitable limit. IDR's
> > on a 64 bit box will do something in the ball park of 2e18 which might
> > be an excessive limit ;) I'll leave this be for next version on basis
> > I'm in favour of moving this into the core and hence as you said this
> > will be irrelevant here anyway.
> > >>>> +static int tsl2550_get_id(void)
> > >>>> +{
> > >>>> + int ret, val;
> > >>>> +
> > >>>> +idr_again:
> > >>>> + if (unlikely(idr_pre_get(&tsl2550_idr, GFP_KERNEL) == 0))
> > >>>> + return -ENOMEM;
> > >>>> + spin_lock(&tsl2550_idr_lock);
> > >>>> + ret = idr_get_new(&tsl2550_idr, NULL, &val);
> > >>>> + if (unlikely(ret == -EAGAIN))
> > >>>> + goto idr_again;
> > >>>> + else if (unlikely(ret))
> > >>>> + return ret;
> > >>>> + if (val > TSL2550_DEV_MAX)
> > >>>> + return -ENOMEM;
> > >>>> + return val;
> > >>>> +}
> > >>>> +
> > >>>> +static void tsl2550_free_id(int val)
> > >>>> +{
> > >>>> + spin_lock(&tsl2550_idr_lock);
> > >>>> + idr_remove(&tsl2550_idr, val);
> > >>>> + spin_unlock(&tsl2550_idr_lock);
> > >>>> +}
> > >>> Having to implement this in _every_ ALS driver sounds wrong and
> > >>> painful. If uniqueness of any kind must be provided, it should be
> > >>> handled by the ALS core and not by individual drivers, otherwise you
> > >>> can be certain that at least one driver will get it wrong someday.
> > >> I agree. The reason this originally occurred is that the acpi layer
> > >> apparently doesn't allow for simultaneous probing of multiple drivers
> > >> and hence can get away with a simple counter and no locking.
> > >>
> > >>> I'd imaging that als-class devices are simply named als%u. Just like
> > >>> hwmon devices are named hwmon%u, input devices are names input%u and
> > >>> event%u, etc. I don't know of any class pushing the naming down to its
> > >>> drivers, do you? The only example I can remember are i2c drivers back
> > >>> in year 1999, when they were part of lm-sensors.I have personally put
> > >>> an end to this years ago.
> > >> This debate started in the als thread. Personally I couldn't care less
> > >> either way but it does need to be put to bed before that subsystem merges.
> > >> To my mind either approach is trivially handled in a userspace library
> > >> so it doesn't matter. I don't suppose you can remember what the original
> > >> reasons for squashing this naming approach were?
> > >
> > > Code duplication. Having the same unique-id handling code repeated in
> > > 50 drivers was no fun, as it did not add any value compared to a
> > > central handling.
> > Counter argument placed (cc'd Pavel and Corentin for this point)
> > is that having a generic name, e.g. hwmon0 and a name field in sysfs
> > is superfluous when we can combine the two.
> > >
> > >>>> @@ -296,13 +333,13 @@ static ssize_t tsl2550_show_lux1_input(struct device *dev,
> > >>>> return ret;
> > >>>> }
> > >>>>
> > >>>> -static DEVICE_ATTR(lux1_input, S_IRUGO,
> > >>>> +static DEVICE_ATTR(illuminance, S_IRUGO,
> > >>>> tsl2550_show_lux1_input, NULL);
> > >>> Question: if I have a light sensing chip with two sensors, how are we
> > >>> going to handle it? Will we register 2 als class devices? The initial
> > >>> naming convention had the advantage that you could have more than one
> > >>> sensor per device, but I don't know if it is desirable in practice.
> > >> This only becomes and issue if we have two sensors measuring illuminance
> > >> (or approximation of it). The only two sensor chips I know of have one
> > >> broadband and one in the infrared tsl2561 and I think the isl chip with
> > >> a driver currently in misc. The combination of these two are needed to
> > >> calculate illuminance. Some of the original discussion went into how
> > >> to support separate access to the individual sensors. Decision was to
> > >> leave that question until it becomes relevant. Basically we would put
> > >> the current drivers in just supporting illuminance and see if anyone asked
> > >> for furthers support. One tricky aspect is what the units should be for
> > >> particular frequency ranges. At least illuminance is cleanly defined
> > >> (even if chips are only fairly coarsely approximating it.
> > >
> > > Hmm, this isn't the point I was raising. I was thinking of a light
> > > sensor chip which would sense light in different locations. Think chip
> > > with remote sensors. This is done frequently for thermal sensors, so I
> > > guess it could be done for light sensors as well. A practical
> > > application could be sensing the ambient light on two sides of an
> > > object, so that you get the correct measurement regardless of the
> > > position.
> > >
> > > I'm not saying such chips exist, I really don't know. I am just raising
> > > the question of how we'd handle them if they do. The current naming
> > > scheme implies that we'd need a separate als instance for each sensor,
> > > and I want you to be aware of this.
> > I agree with this being a possible issue. Zhang, what do you think to changing
> > the acpi driver to use luminance0 and documentation to match? Seems like
> > a cost free way of avoiding possible problems down the line.
> > >
> I don't have any objections to this.
> I think we should push the generic ALS patch upstream first and I'll
> refresh the ACPI ALS patch later.
>
> thanks,
> rui

2009-11-26 14:17:14

by Jean Delvare

[permalink] [raw]

Subject: Re: [PATCH] ALS: TSL2550 driver move from i2c/chips

On Mon, 12 Oct 2009 17:38:24 +0200, Jean Delvare wrote:
> On Mon, 12 Oct 2009 16:13:01 +0100, Jonathan Cameron wrote:
> > > The operating mode selects the measurable range. Standard range is from
> > > 0 to 1846 lux, extended range is from 0 to 9230 lux, with a resolution
> > > divided by 5. Extended mode is also 5 times faster.
> > >
> > > What do we want to do with this? I am open to suggestions. There are
> > > several possibilities. The operating mode could be provided as platform
> > > data and stay internal to the driver. Or we can leave is visible to
> > > user-space, in which case I'd recommend that we do so in terms of
> > > "range" rather than "mode", so that other drivers can use the same
> > > convention, whatever it becomes. For example, one would write the range
> > > of values he/she wants to be able to measure and the driver would put
> > > the device in the most appropriate mode.
> >
> > That sounds like a sensible approach. For now I guess a sysfs parameter
> > like illuminance_range_max would do the job and we can add a min for any
> > device that features an offset?
>
> Yes, this sounds sensible.

Following up publicly so that everyone interested knows what's going on.

Jonathan and me tested the above idea on the tsl2550 driver, and it
turns out that it doesn't work well. The TSL2550 computes the lux of
visible light by computing a difference between total light (infra-red +
visible) and infra-red light. When there's a lot of infra-red light, it
is very easy to saturate the light sensors in standard mode, even
though the amount of visible light is low.

The bottom line is that one may need to reduce the integration time
(which is what the "extended mode" is really about) even if the result
fits in the standard mode's range. In the light of this, it seems
wrong to hide the mode behind the name "illuminance_range_max". It
seems better to export the exposure time as is, readable and writable.
Remains to agree on a file name and unit.

--
Jean Delvare