Re: [RFC PATCH 2/2] iio: light: Add support for APDS9306 Light Sensor

Subhajit Ghosh
2023-04-13T03:34:48Z

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