Re: [PATCH 2/2] usb: chipidea: usbmisc: Add support for i.MX51 CPU

[Alexander Shiyan]

Hello.

> > > Signed-off-by: Alexander Shiyan <shc_work at mail.ru>
> > > ---
> > >  drivers/usb/chipidea/usbmisc_imx.c | 40 +++++++++++++++++++++---------
> > --------
> > >  1 file changed, 22 insertions(+), 18 deletions(-)
> > >
> > >
> > > -static int usbmisc_imx53_init(struct imx_usbmisc_data *data)
> > > +static int usbmisc_imx5_init(struct imx_usbmisc_data *data)
> > 
> > Can we keep usbmisc_imx53_init named this?
> > 
> > There is more to do here on i.MX5 for cases where the bootloader did
> > not already try to set up USB (or did it badly) such as the
> > transceiver reference clock rate and also setting the USB sysclk clock
> > source (internal DPLL or from external transceiver in the external
> > ULPI case) and so on, which is related and needs doing on both, but is
> > different on i.MX51 than i.MX53. This is information sort of best
> > passed in the PHY node that goes along with this, but it's set within
> > the usbmisc block of the chips so the usbmisc driver will have a
> > responsibility to go see if it's an external PHY that is feeding it's
> > clock back into the USB block in this way.
> > 
> > I am not sure we (Peter etc.) discussed how best to do this, the code
> > to pull the correct information out always seems kind of misplaced no
> > matter where it goes, but the responsibility for tweaking those
> > registers is most certainly this driver.
> > 
> > Essentially the layout of usbmisc->base + 0x10 register (USB_CTRL_1)
> > is different when doing the above, and dependent on a board-specific
> > option for the input clock to the transceiver. We could reduce a
> > little churn, later, when usbmisc_imx could be given related usbphy
> > information and actually do the right thing. I have a patch kinda
> > sitting in the wings to do this.. and two *real* pieces of consumer
> > hardware that need it, and some other kicking, to make USB work in the
> > never-touched-before-Linux case.
> > 
> > > -static const struct usbmisc_ops imx53_usbmisc_ops = {
> > > -       .init = usbmisc_imx53_init,
> > > +static const struct usbmisc_ops imx5_usbmisc_ops = {
> > > +       .init = usbmisc_imx5_init,
> > >  };
> > 
> > And keep imx53_usbmisc_ops named this?
> > 
> > >  static const struct usbmisc_ops imx6q_usbmisc_ops = {
> > > @@ -204,8 +204,12 @@ static const struct of_device_id
> > usbmisc_imx_dt_ids[] = {
> > >                 .data = &imx27_usbmisc_ops,
> > >         },
> > >         {
> > > +               .compatible = "fsl,imx51-usbmisc",
> > > +               .data = &imx5_usbmisc_ops,
> > 
> > And then just use &imx53_usbmisc_ops?
> > 
> > This gives us some breathing room later to actually do the right thing
> > without additionally performing renames all over the place to make
> > imx5 -> imx53 (again)/imx51 (new).
> 
> You may take matt's suggestion, it can reduce the code change now and in future.
> We can only add device_id for imx51 in this patch, split imx53 and imx51's ops
> when their differences are added in future.

OK. Names of registers and bits also keep as is?

---