[U-Boot] [PATCH v3 03/30] sunxi: Fix USB PHY index for H3/H5/A64

[Marek Vasut]

On 01/28/2018 07:40 PM, Marek Vasut wrote:
> On 01/28/2018 07:29 PM, Jagan Teki wrote:
>> On Sun, Jan 28, 2018 at 11:55 PM, Marek Vasut <marex at denx.de> wrote:
>>> On 01/28/2018 07:20 PM, Jagan Teki wrote:
>>>> On Sun, Jan 28, 2018 at 10:01 PM, Marek Vasut <marex at denx.de> wrote:
>>>>> On 01/28/2018 05:19 PM, Jagan Teki wrote:
>>>>>> From: Chen-Yu Tsai <wens at csie.org>
>>>>>>
>>>>>> On the new chips such as H3, H5, and A64, the USB OTG controller is
>>>>>> paired with a set of proper EHCI/OHCI USB hosts. To enable these hosts,
>>>>>> the USB PHY index count has to be reworked to start from this pair.
>>>>>>
>>>>>> This patch reworks the USB clock gate and reset indices, and how the
>>>>>> USB host is mapped to a USB phy, for the newer chips.
>>>>>
>>>>> The ifdeffery is awful. The driver is DT capable, do why don't you
>>>>> detect the block type / soc type from DT and handle this dynamically
>>>>> instead of adding ifdefs ?
>>>>
>>>> Though this driver is DT capable phy, reset, clock and other still
>>>> need to have have it. till now we are relying on ifdef's to move
>>>> feature to work first.
>>
>>>
>>> This statement makes no sense, just use the DT compatible to discern the
>>> block type and get rid of statements like this:
>>>
>>> +#if defined(CONFIG_MACH_SUNXI_H3_H5) || defined(CONFIG_MACH_SUN50I)
>>> +       /* Newer chips have a EHCI/OHCI host pair for OTG host mode */
>>> +       priv->phy_index = ((uintptr_t)hccr - SUNXI_USB0_BASE) / BASE_DIST;
>>> +#else
>>>         priv->phy_index = ((uintptr_t)hccr - SUNXI_USB1_BASE) / BASE_DIST;
>>> +#endif
>>
>> if you read it previous comment clearly for "phy, reset, clock" writes
>> it may not possible and true for above ifdef.
> 
> I only see if (platform is foo or bar) set phy_index to something else
> set it to something else, therefore I think the comment about 'phy,
> reset, clock' is irrelevant.
> 
> You can very well add ie.
> { .compatible = "allwinner,sun5i-a13-ohci", .data = TYPE_FOO },
> 
> And then do
> 
> if (dev_get_driver_data(...) == TYPE_FOO)
>   priv->phy_index = bar;
> else
>   priv->.......;
> 
> If you think this is not possible, please do explain why in detail.

Can I expect a new patchset using DT properly ?

-- 
Best regards,
Marek Vasut