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 [off-list ref] wrote:

quoted

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.

...

quoted

[missing MDIO_BUS dependency, provided via REGMAP_MDIO]
Reported-by: kernel test robot <redacted>

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).

...

quoted

+       /* 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);

quoted

+       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?

...

quoted

+       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