Re: [PATCH] add support PXA168/PXA910/MMP2 SD Host Controller

[Wolfram Sang <w.sang@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3287 bytes --]

> >> >> diff --git a/drivers/mmc/host/sdhci-pxa.c b/drivers/mmc/host/sdhci-pxa.c
> >> >> new file mode 100644
> >> >> index 0000000..3e091c1
> >> >> --- /dev/null
> >> >> +++ b/drivers/mmc/host/sdhci-pxa.c
> >> >> @@ -0,0 +1,253 @@
> >> >> +/* linux/drivers/mmc/host/sdhci-pxa.c
> >> >> + *
> >> >> + * Copyright 2010 Marvell
> >> >> + *      Zhangfei Gao <zhangfei.gao@marvell.com>
> >> >> + *
> >> >> + * This program is free software; you can redistribute it and/or modify
> >> >> + * it under the terms of the GNU General Public License version 2 as
> >> >> + * published by the Free Software Foundation.
> >> >> + */
> >> >> +
> >> >> +/* Supports:
> >> >> + * SDHCI support for MMP2/PXA910/PXA168
> >> >> + *
> >> >> + * Based on sdhci-platfm.c
> >> >> + */
> >> >
> >> > Why is it only "based on" and not directly using it?
> >>
> >> Thanks for your suggestion.
> >> This is the first stage, already implement sd card function, we will
> >> have some patches later to enhance the driver.
> >> We have to access many private registers to enable specific control.
> >> Besides some sdio device need some specific control, like marvell 8787
> >> host sleep, it is much reasonable to implement inside the driver.
> >
> > Hmm, which hooks are missing for you to implement this as an extension
> > of the pltfm-driver? Maybe it makes sense to simply add this hook?
> 
> We refered several mmc host drivers, such as sdhci-s3c.c,
> sdhci-spear.c, and we want to align with others, which also meet pxa
> requirement and low risk to transfer from the existing driver.

I see. It is true that the pltfm-part is not widely used up to now. The
question is if it might be a good idea to change this? There have been
voices asking to merge the s3c-driver into the pltfm one. If you look at
sdhci-cns3xxx.c, it looks clearer and reduces code duplication. The
current state of the pltfm-driver might not cover all cases (yet), true,
but those should be at least identified IMHO. This will help us for
similar decisions in the future.

> We have three or four device in one soc with the same driver, for
> example one for sd, one for wifi, one for emmc, etc.
> Each device have specific clock provider, which should be opened at
> start, and closed when no operation to same power.
> Specific quirk is needed for different device, which should be
> transfered from platform data, for example on-chip wifi alwayes stay
> on chip and emmc may require sd_clk free running to init.
> The max_speed also may be different for board issue though controller
> support max is 50M, which could get from capability.

I guess to fully understand all constraints, one must really working
with your platform, what I don't do. I agree that a nicely working
driver is better than no driver; however, I fear once a driver hit the
mainline being non-pltfm, it will hardly be converted later, even if it
was considered to be worthwhile. So this is why I ask initially if it
couldn't be done.

Chris, do you see a rule of thumb here? Or what are your preferences?

Kind regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]