Re: [RESEND] rtc: hym8563: try multiple times to init device

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

 



On 2022-08-21 13:26, Frank Wunderlich wrote:
From: Peter Geis <pgwipeout@xxxxxxxxx>

RTC sometimes does not respond the first time in init.
Try multiple times to get a response.

Signed-off-by: Peter Geis <pgwipeout@xxxxxxxxx>
Signed-off-by: Frank Wunderlich <frank-w@xxxxxxxxxxxxxxx>
---
discussion from v1
https://patchwork.kernel.org/project/linux-rockchip/patch/20220608161150.58919-2-linux@xxxxxxxxx/

On Fri, Jul 8, 2022 at 12:18 PM Robin Murphy <robin.murphy@xxxxxxx> wrote:
FWIW, given that HYM8563 is fairly common on RK3288 boards - I can't say
I've ever noticed an issue with mine, for instance - it seems dubious
that this would be a general issue of the chip itself. Are you sure it's
not a SoC or board-level issue with the I2C bus being in a funny initial
state, timings being marginal, or suchlike?

Peter Geis <pgwipeout@xxxxxxxxx>:
I don't think this is an SoC issue since this is the first instance
I've encountered it. Mind you we don't have the reset lines hooked up
at all for the Rockchip i2c driver, so it's possible that's the case,
but I'd imagine it would be observed more broadly if that was the
case. I've tried pushing the timings out pretty far as well as bumping
up the drive strength to no change. It seems to occur only with the
hym rtc used on this board. I suspect it's a new variant of the hym
that has slightly different behavior.

Sure, if it's documented somewhere that Hayou (or if the BPI-R2 Pro schematic is to be believed, AnalogTek) decided to innovate a new "sometimes doesn't work" feature for a chip that's been in production for a decade or more, and that 2 retries at 20ms intervals is what's recommended, then I'm open to believing that this isn't a complete hack. Or at least if someone can say they've scoped the pins and confirmed that nothing looks suspect at the protocol level when this happens that could explain it.

Otherwise, I'll remain unconvinced that it isn't a coincidence that this has shown up while bringing up a new board with a new SoC, and hacking a mature common driver to bodge around an issue that isn't fully understood, and could very conceivably lie elsewhere, is not the right answer. Especially when it involves a board vendor... let's say, whose reputation proceeds them.

Since I'm not above wasting 20 minutes of my time to prove a point, for starters the schematic seems to imply that it's using a variant of RK809 where LDO4, used as the I/O supply for i2c3, is off by default, so on the face of it it could be something as stupidly simple as the RTC probe racing with the PMIC or I/O domain probe. Sure, the DT claims it's already on at boot, but *is* it? Maybe that was true with some downstream bootloader, but do we know that's what you're using to boot mainline? Maybe this something so obvious that you've already confirmed and taken it for granted, but the patch as presented doesn't give me the confidence to rule *anything* out.

Thanks,
Robin.

---
  drivers/rtc/rtc-hym8563.c | 11 +++++++++--
  1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-hym8563.c b/drivers/rtc/rtc-hym8563.c
index cc710d682121..d9d0b6615a07 100644
--- a/drivers/rtc/rtc-hym8563.c
+++ b/drivers/rtc/rtc-hym8563.c
@@ -13,6 +13,7 @@
  #include <linux/clk-provider.h>
  #include <linux/i2c.h>
  #include <linux/bcd.h>
+#include <linux/delay.h>
  #include <linux/rtc.h>
#define HYM8563_CTL1 0x00
@@ -438,10 +439,16 @@ static irqreturn_t hym8563_irq(int irq, void *dev_id)
static int hym8563_init_device(struct i2c_client *client)
  {
-	int ret;
+	int ret, i;
/* Clear stop flag if present */
-	ret = i2c_smbus_write_byte_data(client, HYM8563_CTL1, 0);
+	for (i = 0; i < 3; i++) {
+		ret = i2c_smbus_write_byte_data(client, HYM8563_CTL1, 0);
+		if (ret == 0)
+			break;
+		msleep(20);
+	}
+
  	if (ret < 0)
  		return ret;



[Index of Archives]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux