| Message ID | 20211021045258.30757-5-samuel@sholland.org |
|---|---|
| State | Accepted |
| Commit | fb6f67013e3a6d74988615a4bea5f5926ec328d5 |
| Delegated to: | Andre Przywara |
| Headers | show |
| Series | gpio: sunxi: Handle pin configuration flags | expand |
Hi, On 10/21/21 1:52 PM, Samuel Holland wrote: > Now that the sunxi_gpio driver handles pull-up/down via the driver > model, pin configuration does not need a platform-specific function. > > Signed-off-by: Samuel Holland <samuel@sholland.org> > --- > > drivers/mmc/sunxi_mmc.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c > index c170c16d5a..955b29826f 100644 > --- a/drivers/mmc/sunxi_mmc.c > +++ b/drivers/mmc/sunxi_mmc.c > @@ -700,12 +700,8 @@ static int sunxi_mmc_probe(struct udevice *dev) > return ret; > > /* This GPIO is optional */ > - if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio, > - GPIOD_IS_IN)) { > - int cd_pin = gpio_get_number(&priv->cd_gpio); > - > - sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP); > - } > + gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio, > + GPIOD_IS_IN | GPIOD_PULL_UP); Is it right to set the pull-up? Best Regards, Jaehoon Chung > > upriv->mmc = &plat->mmc; > >
On Fri, 22 Oct 2021 06:58:48 +0900 Jaehoon Chung <jh80.chung@samsung.com> wrote: Hi Jaehoon, thanks for having a look! > Hi, > > On 10/21/21 1:52 PM, Samuel Holland wrote: > > Now that the sunxi_gpio driver handles pull-up/down via the driver > > model, pin configuration does not need a platform-specific function. > > > > Signed-off-by: Samuel Holland <samuel@sholland.org> > > --- > > > > drivers/mmc/sunxi_mmc.c | 8 ++------ > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c > > index c170c16d5a..955b29826f 100644 > > --- a/drivers/mmc/sunxi_mmc.c > > +++ b/drivers/mmc/sunxi_mmc.c > > @@ -700,12 +700,8 @@ static int sunxi_mmc_probe(struct udevice *dev) > > return ret; > > > > /* This GPIO is optional */ > > - if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio, > > - GPIOD_IS_IN)) { > > - int cd_pin = gpio_get_number(&priv->cd_gpio); > > - > > - sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP); > > - } > > + gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio, > > + GPIOD_IS_IN | GPIOD_PULL_UP); > > Is it right to set the pull-up? You mean, unconditionally? I mean it's just copying the current code, which does that (see the "minus" lines just above). But I think you have a point: I don't see any pull up specified in any DT, and I think most (if not all) boards have a discrete pull up resistor on that line. But I don't dare to touch that code - at least for this series, as it works (TM) right now. After the full DM_PINCTRL series this might be another story, though. Cheers, Andre > > Best Regards, > Jaehoon Chung > > > > > upriv->mmc = &plat->mmc; > > > > >
Hi Andre, On 10/22/21 6:00 PM, Andre Przywara wrote: > On Fri, 22 Oct 2021 06:58:48 +0900 > Jaehoon Chung <jh80.chung@samsung.com> wrote: > > Hi Jaehoon, > > thanks for having a look! > >> Hi, >> >> On 10/21/21 1:52 PM, Samuel Holland wrote: >>> Now that the sunxi_gpio driver handles pull-up/down via the driver >>> model, pin configuration does not need a platform-specific function. >>> >>> Signed-off-by: Samuel Holland <samuel@sholland.org> >>> --- >>> >>> drivers/mmc/sunxi_mmc.c | 8 ++------ >>> 1 file changed, 2 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c >>> index c170c16d5a..955b29826f 100644 >>> --- a/drivers/mmc/sunxi_mmc.c >>> +++ b/drivers/mmc/sunxi_mmc.c >>> @@ -700,12 +700,8 @@ static int sunxi_mmc_probe(struct udevice *dev) >>> return ret; >>> >>> /* This GPIO is optional */ >>> - if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio, >>> - GPIOD_IS_IN)) { >>> - int cd_pin = gpio_get_number(&priv->cd_gpio); >>> - >>> - sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP); >>> - } >>> + gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio, >>> + GPIOD_IS_IN | GPIOD_PULL_UP); >> >> Is it right to set the pull-up? > > You mean, unconditionally? I mean it's just copying the current > code, which does that (see the "minus" lines just above). Yes, it looks like something strange. AFAIK, It can be changed that cd-gpios has dependent how to consist of H/W. But it's not different with original behavior, as you mentioned. > > But I think you have a point: I don't see any pull up specified in any DT, > and I think most (if not all) boards have a discrete pull up resistor on > that line. > > But I don't dare to touch that code - at least for this series, as it > works (TM) right now. > After the full DM_PINCTRL series this might be another story, though. Right. I understood exactly. Thanks for explanation. P.S: Can I test sunxi patch with NeoPlus2 board(Allwinner H5)? Best Regards, Jaehoon Chung > > Cheers, > Andre > >> >> Best Regards, >> Jaehoon Chung >> >>> >>> upriv->mmc = &plat->mmc; >>> >>> >> > >
On Fri, 22 Oct 2021 19:10:20 +0900 Jaehoon Chung <jh80.chung@samsung.com> wrote: > Hi Andre, > > On 10/22/21 6:00 PM, Andre Przywara wrote: > > On Fri, 22 Oct 2021 06:58:48 +0900 > > Jaehoon Chung <jh80.chung@samsung.com> wrote: > > > > Hi Jaehoon, > > > > thanks for having a look! > > > >> Hi, > >> > >> On 10/21/21 1:52 PM, Samuel Holland wrote: > >>> Now that the sunxi_gpio driver handles pull-up/down via the driver > >>> model, pin configuration does not need a platform-specific function. > >>> > >>> Signed-off-by: Samuel Holland <samuel@sholland.org> > >>> --- > >>> > >>> drivers/mmc/sunxi_mmc.c | 8 ++------ > >>> 1 file changed, 2 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c > >>> index c170c16d5a..955b29826f 100644 > >>> --- a/drivers/mmc/sunxi_mmc.c > >>> +++ b/drivers/mmc/sunxi_mmc.c > >>> @@ -700,12 +700,8 @@ static int sunxi_mmc_probe(struct udevice *dev) > >>> return ret; > >>> > >>> /* This GPIO is optional */ > >>> - if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio, > >>> - GPIOD_IS_IN)) { > >>> - int cd_pin = gpio_get_number(&priv->cd_gpio); > >>> - > >>> - sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP); > >>> - } > >>> + gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio, > >>> + GPIOD_IS_IN | GPIOD_PULL_UP); > >> > >> Is it right to set the pull-up? > > > > You mean, unconditionally? I mean it's just copying the current > > code, which does that (see the "minus" lines just above). > > Yes, it looks like something strange. > AFAIK, It can be changed that cd-gpios has dependent how to consist of H/W. > But it's not different with original behavior, as you mentioned. > > > > > But I think you have a point: I don't see any pull up specified in any DT, > > and I think most (if not all) boards have a discrete pull up resistor on > > that line. > > > > But I don't dare to touch that code - at least for this series, as it > > works (TM) right now. > > After the full DM_PINCTRL series this might be another story, though. > > Right. I understood exactly. Thanks for explanation. > > P.S: Can I test sunxi patch with NeoPlus2 board(Allwinner H5)? Yes, please, any testing on any kind of Allwinner board is warmly welcomed. There are 160 supported Allwinner boards in the tree, and even though I have probably more of them than I want, it's only a fraction of them. And experience show that some boards are subtly broken. Also feel free to report any other broken things you see when testing! Thanks, Andre > > Best Regards, > Jaehoon Chung > > > > > Cheers, > > Andre > > > >> > >> Best Regards, > >> Jaehoon Chung > >> > >>> > >>> upriv->mmc = &plat->mmc; > >>> > >>> > >> > > > > >
On 10/21/21 06:52, Samuel Holland wrote: > Now that the sunxi_gpio driver handles pull-up/down via the driver > model, pin configuration does not need a platform-specific function. > > Signed-off-by: Samuel Holland <samuel@sholland.org> I tested on an OrangePi PC (orangepi_pc_defconfig). The 'mmc rescan' command detects correctly if an SD-card is present with this series applied. Tested-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > > drivers/mmc/sunxi_mmc.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c > index c170c16d5a..955b29826f 100644 > --- a/drivers/mmc/sunxi_mmc.c > +++ b/drivers/mmc/sunxi_mmc.c > @@ -700,12 +700,8 @@ static int sunxi_mmc_probe(struct udevice *dev) > return ret; > > /* This GPIO is optional */ > - if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio, > - GPIOD_IS_IN)) { > - int cd_pin = gpio_get_number(&priv->cd_gpio); > - > - sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP); > - } > + gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio, > + GPIOD_IS_IN | GPIOD_PULL_UP); > > upriv->mmc = &plat->mmc; > >
diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c index c170c16d5a..955b29826f 100644 --- a/drivers/mmc/sunxi_mmc.c +++ b/drivers/mmc/sunxi_mmc.c @@ -700,12 +700,8 @@ static int sunxi_mmc_probe(struct udevice *dev) return ret; /* This GPIO is optional */ - if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio, - GPIOD_IS_IN)) { - int cd_pin = gpio_get_number(&priv->cd_gpio); - - sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP); - } + gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio, + GPIOD_IS_IN | GPIOD_PULL_UP); upriv->mmc = &plat->mmc;
Now that the sunxi_gpio driver handles pull-up/down via the driver model, pin configuration does not need a platform-specific function. Signed-off-by: Samuel Holland <samuel@sholland.org> --- drivers/mmc/sunxi_mmc.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)