| Message ID | 874d59be98703bb58a98fea72138de5b94d71a52.1677750859.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Support ROHM BU27034 ALS sensor | expand |
to 2. maalisk. 2023 klo 12.59 Matti Vaittinen (mazziesaccount@gmail.com) kirjoitti: > > ROHM BU27034 is an ambient light sesnor with 3 channels and 3 photo diodes > capable of detecting a very wide range of illuminance. Typical application > is adjusting LCD and backlight power of TVs and mobile phones. > > Add initial support for the ROHM BU27034 ambient light sensor. > > NOTE: > - Driver exposes 4 channels. One IIO_LIGHT channel providing the > calculated lux values based on measured data from diodes #0 and > #1. Additionally 3 IIO_INTENSITY channels are emitting the raw > register data from all diodes for more intense user-space > computations. > - Sensor has adjustible GAIN values ranging from 1x to 4096x. > - Sensor has adjustible measurement times 5, 55, 100, 200 and > 400 mS. Driver does not support 5 mS which has special > limitations. > - Driver exposes standard 'scale' adjustment which is > implemented by: > 1) Trying to adjust only the GAIN > 2) If GAIN adjustment only can't provide requested > scale, adjusting both the time and the gain is > attempted. > - Driver exposes writable INT_TIME property which can be used > for adjusting the measurement time. Time adjustment will also > cause the driver to adjust the GAIN so that the overall scale > is not changed. > - Runtime PM is not implemented. > - Driver starts the measurement on the background when it is > probed. This improves the respnse time to read-requests Seems like I forgot to update the commit message. Note to self: change this when preparing the v3 ,,, > +config ROHM_BU27034 > + tristate "ROHM BU27034 ambient light sensor" > + depends on I2C > + select REGMAP_I2C > + select IIO_GTS_HELPER And, judging some other fixes - it seems this one would also need the select IIO_BUFFER and IIO_KFIFO_BUF now...
On Thu, Mar 02, 2023 at 12:58:59PM +0200, Matti Vaittinen wrote: > ROHM BU27034 is an ambient light sesnor with 3 channels and 3 photo diodes > capable of detecting a very wide range of illuminance. Typical application > is adjusting LCD and backlight power of TVs and mobile phones. > > Add initial support for the ROHM BU27034 ambient light sensor. > > NOTE: > - Driver exposes 4 channels. One IIO_LIGHT channel providing the > calculated lux values based on measured data from diodes #0 and > #1. Additionally 3 IIO_INTENSITY channels are emitting the raw > register data from all diodes for more intense user-space > computations. > - Sensor has adjustible GAIN values ranging from 1x to 4096x. > - Sensor has adjustible measurement times 5, 55, 100, 200 and > 400 mS. Driver does not support 5 mS which has special > limitations. > - Driver exposes standard 'scale' adjustment which is > implemented by: > 1) Trying to adjust only the GAIN > 2) If GAIN adjustment only can't provide requested > scale, adjusting both the time and the gain is > attempted. > - Driver exposes writable INT_TIME property which can be used > for adjusting the measurement time. Time adjustment will also > cause the driver to adjust the GAIN so that the overall scale > is not changed. > - Runtime PM is not implemented. > - Driver starts the measurement on the background when it is > probed. This improves the respnse time to read-requests > compared to starting the read only when data is requested. > When the most accurate 400 mS measurement time is used, data reads > would last quite long if measurement was started only on > demand. This, however, is not appealing for users who would > prefere power saving over measurement response time. ... > +config ROHM_BU27034 > + tristate "ROHM BU27034 ambient light sensor" > + depends on I2C How? I do not see a such. > + select REGMAP_I2C > + select IIO_GTS_HELPER > + help > + Enable support for the ROHM BU27034 ambient light sensor. ROHM BU27034 > + is an ambient light sesnor with 3 channels and 3 photo diodes capable > + of detecting a very wide range of illuminance. > + Typical application is adjusting LCD and backlight power of TVs and > + mobile phones. Module name? ... > obj-$(CONFIG_OPT3001) += opt3001.o > obj-$(CONFIG_PA12203001) += pa12203001.o > +obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o If you see, most of the components are without vendor prefix, why rohm is special? Like you are expecting the very same filename for something else? > obj-$(CONFIG_RPR0521) += rpr0521.o > obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o > obj-$(CONFIG_SI1133) += si1133.o ... > +#include <linux/iio/iio.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/kfifo_buf.h> Sorted? ... > +#define BU27034_REG_DATA0_LO 0x50 > +#define BU27034_REG_DATA1_LO 0x52 > +#define BU27034_REG_DATA2_LO 0x54 I would drop _LO in all these > +#define BU27034_REG_DATA2_HI 0x55 and rename somehow this to something like _END / _MAX (similar to the fields. Perhaps you would need _START / _MIN above. ... > +/* > + * Available scales with gain 1x - 4096x, timings 55, 100, 200, 400 mS > + * Time impacts to gain: 1x, 2x, 4x, 8x. > + * > + * => Max total gain is HWGAIN * gain by integration time (8 * 4096) = 32768 > + * > + * Using NANO precision for scale we must use scale 64x corresponding gain 1x > + * to avoid precision loss. (32x would result scale 976 562.5(nanos). > + */ > +#define BU27034_SCALE_1X 64 > + > +#define BU27034_GSEL_1X 0x00 > +#define BU27034_GSEL_4X 0x08 > +#define BU27034_GSEL_16X 0x0a > +#define BU27034_GSEL_32X 0x0b > +#define BU27034_GSEL_64X 0x0c > +#define BU27034_GSEL_256X 0x18 > +#define BU27034_GSEL_512X 0x19 > +#define BU27034_GSEL_1024X 0x1a > +#define BU27034_GSEL_2048X 0x1b > +#define BU27034_GSEL_4096X 0x1c Shouldn't the values be in plain decimal? Otherwise I would like to understand bit mapping inside these hex values. ... > + .indexed = 1 \ + Comma at the end. ... > + static const int reg[] = { > + [BU27034_CHAN_DATA0] = BU27034_REG_MODE_CONTROL2, > + [BU27034_CHAN_DATA1] = BU27034_REG_MODE_CONTROL3, > + [BU27034_CHAN_DATA2] = BU27034_REG_MODE_CONTROL2 Ditto. > + }; ... > + struct bu27034_gain_check gains[3] = { > + { .chan = BU27034_CHAN_DATA0, }, > + { .chan = BU27034_CHAN_DATA1, }, Inner commas are not needed. > + { .chan = BU27034_CHAN_DATA2 } But here the outer one is good to have. > + }; ... > + if (chan == BU27034_CHAN_ALS) { > + if (val == 0 && val2 == 1000) > + return 0; > + else Redundant 'else'. And probably here is better to use standard pattern for "checking for error first". > + return -EINVAL; > + } ... > + if (helper64 < 0xFFFFFFFFFFFFFLLU) { Perhaps this needs a definition. > + helper64 *= gain0; > + do_div(helper64, ch0); > + } else { > + do_div(helper64, ch0); > + helper64 *= gain0; > + } > + /* Same overflow check here */ Why not a helper function? > + if (helper64 < 0xFFFFFFFFFFFFFLLU) { > + helper64 *= gain0; > + do_div(helper64, helper); > + } else { > + do_div(helper64, helper); > + helper64 *= gain0; > + } ... > + return (val & BU27034_MASK_VALID); Unneeded parentheses. ... > +retry: > + /* Get new value from sensor if data is ready */ > + if (bu27034_has_valid_sample(data)) { > + ret = regmap_bulk_read(data->regmap, BU27034_REG_DATA0_LO, > + res, size); > + if (ret) > + return ret; > + > + bu27034_invalidate_read_data(data); > + } else { > + /* No new data in sensor. Wait and retry */ > + msleep(25); > + > + goto retry; There is no way out. What might go wrong? > + } ... > + ret = bu27034_get_int_time(data); _get_int_time_us() ? (Looking at the below code) > + if (ret < 0) > + return ret; > + > + msleep(ret / 1000); ... > + * Avoid div by zeroi. Not using max() as the data may not be in zeroi? ... > + if (!res[0]) Positive conditional? > + ch0 = 1; > + else > + ch0 = le16_to_cpu(res[0]); > + > + if (!res[1]) > + ch1 = 1; Ditto. > + else > + ch1 = le16_to_cpu(res[1]); But why not to read and convert first and then check. This at least will correctly compare 0 to the LE16 0 (yes, it's the same for 0, but strictly speaking the bits order of lvalue and rvalue is different). ... > + switch (mask) { > + case IIO_CHAN_INFO_INT_TIME: > + return iio_gts_avail_times(&data->gts, vals, type, length); > + case IIO_CHAN_INFO_SCALE: > + return iio_gts_all_avail_scales(&data->gts, vals, type, length); > + default: > + break; > + } > + > + return -EINVAL; You may do it from default case. ... > + ret = regmap_read_poll_timeout(data->regmap, BU27034_REG_MODE_CONTROL4, > + val, (val & BU27034_MASK_VALID), Redundant parentheses. > + BU27034_DATA_WAIT_TIME_US, > + BU27034_TOTAL_DATA_WAIT_TIME_US); > + if (ret) { > + dev_err(data->dev, "data polling %s\n", > + !(val & BU27034_MASK_VALID) ? "timeout" : "fail"); Why not positive conditional in ternary? > + return ret; > + } ... > + fwnode = dev_fwnode(dev); > + if (!fwnode) > + return -ENODEV; So, you deliberately disable a possibility to instantiate this from user space, why? ... > + ret = devm_iio_kfifo_buffer_setup(dev, idev, &bu27034_buffer_ops); > + > + ret = devm_iio_device_register(dev, idev); Don't you find something strange in between? > + if (ret < 0) > + return dev_err_probe(dev, ret, > + "Unable to register iio device\n"); ... > + { .compatible = "rohm,bu27034", }, Inner comma is not needed.
Hi Andy, Thanks again for the review. Some nice catches there. On 3/2/23 17:34, Andy Shevchenko wrote: > On Thu, Mar 02, 2023 at 12:58:59PM +0200, Matti Vaittinen wrote: >> ROHM BU27034 is an ambient light sesnor with 3 channels and 3 photo diodes >> capable of detecting a very wide range of illuminance. Typical application >> is adjusting LCD and backlight power of TVs and mobile phones. >> >> Add initial support for the ROHM BU27034 ambient light sensor. >> >> NOTE: >> - Driver exposes 4 channels. One IIO_LIGHT channel providing the >> calculated lux values based on measured data from diodes #0 and >> #1. Additionally 3 IIO_INTENSITY channels are emitting the raw >> register data from all diodes for more intense user-space >> computations. >> - Sensor has adjustible GAIN values ranging from 1x to 4096x. >> - Sensor has adjustible measurement times 5, 55, 100, 200 and >> 400 mS. Driver does not support 5 mS which has special >> limitations. >> - Driver exposes standard 'scale' adjustment which is >> implemented by: >> 1) Trying to adjust only the GAIN >> 2) If GAIN adjustment only can't provide requested >> scale, adjusting both the time and the gain is >> attempted. >> - Driver exposes writable INT_TIME property which can be used >> for adjusting the measurement time. Time adjustment will also >> cause the driver to adjust the GAIN so that the overall scale >> is not changed. >> - Runtime PM is not implemented. >> - Driver starts the measurement on the background when it is >> probed. This improves the respnse time to read-requests >> compared to starting the read only when data is requested. >> When the most accurate 400 mS measurement time is used, data reads >> would last quite long if measurement was started only on >> demand. This, however, is not appealing for users who would >> prefere power saving over measurement response time. > > ... > >> +config ROHM_BU27034 >> + tristate "ROHM BU27034 ambient light sensor" > >> + depends on I2C > > How? I do not see a such. I have assumed we need this because: We select REGMAP_I2C which depends on I2C. What happens if I2C=n and we select REGMAP_I2C? I may be wrong but I guess the I2C stays 'n' while REGMAP_I2C becomes y/m (?) I think that would be unfortunate - but I can't claim I am confident with how config dependencies are handled. I can drop this depends on if you're sure that's not a problem. >> + select REGMAP_I2C >> + select IIO_GTS_HELPER >> + help >> + Enable support for the ROHM BU27034 ambient light sensor. ROHM BU27034 >> + is an ambient light sesnor with 3 channels and 3 photo diodes capable >> + of detecting a very wide range of illuminance. >> + Typical application is adjusting LCD and backlight power of TVs and >> + mobile phones. > > Module name? I am having a deja-vu. https://lore.kernel.org/all/10c4663b-dd65-a545-786d-10aed6e6e5e9@fi.rohmeurope.com/ Module name is completely irrelevant when selecting a kernel configuration. > ... > >> obj-$(CONFIG_OPT3001) += opt3001.o >> obj-$(CONFIG_PA12203001) += pa12203001.o > >> +obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o > > If you see, most of the components are without vendor prefix, why rohm is > special? Like you are expecting the very same filename for something else? No. I don't. Using the vendor prefix in _file name_ was suggested to me by Lee already a few years ago. And I am actually grateful he did. I've found that _very_ useful as it simplifies finding the files I am looking for. What comes to the config option name, being able to easily search for the configs by vendor name has also been helpful. > ... > >> +#include <linux/iio/iio.h> >> +#include <linux/iio/buffer.h> >> +#include <linux/iio/kfifo_buf.h> > > Sorted? Sure, thanks. > > ... > >> +#define BU27034_REG_DATA0_LO 0x50 >> +#define BU27034_REG_DATA1_LO 0x52 >> +#define BU27034_REG_DATA2_LO 0x54 > > I would drop _LO in all these > >> +#define BU27034_REG_DATA2_HI 0x55 > > and rename somehow this to something like _END / _MAX (similar to the fields. > Perhaps you would need _START / _MIN above. I don't think this would improve anything. The _LO / _HI are descriptive as we have only two registers for each channel, _LO and _HI being more or less standard abbreviations for low and high. > ... > >> +/* >> + * Available scales with gain 1x - 4096x, timings 55, 100, 200, 400 mS >> + * Time impacts to gain: 1x, 2x, 4x, 8x. >> + * >> + * => Max total gain is HWGAIN * gain by integration time (8 * 4096) = 32768 >> + * >> + * Using NANO precision for scale we must use scale 64x corresponding gain 1x >> + * to avoid precision loss. (32x would result scale 976 562.5(nanos). >> + */ >> +#define BU27034_SCALE_1X 64 >> + >> +#define BU27034_GSEL_1X 0x00 >> +#define BU27034_GSEL_4X 0x08 >> +#define BU27034_GSEL_16X 0x0a >> +#define BU27034_GSEL_32X 0x0b >> +#define BU27034_GSEL_64X 0x0c >> +#define BU27034_GSEL_256X 0x18 >> +#define BU27034_GSEL_512X 0x19 >> +#define BU27034_GSEL_1024X 0x1a >> +#define BU27034_GSEL_2048X 0x1b >> +#define BU27034_GSEL_4096X 0x1c > > Shouldn't the values be in plain decimal? Why? > Otherwise I would like to understand bit mapping inside these hex values. I like having register values in hex. It makes it obvious they don't necessarily directly match any 'real world' human-readable values. > ... > >> + .indexed = 1 \ > > + Comma at the end. ok. > > ... > >> + static const int reg[] = { >> + [BU27034_CHAN_DATA0] = BU27034_REG_MODE_CONTROL2, >> + [BU27034_CHAN_DATA1] = BU27034_REG_MODE_CONTROL3, >> + [BU27034_CHAN_DATA2] = BU27034_REG_MODE_CONTROL2 > > Ditto. ok. > >> + }; > > ... > >> + struct bu27034_gain_check gains[3] = { >> + { .chan = BU27034_CHAN_DATA0, }, >> + { .chan = BU27034_CHAN_DATA1, }, > > Inner commas are not needed. > >> + { .chan = BU27034_CHAN_DATA2 } > > But here the outer one is good to have. > >> + }; > ok > ... > >> + if (chan == BU27034_CHAN_ALS) { >> + if (val == 0 && val2 == 1000) >> + return 0; >> + else > > Redundant 'else' Thanks for pointing out the unnecessary else. Killing it makes this nicer. >. And probably here is better to use standard pattern for > "checking for error first". I prefer to check for this one specific exactly supported case for ALS channel. Cheking for 'all other possibilities but what we are expecting' would be counter intuitive. > >> + return -EINVAL; >> + } > > ... > >> + if (helper64 < 0xFFFFFFFFFFFFFLLU) { > > Perhaps this needs a definition. I like seeing the value here. It makes this less obfuscating. Comment makes the purpose obvious so adding a define would not really give any extra advantage. >> + helper64 *= gain0; >> + do_div(helper64, ch0); >> + } else { >> + do_div(helper64, ch0); >> + helper64 *= gain0; >> + } > > >> + /* Same overflow check here */ > > Why not a helper function? I actually was thinking of it - but the check is smallish, only done twice and felt a tad too specific to warrant own function. I am not really against adding a function if you feel strongly about this :) > >> + if (helper64 < 0xFFFFFFFFFFFFFLLU) { >> + helper64 *= gain0; >> + do_div(helper64, helper); >> + } else { >> + do_div(helper64, helper); >> + helper64 *= gain0; >> + } > > ... > >> + return (val & BU27034_MASK_VALID); > > Unneeded parentheses. ok. > > ... > >> +retry: >> + /* Get new value from sensor if data is ready */ >> + if (bu27034_has_valid_sample(data)) { >> + ret = regmap_bulk_read(data->regmap, BU27034_REG_DATA0_LO, >> + res, size); >> + if (ret) >> + return ret; >> + >> + bu27034_invalidate_read_data(data); >> + } else { >> + /* No new data in sensor. Wait and retry */ >> + msleep(25); >> + >> + goto retry; > > There is no way out. What might go wrong? Beyond hanging the user process? :) I think you have a point here. I'll add a timeout. > >> + } > > ... > >> + ret = bu27034_get_int_time(data); > > _get_int_time_us() ? (Looking at the below code) > >> + if (ret < 0) >> + return ret; >> + >> + msleep(ret / 1000); > > ... > >> + * Avoid div by zeroi. Not using max() as the data may not be in > > zeroi? > > ... > >> + if (!res[0]) > > Positive conditional? No. Again, we check for the very specific case where res has all bits zeroed. Inverse condition is counter intuitive. > >> + ch0 = 1; >> + else >> + ch0 = le16_to_cpu(res[0]); >> + >> + if (!res[1]) >> + ch1 = 1; > > Ditto. > >> + else >> + ch1 = le16_to_cpu(res[1]); > > But why not to read and convert first and then check. Because conversion is not needed if channel data is zero. > This at least will > correctly compare 0 to the LE16 0 (yes, it's the same for 0, but strictly > speaking the bits order of lvalue and rvalue is different). and hence we check for !res[0] > > ... > >> + switch (mask) { >> + case IIO_CHAN_INFO_INT_TIME: >> + return iio_gts_avail_times(&data->gts, vals, type, length); >> + case IIO_CHAN_INFO_SCALE: >> + return iio_gts_all_avail_scales(&data->gts, vals, type, length); >> + default: >> + break; >> + } >> + >> + return -EINVAL; > > You may do it from default case. > I think we have discussed this one in the past too. I like having return at the end of a non void function. > ... > >> + ret = regmap_read_poll_timeout(data->regmap, BU27034_REG_MODE_CONTROL4, >> + val, (val & BU27034_MASK_VALID), > > Redundant parentheses. ok > >> + BU27034_DATA_WAIT_TIME_US, >> + BU27034_TOTAL_DATA_WAIT_TIME_US); >> + if (ret) { >> + dev_err(data->dev, "data polling %s\n", >> + !(val & BU27034_MASK_VALID) ? "timeout" : "fail"); > > Why not positive conditional in ternary? Because I check this for a specific case: "Was it a timeout?" - not for unspecified "Was it something else but timeout?" > >> + return ret; >> + } > > ... > >> + fwnode = dev_fwnode(dev); >> + if (!fwnode) >> + return -ENODEV; > > So, you deliberately disable a possibility to instantiate this from user space, > why? Thanks! (And Sorry. Jonathan pointed this out to me already in the RFC.) I thought I already fixed this. > > ... > >> + ret = devm_iio_kfifo_buffer_setup(dev, idev, &bu27034_buffer_ops); >> + >> + ret = devm_iio_device_register(dev, idev); > > Don't you find something strange in between? Thanks! > >> + if (ret < 0) >> + return dev_err_probe(dev, ret, >> + "Unable to register iio device\n"); > > ... > >> + { .compatible = "rohm,bu27034", }, > > Inner comma is not needed. ok
A few follow up comments. Jonathan > > > >> obj-$(CONFIG_OPT3001) += opt3001.o > >> obj-$(CONFIG_PA12203001) += pa12203001.o > > > >> +obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o > > > > If you see, most of the components are without vendor prefix, why rohm is > > special? Like you are expecting the very same filename for something else? > > No. I don't. > > Using the vendor prefix in _file name_ was suggested to me by Lee > already a few years ago. And I am actually grateful he did. I've found > that _very_ useful as it simplifies finding the files I am looking for. > What comes to the config option name, being able to easily search for > the configs by vendor name has also been helpful. For filenames I've no strong preference - though perhaps if I'd been starting out today I might have decided the convention was to always add a vendor because of the slight risk of a naming clash. > > > ... > > > >> +#include <linux/iio/iio.h> > >> +#include <linux/iio/buffer.h> > >> +#include <linux/iio/kfifo_buf.h> > > > > Sorted? > > Sure, thanks. > > > > > ... > > > >> +#define BU27034_REG_DATA0_LO 0x50 > >> +#define BU27034_REG_DATA1_LO 0x52 > >> +#define BU27034_REG_DATA2_LO 0x54 > > > > I would drop _LO in all these > > > >> +#define BU27034_REG_DATA2_HI 0x55 > > > > and rename somehow this to something like _END / _MAX (similar to the fields. > > Perhaps you would need _START / _MIN above. > > I don't think this would improve anything. The _LO / _HI are descriptive > as we have only two registers for each channel, _LO and _HI being more > or less standard abbreviations for low and high. Whilst it might be fairly obvious that we are going to do double reg reads against these, having the register names match the datasheet is useful. But in this case the datasheet register map does given them a top level label without the LO, HI so either option would be easy to find. > > > ... > > > >> +/* > >> + * Available scales with gain 1x - 4096x, timings 55, 100, 200, 400 mS > >> + * Time impacts to gain: 1x, 2x, 4x, 8x. > >> + * > >> + * => Max total gain is HWGAIN * gain by integration time (8 * 4096) = 32768 > >> + * > >> + * Using NANO precision for scale we must use scale 64x corresponding gain 1x > >> + * to avoid precision loss. (32x would result scale 976 562.5(nanos). > >> + */ > >> +#define BU27034_SCALE_1X 64 > >> + > >> +#define BU27034_GSEL_1X 0x00 > >> +#define BU27034_GSEL_4X 0x08 > >> +#define BU27034_GSEL_16X 0x0a > >> +#define BU27034_GSEL_32X 0x0b > >> +#define BU27034_GSEL_64X 0x0c > >> +#define BU27034_GSEL_256X 0x18 > >> +#define BU27034_GSEL_512X 0x19 > >> +#define BU27034_GSEL_1024X 0x1a > >> +#define BU27034_GSEL_2048X 0x1b > >> +#define BU27034_GSEL_4096X 0x1c > > > > Shouldn't the values be in plain decimal? > > Why? Normally go with datasheet on this as it makes reviewer simpler. But datasheet is in binary so meh. > > > Otherwise I would like to understand bit mapping inside these hex values. > > I like having register values in hex. It makes it obvious they don't > necessarily directly match any 'real world' human-readable values. Perhaps a cross reference to the table in the spec is appropriate here? whilst there are some patterns they aren't terribly consistent so probably best to just point at the table in the spec. > >> + if (helper64 < 0xFFFFFFFFFFFFFLLU) { > > > > Perhaps this needs a definition. > > I like seeing the value here. It makes this less obfuscating. Comment > makes the purpose obvious so adding a define would not really give any > extra advantage. It's not immediately obvious why it is that many f's. Perhaps change to refer to number of bits (which is what matters really I think) and then use GENMASK() to fill this in? I think it's 52 bits? > > >> + helper64 *= gain0; > >> + do_div(helper64, ch0); > >> + } else { > >> + do_div(helper64, ch0); > >> + helper64 *= gain0; > >> + } > > > > ...> >> + if (!res[0]) > > > > Positive conditional? > > No. Again, we check for the very specific case where res has all bits > zeroed. Inverse condition is counter intuitive. > > > > >> + ch0 = 1; > >> + else > >> + ch0 = le16_to_cpu(res[0]); > >> + > >> + if (!res[1]) > >> + ch1 = 1; > > > > Ditto. > > > >> + else > >> + ch1 = le16_to_cpu(res[1]); > > > > But why not to read and convert first and then check. > > Because conversion is not needed if channel data is zero. Conversion is trivial. I agree with Andy here that the logic would look a bit simpler as (taking it a little further) ch0 = max(1, le16_to_cpu(res[0])); > > > This at least will > > correctly compare 0 to the LE16 0 (yes, it's the same for 0, but strictly > > speaking the bits order of lvalue and rvalue is different). > > and hence we check for !res[0] > > > > > ... > > > >> + switch (mask) { > >> + case IIO_CHAN_INFO_INT_TIME: > >> + return iio_gts_avail_times(&data->gts, vals, type, length); > >> + case IIO_CHAN_INFO_SCALE: > >> + return iio_gts_all_avail_scales(&data->gts, vals, type, length); > >> + default: > >> + break; > >> + } > >> + > >> + return -EINVAL; > > > > You may do it from default case. > > > > I think we have discussed this one in the past too. I like having return > at the end of a non void function. Given all the earlier returns and the fact that the compiler will shout at you if it you can get here and it is missing, I'd also suggest just putting it in the switch statement.
On Thu, 2 Mar 2023 12:58:59 +0200 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > ROHM BU27034 is an ambient light sesnor with 3 channels and 3 photo diodes > capable of detecting a very wide range of illuminance. Typical application > is adjusting LCD and backlight power of TVs and mobile phones. > > Add initial support for the ROHM BU27034 ambient light sensor. > > NOTE: > - Driver exposes 4 channels. One IIO_LIGHT channel providing the > calculated lux values based on measured data from diodes #0 and > #1. Additionally 3 IIO_INTENSITY channels are emitting the raw > register data from all diodes for more intense user-space > computations. > - Sensor has adjustible GAIN values ranging from 1x to 4096x. > - Sensor has adjustible measurement times 5, 55, 100, 200 and > 400 mS. Driver does not support 5 mS which has special > limitations. > - Driver exposes standard 'scale' adjustment which is > implemented by: > 1) Trying to adjust only the GAIN > 2) If GAIN adjustment only can't provide requested > scale, adjusting both the time and the gain is > attempted. > - Driver exposes writable INT_TIME property which can be used > for adjusting the measurement time. Time adjustment will also > cause the driver to adjust the GAIN so that the overall scale > is not changed. Probably want a 'where possible' note here. > - Runtime PM is not implemented. I wouldn't bother noting things that are not implemented. That's a long list! > - Driver starts the measurement on the background when it is > probed. This improves the respnse time to read-requests Spell check. (I often forget to do this myself) > compared to starting the read only when data is requested. > When the most accurate 400 mS measurement time is used, data reads > would last quite long if measurement was started only on > demand. This, however, is not appealing for users who would > prefere power saving over measurement response time. As you noted this needs updating > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> Some comments inline to add to Andy's review.# It's a big driver so we may well need a few more versions to converge completely. Jonathan > > --- > Changes since RFCv1 > - (really) protect read-only registers > - fix get and set gain > - buffered mode > - Protect the whole sequences including meas_en/meas_dis to avoid messing > up the enable / disable order > - typofixes / doc improvements > - change dropped GAIN_SCALE_ITIME_MS() to GAIN_SCALE_ITIME_US() > - use more accurate scale for lux channel (milli lux) > - provide available scales / integration times (using helpers). > - adapt to renamed iio-gts-helpers.h file > - bu27034 - longer lines in Kconfig > - Drop bu27034_meas_en and bu27034_meas_dis wrappers. > - Change device-name from bu27034-als to bu27034 > --- >> diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c > new file mode 100644 > index 000000000000..b8ca29af22e7 > --- /dev/null > +++ b/drivers/iio/light/rohm-bu27034.c > @@ -0,0 +1,1491 @@ > +/* > + * The BU27034 does not have interrupt or any other mechanism of triggering > + * the data read when measurement has finished. Hence we poll the VALID bit in > + * a thread. We will try to wake the thread BU27034_MEAS_WAIT_PREMATURE_MS > + * milliseconds before the expected sampling time to prevent the drifting. Eg, > + * If we constantly wake up a bit too late we would eventually skip a sample. Lazier approach would be to just sent the sampling frequency at twice the expected frequency and you'll never miss a sample unless you the wake up is delayed massively for some reason. Particularly 'fresh' data might not matter enough that half a cycle late is a problem. > + * And because the sleep can't wake up _exactly_ at given time this would be > + * inevitable even if the sensor clock would be perfectly phase-locked to CPU > + * clock - which we can't say is the case. > + * > + * This is still fragile. No matter how big advance do we have, we will still > + * risk of losing a sample because things can in a rainy-day skenario be > + * delayed a lot. Yet, more we reserve the time for polling, more we also lose > + * the performance by spending cycles polling the register. So, selecting this > + * value is a balancing dance between severity of wasting CPU time and severity > + * of losing samples. > + * > + * In most cases losing the samples is not _that_ crucial because light levels > + * tend to change slowly. > + */ > +#define BU27034_MEAS_WAIT_PREMATURE_MS 5 > +#define BU27034_DATA_WAIT_TIME_US 1000 > +#define BU27034_TOTAL_DATA_WAIT_TIME_US (BU27034_MEAS_WAIT_PREMATURE_MS * 1000) > + Single blank line is enough to separate sections. > + > +enum { > + BU27034_CHAN_ALS, > + BU27034_CHAN_DATA0, > + BU27034_CHAN_DATA1, > + BU27034_CHAN_DATA2, > + BU27034_NUM_CHANS > +}; > +static const struct iio_chan_spec bu27034_channels[] = { > + { > + .type = IIO_LIGHT, > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | > + BIT(IIO_CHAN_INFO_SCALE), What is this scale for? Given the channel is computed from various different inputs, is there a clear definition of how it is scaled? What does a write to it mean? > + .channel = BU27034_CHAN_ALS, > + .scan_index = BU27034_CHAN_ALS, > + .scan_type = { > + .sign = 'u', > + .realbits = 32, > + .storagebits = 32, > + .endianness = IIO_CPU, > + }, > + }, > + /* > + * The BU27034 DATA0 and DATA1 channels are both on the visible light > + * area (mostly). The data0 sensitivity peaks at 500nm, DATA1 at 600nm. > + * These wave lengths are pretty much on the border of colours making > + * these a poor candidates for R/G/B standardization. Hence they're both > + * marked as clear channels > + */ > + BU27034_CHAN_DATA(DATA0, IIO_MOD_LIGHT_CLEAR), > + BU27034_CHAN_DATA(DATA1, IIO_MOD_LIGHT_CLEAR), > + BU27034_CHAN_DATA(DATA2, IIO_MOD_LIGHT_IR), > + IIO_CHAN_SOFT_TIMESTAMP(4), > +}; > +/* Caller should hold the lock to protect data->int_time */ > +static int bu27034_set_int_time(struct bu27034_data *data, int time) > +{ > + int ret; > + > + ret = iio_gts_find_sel_by_int_time(&data->gts, time); > + if (ret < 0) > + return ret; > + > + return regmap_update_bits(data->regmap, BU27034_REG_MODE_CONTROL1, > + BU27034_MASK_MEAS_MODE, ret); > + > + return time; ? Never get to this. > +} > + > +/* > + * We try to change the time in such way that the scale is maintained for > + * given channels by adjusting gain so that it compensates the time change. > + */ > +static int bu27034_try_set_int_time(struct bu27034_data *data, int time_us) > +{ > + int ret, int_time_old, int_time_new, i; > + struct bu27034_gain_check gains[3] = { > + { .chan = BU27034_CHAN_DATA0, }, > + { .chan = BU27034_CHAN_DATA1, }, > + { .chan = BU27034_CHAN_DATA2 } > + }; > + int numg = ARRAY_SIZE(gains); > + > + mutex_lock(&data->mutex); > + ret = bu27034_get_int_time(data); > + if (ret < 0) > + goto unlock_out; > + > + int_time_old = ret; > + > + ret = bu27034_validate_int_time(data, time_us); > + if (ret < 0) { > + dev_err(data->dev, "Unsupported integration time %u\n", > + time_us); > + > + goto unlock_out; > + } > + > + int_time_new = ret; > + > + if (int_time_new == int_time_old) { > + ret = 0; > + goto unlock_out; > + } > + > + for (i = 0; i < numg; i++) { > + ret = bu27034_get_gain(data, gains[i].chan, > + &gains[i].old_gain); > + if (ret) > + goto unlock_out; > + > + gains[i].new_gain = gains[i].old_gain * int_time_old / > + int_time_new; > + > + if (!iio_gts_valid_gain(&data->gts, gains[i].new_gain)) { > + int scale1, scale2; > + bool ok; > + > + _bu27034_get_scale(data, gains[i].chan, &scale1, &scale2); > + dev_dbg(data->dev, > + "chan %u, can't support time %u with scale %u %u\n", > + gains[i].chan, time_us, scale1, scale2); > + > + /* > + * As Jonathan put it, if caller requests for Probably don't reference me directly in a driver. Keep the 'blame' for the email threads :) Comment is fine otherwise. > + * integration time change and we can't support the > + * scale - then the caller should be prepared to > + * 'pick up the pieces and deal with the fact that the > + * scale changed'. > + */ > + ret = iio_find_closest_gain_low(&data->gts, > + gains[i].new_gain, &ok); > + > + if (!ok) { > + dev_dbg(data->dev, > + "optimal gain out of range for chan %u\n", > + gains[i].chan); > + } > + if (ret < 0) { > + dev_warn(data->dev, > + "Total gain increase. Risk of saturation"); dev_dbg() > + ret = iio_gts_get_min_gain(&data->gts); > + if (ret < 0) > + goto unlock_out; > + } > + dev_warn(data->dev, "chan %u scale changed\n", > + gains[i].chan); dev_dbg() We shouldn't convey this sort of info via the kernel log. Most users would never see it anyway!. > + gains[i].new_gain = ret; > + dev_dbg(data->dev, "chan %u new gain %u\n", > + gains[i].chan, gains[i].new_gain); > + } > + } > + > + /* > + * The new integration time can be supported while keeping the scale of > + * channels intact by tuning the gains. This comment is in a path that is hit event if we go through the warnings above that say this isn't true. > + */ > + for (i = 0; i < numg; i++) { > + ret = bu27034_set_gain(data, gains[i].chan, gains[i].new_gain); > + if (ret) > + goto unlock_out; > + } > + > + ret = bu27034_set_int_time(data, int_time_new); > + > +unlock_out: > + mutex_unlock(&data->mutex); > + > + return ret; > +} > + > +static int bu27034_set_scale(struct bu27034_data *data, int chan, > + int val, int val2) > +{ > + int ret, time_sel, gain_sel, i; > + bool found = false; > + > + if (chan == BU27034_CHAN_ALS) { > + if (val == 0 && val2 == 1000) > + return 0; > + else > + return -EINVAL; > + } > + > + mutex_lock(&data->mutex); > + ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL1, &time_sel); > + if (ret) > + goto unlock_out; > + > + ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts, time_sel, > + val, val2 * 1000, &gain_sel); > + if (ret) { > + /* > + * Could'n support scale with given time. Need to change time. Could not > + * We still want to maintain the scale for all channels > + */ > + int new_time_sel; > + struct bu27034_gain_check gains[2]; > + > + /* > + * Populate information for the two other channels which should > + * maintain the scale. > + */ > + if (chan == BU27034_CHAN_DATA0) { > + gains[0].chan = BU27034_CHAN_DATA1; > + gains[1].chan = BU27034_CHAN_DATA2; > + } else if (chan == BU27034_CHAN_DATA1) { > + gains[0].chan = BU27034_CHAN_DATA0; > + gains[1].chan = BU27034_CHAN_DATA2; > + } else { > + gains[0].chan = BU27034_CHAN_DATA0; > + gains[1].chan = BU27034_CHAN_DATA1; > + } > + > + for (i = 0; i < 2; i++) { > + ret = bu27034_get_gain(data, gains[i].chan, > + &gains[i].old_gain); > + if (ret) > + goto unlock_out; > + } > + > + /* > + * Iterate through all the times to see if we find one which > + * can support requested scale for requested channel, while > + * maintaining the scale for other channels > + */ > + for (i = 0; i < data->gts.num_itime; i++) { > + new_time_sel = data->gts.itime_table[i].sel; > + > + if (new_time_sel == time_sel) > + continue; > + > + /* Can we provide requested scale with this time? */ > + ret = iio_gts_find_gain_sel_for_scale_using_time( > + &data->gts, new_time_sel, val, val2 * 1000, > + &gain_sel); > + if (ret) > + continue; > + > + /* Can the two other channels maintain scales? */ > + ret = iio_gts_find_new_gain_sel_by_old_gain_time( > + &data->gts, gains[0].old_gain, time_sel, > + new_time_sel, &gains[0].new_gain); > + if (ret) > + continue; > + > + ret = iio_gts_find_new_gain_sel_by_old_gain_time( > + &data->gts, gains[1].old_gain, time_sel, > + new_time_sel, &gains[1].new_gain); > + if (!ret) { > + /* Yes - we found suitable time */ > + found = true; > + break; > + } > + } > + if (!found) { > + dev_err(data->dev, > + "Can't set scale maintaining other channels\n"); We shouldn't print an error to the log in a path like this as we just gave userspace an easy way to overflow the log. Just return -EINVAL without the print. Fine to add it as dev_dbg() though if you like. > + ret = -EINVAL; > + > + goto unlock_out; > + } > + > + for (i = 0; i < 2; i++) { > + ret = bu27034_set_gain(data, gains[i].chan, > + gains[i].new_gain); > + if (ret) > + goto unlock_out; > + } > + > + ret = regmap_update_bits(data->regmap, BU27034_REG_MODE_CONTROL1, > + BU27034_MASK_MEAS_MODE, new_time_sel); > + if (ret) > + goto unlock_out; > + } > + > + ret = bu27034_write_gain_sel(data, chan, gain_sel); > +unlock_out: > + mutex_unlock(&data->mutex); > + > + return ret; > +} ... > + > +static bool bu27034_has_valid_sample(struct bu27034_data *data) > +{ > + int ret, val; > + > + ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL4, &val); > + if (ret) > + dev_err(data->dev, "Read failed %d\n", ret); Don't use the value if error occurred as unknown state. Just do return false in that path. > + > + return (val & BU27034_MASK_VALID); No brackets needed. > +} ... > +static int bu27034_get_single_result(struct bu27034_data *data, int chan, > + int *val) > +{ > + int ret; > + > + One blank line is enough! > + ret = bu27034_meas_set(data, true); > + if (ret) > + return ret; > + > + ret = bu27034_get_int_time(data); > + if (ret < 0) > + return ret; > + > + msleep(ret / 1000); > + > + return bu27034_read_result(data, chan, val); > +} > + > +/* > + * The formula given by vendor for computing luxes out of data0 and data1 > + * (in open air) is as follows: > + * > + * Let's mark: > + * D0 = data0/ch0_gain/meas_time_ms * 25600 > + * D1 = data1/ch1_gain/meas_time_ms * 25600 > + * > + * Then: > + * if (D1/D0 < 0.87) > + * lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 0.87) * 3.45 + 1) > + * else if (D1/D0 < 1) > + * lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 0.87) * 0.385 + 1) > + * else > + * lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 2) * -0.05 + 1) > + * > + * We use it here. Users who have for example some colored lens > + * need to modify the calculation but I hope this gives a starting point for > + * those working with such devices. > + */ > + > +static int bu27034_calc_lux(struct bu27034_data *data, __le16 *res, int *val) As you are going to put it in the buffer, make val a fixed size integer. The current approach of calculate in an int and copy to a u32 is a bit nasty. Of course if there is a chance of a large enough value you'll have to be careful for the unsigned to signed conversion on 32 bit platforms. I doubt there is, but a comment saying why not would be great in the code that is hit from read_raw() > +{ > + unsigned int gain0, gain1, meastime; > + unsigned int d1_d0_ratio_scaled; > + u16 ch0, ch1; > + u64 helper64; > + int ret; > + > + /* > + * We return 0 luxes if calculation fails. This should be reasonably > + * easy to spot from the buffers especially if raw-data channels show > + * valid values > + */ > + *val = 0; > + > + /* > + * Avoid div by zeroi. Not using max() as the data may not be in > + * native endianes > + */ > + if (!res[0]) > + ch0 = 1; > + else > + ch0 = le16_to_cpu(res[0]); As per other branch of the thread. ch0 = max(1, le16_to_cpu(res[0]); would be cleaner. > + > + if (!res[1]) > + ch1 = 1; > + else > + ch1 = le16_to_cpu(res[1]); > + > + > + ret = bu27034_get_gain(data, BU27034_CHAN_DATA0, &gain0); > + if (ret) > + return ret; > + > + ret = bu27034_get_gain(data, BU27034_CHAN_DATA1, &gain1); > + if (ret) > + return ret; > + > + ret = bu27034_get_int_time(data); > + if (ret < 0) > + return ret; > + > + meastime = ret; > + > + d1_d0_ratio_scaled = (unsigned int)ch1 * (unsigned int)gain0 * 100; > + helper64 = (u64)ch1 * (u64)gain0 * 100LLU; > + > + if (helper64 != d1_d0_ratio_scaled) { > + unsigned int div = (unsigned int)ch0 * gain1; > + > + do_div(helper64, div); > + d1_d0_ratio_scaled = helper64; > + } else { > + d1_d0_ratio_scaled /= ch0 * gain1; > + } > + > + if (d1_d0_ratio_scaled < 87) > + *val = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 0); > + else if (d1_d0_ratio_scaled < 100) > + *val = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 1); > + else > + *val = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 2); > + > + return 0; > + > +} > + > + > +static int bu27034_read_raw(struct iio_dev *idev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct bu27034_data *data = iio_priv(idev); > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_INT_TIME: > + *val = bu27034_get_int_time(data); > + if (*val < 0) > + return *val; > + > + /* > + * We use 50000 uS internally for all calculations and only > + * convert it to 55000 before returning it to the user. > + * > + * This is because the data-sheet says the time is 55 mS - but > + * vendor provided computations used 50 mS. > + */ > + if (*val == 50000) > + *val = 55000; > + > + return IIO_VAL_INT; > + > + case IIO_CHAN_INFO_SCALE: > + return bu27034_get_scale(data, chan->channel, val, val2); > + > + case IIO_CHAN_INFO_RAW: > + { > + if (chan->type != IIO_INTENSITY) > + return -EINVAL; > + > + if (chan->channel < BU27034_CHAN_DATA0 || > + chan->channel > BU27034_CHAN_DATA2) > + return -EINVAL; > + > + /* Don't mess with measurement enabling while buffering */ > + ret = iio_device_claim_direct_mode(idev); > + if (ret) > + return ret; > + > + mutex_lock(&data->mutex); > + /* > + * Reading one channel at a time is ineffiecient but we don't > + * care here. Buffered version should be used if performance is > + * an issue. > + */ > + ret = bu27034_get_single_result(data, chan->channel, val); > + if (ret) > + goto release_err_out; Move the if (ret) after the locks are released then you don't need the messy goto. > + > + mutex_unlock(&data->mutex); > + iio_device_release_direct_mode(idev); > + > + return IIO_VAL_INT; > + } > + > + case IIO_CHAN_INFO_PROCESSED: > + if (chan->type != IIO_LIGHT) > + return -EINVAL; > + > + /* Don't mess with measurement enabling while buffering */ > + ret = iio_device_claim_direct_mode(idev); > + if (ret) > + return ret; > + > + mutex_lock(&data->mutex); See below. I would factor out the rest of this so that you can unconditionally unlock and then check the return value. > + ret = bu27034_meas_set(data, true); > + if (ret) > + goto release_err_out; > + > + ret = bu27034_get_lux(data, val); > + if (ret) > + goto release_err_out; > + > + ret = bu27034_meas_set(data, false); > + if (ret) > + dev_err(data->dev, "failed to disable measurement\n"); > + > + mutex_unlock(&data->mutex); > + iio_device_release_direct_mode(idev); > + > + return IIO_VAL_INT; > + > + default: > + return -EINVAL; > + > + } > + > +release_err_out: > + mutex_unlock(&data->mutex); > + iio_device_release_direct_mode(idev); > + Having goto targets that are outside of case statements within the arms of a switch is usually a strong hint that it makes sense to factor out the code > + return ret; > +} > + blank line here. > +static const struct iio_info bu27034_info = { > + .read_raw = &bu27034_read_raw, > + .write_raw = &bu27034_write_raw, > + .read_avail = &bu27034_read_avail, > +}; > + ... > + > +static int bu27034_probe(struct i2c_client *i2c) > +{ > + struct device *dev = &i2c->dev; > + struct fwnode_handle *fwnode; > + struct bu27034_data *data; > + struct regmap *regmap; > + struct iio_dev *idev; > + unsigned int part_id; > + int ret; > + > + regmap = devm_regmap_init_i2c(i2c, &bu27034_regmap); > + if (IS_ERR(regmap)) > + return dev_err_probe(dev, PTR_ERR(regmap), > + "Failed to initialize Regmap\n"); > + > + fwnode = dev_fwnode(dev); > + if (!fwnode) > + return -ENODEV; You've already noted this shouldn't be here. > + > + idev = devm_iio_device_alloc(dev, sizeof(*data)); > + if (!idev) > + return -ENOMEM; > + > + ret = devm_regulator_get_enable(dev, "vdd"); > + if (ret && ret != -ENODEV) > + return dev_err_probe(dev, ret, "Failed to get regulator\n"); > + > + data = iio_priv(idev); > + > + ret = regmap_read(regmap, BU27034_REG_SYSTEM_CONTROL, &part_id); As it's not all of the register I'd rename the temporary variable to val or reg or something along those lines. > + if (ret) > + return dev_err_probe(dev, ret, "Failed to access sensor\n"); > + > + part_id &= BU27034_MASK_PART_ID; FIELD_GET() even when it's lower bits as then there is no need for a reviewer to confirm that it is the lower bits. Then you can just do if (FIELD_GET(BU27034_MASK_PART_ID, reg) != BU27034_ID) > + > + if (part_id != BU27034_ID) > + dev_warn(dev, "unsupported device 0x%x\n", part_id); I'd adjust that to "unknown device" or "unrecognised device" as it might well be supported just fine based on the compatible fallback, we just have no way of knowing if it is. > + > + ret = iio_init_iio_gts(BU27034_SCALE_1X, 0, bu27034_gains, > + ARRAY_SIZE(bu27034_gains), bu27034_itimes, > + ARRAY_SIZE(bu27034_itimes), &data->gts); > + if (ret) > + return ret; > + > + ret = devm_iio_gts_build_avail_tables(dev, &data->gts); > + if (ret) > + return ret; > + > + mutex_init(&data->mutex); > + data->regmap = regmap; > + data->dev = dev; > + > + idev->channels = bu27034_channels; > + idev->num_channels = ARRAY_SIZE(bu27034_channels); > + idev->name = "bu27034"; > + idev->info = &bu27034_info; > + > + idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE; > + idev->available_scan_masks = bu27034_scan_masks; > + > + ret = bu27034_chip_init(data); > + if (ret) > + return ret; > + > + ret = devm_iio_kfifo_buffer_setup(dev, idev, &bu27034_buffer_ops); check ret and error out on failure. > + > + ret = devm_iio_device_register(dev, idev); > + if (ret < 0) > + return dev_err_probe(dev, ret, > + "Unable to register iio device\n"); > + > + return ret; > +} > + > +static const struct of_device_id bu27034_of_match[] = { > + { .compatible = "rohm,bu27034", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, bu27034_of_match); > + > +static struct i2c_driver bu27034_i2c_driver = { > + .driver = { > + .name = "bu27034-als", > + .of_match_table = bu27034_of_match, > + }, > + .probe_new = bu27034_probe, I'd just use a single space in all cases before the = Attempts to align the values in this structure don't really help readability and often end up messier over time. > +}; > +module_i2c_driver(bu27034_i2c_driver); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>"); > +MODULE_DESCRIPTION("ROHM BU27034 ambient light sensor driver");
On 3/4/23 21:02, Jonathan Cameron wrote: >>> ... >>> >>>> +/* >>>> + * Available scales with gain 1x - 4096x, timings 55, 100, 200, 400 mS >>>> + * Time impacts to gain: 1x, 2x, 4x, 8x. >>>> + * >>>> + * => Max total gain is HWGAIN * gain by integration time (8 * 4096) = 32768 >>>> + * >>>> + * Using NANO precision for scale we must use scale 64x corresponding gain 1x >>>> + * to avoid precision loss. (32x would result scale 976 562.5(nanos). >>>> + */ >>>> +#define BU27034_SCALE_1X 64 >>>> + >>>> +#define BU27034_GSEL_1X 0x00 >>>> +#define BU27034_GSEL_4X 0x08 >>>> +#define BU27034_GSEL_16X 0x0a >>>> +#define BU27034_GSEL_32X 0x0b >>>> +#define BU27034_GSEL_64X 0x0c >>>> +#define BU27034_GSEL_256X 0x18 >>>> +#define BU27034_GSEL_512X 0x19 >>>> +#define BU27034_GSEL_1024X 0x1a >>>> +#define BU27034_GSEL_2048X 0x1b >>>> +#define BU27034_GSEL_4096X 0x1c >>> >>> Shouldn't the values be in plain decimal? >> >> Why? > > Normally go with datasheet on this as it makes reviewer simpler. > But datasheet is in binary so meh. > >> >>> Otherwise I would like to understand bit mapping inside these hex values. >> >> I like having register values in hex. It makes it obvious they don't >> necessarily directly match any 'real world' human-readable values. > > Perhaps a cross reference to the table in the spec is appropriate here? I think adding a reference to the table in the data-sheet is good. Although - I gave some feedback about the data-sheet inside the company. It may be we will eventually see another version of it... > whilst there are some patterns they aren't terribly consistent so probably > best to just point at the table in the spec. > > >>>> + if (helper64 < 0xFFFFFFFFFFFFFLLU) { >>> >>> Perhaps this needs a definition. >> >> I like seeing the value here. It makes this less obfuscating. Comment >> makes the purpose obvious so adding a define would not really give any >> extra advantage. > > It's not immediately obvious why it is that many f's. Perhaps change > to refer to number of bits (which is what matters really I think) > and then use GENMASK() to fill this in? I think it's 52 bits? I am personally not used to the GENMASK(). I am always wondering whether the parameters were start + end bits, or star bit + number of set bits or ... It's somehow easier for me to understand the hex values - (especially when they are composed of all fff's or 1, 3, 7 or 2, 4, 8 ... ). Well, I understand that is not universally true though so GENMASK() can for sure be simpler for most others - and it does not hide the value as define would do. So yes, GENMASK() could make sense here. >>>> + helper64 *= gain0; >>>> + do_div(helper64, ch0); >>>> + } else { >>>> + do_div(helper64, ch0); >>>> + helper64 *= gain0; >>>> + } > >>> >>> ...> >> + if (!res[0]) >>> >>> Positive conditional? >> >> No. Again, we check for the very specific case where res has all bits >> zeroed. Inverse condition is counter intuitive. >> >>> >>>> + ch0 = 1; >>>> + else >>>> + ch0 = le16_to_cpu(res[0]); >>>> + >>>> + if (!res[1]) >>>> + ch1 = 1; >>> >>> Ditto. >>> >>>> + else >>>> + ch1 = le16_to_cpu(res[1]); >>> >>> But why not to read and convert first and then check. >> >> Because conversion is not needed if channel data is zero. > > Conversion is trivial. I agree with Andy here that the logic would look > a bit simpler as (taking it a little further) > > ch0 = max(1, le16_to_cpu(res[0])); It's strange how differently we read the code. For me: if (!val) ch = 1; else ch = le16_to_cpu(val); tells what is happening at a glance whereas the: ch0 = max(1, le16_to_cpu(res[0])); really requires some focusing to see what is going on. For me it is both less clear and less efficient :( But alas, if this is what is preferred by both of you, then I guess that's what it will look like. >>> >>>> + switch (mask) { >>>> + case IIO_CHAN_INFO_INT_TIME: >>>> + return iio_gts_avail_times(&data->gts, vals, type, length); >>>> + case IIO_CHAN_INFO_SCALE: >>>> + return iio_gts_all_avail_scales(&data->gts, vals, type, length); >>>> + default: >>>> + break; >>>> + } >>>> + >>>> + return -EINVAL; >>> >>> You may do it from default case. >>> >> >> I think we have discussed this one in the past too. I like having return >> at the end of a non void function. > > Given all the earlier returns and the fact that the compiler will shout at > you if it you can get here and it is missing, I'd also suggest just putting > it in the switch statement. Ok. IIO is your territory. If the roles were switched I would definitely ask for having the returns at the end of the function - and at the end of the function only (when possible w/o lot of extra complication). So perhaps I just have to accept that you want to have it your way with IIO :) Yours, -- Matti
On 3/4/23 22:17, Jonathan Cameron wrote: > On Thu, 2 Mar 2023 12:58:59 +0200 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> +/* >> + * The BU27034 does not have interrupt or any other mechanism of triggering >> + * the data read when measurement has finished. Hence we poll the VALID bit in >> + * a thread. We will try to wake the thread BU27034_MEAS_WAIT_PREMATURE_MS >> + * milliseconds before the expected sampling time to prevent the drifting. Eg, >> + * If we constantly wake up a bit too late we would eventually skip a sample. > > Lazier approach would be to just sent the sampling frequency at twice the > expected frequency and you'll never miss a sample unless you the wake up is > delayed massively for some reason. Particularly 'fresh' data might not matter > enough that half a cycle late is a problem. Hmm. Do I read this right - You suggest we drop the polling loop for valid bit and just always sleep for int_time / 2 if data was not valid? I don't know. That would probably make the time-stamps for buffered results to be jumping quite a bit - especially with the longer integration times. >> + * And because the sleep can't wake up _exactly_ at given time this would be >> + * inevitable even if the sensor clock would be perfectly phase-locked to CPU >> + * clock - which we can't say is the case. >> + * >> + * This is still fragile. No matter how big advance do we have, we will still >> + * risk of losing a sample because things can in a rainy-day skenario be >> + * delayed a lot. Yet, more we reserve the time for polling, more we also lose >> + * the performance by spending cycles polling the register. So, selecting this >> + * value is a balancing dance between severity of wasting CPU time and severity >> + * of losing samples. >> + * >> + * In most cases losing the samples is not _that_ crucial because light levels >> + * tend to change slowly. >> + */ >> +#define BU27034_MEAS_WAIT_PREMATURE_MS 5 >> +#define BU27034_DATA_WAIT_TIME_US 1000 >> +#define BU27034_TOTAL_DATA_WAIT_TIME_US (BU27034_MEAS_WAIT_PREMATURE_MS * 1000) > >> +static const struct iio_chan_spec bu27034_channels[] = { >> + { >> + .type = IIO_LIGHT, >> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | >> + BIT(IIO_CHAN_INFO_SCALE), > > What is this scale for? The scale is to inform users that we return data using milli lux. > Given the channel is computed from various different inputs, is there a > clear definition of how it is scaled? What does a write to it mean? Nothing. writing anything else but milli lux scale fails with -EINVAL. I guess I am doing something in an unusual way here :) Do you have a suggestion for me? >> + /* >> + * As Jonathan put it, if caller requests for > > Probably don't reference me directly in a driver. Keep the 'blame' for the > email threads :) Comment is fine otherwise. Okay, okay :) I just liked the expression and didn't want to take the credit for it ;) >> + >> + /* >> + * The new integration time can be supported while keeping the scale of >> + * channels intact by tuning the gains. > > This comment is in a path that is hit event if we go through the warnings > above that say this isn't true. Oh! Valid point! This changed when I allowed gain to be changed - need to drop the comment. Thanks! >> + >> +static int bu27034_calc_lux(struct bu27034_data *data, __le16 *res, int *val) > > As you are going to put it in the buffer, make val a fixed size integer. > The current approach of calculate in an int and copy to a u32 is a bit nasty. > Of course if there is a chance of a large enough value you'll have to be careful > for the unsigned to signed conversion on 32 bit platforms. I doubt there is, but > a comment saying why not would be great in the code that is hit from read_raw() This same ..._calc_lux() is used also from the read_raw. I'd rather keep this using ints so there would be no need for involving u32 in read_raw() path. However, you are correct in that the current calling from buffered read where we pass pointer to u32 here - is nasty. I'll add temporary int in the calling function there, and add casting with a comment when storing value to scan.lux. Thanks for pointing this out! There was also missing an error return problem - see below. >> + >> + if (d1_d0_ratio_scaled < 87) >> + *val = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 0); >> + else if (d1_d0_ratio_scaled < 100) >> + *val = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 1); >> + else >> + *val = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 2); >> + The bu27034_fixp_calc_lx() might return -EINVAL - which we missed here. I'll fix also this one. >> + >> + case IIO_CHAN_INFO_PROCESSED: >> + if (chan->type != IIO_LIGHT) >> + return -EINVAL; >> + >> + /* Don't mess with measurement enabling while buffering */ >> + ret = iio_device_claim_direct_mode(idev); >> + if (ret) >> + return ret; >> + >> + mutex_lock(&data->mutex); > > See below. I would factor out the rest of this so that you can > unconditionally unlock and then check the return value. Yes. moving measurement start/stop in bu27034_get_lux() makes this much nicer. I wonder how I didn't see that myself - thanks. >> + >> + ret = regmap_read(regmap, BU27034_REG_SYSTEM_CONTROL, &part_id); > > As it's not all of the register I'd rename the temporary variable to > val or reg or something along those lines. I still like having the variable named part_id - as it makes the check obvious. What I did was adding another temporary variable 'reg' and doing: part_id = FIELD_GET(BU27034_MASK_PART_ID, reg); and then using the part_id in if() and dev_warn(). > >> + if (ret) >> + return dev_err_probe(dev, ret, "Failed to access sensor\n"); >> + >> + part_id &= BU27034_MASK_PART_ID; > > FIELD_GET() even when it's lower bits as then there is no need for > a reviewer to confirm that it is the lower bits. > Then you can just do > > if (FIELD_GET(BU27034_MASK_PART_ID, reg) != BU27034_ID) > >> + >> + if (part_id != BU27034_ID) >> + dev_warn(dev, "unsupported device 0x%x\n", part_id); > > I'd adjust that to "unknown device" or "unrecognised device" as it might > well be supported just fine based on the compatible fallback, we just have > no way of knowing if it is. > Hmm. Won't promise but maybe I have the time to finish v3 tonight. We can then continue discussions towards the v4 :) Yours, -- Matti
On 3/4/23 22:17, Jonathan Cameron wrote: > On Thu, 2 Mar 2023 12:58:59 +0200 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > > As per other branch of the thread. > > ch0 = max(1, le16_to_cpu(res[0]); > > would be cleaner. I tried this out. Comparing u16 to literal 1 results comparison of values with different sizes: ./include/linux/minmax.h:20:28: warning: comparison of distinct pointer types lacks a cast (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1))) ^ ./include/linux/minmax.h:26:4: note: in expansion of macro ‘__typecheck’ (__typecheck(x, y) && __no_side_effects(x, y)) ^~~~~~~~~~~ ./include/linux/minmax.h:36:24: note: in expansion of macro ‘__safe_cmp’ __builtin_choose_expr(__safe_cmp(x, y), \ ^~~~~~~~~~ ./include/linux/minmax.h:74:19: note: in expansion of macro ‘__careful_cmp’ #define max(x, y) __careful_cmp(x, y, >) ^~~~~~~~~~~~~ drivers/iio/light/rohm-bu27034.c:1057:8: note: in expansion of macro ‘max’ ch0 = max(1, ch0); I could work around this by doing: const u16 min_ch_val = 1; ... ch0 = max(min_ch_val, le16_to_cpu(res[0])); but I think that would really be obfuscating the meaning. I assume ch0 = max((u16)1, le16_to_cpu(res[0])); might work too - but to me it's pretty ugly. The more I am looking at this, the stronger I feel we should really just write this as it was. Check if res[0] contains the only unsafe data "!res[0]" - and if yes, set it to 1. The comment above it will clarify it to a reader wondering what happens. I will leave it like it was in v2 for v3. If you still feel strong about it then we need to continue rubbing it. Yours, -- Matti
On Sun, Mar 05, 2023 at 03:10:38PM +0200, Matti Vaittinen wrote: > On 3/4/23 22:17, Jonathan Cameron wrote: > > On Thu, 2 Mar 2023 12:58:59 +0200 > > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > > > > As per other branch of the thread. > > > > ch0 = max(1, le16_to_cpu(res[0]); > > > would be cleaner. > > I tried this out. Comparing u16 to literal 1 results comparison of values > with different sizes: > > ./include/linux/minmax.h:20:28: warning: comparison of distinct pointer > types lacks a cast > (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1))) > ^ > ./include/linux/minmax.h:26:4: note: in expansion of macro ‘__typecheck’ > (__typecheck(x, y) && __no_side_effects(x, y)) > ^~~~~~~~~~~ > ./include/linux/minmax.h:36:24: note: in expansion of macro ‘__safe_cmp’ > __builtin_choose_expr(__safe_cmp(x, y), \ > ^~~~~~~~~~ > ./include/linux/minmax.h:74:19: note: in expansion of macro ‘__careful_cmp’ > #define max(x, y) __careful_cmp(x, y, >) > ^~~~~~~~~~~~~ > drivers/iio/light/rohm-bu27034.c:1057:8: note: in expansion of macro ‘max’ > ch0 = max(1, ch0); > > > I could work around this by doing: > > const u16 min_ch_val = 1; > > ... > > ch0 = max(min_ch_val, le16_to_cpu(res[0])); > > but I think that would really be obfuscating the meaning. I assume > > ch0 = max((u16)1, le16_to_cpu(res[0])); > > might work too - but to me it's pretty ugly. That's why we have max_t() and clamp_val(). And you know that. > > The more I am looking at this, the stronger I feel we should really just > write this as it was. Check if res[0] contains the only unsafe data > "!res[0]" - and if yes, set it to 1. The comment above it will clarify it to > a reader wondering what happens. > > I will leave it like it was in v2 for v3. If you still feel strong about it > then we need to continue rubbing it. You need to convert bit ordering first, then check for 0. It would at least make more sense. (Today is 0 you are comparing with, tomorrow it might be 0xfffe, which is different to 0x7fff).
On Sun, 5 Mar 2023 14:22:51 +0200 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 3/4/23 22:17, Jonathan Cameron wrote: > > On Thu, 2 Mar 2023 12:58:59 +0200 > > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > > > >> +/* > >> + * The BU27034 does not have interrupt or any other mechanism of triggering > >> + * the data read when measurement has finished. Hence we poll the VALID bit in > >> + * a thread. We will try to wake the thread BU27034_MEAS_WAIT_PREMATURE_MS > >> + * milliseconds before the expected sampling time to prevent the drifting. Eg, > >> + * If we constantly wake up a bit too late we would eventually skip a sample. > > > > Lazier approach would be to just sent the sampling frequency at twice the > > expected frequency and you'll never miss a sample unless you the wake up is > > delayed massively for some reason. Particularly 'fresh' data might not matter > > enough that half a cycle late is a problem. > > Hmm. Do I read this right - You suggest we drop the polling loop for > valid bit and just always sleep for int_time / 2 if data was not valid? Yes. There are costs to both methods, but the advantage here is that the chance of being so late you miss becomes much less. > > I don't know. That would probably make the time-stamps for buffered > results to be jumping quite a bit - especially with the longer > integration times. True enough. They are probably fairly noisy either way but this would make them worse. > > >> + * And because the sleep can't wake up _exactly_ at given time this would be > >> + * inevitable even if the sensor clock would be perfectly phase-locked to CPU > >> + * clock - which we can't say is the case. > >> + * > >> + * This is still fragile. No matter how big advance do we have, we will still > >> + * risk of losing a sample because things can in a rainy-day skenario be > >> + * delayed a lot. Yet, more we reserve the time for polling, more we also lose > >> + * the performance by spending cycles polling the register. So, selecting this > >> + * value is a balancing dance between severity of wasting CPU time and severity > >> + * of losing samples. > >> + * > >> + * In most cases losing the samples is not _that_ crucial because light levels > >> + * tend to change slowly. > >> + */ > >> +#define BU27034_MEAS_WAIT_PREMATURE_MS 5 > >> +#define BU27034_DATA_WAIT_TIME_US 1000 > >> +#define BU27034_TOTAL_DATA_WAIT_TIME_US (BU27034_MEAS_WAIT_PREMATURE_MS * 1000) > > > >> +static const struct iio_chan_spec bu27034_channels[] = { > >> + { > >> + .type = IIO_LIGHT, > >> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | > >> + BIT(IIO_CHAN_INFO_SCALE), > > > > What is this scale for? > > The scale is to inform users that we return data using milli lux. > > > Given the channel is computed from various different inputs, is there a > > clear definition of how it is scaled? What does a write to it mean? > > Nothing. writing anything else but milli lux scale fails with -EINVAL. > > I guess I am doing something in an unusual way here :) Do you have a > suggestion for me? Return data in lux? Or return it as INFO_RAW - thus making it clear that the reading is not in expected units and a conversion must be applied by userspace. SCALE is not applied to PROCESSED by userspace. In the rare case where you do get SCALE and PROCESSED it's there to allow for changes in the underlying signal measurement that are eaten up in the computation needed to get to PROCESSED - that is they have no visible affect (beyond range changes etc). ... > >> + > >> + ret = regmap_read(regmap, BU27034_REG_SYSTEM_CONTROL, &part_id); > > > > As it's not all of the register I'd rename the temporary variable to > > val or reg or something along those lines. > > I still like having the variable named part_id - as it makes the check > obvious. What I did was adding another temporary variable 'reg' and doing: > > part_id = FIELD_GET(BU27034_MASK_PART_ID, reg); > > and then using the part_id in if() and dev_warn(). Looks good. Jonathan
On 3/6/23 13:21, Andy Shevchenko wrote: > On Sun, Mar 05, 2023 at 03:10:38PM +0200, Matti Vaittinen wrote: >> On 3/4/23 22:17, Jonathan Cameron wrote: >>> On Thu, 2 Mar 2023 12:58:59 +0200 >>> Matti Vaittinen <mazziesaccount@gmail.com> wrote: >>> >>> As per other branch of the thread. >>> >>> ch0 = max(1, le16_to_cpu(res[0]); >>> > would be cleaner. >> >> I tried this out. Comparing u16 to literal 1 results comparison of values >> with different sizes: >> >> ./include/linux/minmax.h:20:28: warning: comparison of distinct pointer >> types lacks a cast >> (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1))) >> ^ >> ./include/linux/minmax.h:26:4: note: in expansion of macro ‘__typecheck’ >> (__typecheck(x, y) && __no_side_effects(x, y)) >> ^~~~~~~~~~~ >> ./include/linux/minmax.h:36:24: note: in expansion of macro ‘__safe_cmp’ >> __builtin_choose_expr(__safe_cmp(x, y), \ >> ^~~~~~~~~~ >> ./include/linux/minmax.h:74:19: note: in expansion of macro ‘__careful_cmp’ >> #define max(x, y) __careful_cmp(x, y, >) >> ^~~~~~~~~~~~~ >> drivers/iio/light/rohm-bu27034.c:1057:8: note: in expansion of macro ‘max’ >> ch0 = max(1, ch0); >> >> >> I could work around this by doing: >> >> const u16 min_ch_val = 1; >> >> ... >> >> ch0 = max(min_ch_val, le16_to_cpu(res[0])); >> >> but I think that would really be obfuscating the meaning. I assume >> >> ch0 = max((u16)1, le16_to_cpu(res[0])); >> >> might work too - but to me it's pretty ugly. > > That's why we have max_t() and clamp_val(). > And you know that. Thanks for pointing out the max_t(). I really missed that. I didn't think of the clamp_val() either. clamp_val() would "explain" better what we do here - but as we don't have the upper limit using that would (IMO) be slightly more confusing. >> The more I am looking at this, the stronger I feel we should really just >> write this as it was. Check if res[0] contains the only unsafe data >> "!res[0]" - and if yes, set it to 1. The comment above it will clarify it to >> a reader wondering what happens. >> >> I will leave it like it was in v2 for v3. If you still feel strong about it >> then we need to continue rubbing it. > > You need to convert bit ordering first, then check for 0. It would at least > make more sense. (Today is 0 you are comparing with, tomorrow it might be > 0xfffe, which is different to 0x7fff). I don't think being prepared for any other check but the check for 0 is meaningful here. Every other value except 0 is valid for this sensor and can be accepted as such. Preparing for something that may never realize (new sensor with less than 16bits of data) is likely to just make things more complicated. Well, as both You and Jonathan do think we should not just do if(!res[0]) - I'll try using the max_t() as you suggested. I still really dislike it because it makes it harder (for me) to capture what happens here. Well, I try to survive with this and hopefully just read the comment above if puzzled when I need to get back to this code. Yours, -- Matti
On 3/12/23 17:36, Jonathan Cameron wrote: > On Sun, 5 Mar 2023 14:22:51 +0200 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> On 3/4/23 22:17, Jonathan Cameron wrote: >>> On Thu, 2 Mar 2023 12:58:59 +0200 >>> Matti Vaittinen <mazziesaccount@gmail.com> wrote: >>> // snip >>>> +static const struct iio_chan_spec bu27034_channels[] = { >>>> + { >>>> + .type = IIO_LIGHT, >>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | >>>> + BIT(IIO_CHAN_INFO_SCALE), >>> >>> What is this scale for? >> >> The scale is to inform users that we return data using milli lux. >> >>> Given the channel is computed from various different inputs, is there a >>> clear definition of how it is scaled? What does a write to it mean? >> >> Nothing. writing anything else but milli lux scale fails with -EINVAL. >> >> I guess I am doing something in an unusual way here :) Do you have a >> suggestion for me? > > Return data in lux? That's what I did originally have. But then I noticed we can get slightly better accuracy than that. Hence I switched to mLux and added the scale. > Or return it as INFO_RAW - thus making it clear > that the reading is not in expected units and a conversion must be > applied by userspace. SCALE is not applied to PROCESSED by userspace. Ah. This makes sense then. Maybe it would be worth adding a warning to IIO-core if drivers set both the SCALE and PROCESSED info bits? So, I need to select between the simplicity or better accuracy here? :/ I really hate ending up making choices like this without knowing all the real use-cases :( And it happens a lot for me. Well, I guess I'll drop the scale, use luxes and go with the PROCESSED data. My understanding is that the "thing" with the sensor is a wide-range for wavelengths, not the accuracy. So, maybe luxes are just good enough - and again, users needing something more accurate can utilize the raw intensity channels. > > In the rare case where you do get SCALE and PROCESSED it's there to allow > for changes in the underlying signal measurement that are eaten up in the > computation needed to get to PROCESSED - that is they have no visible > affect (beyond range changes etc). Oh, Ok. So there is a valid case for setting both SCALE and PROCESSED. Then we can't add teh warning I assume :( Yours, -- Matti
On 3/12/23 17:36, Jonathan Cameron wrote: > On Sun, 5 Mar 2023 14:22:51 +0200 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> On 3/4/23 22:17, Jonathan Cameron wrote: >>> On Thu, 2 Mar 2023 12:58:59 +0200 >>> Matti Vaittinen <mazziesaccount@gmail.com> wrote: //snip >>> >>>> +static const struct iio_chan_spec bu27034_channels[] = { >>>> + { >>>> + .type = IIO_LIGHT, >>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | >>>> + BIT(IIO_CHAN_INFO_SCALE), >>> >>> What is this scale for? >> >> The scale is to inform users that we return data using milli lux. >> >>> Given the channel is computed from various different inputs, is there a >>> clear definition of how it is scaled? What does a write to it mean? >> >> Nothing. writing anything else but milli lux scale fails with -EINVAL. >> >> I guess I am doing something in an unusual way here :) Do you have a >> suggestion for me? > > Return data in lux? Or return it as INFO_RAW - thus making it clear > that the reading is not in expected units and a conversion must be > applied by userspace. SCALE is not applied to PROCESSED by userspace. > I just noticed a thing. I used the iio_generic_buffer to test the changes - and it got the channel values scaled to luxes even for the PROCESSED channel. So, it seems to me the iio_generic_buffer does apply the scale for PROCESSED channels too. I think that is slightly misleading. Oh, and this is not intended to be a complaint - just a report that there might be some room for an improvement :) Yours, -- Matti
On Mon, 13 Mar 2023 11:39:06 +0200 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 3/12/23 17:36, Jonathan Cameron wrote: > > On Sun, 5 Mar 2023 14:22:51 +0200 > > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > > > >> On 3/4/23 22:17, Jonathan Cameron wrote: > >>> On Thu, 2 Mar 2023 12:58:59 +0200 > >>> Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >>> > > // snip > > >>>> +static const struct iio_chan_spec bu27034_channels[] = { > >>>> + { > >>>> + .type = IIO_LIGHT, > >>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | > >>>> + BIT(IIO_CHAN_INFO_SCALE), > >>> > >>> What is this scale for? > >> > >> The scale is to inform users that we return data using milli lux. > >> > >>> Given the channel is computed from various different inputs, is there a > >>> clear definition of how it is scaled? What does a write to it mean? > >> > >> Nothing. writing anything else but milli lux scale fails with -EINVAL. > >> > >> I guess I am doing something in an unusual way here :) Do you have a > >> suggestion for me? > > > > Return data in lux? > > That's what I did originally have. But then I noticed we can get > slightly better accuracy than that. Hence I switched to mLux and added > the scale. > > > Or return it as INFO_RAW - thus making it clear > > that the reading is not in expected units and a conversion must be > > applied by userspace. SCALE is not applied to PROCESSED by userspace. > > Ah. This makes sense then. Maybe it would be worth adding a warning to > IIO-core if drivers set both the SCALE and PROCESSED info bits? Hmm. I'm not sure that we don't have valid users of it even if they are unusual. We also have some historical messes that do RAW + SCALE + PROCESSED so we can't really have a warning on it. Warning generally is that the test tools that come with the kernel will give you the wrong reading. :) > > So, I need to select between the simplicity or better accuracy here? :/ > I really hate ending up making choices like this without knowing all the > real use-cases :( And it happens a lot for me. Well, I guess I'll drop > the scale, use luxes and go with the PROCESSED data. My understanding is > that the "thing" with the sensor is a wide-range for wavelengths, not > the accuracy. So, maybe luxes are just good enough - and again, users > needing something more accurate can utilize the raw intensity channels. Hmm. For the sysfs case you could use VAL_INT_PLUS_MICRO but that doesn't then work well with the buffered path. It is perfectly valid to just have this as _RAW and keep your _SCALE so that's probably the best option _RAW doesn't have to mean totally raw, it just means userspace is expected to applying a linear conversion to get a reading in the 'base' units for the channel. > > > > > In the rare case where you do get SCALE and PROCESSED it's there to allow > > for changes in the underlying signal measurement that are eaten up in the > > computation needed to get to PROCESSED - that is they have no visible > > affect (beyond range changes etc). > > Oh, Ok. So there is a valid case for setting both SCALE and PROCESSED. > Then we can't add teh warning I assume :( They are obscure but IIRC there are some. Jonathan > > > Yours, > -- Matti >
On Tue, 14 Mar 2023 11:39:22 +0200 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 3/12/23 17:36, Jonathan Cameron wrote: > > On Sun, 5 Mar 2023 14:22:51 +0200 > > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > > > >> On 3/4/23 22:17, Jonathan Cameron wrote: > >>> On Thu, 2 Mar 2023 12:58:59 +0200 > >>> Matti Vaittinen <mazziesaccount@gmail.com> wrote: > > //snip > > >>> > >>>> +static const struct iio_chan_spec bu27034_channels[] = { > >>>> + { > >>>> + .type = IIO_LIGHT, > >>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | > >>>> + BIT(IIO_CHAN_INFO_SCALE), > >>> > >>> What is this scale for? > >> > >> The scale is to inform users that we return data using milli lux. > >> > >>> Given the channel is computed from various different inputs, is there a > >>> clear definition of how it is scaled? What does a write to it mean? > >> > >> Nothing. writing anything else but milli lux scale fails with -EINVAL. > >> > >> I guess I am doing something in an unusual way here :) Do you have a > >> suggestion for me? > > > > Return data in lux? Or return it as INFO_RAW - thus making it clear > > that the reading is not in expected units and a conversion must be > > applied by userspace. SCALE is not applied to PROCESSED by userspace. > > > > I just noticed a thing. I used the iio_generic_buffer to test the > changes - and it got the channel values scaled to luxes even for the > PROCESSED channel. So, it seems to me the iio_generic_buffer does apply > the scale for PROCESSED channels too. I think that is slightly > misleading. Oh, and this is not intended to be a complaint - just a > report that there might be some room for an improvement :) Ah. Looks like the code doesn't have any check on whether the sysfs read is _raw or _processed which is kind of understandable. This may be the first case where those have both applied on a channel that is available via buffered route. Given processed channels are rarely IIO_VAL_INT which is kind of necessary to poke it in the buffered route, that may well be true. Jonathan > > Yours, > -- Matti >
On 3/18/23 18:54, Jonathan Cameron wrote: > On Mon, 13 Mar 2023 11:39:06 +0200 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> On 3/12/23 17:36, Jonathan Cameron wrote: >>> On Sun, 5 Mar 2023 14:22:51 +0200 >>> Matti Vaittinen <mazziesaccount@gmail.com> wrote: >>> >>>> On 3/4/23 22:17, Jonathan Cameron wrote: >>>>> On Thu, 2 Mar 2023 12:58:59 +0200 >>>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote: >>>>> >> >> // snip >> >>>>>> +static const struct iio_chan_spec bu27034_channels[] = { >>>>>> + { >>>>>> + .type = IIO_LIGHT, >>>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | >>>>>> + BIT(IIO_CHAN_INFO_SCALE), >>>>> >>>>> What is this scale for? >>>> >>>> The scale is to inform users that we return data using milli lux. >>>> >>>>> Given the channel is computed from various different inputs, is there a >>>>> clear definition of how it is scaled? What does a write to it mean? >>>> >>>> Nothing. writing anything else but milli lux scale fails with -EINVAL. >>>> >>>> I guess I am doing something in an unusual way here :) Do you have a >>>> suggestion for me? >>> >>> Return data in lux? >> >> That's what I did originally have. But then I noticed we can get >> slightly better accuracy than that. Hence I switched to mLux and added >> the scale. >> >>> Or return it as INFO_RAW - thus making it clear >>> that the reading is not in expected units and a conversion must be >>> applied by userspace. SCALE is not applied to PROCESSED by userspace. >> >> Ah. This makes sense then. Maybe it would be worth adding a warning to >> IIO-core if drivers set both the SCALE and PROCESSED info bits? > > Hmm. I'm not sure that we don't have valid users of it even if they > are unusual. We also have some historical messes that do RAW + SCALE + > PROCESSED so we can't really have a warning on it. > > Warning generally is that the test tools that come with the kernel > will give you the wrong reading. :) > >> >> So, I need to select between the simplicity or better accuracy here? :/ >> I really hate ending up making choices like this without knowing all the >> real use-cases :( And it happens a lot for me. Well, I guess I'll drop >> the scale, use luxes and go with the PROCESSED data. My understanding is >> that the "thing" with the sensor is a wide-range for wavelengths, not >> the accuracy. So, maybe luxes are just good enough - and again, users >> needing something more accurate can utilize the raw intensity channels. > > Hmm. For the sysfs case you could use VAL_INT_PLUS_MICRO but that doesn't > then work well with the buffered path. > > It is perfectly valid to just have this as _RAW and keep your _SCALE so > that's probably the best option > _RAW doesn't have to mean totally raw, it just means userspace is expected > to applying a linear conversion to get a reading in the 'base' units for the channel. Thanks for the insight! I'll return the scale and switch to RAW for v5 :) Yours, -- Matti
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig index 671d84f98c56..14e03732f57d 100644 --- a/drivers/iio/light/Kconfig +++ b/drivers/iio/light/Kconfig @@ -292,6 +292,18 @@ config JSA1212 To compile this driver as a module, choose M here: the module will be called jsa1212. +config ROHM_BU27034 + tristate "ROHM BU27034 ambient light sensor" + depends on I2C + select REGMAP_I2C + select IIO_GTS_HELPER + help + Enable support for the ROHM BU27034 ambient light sensor. ROHM BU27034 + is an ambient light sesnor with 3 channels and 3 photo diodes capable + of detecting a very wide range of illuminance. + Typical application is adjusting LCD and backlight power of TVs and + mobile phones. + config RPR0521 tristate "ROHM RPR0521 ALS and proximity sensor driver" depends on I2C diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile index 82d14ebd3c11..e81c5ce60ccd 100644 --- a/drivers/iio/light/Makefile +++ b/drivers/iio/light/Makefile @@ -39,6 +39,7 @@ obj-$(CONFIG_MAX44009) += max44009.o obj-$(CONFIG_NOA1305) += noa1305.o obj-$(CONFIG_OPT3001) += opt3001.o obj-$(CONFIG_PA12203001) += pa12203001.o +obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o obj-$(CONFIG_RPR0521) += rpr0521.o obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o obj-$(CONFIG_SI1133) += si1133.o diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c new file mode 100644 index 000000000000..b8ca29af22e7 --- /dev/null +++ b/drivers/iio/light/rohm-bu27034.c @@ -0,0 +1,1491 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * BU27034 ROHM Ambient Light Sensor + * + * Copyright (c) 2023, ROHM Semiconductor. + * https://fscdn.rohm.com/en/products/databook/datasheet/ic/sensor/light/bu27034nuc-e.pdf + */ + +#include <linux/bits.h> +#include <linux/device.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/property.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/units.h> + +#include <linux/iio/iio.h> +#include <linux/iio/buffer.h> +#include <linux/iio/kfifo_buf.h> + +#include "iio-gts-helper.h" + +#define BU27034_REG_SYSTEM_CONTROL 0x40 +#define BU27034_MASK_SW_RESET BIT(7) +#define BU27034_MASK_PART_ID GENMASK(5, 0) +#define BU27034_ID 0x19 +#define BU27034_REG_MODE_CONTROL1 0x41 +#define BU27034_MASK_MEAS_MODE GENMASK(2, 0) + +#define BU27034_REG_MODE_CONTROL2 0x42 +#define BU27034_MASK_D01_GAIN GENMASK(7, 3) +#define BU27034_SHIFT_D01_GAIN 3 +#define BU27034_MASK_D2_GAIN_HI GENMASK(7, 6) +#define BU27034_MASK_D2_GAIN_LO GENMASK(2, 0) +#define BU27034_SHIFT_D2_GAIN 3 + +#define BU27034_REG_MODE_CONTROL3 0x43 +#define BU27034_REG_MODE_CONTROL4 0x44 +#define BU27034_MASK_MEAS_EN BIT(0) +#define BU27034_MASK_VALID BIT(7) +#define BU27034_REG_DATA0_LO 0x50 +#define BU27034_REG_DATA1_LO 0x52 +#define BU27034_REG_DATA2_LO 0x54 +#define BU27034_REG_DATA2_HI 0x55 +#define BU27034_REG_MANUFACTURER_ID 0x92 +#define BU27034_REG_MAX BU27034_REG_MANUFACTURER_ID + +/* + * The BU27034 does not have interrupt or any other mechanism of triggering + * the data read when measurement has finished. Hence we poll the VALID bit in + * a thread. We will try to wake the thread BU27034_MEAS_WAIT_PREMATURE_MS + * milliseconds before the expected sampling time to prevent the drifting. Eg, + * If we constantly wake up a bit too late we would eventually skip a sample. + * And because the sleep can't wake up _exactly_ at given time this would be + * inevitable even if the sensor clock would be perfectly phase-locked to CPU + * clock - which we can't say is the case. + * + * This is still fragile. No matter how big advance do we have, we will still + * risk of losing a sample because things can in a rainy-day skenario be + * delayed a lot. Yet, more we reserve the time for polling, more we also lose + * the performance by spending cycles polling the register. So, selecting this + * value is a balancing dance between severity of wasting CPU time and severity + * of losing samples. + * + * In most cases losing the samples is not _that_ crucial because light levels + * tend to change slowly. + */ +#define BU27034_MEAS_WAIT_PREMATURE_MS 5 +#define BU27034_DATA_WAIT_TIME_US 1000 +#define BU27034_TOTAL_DATA_WAIT_TIME_US (BU27034_MEAS_WAIT_PREMATURE_MS * 1000) + + +enum { + BU27034_CHAN_ALS, + BU27034_CHAN_DATA0, + BU27034_CHAN_DATA1, + BU27034_CHAN_DATA2, + BU27034_NUM_CHANS +}; + +static const unsigned long bu27034_scan_masks[] = { + BIT(BU27034_CHAN_ALS) | BIT(BU27034_CHAN_DATA0) | + BIT(BU27034_CHAN_DATA1) | BIT(BU27034_CHAN_DATA2), 0 +}; + +/* + * Available scales with gain 1x - 4096x, timings 55, 100, 200, 400 mS + * Time impacts to gain: 1x, 2x, 4x, 8x. + * + * => Max total gain is HWGAIN * gain by integration time (8 * 4096) = 32768 + * + * Using NANO precision for scale we must use scale 64x corresponding gain 1x + * to avoid precision loss. (32x would result scale 976 562.5(nanos). + */ +#define BU27034_SCALE_1X 64 + +#define BU27034_GSEL_1X 0x00 +#define BU27034_GSEL_4X 0x08 +#define BU27034_GSEL_16X 0x0a +#define BU27034_GSEL_32X 0x0b +#define BU27034_GSEL_64X 0x0c +#define BU27034_GSEL_256X 0x18 +#define BU27034_GSEL_512X 0x19 +#define BU27034_GSEL_1024X 0x1a +#define BU27034_GSEL_2048X 0x1b +#define BU27034_GSEL_4096X 0x1c + +/* Available gain settings */ +static const struct iio_gain_sel_pair bu27034_gains[] = { + GAIN_SCALE_GAIN(1, BU27034_GSEL_1X), + GAIN_SCALE_GAIN(4, BU27034_GSEL_4X), + GAIN_SCALE_GAIN(16, BU27034_GSEL_16X), + GAIN_SCALE_GAIN(32, BU27034_GSEL_32X), + GAIN_SCALE_GAIN(64, BU27034_GSEL_64X), + GAIN_SCALE_GAIN(256, BU27034_GSEL_256X), + GAIN_SCALE_GAIN(512, BU27034_GSEL_512X), + GAIN_SCALE_GAIN(1024, BU27034_GSEL_1024X), + GAIN_SCALE_GAIN(2048, BU27034_GSEL_2048X), + GAIN_SCALE_GAIN(4096, BU27034_GSEL_4096X), +}; + +/* + * The IC has 5 modes for sampling time. 5 mS mode is exceptional as it limits + * the data collection to data0-channel only and cuts the supported range to + * 10 bit. It is not supported by the driver. + * + * "normal" modes are 55, 100, 200 and 400 mS modes - which do have direct + * multiplying impact to the register values (similar to gain). + * + * This means that if meas-mode is changed for example from 400 => 200, + * the scale is doubled. Eg, time impact to total gain is x1, x2, x4, x8. + */ +#define BU27034_MEAS_MODE_100MS 0 +#define BU27034_MEAS_MODE_55MS 1 +#define BU27034_MEAS_MODE_200MS 2 +#define BU27034_MEAS_MODE_400MS 4 + +static const struct iio_itime_sel_mul bu27034_itimes[] = { + GAIN_SCALE_ITIME_US(400000, BU27034_MEAS_MODE_400MS, 8), + GAIN_SCALE_ITIME_US(200000, BU27034_MEAS_MODE_200MS, 4), + GAIN_SCALE_ITIME_US(100000, BU27034_MEAS_MODE_100MS, 2), + GAIN_SCALE_ITIME_US(50000, BU27034_MEAS_MODE_55MS, 1), +}; + +#define BU27034_CHAN_DATA(_name, _ch2) \ +{ \ + .type = IIO_INTENSITY, \ + .channel = BU27034_CHAN_##_name, \ + .channel2 = (_ch2), \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE), \ + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE), \ + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), \ + .info_mask_shared_by_all_available = \ + BIT(IIO_CHAN_INFO_INT_TIME), \ + .address = BU27034_REG_##_name##_LO, \ + .scan_index = BU27034_CHAN_##_name, \ + .scan_type = { \ + .sign = 'u', \ + .realbits = 16, \ + .storagebits = 16, \ + .endianness = IIO_LE, \ + }, \ + .indexed = 1 \ +} + +static const struct iio_chan_spec bu27034_channels[] = { + { + .type = IIO_LIGHT, + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | + BIT(IIO_CHAN_INFO_SCALE), + .channel = BU27034_CHAN_ALS, + .scan_index = BU27034_CHAN_ALS, + .scan_type = { + .sign = 'u', + .realbits = 32, + .storagebits = 32, + .endianness = IIO_CPU, + }, + }, + /* + * The BU27034 DATA0 and DATA1 channels are both on the visible light + * area (mostly). The data0 sensitivity peaks at 500nm, DATA1 at 600nm. + * These wave lengths are pretty much on the border of colours making + * these a poor candidates for R/G/B standardization. Hence they're both + * marked as clear channels + */ + BU27034_CHAN_DATA(DATA0, IIO_MOD_LIGHT_CLEAR), + BU27034_CHAN_DATA(DATA1, IIO_MOD_LIGHT_CLEAR), + BU27034_CHAN_DATA(DATA2, IIO_MOD_LIGHT_IR), + IIO_CHAN_SOFT_TIMESTAMP(4), +}; + +struct bu27034_data { + struct regmap *regmap; + struct device *dev; + /* + * Protect gain and time during scale adjustment and data reading. + * Protect measurement enabling/disabling. + */ + struct mutex mutex; + struct iio_gts gts; + struct task_struct *task; + __le16 raw[3]; + struct { + u32 lux; + __le16 channels[3]; + s64 ts __aligned(8); + } scan; +}; + +struct bu27034_result { + u16 ch0; + u16 ch1; + u16 ch2; +}; + +static const struct regmap_range bu27034_volatile_ranges[] = { + { + .range_min = BU27034_REG_MODE_CONTROL4, + .range_max = BU27034_REG_MODE_CONTROL4, + }, { + .range_min = BU27034_REG_DATA0_LO, + .range_max = BU27034_REG_DATA2_HI, + }, +}; + +static const struct regmap_access_table bu27034_volatile_regs = { + .yes_ranges = &bu27034_volatile_ranges[0], + .n_yes_ranges = ARRAY_SIZE(bu27034_volatile_ranges), +}; + +static const struct regmap_range bu27034_read_only_ranges[] = { + { + .range_min = BU27034_REG_DATA0_LO, + .range_max = BU27034_REG_DATA2_HI, + }, { + .range_min = BU27034_REG_MANUFACTURER_ID, + .range_max = BU27034_REG_MANUFACTURER_ID, + } +}; + +static const struct regmap_access_table bu27034_ro_regs = { + .no_ranges = &bu27034_read_only_ranges[0], + .n_no_ranges = ARRAY_SIZE(bu27034_read_only_ranges), +}; + +static const struct regmap_config bu27034_regmap = { + .reg_bits = 8, + .val_bits = 8, + + .max_register = BU27034_REG_MAX, + .cache_type = REGCACHE_RBTREE, + .volatile_table = &bu27034_volatile_regs, + .wr_table = &bu27034_ro_regs, +}; + +static int bu27034_validate_int_time(struct bu27034_data *data, int time_us) +{ + /* + * The BU27034 has 55 mS integration time which is in the vendor tests + * handled as 50 mS in all of the internal computations. We keep same + * approach here. + */ + if (time_us == 55000) + return 50000; + + if (iio_gts_valid_time(&data->gts, time_us)) + return time_us; + + return -EINVAL; +} + +struct bu27034_gain_check { + int old_gain; + int new_gain; + int chan; +}; + +static int bu27034_get_gain_sel(struct bu27034_data *data, int chan) +{ + int ret, val; + + switch (chan) { + case BU27034_CHAN_DATA0: + case BU27034_CHAN_DATA1: + { + int reg[] = { + [BU27034_CHAN_DATA0] = BU27034_REG_MODE_CONTROL2, + [BU27034_CHAN_DATA1] = BU27034_REG_MODE_CONTROL3, + }; + ret = regmap_read(data->regmap, reg[chan], &val); + if (ret) + return ret; + + val &= BU27034_MASK_D01_GAIN; + return val >> BU27034_SHIFT_D01_GAIN; + } + case BU27034_CHAN_DATA2: + ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL2, &val); + if (ret) + return ret; + + return (val & BU27034_MASK_D2_GAIN_HI) >> BU27034_SHIFT_D2_GAIN + | (val & BU27034_MASK_D2_GAIN_LO); + } + + return -EINVAL; +} + +static int bu27034_get_gain(struct bu27034_data *data, int chan, int *gain) +{ + int ret, sel; + + ret = bu27034_get_gain_sel(data, chan); + if (ret < 0) + return ret; + + sel = ret; + + ret = iio_gts_find_gain_by_sel(&data->gts, sel); + if (ret < 0) { + dev_err(data->dev, "chan %u: unknown gain value 0x%x\n", chan, + sel); + + return ret; + } + + *gain = ret; + + return 0; +} + +static int bu27034_get_int_time(struct bu27034_data *data) +{ + int ret, sel; + + ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL1, &sel); + if (ret) + return ret; + + return iio_gts_find_int_time_by_sel(&data->gts, + sel & BU27034_MASK_MEAS_MODE); +} + +static int _bu27034_get_scale(struct bu27034_data *data, int channel, int *val, + int *val2) +{ + int gain, ret; + + ret = bu27034_get_gain(data, channel, &gain); + if (ret) + return ret; + + ret = bu27034_get_int_time(data); + if (ret < 0) + return ret; + + return iio_gts_get_scale(&data->gts, gain, ret, val, val2); +} + +static int bu27034_get_scale(struct bu27034_data *data, int channel, int *val, + int *val2) +{ + int ret; + + if (channel == BU27034_CHAN_ALS) { + *val = 0; + *val2 = 1000; + return IIO_VAL_INT_PLUS_MICRO; + } + + mutex_lock(&data->mutex); + ret = _bu27034_get_scale(data, channel, val, val2); + mutex_unlock(&data->mutex); + if (!ret) + return IIO_VAL_INT_PLUS_NANO; + + return ret; +} + +/* Caller should hold the lock to protect lux reading */ +static int bu27034_write_gain_sel(struct bu27034_data *data, int chan, int sel) +{ + static const int reg[] = { + [BU27034_CHAN_DATA0] = BU27034_REG_MODE_CONTROL2, + [BU27034_CHAN_DATA1] = BU27034_REG_MODE_CONTROL3, + [BU27034_CHAN_DATA2] = BU27034_REG_MODE_CONTROL2 + }; + int mask; + + if (chan < BU27034_CHAN_DATA0 || chan > BU27034_CHAN_DATA2) + return -EINVAL; + + if (chan == BU27034_CHAN_DATA0 || chan == BU27034_CHAN_DATA1) { + sel <<= BU27034_SHIFT_D01_GAIN; + mask = BU27034_MASK_D01_GAIN; + } else { + /* + * We don't allow setting high bits for DATA2 gain because + * that impacts to DATA0 as well. + */ + mask = BU27034_MASK_D2_GAIN_LO; + } + + return regmap_update_bits(data->regmap, reg[chan], mask, sel); +} + +static int bu27034_set_gain(struct bu27034_data *data, int chan, int gain) +{ + int ret; + + ret = iio_gts_find_sel_by_gain(&data->gts, gain); + if (ret < 0) + return ret; + + return bu27034_write_gain_sel(data, chan, ret); +} + +/* Caller should hold the lock to protect data->int_time */ +static int bu27034_set_int_time(struct bu27034_data *data, int time) +{ + int ret; + + ret = iio_gts_find_sel_by_int_time(&data->gts, time); + if (ret < 0) + return ret; + + return regmap_update_bits(data->regmap, BU27034_REG_MODE_CONTROL1, + BU27034_MASK_MEAS_MODE, ret); + + return time; +} + +/* + * We try to change the time in such way that the scale is maintained for + * given channels by adjusting gain so that it compensates the time change. + */ +static int bu27034_try_set_int_time(struct bu27034_data *data, int time_us) +{ + int ret, int_time_old, int_time_new, i; + struct bu27034_gain_check gains[3] = { + { .chan = BU27034_CHAN_DATA0, }, + { .chan = BU27034_CHAN_DATA1, }, + { .chan = BU27034_CHAN_DATA2 } + }; + int numg = ARRAY_SIZE(gains); + + mutex_lock(&data->mutex); + ret = bu27034_get_int_time(data); + if (ret < 0) + goto unlock_out; + + int_time_old = ret; + + ret = bu27034_validate_int_time(data, time_us); + if (ret < 0) { + dev_err(data->dev, "Unsupported integration time %u\n", + time_us); + + goto unlock_out; + } + + int_time_new = ret; + + if (int_time_new == int_time_old) { + ret = 0; + goto unlock_out; + } + + for (i = 0; i < numg; i++) { + ret = bu27034_get_gain(data, gains[i].chan, + &gains[i].old_gain); + if (ret) + goto unlock_out; + + gains[i].new_gain = gains[i].old_gain * int_time_old / + int_time_new; + + if (!iio_gts_valid_gain(&data->gts, gains[i].new_gain)) { + int scale1, scale2; + bool ok; + + _bu27034_get_scale(data, gains[i].chan, &scale1, &scale2); + dev_dbg(data->dev, + "chan %u, can't support time %u with scale %u %u\n", + gains[i].chan, time_us, scale1, scale2); + + /* + * As Jonathan put it, if caller requests for + * integration time change and we can't support the + * scale - then the caller should be prepared to + * 'pick up the pieces and deal with the fact that the + * scale changed'. + */ + ret = iio_find_closest_gain_low(&data->gts, + gains[i].new_gain, &ok); + + if (!ok) { + dev_dbg(data->dev, + "optimal gain out of range for chan %u\n", + gains[i].chan); + } + if (ret < 0) { + dev_warn(data->dev, + "Total gain increase. Risk of saturation"); + ret = iio_gts_get_min_gain(&data->gts); + if (ret < 0) + goto unlock_out; + } + dev_warn(data->dev, "chan %u scale changed\n", + gains[i].chan); + gains[i].new_gain = ret; + dev_dbg(data->dev, "chan %u new gain %u\n", + gains[i].chan, gains[i].new_gain); + } + } + + /* + * The new integration time can be supported while keeping the scale of + * channels intact by tuning the gains. + */ + for (i = 0; i < numg; i++) { + ret = bu27034_set_gain(data, gains[i].chan, gains[i].new_gain); + if (ret) + goto unlock_out; + } + + ret = bu27034_set_int_time(data, int_time_new); + +unlock_out: + mutex_unlock(&data->mutex); + + return ret; +} + +static int bu27034_set_scale(struct bu27034_data *data, int chan, + int val, int val2) +{ + int ret, time_sel, gain_sel, i; + bool found = false; + + if (chan == BU27034_CHAN_ALS) { + if (val == 0 && val2 == 1000) + return 0; + else + return -EINVAL; + } + + mutex_lock(&data->mutex); + ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL1, &time_sel); + if (ret) + goto unlock_out; + + ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts, time_sel, + val, val2 * 1000, &gain_sel); + if (ret) { + /* + * Could'n support scale with given time. Need to change time. + * We still want to maintain the scale for all channels + */ + int new_time_sel; + struct bu27034_gain_check gains[2]; + + /* + * Populate information for the two other channels which should + * maintain the scale. + */ + if (chan == BU27034_CHAN_DATA0) { + gains[0].chan = BU27034_CHAN_DATA1; + gains[1].chan = BU27034_CHAN_DATA2; + } else if (chan == BU27034_CHAN_DATA1) { + gains[0].chan = BU27034_CHAN_DATA0; + gains[1].chan = BU27034_CHAN_DATA2; + } else { + gains[0].chan = BU27034_CHAN_DATA0; + gains[1].chan = BU27034_CHAN_DATA1; + } + + for (i = 0; i < 2; i++) { + ret = bu27034_get_gain(data, gains[i].chan, + &gains[i].old_gain); + if (ret) + goto unlock_out; + } + + /* + * Iterate through all the times to see if we find one which + * can support requested scale for requested channel, while + * maintaining the scale for other channels + */ + for (i = 0; i < data->gts.num_itime; i++) { + new_time_sel = data->gts.itime_table[i].sel; + + if (new_time_sel == time_sel) + continue; + + /* Can we provide requested scale with this time? */ + ret = iio_gts_find_gain_sel_for_scale_using_time( + &data->gts, new_time_sel, val, val2 * 1000, + &gain_sel); + if (ret) + continue; + + /* Can the two other channels maintain scales? */ + ret = iio_gts_find_new_gain_sel_by_old_gain_time( + &data->gts, gains[0].old_gain, time_sel, + new_time_sel, &gains[0].new_gain); + if (ret) + continue; + + ret = iio_gts_find_new_gain_sel_by_old_gain_time( + &data->gts, gains[1].old_gain, time_sel, + new_time_sel, &gains[1].new_gain); + if (!ret) { + /* Yes - we found suitable time */ + found = true; + break; + } + } + if (!found) { + dev_err(data->dev, + "Can't set scale maintaining other channels\n"); + ret = -EINVAL; + + goto unlock_out; + } + + for (i = 0; i < 2; i++) { + ret = bu27034_set_gain(data, gains[i].chan, + gains[i].new_gain); + if (ret) + goto unlock_out; + } + + ret = regmap_update_bits(data->regmap, BU27034_REG_MODE_CONTROL1, + BU27034_MASK_MEAS_MODE, new_time_sel); + if (ret) + goto unlock_out; + } + + ret = bu27034_write_gain_sel(data, chan, gain_sel); +unlock_out: + mutex_unlock(&data->mutex); + + return ret; +} + +/* + * for (D1/D0 < 0.87): + * lx = 0.004521097 * D1 - 0.002663996 * D0 + + * 0.00012213 * D1 * D1 / D0 + * + * => 115.7400832 * ch1 / gain1 / mt - + * 68.1982976 * ch0 / gain0 / mt + + * 0.00012213 * 25600 * (ch1 / gain1 / mt) * 25600 * + * (ch1 /gain1 / mt) / (25600 * ch0 / gain0 / mt) + * + * A = 0.00012213 * 25600 * (ch1 /gain1 / mt) * 25600 * + * (ch1 /gain1 / mt) / (25600 * ch0 / gain0 / mt) + * => 0.00012213 * 25600 * (ch1 /gain1 / mt) * + * (ch1 /gain1 / mt) / (ch0 / gain0 / mt) + * => 0.00012213 * 25600 * (ch1 / gain1) * (ch1 /gain1 / mt) / + * (ch0 / gain0) + * => 0.00012213 * 25600 * (ch1 / gain1) * (ch1 /gain1 / mt) * + * gain0 / ch0 + * => 3.126528 * ch1 * ch1 * gain0 / gain1 / gain1 / mt /ch0 + * + * lx = (115.7400832 * ch1 / gain1 - 68.1982976 * ch0 / gain0) / + * mt + A + * => (115.7400832 * ch1 / gain1 - 68.1982976 * ch0 / gain0) / + * mt + 3.126528 * ch1 * ch1 * gain0 / gain1 / gain1 / mt / + * ch0 + * + * => (115.7400832 * ch1 / gain1 - 68.1982976 * ch0 / gain0 + + * 3.126528 * ch1 * ch1 * gain0 / gain1 / gain1 / ch0) / + * mt + * + * For (0.87 <= D1/D0 < 1.00) + * lx = (0.001331* D0 + 0.0000354 * D1) * ((D1/D0 – 0.87) * (0.385) + 1) + * => (0.001331 * 256 * 100 * ch0 / gain0 / mt + 0.0000354 * 256 * + * 100 * ch1 / gain1 / mt) * ((D1/D0 - 0.87) * (0.385) + 1) + * => (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) * + * ((D1/D0 - 0.87) * (0.385) + 1) + * => (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) * + * (0.385 * D1/D0 - 0.66505) + * => (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) * + * (0.385 * 256 * 100 * ch1 / gain1 / mt / (256 * 100 * ch0 / gain0 / mt) - 0.66505) + * => (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) * + * (9856 * ch1 / gain1 / mt / (25600 * ch0 / gain0 / mt) + 0.66505) + * => 13.118336 * ch1 / (gain1 * mt) + * + 22.66064768 * ch0 / (gain0 * mt) + * + 8931.90144 * ch1 * ch1 * gain0 / + * (25600 * ch0 * gain1 * gain1 * mt) + * + 0.602694912 * ch1 / (gain1 * mt) + * + * => [0.3489024 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) + * + 22.66064768 * ch0 / gain0 + * + 13.721030912 * ch1 / gain1 + * ] / mt + * + * For (D1/D0 >= 1.00) + * + * lx = (0.001331* D0 + 0.0000354 * D1) * ((D1/D0 – 2.0) * (-0.05) + 1) + * => (0.001331* D0 + 0.0000354 * D1) * (-0.05D1/D0 + 1.1) + * => (0.001331 * 256 * 100 * ch0 / gain0 / mt + 0.0000354 * 256 * + * 100 * ch1 / gain1 / mt) * (-0.05D1/D0 + 1.1) + * => (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) * + * (-0.05 * 256 * 100 * ch1 / gain1 / mt / (256 * 100 * ch0 / gain0 / mt) + 1.1) + * => (34.0736 * ch0 / gain0 / mt + 0.90624 * ch1 / gain1 / mt) * + * (-1280 * ch1 / (gain1 * mt * 25600 * ch0 / gain0 / mt) + 1.1) + * => (34.0736 * ch0 * -1280 * ch1 * gain0 * mt /( gain0 * mt * gain1 * mt * 25600 * ch0) + * + 34.0736 * 1.1 * ch0 / (gain0 * mt) + * + 0.90624 * ch1 * -1280 * ch1 *gain0 * mt / (gain1 * mt *gain1 * mt * 25600 * ch0) + * + 1.1 * 0.90624 * ch1 / (gain1 * mt) + * => -43614.208 * ch1 / (gain1 * mt * 25600) + * + 37.48096 ch0 / (gain0 * mt) + * - 1159.9872 * ch1 * ch1 * gain0 / (gain1 * gain1 * mt * 25600 * ch0) + * + 0.996864 ch1 / (gain1 * mt) + * => [ + * - 0.045312 * ch1 * ch1 * gain0 / (gain1 * gain1 * ch0) + * - 0.706816 * ch1 / gain1 + * + 37.48096 ch0 /gain0 + * ] * mt + * + * + * So, the first case (D1/D0 < 0.87) can be computed to a form: + * + * lx = (3.126528 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) + + * 115.7400832 * ch1 / gain1 + + * -68.1982976 * ch0 / gain0 + * / mt + * + * Second case (0.87 <= D1/D0 < 1.00) goes to form: + * + * => [0.3489024 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) + + * 13.721030912 * ch1 / gain1 + + * 22.66064768 * ch0 / gain0 + * ] / mt + * + * Third case (D1/D0 >= 1.00) goes to form: + * => [-0.045312 * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) + + * -0.706816 * ch1 / gain1 + + * 37.48096 ch0 /(gain0 + * ] / mt + * + * This can be unified to format: + * lx = [ + * A * ch1 * ch1 * gain0 / (ch0 * gain1 * gain1) + + * B * ch1 / gain1 + + * C * ch0 / gain0 + * ] / mt + * + * For case 1: + * A = 3.126528, + * B = 115.7400832 + * C = -68.1982976 + * + * For case 2: + * A = 0.3489024 + * B = 13.721030912 + * C = 22.66064768 + * + * For case 3: + * A = -0.045312 + * B = -0.706816 + * C = 37.48096 + */ + +struct bu27034_lx_coeff { + unsigned int A; + unsigned int B; + unsigned int C; + /* Indicate which of the coefficients above are negative */ + bool is_neg[3]; +}; + +static u64 bu27034_fixp_calc_t1(unsigned int coeff, unsigned int ch0, + unsigned int ch1, unsigned int gain0, + unsigned int gain1) +{ + unsigned int helper, tmp; + u64 helper64; + + /* + * Here we could overflow even the 64bit value. Hence we + * multiply with gain0 only after the divisions - even though + * it may result loss of accuracy + */ + helper64 = (u64)coeff * (u64)ch1 * (u64)ch1; + helper = coeff * ch1 * ch1; + tmp = helper * gain0; + + if (helper == helper64 && (tmp / gain0 == helper)) + return tmp / (gain1 * gain1) / ch0; + + helper = gain1 * gain1; + if (helper > ch0) { + do_div(helper64, helper); + /* + * Max gain for a channel is 4096. The max u64 + * (0xffffffffffffffffULL) divided by 4096 is 0xFFFFFFFFFFFFF + * (floored). Thus, the 0xFFFFFFFFFFFFF is the largest value + * we can safely multiply with the gain, no matter what gain + * is set. + * + * So, multiplication with max gain may overflow + * if helper64 is greater than 0xFFFFFFFFFFFFF. + * + * If this is the case we divide first. + */ + if (helper64 < 0xFFFFFFFFFFFFFLLU) { + helper64 *= gain0; + do_div(helper64, ch0); + } else { + do_div(helper64, ch0); + helper64 *= gain0; + } + + return helper64; + } + + do_div(helper64, ch0); + /* Same overflow check here */ + if (helper64 < 0xFFFFFFFFFFFFFLLU) { + helper64 *= gain0; + do_div(helper64, helper); + } else { + do_div(helper64, helper); + helper64 *= gain0; + } + + return helper64; +} + +static u64 bu27034_fixp_calc_t23(unsigned int coeff, unsigned int ch, + unsigned int gain) +{ + unsigned int helper; + u64 helper64; + + helper64 = (u64)coeff * (u64)ch; + helper = coeff * ch; + + if (helper == helper64) + return helper / gain; + + do_div(helper64, gain); + + return helper64; +} + +static int bu27034_fixp_calc_lx(unsigned int ch0, unsigned int ch1, + unsigned int gain0, unsigned int gain1, + unsigned int meastime, int coeff_idx) +{ + static const struct bu27034_lx_coeff coeff[] = { + { + .A = 31265280, /* 3.126528 */ + .B = 1157400832, /*115.7400832 */ + .C = 681982976, /* -68.1982976 */ + .is_neg = {false, false, true}, + }, { + .A = 3489024, /* 0.3489024 */ + .B = 137210309, /* 13.721030912 */ + .C = 226606476, /* 22.66064768 */ + /* All terms positive */ + }, { + .A = 453120, /* -0.045312 */ + .B = 7068160, /* -0.706816 */ + .C = 374809600, /* 37.48096 */ + .is_neg = {true, true, false}, + } + }; + const struct bu27034_lx_coeff *c = &coeff[coeff_idx]; + u64 res = 0, terms[3]; + int i; + + if (coeff_idx >= ARRAY_SIZE(coeff)) + return -EINVAL; + + terms[0] = bu27034_fixp_calc_t1(c->A, ch0, ch1, gain0, gain1); + terms[1] = bu27034_fixp_calc_t23(c->B, ch1, gain1); + terms[2] = bu27034_fixp_calc_t23(c->C, ch0, gain0); + + /* First, add positive terms */ + for (i = 0; i < 3; i++) + if (!c->is_neg[i]) + res += terms[i]; + + /* No positive term => zero lux */ + if (!res) + return 0; + + /* Then, subtract negative terms (if any) */ + for (i = 0; i < 3; i++) + if (c->is_neg[i]) { + /* + * If the negative term is greater than positive - then + * the darknes has taken over and we are all doomed! Eh, + * I mean, then we can just return 0 lx and go out + */ + if (terms[i] >= res) + return 0; + + res -= terms[i]; + } + + meastime *= 10; + do_div(res, meastime); + + return (int) res; +} + +static bool bu27034_has_valid_sample(struct bu27034_data *data) +{ + int ret, val; + + ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL4, &val); + if (ret) + dev_err(data->dev, "Read failed %d\n", ret); + + return (val & BU27034_MASK_VALID); +} + +/* + * Reading the register where VALID bit is clears this bit. (So does changing + * any gain / integration time configuration registers) The bit gets + * set when we have acquired new data. We use this bit to indicate data + * validity. + */ +static void bu27034_invalidate_read_data(struct bu27034_data *data) +{ + bu27034_has_valid_sample(data); +} + +static int bu27034_read_result(struct bu27034_data *data, int chan, int *res) +{ + int reg[] = { + [BU27034_CHAN_DATA0] = BU27034_REG_DATA0_LO, + [BU27034_CHAN_DATA1] = BU27034_REG_DATA1_LO, + [BU27034_CHAN_DATA2] = BU27034_REG_DATA2_LO, + }; + int valid, ret; + __le16 val; + + ret = regmap_read_poll_timeout(data->regmap, BU27034_REG_MODE_CONTROL4, + valid, (valid & BU27034_MASK_VALID), + BU27034_DATA_WAIT_TIME_US, 0); + if (ret) + return ret; + + ret = regmap_bulk_read(data->regmap, reg[chan], &val, sizeof(val)); + if (ret) + return ret; + + *res = le16_to_cpu(val); + + return 0; +} + +static int bu27034_get_result_unlocked(struct bu27034_data *data, __le16 *res, + int size) +{ + int ret = 0; + +retry: + /* Get new value from sensor if data is ready */ + if (bu27034_has_valid_sample(data)) { + ret = regmap_bulk_read(data->regmap, BU27034_REG_DATA0_LO, + res, size); + if (ret) + return ret; + + bu27034_invalidate_read_data(data); + } else { + /* No new data in sensor. Wait and retry */ + msleep(25); + + goto retry; + } + + return ret; +} + +static int bu27034_meas_set(struct bu27034_data *data, bool en) +{ + if (en) + return regmap_set_bits(data->regmap, BU27034_REG_MODE_CONTROL4, + BU27034_MASK_MEAS_EN); + + return regmap_clear_bits(data->regmap, BU27034_REG_MODE_CONTROL4, + BU27034_MASK_MEAS_EN); +} + +static int bu27034_get_single_result(struct bu27034_data *data, int chan, + int *val) +{ + int ret; + + + ret = bu27034_meas_set(data, true); + if (ret) + return ret; + + ret = bu27034_get_int_time(data); + if (ret < 0) + return ret; + + msleep(ret / 1000); + + return bu27034_read_result(data, chan, val); +} + +/* + * The formula given by vendor for computing luxes out of data0 and data1 + * (in open air) is as follows: + * + * Let's mark: + * D0 = data0/ch0_gain/meas_time_ms * 25600 + * D1 = data1/ch1_gain/meas_time_ms * 25600 + * + * Then: + * if (D1/D0 < 0.87) + * lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 0.87) * 3.45 + 1) + * else if (D1/D0 < 1) + * lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 0.87) * 0.385 + 1) + * else + * lx = (0.001331 * D0 + 0.0000354 * D1) * ((D1 / D0 - 2) * -0.05 + 1) + * + * We use it here. Users who have for example some colored lens + * need to modify the calculation but I hope this gives a starting point for + * those working with such devices. + */ + +static int bu27034_calc_lux(struct bu27034_data *data, __le16 *res, int *val) +{ + unsigned int gain0, gain1, meastime; + unsigned int d1_d0_ratio_scaled; + u16 ch0, ch1; + u64 helper64; + int ret; + + /* + * We return 0 luxes if calculation fails. This should be reasonably + * easy to spot from the buffers especially if raw-data channels show + * valid values + */ + *val = 0; + + /* + * Avoid div by zeroi. Not using max() as the data may not be in + * native endianes + */ + if (!res[0]) + ch0 = 1; + else + ch0 = le16_to_cpu(res[0]); + + if (!res[1]) + ch1 = 1; + else + ch1 = le16_to_cpu(res[1]); + + + ret = bu27034_get_gain(data, BU27034_CHAN_DATA0, &gain0); + if (ret) + return ret; + + ret = bu27034_get_gain(data, BU27034_CHAN_DATA1, &gain1); + if (ret) + return ret; + + ret = bu27034_get_int_time(data); + if (ret < 0) + return ret; + + meastime = ret; + + d1_d0_ratio_scaled = (unsigned int)ch1 * (unsigned int)gain0 * 100; + helper64 = (u64)ch1 * (u64)gain0 * 100LLU; + + if (helper64 != d1_d0_ratio_scaled) { + unsigned int div = (unsigned int)ch0 * gain1; + + do_div(helper64, div); + d1_d0_ratio_scaled = helper64; + } else { + d1_d0_ratio_scaled /= ch0 * gain1; + } + + if (d1_d0_ratio_scaled < 87) + *val = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 0); + else if (d1_d0_ratio_scaled < 100) + *val = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 1); + else + *val = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 2); + + return 0; + +} + +static int bu27034_get_lux(struct bu27034_data *data, int *val) +{ + __le16 res[3]; + int ret; + + ret = bu27034_get_result_unlocked(data, &res[0], sizeof(res)); + if (ret) + return ret; + + return bu27034_calc_lux(data, res, val); +} + +static int bu27034_read_raw(struct iio_dev *idev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct bu27034_data *data = iio_priv(idev); + int ret; + + switch (mask) { + case IIO_CHAN_INFO_INT_TIME: + *val = bu27034_get_int_time(data); + if (*val < 0) + return *val; + + /* + * We use 50000 uS internally for all calculations and only + * convert it to 55000 before returning it to the user. + * + * This is because the data-sheet says the time is 55 mS - but + * vendor provided computations used 50 mS. + */ + if (*val == 50000) + *val = 55000; + + return IIO_VAL_INT; + + case IIO_CHAN_INFO_SCALE: + return bu27034_get_scale(data, chan->channel, val, val2); + + case IIO_CHAN_INFO_RAW: + { + if (chan->type != IIO_INTENSITY) + return -EINVAL; + + if (chan->channel < BU27034_CHAN_DATA0 || + chan->channel > BU27034_CHAN_DATA2) + return -EINVAL; + + /* Don't mess with measurement enabling while buffering */ + ret = iio_device_claim_direct_mode(idev); + if (ret) + return ret; + + mutex_lock(&data->mutex); + /* + * Reading one channel at a time is ineffiecient but we don't + * care here. Buffered version should be used if performance is + * an issue. + */ + ret = bu27034_get_single_result(data, chan->channel, val); + if (ret) + goto release_err_out; + + mutex_unlock(&data->mutex); + iio_device_release_direct_mode(idev); + + return IIO_VAL_INT; + } + + case IIO_CHAN_INFO_PROCESSED: + if (chan->type != IIO_LIGHT) + return -EINVAL; + + /* Don't mess with measurement enabling while buffering */ + ret = iio_device_claim_direct_mode(idev); + if (ret) + return ret; + + mutex_lock(&data->mutex); + ret = bu27034_meas_set(data, true); + if (ret) + goto release_err_out; + + ret = bu27034_get_lux(data, val); + if (ret) + goto release_err_out; + + ret = bu27034_meas_set(data, false); + if (ret) + dev_err(data->dev, "failed to disable measurement\n"); + + mutex_unlock(&data->mutex); + iio_device_release_direct_mode(idev); + + return IIO_VAL_INT; + + default: + return -EINVAL; + + } + +release_err_out: + mutex_unlock(&data->mutex); + iio_device_release_direct_mode(idev); + + return ret; +} + +static int bu27034_write_raw(struct iio_dev *idev, + struct iio_chan_spec const *chan, + int val, int val2, long mask) +{ + struct bu27034_data *data = iio_priv(idev); + int ret; + + ret = iio_device_claim_direct_mode(idev); + if (ret) + return ret; + + switch (mask) { + case IIO_CHAN_INFO_SCALE: + ret = bu27034_set_scale(data, chan->channel, val, val2); + break; + case IIO_CHAN_INFO_INT_TIME: + ret = bu27034_try_set_int_time(data, val); + break; + default: + ret = -EINVAL; + break; + } + + iio_device_release_direct_mode(idev); + + return ret; +} + +static int bu27034_read_avail(struct iio_dev *idev, + struct iio_chan_spec const *chan, const int **vals, + int *type, int *length, long mask) +{ + struct bu27034_data *data = iio_priv(idev); + + switch (mask) { + case IIO_CHAN_INFO_INT_TIME: + return iio_gts_avail_times(&data->gts, vals, type, length); + case IIO_CHAN_INFO_SCALE: + return iio_gts_all_avail_scales(&data->gts, vals, type, length); + default: + break; + } + + return -EINVAL; +} +static const struct iio_info bu27034_info = { + .read_raw = &bu27034_read_raw, + .write_raw = &bu27034_write_raw, + .read_avail = &bu27034_read_avail, +}; + +static int bu27034_chip_init(struct bu27034_data *data) +{ + int ret, sel; + + /* Reset */ + ret = regmap_update_bits(data->regmap, BU27034_REG_SYSTEM_CONTROL, + BU27034_MASK_SW_RESET, BU27034_MASK_SW_RESET); + if (ret) + return dev_err_probe(data->dev, ret, "Sensor reset failed\n"); + + /* + * Delay to allow IC to initialize. We don't care if we delay + * for more than 1 ms so msleep() is Ok. We just don't want to + * block + */ + msleep(1); + /* + * Read integration time here to ensure it is in regmap cache. We do + * this to speed-up the int-time acquisition in the start of the buffer + * handling thread where longer delays could make it more likely we end + * up skipping a sample, and where the longer delays make timestamps + * less accurate. + */ + ret = regmap_read(data->regmap, BU27034_REG_MODE_CONTROL1, &sel); + if (ret) + dev_err(data->dev, "reading integration time failed\n"); + + return 0; +} + +static int bu27034_wait_for_data(struct bu27034_data *data) +{ + int ret, val; + + ret = regmap_read_poll_timeout(data->regmap, BU27034_REG_MODE_CONTROL4, + val, (val & BU27034_MASK_VALID), + BU27034_DATA_WAIT_TIME_US, + BU27034_TOTAL_DATA_WAIT_TIME_US); + if (ret) { + dev_err(data->dev, "data polling %s\n", + !(val & BU27034_MASK_VALID) ? "timeout" : "fail"); + return ret; + } + ret = regmap_bulk_read(data->regmap, BU27034_REG_DATA0_LO, + &data->scan.channels[0], + sizeof(data->scan.channels)); + bu27034_invalidate_read_data(data); + + return ret; +} + +static int bu27034_buffer_thread(void *arg) +{ + struct iio_dev *idev = arg; + struct bu27034_data *data; + int wait_ms; + + data = iio_priv(idev); + + wait_ms = bu27034_get_int_time(data); + wait_ms /= 1000; + + wait_ms -= BU27034_MEAS_WAIT_PREMATURE_MS; + + while (!kthread_should_stop()) { + int ret; + int64_t tstamp; + + msleep(wait_ms); + ret = bu27034_wait_for_data(data); + if (ret) + continue; + + tstamp = iio_get_time_ns(idev); + + if (*idev->active_scan_mask & BIT(BU27034_CHAN_ALS)) { + ret = bu27034_calc_lux(data, &data->scan.channels[0], + &data->scan.lux); + if (ret) + dev_err(data->dev, "failed to calculate lux\n"); + } + iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp); + } + + return 0; +} + +static int bu27034_buffer_enable(struct iio_dev *idev) +{ + struct bu27034_data *data = iio_priv(idev); + struct task_struct *task; + int ret; + + mutex_lock(&data->mutex); + ret = bu27034_meas_set(data, true); + if (ret) + goto unlock_out; + + task = kthread_run(bu27034_buffer_thread, idev, + "bu27034-buffering-%u", + iio_device_id(idev)); + if (IS_ERR(task)) { + ret = PTR_ERR(task); + goto unlock_out; + } + + data->task = task; + +unlock_out: + mutex_unlock(&data->mutex); + + return ret; +} + +static int bu27034_buffer_disable(struct iio_dev *idev) +{ + struct bu27034_data *data = iio_priv(idev); + int ret; + + mutex_lock(&data->mutex); + if (data->task) { + kthread_stop(data->task); + data->task = NULL; + } + + ret = bu27034_meas_set(data, false); + mutex_unlock(&data->mutex); + + return ret; +} + +static const struct iio_buffer_setup_ops bu27034_buffer_ops = { + .postenable = &bu27034_buffer_enable, + .predisable = &bu27034_buffer_disable, +}; + +static int bu27034_probe(struct i2c_client *i2c) +{ + struct device *dev = &i2c->dev; + struct fwnode_handle *fwnode; + struct bu27034_data *data; + struct regmap *regmap; + struct iio_dev *idev; + unsigned int part_id; + int ret; + + regmap = devm_regmap_init_i2c(i2c, &bu27034_regmap); + if (IS_ERR(regmap)) + return dev_err_probe(dev, PTR_ERR(regmap), + "Failed to initialize Regmap\n"); + + fwnode = dev_fwnode(dev); + if (!fwnode) + return -ENODEV; + + idev = devm_iio_device_alloc(dev, sizeof(*data)); + if (!idev) + return -ENOMEM; + + ret = devm_regulator_get_enable(dev, "vdd"); + if (ret && ret != -ENODEV) + return dev_err_probe(dev, ret, "Failed to get regulator\n"); + + data = iio_priv(idev); + + ret = regmap_read(regmap, BU27034_REG_SYSTEM_CONTROL, &part_id); + if (ret) + return dev_err_probe(dev, ret, "Failed to access sensor\n"); + + part_id &= BU27034_MASK_PART_ID; + + if (part_id != BU27034_ID) + dev_warn(dev, "unsupported device 0x%x\n", part_id); + + ret = iio_init_iio_gts(BU27034_SCALE_1X, 0, bu27034_gains, + ARRAY_SIZE(bu27034_gains), bu27034_itimes, + ARRAY_SIZE(bu27034_itimes), &data->gts); + if (ret) + return ret; + + ret = devm_iio_gts_build_avail_tables(dev, &data->gts); + if (ret) + return ret; + + mutex_init(&data->mutex); + data->regmap = regmap; + data->dev = dev; + + idev->channels = bu27034_channels; + idev->num_channels = ARRAY_SIZE(bu27034_channels); + idev->name = "bu27034"; + idev->info = &bu27034_info; + + idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE; + idev->available_scan_masks = bu27034_scan_masks; + + ret = bu27034_chip_init(data); + if (ret) + return ret; + + ret = devm_iio_kfifo_buffer_setup(dev, idev, &bu27034_buffer_ops); + + ret = devm_iio_device_register(dev, idev); + if (ret < 0) + return dev_err_probe(dev, ret, + "Unable to register iio device\n"); + + return ret; +} + +static const struct of_device_id bu27034_of_match[] = { + { .compatible = "rohm,bu27034", }, + { } +}; +MODULE_DEVICE_TABLE(of, bu27034_of_match); + +static struct i2c_driver bu27034_i2c_driver = { + .driver = { + .name = "bu27034-als", + .of_match_table = bu27034_of_match, + }, + .probe_new = bu27034_probe, +}; +module_i2c_driver(bu27034_i2c_driver); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>"); +MODULE_DESCRIPTION("ROHM BU27034 ambient light sensor driver");
ROHM BU27034 is an ambient light sesnor with 3 channels and 3 photo diodes capable of detecting a very wide range of illuminance. Typical application is adjusting LCD and backlight power of TVs and mobile phones. Add initial support for the ROHM BU27034 ambient light sensor. NOTE: - Driver exposes 4 channels. One IIO_LIGHT channel providing the calculated lux values based on measured data from diodes #0 and #1. Additionally 3 IIO_INTENSITY channels are emitting the raw register data from all diodes for more intense user-space computations. - Sensor has adjustible GAIN values ranging from 1x to 4096x. - Sensor has adjustible measurement times 5, 55, 100, 200 and 400 mS. Driver does not support 5 mS which has special limitations. - Driver exposes standard 'scale' adjustment which is implemented by: 1) Trying to adjust only the GAIN 2) If GAIN adjustment only can't provide requested scale, adjusting both the time and the gain is attempted. - Driver exposes writable INT_TIME property which can be used for adjusting the measurement time. Time adjustment will also cause the driver to adjust the GAIN so that the overall scale is not changed. - Runtime PM is not implemented. - Driver starts the measurement on the background when it is probed. This improves the respnse time to read-requests compared to starting the read only when data is requested. When the most accurate 400 mS measurement time is used, data reads would last quite long if measurement was started only on demand. This, however, is not appealing for users who would prefere power saving over measurement response time. Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> --- Changes since RFCv1 - (really) protect read-only registers - fix get and set gain - buffered mode - Protect the whole sequences including meas_en/meas_dis to avoid messing up the enable / disable order - typofixes / doc improvements - change dropped GAIN_SCALE_ITIME_MS() to GAIN_SCALE_ITIME_US() - use more accurate scale for lux channel (milli lux) - provide available scales / integration times (using helpers). - adapt to renamed iio-gts-helpers.h file - bu27034 - longer lines in Kconfig - Drop bu27034_meas_en and bu27034_meas_dis wrappers. - Change device-name from bu27034-als to bu27034 --- drivers/iio/light/Kconfig | 12 + drivers/iio/light/Makefile | 1 + drivers/iio/light/rohm-bu27034.c | 1491 ++++++++++++++++++++++++++++++ 3 files changed, 1504 insertions(+) create mode 100644 drivers/iio/light/rohm-bu27034.c