[U-Boot] [PATCH 1/2] mmc: sunxi: Support cd-inverted DT property

[Maxime Ripard]

Hi Tuomas,

On Wed, Dec 20, 2017 at 03:34:47PM +0200, Tuomas Tynkkynen wrote:
> > > @@ -606,8 +607,15 @@ static int sunxi_mmc_probe(struct udevice
> > > *dev)
> > >  	if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv-
> > > >cd_gpio,
> > >  				  GPIOD_IS_IN)) {
> > >  		int cd_pin = gpio_get_number(&priv->cd_gpio);
> > > +		int cd_state = priv->cd_gpio.flags &
> > > GPIOD_ACTIVE_LOW ? 0 : 1;
> > 
> > Isn't that change itself enough to get what you wanted?
> > 
> > You really shouldn't be doing anything else.
> 
> Was that the correct part you meant to quote? The cd_state
> was only for the somewhat pull-up change... Sorry about that,
> here's the minimal (probably whitespace-damaged) change:

Sorry if it wasn't really clear, I actually wanted to say that
cd-inverted is deprecated, and you should just teach about the gpio
flags like you did.

The rest seems superfluous, especially since I'd expect the
combination of our GPIO flags and cd-inverted in our DTs to be buggy
in most cases.

> diff --git a/arch/arm/dts/sun7i-a20-pcduino3.dts b/arch/arm/dts/sun7i-a20-pcduino3.dts
> index 37b1e0ee9b..1a8b39be1d 100644
> --- a/arch/arm/dts/sun7i-a20-pcduino3.dts
> +++ b/arch/arm/dts/sun7i-a20-pcduino3.dts
> @@ -164,7 +164,7 @@
>  	pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin_reference_design>;
>  	vmmc-supply = <&reg_vcc3v3>;
>  	bus-width = <4>;
> -	cd-gpios = <&pio 7 1 GPIO_ACTIVE_LOW>; /* PH1 */
> +	cd-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; /* PH1 */
>  	cd-inverted;
>  	status = "okay";
>  };
> diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c
> index 4edb4be46c..a974543148 100644
> --- a/drivers/mmc/sunxi_mmc.c
> +++ b/drivers/mmc/sunxi_mmc.c
> @@ -30,6 +30,7 @@ struct sunxi_mmc_priv {
>  	uint32_t *mclkreg;
>  	unsigned fatal_err;
>  	struct gpio_desc cd_gpio;	/* Change Detect GPIO */
> +	int cd_inverted;
>  	struct sunxi_mmc *reg;
>  	struct mmc_config cfg;
>  };
> @@ -545,7 +546,7 @@ static int sunxi_mmc_getcd(struct udevice *dev)
>  	struct sunxi_mmc_priv *priv = dev_get_priv(dev);
>  
>  	if (dm_gpio_is_valid(&priv->cd_gpio))
> -		return dm_gpio_get_value(&priv->cd_gpio);
> +		return dm_gpio_get_value(&priv->cd_gpio) ^ priv->cd_inverted;
>  
>  	return 1;
>  }
> @@ -607,6 +608,8 @@ static int sunxi_mmc_probe(struct udevice *dev)
>  				  GPIOD_IS_IN)) {
>  		int cd_pin = gpio_get_number(&priv->cd_gpio);
>  
> +		priv->cd_inverted = dev_read_bool(dev, "cd-inverted");
> +
>  		sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP);
>  	}
>  
> So: if the DT specifies GPIO_ACTIVE_LOW, dm_gpio_get_value()
> does its own inversion of the GPIO level. Then, on top of that
> we do another inversion if the "cd-inverted" property was specified.
> This matches what the Linux MMC driver does.

Hmmm, right.

I guess in your particular case then, you'd be better off droping the
cd-inverted property. Like I said, it's really legacy these days. That
of course doesn't prevent the rest of the patch to go in.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171221/14fca879/attachment.sig>