Hi Andy,
Thank you for your review!
> From: Andy Shevchenko, Sent: Monday, May 28, 2018 9:47 PM
>
> On Fri, May 25, 2018 at 10:12 AM, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
>
> > -static void usb3_set_mode(struct renesas_usb3 *usb3, bool host)
>
> Wouldn't be better to choose another name for a new function?
I got it. I'll choose another name for a new function as "usb3_set_mode_by_role_sw".
> > + struct renesas_usb3 *usb3 = container_of(work, struct renesas_usb3,
> > + role_work);
>
> Matter of style, though I would rather put entire container_of() on
> the next line (see for the existing style in the module and use it).
I got it. I'll modify that.
> > + /* This device_attach() might sleep */
> > + if (device_attach(host) < 0)
> > + dev_err(dev, "device_attach(usb3_port) failed\n");
>
> can't be "usb3_port" part derived from the host variable somehow and
> to some extend?
Oops. I should describe "host" instead of "usb3_port".
> > + usb3->role_sw = usb_role_switch_register(&pdev->dev,
> > + &renesas_usb3_role_switch_desc);
> > + if (!IS_ERR(usb3->role_sw)) {
>
> > + usb3->host_dev = usb_of_get_companion_dev(&pdev->dev);
>
> Hmm... Can it possible return -EPROBE_DEFER? If so, would it be better
> to use other approach to handle it?
Does this "it" mean " usb_of_get_companion_dev()"? If so, it is not possible
to return -EPROBE_DEFER.
Or, " usb_role_switch_register()"? Also this function is not possible to
return -EPROBE_DEFER, IIUC.
> > + if (IS_ERR_OR_NULL(usb3->host_dev)) {
So, I can just use "if (!usb3->host_dev) {" instead of above.
Best regards,
Yoshihiro Shimoda
> > + /* If not found, this driver will not use a role sw */
> > + usb_role_switch_unregister(usb3->role_sw);
> > + usb3->role_sw = NULL;
> > + }
> > + } else {
> > + usb3->role_sw = NULL;
> > + }
>
>
> --
> With Best Regards,
> Andy Shevchenko
��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
