From mboxrd@z Thu Jan  1 00:00:00 1970
From: eric.y.miao@gmail.com (Eric Miao)
Date: Fri, 29 Apr 2011 10:21:49 +0800
Subject: [PATCH v2 1/3] ARM: mmp: add pxa910 mmc resource
In-Reply-To: <BANLkTikePbf43ffyCdQzNM0kyTo6tYLDFg@mail.gmail.com>
References: <BANLkTi=XLp6hZ3H5rM6G32jhibz4xQo-Vg@mail.gmail.com>
	<20110428091110.GO17290@n2100.arm.linux.org.uk>
	<BANLkTinZfCzrM0+WomZ4cS2mkQRkty9qYA@mail.gmail.com>
	<BANLkTikePbf43ffyCdQzNM0kyTo6tYLDFg@mail.gmail.com>
Message-ID: <BANLkTinzr-ByRUy9YFpskPxA6VwMCieKLw@mail.gmail.com>
To: linux-arm-kernel@lists.infradead.org
List-Id: linux-arm-kernel.lists.infradead.org

On Fri, Apr 29, 2011 at 10:13 AM, Jun Nie <niej0001@gmail.com> wrote:
> 2011/4/28 Eric Miao <eric.y.miao@gmail.com>:
>> On Thu, Apr 28, 2011 at 5:11 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Thu, Apr 21, 2011 at 09:56:53AM +0800, Jun Nie wrote:
>>>> +static inline int pxa910_add_sdhost(int id, struct sdhci_pxa_platdata *data)
>>>> +{
>>>> + ? ? struct pxa_device_desc *d = NULL;
>>>> +
>>>> + ? ? switch (id) {
>>>> + ? ? case 0: d = &pxa910_device_sdh0; break;
>>>> + ? ? case 1: d = &pxa910_device_sdh1; break;
>>>> + ? ? case 2: d = &pxa910_device_sdh2; break;
>>>> + ? ? default:
>>>> + ? ? ? ? ? ? return -EINVAL;
>>>> + ? ? }
>>>> +
>>>> + ? ? return pxa_register_device(d, data, sizeof(*data));
>>>> +}
>>>
>>> How about:
>>>
>>> static inline int pxa910_add_sdhost(struct pxa_device_desc *dev,
>>> ? ? ? ?struct sdhci_pxa_platdata *data)
>>> {
>>> ? ? ? ?return pxa_register_device(dev, data, sizeof(*data));
>>> }
>>>
>>> #define pxa910_add_sdhost0(data) pxa910_add_sdhost(&pxa910_device_sdh0, data)
>>> #define pxa910_add_sdhost1(data) pxa910_add_sdhost(&pxa910_device_sdh1, data)
>>> #define pxa910_add_sdhost2(data) pxa910_add_sdhost(&pxa910_device_sdh2, data)
>>>
>>> instead - which seems more sane if you're going to call the function with
>>> constant ID values.
>>>
>>
>> Indeed. Better to revise the original code as well.
>>
>
> I am thinking to move QUIRKs into this function for it is SOC
> specific, not board specific. Another item I want to move is bus
> timing adjustment register setting API, which is for clock fine tune
> and have different address/detail definition for different PXAs.
> Zhangfei is preparing driver code for timing register setting.
> How do you think about below change?
>

Basically I'm good with the move. Yet we can still incorporate this change
into the function Russell suggested pxa910_add_sdh().

> @@ -110,6 +110,8 @@ static inline int pxa910_add_sdh(int id, struct
> sdhci_pxa_platdata *data)
> ? ? ? ? ? ? ? ?case 2: d = &pxa910_device_sdh2; break;
> ? ? ? ? ? ? ? ?default:
> ? ? ? ? ? ? ? ? ? ? ? ? ? return -EINVAL;
> ? ? ? ?}
>
> + ? ? ? data->quirks ? ?= SDHCI_QUIRK_BROKEN_ADMA,
> + ? ? ? data->soc_set_timing ? ? ? ? ? ?= pxa910_sdh_specific_ctrl,
> ? ? ? ?return pxa_register_device(d, data, sizeof(*data));
> ?}
>
> diff --git a/arch/arm/mach-mmp/ttc_dkb.c b/arch/arm/mach-mmp/ttc_dkb.c
> index d616d09..fdd3457 100644
> --- a/arch/arm/mach-mmp/ttc_dkb.c
> +++ b/arch/arm/mach-mmp/ttc_dkb.c
> @@ -1534,13 +1534,11 @@ static void mmc2_set_ops(struct sdhci_pxa *pxa)
>
> ?/* MMC0 controller for SD-MMC */
> ?static struct sdhci_pxa_platdata pxa910_sdh_platdata_mmc0 = {
> - ? ? ? .quirks ? ? ? ? ? ? ? ? = SDHCI_QUIRK_BROKEN_ADMA,
> ? ? ? ?.max_speed ? ? ? ? ? ? ?= 48000000,
> - ? ? ? .soc_set_timing ? ? ? ? = pxa910_sdh_specific_ctrl,
> ?};
>