Hi Andy, On Mon, 2021-05-24 at 11:02 +0300, Andy Shevchenko wrote: > On Mon, May 24, 2021 at 1:34 AM Sander Vanheule <sander@xxxxxxxxxxxxx> wrote: > > > > The RTL8231 is implemented as an MDIO device, and provides a regmap > > interface for register access by the core and child devices. > > > > The chip can also be a device on an SMI bus, an I2C-like bus by Realtek. > > Since kernel support for SMI is limited, and no real-world SMI > > implementations have been encountered for this device, this is currently > > unimplemented. The use of the regmap interface should make any future > > support relatively straightforward. > > > > After reset, all pins are muxed to GPIO inputs before the pin drivers > > are enabled. This is done to prevent accidental system resets, when a > > pin is connected to the parent SoC's reset line. > > ... > > > [missing MDIO_BUS dependency, provided via REGMAP_MDIO] > > Reported-by: kernel test robot <lkp@xxxxxxxxx> > > What does this fix? Shouldn't it have a Fixes tag? (Yes, I know that > you answered in the other email, but here is a hint: before settling > these kinds of things do not send a new version. Instead of speeding > up the review you are closer to the chance to have this been not > applied for v5.14 at all) > I'll drop this from the commit message, if this isn't appropriate without a Fixes-tag (which I can't provide anyway). > ... > > > + /* SOFT_RESET bit self-clears when done */ > > + regmap_update_bits(map, RTL8231_REG_PIN_HI_CFG, > > + RTL8231_PIN_HI_CFG_SOFT_RESET, > > RTL8231_PIN_HI_CFG_SOFT_RESET); > > > + usleep_range(1000, 10000); > > It's strange to see this big range of minimum and maximum sleep. > Usually the ratio should not be bigger than ~3-4 between the values. I could also change this from a usleep to a polling loop that checks (with a loop limit) if the reset bit has self-cleared already. The datasheet that I have doesn't mention how fast it should self-clear. So I checked, and it appears to be done after one loop iteration already. So, certainly faster than the current usleep. Would a polling loop (with maybe like max. 10 iterations) be a good alternative for you? > > ... > > > + regmap_write(map, RTL8231_REG_PIN_MODE0, 0xffff); > > + regmap_write(map, RTL8231_REG_GPIO_DIR0, 0xffff); > > + regmap_write(map, RTL8231_REG_PIN_MODE1, 0xffff); > > + regmap_write(map, RTL8231_REG_GPIO_DIR1, 0xffff); > > GENMASK() ? > Actually it seems it deserves a special definition like > > ..._ALL_PIN_MASK GENMASK(15, 0) Ok, I'll add the extra macro to clarify the intent of the values. Best, Sander
