| Message ID | 20210405102836.24515-4-kishon@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Lokesh Vutla |
| Headers | show |
| Series | TI/Cadence: Add Sierra/Torrent SERDES driver | expand |
Hi Kishon, On Mon, 5 Apr 2021 at 11:28, Kishon Vijay Abraham I <kishon@ti.com> wrote: > > From: Jean-Jacques Hiblot <jjhiblot@ti.com> > > The reset framework provides devm_reset_control_get_optional() > which can return NULL (not an error case). So all the other reset_ops > should handle NULL gracefully. 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> > Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com> > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > drivers/reset/reset-uclass.c | 30 +++++++++++++++++------------- > 1 file changed, 17 insertions(+), 13 deletions(-) > > diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c > index 071c389ca0..98304bc0ee 100644 > --- a/drivers/reset/reset-uclass.c > +++ b/drivers/reset/reset-uclass.c > @@ -13,9 +13,12 @@ > #include <dm/devres.h> > #include <dm/lists.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; This behaviour still seems odd to me. Why do you have a reset driver with no ops? That is not allowed. > } > > static int reset_of_xlate_default(struct reset_ctl *reset_ctl, > @@ -54,9 +57,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 > @@ -162,29 +166,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; Can you check this first and return -ENOSYS ? E.g. if (!ops->request) return -ENOSYS; return ops->request(reset_ctl); Same below > } > > 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->rfree(reset_ctl); > + return ops->rfree ? ops->rfree(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) > @@ -202,11 +206,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) > @@ -224,11 +228,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) > -- > 2.17.1 > Regards, Simon
Hi Simon, On 15/04/21 1:08 am, Simon Glass wrote: > Hi Kishon, > > On Mon, 5 Apr 2021 at 11:28, Kishon Vijay Abraham I <kishon@ti.com> wrote: >> >> From: Jean-Jacques Hiblot <jjhiblot@ti.com> >> >> The reset framework provides devm_reset_control_get_optional() >> which can return NULL (not an error case). So all the other reset_ops >> should handle NULL gracefully. 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> >> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> --- >> drivers/reset/reset-uclass.c | 30 +++++++++++++++++------------- >> 1 file changed, 17 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c >> index 071c389ca0..98304bc0ee 100644 >> --- a/drivers/reset/reset-uclass.c >> +++ b/drivers/reset/reset-uclass.c >> @@ -13,9 +13,12 @@ >> #include <dm/devres.h> >> #include <dm/lists.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; > > This behaviour still seems odd to me. Why do you have a reset driver > with no ops? That is not allowed. Okay. I'll re-work this patch and post a fresh series. Thanks Kishon > >> } >> >> static int reset_of_xlate_default(struct reset_ctl *reset_ctl, >> @@ -54,9 +57,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 >> @@ -162,29 +166,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; > > Can you check this first and return -ENOSYS ? E.g. > > if (!ops->request) > return -ENOSYS; > > return ops->request(reset_ctl); > > Same below > >> } >> >> 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->rfree(reset_ctl); >> + return ops->rfree ? ops->rfree(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) >> @@ -202,11 +206,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) >> @@ -224,11 +228,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) >> -- >> 2.17.1 >> > > Regards, > Simon >
diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c index 071c389ca0..98304bc0ee 100644 --- a/drivers/reset/reset-uclass.c +++ b/drivers/reset/reset-uclass.c @@ -13,9 +13,12 @@ #include <dm/devres.h> #include <dm/lists.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, @@ -54,9 +57,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 @@ -162,29 +166,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->rfree(reset_ctl); + return ops->rfree ? ops->rfree(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) @@ -202,11 +206,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) @@ -224,11 +228,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)