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

[Ian Arkver <ian.arkver.dev@gmail.com>]

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.
> 
> regards
> Philipp
> 

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?

Regards,
Ian