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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux