Re: [RFC PATCH 1/6] usb: musb: omap: Configure OTG_INTERFSEL for proper charger detection

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

 



Sergei,

Thanks for your comments.

On Fri, Sep 16, 2011 at 3:18 PM, Sergei Shtylyov <sshtylyov@xxxxxxxxxx> wrote:
> Hello.
>
> On 15-09-2011 18:19, Kishon Vijay Abraham I wrote:
>
>> Setting OTG_INTERFSEL to UTMI interferes with charger detection resulting
>> in incorrect detection of charger type. Hence OTG_INTERFSEL is configured
>> to
>> ULPI initially and changed to UTMI only after receiving USB_EVENT_ID or
>> USB_EVENT_VBUS notification.
>
>> Signed-off-by: Kishon Vijay Abraham I<kishon@xxxxxx>
>> Signed-off-by: Balaji T K<balajitk@xxxxxx>
>
>   AFAIK, full name is needed here.

is it not the prerogative of the person giving his signed-off by??

>
>> [sandesh.gowda@xxxxxx: The interface selection has to be done before the
>> PHY
>> is activated and is not allowed to change when the PHY clock is already
>> running]
>
>   No signoff from him?

ok. will add his signoff.
>
>> ---
>>  drivers/usb/musb/omap2430.c |   50
>> +++++++++++++++++++++++++++++++++++-------
>>  1 files changed, 41 insertions(+), 9 deletions(-)
>
>> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
>> index fc9377c..724450b 100644
>> --- a/drivers/usb/musb/omap2430.c
>> +++ b/drivers/usb/musb/omap2430.c
>> @@ -228,6 +228,7 @@ static inline void omap2430_low_level_init(struct musb
>> *musb)
>>  static int musb_otg_notifications(struct notifier_block *nb,
>>                unsigned long event, void *unused)
>>  {
>> +       u32 val;
>>        struct musb     *musb = container_of(nb, struct musb, nb);
>>        struct device *dev = musb->controller;
>>        struct musb_hdrc_platform_data *pdata = dev->platform_data;
>> @@ -240,11 +241,32 @@ static int musb_otg_notifications(struct
>> notifier_block *nb,
>>                if (is_otg_enabled(musb)) {
>>                        if (musb->gadget_driver) {
>>                                pm_runtime_get_sync(musb->controller);
>> +
>> +                               val = musb_readl(musb->mregs,
>> OTG_INTERFSEL);
>> +                               if (data->interface_type ==
>> +
>> MUSB_INTERFACE_UTMI) {
>> +                                       val&= ~ULPI_12PIN;
>> +                                       val |= UTMI_8BIT;
>> +                               } else {
>> +                                       val |= ULPI_12PIN;
>> +                               }
>> +                               musb_writel(musb->mregs, OTG_INTERFSEL,
>> val);
>> +
>>                                usb_phy_init(musb->xceiv);
>>                                omap2430_musb_set_vbus(musb, 1);
>>                        }
>>                } else {
>>                        pm_runtime_get_sync(musb->controller);
>> +
>> +                       val = musb_readl(musb->mregs, OTG_INTERFSEL);
>> +                       if (data->interface_type == MUSB_INTERFACE_UTMI) {
>> +                               val&= ~ULPI_12PIN;
>> +                               val |= UTMI_8BIT;
>> +                       } else {
>> +                               val |= ULPI_12PIN;
>> +                       }
>> +                       musb_writel(musb->mregs, OTG_INTERFSEL, val);
>> +
>>                        usb_phy_init(musb->xceiv);
>>                        omap2430_musb_set_vbus(musb, 1);
>>                }
>> @@ -255,6 +277,16 @@ static int musb_otg_notifications(struct
>> notifier_block *nb,
>>
>>                if (musb->gadget_driver)
>>                        pm_runtime_get_sync(musb->controller);
>> +
>> +               val = musb_readl(musb->mregs, OTG_INTERFSEL);
>> +               if (data->interface_type == MUSB_INTERFACE_UTMI) {
>> +                       val&= ~ULPI_12PIN;
>> +                       val |= UTMI_8BIT;
>> +               } else {
>> +                       val |= ULPI_12PIN;
>> +               }
>> +               musb_writel(musb->mregs, OTG_INTERFSEL, val);
>> +
>>                usb_phy_init(musb->xceiv);
>>                break;
>
>   Why repeat the same sequence 3 times. Couldn't you factor it out into a
> subroutine?

I thought it will be unnecessary to to add a subroutine just for a
single register modification.
Since you have pointed that out, I'll add a subroutine for it.

>
>> @@ -272,6 +304,11 @@ static int musb_otg_notifications(struct
>> notifier_block *nb,
>>                                otg_set_vbus(musb->xceiv->otg, 0);
>>                }
>>                usb_phy_shutdown(musb->xceiv);
>> +
>> +               val = musb_readl(musb->mregs, OTG_INTERFSEL);
>> +               val |= ULPI_12PIN;
>> +               musb_writel(musb->mregs, OTG_INTERFSEL, val);
>> +
>>                break;
>>        default:
>>                dev_dbg(musb->controller, "ID float\n");
>> @@ -304,16 +341,11 @@ static int omap2430_musb_init(struct musb *musb)
>>                goto err1;
>>        }
>>
>> +       /* Set OTG_INTERFSEL to ULPI for correct charger detection.
>> +        * This should be later set to UTMI.
>> +        */
>>        l = musb_readl(musb->mregs, OTG_INTERFSEL);
>> -
>> -       if (data->interface_type == MUSB_INTERFACE_UTMI) {
>> -               /* OMAP4 uses Internal PHY GS70 which uses UTMI interface
>> */
>> -               l&= ~ULPI_12PIN;       /* Disable ULPI */
>> -               l |= UTMI_8BIT;         /* Enable UTMI  */
>> -       } else {
>> -               l |= ULPI_12PIN;
>> -       }
>> -
>> +       l |= ULPI_12PIN;
>>        musb_writel(musb->mregs, OTG_INTERFSEL, l);
>
>   This also seems to be the same code repeated twice...

Ok. So i'll create a new sub-routine which takes interface type as
argument and set OTG_INTERFSEL to ULPI_12PIN or UTMI_8BIT based on the
interface type.

Regards
Kishon
>
> WBR, Sergei
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux