| Message ID | 20230411011203.5013-3-subhajit.ghosh@tweaklogic.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Support for Avago APDS9306 Ambient Light Sensor | expand |
On Tue, Apr 11, 2023 at 09:12:03AM +0800, Subhajit Ghosh wrote: > Driver support for Avago (Broadcom) APDS9306 > Ambient Light Sensor with als and clear channels. > This driver exposes raw values for both the channels. ... > Datasheet at https://docs.broadcom.com/doc/AV02-4755EN > > Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> Drop a blank line and make Datasheet: to be a tag. Datasheet: https://docs.broadcom.com/doc/AV02-4755EN ... > +/* > + * APDS-9306/APDS-9306-065 Ambient Light Sensor > + * > + * Datasheet: https://docs.broadcom.com/doc/AV02-4755EN > + * > + * I2C Address: 0x52 I guess this can be reordered and condensed a bit: * APDS-9306/APDS-9306-065 Ambient Light Sensor * I2C Address: 0x52 * Datasheet: https://docs.broadcom.com/doc/AV02-4755EN > + * Copyright (C) 2023 Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> > + */ ... > +#include <linux/pm.h> > +#include <linux/pm_runtime.h> > +#include <linux/module.h> > +#include <linux/i2c.h> > +#include <linux/err.h> > +#include <linux/mutex.h> > +#include <linux/delay.h> > +#include <linux/regmap.h> > +#include <linux/interrupt.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > +#include <linux/iio/events.h> > +#include <linux/regulator/consumer.h> Sorted? + Blank line. > +#include <asm/unaligned.h> ... > +enum apds9306_power_states { > + standby, > + active, Namespace? Capital letters? > +}; > + > +enum apds9306_int_channels { > + clear, > + als, Ditto. > +}; ... > +/* Sampling Frequency in uSec */ > +static const int apds9306_repeat_rate_period[] = { > + 25000, 50000, 100000, 200000, 500000, 1000000, > + 2000000 Can be on a single line. > +}; ... Perhaps you want to add a few static_asserts() to be sure that the ARRAY_SIZE() of the tables are all greater or equal to each others. ... > +struct apds9306_data { > + struct i2c_client *client; > + struct iio_dev *indio_dev; > + /* > + * Protect single als reads and device > + * power states. > + */ > + struct mutex mutex; > + > + struct regmap *regmap; > + /* Reg: MAIN_CTRL, Field: SW_Reset */ > + struct regmap_field *regfield_sw_reset; > + /* Reg: MAIN_CTRL, Field: ALS_EN */ > + struct regmap_field *regfield_en; > + /* Reg: ALS_MEAS_RATE, Field: ALS Resolution/Bit Width */ Why not converting all comments to a kernel-doc for the entire structure? > + struct regmap_field *regfield_intg_time; > + /* Reg: ALS_MEAS_RATE, Field: ALS Measurement Rate */ > + struct regmap_field *regfield_repeat_rate; > + /* Reg: ALS_GAIN, Field: ALS Gain Range */ > + struct regmap_field *regfield_scale; > + /* Reg: INT_CFG, Field: ALS Interrupt Source */ > + struct regmap_field *regfield_int_src; > + /* Reg: INT_CFG, Field: ALS Variation Interrupt Mode */ > + struct regmap_field *regfield_int_thresh_var_en; > + /* Reg: INT_CFG, Field: ALS Interrupt Enable */ > + struct regmap_field *regfield_int_en; > + /* Reg: INT_PERSISTENCE, Field: ALS_PERSIST */ > + struct regmap_field *regfield_int_persist_val; > + /* Reg: ALS_THRESH_VAR, Field: ALS_THRES_VAR */ > + struct regmap_field *regfield_int_thresh_var_val; > + > + u8 intg_time_idx; > + u8 repeat_rate_idx; > + u8 gain_idx; > + u8 int_ch; > +}; ... > +static const struct regmap_config apds9306_regmap = { > + .name = "apds9306_regmap", > + .reg_bits = 8, > + .val_bits = 8, > + .rd_table = &apds9306_readable_table, > + .wr_table = &apds9306_writable_table, > + .volatile_table = &apds9306_volatile_table, > + .precious_table = &apds9306_precious_table, > + .max_register = APDS9306_ALS_THRES_VAR, > + .cache_type = REGCACHE_RBTREE, Do you need an internal regmap lock? If so, why? > +}; ... > +static ssize_t thresh_channel_select_show(struct device *dev, > + struct device_attribute *attr, char *buff) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct apds9306_data *data = iio_priv(indio_dev); > + int ret; > + > + if (data->int_ch == 1) > + ret = sprintf(buff, "als\n"); Must be sysfs_emit(). > + else if (data->int_ch == 0) > + ret = sprintf(buff, "clear\n"); Must be sysfs_emit(). > + else > + ret = -EINVAL; > + > + return ret; Make the string literal array for these and... > +} > + > +static ssize_t thresh_channel_select_store(struct device *dev, > + struct device_attribute *attr, const char *buff, size_t len) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct apds9306_data *data = iio_priv(indio_dev); > + int ret, channel; > + > + if (len <= 0 || len > 6) > + return -EINVAL; > + > + if (!strncmp(buff, "als", 3)) > + channel = 1; > + else if (!strncmp(buff, "clear", 5)) > + channel = 0; > + else > + return -EINVAL; ...use sysfs_match_string(). > + > + ret = regmap_field_write(data->regfield_int_src, channel); > + if (ret) > + return ret; > + > + data->int_ch = channel; > + > + return len; > +} ... > +static struct attribute *apds9306_event_attributes[] = { > + &iio_const_attr_thresh_either_period_available.dev_attr.attr, > + &iio_const_attr_thresh_channels_available.dev_attr.attr, > + &iio_const_attr_thresh_adaptive_either_values_available.dev_attr.attr, > + &iio_dev_attr_thresh_channel_select.dev_attr.attr, > + NULL, No comma for a terminator entry. > +}; ... > +static int apds9306_power_state(struct apds9306_data *data, > + enum apds9306_power_states state) > +{ > + int ret; > + > + /* Reset not included as it causes ugly I2C bus error */ > + switch (state) { > + case standby: > + return regmap_field_write(data->regfield_en, 0); > + case active: > + ret = regmap_field_write(data->regfield_en, 1); > + if (ret) > + return ret; > + /* 5ms wake up time */ > + usleep_range(5000, 10000); > + break; > + default: > + return -EINVAL; > + } > + return 0; Move that to a single user of this line inside the switch-case. > +} ... > + struct device *dev = &data->client->dev; Why data contains I²C client pointer, what for? ... > + int ret = 0; Unneeded assignment... > + if (en) { > + ret = pm_runtime_resume_and_get(dev); > + if (ret < 0) { > + dev_err(dev, "runtime resume failed: %d\n", ret); > + return ret; > + } > + ret = 0; > + } else { > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > + } > + > + return ret; ...just return 0 here. ... > + while (retries--) { > + ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS, > + &status); > + if (ret) { > + dev_err(dev, "read status failed: %d\n", ret); > + return ret; > + } > + if (status & APDS9306_ALS_DATA_STAT_MASK) > + break; > + /* > + * In case of continuous one-shot read from userspace, > + * new data is available after sampling period. > + * Delays are in the range of 25ms to 2secs. > + */ > + fsleep(delay); > + } regmap_read_poll_timeout(). ... > + *val = get_unaligned_le24(&buff[0]); buff is enough, but if you want to be too explicit... ... > + ret = apds9306_runtime_power(data, 0); > + if (ret) > + return ret; > + > + return ret; return apds...(...); ... > + if (apds9306_intg_time[i][0] == val && > + apds9306_intg_time[i][1] == val2) { Too many spaces and wrong indentation. ... > + if (apds9306_repeat_rate_freq[i][0] == val && > + apds9306_repeat_rate_freq[i][1] == val2) { Ditto. ... > + if (apds9306_gain[i] == val) { Too many spaces. ... > + if (thad > 7) > + return -EINVAL; This 7 should be defined with a meaningful name. ... > + if (val < 0 || val > 7) > + return -EINVAL; Ditto. ... > + struct apds9306_data *data = iio_priv(indio_dev); > + int ret = -EINVAL; This assignment is used only once, so make it explicit in that user below. > + mutex_lock(&data->mutex); > + switch (mask) { > + case IIO_CHAN_INFO_INT_TIME: > + ret = apds9306_intg_time_set(data, val, val2); > + break; > + case IIO_CHAN_INFO_SAMP_FREQ: > + ret = apds9306_sampling_freq_set(data, val, val2); > + break; > + case IIO_CHAN_INFO_SCALE: > + if (val2 != 0) > + break; > + ret = apds9306_gain_set(data, val); > + break; > + default: > + ret = -EINVAL; > + } > + mutex_unlock(&data->mutex); > + > + return ret; > +} ... > + /* The interrupt line is released and the interrupt flag is > + * cleared as a result of reading the status register > + */ /* * Style of the multi-line comment * is wrong. */ ... > + break; > + case IIO_EV_TYPE_THRESH_ADAPTIVE: > + ret = apds9306_event_thresh_adaptive_get(data, val); > + if (ret) > + return ret; > + break; Wrong indentation of this in entire switch-case. Why the style is different from piece of code to piece of code? ... > + if (val < 0 || val > 0xFFFFF) > + return -EINVAL; Definition and use some plain decimal number if it's about the upper limit of the respective non-bitwise value. > + put_unaligned_le24(val, buff); > + return regmap_bulk_write(data->regmap, var, buff, sizeof(buff)); > + case IIO_EV_TYPE_THRESH_ADAPTIVE: > + return apds9306_event_thresh_adaptive_set(data, val); > + default: > + return -EINVAL; > + } > + return 0; Is it dead code? > +} ... > + case IIO_EV_TYPE_THRESH: > + ret = regmap_field_read(data->regfield_int_en, &val); > + if (ret) > + return ret; > + return val; > + case IIO_EV_TYPE_THRESH_ADAPTIVE: > + ret = regmap_field_read(data->regfield_int_thresh_var_en, &val); > + if (ret) > + return ret; > + return val; > + default: > + return -EINVAL; > + } > + > + return 0; Ditto. ... > + switch (type) { > + case IIO_EV_TYPE_THRESH: > + ret = regmap_field_read(data->regfield_int_en, &curr_state); > + if (ret) > + return ret; > + if (curr_state == state) > + return 0; > + ret = regmap_field_write(data->regfield_int_en, state); > + if (ret) > + return ret; > + mutex_lock(&data->mutex); > + ret = apds9306_runtime_power(data, state); > + mutex_unlock(&data->mutex); > + if (ret) > + return ret; > + break; > + case IIO_EV_TYPE_THRESH_ADAPTIVE: > + ret = regmap_field_write(data->regfield_int_thresh_var_en, state); > + if (ret) > + return ret; > + break; > + default: > + ret = -EINVAL; Again, use the same style, here you have no lock, so you may return directly. No need to have this. > + } > + > + return ret; Why ret? ... > + regmap_field_write(data->regfield_int_en, 0); > + /* Clear status */ > + regmap_read(data->regmap, APDS9306_MAIN_STATUS, &status); > + /* Put the device in standby mode */ > + apds9306_power_state(data, standby); No error checks at all? ... > + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, > + apds9306_irq_handler, IRQF_TRIGGER_FALLING | > + IRQF_ONESHOT, "apds9306_event", indio_dev); Re-indent them to have logical split on the lines. > + if (ret) > + return dev_err_probe(&client->dev, ret, > + "failed to assign interrupt.\n"); ... > +static int apds9306_runtime_suspend(struct device *dev) > +{ > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); WHy? Use dev_get_drvdata() directly. > + struct apds9306_data *data = iio_priv(indio_dev); > + int ret; > + > + ret = apds9306_power_state(data, standby); > + if (ret) > + regcache_cache_only(data->regmap, true); > + > + return ret; > +} > + > +static int apds9306_runtime_resume(struct device *dev) > +{ > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); Ditto. > + struct apds9306_data *data = iio_priv(indio_dev); > + int ret; > + > + regcache_cache_only(data->regmap, false); > + ret = regcache_sync(data->regmap); > + if (!ret) { if (ret) return ret; > + ret = apds9306_power_state(data, active); > + if (ret) > + regcache_cache_only(data->regmap, true); > + } > + > + return ret; > +} ... > +static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops, apds9306_runtime_suspend, > + apds9306_runtime_resume, NULL); Do a logical split between lines. static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops, apds9306_runtime_suspend, apds9306_runtime_resume, NULL); Alternatively static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops, apds9306_runtime_suspend, apds9306_runtime_resume, NULL); ... > +static struct i2c_driver apds9306_driver = { > + .driver = { > + .name = "apds9306", > + .pm = pm_ptr(&apds9306_pm_ops), > + .of_match_table = apds9306_of_match, > + }, > + .probe_new = apds9306_probe, > + .id_table = apds9306_id, > +}; > + Redundant blank line. > +module_i2c_driver(apds9306_driver);
Hi Andy, Thank you so much for the detailed review. Appreciate you taking time to do this. It would be helpful if you could answers few questions inline? > >> +/* >> + * APDS-9306/APDS-9306-065 Ambient Light Sensor >> + * >> + * Datasheet: https://docs.broadcom.com/doc/AV02-4755EN >> + * >> + * I2C Address: 0x52 > > I guess this can be reordered and condensed a bit: > > * APDS-9306/APDS-9306-065 Ambient Light Sensor > * I2C Address: 0x52 > * Datasheet: https://docs.broadcom.com/doc/AV02-4755EN > >> + * Copyright (C) 2023 Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> >> + */ > Understood. It will be fixed. >> +#include <linux/iio/events.h> >> +#include <linux/regulator/consumer.h> > > Sorted? > > + Blank line. > >> +#include <asm/unaligned.h> > Understood. It will be fixed. > Namespace? Capital letters? > >> +}; >> + >> +enum apds9306_int_channels { >> + clear, >> + als, > > Ditto. > >> +}; > Understood. It will be fixed. > >> +/* Sampling Frequency in uSec */ >> +static const int apds9306_repeat_rate_period[] = { >> + 25000, 50000, 100000, 200000, 500000, 1000000, >> + 2000000 > > Can be on a single line. > >> +}; > > ... > > Perhaps you want to add a few static_asserts() to be sure that the ARRAY_SIZE() > of the tables are all greater or equal to each others. > > ... Ok. I'll do that. > > Why not converting all comments to a kernel-doc for the entire structure? > Yes. Sure. > > ... > >> +static const struct regmap_config apds9306_regmap = { >> + .name = "apds9306_regmap", >> + .reg_bits = 8, >> + .val_bits = 8, >> + .rd_table = &apds9306_readable_table, >> + .wr_table = &apds9306_writable_table, >> + .volatile_table = &apds9306_volatile_table, >> + .precious_table = &apds9306_precious_table, >> + .max_register = APDS9306_ALS_THRES_VAR, >> + .cache_type = REGCACHE_RBTREE, > > Do you need an internal regmap lock? If so, why? For event interface - interrupt enable, adaptive interrupt enable, upper and lower threshold values, selection of clear or als channels for interrupt, the mutex in the driver's private data structure is not used. I thought to use the regmap's internal locking mechanism for mutual exclusion as the values are directly written to or read from the device registers form the write_event(), read_event(), write_event_config() and read_event_config(). What do you think? > >> + else if (data->int_ch == 0) >> + ret = sprintf(buff, "clear\n"); > > Must be sysfs_emit(). Sure. I'll use that. > >> + else >> + ret = -EINVAL; >> + >> + return ret; > > Make the string literal array for these and... > ... >> + channel = 1; >> + else if (!strncmp(buff, "clear", 5)) >> + channel = 0; >> + else >> + return -EINVAL; > > ...use sysfs_match_string(). Understood. It will be done. >> + NULL, > > No comma for a terminator entry. > >> +}; > Sorry. Mistake. It will be fixed. > >> +static int apds9306_power_state(struct apds9306_data *data, >> + enum apds9306_power_states state) >> +{ >> + int ret; >> + >> + /* Reset not included as it causes ugly I2C bus error */ >> + switch (state) { >> + case standby: >> + return regmap_field_write(data->regfield_en, 0); >> + case active: >> + ret = regmap_field_write(data->regfield_en, 1); >> + if (ret) >> + return ret; >> + /* 5ms wake up time */ >> + usleep_range(5000, 10000); >> + break; >> + default: >> + return -EINVAL; >> + } > >> + return 0; > > Move that to a single user of this line inside the switch-case. Sorry, I did not get you. Can you please elaborate? >> + struct device *dev = &data->client->dev; > > Why data contains I²C client pointer, what for? I copied the implementation. It will be re-implemented. > > ... > >> + int ret = 0; > > Unneeded assignment... Yes. It will be fixed. > >> + if (en) { >> + ret = pm_runtime_resume_and_get(dev); >> + if (ret < 0) { >> + dev_err(dev, "runtime resume failed: %d\n", ret); >> + return ret; >> + } >> + ret = 0; >> + } else { >> + pm_runtime_mark_last_busy(dev); >> + pm_runtime_put_autosuspend(dev); >> + } >> + >> + return ret; > > ...just return 0 here. Ok. > > ... > >> + while (retries--) { >> + ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS, >> + &status); >> + if (ret) { >> + dev_err(dev, "read status failed: %d\n", ret); >> + return ret; >> + } >> + if (status & APDS9306_ALS_DATA_STAT_MASK) >> + break; >> + /* >> + * In case of continuous one-shot read from userspace, >> + * new data is available after sampling period. >> + * Delays are in the range of 25ms to 2secs. >> + */ >> + fsleep(delay); >> + } > > regmap_read_poll_timeout(). According to the regmap_read_poll_timeout() documentation, the maximum time to sleep between reads should be less than ~20ms as it uses usleep_range(). If userspace is doing continuous reads, then data is available after sampling period (25ms to 2sec) or integration time (3.125ms to 400ms) whichever is greater. The runtime_suspend() function is called after 5 seconds, so the device is still active and running. If the ALS data bit is not set in status reg, it is efficient to sleep for one sampling period rather than continuously checking the status reg within ~20ms if we use regmap_read_poll_timeout(). Do you have any suggestions? > > ... > >> + *val = get_unaligned_le24(&buff[0]); > > buff is enough, but if you want to be too explicit... Ok. It will be done. > > ... > >> + ret = apds9306_runtime_power(data, 0); >> + if (ret) >> + return ret; >> + >> + return ret; > > return apds...(...); Oh yes. > > ... > >> + if (apds9306_intg_time[i][0] == val && >> + apds9306_intg_time[i][1] == val2) { > > Too many spaces and wrong indentation. > > ... > >> + if (apds9306_repeat_rate_freq[i][0] == val && >> + apds9306_repeat_rate_freq[i][1] == val2) { > > Ditto. > > ... > >> + if (apds9306_gain[i] == val) { > > Too many spaces. Thank you. It will be fixed. > > ... > >> + if (thad > 7) >> + return -EINVAL; > > This 7 should be defined with a meaningful name. > > ... > >> + if (val < 0 || val > 7) >> + return -EINVAL; > > Ditto. > Understood. > ... > >> + struct apds9306_data *data = iio_priv(indio_dev); >> + int ret = -EINVAL; > > This assignment is used only once, so make it explicit in that user below. > Got it. > > ... > >> + /* The interrupt line is released and the interrupt flag is >> + * cleared as a result of reading the status register >> + */ > > /* > * Style of the multi-line comment > * is wrong. > */ > Thank you for this. I've been scratching my head for some time on the style of commenting. > ... > >> + break; >> + case IIO_EV_TYPE_THRESH_ADAPTIVE: >> + ret = apds9306_event_thresh_adaptive_get(data, val); >> + if (ret) >> + return ret; > >> + break; > > Wrong indentation of this in entire switch-case. Why the style is different > from piece of code to piece of code? > > ... > >> + if (val < 0 || val > 0xFFFFF) >> + return -EINVAL; > > Definition and use some plain decimal number if it's about the upper limit of > the respective non-bitwise value. Ok, it will be done > >> + default: >> + return -EINVAL; >> + } > >> + return 0; > > Is it dead code? Yes, seems to be. It will be fixed. >> + default: >> + return -EINVAL; >> + } >> + >> + return 0; > > Ditto. Yes, seems to be. It will be fixed. > > ... > >> + switch (type) { >> + case IIO_EV_TYPE_THRESH: >> + ret = regmap_field_read(data->regfield_int_en, &curr_state); >> + if (ret) >> + return ret; >> + if (curr_state == state) >> + return 0; >> + ret = regmap_field_write(data->regfield_int_en, state); >> + if (ret) >> + return ret; >> + mutex_lock(&data->mutex); >> + ret = apds9306_runtime_power(data, state); >> + mutex_unlock(&data->mutex); >> + if (ret) >> + return ret; >> + break; >> + case IIO_EV_TYPE_THRESH_ADAPTIVE: >> + ret = regmap_field_write(data->regfield_int_thresh_var_en, state); >> + if (ret) >> + return ret; >> + break; >> + default: >> + ret = -EINVAL; > > Again, use the same style, here you have no lock, so you may return directly. > No need to have this. > >> + } >> + >> + return ret; > > Why ret? Once, it is clear whether regmap's internal locking should be used or not, this function will be re-implemented. > > ... > >> + regmap_field_write(data->regfield_int_en, 0); >> + /* Clear status */ >> + regmap_read(data->regmap, APDS9306_MAIN_STATUS, &status); >> + /* Put the device in standby mode */ >> + apds9306_power_state(data, standby); > > > No error checks at all? Missed it. I'll do that. > > ... > >> + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, >> + apds9306_irq_handler, IRQF_TRIGGER_FALLING | >> + IRQF_ONESHOT, "apds9306_event", indio_dev); > > Re-indent them to have logical split on the lines. Yes. > >> +static int apds9306_runtime_suspend(struct device *dev) >> +{ >> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); > > WHy? Use dev_get_drvdata() directly. Thanks. I'll use that function. >> + >> +static int apds9306_runtime_resume(struct device *dev) >> +{ >> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); > > Ditto. Yes. > Alternatively > > static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops, > apds9306_runtime_suspend, > apds9306_runtime_resume, > NULL); > Thank you. Second option looks nice. I'll stick to that. > > Redundant blank line. It will be fixed. Regards, Subhajit Ghosh
On Wed, Apr 12, 2023 at 12:29:15PM +0800, Subhajit Ghosh wrote: ... > > > +static const struct regmap_config apds9306_regmap = { > > > + .name = "apds9306_regmap", > > > + .reg_bits = 8, > > > + .val_bits = 8, > > > + .rd_table = &apds9306_readable_table, > > > + .wr_table = &apds9306_writable_table, > > > + .volatile_table = &apds9306_volatile_table, > > > + .precious_table = &apds9306_precious_table, > > > + .max_register = APDS9306_ALS_THRES_VAR, > > > + .cache_type = REGCACHE_RBTREE, > > > > Do you need an internal regmap lock? If so, why? > For event interface - interrupt enable, adaptive interrupt enable, > upper and lower threshold values, selection of clear or als > channels for interrupt, the mutex in the driver's private data structure > is not used. > I thought to use the regmap's internal locking mechanism for > mutual exclusion as the values are directly written to or read from > the device registers form the write_event(), read_event(), > write_event_config() and read_event_config(). > What do you think? I didn't get. If you have a sequence of registers to be read/write/modified/etc in IRQ handler and/or elsewhere and at the same time in IRQ or elsewhere you have even a single IO access to the hardware you have to be sure that the IO ordering has no side effects. regmap API does not guarantee that. It only works on a simple read/write/modify of a _single_ register, or a coupled group of registers (like bulk ops), if your case is sparse, you on your own and probably lucky enough not to have an issue during the testing. So, take your time and think more about what you are doing in the driver and what locking schema should take place. ... > > > +static int apds9306_power_state(struct apds9306_data *data, > > > + enum apds9306_power_states state) > > > +{ > > > + int ret; > > > + > > > + /* Reset not included as it causes ugly I2C bus error */ > > > + switch (state) { > > > + case standby: > > > + return regmap_field_write(data->regfield_en, 0); > > > + case active: > > > + ret = regmap_field_write(data->regfield_en, 1); > > > + if (ret) > > > + return ret; > > > + /* 5ms wake up time */ > > > + usleep_range(5000, 10000); > > > + break; > > > + default: > > > + return -EINVAL; > > > + } > > > > > + return 0; > > > > Move that to a single user of this line inside the switch-case. > Sorry, I did not get you. Can you please elaborate? The user of this return is only one case in the switch. Instead of breaking the switch-case, just move this return statement to there. ... > > > + struct device *dev = &data->client->dev; > > > > Why data contains I²C client pointer, what for? > I copied the implementation. It will be re-implemented. I mean, how client pointer is used in comparison to the plain pointer to the generic device object. ... > > > + while (retries--) { > > > + ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS, > > > + &status); > > > + if (ret) { > > > + dev_err(dev, "read status failed: %d\n", ret); > > > + return ret; > > > + } > > > + if (status & APDS9306_ALS_DATA_STAT_MASK) > > > + break; > > > + /* > > > + * In case of continuous one-shot read from userspace, > > > + * new data is available after sampling period. > > > + * Delays are in the range of 25ms to 2secs. > > > + */ > > > + fsleep(delay); > > > + } > > > > regmap_read_poll_timeout(). > According to the regmap_read_poll_timeout() documentation, the maximum time > to sleep between reads should be less than ~20ms as it uses usleep_range(). > > If userspace is doing continuous reads, then data is available after sampling > period (25ms to 2sec) or integration time (3.125ms to 400ms) whichever is > greater. > > The runtime_suspend() function is called after 5 seconds, so the device is > still active and running. > > If the ALS data bit is not set in status reg, it is efficient to sleep for > one sampling period rather than continuously checking the status reg > within ~20ms if we use regmap_read_poll_timeout(). > > Do you have any suggestions? Yes, Use proposed API. It takes _two_ timeout parameters, one of which is the same as your delay. You may actually resplit it by multiplying retries and decreasing delay to satisfy the regmap_read_poll_timeout() recommendation.
On 12/4/23 21:37, Andy Shevchenko wrote: > On Wed, Apr 12, 2023 at 12:29:15PM +0800, Subhajit Ghosh wrote: > > ... > >>>> +static const struct regmap_config apds9306_regmap = { >>>> + .name = "apds9306_regmap", >>>> + .reg_bits = 8, >>>> + .val_bits = 8, >>>> + .rd_table = &apds9306_readable_table, >>>> + .wr_table = &apds9306_writable_table, >>>> + .volatile_table = &apds9306_volatile_table, >>>> + .precious_table = &apds9306_precious_table, >>>> + .max_register = APDS9306_ALS_THRES_VAR, >>>> + .cache_type = REGCACHE_RBTREE, >>> >>> Do you need an internal regmap lock? If so, why? >> For event interface - interrupt enable, adaptive interrupt enable, >> upper and lower threshold values, selection of clear or als >> channels for interrupt, the mutex in the driver's private data structure >> is not used. >> I thought to use the regmap's internal locking mechanism for >> mutual exclusion as the values are directly written to or read from >> the device registers form the write_event(), read_event(), >> write_event_config() and read_event_config(). >> What do you think? > > I didn't get. If you have a sequence of registers to be read/write/modified/etc > in IRQ handler and/or elsewhere and at the same time in IRQ or elsewhere you > have even a single IO access to the hardware you have to be sure that the IO > ordering has no side effects. regmap API does not guarantee that. It only works > on a simple read/write/modify of a _single_ register, or a coupled group of > registers (like bulk ops), if your case is sparse, you on your own and probably > lucky enough not to have an issue during the testing. So, take your time and > think more about what you are doing in the driver and what locking schema > should take place. > > ... Agree. I have to rethink and re-implement the locking mechanism. > >>>> +static int apds9306_power_state(struct apds9306_data *data, >>>> + enum apds9306_power_states state) >>>> +{ >>>> + int ret; >>>> + >>>> + /* Reset not included as it causes ugly I2C bus error */ >>>> + switch (state) { >>>> + case standby: >>>> + return regmap_field_write(data->regfield_en, 0); >>>> + case active: >>>> + ret = regmap_field_write(data->regfield_en, 1); >>>> + if (ret) >>>> + return ret; >>>> + /* 5ms wake up time */ >>>> + usleep_range(5000, 10000); >>>> + break; >>>> + default: >>>> + return -EINVAL; >>>> + } >>> >>>> + return 0; >>> >>> Move that to a single user of this line inside the switch-case. >> Sorry, I did not get you. Can you please elaborate? > > The user of this return is only one case in the switch. Instead of breaking > the switch-case, just move this return statement to there. > Ok. It will be done. > ... > >>>> + struct device *dev = &data->client->dev; >>> >>> Why data contains I²C client pointer, what for? >> I copied the implementation. It will be re-implemented. > > I mean, how client pointer is used in comparison to the plain pointer to the > generic device object. > > ... > >>>> + while (retries--) { >>>> + ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS, >>>> + &status); >>>> + if (ret) { >>>> + dev_err(dev, "read status failed: %d\n", ret); >>>> + return ret; >>>> + } >>>> + if (status & APDS9306_ALS_DATA_STAT_MASK) >>>> + break; >>>> + /* >>>> + * In case of continuous one-shot read from userspace, >>>> + * new data is available after sampling period. >>>> + * Delays are in the range of 25ms to 2secs. >>>> + */ >>>> + fsleep(delay); >>>> + } >>> >>> regmap_read_poll_timeout(). >> According to the regmap_read_poll_timeout() documentation, the maximum time >> to sleep between reads should be less than ~20ms as it uses usleep_range(). >> >> If userspace is doing continuous reads, then data is available after sampling >> period (25ms to 2sec) or integration time (3.125ms to 400ms) whichever is >> greater. >> >> The runtime_suspend() function is called after 5 seconds, so the device is >> still active and running. >> >> If the ALS data bit is not set in status reg, it is efficient to sleep for >> one sampling period rather than continuously checking the status reg >> within ~20ms if we use regmap_read_poll_timeout(). >> >> Do you have any suggestions? > > Yes, Use proposed API. It takes _two_ timeout parameters, one of which is the > same as your delay. You may actually resplit it by multiplying retries and > decreasing delay to satisfy the regmap_read_poll_timeout() recommendation. > Yes, that can be done. I will re-write this function in the next patch. Thanks once again Andy for the detailed review. Regards, Subhajit Ghosh
On Tue, 11 Apr 2023 09:12:03 +0800 Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote: > Driver support for Avago (Broadcom) APDS9306 > Ambient Light Sensor with als and clear channels. > This driver exposes raw values for both the channels. > > Datasheet at https://docs.broadcom.com/doc/AV02-4755EN > > Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> > > --- > > Software reset feature is not implemented as it causes I2C bus error, > the I2C bus driver throws an ugly error message. That's unfortunate and perhaps something we should consider fixing at the i2c layer. Could you point to where it happens? We have a lot of drivers where reset causes an error (Ack missing normally due to simple state machines in the devices). > > Could not locate the Lux calculations from datasheet, only exporting > raw values. Ah. That's annoying as userspace is generally not able to do much with the raw values. Any other known code supporting this device that you can raid for info? If not, then this ist he best we can do. > > Reading of the Status register clears the Data Ready and the Interrupt > Status flags. It makes it tricky to read oneshot values together with > interrupts enabled as the IRQ handler clears the status on receipt > of an interrupt signal. > > Not checking the status in IRQ handler will make the interrupt line > unsharable and it does not reset the interrupt line if the Interrupt > status flag is not cleared. Definitely need to check it but I'm not sure I follow why you can't use it for both purposes with a slightly complex interrupt handler design. Maybe the code makes it clear what the issue is here. > > Many applications don't need interrupts. It is good to have support > for both in the driver. Agreed. > > Sysfs structure with interrupt line defined in DT: > root@stm32mp1:~# tree -I 'dev|name|of_node|power|subsystem|uevent' \ > /sys/bus/iio/devices/iio:device0/ > /sys/bus/iio/devices/iio:device0/ > |-- events These should include the channel they apply to... > | |-- thresh_adaptive_either_en > | |-- thresh_adaptive_either_value > | |-- thresh_adaptive_either_values_available > | |-- thresh_channel_select Non standards ABI needs documentation. Guessing somewhat but do you have a single set of threshold detectors that are fed by a mux off the channels? So can only check one channel at a time? If so then you present one set of controls for each channel and either a) return -EBUSY if someone tries to turn more on than you support b) operating in a fifo way - so always enable last one that was enabled. reading back values of the attributes will allow userspace to see what is turned on. > | |-- thresh_channels_available > | |-- thresh_either_en > | |-- thresh_either_period > | |-- thresh_either_period_available > | |-- thresh_falling_value > | `-- thresh_rising_value > |-- in_illuminance_raw > |-- in_intensity_clear_raw > |-- integration_time > |-- integration_time_available > |-- sampling_frequency > |-- sampling_frequency_available > |-- scale > `-- scale_available > > Sysfs structure with no interrupt: > root@stm32mp1:~# tree -I 'dev|name|of_node|power|subsystem|uevent' \ > /sys/bus/iio/devices/iio:device0/ > /sys/bus/iio/devices/iio:device0/ > |-- in_illuminance_raw > |-- in_intensity_clear_raw > |-- integration_time > |-- integration_time_available > |-- sampling_frequency > |-- sampling_frequency_available > |-- scale > `-- scale_available > > As there is a single interrupt enable option, using the existing > IIO bitmasks to cater for channel selection - als or clear and > interrupt type selection - threshold or adaptive > was looking ambiguous. I am open to any other implementations. This sort of dependency is common but it is effectively impossible to design a usable ABI that covers all combinations that can occur. As a result the ABI defines that a write to any sysfs attribute 'might' change the value of any other. Hence you just have two enables and enabling 1 turns them both on. > > I could not find any _available implementations for the event > interface. Yes - we haven't yet added support except by creating the attributes manually. It might be worth doing. I've never checked how useful it would be. > > Created one sysfs attribute for interrupt channel selection > and one RO attribute for channel availability. > > Created two RO attributes for interrupt period and threshold > adaptive value available ranges. > > The ALS Measurement Rate (Sampling Frequency) has max values > of 2000ms and 2000ms for both 110b and 111b respectively. > This is not a typo. Found out experimentally that assigning > either values, interrupt is generated 2 secs apart when > interrupt conditions are met. > Some comments inline. Given we have these big questions, I didn't focus too much on details. That can come later once the big stuff looks right. Thanks, Jonathan > diff --git a/drivers/iio/light/apds9306.c b/drivers/iio/light/apds9306.c > new file mode 100644 > index 000000000000..d57fb7aaa24d > --- /dev/null > +++ b/drivers/iio/light/apds9306.c > @@ -0,0 +1,1146 @@ > +static struct iio_event_spec apds9306_event_spec[] = { > + { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_RISING, > + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE), > + }, > + { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_FALLING, > + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE), > + }, > + { > + .type = IIO_EV_TYPE_THRESH, > + .mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD), > + }, > + { > + .type = IIO_EV_TYPE_THRESH_ADAPTIVE, > + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) | > + BIT(IIO_EV_INFO_ENABLE), > + }, > + { > + .type = IIO_EV_TYPE_THRESH, > + .mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE), > + }, > +}; > + > +static struct iio_chan_spec apds9306_channels[] = { > + { > + .type = IIO_LIGHT, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), > + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), > + .scan_type = { > + .sign = 'u', > + .realbits = 20, > + .storagebits = 32, > + }, > + .event_spec = apds9306_event_spec, > + .num_event_specs = ARRAY_SIZE(apds9306_event_spec), You'll end up with event specs for both channels. > + }, > + { }, { is fine and more compact. Can use this in all similar places. > + .type = IIO_INTENSITY, > + .channel2 = IIO_MOD_LIGHT_CLEAR, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), > + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), > + .scan_type = { > + .sign = 'u', > + .realbits = 20, > + .storagebits = 32, > + }, > + .modified = 1, > + }, > +}; > + > +static struct iio_chan_spec apds9306_channels_no_events[] = { > + { > + .type = IIO_LIGHT, LIGHT is an illumiance measure. Given you don't have the info to get anywhere near that and the absorption characteristics are unlikely to be such that this channel alone can be used to work out the value LIGHT is not the right type. This is just another INTENSITY channel. > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), > + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), > + .scan_type = { > + .sign = 'u', > + .realbits = 20, > + .storagebits = 32, > + }, > + }, > + { > + .type = IIO_INTENSITY, > + .channel2 = IIO_MOD_LIGHT_CLEAR, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), > + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | > + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), > + .scan_type = { > + .sign = 'u', > + .realbits = 20, > + .storagebits = 32, > + }, > + .modified = 1, > + }, > +}; > + > +static ssize_t thresh_channel_select_show(struct device *dev, > + struct device_attribute *attr, char *buff) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct apds9306_data *data = iio_priv(indio_dev); > + int ret; > + > + if (data->int_ch == 1) > + ret = sprintf(buff, "als\n"); > + else if (data->int_ch == 0) > + ret = sprintf(buff, "clear\n"); > + else > + ret = -EINVAL; > + > + return ret; > +} > + > +static ssize_t thresh_channel_select_store(struct device *dev, > + struct device_attribute *attr, const char *buff, size_t len) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct apds9306_data *data = iio_priv(indio_dev); > + int ret, channel; > + > + if (len <= 0 || len > 6) > + return -EINVAL; > + > + if (!strncmp(buff, "als", 3)) > + channel = 1; > + else if (!strncmp(buff, "clear", 5)) > + channel = 0; As above, provide an event for each channel. Only one can be enabled at a time. > + else > + return -EINVAL; > + > + ret = regmap_field_write(data->regfield_int_src, channel); > + if (ret) > + return ret; > + > + data->int_ch = channel; > + > + return len; > +} ... > > +static int apds9306_power_state(struct apds9306_data *data, > + enum apds9306_power_states state) > +{ > + int ret; > + > + /* Reset not included as it causes ugly I2C bus error */ > + switch (state) { > + case standby: > + return regmap_field_write(data->regfield_en, 0); > + case active: > + ret = regmap_field_write(data->regfield_en, 1); > + if (ret) > + return ret; > + /* 5ms wake up time */ > + usleep_range(5000, 10000); > + break; return 0; > + default: > + return -EINVAL; > + } > + return 0; > +} > + > +static int apds9306_runtime_power(struct apds9306_data *data, int en) > +{ > + struct device *dev = &data->client->dev; > + int ret = 0; > + > + if (en) { > + ret = pm_runtime_resume_and_get(dev); > + if (ret < 0) { > + dev_err(dev, "runtime resume failed: %d\n", ret); > + return ret; > + } > + ret = 0; return 0; > + } else { > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); return 0; > + } > + > + return ret; > +} > + > +static int apds9306_read_data(struct apds9306_data *data, int *val, int reg) > +{ > + struct device *dev = &data->client->dev; > + int ret, delay, status, int_en; > + int retries = 4; > + u8 buff[3]; > + > + ret = apds9306_runtime_power(data, 1); > + if (ret) > + return ret; > + > + /* > + * Whichever is greater - integration time period or > + * sampling period. > + */ > + delay = max(apds9306_intg_time[data->intg_time_idx][1], > + apds9306_repeat_rate_period[data->repeat_rate_idx]); > + > + /* > + * If interrupts are enabled then Status resistor cannot be > + * relied upon as all the status bits are cleared by the > + * interrupt handler in case of an event. Ah. I was assuming sane hardware (always an error :) that would issue an interrupt on the data being ready. I think we can make this work but it is ugly. Add some flags to the state structure. Then whenever you read this register, set whether the two status flags are set of not. Thus in the interrupt handler you can tell if this got there first and here you can tell if the interrupt handler got their first. One messy corner. A status read resets the interrupt line, potentially before we saw the interrupt. Oh goody - normally this silliness only happens as a result of complex interrupt migration or errata. However it is understood what to do about it. If you see the interrupt status flag here, you have no way of knowing if the interrupt line was high for long enough that the interrupt controller saw it. As such your only option is to assume it didn't and inject an extra one. Given a passing of the threshold could in theory have been noisy enough to trigger two actual interrupts very close together userspace should be fine with the extra event - we probably just wasted some cycles doing something twice. The annoying bit will be testing as these races will be somewhat rare. > + */ > + ret = regmap_field_read(data->regfield_int_en, &int_en); > + if (ret) { > + dev_err(dev, "read interrupt status failed: %d\n", ret); > + return ret; > + } > + > + if (!int_en) { > + while (retries--) { > + ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS, > + &status); > + if (ret) { > + dev_err(dev, "read status failed: %d\n", ret); > + return ret; > + } > + if (status & APDS9306_ALS_DATA_STAT_MASK) > + break; > + /* > + * In case of continuous one-shot read from userspace, > + * new data is available after sampling period. > + * Delays are in the range of 25ms to 2secs. > + */ > + fsleep(delay); > + } > + } else > + fsleep(delay); > + > + if (!retries) > + return -EBUSY; > + > + ret = regmap_bulk_read(data->regmap, reg, buff, sizeof(buff)); > + if (ret) { > + dev_err(&data->client->dev, "read data failed\n"); > + return ret; > + } > + > + *val = get_unaligned_le24(&buff[0]); > + > + ret = apds9306_runtime_power(data, 0); > + if (ret) > + return ret; > + > + return ret; > +} > + > +static int apds9306_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct apds9306_data *data = iio_priv(indio_dev); > + int ret, reg; > + > + mutex_lock(&data->mutex); > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + if (chan->channel2 == IIO_MOD_LIGHT_CLEAR) > + reg = APDS9306_CLEAR_DATA_0; > + else > + reg = APDS9306_ALS_DATA_0; > + ret = apds9306_read_data(data, val, reg); > + if (!ret) > + ret = IIO_VAL_INT; > + *val2 = 0; > + break; > + case IIO_CHAN_INFO_INT_TIME: > + ret = apds9306_intg_time_get(data, val, val2); > + if (!ret) > + ret = IIO_VAL_INT_PLUS_MICRO; > + break; > + case IIO_CHAN_INFO_SAMP_FREQ: > + ret = apds9306_sampling_freq_get(data, val, val2); > + if (!ret) > + ret = IIO_VAL_INT_PLUS_MICRO; > + break; > + case IIO_CHAN_INFO_SCALE: > + ret = apds9306_gain_get(data, val); > + if (!ret) Please keep the errors out of line. if (ret) break; ret = IIO_VAL_INT; *val2 = 0; break; > + ret = IIO_VAL_INT; > + *val2 = 0; > + break; > + default: > + ret = -EINVAL; > + } > + mutex_unlock(&data->mutex); > + > + return ret; > +}; > + > +static int apds9306_read_avail(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, const int **vals, > + int *type, int *length, long mask) > +{ > + switch (mask) { > + case IIO_CHAN_INFO_INT_TIME: > + *length = ARRAY_SIZE(apds9306_intg_time) * 2; > + *vals = (const int *)apds9306_intg_time; > + *type = IIO_VAL_INT_PLUS_MICRO; > + return IIO_AVAIL_LIST; > + case IIO_CHAN_INFO_SAMP_FREQ: > + *length = ARRAY_SIZE(apds9306_repeat_rate_freq) * 2; > + *vals = (const int *)apds9306_repeat_rate_freq; > + *type = IIO_VAL_INT_PLUS_MICRO; > + return IIO_AVAIL_LIST; > + case IIO_CHAN_INFO_SCALE: > + *length = ARRAY_SIZE(apds9306_gain); > + *vals = apds9306_gain; > + *type = IIO_VAL_INT; > + return IIO_AVAIL_LIST; > + default: > + return -EINVAL; > + } > + > + return 0; Can't get here. > +} > + > +static int apds9306_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + struct apds9306_data *data = iio_priv(indio_dev); > + int ret = -EINVAL; > + > + mutex_lock(&data->mutex); > + switch (mask) { > + case IIO_CHAN_INFO_INT_TIME: > + ret = apds9306_intg_time_set(data, val, val2); > + break; > + case IIO_CHAN_INFO_SAMP_FREQ: > + ret = apds9306_sampling_freq_set(data, val, val2); > + break; > + case IIO_CHAN_INFO_SCALE: > + if (val2 != 0) > + break; > + ret = apds9306_gain_set(data, val); > + break; > + default: > + ret = -EINVAL; > + } > + mutex_unlock(&data->mutex); > + > + return ret; > +} > + > +static const struct iio_info apds9306_info_no_events = { > + .read_raw = apds9306_read_raw, > + .read_avail = apds9306_read_avail, > + .write_raw = apds9306_write_raw, > +}; > + > +static irqreturn_t apds9306_irq_handler(int irq, void *priv) > +{ > + struct iio_dev *indio_dev = priv; > + struct apds9306_data *data = iio_priv(indio_dev); > + int ret, status; > + > + /* The interrupt line is released and the interrupt flag is /* * The interrupt. > + * cleared as a result of reading the status register > + */ > + ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS, &status); > + if (ret < 0) { > + dev_err(&data->client->dev, "status reg read failed\n"); > + return IRQ_HANDLED; > + } > + > + if ((status & APDS9306_ALS_INT_STAT_MASK)) { > + iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_LIGHT, > + data->int_ch, IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER), > + iio_get_time_ns(indio_dev)); > + } > + > + return IRQ_HANDLED; > +} > + > +static int apds9306_read_event(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + enum iio_event_info info, > + int *val, int *val2) > +{ > + struct apds9306_data *data = iio_priv(indio_dev); > + u8 buff[3]; > + int var, ret; > + > + switch (type) { > + case IIO_EV_TYPE_THRESH: > + if (dir == IIO_EV_DIR_EITHER && info == IIO_EV_INFO_PERIOD) { > + ret = apds9306_event_period_get(data, val); > + if (ret) > + return ret; > + break; > + } > + > + if (dir == IIO_EV_DIR_RISING) > + var = APDS9306_ALS_THRES_UP_0; > + else if (dir == IIO_EV_DIR_FALLING) > + var = APDS9306_ALS_THRES_LOW_0; > + else > + return -EINVAL; > + > + ret = regmap_bulk_read(data->regmap, var, buff, sizeof(buff)); > + if (ret) > + return ret; > + *val = get_unaligned_le24(&buff[0]); > + break; > + case IIO_EV_TYPE_THRESH_ADAPTIVE: > + ret = apds9306_event_thresh_adaptive_get(data, val); > + if (ret) > + return ret; > + break; > + default: > + return -EINVAL; > + } > + > + *val2 = 0; > + return IIO_VAL_INT; Pull this into the cases above so you can return directly. > +} > + > +static int apds9306_write_event(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + enum iio_event_info info, > + int val, int val2) > +{ > + struct apds9306_data *data = iio_priv(indio_dev); > + unsigned int var; > + u8 buff[3]; > + > + switch (type) { > + case IIO_EV_TYPE_THRESH: > + if (dir == IIO_EV_DIR_EITHER && info == IIO_EV_INFO_PERIOD) > + return apds9306_event_period_set(data, val); > + if (dir == IIO_EV_DIR_RISING) > + var = APDS9306_ALS_THRES_UP_0; > + else if (dir == IIO_EV_DIR_FALLING) > + var = APDS9306_ALS_THRES_LOW_0; > + else > + return -EINVAL; > + > + if (val < 0 || val > 0xFFFFF) > + return -EINVAL; > + > + put_unaligned_le24(val, buff); > + return regmap_bulk_write(data->regmap, var, buff, sizeof(buff)); > + case IIO_EV_TYPE_THRESH_ADAPTIVE: > + return apds9306_event_thresh_adaptive_set(data, val); > + default: > + return -EINVAL; > + } > + Can't get here. > + return 0; > +} > + > +static int apds9306_read_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir) > +{ > + struct apds9306_data *data = iio_priv(indio_dev); > + unsigned int val; > + int ret; > + > + switch (type) { > + case IIO_EV_TYPE_THRESH: > + ret = regmap_field_read(data->regfield_int_en, &val); > + if (ret) > + return ret; > + return val; > + case IIO_EV_TYPE_THRESH_ADAPTIVE: > + ret = regmap_field_read(data->regfield_int_thresh_var_en, &val); > + if (ret) > + return ret; > + return val; > + default: > + return -EINVAL; > + } > + > + return 0; Can't get here so don't have this final return. > +} > + > +static int apds9306_write_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + int state) > +{ > + struct apds9306_data *data = iio_priv(indio_dev); > + int ret, curr_state; > + > + state = !!state; > + switch (type) { > + case IIO_EV_TYPE_THRESH: > + ret = regmap_field_read(data->regfield_int_en, &curr_state); > + if (ret) > + return ret; > + if (curr_state == state) > + return 0; > + ret = regmap_field_write(data->regfield_int_en, state); > + if (ret) > + return ret; > + mutex_lock(&data->mutex); This seems unusual. I don't follow why lock is needed here. > + ret = apds9306_runtime_power(data, state); > + mutex_unlock(&data->mutex); > + if (ret) > + return ret; return ret; > + break; > + case IIO_EV_TYPE_THRESH_ADAPTIVE: > + ret = regmap_field_write(data->regfield_int_thresh_var_en, state); > + if (ret) > + return ret; return regmap... > + break; > + default: > + ret = -EINVAL; Early returns are more readable. return -EINVAL; > + } > + > + return ret; > +} > + > +static const struct iio_info apds9306_info = { > + .event_attrs = &apds9306_event_attr_group, Indented one tab more than needed. > + .read_raw = apds9306_read_raw, > + .read_avail = apds9306_read_avail, > + .write_raw = apds9306_write_raw, > + .read_event_value = apds9306_read_event, > + .write_event_value = apds9306_write_event, > + .read_event_config = apds9306_read_event_config, > + .write_event_config = apds9306_write_event_config, > +}; > + > +static void apds9306_powerdown(void *ptr) > +{ > + struct apds9306_data *data = (struct apds9306_data *)ptr; > + int status; > + > + /* Disable interrupts */ > + regmap_field_write(data->regfield_int_thresh_var_en, 0); > + regmap_field_write(data->regfield_int_en, 0); > + /* Clear status */ Why? > + regmap_read(data->regmap, APDS9306_MAIN_STATUS, &status); > + /* Put the device in standby mode */ > + apds9306_power_state(data, standby); > +} > + > +static int apds9306_init_device(struct apds9306_data *data) If an error occurs, this function should be as side effect free as we can make it easily. So I'd expect to see the power state disabled on error. Some of the register writes look harmless to leave in place on error though so can skip those. > +{ > + struct device *dev = &data->client->dev; > + int ret; > + > + ret = apds9306_power_state(data, active); > + if (ret) > + return ret; > + > + ret = pm_runtime_set_active(dev); > + if (ret) > + return ret; > + > + ret = devm_pm_runtime_enable(dev); > + if (ret) > + return ret; > + > + pm_runtime_set_autosuspend_delay(dev, 5000); > + pm_runtime_use_autosuspend(dev); > + > + ret = regmap_reinit_cache(data->regmap, &apds9306_regmap); > + if (ret) > + return ret; > + > + /* Integration time: 100 msec */ > + ret = apds9306_intg_time_set(data, 0, 100000); > + if (ret) > + return ret; > + > + /* Sampling frequency: 100 msec, 10 Hz */ > + ret = apds9306_sampling_freq_set(data, 10, 0); > + if (ret) > + return ret; > + > + /* Scale: x3 */ > + ret = apds9306_gain_set(data, 3); > + if (ret) > + return ret; > + > + /* Interrupt source channel: als */ > + ret = regmap_field_write(data->regfield_int_src, 1); > + if (ret) > + return ret; > + data->int_ch = 1; > + > + /* Interrupts disabled */ > + ret = regmap_field_write(data->regfield_int_en, 0); > + if (ret) > + return ret; > + > + /* Threshold Variance disabled */ > + ret = regmap_field_write(data->regfield_int_thresh_var_en, 0); > + if (ret) > + return ret; return regmap_field_write() one of the static analysis tools loves this, so if we don't tidy it up we'll probably get a patch soon to 'fix' it. Better to do it from the start. > + > + return 0; > +} > + > +static int apds9306_probe(struct i2c_client *client) > +{ > + struct apds9306_data *data; > + struct iio_dev *indio_dev; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > + if (!indio_dev) > + return -ENOMEM; > + > + data = iio_priv(indio_dev); > + data->indio_dev = indio_dev; > + > + mutex_init(&data->mutex); > + > + data->regmap = devm_regmap_init_i2c(client, &apds9306_regmap); > + if (IS_ERR(data->regmap)) > + return dev_err_probe(&client->dev, PTR_ERR(data->regmap), > + "regmap initialization failed\n"); > + > + i2c_set_clientdata(client, indio_dev); > + data->client = client; > + > + ret = apds9306_regfield_init(data); > + if (ret) > + return dev_err_probe(&client->dev, ret, > + "regfield initialization failed\n"); > + > + ret = devm_regulator_get_enable(&client->dev, "vin"); > + if (ret) > + return dev_err_probe(&client->dev, ret, > + "Failed to enable regulator\n"); > + > + indio_dev->name = "apds9306"; > + indio_dev->modes = INDIO_DIRECT_MODE; > + if (client->irq) { > + indio_dev->info = &apds9306_info; > + indio_dev->channels = apds9306_channels; > + indio_dev->num_channels = ARRAY_SIZE(apds9306_channels); > + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, > + apds9306_irq_handler, IRQF_TRIGGER_FALLING | > + IRQF_ONESHOT, "apds9306_event", indio_dev); > + if (ret) > + return dev_err_probe(&client->dev, ret, > + "failed to assign interrupt.\n"); > + } else { > + indio_dev->info = &apds9306_info_no_events; > + indio_dev->channels = apds9306_channels_no_events; > + indio_dev->num_channels = ARRAY_SIZE(apds9306_channels_no_events); > + } > + > + ret = devm_add_action_or_reset(&client->dev, apds9306_powerdown, data); What is this paired with? Normally a devm action has to be undoing something that it follows. Here I'm not immediately seeing a power up. Perhaps it should follow init below? > + if (ret) > + return dev_err_probe(&client->dev, ret, > + "failed to add action on driver unwind\n"); > + > + ret = apds9306_init_device(data); > + if (ret) > + return dev_err_probe(&client->dev, ret, "failed to init device\n"); > + > + return devm_iio_device_register(&client->dev, indio_dev); > +} > + > +static int apds9306_runtime_suspend(struct device *dev) > +{ > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); > + struct apds9306_data *data = iio_priv(indio_dev); > + int ret; > + > + ret = apds9306_power_state(data, standby); > + if (ret) > + regcache_cache_only(data->regmap, true); > + > + return ret; > +} > + > +static int apds9306_runtime_resume(struct device *dev) > +{ > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); > + struct apds9306_data *data = iio_priv(indio_dev); > + int ret; > + > + regcache_cache_only(data->regmap, false); > + ret = regcache_sync(data->regmap); > + if (!ret) { > + ret = apds9306_power_state(data, active); Add a comment on why this might be needed only sometimes. Not obvious to me. > + if (ret) > + regcache_cache_only(data->regmap, true); > + } > + > + return ret; > +}
Thank you Jonathan for the review. Answering only to the Big questions. >> >> Software reset feature is not implemented as it causes I2C bus error, >> the I2C bus driver throws an ugly error message. > > That's unfortunate and perhaps something we should consider fixing > at the i2c layer. Could you point to where it happens? > > We have a lot of drivers where reset causes an error (Ack missing normally > due to simple state machines in the devices). Below function which cause the error: regmap_field_write(data->reg_sw_reset, 1); regmap_write(data->regmap, APDS9306_MAIN_CTRL, 0x10); i2c_smbus_write_byte_data(data->client, APDS9306_MAIN_CTRL, 0x10); Error messages: [ 3340.426180] stm32f7-i2c 40012000.i2c: <stm32f7_i2c_isr_error>: Bus error accessing addr 0x52 [ 3340.433880] stm32f7-i2c 40012000.i2c: Trying to recover bus The function which gets called: https://elixir.bootlin.com/linux/latest/source/drivers/i2c/busses/i2c-stm32f7.c#L1622 There is an errata associated with I2C for STM32MP157C (Section 2.19.2, Pg 35): https://www.st.com/resource/en/errata_sheet/es0438-stm32mp151x3x7x-device-errata-stmicroelectronics.pdf It speaks about - "Spurious bus error detection in master mode". But I don't think it has got anything to do with our case. I use STM32MP157C-DK2 board as my reference device. The Reference manual to the STM32MP157C: https://www.st.com/resource/en/reference_manual/DM00327659.pdf stm32f7_i2c_isr_error() handler gets called because a control bit is set ERRIE which enables interrupts from the I2C controller for Buss errors, Arbitration losses, Overrun/Underrun PEC error, Timeout, etc. I am not sure about other chips. A possible way to mitigate these kind of issues would be to pass a flag from upper layers to the i2c bus driver (I2C_SMBUS_REPORT_ERR_OFF or something on those lines). The drivers can then implement in struct i2c_algorithm in smbus_xfer() function where they can check for the flag and disable error checking. I don't have in depth knowledge on this subject so excuse my lack of understanding. > >> >> Could not locate the Lux calculations from datasheet, only exporting >> raw values. > > Ah. That's annoying as userspace is generally not able to do much with > the raw values. Any other known code supporting this device that you > can raid for info? > > If not, then this ist he best we can do. > This device is similar to LTRF216A. If I use the calculation in ltrf216a then I would have to verify them with Lux meter and controlled light source or ltrf216a. This will be bit difficult for me at this moment. >> >> Reading of the Status register clears the Data Ready and the Interrupt >> Status flags. It makes it tricky to read oneshot values together with >> interrupts enabled as the IRQ handler clears the status on receipt >> of an interrupt signal. >> >> Not checking the status in IRQ handler will make the interrupt line >> unsharable and it does not reset the interrupt line if the Interrupt >> status flag is not cleared. > > Definitely need to check it but I'm not sure I follow why you can't > use it for both purposes with a slightly complex interrupt handler design. > Maybe the code makes it clear what the issue is here. Answers are in below comment. >> + >> +static int apds9306_read_data(struct apds9306_data *data, int *val, int reg) >> +{ >> + struct device *dev = &data->client->dev; >> + int ret, delay, status, int_en; >> + int retries = 4; >> + u8 buff[3]; >> + >> + ret = apds9306_runtime_power(data, 1); >> + if (ret) >> + return ret; >> + >> + /* >> + * Whichever is greater - integration time period or >> + * sampling period. >> + */ >> + delay = max(apds9306_intg_time[data->intg_time_idx][1], >> + apds9306_repeat_rate_period[data->repeat_rate_idx]); >> + >> + /* >> + * If interrupts are enabled then Status resistor cannot be >> + * relied upon as all the status bits are cleared by the >> + * interrupt handler in case of an event. > > Ah. I was assuming sane hardware (always an error :) that would issue > an interrupt on the data being ready. I think we can make this work > but it is ugly. Add some flags to the state structure. Then whenever > you read this register, set whether the two status flags are set of not. > Thus in the interrupt handler you can tell if this got there first and > here you can tell if the interrupt handler got their first. If I have a flag in the state structure for this, then a timestamp would also be required or may be just a timestamp. > > One messy corner. A status read resets the interrupt line, potentially before > we saw the interrupt. Oh goody - normally this silliness only happens as > a result of complex interrupt migration or errata. However it is understood > what to do about it. > > If you see the interrupt status flag here, you have no way of knowing > if the interrupt line was high for long enough that the interrupt controller > saw it. As such your only option is to assume it didn't and inject an extra > one. Given a passing of the threshold could in theory have been noisy enough > to trigger two actual interrupts very close together userspace should be fine > with the extra event - we probably just wasted some cycles doing something twice. > > The annoying bit will be testing as these races will be somewhat rare. As per my understanding the Status register has - "Observer effect" The Status register has got 3 bits - Power On Status, ALS Interrupt and ALS Data. A single read of the register clears all three bits. In case of an interrupt event, the interrupt line is held low (Active) by the device till the Status register is read. As this is an edge triggered interrupt, we stop receiving interrupts till we read the status register. This is why it is good to acknowledge the interrupt by reading the Status register in the ISR. Reading the Status register in the ISR inadvertently clears the "ALS Data" bit as well. What you are asking is to have a read flag in the static struct and a timestamp flag may be. In the isr, we check the flag and if the timestamp is within one integration period, if yes, we don't read the Status register any more from the isr. And in this function we read the Status register and update the read flag and the timestamp with current time. Not sure if my understanding is correct but I will try the above. > >> + */ >> + ret = regmap_field_read(data->regfield_int_en, &int_en); >> + if (ret) { >> + dev_err(dev, "read interrupt status failed: %d\n", ret); >> + return ret; >> + } >> + >> + if (!int_en) { >> + while (retries--) { >> + ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS, >> + &status); >> + if (ret) { >> + dev_err(dev, "read status failed: %d\n", ret); >> + return ret; >> + } >> + if (status & APDS9306_ALS_DATA_STAT_MASK) >> + break; >> + /* >> + * In case of continuous one-shot read from userspace, >> + * new data is available after sampling period. >> + * Delays are in the range of 25ms to 2secs. >> + */ >> + fsleep(delay); >> + } >> + } else >> + fsleep(delay); >> + >> + if (!retries) >> + return -EBUSY; >> + >> + ret = regmap_bulk_read(data->regmap, reg, buff, sizeof(buff)); >> + if (ret) { >> + dev_err(&data->client->dev, "read data failed\n"); >> + return ret; >> + } >> + >> + *val = get_unaligned_le24(&buff[0]); >> + >> + ret = apds9306_runtime_power(data, 0); >> + if (ret) >> + return ret; >> + >> + return ret; >> +} >> + Regards, Subhajit Ghosh
On Mon, 17 Apr 2023 17:25:50 +0800 Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote: > Thank you Jonathan for the review. > Answering only to the Big questions. > >> > >> Software reset feature is not implemented as it causes I2C bus error, > >> the I2C bus driver throws an ugly error message. > > > > That's unfortunate and perhaps something we should consider fixing > > at the i2c layer. Could you point to where it happens? > > > > We have a lot of drivers where reset causes an error (Ack missing normally > > due to simple state machines in the devices). > > Below function which cause the error: > regmap_field_write(data->reg_sw_reset, 1); > regmap_write(data->regmap, APDS9306_MAIN_CTRL, 0x10); > i2c_smbus_write_byte_data(data->client, APDS9306_MAIN_CTRL, 0x10); > > Error messages: > [ 3340.426180] stm32f7-i2c 40012000.i2c: <stm32f7_i2c_isr_error>: Bus error accessing addr 0x52 > [ 3340.433880] stm32f7-i2c 40012000.i2c: Trying to recover bus > > The function which gets called: > https://elixir.bootlin.com/linux/latest/source/drivers/i2c/busses/i2c-stm32f7.c#L1622 > > There is an errata associated with I2C for STM32MP157C (Section 2.19.2, Pg 35): > https://www.st.com/resource/en/errata_sheet/es0438-stm32mp151x3x7x-device-errata-stmicroelectronics.pdf > It speaks about - "Spurious bus error detection in master mode". But I > don't think it has got anything to do with our case. > > I use STM32MP157C-DK2 board as my reference device. > The Reference manual to the STM32MP157C: > https://www.st.com/resource/en/reference_manual/DM00327659.pdf > > stm32f7_i2c_isr_error() handler gets called because a control bit is set > ERRIE which enables interrupts from the I2C controller for Buss errors, > Arbitration losses, Overrun/Underrun PEC error, Timeout, etc. > > I am not sure about other chips. > A possible way to mitigate these kind of issues would be to pass a flag > from upper layers to the i2c bus driver (I2C_SMBUS_REPORT_ERR_OFF or > something on those lines). The drivers can then implement in > struct i2c_algorithm in smbus_xfer() function where they can check for > the flag and disable error checking. > > I don't have in depth knowledge on this subject so excuse my lack > of understanding. No problem and thanks for the details. So there is an existing flag: I2C_M_IGNORE_NAK but I'm not sure that's suitable and isn't supported by this particular bus driver. https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/msp3400-driver.c#L100 is an example of it being used in a reset routine. It might be sufficient to just check that flag in the stm32f7 i2c driver and not print an error message if it is set. +CC Wolfram for input on this. > > > > >> > >> Could not locate the Lux calculations from datasheet, only exporting > >> raw values. > > > > Ah. That's annoying as userspace is generally not able to do much with > > the raw values. Any other known code supporting this device that you > > can raid for info? > > > > If not, then this ist he best we can do. > > > This device is similar to LTRF216A. > If I use the calculation in ltrf216a then I would have to verify them > with Lux meter and controlled light source or ltrf216a. > This will be bit difficult for me at this moment. no problem. We can add that later if needed. > > >> > >> Reading of the Status register clears the Data Ready and the Interrupt > >> Status flags. It makes it tricky to read oneshot values together with > >> interrupts enabled as the IRQ handler clears the status on receipt > >> of an interrupt signal. > >> > >> Not checking the status in IRQ handler will make the interrupt line > >> unsharable and it does not reset the interrupt line if the Interrupt > >> status flag is not cleared. > > > > Definitely need to check it but I'm not sure I follow why you can't > > use it for both purposes with a slightly complex interrupt handler design. > > Maybe the code makes it clear what the issue is here. > Answers are in below comment. > > >> + > >> +static int apds9306_read_data(struct apds9306_data *data, int *val, int reg) > >> +{ > >> + struct device *dev = &data->client->dev; > >> + int ret, delay, status, int_en; > >> + int retries = 4; > >> + u8 buff[3]; > >> + > >> + ret = apds9306_runtime_power(data, 1); > >> + if (ret) > >> + return ret; > >> + > >> + /* > >> + * Whichever is greater - integration time period or > >> + * sampling period. > >> + */ > >> + delay = max(apds9306_intg_time[data->intg_time_idx][1], > >> + apds9306_repeat_rate_period[data->repeat_rate_idx]); > >> + > >> + /* > >> + * If interrupts are enabled then Status resistor cannot be > >> + * relied upon as all the status bits are cleared by the > >> + * interrupt handler in case of an event. > > > > Ah. I was assuming sane hardware (always an error :) that would issue > > an interrupt on the data being ready. I think we can make this work > > but it is ugly. Add some flags to the state structure. Then whenever > > you read this register, set whether the two status flags are set of not. > > Thus in the interrupt handler you can tell if this got there first and > > here you can tell if the interrupt handler got their first. > If I have a flag in the state structure for this, then a timestamp would > also be required or may be just a timestamp. True - though given we are dealing with a race here, I'd imagine any wrong timestamp issues will only make a small difference. > > > > > One messy corner. A status read resets the interrupt line, potentially before > > we saw the interrupt. Oh goody - normally this silliness only happens as > > a result of complex interrupt migration or errata. However it is understood > > what to do about it. > > > > If you see the interrupt status flag here, you have no way of knowing > > if the interrupt line was high for long enough that the interrupt controller > > saw it. As such your only option is to assume it didn't and inject an extra > > one. Given a passing of the threshold could in theory have been noisy enough > > to trigger two actual interrupts very close together userspace should be fine > > with the extra event - we probably just wasted some cycles doing something twice. > > > > The annoying bit will be testing as these races will be somewhat rare. > > As per my understanding the Status register has - "Observer effect" > > The Status register has got 3 bits - Power On Status, ALS Interrupt > and ALS Data. > A single read of the register clears all three bits. > > In case of an interrupt event, the interrupt line is held low (Active) by the > device till the Status register is read. As this is an edge triggered interrupt, > we stop receiving interrupts till we read the status register. > This is why it is good to acknowledge the interrupt by reading the Status register > in the ISR. > > Reading the Status register in the ISR inadvertently clears the "ALS Data" bit > as well. > > What you are asking is to have a read flag in the static struct and a timestamp > flag may be. In the isr, we check the flag and if the timestamp is within one > integration period, if yes, we don't read the Status register any more from the > isr. Not quite. Reading the ISR should be safe even if we already read it from the read_raw() path. The worst that happens is we report one event rather than two, but they will close enough together that won't matter. Potentially races may mean we signal 2 events, but that's fine too. Just looks like a bit of noise on the analog signal so shouldn't confuse software anyway. To ensure the read raw path doesn't wait for ever you'll also need to store a flag to say the data is good which is then used to detect we have wiped that bit before read_raw saw it. The read_raw path can clear that flag so we know it's fresh if the interrupt sets it later. > And in this function we read the Status register and update the read flag and > the timestamp with current time. If you see the bit that corresponds to the interrupt here, then you might never see the interrupt (it might be too short to rise properly). As such I think you'll need to push the event from the read_raw path. If the interrupt is seen then we may find no bits set and that will be fine. Need a bit of scribbling of timing diagrams to get all paths covered, but I'm fairly sure it is possible. Main challenge will be to keep it minimal and not over engineer the solution! > > Not sure if my understanding is correct but I will try the above. > > > > >> + */ > >> + ret = regmap_field_read(data->regfield_int_en, &int_en); > >> + if (ret) { > >> + dev_err(dev, "read interrupt status failed: %d\n", ret); > >> + return ret; > >> + } > >> + > >> + if (!int_en) { > >> + while (retries--) { > >> + ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS, > >> + &status); > >> + if (ret) { > >> + dev_err(dev, "read status failed: %d\n", ret); > >> + return ret; > >> + } > >> + if (status & APDS9306_ALS_DATA_STAT_MASK) > >> + break; > >> + /* > >> + * In case of continuous one-shot read from userspace, > >> + * new data is available after sampling period. > >> + * Delays are in the range of 25ms to 2secs. > >> + */ > >> + fsleep(delay); > >> + } > >> + } else > >> + fsleep(delay); > >> + > >> + if (!retries) > >> + return -EBUSY; > >> + > >> + ret = regmap_bulk_read(data->regmap, reg, buff, sizeof(buff)); > >> + if (ret) { > >> + dev_err(&data->client->dev, "read data failed\n"); > >> + return ret; > >> + } > >> + > >> + *val = get_unaligned_le24(&buff[0]); > >> + > >> + ret = apds9306_runtime_power(data, 0); > >> + if (ret) > >> + return ret; > >> + > >> + return ret; > >> +} > >> + > > Regards, > Subhajit Ghosh
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig index 0d4447df7200..8bd5aa13f49e 100644 --- a/drivers/iio/light/Kconfig +++ b/drivers/iio/light/Kconfig @@ -73,6 +73,17 @@ config APDS9300 To compile this driver as a module, choose M here: the module will be called apds9300. +config APDS9306 + tristate "Avago APDS9306 Ambient Light Sensor" + depends on I2C + select REGMAP_I2C + help + If you say Y or M here, you get support for Avago APDS9306 + Ambient Light Sensor. + + If built as a dynamically linked module, it will be called + apds9306. + config APDS9960 tristate "Avago APDS9960 gesture/RGB/ALS/proximity sensor" select REGMAP_I2C diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile index d74d2b5ff14c..978ec4b3de4f 100644 --- a/drivers/iio/light/Makefile +++ b/drivers/iio/light/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_ADUX1020) += adux1020.o obj-$(CONFIG_AL3010) += al3010.o obj-$(CONFIG_AL3320A) += al3320a.o obj-$(CONFIG_APDS9300) += apds9300.o +obj-$(CONFIG_APDS9306) += apds9306.o obj-$(CONFIG_APDS9960) += apds9960.o obj-$(CONFIG_AS73211) += as73211.o obj-$(CONFIG_BH1750) += bh1750.o diff --git a/drivers/iio/light/apds9306.c b/drivers/iio/light/apds9306.c new file mode 100644 index 000000000000..d57fb7aaa24d --- /dev/null +++ b/drivers/iio/light/apds9306.c @@ -0,0 +1,1146 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * APDS-9306/APDS-9306-065 Ambient Light Sensor + * + * Datasheet: https://docs.broadcom.com/doc/AV02-4755EN + * + * I2C Address: 0x52 + * + * Copyright (C) 2023 Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> + */ + +#include <linux/pm.h> +#include <linux/pm_runtime.h> +#include <linux/module.h> +#include <linux/i2c.h> +#include <linux/err.h> +#include <linux/mutex.h> +#include <linux/delay.h> +#include <linux/regmap.h> +#include <linux/interrupt.h> +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> +#include <linux/iio/events.h> +#include <linux/regulator/consumer.h> +#include <asm/unaligned.h> + +#define APDS9306_MAIN_CTRL 0x00 +#define APDS9306_ALS_MEAS_RATE 0x04 +#define APDS9306_ALS_GAIN 0x05 +#define APDS9306_PART_ID 0x06 +#define APDS9306_MAIN_STATUS 0x07 +#define APDS9306_CLEAR_DATA_0 0x0A +#define APDS9306_CLEAR_DATA_1 0x0B +#define APDS9306_CLEAR_DATA_2 0x0C +#define APDS9306_ALS_DATA_0 0x0D +#define APDS9306_ALS_DATA_1 0x0E +#define APDS9306_ALS_DATA_2 0x0F +#define APDS9306_INT_CFG 0x19 +#define APDS9306_INT_PERSISTENCE 0x1A +#define APDS9306_ALS_THRES_UP_0 0x21 +#define APDS9306_ALS_THRES_UP_1 0x22 +#define APDS9306_ALS_THRES_UP_2 0x23 +#define APDS9306_ALS_THRES_LOW_0 0x24 +#define APDS9306_ALS_THRES_LOW_1 0x25 +#define APDS9306_ALS_THRES_LOW_2 0x26 +#define APDS9306_ALS_THRES_VAR 0x27 + +#define APDS9306_ALS_INT_STAT_MASK BIT(4) +#define APDS9306_ALS_DATA_STAT_MASK BIT(3) + +enum apds9306_power_states { + standby, + active, +}; + +enum apds9306_int_channels { + clear, + als, +}; + +/* Integration Time (ALS Resolution) uSec */ +static const int apds9306_intg_time[][2] = { + {0, 400000}, + {0, 200000}, + {0, 100000}, + {0, 50000}, + {0, 25000}, + {0, 3125}, +}; + +/* Sampling Frequency in uHz */ +static const int apds9306_repeat_rate_freq[][2] = { + {40, 0}, + {20, 0}, + {10, 0}, + {5, 0}, + {2, 0}, + {1, 0}, + {0, 500000}, +}; + +/* Sampling Frequency in uSec */ +static const int apds9306_repeat_rate_period[] = { + 25000, 50000, 100000, 200000, 500000, 1000000, + 2000000 +}; + +/* ALS Gain Range (Scale) */ +static const int apds9306_gain[] = { + 1, 3, 6, 9, 18 +}; + +struct apds9306_data { + struct i2c_client *client; + struct iio_dev *indio_dev; + /* + * Protect single als reads and device + * power states. + */ + struct mutex mutex; + + struct regmap *regmap; + /* Reg: MAIN_CTRL, Field: SW_Reset */ + struct regmap_field *regfield_sw_reset; + /* Reg: MAIN_CTRL, Field: ALS_EN */ + struct regmap_field *regfield_en; + /* Reg: ALS_MEAS_RATE, Field: ALS Resolution/Bit Width */ + struct regmap_field *regfield_intg_time; + /* Reg: ALS_MEAS_RATE, Field: ALS Measurement Rate */ + struct regmap_field *regfield_repeat_rate; + /* Reg: ALS_GAIN, Field: ALS Gain Range */ + struct regmap_field *regfield_scale; + /* Reg: INT_CFG, Field: ALS Interrupt Source */ + struct regmap_field *regfield_int_src; + /* Reg: INT_CFG, Field: ALS Variation Interrupt Mode */ + struct regmap_field *regfield_int_thresh_var_en; + /* Reg: INT_CFG, Field: ALS Interrupt Enable */ + struct regmap_field *regfield_int_en; + /* Reg: INT_PERSISTENCE, Field: ALS_PERSIST */ + struct regmap_field *regfield_int_persist_val; + /* Reg: ALS_THRESH_VAR, Field: ALS_THRES_VAR */ + struct regmap_field *regfield_int_thresh_var_val; + + u8 intg_time_idx; + u8 repeat_rate_idx; + u8 gain_idx; + u8 int_ch; +}; + +static const struct regmap_range apds9306_readable_ranges[] = { + regmap_reg_range(APDS9306_MAIN_CTRL, APDS9306_ALS_THRES_VAR) +}; + +static const struct regmap_range apds9306_writable_ranges[] = { + regmap_reg_range(APDS9306_MAIN_CTRL, APDS9306_ALS_GAIN), + regmap_reg_range(APDS9306_INT_CFG, APDS9306_ALS_THRES_VAR) +}; + +static const struct regmap_range apds9306_volatile_ranges[] = { + regmap_reg_range(APDS9306_MAIN_STATUS, APDS9306_MAIN_STATUS), + regmap_reg_range(APDS9306_CLEAR_DATA_0, APDS9306_ALS_DATA_2) +}; + +static const struct regmap_range apds9306_precious_ranges[] = { + regmap_reg_range(APDS9306_MAIN_STATUS, APDS9306_MAIN_STATUS) +}; + +static const struct regmap_access_table apds9306_readable_table = { + .yes_ranges = apds9306_readable_ranges, + .n_yes_ranges = ARRAY_SIZE(apds9306_readable_ranges) +}; + +static const struct regmap_access_table apds9306_writable_table = { + .yes_ranges = apds9306_writable_ranges, + .n_yes_ranges = ARRAY_SIZE(apds9306_writable_ranges) +}; + +static const struct regmap_access_table apds9306_volatile_table = { + .yes_ranges = apds9306_volatile_ranges, + .n_yes_ranges = ARRAY_SIZE(apds9306_volatile_ranges) +}; + +static const struct regmap_access_table apds9306_precious_table = { + .yes_ranges = apds9306_precious_ranges, + .n_yes_ranges = ARRAY_SIZE(apds9306_precious_ranges) +}; + +static const struct regmap_config apds9306_regmap = { + .name = "apds9306_regmap", + .reg_bits = 8, + .val_bits = 8, + .rd_table = &apds9306_readable_table, + .wr_table = &apds9306_writable_table, + .volatile_table = &apds9306_volatile_table, + .precious_table = &apds9306_precious_table, + .max_register = APDS9306_ALS_THRES_VAR, + .cache_type = REGCACHE_RBTREE, +}; + +static const struct reg_field apds9306_regfield_sw_reset = + REG_FIELD(APDS9306_MAIN_CTRL, 4, 4); + +static const struct reg_field apds9306_regfield_en = + REG_FIELD(APDS9306_MAIN_CTRL, 1, 1); + +static const struct reg_field apds9306_regfield_intg_time = + REG_FIELD(APDS9306_ALS_MEAS_RATE, 4, 6); + +static const struct reg_field apds9306_regfield_repeat_rate = + REG_FIELD(APDS9306_ALS_MEAS_RATE, 0, 2); + +static const struct reg_field apds9306_regfield_scale = + REG_FIELD(APDS9306_ALS_GAIN, 0, 2); + +static const struct reg_field apds9306_regfield_int_src = + REG_FIELD(APDS9306_INT_CFG, 4, 5); + +static const struct reg_field apds9306_regfield_int_thresh_var_en = + REG_FIELD(APDS9306_INT_CFG, 3, 3); + +static const struct reg_field apds9306_regfield_int_en = + REG_FIELD(APDS9306_INT_CFG, 2, 2); + +static const struct reg_field apds9306_regfield_int_persist_val = + REG_FIELD(APDS9306_INT_PERSISTENCE, 4, 7); + +static const struct reg_field apds9306_regfield_int_thresh_var_val = + REG_FIELD(APDS9306_ALS_THRES_VAR, 0, 2); + +static struct iio_event_spec apds9306_event_spec[] = { + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_RISING, + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE), + }, + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_FALLING, + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE), + }, + { + .type = IIO_EV_TYPE_THRESH, + .mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD), + }, + { + .type = IIO_EV_TYPE_THRESH_ADAPTIVE, + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) | + BIT(IIO_EV_INFO_ENABLE), + }, + { + .type = IIO_EV_TYPE_THRESH, + .mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE), + }, +}; + +static struct iio_chan_spec apds9306_channels[] = { + { + .type = IIO_LIGHT, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), + .scan_type = { + .sign = 'u', + .realbits = 20, + .storagebits = 32, + }, + .event_spec = apds9306_event_spec, + .num_event_specs = ARRAY_SIZE(apds9306_event_spec), + }, + { + .type = IIO_INTENSITY, + .channel2 = IIO_MOD_LIGHT_CLEAR, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), + .scan_type = { + .sign = 'u', + .realbits = 20, + .storagebits = 32, + }, + .modified = 1, + }, +}; + +static struct iio_chan_spec apds9306_channels_no_events[] = { + { + .type = IIO_LIGHT, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), + .scan_type = { + .sign = 'u', + .realbits = 20, + .storagebits = 32, + }, + }, + { + .type = IIO_INTENSITY, + .channel2 = IIO_MOD_LIGHT_CLEAR, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), + .scan_type = { + .sign = 'u', + .realbits = 20, + .storagebits = 32, + }, + .modified = 1, + }, +}; + +static ssize_t thresh_channel_select_show(struct device *dev, + struct device_attribute *attr, char *buff) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct apds9306_data *data = iio_priv(indio_dev); + int ret; + + if (data->int_ch == 1) + ret = sprintf(buff, "als\n"); + else if (data->int_ch == 0) + ret = sprintf(buff, "clear\n"); + else + ret = -EINVAL; + + return ret; +} + +static ssize_t thresh_channel_select_store(struct device *dev, + struct device_attribute *attr, const char *buff, size_t len) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct apds9306_data *data = iio_priv(indio_dev); + int ret, channel; + + if (len <= 0 || len > 6) + return -EINVAL; + + if (!strncmp(buff, "als", 3)) + channel = 1; + else if (!strncmp(buff, "clear", 5)) + channel = 0; + else + return -EINVAL; + + ret = regmap_field_write(data->regfield_int_src, channel); + if (ret) + return ret; + + data->int_ch = channel; + + return len; +} + +/* INT_PERSISTENCE available */ +IIO_CONST_ATTR(thresh_either_period_available, "[0 15]"); + +/* ALS_THRESH_VAR available */ +IIO_CONST_ATTR(thresh_adaptive_either_values_available, "[0 7]"); + +/* Interrupt channel selection available */ +IIO_CONST_ATTR(thresh_channels_available, "clear als"); +IIO_DEVICE_ATTR_RW(thresh_channel_select, 0); + +static struct attribute *apds9306_event_attributes[] = { + &iio_const_attr_thresh_either_period_available.dev_attr.attr, + &iio_const_attr_thresh_channels_available.dev_attr.attr, + &iio_const_attr_thresh_adaptive_either_values_available.dev_attr.attr, + &iio_dev_attr_thresh_channel_select.dev_attr.attr, + NULL, +}; + +static const struct attribute_group apds9306_event_attr_group = { + .attrs = apds9306_event_attributes, +}; + +static int apds9306_regfield_init(struct apds9306_data *data) +{ + struct device *dev = &data->client->dev; + struct regmap *regmap = data->regmap; + + data->regfield_sw_reset = devm_regmap_field_alloc(dev, regmap, + apds9306_regfield_sw_reset); + if (IS_ERR(data->regfield_sw_reset)) + return PTR_ERR(data->regfield_sw_reset); + + data->regfield_en = devm_regmap_field_alloc(dev, regmap, + apds9306_regfield_en); + if (IS_ERR(data->regfield_en)) + return PTR_ERR(data->regfield_en); + + data->regfield_intg_time = devm_regmap_field_alloc(dev, regmap, + apds9306_regfield_intg_time); + if (IS_ERR(data->regfield_intg_time)) + return PTR_ERR(data->regfield_intg_time); + + data->regfield_repeat_rate = devm_regmap_field_alloc(dev, regmap, + apds9306_regfield_repeat_rate); + if (IS_ERR(data->regfield_repeat_rate)) + return PTR_ERR(data->regfield_repeat_rate); + + data->regfield_scale = devm_regmap_field_alloc(dev, regmap, + apds9306_regfield_scale); + if (IS_ERR(data->regfield_scale)) + return PTR_ERR(data->regfield_scale); + + data->regfield_int_src = devm_regmap_field_alloc(dev, regmap, + apds9306_regfield_int_src); + if (IS_ERR(data->regfield_int_src)) + return PTR_ERR(data->regfield_int_src); + + data->regfield_int_thresh_var_en = devm_regmap_field_alloc(dev, regmap, + apds9306_regfield_int_thresh_var_en); + if (IS_ERR(data->regfield_int_thresh_var_en)) + return PTR_ERR(data->regfield_int_thresh_var_en); + + data->regfield_int_en = devm_regmap_field_alloc(dev, regmap, + apds9306_regfield_int_en); + if (IS_ERR(data->regfield_int_en)) + return PTR_ERR(data->regfield_int_en); + + data->regfield_int_persist_val = devm_regmap_field_alloc(dev, regmap, + apds9306_regfield_int_persist_val); + if (IS_ERR(data->regfield_int_persist_val)) + return PTR_ERR(data->regfield_int_en); + + data->regfield_int_thresh_var_val = devm_regmap_field_alloc(dev, regmap, + apds9306_regfield_int_thresh_var_val); + if (IS_ERR(data->regfield_int_thresh_var_val)) + return PTR_ERR(data->regfield_int_thresh_var_en); + + return 0; +} + +static int apds9306_power_state(struct apds9306_data *data, + enum apds9306_power_states state) +{ + int ret; + + /* Reset not included as it causes ugly I2C bus error */ + switch (state) { + case standby: + return regmap_field_write(data->regfield_en, 0); + case active: + ret = regmap_field_write(data->regfield_en, 1); + if (ret) + return ret; + /* 5ms wake up time */ + usleep_range(5000, 10000); + break; + default: + return -EINVAL; + } + return 0; +} + +static int apds9306_runtime_power(struct apds9306_data *data, int en) +{ + struct device *dev = &data->client->dev; + int ret = 0; + + if (en) { + ret = pm_runtime_resume_and_get(dev); + if (ret < 0) { + dev_err(dev, "runtime resume failed: %d\n", ret); + return ret; + } + ret = 0; + } else { + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); + } + + return ret; +} + +static int apds9306_read_data(struct apds9306_data *data, int *val, int reg) +{ + struct device *dev = &data->client->dev; + int ret, delay, status, int_en; + int retries = 4; + u8 buff[3]; + + ret = apds9306_runtime_power(data, 1); + if (ret) + return ret; + + /* + * Whichever is greater - integration time period or + * sampling period. + */ + delay = max(apds9306_intg_time[data->intg_time_idx][1], + apds9306_repeat_rate_period[data->repeat_rate_idx]); + + /* + * If interrupts are enabled then Status resistor cannot be + * relied upon as all the status bits are cleared by the + * interrupt handler in case of an event. + */ + ret = regmap_field_read(data->regfield_int_en, &int_en); + if (ret) { + dev_err(dev, "read interrupt status failed: %d\n", ret); + return ret; + } + + if (!int_en) { + while (retries--) { + ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS, + &status); + if (ret) { + dev_err(dev, "read status failed: %d\n", ret); + return ret; + } + if (status & APDS9306_ALS_DATA_STAT_MASK) + break; + /* + * In case of continuous one-shot read from userspace, + * new data is available after sampling period. + * Delays are in the range of 25ms to 2secs. + */ + fsleep(delay); + } + } else + fsleep(delay); + + if (!retries) + return -EBUSY; + + ret = regmap_bulk_read(data->regmap, reg, buff, sizeof(buff)); + if (ret) { + dev_err(&data->client->dev, "read data failed\n"); + return ret; + } + + *val = get_unaligned_le24(&buff[0]); + + ret = apds9306_runtime_power(data, 0); + if (ret) + return ret; + + return ret; +} + +static int apds9306_intg_time_get(struct apds9306_data *data, int *val, + int *val2) +{ + if (data->intg_time_idx > ARRAY_SIZE(apds9306_intg_time)) + return -EINVAL; + + *val = apds9306_intg_time[data->intg_time_idx][0]; + *val2 = apds9306_intg_time[data->intg_time_idx][1]; + + return 0; +} + +static int apds9306_intg_time_set(struct apds9306_data *data, int val, + int val2) +{ + int i, ret = -EINVAL; + + for (i = 0; i < ARRAY_SIZE(apds9306_intg_time); i++) + if (apds9306_intg_time[i][0] == val && + apds9306_intg_time[i][1] == val2) { + ret = regmap_field_write(data->regfield_intg_time, i); + if (ret) + return ret; + data->intg_time_idx = i; + break; + } + + return ret; +} + +static int apds9306_sampling_freq_get(struct apds9306_data *data, int *val, + int *val2) +{ + if (data->repeat_rate_idx > ARRAY_SIZE(apds9306_repeat_rate_freq)) + return -EINVAL; + + *val = apds9306_repeat_rate_freq[data->repeat_rate_idx][0]; + *val2 = apds9306_repeat_rate_freq[data->repeat_rate_idx][1]; + + return 0; +} + +static int apds9306_sampling_freq_set(struct apds9306_data *data, int val, + int val2) +{ + int i, ret = -EINVAL; + + for (i = 0; i < ARRAY_SIZE(apds9306_repeat_rate_freq); i++) + if (apds9306_repeat_rate_freq[i][0] == val && + apds9306_repeat_rate_freq[i][1] == val2) { + ret = regmap_field_write(data->regfield_repeat_rate, i); + if (ret) + return ret; + data->repeat_rate_idx = i; + break; + } + + return ret; +} + +static int apds9306_gain_get(struct apds9306_data *data, int *val) +{ + if (data->gain_idx > ARRAY_SIZE(apds9306_gain)) + return -EINVAL; + + *val = apds9306_gain[data->gain_idx]; + + return 0; +} + +static int apds9306_gain_set(struct apds9306_data *data, int val) +{ + int i, ret; + + for (i = 0; i < ARRAY_SIZE(apds9306_gain); i++) + if (apds9306_gain[i] == val) { + ret = regmap_field_write(data->regfield_scale, i); + if (ret) + return ret; + data->gain_idx = i; + } + + return 0; +} + +static int apds9306_event_period_get(struct apds9306_data *data, int *val) +{ + int period, ret; + + ret = regmap_field_read(data->regfield_int_persist_val, &period); + if (ret) + return ret; + + if (period > 15) + return -EINVAL; + + *val = period; + + return ret; +} + +static int apds9306_event_period_set(struct apds9306_data *data, int val) +{ + if (val < 0 || val > 15) + return -EINVAL; + + return regmap_field_write(data->regfield_int_persist_val, val); +} + +static int apds9306_event_thresh_adaptive_get(struct apds9306_data *data, + int *val) +{ + int thad, ret; + + ret = regmap_field_read(data->regfield_int_thresh_var_val, &thad); + if (ret) + return ret; + + if (thad > 7) + return -EINVAL; + + *val = thad; + + return ret; +} + +static int apds9306_event_thresh_adaptive_set(struct apds9306_data *data, + int val) +{ + if (val < 0 || val > 7) + return -EINVAL; + + return regmap_field_write(data->regfield_int_thresh_var_val, val); +} + +static int apds9306_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct apds9306_data *data = iio_priv(indio_dev); + int ret, reg; + + mutex_lock(&data->mutex); + switch (mask) { + case IIO_CHAN_INFO_RAW: + if (chan->channel2 == IIO_MOD_LIGHT_CLEAR) + reg = APDS9306_CLEAR_DATA_0; + else + reg = APDS9306_ALS_DATA_0; + ret = apds9306_read_data(data, val, reg); + if (!ret) + ret = IIO_VAL_INT; + *val2 = 0; + break; + case IIO_CHAN_INFO_INT_TIME: + ret = apds9306_intg_time_get(data, val, val2); + if (!ret) + ret = IIO_VAL_INT_PLUS_MICRO; + break; + case IIO_CHAN_INFO_SAMP_FREQ: + ret = apds9306_sampling_freq_get(data, val, val2); + if (!ret) + ret = IIO_VAL_INT_PLUS_MICRO; + break; + case IIO_CHAN_INFO_SCALE: + ret = apds9306_gain_get(data, val); + if (!ret) + ret = IIO_VAL_INT; + *val2 = 0; + break; + default: + ret = -EINVAL; + } + mutex_unlock(&data->mutex); + + return ret; +}; + +static int apds9306_read_avail(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, const int **vals, + int *type, int *length, long mask) +{ + switch (mask) { + case IIO_CHAN_INFO_INT_TIME: + *length = ARRAY_SIZE(apds9306_intg_time) * 2; + *vals = (const int *)apds9306_intg_time; + *type = IIO_VAL_INT_PLUS_MICRO; + return IIO_AVAIL_LIST; + case IIO_CHAN_INFO_SAMP_FREQ: + *length = ARRAY_SIZE(apds9306_repeat_rate_freq) * 2; + *vals = (const int *)apds9306_repeat_rate_freq; + *type = IIO_VAL_INT_PLUS_MICRO; + return IIO_AVAIL_LIST; + case IIO_CHAN_INFO_SCALE: + *length = ARRAY_SIZE(apds9306_gain); + *vals = apds9306_gain; + *type = IIO_VAL_INT; + return IIO_AVAIL_LIST; + default: + return -EINVAL; + } + + return 0; +} + +static int apds9306_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int val, int val2, long mask) +{ + struct apds9306_data *data = iio_priv(indio_dev); + int ret = -EINVAL; + + mutex_lock(&data->mutex); + switch (mask) { + case IIO_CHAN_INFO_INT_TIME: + ret = apds9306_intg_time_set(data, val, val2); + break; + case IIO_CHAN_INFO_SAMP_FREQ: + ret = apds9306_sampling_freq_set(data, val, val2); + break; + case IIO_CHAN_INFO_SCALE: + if (val2 != 0) + break; + ret = apds9306_gain_set(data, val); + break; + default: + ret = -EINVAL; + } + mutex_unlock(&data->mutex); + + return ret; +} + +static const struct iio_info apds9306_info_no_events = { + .read_raw = apds9306_read_raw, + .read_avail = apds9306_read_avail, + .write_raw = apds9306_write_raw, +}; + +static irqreturn_t apds9306_irq_handler(int irq, void *priv) +{ + struct iio_dev *indio_dev = priv; + struct apds9306_data *data = iio_priv(indio_dev); + int ret, status; + + /* The interrupt line is released and the interrupt flag is + * cleared as a result of reading the status register + */ + ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS, &status); + if (ret < 0) { + dev_err(&data->client->dev, "status reg read failed\n"); + return IRQ_HANDLED; + } + + if ((status & APDS9306_ALS_INT_STAT_MASK)) { + iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_LIGHT, + data->int_ch, IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER), + iio_get_time_ns(indio_dev)); + } + + return IRQ_HANDLED; +} + +static int apds9306_read_event(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info, + int *val, int *val2) +{ + struct apds9306_data *data = iio_priv(indio_dev); + u8 buff[3]; + int var, ret; + + switch (type) { + case IIO_EV_TYPE_THRESH: + if (dir == IIO_EV_DIR_EITHER && info == IIO_EV_INFO_PERIOD) { + ret = apds9306_event_period_get(data, val); + if (ret) + return ret; + break; + } + + if (dir == IIO_EV_DIR_RISING) + var = APDS9306_ALS_THRES_UP_0; + else if (dir == IIO_EV_DIR_FALLING) + var = APDS9306_ALS_THRES_LOW_0; + else + return -EINVAL; + + ret = regmap_bulk_read(data->regmap, var, buff, sizeof(buff)); + if (ret) + return ret; + *val = get_unaligned_le24(&buff[0]); + break; + case IIO_EV_TYPE_THRESH_ADAPTIVE: + ret = apds9306_event_thresh_adaptive_get(data, val); + if (ret) + return ret; + break; + default: + return -EINVAL; + } + + *val2 = 0; + return IIO_VAL_INT; +} + +static int apds9306_write_event(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info, + int val, int val2) +{ + struct apds9306_data *data = iio_priv(indio_dev); + unsigned int var; + u8 buff[3]; + + switch (type) { + case IIO_EV_TYPE_THRESH: + if (dir == IIO_EV_DIR_EITHER && info == IIO_EV_INFO_PERIOD) + return apds9306_event_period_set(data, val); + if (dir == IIO_EV_DIR_RISING) + var = APDS9306_ALS_THRES_UP_0; + else if (dir == IIO_EV_DIR_FALLING) + var = APDS9306_ALS_THRES_LOW_0; + else + return -EINVAL; + + if (val < 0 || val > 0xFFFFF) + return -EINVAL; + + put_unaligned_le24(val, buff); + return regmap_bulk_write(data->regmap, var, buff, sizeof(buff)); + case IIO_EV_TYPE_THRESH_ADAPTIVE: + return apds9306_event_thresh_adaptive_set(data, val); + default: + return -EINVAL; + } + + return 0; +} + +static int apds9306_read_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir) +{ + struct apds9306_data *data = iio_priv(indio_dev); + unsigned int val; + int ret; + + switch (type) { + case IIO_EV_TYPE_THRESH: + ret = regmap_field_read(data->regfield_int_en, &val); + if (ret) + return ret; + return val; + case IIO_EV_TYPE_THRESH_ADAPTIVE: + ret = regmap_field_read(data->regfield_int_thresh_var_en, &val); + if (ret) + return ret; + return val; + default: + return -EINVAL; + } + + return 0; +} + +static int apds9306_write_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + int state) +{ + struct apds9306_data *data = iio_priv(indio_dev); + int ret, curr_state; + + state = !!state; + switch (type) { + case IIO_EV_TYPE_THRESH: + ret = regmap_field_read(data->regfield_int_en, &curr_state); + if (ret) + return ret; + if (curr_state == state) + return 0; + ret = regmap_field_write(data->regfield_int_en, state); + if (ret) + return ret; + mutex_lock(&data->mutex); + ret = apds9306_runtime_power(data, state); + mutex_unlock(&data->mutex); + if (ret) + return ret; + break; + case IIO_EV_TYPE_THRESH_ADAPTIVE: + ret = regmap_field_write(data->regfield_int_thresh_var_en, state); + if (ret) + return ret; + break; + default: + ret = -EINVAL; + } + + return ret; +} + +static const struct iio_info apds9306_info = { + .event_attrs = &apds9306_event_attr_group, + .read_raw = apds9306_read_raw, + .read_avail = apds9306_read_avail, + .write_raw = apds9306_write_raw, + .read_event_value = apds9306_read_event, + .write_event_value = apds9306_write_event, + .read_event_config = apds9306_read_event_config, + .write_event_config = apds9306_write_event_config, +}; + +static void apds9306_powerdown(void *ptr) +{ + struct apds9306_data *data = (struct apds9306_data *)ptr; + int status; + + /* Disable interrupts */ + regmap_field_write(data->regfield_int_thresh_var_en, 0); + regmap_field_write(data->regfield_int_en, 0); + /* Clear status */ + regmap_read(data->regmap, APDS9306_MAIN_STATUS, &status); + /* Put the device in standby mode */ + apds9306_power_state(data, standby); +} + +static int apds9306_init_device(struct apds9306_data *data) +{ + struct device *dev = &data->client->dev; + int ret; + + ret = apds9306_power_state(data, active); + if (ret) + return ret; + + ret = pm_runtime_set_active(dev); + if (ret) + return ret; + + ret = devm_pm_runtime_enable(dev); + if (ret) + return ret; + + pm_runtime_set_autosuspend_delay(dev, 5000); + pm_runtime_use_autosuspend(dev); + + ret = regmap_reinit_cache(data->regmap, &apds9306_regmap); + if (ret) + return ret; + + /* Integration time: 100 msec */ + ret = apds9306_intg_time_set(data, 0, 100000); + if (ret) + return ret; + + /* Sampling frequency: 100 msec, 10 Hz */ + ret = apds9306_sampling_freq_set(data, 10, 0); + if (ret) + return ret; + + /* Scale: x3 */ + ret = apds9306_gain_set(data, 3); + if (ret) + return ret; + + /* Interrupt source channel: als */ + ret = regmap_field_write(data->regfield_int_src, 1); + if (ret) + return ret; + data->int_ch = 1; + + /* Interrupts disabled */ + ret = regmap_field_write(data->regfield_int_en, 0); + if (ret) + return ret; + + /* Threshold Variance disabled */ + ret = regmap_field_write(data->regfield_int_thresh_var_en, 0); + if (ret) + return ret; + + return 0; +} + +static int apds9306_probe(struct i2c_client *client) +{ + struct apds9306_data *data; + struct iio_dev *indio_dev; + int ret; + + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); + if (!indio_dev) + return -ENOMEM; + + data = iio_priv(indio_dev); + data->indio_dev = indio_dev; + + mutex_init(&data->mutex); + + data->regmap = devm_regmap_init_i2c(client, &apds9306_regmap); + if (IS_ERR(data->regmap)) + return dev_err_probe(&client->dev, PTR_ERR(data->regmap), + "regmap initialization failed\n"); + + i2c_set_clientdata(client, indio_dev); + data->client = client; + + ret = apds9306_regfield_init(data); + if (ret) + return dev_err_probe(&client->dev, ret, + "regfield initialization failed\n"); + + ret = devm_regulator_get_enable(&client->dev, "vin"); + if (ret) + return dev_err_probe(&client->dev, ret, + "Failed to enable regulator\n"); + + indio_dev->name = "apds9306"; + indio_dev->modes = INDIO_DIRECT_MODE; + if (client->irq) { + indio_dev->info = &apds9306_info; + indio_dev->channels = apds9306_channels; + indio_dev->num_channels = ARRAY_SIZE(apds9306_channels); + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, + apds9306_irq_handler, IRQF_TRIGGER_FALLING | + IRQF_ONESHOT, "apds9306_event", indio_dev); + if (ret) + return dev_err_probe(&client->dev, ret, + "failed to assign interrupt.\n"); + } else { + indio_dev->info = &apds9306_info_no_events; + indio_dev->channels = apds9306_channels_no_events; + indio_dev->num_channels = ARRAY_SIZE(apds9306_channels_no_events); + } + + ret = devm_add_action_or_reset(&client->dev, apds9306_powerdown, data); + if (ret) + return dev_err_probe(&client->dev, ret, + "failed to add action on driver unwind\n"); + + ret = apds9306_init_device(data); + if (ret) + return dev_err_probe(&client->dev, ret, "failed to init device\n"); + + return devm_iio_device_register(&client->dev, indio_dev); +} + +static int apds9306_runtime_suspend(struct device *dev) +{ + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); + struct apds9306_data *data = iio_priv(indio_dev); + int ret; + + ret = apds9306_power_state(data, standby); + if (ret) + regcache_cache_only(data->regmap, true); + + return ret; +} + +static int apds9306_runtime_resume(struct device *dev) +{ + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev)); + struct apds9306_data *data = iio_priv(indio_dev); + int ret; + + regcache_cache_only(data->regmap, false); + ret = regcache_sync(data->regmap); + if (!ret) { + ret = apds9306_power_state(data, active); + if (ret) + regcache_cache_only(data->regmap, true); + } + + return ret; +} + +static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops, apds9306_runtime_suspend, + apds9306_runtime_resume, NULL); + +static const struct i2c_device_id apds9306_id[] = { + { "apds9306" }, + {} +}; +MODULE_DEVICE_TABLE(i2c, apds9306_id); + +static const struct of_device_id apds9306_of_match[] = { + { .compatible = "avago,apds9306" }, + {} +}; +MODULE_DEVICE_TABLE(of, apds9306_of_match); + +static struct i2c_driver apds9306_driver = { + .driver = { + .name = "apds9306", + .pm = pm_ptr(&apds9306_pm_ops), + .of_match_table = apds9306_of_match, + }, + .probe_new = apds9306_probe, + .id_table = apds9306_id, +}; + +module_i2c_driver(apds9306_driver); + +MODULE_AUTHOR("Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>"); +MODULE_DESCRIPTION("APDS9306 Ambient Light Sensor driver"); +MODULE_LICENSE("GPL");
Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor with als and clear channels. This driver exposes raw values for both the channels. Datasheet at https://docs.broadcom.com/doc/AV02-4755EN Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> --- Software reset feature is not implemented as it causes I2C bus error, the I2C bus driver throws an ugly error message. Could not locate the Lux calculations from datasheet, only exporting raw values. Reading of the Status register clears the Data Ready and the Interrupt Status flags. It makes it tricky to read oneshot values together with interrupts enabled as the IRQ handler clears the status on receipt of an interrupt signal. Not checking the status in IRQ handler will make the interrupt line unsharable and it does not reset the interrupt line if the Interrupt status flag is not cleared. Many applications don't need interrupts. It is good to have support for both in the driver. Sysfs structure with interrupt line defined in DT: root@stm32mp1:~# tree -I 'dev|name|of_node|power|subsystem|uevent' \ /sys/bus/iio/devices/iio:device0/ /sys/bus/iio/devices/iio:device0/ |-- events | |-- thresh_adaptive_either_en | |-- thresh_adaptive_either_value | |-- thresh_adaptive_either_values_available | |-- thresh_channel_select | |-- thresh_channels_available | |-- thresh_either_en | |-- thresh_either_period | |-- thresh_either_period_available | |-- thresh_falling_value | `-- thresh_rising_value |-- in_illuminance_raw |-- in_intensity_clear_raw |-- integration_time |-- integration_time_available |-- sampling_frequency |-- sampling_frequency_available |-- scale `-- scale_available Sysfs structure with no interrupt: root@stm32mp1:~# tree -I 'dev|name|of_node|power|subsystem|uevent' \ /sys/bus/iio/devices/iio:device0/ /sys/bus/iio/devices/iio:device0/ |-- in_illuminance_raw |-- in_intensity_clear_raw |-- integration_time |-- integration_time_available |-- sampling_frequency |-- sampling_frequency_available |-- scale `-- scale_available As there is a single interrupt enable option, using the existing IIO bitmasks to cater for channel selection - als or clear and interrupt type selection - threshold or adaptive was looking ambiguous. I am open to any other implementations. I could not find any _available implementations for the event interface. Created one sysfs attribute for interrupt channel selection and one RO attribute for channel availability. Created two RO attributes for interrupt period and threshold adaptive value available ranges. The ALS Measurement Rate (Sampling Frequency) has max values of 2000ms and 2000ms for both 110b and 111b respectively. This is not a typo. Found out experimentally that assigning either values, interrupt is generated 2 secs apart when interrupt conditions are met. drivers/iio/light/Kconfig | 11 + drivers/iio/light/Makefile | 1 + drivers/iio/light/apds9306.c | 1146 ++++++++++++++++++++++++++++++++++ 3 files changed, 1158 insertions(+) create mode 100644 drivers/iio/light/apds9306.c