[U-Boot] [PATCH 3/5] usb: s3c-otg: Split out PHY control

[Marek Vasut]

On Friday, November 07, 2014 at 03:23:42 PM, Lukasz Majewski wrote:
> Hi Marek,
> 
> > On Friday, November 07, 2014 at 11:08:44 AM, Lukasz Majewski wrote:
> > > Hi Marek,
> > > 
> > > > On Friday, November 07, 2014 at 09:59:25 AM, Lukasz Majewski
> > > > 
> > > > wrote:
> > > > > Hi Marek,
> > > > > 
> > > > > > On Tuesday, November 04, 2014 at 08:34:21 PM, Pavel Machek
> > > > > > 
> > > > > > wrote:
> > > > > > > On Tue 2014-11-04 06:07:32, Marek Vasut wrote:
> > > > > > > > Split the Samsung specific PHY control into a separate
> > > > > > > > file and compile this into the S3C OTG driver only if
> > > > > > > > used on a Samsung system.
> > > > > > > > 
> > > > > > > > Signed-off-by: Marek Vasut <marex at denx.de>
> > > > > > > > Cc: Chin Liang See <clsee at altera.com>
> > > > > > > > Cc: Dinh Nguyen <dinguyen at opensource.altera.com>
> > > > > > > > Cc: Vince Bridgers <vbridger at altera.com>
> > > > > > > 
> > > > > > > Acked-by: Pavel Machek <pavel at denx.de>
> > > > > > > 
> > > > > > > I know you are just moving the code, but...
> > > > > > > 
> > > > > > > > +void otg_phy_init(struct s3c_udc *dev)
> > > > > > > > +{
> > > > > > > > +	unsigned int usb_phy_ctrl =
> > > > > > > > dev->pdata->usb_phy_ctrl;
> > > > > > > > +	struct s3c_usbotg_phy *phy =
> > > > > > > > +		(struct s3c_usbotg_phy
> > > > > > > > *)dev->pdata->regs_phy; +
> > > > > > > > +	dev->pdata->phy_control(1);
> > > > > > > > +
> > > > > > > > +	/*USB PHY0 Enable */
> > > > > > > 
> > > > > > > Wrong comment style.
> > > > > > 
> > > > > > I'll fix this one.
> > > > > > 
> > > > > > > > +	printf("USB PHY0 Enable\n");
> > > > > > > > +
> > > > > > > > +	/* Enable PHY */
> > > > > > > > +	writel(readl(usb_phy_ctrl) | USB_PHY_CTRL_EN0,
> > > > > > > > usb_phy_ctrl);
> > > > > > > 
> > > > > > > We have helpers for setting/clearing bits, right?
> > > > > > 
> > > > > > Yes we do, Lukasz ... ? :)
> > > > > 
> > > > > If you think about set_bit()/ clear_bit() functions
> > > > > from ./arch/arm/include/asm/bitops.h, then I don't mind to
> > > > > replace current code with them.
> > > > > 
> > > > > Feel free to use them for next version of the patch.
> > > > 
> > > > Pavel meant clrsetbits_le32() and friends . Personally, I would
> > > > leave that to subsequent set :)
> > > 
> > > This is up to you. I just would like to avoid changing too many
> > > things at once.
> > 
> > Definitelly. Did adding the missing include fix the issues with this
> > patch ?
> 
> I've took the topic/s3c-otg from u-boot-usb repo.
> 
> Reviewed-by: Lukasz Majewski <l.majewski at samsung.com>
> Tested-by: Lukasz Majewski <l.majewski at samsung.com>

Thanks, I'll queue that up for master then.