Hi Andy,
Thank you for the review!
Advertising
> 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
