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

[Tuomas Tynkkynen]

On Wed, 2017-12-20 at 13:26 +0100, Maxime Ripard wrote:
> On Mon, Dec 18, 2017 at 10:09:41PM +0200, Tuomas Tynkkynen wrote:
> > Commit 8620f384098b ("dm: sunxi: Linksprite_pcDuino3: Correct
> > polarity
> > of MMC card detect") claims that the Pcduino3 device tree has an
> > incorrect polarity for the card detect pin, but the actual problem
> > is
> > that unlike the Linux MMC driver, the U-Boot driver isn't
> > respecting the
> > cd-invert property (see
> > Documentation/devicetree/bindings/mmc/mmc.txt)
> > which tells that the card detect signal level should be inverted.
> > 
> > Fix this properly by adding support for the cd-inverted property
> > while
> > reverting the original commit at the same time. While at it, I
> > noticed
> > the driver always enables pullups for the card detect line, which
> > is not
> > right if the card detect GPIO is active-high, so fix that as well.
> > 
> > Signed-off-by: Tuomas Tynkkynen <tuomas at tuxera.com>
> > ---
> >  arch/arm/dts/sun7i-a20-pcduino3.dts |  2 +-
> >  drivers/mmc/sunxi_mmc.c             | 12 ++++++++++--
> >  2 files changed, 11 insertions(+), 3 deletions(-)
> > 
> > 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..7cc7303570 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;
> >  }
> > @@ -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:

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.

Does that make sense?

Thanks.