| Message ID | 20190930142449.4480-2-jjhiblot@ti.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | reset: Add a managed API | expand |
Hi Jean-Jacques, On Mon, 30 Sep 2019 at 08:31, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote: > > Prepare the way for a managed reset API by handling NULL pointers without > crashing nor failing. > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> > --- > > drivers/reset/reset-uclass.c | 30 +++++++++++++++++------------- > 1 file changed, 17 insertions(+), 13 deletions(-) Same comment here about code size / Kconfig option. Regards, Simon
Hi Simon, On 29/10/19 07:48PM, Simon Glass wrote: > Hi Jean-Jacques, > > On Mon, 30 Sep 2019 at 08:31, Jean-Jacques Hiblot <jjhiblot at ti.com> wrote: > > > > Prepare the way for a managed reset API by handling NULL pointers without > > crashing nor failing. > > > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot at ti.com> > > --- > > > > drivers/reset/reset-uclass.c | 30 +++++++++++++++++------------- > > 1 file changed, 17 insertions(+), 13 deletions(-) > > Same comment here about code size / Kconfig option. A lot of the changes below are ternary operators. I'm not sure how to put them behind a Kconfig option to reduce code size. Do you want me to change the NULL check to a separate if() block to put behind an #ifdef? One way of doing this is like in this patch [0]. The other would be to wrap individual if statements in #ifdef, which tends to hurt readability. [0] https://patchwork.ozlabs.org/project/uboot/patch/20191001115130.18886-2-jjhiblot@ti.com/ > diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c > index ee1a423ffb..1cfcc8b367 100644 > --- a/drivers/reset/reset-uclass.c > +++ b/drivers/reset/reset-uclass.c > @@ -9,9 +9,12 @@ > #include <reset.h> > #include <reset-uclass.h> > > -static inline struct reset_ops *reset_dev_ops(struct udevice *dev) > +struct reset_ops nop_reset_ops = { > +}; > + > +static inline struct reset_ops *reset_dev_ops(struct reset_ctl *r) > { > - return (struct reset_ops *)dev->driver->ops; > + return r ? (struct reset_ops *)r->dev->driver->ops : &nop_reset_ops; > } > > static int reset_of_xlate_default(struct reset_ctl *reset_ctl, > @@ -50,9 +53,10 @@ static int reset_get_by_index_tail(int ret, ofnode node, > debug("%s %d\n", ofnode_get_name(args->node), args->args[0]); > return ret; > } > - ops = reset_dev_ops(dev_reset); > > reset_ctl->dev = dev_reset; > + ops = reset_dev_ops(reset_ctl); > + > if (ops->of_xlate) > ret = ops->of_xlate(reset_ctl, args); > else > @@ -151,29 +155,29 @@ int reset_get_by_name(struct udevice *dev, const char *name, > > int reset_request(struct reset_ctl *reset_ctl) > { > - struct reset_ops *ops = reset_dev_ops(reset_ctl->dev); > + struct reset_ops *ops = reset_dev_ops(reset_ctl); > > debug("%s(reset_ctl=%p)\n", __func__, reset_ctl); > > - return ops->request(reset_ctl); > + return ops->request ? ops->request(reset_ctl) : 0; > } > > int reset_free(struct reset_ctl *reset_ctl) > { > - struct reset_ops *ops = reset_dev_ops(reset_ctl->dev); > + struct reset_ops *ops = reset_dev_ops(reset_ctl); > > debug("%s(reset_ctl=%p)\n", __func__, reset_ctl); > > - return ops->free(reset_ctl); > + return ops->free ? ops->free(reset_ctl) : 0; > } > > int reset_assert(struct reset_ctl *reset_ctl) > { > - struct reset_ops *ops = reset_dev_ops(reset_ctl->dev); > + struct reset_ops *ops = reset_dev_ops(reset_ctl); > > debug("%s(reset_ctl=%p)\n", __func__, reset_ctl); > > - return ops->rst_assert(reset_ctl); > + return ops->rst_assert ? ops->rst_assert(reset_ctl) : 0; > } > > int reset_assert_bulk(struct reset_ctl_bulk *bulk) > @@ -191,11 +195,11 @@ int reset_assert_bulk(struct reset_ctl_bulk *bulk) > > int reset_deassert(struct reset_ctl *reset_ctl) > { > - struct reset_ops *ops = reset_dev_ops(reset_ctl->dev); > + struct reset_ops *ops = reset_dev_ops(reset_ctl); > > debug("%s(reset_ctl=%p)\n", __func__, reset_ctl); > > - return ops->rst_deassert(reset_ctl); > + return ops->rst_deassert ? ops->rst_deassert(reset_ctl) : 0; > } > > int reset_deassert_bulk(struct reset_ctl_bulk *bulk) > @@ -213,11 +217,11 @@ int reset_deassert_bulk(struct reset_ctl_bulk *bulk) > > int reset_status(struct reset_ctl *reset_ctl) > { > - struct reset_ops *ops = reset_dev_ops(reset_ctl->dev); > + struct reset_ops *ops = reset_dev_ops(reset_ctl); > > debug("%s(reset_ctl=%p)\n", __func__, reset_ctl); > > - return ops->rst_status(reset_ctl); > + return ops->rst_status ? ops->rst_status(reset_ctl) : 0; > } > > int reset_release_all(struct reset_ctl *reset_ctl, int count)
Hi Pratyush, On Wed, 27 May 2020 at 11:12, Pratyush Yadav <p.yadav@ti.com> wrote: > > Hi Simon, > > On 29/10/19 07:48PM, Simon Glass wrote: > > Hi Jean-Jacques, > > > > On Mon, 30 Sep 2019 at 08:31, Jean-Jacques Hiblot <jjhiblot at ti.com> wrote: > > > > > > Prepare the way for a managed reset API by handling NULL pointers without > > > crashing nor failing. > > > > > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot at ti.com> > > > --- > > > > > > drivers/reset/reset-uclass.c | 30 +++++++++++++++++------------- > > > 1 file changed, 17 insertions(+), 13 deletions(-) > > > > Same comment here about code size / Kconfig option. > > A lot of the changes below are ternary operators. I'm not sure how to > put them behind a Kconfig option to reduce code size. Do you want me to > change the NULL check to a separate if() block to put behind an #ifdef? > > One way of doing this is like in this patch [0]. The other would be to > wrap individual if statements in #ifdef, which tends to hurt > readability. Well for this I would really favour: int reset_request(struct reset_ctl *reset_ctl) { struct reset_ops *ops; if (!result_ctl) return 0; ops = reset_dev_ops(reset_ctl->dev); But why allow result_ctl to be NULL? You should explain that in your commit message. > > - struct reset_ops *ops = reset_dev_ops(reset_ctl->dev); > > + struct reset_ops *ops = reset_dev_ops(reset_ctl); > > > > debug("%s(reset_ctl=%p)\n", __func__, reset_ctl); > > > > - return ops->request(reset_ctl); > > + return ops->request ? ops->request(reset_ctl) : 0; > > } > > > > [0] https://patchwork.ozlabs.org/project/uboot/patch/20191001115130.18886-2-jjhiblot@ti.com/ [..] Regards, Simon
diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c index ee1a423ffb..1cfcc8b367 100644 --- a/drivers/reset/reset-uclass.c +++ b/drivers/reset/reset-uclass.c @@ -9,9 +9,12 @@ #include <reset.h> #include <reset-uclass.h> -static inline struct reset_ops *reset_dev_ops(struct udevice *dev) +struct reset_ops nop_reset_ops = { +}; + +static inline struct reset_ops *reset_dev_ops(struct reset_ctl *r) { - return (struct reset_ops *)dev->driver->ops; + return r ? (struct reset_ops *)r->dev->driver->ops : &nop_reset_ops; } static int reset_of_xlate_default(struct reset_ctl *reset_ctl, @@ -50,9 +53,10 @@ static int reset_get_by_index_tail(int ret, ofnode node, debug("%s %d\n", ofnode_get_name(args->node), args->args[0]); return ret; } - ops = reset_dev_ops(dev_reset); reset_ctl->dev = dev_reset; + ops = reset_dev_ops(reset_ctl); + if (ops->of_xlate) ret = ops->of_xlate(reset_ctl, args); else @@ -151,29 +155,29 @@ int reset_get_by_name(struct udevice *dev, const char *name, int reset_request(struct reset_ctl *reset_ctl) { - struct reset_ops *ops = reset_dev_ops(reset_ctl->dev); + struct reset_ops *ops = reset_dev_ops(reset_ctl); debug("%s(reset_ctl=%p)\n", __func__, reset_ctl); - return ops->request(reset_ctl); + return ops->request ? ops->request(reset_ctl) : 0; } int reset_free(struct reset_ctl *reset_ctl) { - struct reset_ops *ops = reset_dev_ops(reset_ctl->dev); + struct reset_ops *ops = reset_dev_ops(reset_ctl); debug("%s(reset_ctl=%p)\n", __func__, reset_ctl); - return ops->free(reset_ctl); + return ops->free ? ops->free(reset_ctl) : 0; } int reset_assert(struct reset_ctl *reset_ctl) { - struct reset_ops *ops = reset_dev_ops(reset_ctl->dev); + struct reset_ops *ops = reset_dev_ops(reset_ctl); debug("%s(reset_ctl=%p)\n", __func__, reset_ctl); - return ops->rst_assert(reset_ctl); + return ops->rst_assert ? ops->rst_assert(reset_ctl) : 0; } int reset_assert_bulk(struct reset_ctl_bulk *bulk) @@ -191,11 +195,11 @@ int reset_assert_bulk(struct reset_ctl_bulk *bulk) int reset_deassert(struct reset_ctl *reset_ctl) { - struct reset_ops *ops = reset_dev_ops(reset_ctl->dev); + struct reset_ops *ops = reset_dev_ops(reset_ctl); debug("%s(reset_ctl=%p)\n", __func__, reset_ctl); - return ops->rst_deassert(reset_ctl); + return ops->rst_deassert ? ops->rst_deassert(reset_ctl) : 0; } int reset_deassert_bulk(struct reset_ctl_bulk *bulk) @@ -213,11 +217,11 @@ int reset_deassert_bulk(struct reset_ctl_bulk *bulk) int reset_status(struct reset_ctl *reset_ctl) { - struct reset_ops *ops = reset_dev_ops(reset_ctl->dev); + struct reset_ops *ops = reset_dev_ops(reset_ctl); debug("%s(reset_ctl=%p)\n", __func__, reset_ctl); - return ops->rst_status(reset_ctl); + return ops->rst_status ? ops->rst_status(reset_ctl) : 0; } int reset_release_all(struct reset_ctl *reset_ctl, int count)
Prepare the way for a managed reset API by handling NULL pointers without crashing nor failing. Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com> --- drivers/reset/reset-uclass.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-)