Re: [PATCH 2/2] sdhci-pxa: add call back interface to share sdhci-pxa

From: zhangfei gao <zhangfei.gao@gmail.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Philip Rakity <prakity@marvell.com>,
	Wolfram Sang <w.sang@pengutronix.de>, Chris Ball <cjb@laptop.org>,
	"linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
	Jun Nie <njun@marvell.com>, Raymond Wu <xywu@marvell.com>,
	Haojian Zhuang <haojian.zhuang@gmail.com>,
	Mark Brown <mark.brown314@gmail.com>
Subject: Re: [PATCH 2/2] sdhci-pxa: add call back interface to share sdhci-pxa
Date: Thu, 19 May 2011 07:34:29 -0400	[thread overview]
Message-ID: <BANLkTinGQLPD4LmjiSjtswqJ19xtvimjUw@mail.gmail.com> (raw)
In-Reply-To: <201105182238.45614.arnd@arndb.de>

On Wed, May 18, 2011 at 4:38 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 17 May 2011, zhangfei gao wrote:
>> >> The code handle several register difference are located at
>> >> arch/arm/mach-mmp/mmp2.c is for mmp2
>> >> arch/arm/mach-mmp/pxa910.c is for pxa910 serious,
>> >> arch/arm/mach-mmp/mmp3.c is for mmp3 serious, considering there may
>> >> still registers changing.
>> >>
>> >> The board difference are directly put in board.c
>> >> For example ttc_dkb do not use wp pin, so get_ro is provided.
>> >
>> > embedding the code in these chip files stops code sharing.  For example,
>> > mmp3 has same controller as mmp2.
>>
>> If register are totally same, they can share.
>> If there is minor difference with chip upgrading, they can put in
>> different file.
>
> When we talked about similar problems in drivers during the Linaro Developer
> Summit, the broad consensus was to move code from arch/arm into individual
> device drivers, and I think this should be done for this driver too.
>
> My recommendation here would be to have no callbacks in the platform
> data at all, but instead have the code for all variants in the
> sdhci-pxa driver itself. You can still have a structure to describe
> the differences using function pointers, but it's better if that is
> part of the driver itself. In the platform data, provide a way
> to identify the variant of the controller (e.g. using an enum)
> and point to the base addresses as required.
>
>        Arnd
>
Hi, Arnd

Thanks for your suggestion.

To make sdhci-pxa easy to maintain, we still prefer put platform
specific difference under platform code.

"Identify controller from platform data" looks to me is same as using #ifdef.
The method is used in our another driver before.
More and more workaround comes when more and more platform need to
support, making the driver bigger and bigger and not easy to maintain.
Nobody remember what's the workaround purpose several years later.
So the result is we have to re-write the driver again to make it simple :(

Currently, the ip is shared among mmp2, pxa910, pxa168 etc, pxa910
maintainer does not care what workaround is used on pxa168.
When mmp3 wants to reuse the ip, it's easier to not care what's the
history at all.

So we still prefer keep driver as simple as possible, while specific
platform self-maintain specific workaround , which is not aware to
other platform.

Thanks
>