| Message ID | 1613623791-4598-7-git-send-email-shubhrajyoti.datta@xilinx.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: clk-wizard: clock-wizard: Driver updates | expand |
Hi Shubhrajyoti, Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> wrote on Thu, 18 Feb 2021 10:19:50 +0530: > The number of output clocks are configurable in the hardware. > Currently the driver registers the maximum number of outputs. > Fix the same by registering only the outputs that are there. > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > --- > v4: > Assign output in this patch > > drivers/clk/clk-xlnx-clock-wizard.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c > index ed3b0ef..d403a74 100644 > --- a/drivers/clk/clk-xlnx-clock-wizard.c > +++ b/drivers/clk/clk-xlnx-clock-wizard.c > @@ -473,6 +473,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) > unsigned long rate; > const char *clk_name; > struct clk_wzrd *clk_wzrd; > + int outputs; > struct device_node *np = pdev->dev.of_node; > > clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL); > @@ -541,6 +542,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) > goto err_disable_clk; > } > > + outputs = of_property_count_strings(np, "clock-output-names"); A check on outputs validity is probably welcome. Also I usually prefer noutputs or nb_outputs for such variable name, which implies a number rather than an array, but this is personal taste. > /* register div */ > reg = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) & > WZRD_DIVCLK_DIVIDE_MASK) >> WZRD_DIVCLK_DIVIDE_SHIFT; > @@ -562,7 +564,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) > } > > /* register div per output */ > - for (i = WZRD_NUM_OUTPUTS - 1; i >= 0 ; i--) { > + for (i = outputs - 1; i >= 0 ; i--) { > const char *clkout_name; > > if (of_property_read_string_index(np, "clock-output-names", i, > @@ -593,7 +595,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) > if (IS_ERR(clk_wzrd->clkout[i])) { > int j; > > - for (j = i + 1; j < WZRD_NUM_OUTPUTS; j++) > + for (j = i + 1; j < outputs; j++) > clk_unregister(clk_wzrd->clkout[j]); > dev_err(&pdev->dev, > "unable to register divider clock\n"); Thanks, Miquèl
Quoting Miquel Raynal (2021-02-18 00:37:15) > Hi Shubhrajyoti, > > Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> wrote on Thu, 18 Feb > 2021 10:19:50 +0530: > > > The number of output clocks are configurable in the hardware. > > Currently the driver registers the maximum number of outputs. > > Fix the same by registering only the outputs that are there. > > > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > --- > > v4: > > Assign output in this patch > > > > drivers/clk/clk-xlnx-clock-wizard.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c > > index ed3b0ef..d403a74 100644 > > --- a/drivers/clk/clk-xlnx-clock-wizard.c > > +++ b/drivers/clk/clk-xlnx-clock-wizard.c > > @@ -473,6 +473,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > unsigned long rate; > > const char *clk_name; > > struct clk_wzrd *clk_wzrd; > > + int outputs; > > struct device_node *np = pdev->dev.of_node; > > > > clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL); > > @@ -541,6 +542,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > goto err_disable_clk; > > } > > > > + outputs = of_property_count_strings(np, "clock-output-names"); > > A check on outputs validity is probably welcome. > > Also I usually prefer noutputs or nb_outputs for such variable name, > which implies a number rather than an array, but this is personal taste. Ideally we get rid of clock-output-names and generate them at runtime instead based on some combination of device name and something else.
Hi Stephen, On Fri, Feb 19, 2021 at 6:55 AM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Miquel Raynal (2021-02-18 00:37:15) > > Hi Shubhrajyoti, > > > > Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> wrote on Thu, 18 Feb > > 2021 10:19:50 +0530: > > > > > The number of output clocks are configurable in the hardware. > > > Currently the driver registers the maximum number of outputs. > > > Fix the same by registering only the outputs that are there. > > > > > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > > --- > > > v4: > > > Assign output in this patch > > > > > > drivers/clk/clk-xlnx-clock-wizard.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c > > > index ed3b0ef..d403a74 100644 > > > --- a/drivers/clk/clk-xlnx-clock-wizard.c > > > +++ b/drivers/clk/clk-xlnx-clock-wizard.c > > > @@ -473,6 +473,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > > unsigned long rate; > > > const char *clk_name; > > > struct clk_wzrd *clk_wzrd; > > > + int outputs; > > > struct device_node *np = pdev->dev.of_node; > > > > > > clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL); > > > @@ -541,6 +542,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > > goto err_disable_clk; > > > } > > > > > > + outputs = of_property_count_strings(np, "clock-output-names"); > > > > A check on outputs validity is probably welcome. > > > > Also I usually prefer noutputs or nb_outputs for such variable name, > > which implies a number rather than an array, but this is personal taste. > > Ideally we get rid of clock-output-names and generate them at runtime > instead based on some combination of device name and something else. Makes sense. However it may break the current binding. Do you think that shoud be okay?
Quoting Shubhrajyoti Datta (2021-02-21 22:47:26) > Hi Stephen, > > On Fri, Feb 19, 2021 at 6:55 AM Stephen Boyd <sboyd@kernel.org> wrote: > > > > Quoting Miquel Raynal (2021-02-18 00:37:15) > > > Hi Shubhrajyoti, > > > > > > Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> wrote on Thu, 18 Feb > > > 2021 10:19:50 +0530: > > > > > > > The number of output clocks are configurable in the hardware. > > > > Currently the driver registers the maximum number of outputs. > > > > Fix the same by registering only the outputs that are there. > > > > > > > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > > > --- > > > > v4: > > > > Assign output in this patch > > > > > > > > drivers/clk/clk-xlnx-clock-wizard.c | 6 ++++-- > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c > > > > index ed3b0ef..d403a74 100644 > > > > --- a/drivers/clk/clk-xlnx-clock-wizard.c > > > > +++ b/drivers/clk/clk-xlnx-clock-wizard.c > > > > @@ -473,6 +473,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > > > unsigned long rate; > > > > const char *clk_name; > > > > struct clk_wzrd *clk_wzrd; > > > > + int outputs; > > > > struct device_node *np = pdev->dev.of_node; > > > > > > > > clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL); > > > > @@ -541,6 +542,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > > > goto err_disable_clk; > > > > } > > > > > > > > + outputs = of_property_count_strings(np, "clock-output-names"); > > > > > > A check on outputs validity is probably welcome. > > > > > > Also I usually prefer noutputs or nb_outputs for such variable name, > > > which implies a number rather than an array, but this is personal taste. > > > > Ideally we get rid of clock-output-names and generate them at runtime > > instead based on some combination of device name and something else. > > Makes sense. However it may break the current binding. > Do you think that shoud be okay? I think it is OK given that the current binding is for the staging tree. The assumption is those bindings aren't stable.
On Tue, Feb 23, 2021 at 6:32 AM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Shubhrajyoti Datta (2021-02-21 22:47:26) > > Hi Stephen, > > > > On Fri, Feb 19, 2021 at 6:55 AM Stephen Boyd <sboyd@kernel.org> wrote: > > > > > > Quoting Miquel Raynal (2021-02-18 00:37:15) > > > > Hi Shubhrajyoti, > > > > > > > > Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> wrote on Thu, 18 Feb > > > > 2021 10:19:50 +0530: > > > > > > > > > The number of output clocks are configurable in the hardware. > > > > > Currently the driver registers the maximum number of outputs. > > > > > Fix the same by registering only the outputs that are there. > > > > > > > > > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > > > > --- > > > > > v4: > > > > > Assign output in this patch > > > > > > > > > > drivers/clk/clk-xlnx-clock-wizard.c | 6 ++++-- > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c > > > > > index ed3b0ef..d403a74 100644 > > > > > --- a/drivers/clk/clk-xlnx-clock-wizard.c > > > > > +++ b/drivers/clk/clk-xlnx-clock-wizard.c > > > > > @@ -473,6 +473,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > > > > unsigned long rate; > > > > > const char *clk_name; > > > > > struct clk_wzrd *clk_wzrd; > > > > > + int outputs; > > > > > struct device_node *np = pdev->dev.of_node; > > > > > > > > > > clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL); > > > > > @@ -541,6 +542,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > > > > goto err_disable_clk; > > > > > } > > > > > > > > > > + outputs = of_property_count_strings(np, "clock-output-names"); > > > > > > > > A check on outputs validity is probably welcome. > > > > > > > > Also I usually prefer noutputs or nb_outputs for such variable name, > > > > which implies a number rather than an array, but this is personal taste. > > > > > > Ideally we get rid of clock-output-names and generate them at runtime > > > instead based on some combination of device name and something else. > > > > Makes sense. However it may break the current binding. > > Do you think that shoud be okay? > > I think it is OK given that the current binding is for the staging tree. > The assumption is those bindings aren't stable. Updated it in next version.
diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c index ed3b0ef..d403a74 100644 --- a/drivers/clk/clk-xlnx-clock-wizard.c +++ b/drivers/clk/clk-xlnx-clock-wizard.c @@ -473,6 +473,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) unsigned long rate; const char *clk_name; struct clk_wzrd *clk_wzrd; + int outputs; struct device_node *np = pdev->dev.of_node; clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL); @@ -541,6 +542,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) goto err_disable_clk; } + outputs = of_property_count_strings(np, "clock-output-names"); /* register div */ reg = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) & WZRD_DIVCLK_DIVIDE_MASK) >> WZRD_DIVCLK_DIVIDE_SHIFT; @@ -562,7 +564,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) } /* register div per output */ - for (i = WZRD_NUM_OUTPUTS - 1; i >= 0 ; i--) { + for (i = outputs - 1; i >= 0 ; i--) { const char *clkout_name; if (of_property_read_string_index(np, "clock-output-names", i, @@ -593,7 +595,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) if (IS_ERR(clk_wzrd->clkout[i])) { int j; - for (j = i + 1; j < WZRD_NUM_OUTPUTS; j++) + for (j = i + 1; j < outputs; j++) clk_unregister(clk_wzrd->clkout[j]); dev_err(&pdev->dev, "unable to register divider clock\n");
The number of output clocks are configurable in the hardware. Currently the driver registers the maximum number of outputs. Fix the same by registering only the outputs that are there. Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> --- v4: Assign output in this patch drivers/clk/clk-xlnx-clock-wizard.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)