Re: DS3231 v. MAX77620

On Sun, 2021-11-07 at 12:30 +1100, Brian Scott wrote:
> Hi All,
> 
> I just plugged a DS3231 (RTC) into a RPi4 running 13.0 Release. Not 
> strictly necessary but I had one on my desk and it's a weekend. Added
> an 
> appropriate dtb overlay and loaded the ds3231.ko via loader.conf.
> Done 
> this a few times before on other boards and not expecting any drama.
> 
> Instead of showing up during boot as a DS3231, it appears to be
> probed 
> as a MAX77620 (which fails) and  leaves the real device unavailable
> to 
> the ds3231 driver.
> 
> 
> Nov  5 17:23:00 427269616e-60 kernel: iic0: <I2C generic I/O> on
> iicbus0
> Nov  5 17:23:00 427269616e-60 kernel: rtc0: <MAX77620 RTC> at addr
> 0xd0 
> on iicbus0
> 
> Nov  5 17:23:00 427269616e-60 kernel: rtc0: Error when reading reg
> 0x02, 
> rv: 35
> Nov  5 17:23:00 427269616e-60 kernel: rtc0: Failed to configure RTC
> Nov  5 17:23:00 427269616e-60 kernel: device_attach: rtc0 attach
> returned 5
> 
> After some investigation I have found that all I need to provoke
> these 
> messages is the dtb overlay loaded. Exactly the same messages are 
> generated without the ds3231.ko module and even when no physical
> device 
> is present.
> 
> Looking at max77620_rtc_probe in 
> sys/arm64/nvidia/tegra210/max77620_rtc.c shows:
> 
> static int
> max77620_rtc_probe(device_t dev)
> {
> struct iicbus_ivar *dinfo;
> dinfo = device_get_ivars(dev);
> if (dinfo == NULL)
> return (ENXIO);
> if (dinfo->addr != MAX77620_RTC_I2C_ADDR << 1)
> return (ENXIO);
> device_set_desc(dev, "MAX77620 RTC");
> return (BUS_PROBE_DEFAULT);
> }
> 
> This device will attempt to attach to anything with address == 
> MAX77620_RTC_I2C_ADDR (0x68) that is found in the device tree.
> However, 
> https://learn.adafruit.com/i2c-addresses/the-list lists:
> 
> 0x68 This address is really popular with real time clocks, almost all
> of 
> them use 0x68! AMG8833 IR Thermal Camera Breakout (0x68 or 0x69)
> DS1307 
> RTC (0x68 only) DS3231 RTC (0x68 only) ICM-20649 Accel+Gyro (0x68 or 
> 0x69) ITG3200 Gyro (0x68 or 0x69) MPU-9250 9-DoF IMU (0x68 or 0x69) 
> MPU-60X0 Accel+Gyro (0x68 or 0x69) PCF8523 RTC (0x68 only)
> 
> A seven bit device address is clearly not enough to uniquely identify
> a 
> type of device and so shouldn't be used like this in the driver.
> 
> Either the driver should use ofw_bus_search_compatible (although I 
> believe there is no entry for the rtc in the linux device tree) or at
> least made conditional on the parent device (the MAX77620) being in
> the 
> device tree.
> 
> As I said earlier, this doesn't matter a huge amount for me at this 
> stage because in my current application time will be configured by
> ntp. 
> In the future it will matter more. While it may well be that the
> target 
> device cannot be identified any other way, this doesn't belong in
> GENERIC.
> 
> Thanks for reading,
> 
> Brian Scott
> 

You're right, the max77620 driver must use additional data (fdt, acpi,
hints, whatever) to decide whether to attach.

A very quick fix should be to change the existing driver to return
BUS_PROBE_NOWILDCARD instead of BUS_PROBE_DEFAULT; that will give the
proper driver a chance to outbid the errant driver for control.

-- Ian