On 14/10/2021 12:04, Michael Tretter wrote:
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.
Thanks Michael, I can see how the prints could be useful and I'd
neglected them before. I see now this is the only place the device ID is
read and reported.
Regards,
Ian
