Messages in this thread | | | Date | Thu, 13 Apr 2023 11:33:57 +0800 | Subject | Re: [RFC PATCH 2/2] iio: light: Add support for APDS9306 Light Sensor | From | Subhajit Ghosh <> |
| |
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
| |