| Message ID | 20240822161452.1780149-2-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: x86: lpss-atom: A couple of cleanups | expand |
Quoting Andy Shevchenko (2024-08-22 09:14:07) > diff --git a/drivers/clk/x86/clk-lpss-atom.c b/drivers/clk/x86/clk-lpss-atom.c > index aa9d0bb98f8b..c70088be72d1 100644 > --- a/drivers/clk/x86/clk-lpss-atom.c > +++ b/drivers/clk/x86/clk-lpss-atom.c > @@ -12,20 +12,24 @@ > #include <linux/module.h> > #include <linux/platform_data/x86/clk-lpss.h> > #include <linux/platform_device.h> > +#include <linux/units.h> > > static int lpss_atom_clk_probe(struct platform_device *pdev) > { > struct lpss_clk_data *drvdata; > struct clk *clk; > + u32 rate; Do we need a local variable? > > drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL); > if (!drvdata) > return -ENOMEM; > > + /* Default frequency is 100MHz */ > + rate = 100 * HZ_PER_MHZ; > + > /* LPSS free running clock */ > drvdata->name = "lpss_clk"; > - clk = clk_register_fixed_rate(&pdev->dev, drvdata->name, NULL, > - 0, 100000000); > + clk = clk_register_fixed_rate(&pdev->dev, drvdata->name, NULL, 0, rate); This should be a one line patch. > if (IS_ERR(clk)) > return PTR_ERR(clk); >
On Tue, Aug 27, 2024 at 05:21:00PM -0700, Stephen Boyd wrote: > Quoting Andy Shevchenko (2024-08-22 09:14:07) ... > > static int lpss_atom_clk_probe(struct platform_device *pdev) > > { > > struct lpss_clk_data *drvdata; > > struct clk *clk; > > + u32 rate; > > Do we need a local variable? Hmm... The idea was to allow retrieving this via device properties, that's why a separate variable, but that patch wasn't included here. Nevertheless, despite above the separate variable makes code a bit better to read as we can see what is this value about. > > drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL); > > if (!drvdata) > > return -ENOMEM; > > > > + /* Default frequency is 100MHz */ > > + rate = 100 * HZ_PER_MHZ; > > + > > /* LPSS free running clock */ > > drvdata->name = "lpss_clk"; > > - clk = clk_register_fixed_rate(&pdev->dev, drvdata->name, NULL, > > - 0, 100000000); > > + clk = clk_register_fixed_rate(&pdev->dev, drvdata->name, NULL, 0, rate); > > This should be a one line patch. I don't get this. You mean the entire thingy? It's possible, but as I mentioned above there is a rationale for making it with a temporary variable. > > if (IS_ERR(clk)) > > return PTR_ERR(clk);
Quoting Andy Shevchenko (2024-08-28 06:11:55) > On Tue, Aug 27, 2024 at 05:21:00PM -0700, Stephen Boyd wrote: > > Quoting Andy Shevchenko (2024-08-22 09:14:07) > > ... > > > > static int lpss_atom_clk_probe(struct platform_device *pdev) > > > { > > > struct lpss_clk_data *drvdata; > > > struct clk *clk; > > > + u32 rate; > > > > Do we need a local variable? > > Hmm... The idea was to allow retrieving this via device properties, that's why > a separate variable, but that patch wasn't included here. > > Nevertheless, despite above the separate variable makes code a bit better to > read as we can see what is this value about. > > > > drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL); > > > if (!drvdata) > > > return -ENOMEM; > > > > > > + /* Default frequency is 100MHz */ > > > + rate = 100 * HZ_PER_MHZ; > > > + > > > /* LPSS free running clock */ > > > drvdata->name = "lpss_clk"; > > > - clk = clk_register_fixed_rate(&pdev->dev, drvdata->name, NULL, > > > - 0, 100000000); > > > + clk = clk_register_fixed_rate(&pdev->dev, drvdata->name, NULL, 0, rate); > > > > This should be a one line patch. > > I don't get this. You mean the entire thingy? Yes. > > It's possible, but as I mentioned above there is a rationale for making it with > a temporary variable. The rationale looks like future code will want a local variable. When that happens the local variable would make sense.
diff --git a/drivers/clk/x86/clk-lpss-atom.c b/drivers/clk/x86/clk-lpss-atom.c index aa9d0bb98f8b..c70088be72d1 100644 --- a/drivers/clk/x86/clk-lpss-atom.c +++ b/drivers/clk/x86/clk-lpss-atom.c @@ -12,20 +12,24 @@ #include <linux/module.h> #include <linux/platform_data/x86/clk-lpss.h> #include <linux/platform_device.h> +#include <linux/units.h> static int lpss_atom_clk_probe(struct platform_device *pdev) { struct lpss_clk_data *drvdata; struct clk *clk; + u32 rate; drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL); if (!drvdata) return -ENOMEM; + /* Default frequency is 100MHz */ + rate = 100 * HZ_PER_MHZ; + /* LPSS free running clock */ drvdata->name = "lpss_clk"; - clk = clk_register_fixed_rate(&pdev->dev, drvdata->name, NULL, - 0, 100000000); + clk = clk_register_fixed_rate(&pdev->dev, drvdata->name, NULL, 0, rate); if (IS_ERR(clk)) return PTR_ERR(clk);
Use predefined constants from units.h to make code robust against typos like how many zeros to put. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/clk/x86/clk-lpss-atom.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)