Re: [PATCH V4 1/3] mmc: sdhci: Add size for caller in init+register

Re: [PATCH V4 1/3] mmc: sdhci: Add size for caller in init+register

[Christian Daudt]

Hi Chris,
  Can you pls take this patch and the one named "[PATCH V4 2/3] ARM: 
mmc: bcm281xx SDHCI driver" into your tree ?

  Thanks,
     csd.

On 13-05-29 01:50 PM, Christian Daudt wrote:
> Add a param to allow users of sdhci_pltfm to allocate private space
> in calls to sdhci_pltfm_init+sdhci_pltfm_register. This is implemented
> in the same way as sdhci does for its users.
>   None of the users have been migrated yet and are passing in zero to
> retain their private allocation.
>
> - todo: migrate clients to using allocation this way
> - todo: remove priv variable once migration is complete
>
> Also removed unused variable in sdhci_pltfm_init fn
>
> Signed-off-by: Christian Daudt <csd@broadcom.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Re: [PATCH V4 2/3] ARM: mmc: bcm281xx SDHCI driver

[Chris Ball]

Hi Christian,

On Wed, May 29 2013, Christian Daudt wrote:
> +	/* if device is eMMC, emulate card insert right here */
> +	if (kona_dev->non_removable) {
> +		ret = sdhci_bcm_kona_sd_card_emulate(host, 1);
> +		if (ret) {
> +			dev_err(dev,
> +				"unable to emulate card insertion\n");
> +			goto err_remove_host;
> +		}
> +	} else if (gpio_is_valid(kona_dev->cd_gpio)) {
> +		ret = devm_gpio_request(dev, kona_dev->cd_gpio, "sdio cd");
> +		if (ret < 0) {
> +			dev_err(mmc_dev(host->mmc),
> +				"Unable to request GPIO pin %d\n",
> +				kona_dev->cd_gpio);
> +			goto err_remove_host;
> +		}
> +
> +		gpio_direction_input(kona_dev->cd_gpio);
> +
> +		/* Set debounce for SD Card detect to maximum value (128ms)
> +		 *
> +		 * NOTE-1: If gpio_set_debounce() returns error we still
> +		 * continue with the default debounce value set. Another reason
> +		 * for doing this is that on rhea-ray boards the SD Detect GPIO
> +		 * is on GPIO Expander and gpio_set_debounce() will return error
> +		 * and if we return error from here, then probe() would fail and
> +		 * SD detection would always fail.
> +		 *
> +		 * NOTE-2: We also give a msleep() of the "debounce" time here
> +		 * so that we give enough time for the debounce to stabilize
> +		 * before we read the gpio value in gpio_get_value_cansleep().
> +		 */
> +		ret = gpio_set_debounce(kona_dev->cd_gpio,
> +				(SD_DETECT_GPIO_DEBOUNCE_128MS * 1000));
> +		if (ret < 0) {
> +			dev_err(mmc_dev(host->mmc),
> +				"%s: gpio set debounce failed."
> +				"default debounce value assumed\n", __func__);
> +		}
> +
> +		/* Sleep for 128ms to allow debounce to stabilize */
> +		msleep(SD_DETECT_GPIO_DEBOUNCE_128MS);
> +		/* request irq for cd_gpio after the gpio debounce is
> +		 * stabilized, otherwise, some bogus gpio interrupts might be
> +		 * triggered.
> +		 */
> +		irq = gpio_to_irq(kona_dev->cd_gpio);
> +		ret = devm_request_threaded_irq(dev,
> +				irq,
> +				NULL,
> +				sdhci_bcm_kona_pltfm_cd_interrupt,
> +				IRQF_TRIGGER_FALLING|
> +				IRQF_TRIGGER_RISING |
> +				IRQF_ONESHOT |
> +				IRQF_NO_SUSPEND, "sdio cd", host);
> +		if (ret) {
> +			dev_err(mmc_dev(host->mmc),
> +				"Failed irq %d request for gpio=%d ret=%d\n",
> +				gpio_to_irq(kona_dev->cd_gpio),
> +				kona_dev->cd_gpio, ret);
> +			goto err_remove_host;
> +		}
> +		if (gpio_is_valid(kona_dev->wp_gpio)) {
> +			ret = devm_gpio_request(dev,
> +				kona_dev->wp_gpio, "sdio wp");
> +			if (ret < 0) {
> +				dev_err(&pdev->dev,
> +					"Unable to request WP pin %d\n",
> +					kona_dev->wp_gpio);
> +				kona_dev->wp_gpio = -1;
> +			} else {
> +				gpio_direction_input(kona_dev->wp_gpio);
> +			}
> +		}

Could you investigate replacing this section of code with a call to
mmc_of_parse(), please?  It should be able to replace your parsing
and handling of bus-width, cd-gpios, wp-gpios, and non-removable
(other than the initialization quirks specific to your hardware).

I'd like to avoid every driver having its own DT parsing.
Thanks,

- Chris.
-- 
Chris Ball   <cjb-2X9k7bc8m7Mdnm+yROfE0A@public.gmane.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH V4 2/3] ARM: mmc: bcm281xx SDHCI driver

[Christian Daudt]

Hi Chris,
On 13-05-31 09:15 AM, Chris Ball wrote:
>
> Could you investigate replacing this section of code with a call to
> mmc_of_parse(), please?  It should be able to replace your parsing
> and handling of bus-width, cd-gpios, wp-gpios, and non-removable
> (other than the initialization quirks specific to your hardware).
>
> I'd like to avoid every driver having its own DT parsing.
> Thanks,
>
> - Chris.
I looked into it and it can replace just about everything that our own 
dt parsing code is doing. The only problem is with cd_gpio. If I call 
mmc_of_parse, then it will install its own irq handler for cd gpio, and 
our driver needs a different handler. the mmc irq handler can call a 
card_event callback, but that is already pointing to sdhci_card_event. 
So the only way I can use mmc_of_parse that I can see is by adding a 
flag to it to skip cd_gpio parsing which only this driver will then set 
to true. Any other ideas ?

  Thanks,
    csd

Re: [PATCH V4 2/3] ARM: mmc: bcm281xx SDHCI driver

[Arnd Bergmann]

On Saturday 01 June 2013, Christian Daudt wrote:
> I looked into it and it can replace just about everything that our own 
> dt parsing code is doing. The only problem is with cd_gpio. If I call 
> mmc_of_parse, then it will install its own irq handler for cd gpio, and 
> our driver needs a different handler. the mmc irq handler can call a 
> card_event callback, but that is already pointing to sdhci_card_event. 
> So the only way I can use mmc_of_parse that I can see is by adding a 
> flag to it to skip cd_gpio parsing which only this driver will then set 
> to true. Any other ideas ?

Can't you add a callback handler into sdhci_card_event that can call
a platform specific function?

	Arnd

Re: [PATCH V4 2/3] ARM: mmc: bcm281xx SDHCI driver

[Christian Daudt]

On 13-06-01 01:13 PM, Arnd Bergmann wrote:
> On Saturday 01 June 2013, Christian Daudt wrote:
>> I looked into it and it can replace just about everything that our own
>> dt parsing code is doing. The only problem is with cd_gpio. If I call
>> mmc_of_parse, then it will install its own irq handler for cd gpio, and
>> our driver needs a different handler. the mmc irq handler can call a
>> card_event callback, but that is already pointing to sdhci_card_event.
>> So the only way I can use mmc_of_parse that I can see is by adding a
>> flag to it to skip cd_gpio parsing which only this driver will then set
>> to true. Any other ideas ?
> Can't you add a callback handler into sdhci_card_event that can call
> a platform specific function?
>
>
I can add another layer of callbacks. I have to first check if I can 
rework our driver to fit into this callback model. I'll update this when 
I've tried it.

  thanks,
    csd