Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 12 Apr 2023 16:37:56 +0300
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Shreeya Patel <shreeya.patel@collabora.com>,
        Paul Gazzillo <paul@pgazz.com>,
        Zhigang Shi <Zhigang.Shi@liteon.com>,
        Dmitry Osipenko <dmitry.osipenko@collabora.com>
Subject: Re: [RFC PATCH 2/2] iio: light: Add support for APDS9306 Light Sensor
Message-ID: <ZDa0NIot/4aRJ0pI@smile.fi.intel.com>
References: <20230411011203.5013-1-subhajit.ghosh@tweaklogic.com>
 <20230411011203.5013-3-subhajit.ghosh@tweaklogic.com>
 <ZDVWB9xV9Cdbwyqd@smile.fi.intel.com>
 <ab1d9746-4d23-efcc-0ee1-d2b8c634becd@tweaklogic.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ab1d9746-4d23-efcc-0ee1-d2b8c634becd@tweaklogic.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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.

-- 
With Best Regards,
Andy Shevchenko