diff mbox series

[v2,2/2] clk: gate: Add missing fw_name for clk_gate_register_test_parent_data_legacy

Message ID 20230131160829.23369-2-ansuelsmth@gmail.com
State New
Headers
Series [v2,1/2] clk: Warn and add workaround on misuse of .parent_data with .name only |

Commit Message

Christian Marangi Jan. 31, 2023, 4:08 p.m. UTC 
  Fix warning for missing .fw_name in parent_data based on names.
It's wrong to define only .name since clk core expect always .fw_name to
be defined.

Reported-by: kernel test robot <oliver.sang@intel.com>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/clk/clk-gate_test.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Christian Marangi Feb. 10, 2023, 6:39 p.m. UTC | #1 
On Fri, Feb 10, 2023 at 04:52:36PM -0800, Stephen Boyd wrote:
> Quoting Christian Marangi (2023-01-31 08:08:29)
> > Fix warning for missing .fw_name in parent_data based on names.
> > It's wrong to define only .name since clk core expect always .fw_name to
> > be defined.
> > 
> > Reported-by: kernel test robot <oliver.sang@intel.com>
> 
> What was the report?
>

With the previous patch applied kernel test robot report the WARN for
declaring a parent_data with .name but no .fw_name.

> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/clk/clk-gate_test.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/clk/clk-gate_test.c b/drivers/clk/clk-gate_test.c
> > index e136aaad48bf..a0a63cd4ce0b 100644
> > --- a/drivers/clk/clk-gate_test.c
> > +++ b/drivers/clk/clk-gate_test.c
> > @@ -74,6 +74,7 @@ static void clk_gate_register_test_parent_data_legacy(struct kunit *test)
> >                                             1000000);
> >         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
> >         pdata.name = "test_parent";
> > +       pdata.fw_name = "test_parent";
> >  
> >         ret = clk_hw_register_gate_parent_data(NULL, "test_gate", &pdata, 0,
> 
> We don't pass a 'dev' here, so the pdata.index isn't looked at. I
> suppose we can assign .index to -1 to be more explicit, but because
> there isn't a device used for registering, we won't try to use the
> .index. Instead we'll try to use .fw_name for clkdev, of which there
> won't be a clkdev lookup either. Eventually we'll fallback to the .name
> lookup, and it will be fine.

Problem is that from what we observed, it won't fallback to .name if
.fw_name is not declared.

But it will work if .fw_name is declared but not exposed by DT. (and
will correctly fallback to .name as .fw_name is not found)
(but this is to explain why the change in the other patch is needed so I
may be OT here)

> 
> We need tests that exercises the 'dev' path and also the DT path and the
> clkdev path. I was thinking about working on that outside of the gate
> test though, and just having a generic clk test for that with simple
> clk_ops that do basically nothing.
Stephen Boyd Feb. 11, 2023, 12:52 a.m. UTC | #2 
Quoting Christian Marangi (2023-01-31 08:08:29)
> Fix warning for missing .fw_name in parent_data based on names.
> It's wrong to define only .name since clk core expect always .fw_name to
> be defined.
> 
> Reported-by: kernel test robot <oliver.sang@intel.com>

What was the report?

> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/clk/clk-gate_test.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/clk/clk-gate_test.c b/drivers/clk/clk-gate_test.c
> index e136aaad48bf..a0a63cd4ce0b 100644
> --- a/drivers/clk/clk-gate_test.c
> +++ b/drivers/clk/clk-gate_test.c
> @@ -74,6 +74,7 @@ static void clk_gate_register_test_parent_data_legacy(struct kunit *test)
>                                             1000000);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
>         pdata.name = "test_parent";
> +       pdata.fw_name = "test_parent";
>  
>         ret = clk_hw_register_gate_parent_data(NULL, "test_gate", &pdata, 0,

We don't pass a 'dev' here, so the pdata.index isn't looked at. I
suppose we can assign .index to -1 to be more explicit, but because
there isn't a device used for registering, we won't try to use the
.index. Instead we'll try to use .fw_name for clkdev, of which there
won't be a clkdev lookup either. Eventually we'll fallback to the .name
lookup, and it will be fine.

We need tests that exercises the 'dev' path and also the DT path and the
clkdev path. I was thinking about working on that outside of the gate
test though, and just having a generic clk test for that with simple
clk_ops that do basically nothing.
diff mbox series

Patch

diff --git a/drivers/clk/clk-gate_test.c b/drivers/clk/clk-gate_test.c
index e136aaad48bf..a0a63cd4ce0b 100644
--- a/drivers/clk/clk-gate_test.c
+++ b/drivers/clk/clk-gate_test.c
@@ -74,6 +74,7 @@  static void clk_gate_register_test_parent_data_legacy(struct kunit *test)
 					    1000000);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
 	pdata.name = "test_parent";
+	pdata.fw_name = "test_parent";
 
 	ret = clk_hw_register_gate_parent_data(NULL, "test_gate", &pdata, 0,
 					       NULL, 0, 0, NULL);