| Message ID | 20200225065803.30275-1-yamada.masahiro@socionext.com |
|---|---|
| State | New |
| Headers | show |
| Series | dm: make uclass_find_first_device() return error when no defice is found | expand |
Hi Masahiro, On Mon, 24 Feb 2020 at 23:58, Masahiro Yamada <yamada.masahiro at socionext.com> wrote: > > uclass_find_first_device() succeeds even if it cannot find any device. > So, the caller must check the return code and also *devp is not NULL. > > Returning -ENODEV will be sensible in this case. > > Signed-off-by: Masahiro Yamada <yamada.masahiro at socionext.com> > --- > > If this patch is acceptable, I want to fold this > into my pull request because it need it > for my another patch: > http://patchwork.ozlabs.org/patch/1238000/ > > drivers/core/uclass.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) I sort-of agree and have thought about this a lot. But what are you doing in the case of uclass_find_next_device()? Also what about the tests and other code that expects the current behaviour? > > diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c > index 58b19a421091..3580974f3b85 100644 > --- a/drivers/core/uclass.c > +++ b/drivers/core/uclass.c > @@ -227,7 +227,7 @@ int uclass_find_first_device(enum uclass_id id, struct udevice **devp) > if (ret) > return ret; > if (list_empty(&uc->dev_head)) > - return 0; > + return -ENODEV; > > *devp = list_first_entry(&uc->dev_head, struct udevice, uclass_node); > > -- > 2.17.1 > Regards, Simon
Hi Simon, On Thu, Feb 27, 2020 at 12:33 AM Simon Glass <sjg at chromium.org> wrote: > > Hi Masahiro, > > On Mon, 24 Feb 2020 at 23:58, Masahiro Yamada > <yamada.masahiro at socionext.com> wrote: > > > > uclass_find_first_device() succeeds even if it cannot find any device. > > So, the caller must check the return code and also *devp is not NULL. > > > > Returning -ENODEV will be sensible in this case. > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro at socionext.com> > > --- > > > > If this patch is acceptable, I want to fold this > > into my pull request because it need it > > for my another patch: > > http://patchwork.ozlabs.org/patch/1238000/ > > > > drivers/core/uclass.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > I sort-of agree and have thought about this a lot. > > But what are you doing in the case of uclass_find_next_device()? > > Also what about the tests and other code that expects the current behaviour? OK, I will change my caller side. I would probably design these functions as follows: struct udevice *uclass_find_first_device(enum uclass_id id); struct udevice *uclass_find_next_device(struct udevice *dev); Return the pointer to the device, or NULL if not found. > > > > diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c > > index 58b19a421091..3580974f3b85 100644 > > --- a/drivers/core/uclass.c > > +++ b/drivers/core/uclass.c > > @@ -227,7 +227,7 @@ int uclass_find_first_device(enum uclass_id id, struct udevice **devp) > > if (ret) > > return ret; > > if (list_empty(&uc->dev_head)) > > - return 0; > > + return -ENODEV; > > > > *devp = list_first_entry(&uc->dev_head, struct udevice, uclass_node); > > > > -- > > 2.17.1 > > > > Regards, > Simon -- Best Regards Masahiro Yamada
Hi Masahiro, On Thu, 27 Feb 2020 at 09:57, Masahiro Yamada <masahiroy at kernel.org> wrote: > > Hi Simon, > > On Thu, Feb 27, 2020 at 12:33 AM Simon Glass <sjg at chromium.org> wrote: > > > > Hi Masahiro, > > > > On Mon, 24 Feb 2020 at 23:58, Masahiro Yamada > > <yamada.masahiro at socionext.com> wrote: > > > > > > uclass_find_first_device() succeeds even if it cannot find any device. > > > So, the caller must check the return code and also *devp is not NULL. > > > > > > Returning -ENODEV will be sensible in this case. > > > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro at socionext.com> > > > --- > > > > > > If this patch is acceptable, I want to fold this > > > into my pull request because it need it > > > for my another patch: > > > http://patchwork.ozlabs.org/patch/1238000/ > > > > > > drivers/core/uclass.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > I sort-of agree and have thought about this a lot. > > > > But what are you doing in the case of uclass_find_next_device()? > > > > Also what about the tests and other code that expects the current behaviour? > > > OK, I will change my caller side. > > > > I would probably design these functions as follows: > > struct udevice *uclass_find_first_device(enum uclass_id id); > > struct udevice *uclass_find_next_device(struct udevice *dev); > > > Return the pointer to the device, or NULL if not found. I think that is fine, since we don't support any other error than -ENODEV. I did intend to add checks that the structs are correct (by adding type information into each struct conditional on a DM_DEBUG option) and returning a special error when something goes wrong. But even then I think we could just return NULL and -ENODEV in the caller would be good enough. Regards, Simon
diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index 58b19a421091..3580974f3b85 100644 --- a/drivers/core/uclass.c +++ b/drivers/core/uclass.c @@ -227,7 +227,7 @@ int uclass_find_first_device(enum uclass_id id, struct udevice **devp) if (ret) return ret; if (list_empty(&uc->dev_head)) - return 0; + return -ENODEV; *devp = list_first_entry(&uc->dev_head, struct udevice, uclass_node);
uclass_find_first_device() succeeds even if it cannot find any device. So, the caller must check the return code and also *devp is not NULL. Returning -ENODEV will be sensible in this case. Signed-off-by: Masahiro Yamada <yamada.masahiro at socionext.com> --- If this patch is acceptable, I want to fold this into my pull request because it need it for my another patch: http://patchwork.ozlabs.org/patch/1238000/ drivers/core/uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)