Re: [PATCH v5 3/3] media: i2c: isl7998x: Add driver for Intersil ISL7998x

[Michael Tretter <m.tretter@pengutronix.de>]

On Thu, 14 Oct 2021 11:45:25 +0100, Ian Arkver wrote:
> On 14/10/2021 11:17, Philipp Zabel wrote:
> > On Thu, 2021-10-14 at 11:34 +0200, Michael Tretter wrote:
> > > On Wed, 13 Oct 2021 10:05:30 +0200, Michael Tretter wrote:
> > > > On Tue, 12 Oct 2021 15:27:11 +0200, Philipp Zabel wrote:
> > [...]
> > > > > > +static int isl7998x_wait_power_on(struct isl7998x *isl7998x)
> > > > > > +{
> > > > > > +	struct device *dev = isl7998x->subdev.dev;
> > > > > > +	unsigned int retry;
> > > > > > +	u32 chip_id;
> > > > > > +	int ret = -ETIMEDOUT;
> > > > > > +
> > > > > > +	for (retry = 10; ret && retry > 0; retry--) {
> > > > > > +		ret = regmap_read(isl7998x->regmap,
> > > > > > +				  ISL7998x_REG_P0_PRODUCT_ID_CODE, &chip_id);
> > > > > > +		usleep_range(1000, 2000);
> > > > > > +	}
> > > > > 
> > > > > Consider using regmap_read_poll_timeout() here.
> > > > 
> > > > Ack. I forgot about this function.
> > > 
> > > regmap_read_poll_timeout() cannot be used here, because it returns if
> > > regmap_read() returns an error. The driver uses the return value of
> > > regmap_read() to detect, if the chip is available, and should continue polling
> > > if regmap_read() failed. I can implement the necessary behavior with
> > > read_poll_timeout(), but am not sure if it is really worth it.
> > 
> > Oh, right. I still think so, but your call.
> > 
> This wait_power_on function seems odd to me. Wouldn't it be better to just
> wait for the power-on delay specified in the datasheet and then
> unconditionally go into isl7998x_init? If the device has failed to come up,
> that init will fail in its regmap accesses. If you're trying to do the init
> earlier than the datasheet specified time then being able to read the
> product id code doesn't guarantee the rest of the chip is ready. If there's
> no datasheet specification maybe just wait 10ms to 20ms?

The datasheet does not specify the power-on delay. The wait_power_on() enables
the driver to at least tell the user that the chip was not detected and print
the exact chip variant if it was detected. I think this is still better than
waiting some arbitrary time and hoping for the best.

Michael