Hi Andy,

Thank you for the review!

> From: Andy Shevchenko, Sent: Tuesday, April 10, 2018 9:40 PM
> 
> On Tue, Apr 10, 2018 at 3:03 PM, Yoshihiro Shimoda
> <yoshihiro.shimoda...@renesas.com> wrote:
<snip>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> 
> > +#include <linux/of_device.h>
> 
> Do you need this one?

If using of_parse_phandle() is acceptable, I need linux/of_platform.h at least.

> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/slab.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/usb/role.h>
> 
> > +static const struct of_device_id rcar_usb3_role_switch_of_match[] = {
> > +       { .compatible = "renesas,rcar-usb3-role-switch" },
> > +       { },
> 
> Better to remove comma at terminator line.

I got it. I will fix it in v2.

> > +};
> > +MODULE_DEVICE_TABLE(of, rcar_usb3_role_switch_of_match);
> 
> > +static int rcar_usb3_role_switch_probe(struct platform_device *pdev)
> > +{
> 
> > +       /* Avoid devm_request_mem_region() calling by 
> > devm_ioremap_resource() */
> 
> Redundant comment.

I will remove this comment.

> > +       host_node = of_parse_phandle(pdev->dev.of_node, "renesas,host", 0);
> > +       if (!host_node)
> > +               return -ENODEV;
> > +
> > +       pdev_host = of_find_device_by_node(host_node);
> > +       if (!pdev_host)
> > +               return -ENODEV;
> > +
> > +       of_node_put(host_node);
> 
> Isn't what Heikki tried to solve with graphs and stuff like that?

Heikki prepared "device connection" framework (devcon) [1] to get a device 
pointer.
However, IIUC, we need to improve the framework for device tree environment 
because:
 - The devcon needs each dev_name() through the endpoint array of struct 
device_connection.
 - Each dev_name() on device tree environment will be <base address 
register>.<device name>,
   f.e. "ee020000.usb". So, I don’t think adding such strings to a driver is a 
good way.

So, I wrote such a code by using existing APIs.
Should I improve the framework first somehow? Or, is my understanding incorrect?

[1] https://patchwork.kernel.org/patch/10297041/

> > +       dev_info(&pdev->dev, "probed\n");
> 
> Noise. (Hint: initcall_debug is a good command line option)

Thank you for the hint! I will remove the dev_info().

> > +}
> 
> > +#ifdef CONFIG_PM_SLEEP
> 
> Instead...
> 
> > +static int rcar_usb3_role_switch_suspend(struct device *dev)
> 
> ...use __maybe_unused annotation.
> 
> > +static int rcar_usb3_role_switch_resume(struct device *dev)
> 
> Ditto.

I will fix them in v2 patch.

> > +#endif
> 
> 
> > +static struct platform_driver rcar_usb3_role_switch_driver = {
> > +       .probe          = rcar_usb3_role_switch_probe,
> > +       .remove         = rcar_usb3_role_switch_remove,
> > +       .driver         = {
> > +               .name = "rcar_usb3_role_switch",
> > +               .pm             = &rcar_usb3_role_switch_pm_ops,
> 
> > +               .of_match_table = 
> > of_match_ptr(rcar_usb3_role_switch_of_match),
> 
> Is it possible to have this driver w/o OF? Does it make sense in that case?
> Otherwise remove of_match_ptr() call and below modalias.

This driver depends on OF now. So, I will remove of_match_ptr() and 
MODULE_ALIAS().

Best regards,
Yoshihiro Shimoda

> > +       },
> > +};
> 
> > +MODULE_ALIAS("platform:rcar_usb3_role_switch");
> 
> --
> With Best Regards,
> Andy Shevchenko

Reply via email to