Hi Arnd,
> > +#ifdef CONFIG_DEBUG_FS
> > + struct dentry *debug_dir;
> > +#endif
> > +};
>
> I'd avoid the #ifdefs here and just leave the debugfs code in unconditionally in
> favor of readability. When debugfs is disabled, everything except for the one
> pointer value should get optimized out.
I will remove this #ifdef.
> > +#define phy_read(addr) __raw_readl(addr) #define phy_write(addr, val)
> > +do { \
> > + /* Do smp_wmb */ \
> > + smp_wmb(); __raw_writel(val, addr); \ } while (0)
>
> Using __raw_readl()/__raw_writel() in a driver is almost never the right
> interface, it does not guarantee atomicity of the access, has the wrong
> byte-order on big-endian kernels and misses the barriers to serialize against
> DMA accesses. smp_wmb() should have no effect here since this only serializes
> access to memory against another CPU if it's paired with an smp_rmb(), but
> not on MMIO registers.
I will try using readl/writel directly.
> > +#define PHY_IO_TIMEOUT_MSEC (50)
> > +
> > +static inline int utmi_wait_register(void __iomem *reg, u32 mask, u32
> > result)
> > +{
> > + unsigned long timeout = jiffies +
> > msecs_to_jiffies(PHY_IO_TIMEOUT_MSEC);
> > +
> > + while (time_before(jiffies, timeout)) {
> > + /* Do smp_rmb */
> > + smp_rmb();
> > + if ((phy_read(reg) & mask) == result)
> > + return 0;
> > + udelay(100);
> > + }
> > + pr_err("\033[0;32;31m can't program USB phy \033[m\n");
> > +
> > + return -ETIMEDOUT;
> > +}
>
> This should just use read_poll_timeout() or possibly
> read_poll_timeout_atomic(), but not an open-coded version.
>
I've tried using read_poll_timeout() instead and it works.
> I don't think I've seen escape sequences in a printk in any other driver, so
> please don't start that now.
Okay. I will remove it.
> > +#define DEFAULT_CHIP_REVISION 0xA00
> > +#define MAX_CHIP_REVISION 0xC00
> > +
> > +static inline int __get_chip_revision(void) {
> > + int chip_revision = 0xFFF;
> > + char revision[] = "FFF";
> > + struct soc_device_attribute soc_att[] = {{.revision = revision},
> > +{}};
>
> You should probably check that you are actually on the right SoC type here as
> well, not just the right revision of an arbitrary chip.
>
> Ideally I think the revision check should be based off a DT proporty if that's
> possible, so you can have this code in the boot loader.
In this case I just care in the chip version number.
Because the revision number is not recorded in the DTB.
> > +#define RTK_USB2PHY_NAME "rtk-usb2phy"
>
> Better avoid hiding the driver name like this, it makes it harder to grep the
> source tree for particular driver names.
I will remove this coding style.
> > + /* rmb for reg read */
> > + smp_rmb();
> > + regVal = phy_read(reg_gusb2phyacc0);
>
> I would expect that you don't need barriers like this, especially if you change
> the phy_read() helper to use the proper readl().
>
> If you do need to serialize against other CPUs, still, there should be a longer
> explanation about that, since it's so unexpected.
I will use readl to instead the phy_read and remove smp_rmb.
> > +
> > +static void do_rtk_usb2_phy_toggle(struct rtk_usb_phy *rtk_phy,
> > + int index, bool isConnect);
>
> It's best to sort your function definitions in a way that avoids forward
> declarations. This makes it easier to read and shows that there are no obvious
> recursions in the source. If you do have an intentional recursion, make sure that
> there is a way to prevent unbounded stack usage, and explain that in a
> comment.
Ok, I'll move this function to just before the first use.
> > + regAddr = &((struct reg_addr *)rtk_phy->reg_addr)[index];
> > + phy_data = &((struct phy_data *)rtk_phy->phy_data)[index];
> Why do you need the casts here? It looks like regAddr should be an __iomem
> pointer. Please build your driver with 'make C=1'
> to see if there are any incorrect address space annotations.
I define member reg_addr as
void *reg_addr
So I have to convert it to "struct reg_addr" and use it as array.
And I ran "make C=1" with no error or warning.
> > +static int __get_phy_parameter_by_efuse(struct rtk_usb_phy *rtk_phy,
> > + struct phy_data *phy_data, int index) {
> > + u8 value = 0;
> > + struct nvmem_cell *cell;
> > + struct soc_device_attribute rtk_soc_groot[] = {
> > + { .family = "Realtek Groot",},
> > + { /* empty */ }
> > + };
> > + struct soc_device_attribute rtk_soc_hank[] = {
> > + { .family = "Realtek Hank",},
> > + { /* empty */ }
> > + };
> > + struct soc_device_attribute rtk_soc_efuse_v1[] = {
> > + { .family = "Realtek Phoenix",},
> > + { .family = "Realtek Kylin",},
> > + { .family = "Realtek Hercules",},
> > + { .family = "Realtek Thor",},
> > + { .family = "Realtek Hank",},
> > + { .family = "Realtek Groot",},
> > + { .family = "Realtek Stark",},
> > + { .family = "Realtek Parker",},
> > + { /* empty */ }
> > + };
> > + struct soc_device_attribute rtk_soc_dis_level_at_page0[] = {
> > + { .family = "Realtek Phoenix",},
> > + { .family = "Realtek Kylin",},
> > + { .family = "Realtek Hercules",},
> > + { .family = "Realtek Thor",},
> > + { .family = "Realtek Hank",},
> > + { .family = "Realtek Groot",},
> > + { /* empty */ }
> > + };
> > +
> > + if (soc_device_match(rtk_soc_efuse_v1)) {
> > + dev_dbg(rtk_phy->dev, "Use efuse v1 to updated phy
> parameter\n");
> > + phy_data->check_efuse_version = CHECK_EFUSE_V1;
>
> I'm not entirely sure what you are trying to do here, but it looks the purpose is
> to tell the difference between implementations of the phy device by looking at
> which SoC it's in. You should only need that very rarely when this information
> cannot be passed through the DT, but you literally already have the per-SoC
> compatible strings below, so just use those, or add other DT properties in the
> binding for specific quirks or capabilities.
My purpose is to judge different SoCs and set different parameters.
The DTS property might be a good way to go, I'll check to see if it's a good fit.
> Remove that of_match_ptr() and ifdef CONFIG_OF check here, new drivers
> should no longer use static platform device definitions and just assume that
> CONFIG_OF is used.
>
Ok, I will remove it.
Thanks,
Stanley
