Re: [PATCH 2.6.32]: Add new device IDs and registers forMULTIMEDIA CARD (MMC), SECURE DIGITAL (SD) cards.

[Chris Ball <cjb@laptop.org>]

Hi Jennifer,

This is looking better, thanks -- just a few more questions:

On Tue, Nov 16, 2010 at 02:05:16PM +0800, Jennifer Li (TP) wrote:
> Root cause:
> O2Micro's ADMA and related functions can't work by default Linux SD
> driver. We need mark those related registers.

If the goal is to disable ADMA, have you tried using the BROKEN_ADMA
quirk?  Like this:

static const struct sdhci_pci_fixes sdhci_o2 = {
        .probe          = o2_probe,
        .quirks         = SDHCI_QUIRK_BROKEN_ADMA,
};

If you do this, do you still need to perform the PCI writes?

> +		ret = pci_read_config_byte(chip->pdev, O2_SDMMC_MULTI_VCC3V, &scratch);
> +		if (ret)
> +			return ret;
> +
> +		scratch = 0x08;
> +		ret = pci_write_config_byte(chip->pdev, O2_SDMMC_MULTI_VCC3V, scratch);

Here you read in a value to scratch, but then you go ahead and write
0x08 to the register regardless.  We can skip the read here, right?

> +		ret = pci_read_config_byte(chip->pdev, O2_SDMMC_CAPABILITIES, &scratch);
> +		if (ret)
> +			return ret;
> +
> +		scratch |= 0x01;
> +		ret = pci_write_config_byte(chip->pdev, O2_SDMMC_CAPABILITIES, scratch);
> +
> +		ret = pci_read_config_byte(chip->pdev, O2_SDMMC_CAPABILITIES, &scratch);
> +		if (ret)
> +			return ret;
> +
> +		scratch = 0x73;
> +		ret = pci_write_config_byte(chip->pdev, O2_SDMMC_CAPABILITIES, scratch);

Here you write a value to O2_SDMMC_CAPABILITIES that's dependent on the
read value, but then after that you immediately overwrite the register
with 0x73 regardless of what was in it before.  Why perform the first
read-and-write cycle and the second read, if you're just going to write
0x73 in the end?

> +
> +		ret = pci_read_config_byte(chip->pdev, O2_SDMMC_ADMA1, &scratch);
> +		if (ret)
> +			return ret;
> +
> +		scratch = 0x39;
> +		ret = pci_write_config_byte(chip->pdev, O2_SDMMC_ADMA1, scratch);
> +
> +		ret = pci_read_config_byte(chip->pdev, O2_SDMMC_ADMA2, &scratch);
> +		if (ret)
> +			return ret;
> +
> +		scratch = 0x08;
> +		ret = pci_write_config_byte(chip->pdev, O2_SDMMC_ADMA2, scratch);

Same as above -- if the write isn't dependent on the read, I don't see
why to bother with the read.  Also, you can avoid assigning the return
value of pci_write_config_byte() to "ret" now that we aren't testing that.

> Signed-off-by: Jennifer Li Developer <Jennifer.li@o2micro.com>

I don't think "Developer" should be here, since the text should just be
your name.

Thanks,

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child