RE: [PATCH v4 2/5] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0 PHY

["Stanley Chang[昌育德]" <stanley_chang@realtek.com>]

Hi Krzysztof,

> > +     addr = phy_data->addr;
> > +     data = phy_data->data;
> > +     dc_disconnect_mask = phy_cfg->dc_disconnect_mask;
> > +
> > +     if (update)
> > +             data =
> __updated_dc_disconnect_level_page0_0xe4(phy_cfg, phy_parameter, data);
> > +     else
> > +             data = (data & ~(dc_disconnect_mask << offset)) |
> > +                     (DEFAULT_DC_DISCONNECTION_VALUE <<
> offset);
> > +
> > +     if (rtk_phy_write(phy_reg, addr, data))
> > +             dev_err(rtk_phy->dev,
> > +                     "[%s:%d] Error page1 addr=0x%x value=0x%x\n",
> > +                     __func__, __LINE__,
> > +                     addr, data);
> 
> Is addr a kernel address or any memory (not SFR) address? If so, you cannot
> print it.

It is not memory address.

> > +
> > +     if (rtk_phy_write(phy_reg, addr, data))
> > +             dev_err(rtk_phy->dev,
> > +                     "[%s:%d] Error page1 addr=0x%x value=0x%x\n",
> > +                     __func__, __LINE__,
> > +                     addr, data);
> 
> Ditto and in all other places.

It is not memory address.

> > +static u8 __update_dc_driving_page0_0xe4(struct phy_cfg *phy_cfg,
> > +                                      struct phy_parameter
> > +*phy_parameter, u8 data) {
> > +     s32 driving_compensate = phy_parameter->driving_compensate;
> > +     s32 dc_driving_mask = phy_cfg->dc_driving_mask;
> > +     s32 __val;
> > +     u8 val;
> 
> Two variables with the same name. No, it is not readable code.

Okay. I will revise it.

> > +static void rtk_phy_toggle(struct usb_phy *usb2_phy, bool connect,
> > +int port) {
> > +     int index = port;
> > +     struct rtk_phy *rtk_phy = NULL;
> > +
> > +     rtk_phy = dev_get_drvdata(usb2_phy->dev);
> > +
> > +     if (index > rtk_phy->num_phy) {
> > +             pr_err("%s %d ERROR! port=%d > num_phy=%d\n",
> 
> dev_err

I revised it.

> > +                    __func__, __LINE__, index, rtk_phy->num_phy);
> 
> all these func and LINE point to poor code quality and poor debugging
> practices. These are added dugin development, not for production code,
> because error message should be obvious. Your usage of pr_err, func, LINE and
> some unprecise messages suggests this is not ready.
> 
> Fix all your error messages to be meaningful.

I will review all error messages.
Thanks.

> > +static const struct file_operations rtk_usb2_set_parameter_fops = {
> > +     .open                   = rtk_usb2_set_parameter_open,
> > +     .write                  = rtk_usb2_set_parameter_write,
> 
> NAK. You just created user interface via debugfs. You cannot. Reading for some
> debug is okay, but configuring device via undocumented debugfs is a source of
> troubles.
> 
> Drop all writes to debugfs.

I will remove this.

> 
> > +
> > +static int parse_phy_data(struct rtk_phy *rtk_phy) {
> > +     struct device *dev = rtk_phy->dev;
> > +     struct device_node *node;
> 
> By convention:
> s/node/np/

Okay.

> > +     struct phy_cfg *phy_cfg;
> > +     struct phy_parameter *phy_parameter;
> > +     int ret = 0;
> > +     int index;
> > +
> > +     node = dev->of_node;
> 
> Keep it in variable definition.

Okay.

> > +
> > +static int rtk_usb2phy_probe(struct platform_device *pdev) {
> > +     struct rtk_phy *rtk_phy;
> > +     struct device *dev = &pdev->dev;
> > +     struct device_node *node;
> > +     struct phy *generic_phy;
> > +     struct phy_provider *phy_provider;
> > +     const struct phy_cfg *phy_cfg;
> > +     int ret = 0;
> > +
> > +     phy_cfg = of_device_get_match_data(dev);
> > +     if (!phy_cfg) {
> > +             dev_err(dev, "phy config are not assigned!\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     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-usb2phy";
> > +     rtk_phy->phy.notify_port_status = rtk_phy_notify_port_status;
> > +
> > +     rtk_phy->phy_cfg = devm_kzalloc(dev, sizeof(*phy_cfg),
> > + GFP_KERNEL);
> > +
> > +     memcpy(rtk_phy->phy_cfg, phy_cfg, sizeof(*phy_cfg));
> > +
> > +     node = dev->of_node;
> 
> Drop it. Useless assignment.

Okay.

> > +
> > +     if (of_device_is_compatible(node,
> > + "realtek,rtd1395-usb2phy-2port"))
> 
> No, customize variant with driver_data. Don't embed compatibles in the code.

I will use the compatible data to match this case.

> 
> > +             rtk_phy->num_phy = 2;
> > +     else
> > +             rtk_phy->num_phy = 1;
> > +
> > +     ret = parse_phy_data(rtk_phy);
> > +     if (ret)
> > +             goto err;
> > +
> > +     platform_set_drvdata(pdev, rtk_phy);
> > +
> > +     generic_phy = devm_phy_create(rtk_phy->dev, NULL, &ops);
> > +     if (IS_ERR(generic_phy))
> > +             return PTR_ERR(generic_phy);
> > +
> > +     phy_set_drvdata(generic_phy, rtk_phy);
> > +
> > +     phy_provider = devm_of_phy_provider_register(rtk_phy->dev,
> > +
> of_phy_simple_xlate);
> > +     if (IS_ERR(phy_provider))
> > +             return PTR_ERR(phy_provider);
> > +
> > +     ret = usb_add_phy_dev(&rtk_phy->phy);
> > +     if (ret)
> > +             goto err;
> > +
> > +     create_debug_files(rtk_phy);
> > +
> > +err:
> > +     dev_dbg(dev, "Probe RTK USB 2.0 PHY (ret=%d)\n", ret);
> 
> NAK. I made it pretty clear last time.
> 
> 
> This is a friendly reminder during the review process.
> 
> It seems my previous comments were not fully addressed. Maybe my feedback
> got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all requested
> changes or keep discussing them.
> 
> Thank you.

Sorry. I left out this print, I will delete it.
Thank you.

> > +
> > +     return ret;
> > +}
> > +
> > +static void rtk_usb2phy_remove(struct platform_device *pdev) {
> > +     struct rtk_phy *rtk_phy = platform_get_drvdata(pdev);
> > +
> > +     remove_debug_files(rtk_phy);
> > +
> > +     usb_remove_phy(&rtk_phy->phy);
> > +}
> 
> ...
> 
> > +
> > +static const struct of_device_id usbphy_rtk_dt_match[] = {
> > +     { .compatible = "realtek,rtd1295-usb2phy", .data =
> &rtd1295_phy_cfg },
> > +     { .compatible = "realtek,rtd1312c-usb2phy", .data =
> &rtd1312c_phy_cfg },
> > +     { .compatible = "realtek,rtd1315e-usb2phy", .data =
> &rtd1315e_phy_cfg },
> > +     { .compatible = "realtek,rtd1319-usb2phy", .data =
> &rtd1319_phy_cfg },
> > +     { .compatible = "realtek,rtd1319d-usb2phy", .data =
> &rtd1319d_phy_cfg },
> > +     { .compatible = "realtek,rtd1395-usb2phy", .data =
> &rtd1395_phy_cfg },
> > +     { .compatible = "realtek,rtd1395-usb2phy-2port", .data =
> &rtd1395_phy_cfg },
> > +     { .compatible = "realtek,rtd1619-usb2phy", .data =
> &rtd1619_phy_cfg },
> > +     { .compatible = "realtek,rtd1619b-usb2phy", .data =
> &rtd1619b_phy_cfg },
> > +     { .compatible = "realtek,usb2phy", .data = &rtk_phy_cfg },
> 
> This is now even more suprising. Drop "realtek,usb2phy"

I will remove it.
> > +     {},
> > +};
> > +MODULE_DEVICE_TABLE(of, usbphy_rtk_dt_match);
> > +
> > +static struct platform_driver rtk_usb2phy_driver = {
> > +     .probe          = rtk_usb2phy_probe,
> > +     .remove_new     = rtk_usb2phy_remove,
> > +     .driver         = {
> > +             .name   = "rtk-usb2phy",
> > +             .owner  = THIS_MODULE,
> 
> ??? Didn't you base your driver on some really, really ancient code (like 5 years
> old)? If so, please don't.


Thank you.
I will remove it.

> Run coccicenelle/coccicheck, smatch and sparse, to avoid common mistakes.
> 
I will run these tools.

Thanks,
Stanley