Re: [PATCH v2 2/6] phy: realtek: usb2: introduce read and write functions to driver data

[Vladimir Oltean]

On Tue, Mar 31, 2026 at 12:19:18AM +0300, Vladimir Oltean wrote:
> On Fri, Mar 27, 2026 at 09:06:34PM +0500, Rustam Adilov wrote:
> > +static inline u32 phy_read(void __iomem *reg)
> > +{
> > +	return readl(reg);
> > +}
> > +
> > +static inline u32 phy_read_le(void __iomem *reg)
> > +{
> > +	return le32_to_cpu(readl(reg));
> > +}
> > +
> > +static inline void phy_write(u32 val, void __iomem *reg)
> > +{
> > +	writel(val, reg);
> > +}
> > +
> > +static inline void phy_write_le(u32 val, void __iomem *reg)
> > +{
> > +	writel(cpu_to_le32(val), reg);
> > +}
> 
> Please don't name driver-level functions phy_read() and phy_write().
> That will collide with networking API functions of the same name and
> will make grep-based code searching more difficult.
> 
> Also, have you looked at regmap? It has native support for endianness;
> it supports regmap_field_read()/regmap_field_write() for abstracting
> registers which may be found at different places for different HW;
> it offers regmap_read_poll_timeout() so you don't have to pass the
> function pointer to utmi_wait_register(). It seems the result would be a
> bit more elegant.

Even if you decide not to use regmap. I thought I should let you know
that LLM review says:

  Are these double byte-swaps intentional?

  Since readl() and writel() inherently perform little-endian memory accesses
  and handle byte-swapping on big-endian architectures automatically, won't
  wrapping them in le32_to_cpu() and cpu_to_le32() apply a second, redundant
  byte-swap?

  On big-endian systems, wouldn't these double swaps cancel each other out
  and result in a native big-endian access instead of the intended
  little-endian access? If the SoC bus bridge implicitly swaps and requires
  a native access, should __raw_readl() and __raw_writel() (or ioread32be /
  iowrite32be) be used instead to avoid obfuscating it with double-swaps?

  Also, does passing the __le32 restricted type returned by cpu_to_le32()
  into writel() (which expects a native u32) trigger Sparse static analysis
  warnings for an incorrect type in argument?

For reference:
https://elixir.bootlin.com/linux/v6.19.10/source/include/asm-generic/io.h#L184
/*
 * {read,write}{b,w,l,q}() access little endian memory and return result in
 * native endianness.
 */

and yes, your patch does trigger sparse warnings:
../drivers/phy/realtek/phy-rtk-usb2.c:153:16: warning: cast to restricted __le32
../drivers/phy/realtek/phy-rtk-usb2.c:163:16: warning: incorrect type in argument 1 (different base types)
../drivers/phy/realtek/phy-rtk-usb2.c:163:16:    expected unsigned int val
../drivers/phy/realtek/phy-rtk-usb2.c:163:16:    got restricted __le32 [usertype]

Furthermore, please drop the 'inline' keyword from C files and let the
compiler decide. Your use of this keyword has no value - you declare
phy_read(), phy_read_le() etc as inline but then assign function
pointers to them. How can the compiler inline the indirect calls?