On 6/07/19 2:20 PM, Angelo Dureghello wrote:
> Hi Adrian,
> 
> On Tue, Jul 02, 2019 at 12:10:54PM +0300, Adrian Hunter wrote:
>> On 20/06/19 1:22 AM, Angelo Dureghello wrote:
>>> Hi Christoph,
>>>
>>> On Sun, Jun 16, 2019 at 11:58:07PM -0700, Christoph Hellwig wrote:
>>>> On Sun, Jun 16, 2019 at 10:48:21PM +0200, Angelo Dureghello wrote:
>>>>> This driver has been developed as a separate module starting
>>>>> from the similar sdhci-esdhc-fls.c.
>>>>> Separation has been mainly driven from change in endianness.
>>>>
>>>> Can't we have a way to define the endianess at build or even runtime?
>>>> We have plenty of that elsewhere in the kernel.
>>>
>>> well, the base sdhci layer wants to access byte-size fields of the
>>> esdhc controller registers.
>>> But this same Freescale esdhc controller may be found in 2
>>> flavors, big-endian or little-endian organized.
>>> So in this driver i am actually correcting byte-addresses to
>>> access the wanted byte-field in the big-endian hw controller.
>>>
>>> So this is a bit different from a be-le endian swap of a
>>> long or a short that the kernel is organized to do..
>>
>> Did you consider just using different sdhci_ops so that you could support
>> different sdhci I/O accessors?
> 
> Initially i tried to modify the IMX/Vybrid (arm) driver. But was stopped from
> several points, trying to remember now, 
> 
> - the I/O accessors was a const struct, but this of course is not a big 
>   issue,
> - here and there bitfields and positions of the ColdFire controller
>   registers, compared to the arm versions, are different, so controller hw
>   is not exactly the same,
> - on ColdFire controller and some behaviors and errata are different,
> - dma endiannes not hw-configurable,
> - ColdFire has max clock limitations, a bit different clock init.
> 
> Finally, since there is already a common library (shdci.c) i decided to go
> as a separate driver, instead of filling the driver of "if (coldfire)" also 
> mainly for the following reasons:
> 
> 1) separated ColdFire driver has a quite small amount of code, simple to
> understand.
> 2) having drivers used by multiple architectures, it add risks, each time
> i perform a change, i can test it only on ColdFire, and can break
> the driver for the other architectures (i see this not rarely happening for
> multiple-arch used drivers).
> 
> So let me know if the way chosen can be ok. Otherwise i will roll back 
> trying to modify the iMX/Vybrid driver, likely adding a lot of "if (coldfire)"
> complicating it quite a lot.

"if (coldfire)" is not very nice, and there doesn't seem to be that much
common code, so let's stick with a separate driver, but please change the
commit message in consideration of this discussion.

Reply via email to