| Message ID | 20180929234553.31019-3-vagrant@debian.org |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | [U-Boot,1/5] mmc: sunxi: add support for automatic delay calibration | expand |
Hi Vagrant, On Sat, Sep 29, 2018 at 4:46 PM Vagrant Cascadian <vagrant@debian.org> wrote: > > From: Vasily Khoruzhick <anarsoul@gmail.com> > > Sleep gpio is optional, so it's possible to have reset gpio, but no sleep gpio. > We shouldn't fail early in case of missing sleep gpio, otherwise we won't > deassert reset. > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> > Signed-off-by: Vagrant Cascadian <vagrant@debian.org> > --- > > drivers/video/bridge/video-bridge-uclass.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/video/bridge/video-bridge-uclass.c b/drivers/video/bridge/video-bridge-uclass.c > index cd4959cc71..46936a0626 100644 > --- a/drivers/video/bridge/video-bridge-uclass.c > +++ b/drivers/video/bridge/video-bridge-uclass.c > @@ -110,7 +110,7 @@ int video_bridge_set_active(struct udevice *dev, bool active) > > debug("%s: %d\n", __func__, active); > ret = dm_gpio_set_value(&uc_priv->sleep, !active); > - if (ret) > + if (ret != -ENOENT) It should be 'if (ret && ret != -ENOENT)'. Btw, I fixed it in pinebook-wip-20180909 branch. > return ret; > if (active) { > ret = dm_gpio_set_value(&uc_priv->reset, true); > @@ -120,7 +120,7 @@ int video_bridge_set_active(struct udevice *dev, bool active) > ret = dm_gpio_set_value(&uc_priv->reset, false); > } > > - return ret; > + return 0; > } > > UCLASS_DRIVER(video_bridge) = { > -- > 2.11.0 >
On 2018-09-29, Vasily Khoruzhick <anarsoul@gmail.com> wrote: > On Sat, Sep 29, 2018 at 4:46 PM Vagrant Cascadian <vagrant@debian.org> wrote: >> Sleep gpio is optional, so it's possible to have reset gpio, but no sleep gpio. >> We shouldn't fail early in case of missing sleep gpio, otherwise we won't >> deassert reset. ... >> diff --git a/drivers/video/bridge/video-bridge-uclass.c b/drivers/video/bridge/video-bridge-uclass.c >> index cd4959cc71..46936a0626 100644 >> --- a/drivers/video/bridge/video-bridge-uclass.c >> +++ b/drivers/video/bridge/video-bridge-uclass.c >> @@ -110,7 +110,7 @@ int video_bridge_set_active(struct udevice *dev, bool active) >> >> debug("%s: %d\n", __func__, active); >> ret = dm_gpio_set_value(&uc_priv->sleep, !active); >> - if (ret) >> + if (ret != -ENOENT) > > It should be 'if (ret && ret != -ENOENT)'. Btw, I fixed it in > pinebook-wip-20180909 branch. That's where I pulled the patch from; it's present in the patch to anx6345.c, but apparently unpatched in this patch against video-bridge-uclass.c. I'll submit the fixed version in a new patch series after collecting more comments... Thanks for all your work on it! live well, vagrant
On Sun, Sep 30, 2018 at 10:48 AM Vagrant Cascadian <vagrant@debian.org> wrote: > > On 2018-09-29, Vasily Khoruzhick <anarsoul@gmail.com> wrote: > > On Sat, Sep 29, 2018 at 4:46 PM Vagrant Cascadian <vagrant@debian.org> wrote: > >> Sleep gpio is optional, so it's possible to have reset gpio, but no sleep gpio. > >> We shouldn't fail early in case of missing sleep gpio, otherwise we won't > >> deassert reset. > ... > >> diff --git a/drivers/video/bridge/video-bridge-uclass.c b/drivers/video/bridge/video-bridge-uclass.c > >> index cd4959cc71..46936a0626 100644 > >> --- a/drivers/video/bridge/video-bridge-uclass.c > >> +++ b/drivers/video/bridge/video-bridge-uclass.c > >> @@ -110,7 +110,7 @@ int video_bridge_set_active(struct udevice *dev, bool active) > >> > >> debug("%s: %d\n", __func__, active); > >> ret = dm_gpio_set_value(&uc_priv->sleep, !active); > >> - if (ret) > >> + if (ret != -ENOENT) > > > > It should be 'if (ret && ret != -ENOENT)'. Btw, I fixed it in > > pinebook-wip-20180909 branch. > > That's where I pulled the patch from; it's present in the patch to > anx6345.c, but apparently unpatched in this patch against > video-bridge-uclass.c. You're right, I fixed it but forgot to push it. Sorry for the noise. > I'll submit the fixed version in a new patch series after collecting > more comments... > > Thanks for all your work on it! Thanks a lot for submitting it! > live well, > vagrant
On 9/30/18 12:45 AM, Vagrant Cascadian wrote: > From: Vasily Khoruzhick <anarsoul@gmail.com> > > Sleep gpio is optional, so it's possible to have reset gpio, but no sleep gpio. > We shouldn't fail early in case of missing sleep gpio, otherwise we won't > deassert reset. > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> > Signed-off-by: Vagrant Cascadian <vagrant@debian.org> > --- > > drivers/video/bridge/video-bridge-uclass.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/video/bridge/video-bridge-uclass.c b/drivers/video/bridge/video-bridge-uclass.c > index cd4959cc71..46936a0626 100644 > --- a/drivers/video/bridge/video-bridge-uclass.c > +++ b/drivers/video/bridge/video-bridge-uclass.c > @@ -110,7 +110,7 @@ int video_bridge_set_active(struct udevice *dev, bool active) > > debug("%s: %d\n", __func__, active); > ret = dm_gpio_set_value(&uc_priv->sleep, !active); So if I get this correctly, uc_priv->sleep.dev would be NULL if there was no GPIO specified? So wouldn't it be cleaner to say: if (uc_priv->sleep.dev) { ret = dm_gpio_set_value(&uc_priv->sleep, !active); ... > - if (ret) > + if (ret != -ENOENT) > return ret; > if (active) { > ret = dm_gpio_set_value(&uc_priv->reset, true); > @@ -120,7 +120,7 @@ int video_bridge_set_active(struct udevice *dev, bool active) > ret = dm_gpio_set_value(&uc_priv->reset, false); > } > > - return ret; > + return 0; This would loose the return value from the last statement in the if clause. So what about negating this clause: if (!active) return 0; and change the rest accordingly? Cheers, Andre. > } > > UCLASS_DRIVER(video_bridge) = { >
diff --git a/drivers/video/bridge/video-bridge-uclass.c b/drivers/video/bridge/video-bridge-uclass.c index cd4959cc71..46936a0626 100644 --- a/drivers/video/bridge/video-bridge-uclass.c +++ b/drivers/video/bridge/video-bridge-uclass.c @@ -110,7 +110,7 @@ int video_bridge_set_active(struct udevice *dev, bool active) debug("%s: %d\n", __func__, active); ret = dm_gpio_set_value(&uc_priv->sleep, !active); - if (ret) + if (ret != -ENOENT) return ret; if (active) { ret = dm_gpio_set_value(&uc_priv->reset, true); @@ -120,7 +120,7 @@ int video_bridge_set_active(struct udevice *dev, bool active) ret = dm_gpio_set_value(&uc_priv->reset, false); } - return ret; + return 0; } UCLASS_DRIVER(video_bridge) = {