[U-Boot] [PATCH v3 5/5] ARM: dm: spi: Add support DM/DTS for i.MX28 mxs SPI driver (DM_SPI conversion)

[Marek Vasut]

On 6/17/19 3:41 PM, Lukasz Majewski wrote:
> On Mon, 17 Jun 2019 15:23:55 +0200
> Marek Vasut <marex at denx.de> wrote:
> 
>> On 6/17/19 2:27 PM, Lukasz Majewski wrote:
>>> Hi Marek,
>>>   
>>>> On 6/17/19 8:49 AM, Lukasz Majewski wrote:  
>>>>> Hi Marek,
>>>>>     
>>>>>> On 6/16/19 12:34 AM, Lukasz Majewski wrote:    
>>>>>>> This commit converts mxs_spi driver to support DM/DTS.
>>>>>>>
>>>>>>> Signed-off-by: Lukasz Majewski <lukma at denx.de>      
>>>>>>
>>>>>> Is the non-DM part needed for anything ?     
>>>>>
>>>>> Do you mean the non-DM part of the mxs_gpio driver? Yes, it is
>>>>> used by not converted boards.    
>>>>
>>>> This is a SPI driver though.
>>>>  
>>>>>> I recall the SPL jumps back
>>>>>> to BootROM when loading the U-Boot proper. So if not, drop it.
>>>>>>
>>>>>> Also, please don't do partial conversion for iMX28 only, do the
>>>>>> iMX23 part as well, it cannot be hard.    
>>>>>
>>>>> Maybe it is not hard, but I cannot test it properly as I don't
>>>>> have i.MX23 device. If you are offering your help with testing
>>>>> (i.e. you do have the access to i.MX23 device and you will test
>>>>> those changes) I can add support for it.
>>>>>
>>>>> Otherwise, NO, I will not add ANY new untested code.    
>>>>
>>>> In general, you don't have to add any code, the iMX23/28 SPI IP is
>>>> very much the same hardware, DTTO for most of the other blocks. If
>>>> there are any differences between iMX23/28, they are already
>>>> handled in the existing driver(s).
>>>>
>>>> Half-way converted drivers in fact increase maintenance burden,
>>>> because then we have to deal with two different variants of the
>>>> code, instead of only one.  
>>>
>>> I cannot agree with this sentence.  
>>
>> Do you think maintaining - one DM driver which supports both iMX23 and
>> iMX28 - is more burden than maintaining - one driver which supports
>> DM, but only for iMX28 and non-DM for iMX23 and iMX28 ? I don't think
>> so.
>>
>>> The conversion would be done for
>>> i.MX28, which is then tested and validated (and clearly stated in
>>> the cover letter/commit message that only supported was i.MX28).>
>>> If I don't need to adjust common, reused code (which already
>>> supports both variants as it is the case with mxs_spi.c), then I
>>> don't mind.  
>>
>> Well, that is what I said above, you don't.
> 
> To make myself clear - If I can reuse the common code (which supports
> both imx23 and 28) for DM/DTS conversion then I'm OK with doing so.
> 
> If you require me to add untested code specific to i.MX23 - then NO.

Yes, you can.

>>
>>> For more intrusive changes - the driver needs to be tested and
>>> validated (by somebody who has the HW for testing).  
>>
>> That's up to board maintainers.
>>
>>>> That's why I would like to see this practice go
>>>> away wherever possible, and in this case it is possible.  
>>>
>>> In this particular case it is possible to add support for both as
>>> SoC specific changes (i.MX23 vs i.MX28) is performed in common code
>>> (e.g. mxs_spi_xfer_dma).  
>>
>> Both SPI and DMA blocks are basically the same on iMX23 and iMX28.
> 
> If I can reuse the common code, then I'm fine to do it.
> 
>>
>>>> If you need someone to test your changes, CC the board maintainers,
>>>> that's standard practice.  
>>>
>>> As fair as I remember only Angelo and Michael had also interest in
>>> testing converted code for i.MX28 based board.
>>>
>>> There was NO reply from other people when this (and few others)
>>> driver was marked as DEPRECATED.  
>>
>> Well, too bad, clearly the interest in this platform is low.
> 
> This means that people are using either some old U-Boot version, or
> there are a few people who want to refurbish the old HW with new code
> (e.g. Michael, Angelo).

Yes

>> That does not mean we should do sub-par upstream work, does it ?
>>
> 
> We shall not add untested code.

You cannot test every single platform in existence. Post patches and let
board maintainers test them ; if they won't, too bad, their platform
might become broken. There's no other way to move forward without
dragging behind a tremendous amount of ancient baggage, and that in turn
is not viable.

-- 
Best regards,
Marek Vasut