Messages in this thread Patch in this message | | | Date | Thu, 18 Mar 2021 15:44:14 +0100 | From | Michael Tretter <> | Subject | Re: [PATCH][next] soc: xilinx: vcu: remove deadcode on null divider check |
| |
On Thu, 18 Mar 2021 12:48:06 +0000, Colin Ian King wrote: > On 11/02/2021 19:05, Stephen Boyd wrote: > > Quoting Michael Tretter (2021-02-10 23:39:06) > >> On Wed, 10 Feb 2021 19:28:18 -0800, Stephen Boyd wrote: > >>> Quoting Colin King (2021-02-10 10:49:38) > >>>> From: Colin Ian King <colin.king@canonical.com> > >>>> > >>>> The pointer 'divider' has previously been null checked followed by > >>>> a return, hence the subsequent null check is redundant deadcode > >>>> that can be removed. Clean up the code and remove it. > >>>> > >>>> Fixes: 9c789deea206 ("soc: xilinx: vcu: implement clock provider for output clocks") > >>>> Signed-off-by: Colin Ian King <colin.king@canonical.com> > >>>> --- > >>>> drivers/clk/xilinx/xlnx_vcu.c | 3 --- > >>>> 1 file changed, 3 deletions(-) > >>>> > >>>> diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c > >>>> index d66b1315114e..607936d7a413 100644 > >>>> --- a/drivers/clk/xilinx/xlnx_vcu.c > >>>> +++ b/drivers/clk/xilinx/xlnx_vcu.c > >>>> @@ -512,9 +512,6 @@ static void xvcu_clk_hw_unregister_leaf(struct clk_hw *hw) > >>>> > >>>> mux = clk_hw_get_parent(divider); > >>>> clk_hw_unregister_mux(mux); > >>>> - if (!divider) > >>>> - return; > >>>> - > >>> > >>> This code is pretty confusing. Waiting for m.tretter@pengutronix.de to > >>> reply > >> > >> Can you elaborate what you find confusing about this code. I would gladly try > >> to clarify and improve the code. > > > > The fact that pointers are being checked and then bailing out of the > > function early, vs. doing something if the pointer is non-NULL. > > > >> > >> What happens here is that the driver registers a mux -> divider -> gate chain > >> for each output clock, but only stores the gate clock. When unregistering the > >> clocks, the driver starts at the gate and walks up to the mux while > >> unregistering the clocks. > >> > > OK, so I think I understand this better, should the order of > unregisteration be as follows: > > diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c > index d66b1315114e..66bac8421460 100644 > --- a/drivers/clk/xilinx/xlnx_vcu.c > +++ b/drivers/clk/xilinx/xlnx_vcu.c > @@ -511,11 +511,11 @@ static void xvcu_clk_hw_unregister_leaf(struct > clk_hw *hw) > return; > > mux = clk_hw_get_parent(divider); > - clk_hw_unregister_mux(mux); > - if (!divider) > + clk_hw_unregister_mux(divider);
The order is correct, but this must be:
clk_hw_unregister_divider(divider);
> + if (!mux) > return; > > - clk_hw_unregister_divider(divider); > + clk_hw_unregister_divider(mux); >
clk_hw_unregister_mux(mux);
Taking the confusion expressed by Stephen into account, I rewrote the entire function to clarify what is happening in this function. Please take a look that patch [0] and tell me, if it is now easier to understand.
[0] 20210318144230.3438009-1-m.tretter@pengutronix.de
Michael
| |