| Message ID | 20210309122748.31842-3-kishon@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | TI/Cadence: Add Sierra/Torrent SERDES driver | expand |
Hi Kishon, On Tue, 9 Mar 2021 at 05:27, Kishon Vijay Abraham I <kishon@ti.com> wrote: > > From: Jean-Jacques Hiblot <jjhiblot@ti.com> > > 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(-) Why do you want this? This patch is missing the motivation which should be at the start of the commit message. Regards, Simon
Hi Simon, On 12/03/21 10:15 am, Simon Glass wrote: > Hi Kishon, > > On Tue, 9 Mar 2021 at 05:27, Kishon Vijay Abraham I <kishon@ti.com> wrote: >> >> From: Jean-Jacques Hiblot <jjhiblot@ti.com> >> >> 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(-) > > Why do you want this? This patch is missing the motivation which > should be at the start of the commit message. This is for "optional" reset controllers used by peripheral drivers. This will help avoid adding checks in peripheral drivers. Thanks Kishon
Hi Kishon, On Mon, 22 Mar 2021 at 18:11, Kishon Vijay Abraham I <kishon@ti.com> wrote: > > Hi Simon, > > On 12/03/21 10:15 am, Simon Glass wrote: > > Hi Kishon, > > > > On Tue, 9 Mar 2021 at 05:27, Kishon Vijay Abraham I <kishon@ti.com> wrote: > >> > >> From: Jean-Jacques Hiblot <jjhiblot@ti.com> > >> > >> 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(-) > > > > Why do you want this? This patch is missing the motivation which > > should be at the start of the commit message. > > This is for "optional" reset controllers used by peripheral drivers. > This will help avoid adding checks in peripheral drivers. Can you please be more specific? Reset drivers are required to have operations. Only a very few uclasses allow the operations pointer to be NULL. Regards, Simon
Hi Simon, On 23/03/21 6:26 am, Simon Glass wrote: > Hi Kishon, > > On Mon, 22 Mar 2021 at 18:11, Kishon Vijay Abraham I <kishon@ti.com> wrote: >> >> Hi Simon, >> >> On 12/03/21 10:15 am, Simon Glass wrote: >>> Hi Kishon, >>> >>> On Tue, 9 Mar 2021 at 05:27, Kishon Vijay Abraham I <kishon@ti.com> wrote: >>>> >>>> From: Jean-Jacques Hiblot <jjhiblot@ti.com> >>>> >>>> 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(-) >>> >>> Why do you want this? This patch is missing the motivation which >>> should be at the start of the commit message. >> >> This is for "optional" reset controllers used by peripheral drivers. >> This will help avoid adding checks in peripheral drivers. > > Can you please be more specific? Sorry for not being clear. Cadence Torrent SERDES has a reset line for APB (APB_RESET). This reset line is connected in Cadence platform but not connected in TI platform which also uses the Torrent SERDES. So in the Cadence Torrent driver we use devm_reset_control_get_optional() which returns NULL when used in TI platform. However if we use the return value of devm_reset_control_get_optional() directly in reset_ops (for TI), it will result in abort. This patch helps to avoid adding additional checks before invoking reset_ops. Thanks Kishon
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)