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

[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