RE: [PATCH v3 3/5] phy: realtek: usb: Add driver for the Realtek SoC USB 3.0 PHY

[Stanley Chang[昌育德]]

Hi Krzysztof,

> > +static void do_rtk_usb3_phy_toggle(struct rtk_usb_phy *rtk_phy, int i,
> > +         bool isConnect)
> > +{
> > +     struct reg_addr *regAddr;
> > +     struct phy_data *phy_data;
> > +     struct phy_parameter *phy_parameter;
> > +     size_t index;
> > +
> > +     regAddr = &((struct reg_addr *)rtk_phy->reg_addr)[i];
> > +     phy_data = &((struct phy_data *)rtk_phy->phy_data)[i];
> > +
> > +     if (!phy_data) {
> > +             dev_err(rtk_phy->dev, "%s phy_data is NULL!\n",
> > + __func__);
> 
> ???
Sorry, this check is redundant.

> 
> > +             return;
> > +     }
> > +
> > +     if (!phy_data->do_toggle)
> > +             return;
> > +
> > +     phy_parameter = phy_data->parameter;
> > +
> > +     index = PHY_ADDR_MAP_ARRAY_INDEX(PHY_ADDR_0x09);
> > +
> > +     if (index < phy_data->size) {
> > +             u8 addr = (phy_parameter + index)->addr;
> > +             u16 data = (phy_parameter + index)->data;
> > +
> > +             if (addr == 0xFF) {
> > +                     addr = ARRAY_INDEX_MAP_PHY_ADDR(index);
> > +                     data = rtk_usb_phy_read(regAddr, addr);
> > +                     (phy_parameter + index)->addr = addr;
> > +                     (phy_parameter + index)->data = data;
> > +             }
> > +             mdelay(1);
> > +             dev_info(rtk_phy->dev,
> > +                         "%s ########## to toggle PHY addr 0x09
> BIT(9)\n",
> > +                         __func__);
> > +             rtk_usb_phy_write(regAddr, addr, data&(~BIT(9)));
> > +             mdelay(1);
> > +             rtk_usb_phy_write(regAddr, addr, data);
> > +     }
> > +     dev_info(rtk_phy->dev, "%s ########## PHY addr 0x1f = 0x%04x\n",
> > +                 __func__, rtk_usb_phy_read(regAddr,
> PHY_ADDR_0x1F));
> 
> Please drop all simple debug success messages. Linux has already
> infrastructure for this.
> 
Okay. I will change the print dev_info to dev_dbg about debug message.

> ...
> 
> > +     return 0;
> > +}
> > +
> > +static int rtk_usb_phy_init(struct phy *phy) {
> > +     struct rtk_usb_phy *rtk_phy = phy_get_drvdata(phy);
> > +     int ret = 0;
> > +     int i;
> > +     unsigned long phy_init_time = jiffies;
> > +
> > +     if (!rtk_phy) {
> > +             pr_err("%s rtk_phy is NULL!\n", __func__);
> 
> What? How is this possible?
It should be not necessary. I will remove it.

> > +             return -ENODEV;
> > +     }
> > +
> > +     dev_dbg(rtk_phy->dev, "Init RTK USB 3.0 PHY\n");
> > +     for (i = 0; i < rtk_phy->phyN; i++)
> > +             ret = do_rtk_usb_phy_init(rtk_phy, i);
> > +
> > +     dev_info(rtk_phy->dev, "Initialized RTK USB 3.0 PHY (take %dms)\n",
> > +                 jiffies_to_msecs(jiffies - phy_init_time));
> 
> Please drop all simple debug success messages. Linux has already
> infrastructure for this.

Ok, Thanks.

> > +     return ret;
> > +}
> > +
> > +static int rtk_usb_phy_exit(struct phy *phy) {
> > +     struct rtk_usb_phy *rtk_phy = phy_get_drvdata(phy);
> > +
> > +     if (!rtk_phy) {
> > +             pr_err("%s rtk_phy is NULL!\n", __func__);
> > +             return -ENODEV;
> > +     }
> > +
> > +     dev_dbg(rtk_phy->dev, "Exit RTK USB 3.0 PHY\n");
> 
> Please drop all simple debug success messages. Linux has already
> infrastructure for this.

Can I keep log for dev_dbg?

> > +static void rtk_usb_phy_toggle(struct usb_phy *usb3_phy, bool
> > +isConnect, int port) {
> > +     int index = port;
> > +     struct rtk_usb_phy *rtk_phy = NULL;
> > +
> > +     if (usb3_phy != NULL && usb3_phy->dev != NULL)
> > +             rtk_phy = dev_get_drvdata(usb3_phy->dev);
> > +
> > +     if (rtk_phy == NULL) {
> > +             pr_err("%s ERROR! NO this device\n", __func__);
> 
> Your error messages are not helping. No need to shout, no need to handle
> some non-existing cases. If this is real case, you have broken driver. I actually
> suspect that.
> 
> How can you interface with a driver where there is no device?

OK, I know this is not good programming practice, I will improve this question.

> > +             return;
> > +     }
> > +
> > +     if (index > rtk_phy->phyN) {
> > +             pr_err("%s %d ERROR! port=%d > phyN=%d\n",
> > +                         __func__, __LINE__, index, rtk_phy->phyN);
> > +             return;
> > +     }
> > +
> > +     do_rtk_usb3_phy_toggle(rtk_phy, index, isConnect); }
> > +
> > +static int rtk_usb_phy_notify_port_status(struct usb_phy *x, int port,
> > +         u16 portstatus, u16 portchange) {
> > +     bool isConnect = false;
> 
> This is not C++. Don't use camelcase. See Coding style document.

I will revised for this style.

> > +
> > +     pr_debug("%s port=%d portstatus=0x%x portchange=0x%x\n",
> > +                 __func__, port, (int)portstatus, (int)portchange);
> > +     if (portstatus & USB_PORT_STAT_CONNECTION)
> > +             isConnect = true;
> > +
> 
> ...
> 
> > +
> > +static int rtk_usb3_set_parameter_show(struct seq_file *s, void
> > +*unused) {
> > +     struct rtk_usb_phy *rtk_phy = s->private;
> > +     const struct file *file = s->file;
> > +     const char *file_name = file_dentry(file)->d_iname;
> > +     struct dentry *p_dentry = file_dentry(file)->d_parent;
> > +     const char *phy_dir_name = p_dentry->d_iname;
> > +     int ret, index;
> > +     struct phy_data *phy_data = NULL;
> > +
> > +     for (index = 0; index < rtk_phy->phyN; index++) {
> > +             size_t sz = 30;
> > +             char name[30] = {0};
> > +
> > +             snprintf(name, sz, "phy%d", index);
> > +             if (strncmp(name, phy_dir_name, strlen(name)) == 0) {
> > +                     phy_data = &((struct phy_data
> *)rtk_phy->phy_data)[index];
> > +                     break;
> > +             }
> > +     }
> > +     if (!phy_data) {
> > +             dev_err(rtk_phy->dev,
> > +                                 "%s: No phy_data for %s/%s\n",
> > +                                 __func__, phy_dir_name,
> file_name);
> 
> Mess wrapping/indentation. Actually everywhere in the file...

I will improve this.

> > +static int rtk_usb3_set_parameter_open(struct inode *inode, struct
> > +file *file) {
> > +     return single_open(file, rtk_usb3_set_parameter_show,
> > +inode->i_private); }
> > +
> > +static ssize_t rtk_usb3_set_parameter_write(struct file *file,
> > +             const char __user *ubuf, size_t count, loff_t *ppos) {
> > +     const char *file_name = file_dentry(file)->d_iname;
> > +     struct dentry *p_dentry = file_dentry(file)->d_parent;
> > +     const char *phy_dir_name = p_dentry->d_iname;
> > +     struct seq_file         *s = file->private_data;
> > +     struct rtk_usb_phy              *rtk_phy = s->private;
> > +     struct reg_addr *regAddr = NULL;
> > +     struct phy_data *phy_data = NULL;
> > +     int ret = 0;
> > +     char buffer[40] = {0};
> > +     int index;
> > +
> > +     if (copy_from_user(&buffer, ubuf,
> > +                 min_t(size_t, sizeof(buffer) - 1, count)))
> > +             return -EFAULT;
> > +
> > +     for (index = 0; index < rtk_phy->phyN; index++) {
> > +             size_t sz = 30;
> > +             char name[30] = {0};
> > +
> > +             snprintf(name, sz, "phy%d", index);
> > +             if (strncmp(name, phy_dir_name, strlen(name)) == 0) {
> > +                     regAddr = &((struct reg_addr
> *)rtk_phy->reg_addr)[index];
> > +                     phy_data = &((struct phy_data
> *)rtk_phy->phy_data)[index];
> > +                     break;
> 
> 
> Where is the ABI documentation for user interface?

Do debugfs nodes need ABI documentation?
Is there a reference?
> 
> > +
> > +static inline void create_debug_files(struct rtk_usb_phy *rtk_phy) {
> > +     struct dentry *phy_debug_root = NULL;
> > +     struct dentry *set_parameter_dir = NULL;
> > +
> > +     phy_debug_root = create_phy_debug_root();
> > +
> > +     if (!phy_debug_root) {
> > +             dev_err(rtk_phy->dev, "%s Error phy_debug_root is NULL",
> > +                         __func__);
> > +             return;
> > +     }
> > +     rtk_phy->debug_dir = debugfs_create_dir(dev_name(rtk_phy->dev),
> > +                 phy_debug_root);
> > +     if (!rtk_phy->debug_dir) {
> > +             dev_err(rtk_phy->dev, "%s Error debug_dir is NULL",
> > + __func__);
> 
> Are you sure you run checkpatch on this? Error messages on debugfs are
> almost always incorrect.

Yes, I have run checkpatch for patches. 
Why the message is incorrect?

> > +static int get_phy_parameter(struct rtk_usb_phy *rtk_phy,
> > +         struct device_node *sub_node) {
> > +     struct device *dev = rtk_phy->dev;
> > +     struct reg_addr *addr;
> > +     struct phy_data *phy_data;
> > +     int ret = 0;
> > +     int index;
> > +
> > +     if (of_property_read_u32(sub_node, "reg", &index)) {
> > +             dev_err(dev, "sub_node without reg\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     dev_dbg(dev, "sub_node index=%d\n", index);
> 
> Please drop all simple debug success messages. Linux has already
> infrastructure for this.

Can I keep log for dev_dbg?

> ...
> 
> > +
> > +static int rtk_usb3phy_probe(struct platform_device *pdev) {
> > +     struct rtk_usb_phy *rtk_phy;
> > +     struct device *dev = &pdev->dev;
> > +     struct device_node *node;
> > +     struct device_node *sub_node;
> > +     struct phy *generic_phy;
> > +     struct phy_provider *phy_provider;
> > +     int ret, phyN;
> > +
> > +     rtk_phy = devm_kzalloc(dev, sizeof(*rtk_phy), GFP_KERNEL);
> > +     if (!rtk_phy)
> > +             return -ENOMEM;
> > +
> > +     rtk_phy->dev                    = &pdev->dev;
> > +     rtk_phy->phy.dev                = rtk_phy->dev;
> > +     rtk_phy->phy.label              = "rtk-usb3phy";
> > +     rtk_phy->phy.notify_port_status =
> > + rtk_usb_phy_notify_port_status;
> > +
> > +     if (!dev->of_node) {
> > +             dev_err(dev, "%s %d No device node\n", __func__,
> > + __LINE__);
> 
> How is it even possible? If you do not have device node here, how did it probe?

You are right. The of_node is always exist.
I will remove it.

> 
> > +             ret = -ENODEV;
> > +             goto err;
> > +     }
> > +
> > +     node = dev->of_node;
> > +
> > +     phyN = of_get_child_count(node);
> > +     rtk_phy->phyN = phyN;
> > +     dev_dbg(dev, "%s phyN=%d\n", __func__, rtk_phy->phyN);
> 
> Please drop all simple debug success messages. Linux has already
> infrastructure for this.
Okay. I will remove debug message.


Thanks,
Stanley