[PATCH for-4.11 1/2] Revert "phy: Add USB3 PHY support for Broadcom NSP SoC"

[Kishon Vijay Abraham I]

On Wednesday 08 March 2017 05:56 PM, Kishon Vijay Abraham I wrote:
> Hi Rafal,
> 
> On Wednesday 08 March 2017 05:43 PM, Rafał Miłecki wrote:
>> On 10 February 2017 at 01:27, Jon Mason <jon.mason at broadcom.com> wrote:
>>> On Thu, Feb 9, 2017 at 2:18 PM, Florian Fainelli <f.fainelli at gmail.com>
>>> wrote:
>>>>
>>>> On 02/08/2017 11:21 PM, Rafał Miłecki wrote:
>>>>> On 2017-02-09 00:44, Florian Fainelli wrote:
>>>>>> On 02/08/2017 03:39 PM, Rafał Miłecki wrote:
>>>>>>> On 2017-02-09 00:32, Florian Fainelli wrote:
>>>>>>>> On 02/08/2017 03:30 PM, Rafał Miłecki wrote:
>>>>>>>>> From: Rafał Miłecki <rafal at milecki.pl>
>>>>>>>>>
>>>>>>>>> This reverts commit d7bc1a7d41bf ("phy: Add USB3 PHY support for
>>>>>>>>> Broadcom NSP SoC") as we already have driver for this PHY (shared
>>>>>>>>> by NS
>>>>>>>>> and NSP). It was added in commit e5666281d9ea ("phy: bcm-ns-usb3:
>>>>>>>>> new
>>>>>>>>> driver for USB 3.0 PHY on Northstar").
>>>>>>>>>
>>>>>>>>> Instead of adding separated driver & duplicating code we should
>>>>>>>>> work on
>>>>>>>>> improving existing (old) one. Thanks to work done by Broadcom we
>>>>>>>>> know
>>>>>>>>> there is MDIO bus we weren't aware of & we know register names which
>>>>>>>>> makes initialization more clear. This is very valuable info and we
>>>>>>>>> should work on using it in existing driver afterwards.
>>>>>>>>
>>>>>>>> Should not we first extend the old driver to support NSP and then
>>>>>>>> revert
>>>>>>>> d7bc1a7d41bf ("phy: Add USB3 PHY support for Broadcom NSP SoC")?
>>>>>>>
>>>>>>> Sounds like a weird / dirty development method to me: adding
>>>>>>> duplicated
>>>>>>> code
>>>>>>> first then working on cleaning it. Unless you mean drivers/staging/.
>>>>>>
>>>>>> There was clearly a mistake in submitting this NSP USB PHY driver, and
>>>>>> it should have been a patch against the existing NS USB PHY driver, but
>>>>>> it was not, okay fair enough.
>>>>>>
>>>>>> It's one thing to address that in the future, and it's another thing to
>>>>>> flat out revert the driver just because you don't like the duplication.
>>>>>>
>>>>>> I don't like that either, and we can discuss on how to improve things
>>>>>> (like have the maintainer review that too), but duplication is a lesser
>>>>>> evil than not having the hardware supported at all, and even more so,
>>>>>> purposely reverting in the name of removing that duplication, that's
>>>>>> intentionally breaking working hardware!
>>>>>
>>>>> Hardware support is not excuse and I don't think it ever was in the
>>>>> Linux.
>>>>>
>>>>> We don't accept badly designed drivers just because they provide new hw
>>>>> support.
>>>>> We have various standards (for quality, style, design, code) at kernel
>>>>> and we
>>>>> stick to them unless it's drivers/staging/. As you said this driver
>>>>> shouldn't be
>>>>> pushed in the first place.
>>>>>
>>>>> Dropping hardware support in kernel happens. Sometimes it's about
>>>>> ancient
>>>>> devices, sometimes about code quality (some forgotten staging drivers
>>>>> used to be
>>>>> dropped AFAIK).
>>>>>
>>>>> Additionally you're talking about support that was *just* added and
>>>>> isn't used
>>>>> by anyone in the wild world yet.
>>>>
>>>> Except people working on it at Broadcom, but fair enough.
>>>>
>>>>>
>>>>> This hardware was missing upstream support for 4 years so 2 extra months
>>>>> won't
>>>>> really hurt anyone.
>>>>>
>>>>> I really don't see excusee or need for keeping this driver.
>>>>>
>>>>> If you want to (and you feel it's well designed), we can keep
>>>>> brcm,nsp-usb3-phy.txt
>>>>
>>>> No it's fine, let's drop it all and replace it with whatever you and Jon
>>>> come up with next.
>>>>
>>>>>
>>>>> I vote for focusing on existing driver improvements instead of looking
>>>>> for
>>>>> excuses for keeping driver that shouldn't be added in the first place.
>>>>> Jon seems to be already working on this, I'm willing to help him, I'm
>>>>> sure we
>>>>> can get you a proper support for the next merge window.
>>>>
>>>> Fair enough, I dropped Dhanajay's changes ("ARM: dts: NSP: Add USB nodes
>>>> for Northstar Plus") from devicetree/next so you and Jon can figure out
>>>> what is the best thing to move forward and we minimize the amount of
>>>> incompatible DT stuff to be sorted out later on. So as far as I am
>>>> concerned, there are no board/SoC DTS changes to be patched later on, we
>>>> could re-apply this patch as-is, or we could have to define a new binding.
>>>
>>> Per the discussion with Rafal, this is acceptable
>>>
>>> Acked-by: Jon Mason <jon.mason at broadcom.com>
>>
>> Hi Kishon, what's the status of this?
> 
> Will be merging this in a day or so.

merged, thanks.

-Kishon
> 
> Thanks
> Kishon
>