RE: [PATCH v5] usb: gadget: udc: renesas_usb3: Add register of usb role switch

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux