On 1/30/23 15:02, Jonathan Cameron wrote: > On Mon, 30 Jan 2023 14:04:53 +0200 > Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote: Hi Jonathan! Thanks for a _very fast_ response! It's nice "chatting" with you again! >> >> I hope this is all Ok from interface POV. > > This matches what I'd expect to see. Glad to hear :) >> Now, finally, my dear persistent readers - the question: >> As mentioned, sensor allows setting the sampling time. I thought I'll >> map this to the IIO_CHAN_INFO_INT_TIME. This config is not per/channel >> in the hardware. Again, my lux-computing algorithm takes the integration >> time into account - and changing the time should not be reflected to the >> IIO_LIGHT channel values (other than accuracy). However, the values >> spilled from raw IIO_INTENSITY channels will change when integration >> time is changed. So, should I use the info_mask_shared_by_type = >> BIT(IIO_CHAN_INFO_INT_TIME) for IIO_INTENSITY channels? > > Ah. This problem. The mixture of two things that effectively map to scaling > of raw channels. As _scale must be applied by userspace to the _raw channel > that has to reflect both the result of integration time and a front end amplifier Ouch. I know that this makes perfectly sense. It indeed may not be clear how the integration time impacts the scale. Thus, it is very reasonable that the driver code should know this and not leave the burden to the applications :/ Ouch because now I need to try inventing this logic in driver myself ;) > and is the control typical userspace expects to use to vary the sensitivity. Yes. Now that you wrote this it seems obvious. > That makes it messy because it's not always totally obvious whether, when > trying to increase sensitivity, it is better to increase sample time or gain. > Usually you do sample time first as that tends to reduce noise and for light > sensors we rarely need particular quick answers. > > So in the interests of keeping things easy to understand for userspace code > you would need to provide writeable _scale that then attempts to find the > best combination of amplifier gain and sampling time. You can allow read > only access to allow a curious user to find out what was chosen: > INT_TIME for the integration time. Right. This still makes sense. > Not really a good option for the amplifier gain though. I don't like using > HARDWARE_GAIN for this though maybe we could stretch the ABI to cover this > as long as it was read only. Well, I don't (yet) have the need for this but it is good to keep on mind :) It shouldn't be a big thing to add reading of (hardware)-gain. >> Sorry for the long post. I do appreciate all help/pointers on my journey >> to writing my first light sensor drivers ;) And yes, my plan is to send >> out the patches - when I first get the sensor hardware at my hands ;) > No problem. Light sensors tend to cause us more ABI headaches than almost > anything else Hm. This is the reason why I wanted to ask this right away. I realized that we have a kernel<->user ABI once the sysfs starts spilling the values from the driver. It wouldn't be such much fun 'fixing' this later on. (don't get me started on the ones used for blood oxygen level > measurement in which it's a light sensor and a controllable light source). Hmm... I think a colleague of mine did actually one such driver (AFAIR a BH<add some numbers here>) not so many years ago ;) Not sure if it went upstream though. For an occasional contributor like me it could be helpful if the defines like IIO_INTENSITY, IIO_LIGHT had documentation in headers explaining for example the units. Maybe also some words about the IIO_CHAN_INFO_INT_TIME and IIO_CHAN_INFO_SCALE as well ;) I guess I can cook some doc - but only for couple of defines which I have discussed with you this far. Do you think such comment docs would be welcome - even if they covered only couple of defines? Maybe others would continue from that. Yours, -- Matti -- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~
