Re: [PATCH 1/2] mmc: sdhci: Replace SDHCI_QUIRK_FORCE_BLK_SZ_2048 with a pklatform hook.

[Wolfram Sang]

On Mon, Feb 07, 2011 at 05:54:24PM +0000, Chris Ball wrote:
> Hi Wolfram, thanks for the review,
> 
> On Mon, Feb 07, 2011 at 06:40:39PM +0100, Wolfram Sang wrote:
> > On Sun, Feb 06, 2011 at 01:13:10AM -0500, Chris Ball wrote:
> > > Part of a quirk cleanup run.  This quirk was only used by sdhci-esdhc.
> > > This patch is untested.
> > > 
> > > Signed-off-by: Chris Ball <c...@laptop.org>
> > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > > index 9e15f41..fcd6188 100644
> > > --- a/drivers/mmc/host/sdhci.c
> > > +++ b/drivers/mmc/host/sdhci.c
> > > @@ -1962,8 +1962,8 @@ int sdhci_add_host(struct sdhci_host *host)
> > >    * Maximum block size. This varies from controller to controller and
> > >    * is specified in the capabilities register.
> > >    */
> > > - if (host->quirks & SDHCI_QUIRK_FORCE_BLK_SZ_2048) {
> > > -         mmc->max_blk_size = 2;
> > > + if (host->ops->get_max_blk_size) {
> > > +         mmc->max_blk_size = host->ops->get_max_blk_size(host);
> > >   } else {
> > >           mmc->max_blk_size = (caps & SDHCI_MAX_BLOCK_MASK) >>
> > >                           SDHCI_MAX_BLOCK_SHIFT;
> > 
> > I tend to think this could be fixed using io-accessors when reading the
> > caps register?
> 
> Hm, I agree that it would work, but I'm not sure it's going to be
> cleaner/more readable that way.  I like that here we have one place
> for setting the max_blk_size, and it's obvious from the code exactly
> where you need to look to see if it's been overloaded by the driver.

Without the quirk and its handling, we lose the if-else-block which will
lead to:

        mmc->max_blk_size = (caps & SDHCI_MAX_BLOCK_MASK) >>
                        SDHCI_MAX_BLOCK_SHIFT;
        if (mmc->max_blk_size >= 3) {
                printk(KERN_WARNING "%s: Invalid maximum block size, "
                        "assuming 512 bytes\n", mmc_hostname(mmc));
                mmc->max_blk_size = 0;
        }

This is what I would prefer. Straight code, handling an sdhci-controller
adhering to the standard. A controller failing to do that, will have to take
care of that in its specific source file. For the sdhci-core, it would then
simply look like a "good" controller. We probably can't achieve this for all
quirks, but still...

Regards,

   Wolfram

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

signature.asc
Description: Digital signature