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.uh@xxxxxxxxxxx> 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