Re: [PATCH] regulator: s2m,s5m: constify regulator_ops structures
On Wed, 23 Dec 2015, Mark Brown wrote: > On Sat, Dec 19, 2015 at 03:58:00PM +0100, Julia Lawall wrote: > > The regulator_ops structures are never modified, so declare them as const. > > > > Done with the help of Coccinelle. > > This doesn't apply against current code, please check and resend. It works fine for me on today's linux-next (80c75a0f1d). What goes wrong? julia -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] regulator: s2m,s5m: constify regulator_ops structures
The regulator_ops structures are never modified, so declare them as const.
Done with the help of Coccinelle.
Signed-off-by: Julia Lawall
---
drivers/regulator/s2mpa01.c |4 ++--
drivers/regulator/s2mps11.c | 16
drivers/regulator/s5m8767.c |4 ++--
3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/regulator/s2mpa01.c b/drivers/regulator/s2mpa01.c
index 92f8875..dc2105c 100644
--- a/drivers/regulator/s2mpa01.c
+++ b/drivers/regulator/s2mpa01.c
@@ -212,7 +212,7 @@ ramp_disable:
1 << enable_shift, 0);
}
-static struct regulator_ops s2mpa01_ldo_ops = {
+static const struct regulator_ops s2mpa01_ldo_ops = {
.list_voltage = regulator_list_voltage_linear,
.map_voltage= regulator_map_voltage_linear,
.is_enabled = regulator_is_enabled_regmap,
@@ -223,7 +223,7 @@ static struct regulator_ops s2mpa01_ldo_ops = {
.set_voltage_time_sel = regulator_set_voltage_time_sel,
};
-static struct regulator_ops s2mpa01_buck_ops = {
+static const struct regulator_ops s2mpa01_buck_ops = {
.list_voltage = regulator_list_voltage_linear,
.map_voltage= regulator_map_voltage_linear,
.is_enabled = regulator_is_enabled_regmap,
diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
index 3242ffc..e5be2ad 100644
--- a/drivers/regulator/s2mps11.c
+++ b/drivers/regulator/s2mps11.c
@@ -236,7 +236,7 @@ ramp_disable:
1 << enable_shift, 0);
}
-static struct regulator_ops s2mps11_ldo_ops = {
+static const struct regulator_ops s2mps11_ldo_ops = {
.list_voltage = regulator_list_voltage_linear,
.map_voltage= regulator_map_voltage_linear,
.is_enabled = regulator_is_enabled_regmap,
@@ -247,7 +247,7 @@ static struct regulator_ops s2mps11_ldo_ops = {
.set_voltage_time_sel = regulator_set_voltage_time_sel,
};
-static struct regulator_ops s2mps11_buck_ops = {
+static const struct regulator_ops s2mps11_buck_ops = {
.list_voltage = regulator_list_voltage_linear,
.map_voltage= regulator_map_voltage_linear,
.is_enabled = regulator_is_enabled_regmap,
@@ -373,7 +373,7 @@ static const struct regulator_desc s2mps11_regulators[] = {
regulator_desc_s2mps11_buck6_10(10, MIN_750_MV, STEP_12_5_MV),
};
-static struct regulator_ops s2mps14_reg_ops;
+static const struct regulator_ops s2mps14_reg_ops;
#define regulator_desc_s2mps13_ldo(num, min, step, min_sel) { \
.name = "LDO"#num,\
@@ -580,7 +580,7 @@ static int s2mps14_regulator_set_suspend_disable(struct
regulator_dev *rdev)
rdev->desc->enable_mask, state);
}
-static struct regulator_ops s2mps14_reg_ops = {
+static const struct regulator_ops s2mps14_reg_ops = {
.list_voltage = regulator_list_voltage_linear,
.map_voltage= regulator_map_voltage_linear,
.is_enabled = regulator_is_enabled_regmap,
@@ -662,7 +662,7 @@ static const struct regulator_desc s2mps14_regulators[] = {
S2MPS14_BUCK1235_START_SEL),
};
-static struct regulator_ops s2mps15_reg_ldo_ops = {
+static const struct regulator_ops s2mps15_reg_ldo_ops = {
.list_voltage = regulator_list_voltage_linear_range,
.map_voltage= regulator_map_voltage_linear_range,
.is_enabled = regulator_is_enabled_regmap,
@@ -672,7 +672,7 @@ static struct regulator_ops s2mps15_reg_ldo_ops = {
.set_voltage_sel= regulator_set_voltage_sel_regmap,
};
-static struct regulator_ops s2mps15_reg_buck_ops = {
+static const struct regulator_ops s2mps15_reg_buck_ops = {
.list_voltage = regulator_list_voltage_linear_range,
.map_voltage= regulator_map_voltage_linear_range,
.is_enabled = regulator_is_enabled_regmap,
@@ -866,7 +866,7 @@ static int s2mpu02_set_ramp_delay(struct regulator_dev
*rdev, int ramp_delay)
ramp_val << ramp_shift);
}
-static struct regulator_ops s2mpu02_ldo_ops = {
+static const struct regulator_ops s2mpu02_ldo_ops = {
.list_voltage = regulator_list_voltage_linear,
.map_voltage= regulator_map_voltage_linear,
.is_enabled = regulator_is_enabled_regmap,
@@ -878,7 +878,7 @@ static struct regulator_ops s2mpu02_ldo_ops = {
.set_suspend_disable= s2mps14_regulator_set_suspend_disable,
};
-static struct regulator_ops s2mpu02_buck_ops = {
+static const struct regulator_ops s2mpu02_buck_ops = {
.list_voltage = regulator_list_voltage_linear,
.map_voltage= regulator_map_v
Re: [PATCH v2 11/25] coccinelle: nand: detect and correct drivers embedding an mtd_info object
On Tue, 1 Dec 2015, Boris Brezillon wrote:
> Add nand-priv-no-mtd.cocci to detect and correct NAND controller drivers
> directly embedding an mtd_info struct in their private struct.
>
> Signed-off-by: Boris Brezillon
> Cc: Julia Lawall
> ---
> Hi Julia,
>
> Not sure this is the correct way to detect and fix offending drivers,
> but I get some warnings when launching coccicheck in org or report mode:
>
> "warning: fix2: inherited metavariable __chipfield not used in the -, +,
> or context code"
>
> Note that I don't get those warnings when running in patch mode.
>
> Any idea (feel free to propose a better solution to detect and fix those
> offending drivers)?
Hi,
Is this code generated with sgen? If so, could you send me the original
semantic patch?
Another thing that is immediately apparent is that you have <... ...> on
the outside of one of the rules. This should never be needed.
The warning suggests that your org and report versions are not doing as
much as the patch version. If you have used sgen to generate the semantic
patch then that would be strange. If you have hand written the whole
thing, then maybe you could simplify it to just do the patch version, and
then I can check it and run sgen on it to make a complete version.
julia
>
> Best Regards,
>
> Boris
> ---
> scripts/coccinelle/api/nand-priv-no-mtd.cocci | 91
> +++
> 1 file changed, 91 insertions(+)
> create mode 100644 scripts/coccinelle/api/nand-priv-no-mtd.cocci
>
> diff --git a/scripts/coccinelle/api/nand-priv-no-mtd.cocci
> b/scripts/coccinelle/api/nand-priv-no-mtd.cocci
> new file mode 100644
> index 000..b2c0c22
> --- /dev/null
> +++ b/scripts/coccinelle/api/nand-priv-no-mtd.cocci
> @@ -0,0 +1,91 @@
> +/// Fix NAND controller drivers declaring their own mtd_info struct instead
> +/// of the one provided in struct nand_chip.
> +///
> +// Confidence: Low
> +// Copyright: (C) 2015 Boris Brezillon GPL v2.
> +
> +virtual patch
> +virtual context
> +virtual org
> +virtual report
> +
> +@match1@
> +identifier __chipfield, __mtdfield;
> +type __type;
> +@@
> +(
> + __type {
> + ...
> + struct nand_chip __chipfield;
> + ...
> + struct mtd_info __mtdfield;
> + ...
> + };
> +|
> + __type {
> + ...
> + struct mtd_info __mtdfield;
> + ...
> + struct nand_chip __chipfield;
> + ...
> + };
> +)
> +
> +@fix1 depends on match1 && patch && !context && !org && !report@
> +identifier match1.__mtdfield;
> +type match1.__type;
> +@@
> + __type {
> + ...
> +-struct mtd_info __mtdfield;
> + ...
> + };
> +
> +@fix2 depends on match1 && patch && !context && !org && !report@
> +identifier match1.__chipfield, match1.__mtdfield;
> +identifier __subfield;
> +type match1.__type;
> +__type *__priv;
> +@@
> +<...
> +(
> +-__priv->__mtdfield.__subfield
> ++nand_to_mtd(&__priv->__chipfield)->__subfield
> +|
> +-&(__priv->__mtdfield)
> ++nand_to_mtd(&__priv->__chipfield)
> +)
> +...>
> +
> +//
>
> +
> +@fix1_context depends on match1 && !patch && (context || org || report)@
> +identifier match1.__mtdfield;
> +type match1.__type;
> +position j0;
> +@@
> +
> + __type {
> + ...
> +*struct mtd_info __mtdfield@j0;
> + ...
> + };
> +
> +//
>
> +
> +@script:python fix1_org depends on org@
> +j0 << fix1_context.j0;
> +@@
> +
> +msg = "struct nand_chip already embeds an mtd_info object (use
> nand_to_mtd())."
> +coccilib.org.print_todo(j0[0], msg)
> +
> +//
>
> +
> +@script:python fix1_report depends on report@
> +j0 << fix1_context.j0;
> +@@
> +
> +msg = "struct nand_chip already embeds an mtd_info object (use
> nand_to_mtd())."
> +coccilib.report.print_report(j0[0], msg)
> +
> --
> 2.1.4
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/27] mtd: nand: use the mtd instance embedded in struct nand_chip
On Tue, 17 Nov 2015, Boris Brezillon wrote:
> Hi Julia,
>
> On Tue, 17 Nov 2015 10:05:03 +0100 (CET)
> Julia Lawall wrote:
>
> > > > (This isn't the worst one, but it just happens to be one of the first.)
> > > > There are many cases where the typical style would be to declare a new
> > > > variable at the top of the function, where you perform the
> > > > macro/function-call to convert from one abstraction to another. Like
> > > >
> > > > static int nfc_set_sram_bank(struct atmel_nand_host *host, unsigned int
> > > > bank)
> > > > {
> > > > struct mtd_info *mtd = nand_to_mtd(&hot->nand_chip);
> > > > ...
> > > >
> > > > and then use it later. Can that be done very easily?
> > > >
> > > > > return -EINVAL;
> > > > > nfc_writel(host->nfc->hsmc_regs, BANK,
> > > > > ATMEL_HSMC_NFC_BANK1);
> > > > > } else {
> > > >
> > > > ...
> > >
> > > Honestly, I don't know how to do that with a coccinelle script, and it
> > > will probably take me more time to find how to do it than addressing
> > > those problems manually.
> > >
> > > Julia, could you give us some hint?
> >
> > Probably something like the following would be easiest. You can just run
> > it after your other transformations:
> >
> > @r exists@
> > identifier f;
> > expression e;
> > @@
> >
> > f(...) { <+... nand_to_mtd(e) ...+> }
> >
> > @@
> > identifier r.f;
> > expression r.e;
> > @@
> >
> > f(...) {
> > + struct mtd_info *mtd = nand_to_mtd(e);
> > ...
> > }
>
> Thanks for the the suggestion...
>
> >
> > This won't work if there is more than one possible value of e. If that is
> > likely, then I could come up with something more complex. It also assumes
> > that you want to convert all such calls. If you only want to convert calls
> > that occur in a particular context, eg a field reference, then you could
> > enhance the pattern inside the <+... ...+> in the first rule.
>
> ... unfortunately, as you've guessed, it's a bit more complicated.
> This mtd local variable is usually extracted from another local
> variable (struct nand_chip *chip), so we have to declare
>
> struct mtd_info *mtd = nand_to_mtd(e);
>
> after
>
> struct nand_chip *chip = expression;
>
> But this is not the only particular case. Sometime the chip variable is
> not assigned where it's declared (especially when it is dynamically
> allocated), and sometime it does not exist at all (we just extract it
> from a private struct: &priv->chip).
> The mtd variable can also be declared in sub-code blocks (loop or
> conditional statements).
> And, as you stated, we might want to keep direct calls to nand_to_mtd()
> if it's only called once in a function context.
>
> I'm pretty sure we could describe all those cases with specific context
> description, but I must admit that it takes less time for me to fix
> those specific cases manually than figuring out how to describe them
> correctly in a coccinelle script :).
>
> This being said, I'd be happy to see how you would handle all these
> different cases.
Maybe the following would be useful. It won't handle all the cases, but
maybe it will take care of a good number of the nontrivial ones.
julia
@r@
local idexpression x;
identifier fld,y;
@@
nand_to_mtd(&x->fld)->y
@exists@
type T;
local idexpression r.x;
identifier xx,r.y,r.fld;
@@
T xx;
+ struct mtd_info *mtd = nand_to_mtd(&x->fld);
...
nand_to_mtd(&x@xx->fld)->y
@@
local idexpression r.x;
identifier r.fld;
@@
struct mtd_info *mtd = nand_to_mtd(&x->fld);
<...
- nand_to_mtd(&x->fld)
+ mtd
...>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/27] mtd: nand: use the mtd instance embedded in struct nand_chip
> > (This isn't the worst one, but it just happens to be one of the first.)
> > There are many cases where the typical style would be to declare a new
> > variable at the top of the function, where you perform the
> > macro/function-call to convert from one abstraction to another. Like
> >
> > static int nfc_set_sram_bank(struct atmel_nand_host *host, unsigned int
> > bank)
> > {
> > struct mtd_info *mtd = nand_to_mtd(&hot->nand_chip);
> > ...
> >
> > and then use it later. Can that be done very easily?
> >
> > > return -EINVAL;
> > > nfc_writel(host->nfc->hsmc_regs, BANK, ATMEL_HSMC_NFC_BANK1);
> > > } else {
> >
> > ...
>
> Honestly, I don't know how to do that with a coccinelle script, and it
> will probably take me more time to find how to do it than addressing
> those problems manually.
>
> Julia, could you give us some hint?
Probably something like the following would be easiest. You can just run
it after your other transformations:
@r exists@
identifier f;
expression e;
@@
f(...) { <+... nand_to_mtd(e) ...+> }
@@
identifier r.f;
expression r.e;
@@
f(...) {
+ struct mtd_info *mtd = nand_to_mtd(e);
...
}
This won't work if there is more than one possible value of e. If that is
likely, then I could come up with something more complex. It also assumes
that you want to convert all such calls. If you only want to convert calls
that occur in a particular context, eg a field reference, then you could
enhance the pattern inside the <+... ...+> in the first rule.
julia
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] delete unneeded of_node_put
Device node iterators perform an of_node_put on each iteration, so putting
an of_node_put before going around to the next iteration results in a
double put.
The complete semantic patch that fixes this problem is as follows
(http://coccinelle.lip6.fr):
//
@r exists@
expression e1,e2;
local idexpression n;
iterator name for_each_node_by_name, for_each_node_by_type,
for_each_compatible_node, for_each_matching_node,
for_each_matching_node_and_match, for_each_child_of_node,
for_each_available_child_of_node, for_each_node_with_property;
iterator i;
position p1,p2;
statement S;
@@
(
(
for_each_node_by_name(n,e1) S
|
for_each_node_by_type(n,e1) S
|
for_each_compatible_node(n,e1,e2) S
|
for_each_matching_node(n,e1) S
|
for_each_matching_node_and_match(n,e1,e2) S
|
for_each_child_of_node(e1,n) S
|
for_each_available_child_of_node(e1,n) S
|
for_each_node_with_property(n,e1) S
)
&
i@p1(...) {
... when != of_node_get(n)
when any
of_node_put@p2(n);
... when any
}
)
@s exists@
local idexpression r.n;
statement S;
position r.p1,r.p2;
iterator i;
@@
of_node_put@p2(n);
... when any
i@p1(..., n, ...)
S
@depends on s@
local idexpression n;
position r.p2;
@@
- of_node_put@p2(n);
//
---
arch/arm/mach-exynos/pm_domains.c |8 +++-
drivers/soc/qcom/smd.c|4 +---
drivers/video/fbdev/omap2/dss/omapdss-boot-init.c |4 +---
3 files changed, 5 insertions(+), 11 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] ARM: EXYNOS: delete unneeded of_node_put
Device node iterators perform an of_node_put on each iteration, so putting
an of_node_put before going around to the next iteration results in a
double put.
Problem found using Coccinelle.
Signed-off-by: Julia Lawall
---
arch/arm/mach-exynos/pm_domains.c |8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-exynos/pm_domains.c
b/arch/arm/mach-exynos/pm_domains.c
index 4a87e86..7c21760 100644
--- a/arch/arm/mach-exynos/pm_domains.c
+++ b/arch/arm/mach-exynos/pm_domains.c
@@ -200,15 +200,15 @@ no_clk:
args.args_count = 0;
child_domain = of_genpd_get_from_provider(&args);
if (IS_ERR(child_domain))
- goto next_pd;
+ continue;
if (of_parse_phandle_with_args(np, "power-domains",
"#power-domain-cells", 0, &args) != 0)
- goto next_pd;
+ continue;
parent_domain = of_genpd_get_from_provider(&args);
if (IS_ERR(parent_domain))
- goto next_pd;
+ continue;
if (pm_genpd_add_subdomain(parent_domain, child_domain))
pr_warn("%s failed to add subdomain: %s\n",
@@ -216,8 +216,6 @@ no_clk:
else
pr_info("%s has as child subdomain: %s.\n",
parent_domain->name, child_domain->name);
-next_pd:
- of_node_put(np);
}
return 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] thermal: exynos: fix semicolon.cocci warnings
Remove unneeded semicolons.
Generated by: scripts/coccinelle/misc/semicolon.cocci
Signed-off-by: Julia Lawall
Signed-off-by: Fengguang Wu
---
exynos_tmu.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -548,7 +548,7 @@ static int exynos5433_tmu_initialize(str
default:
pdata->cal_type = TYPE_ONE_POINT_TRIMMING;
break;
- };
+ }
dev_info(&pdev->dev, "Calibration type is %d-point calibration\n",
cal_type ? 2 : 1);
@@ -1363,7 +1363,7 @@ static int exynos_tmu_probe(struct platf
break;
default:
break;
- };
+ }
ret = exynos_tmu_initialize(pdev);
if (ret) {
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 08/11] drm/exynos: mic: fix error return code
Propagate error code on failure.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
//
@@
identifier ret; expression e1,e2;
@@
(
if (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
//
Signed-off-by: Julia Lawall
---
drivers/gpu/drm/exynos/exynos_drm_mic.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_mic.c
b/drivers/gpu/drm/exynos/exynos_drm_mic.c
index 8994eab..ddf0b6c 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_mic.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_mic.c
@@ -432,6 +432,7 @@ int exynos_mic_probe(struct platform_device *pdev)
"samsung,disp-syscon");
if (IS_ERR(mic->sysreg)) {
DRM_ERROR("mic: Failed to get system register.\n");
+ ret = PTR_ERR(mic->sysreg);
goto err;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 00/11] fix error return code
The complate semantic patch that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
//
@ok exists@
identifier f,ret,i;
expression e;
constant c;
@@
// identify a function that returns a negative return value at least once.
f(...) {
... when any
(
return -c@i;
|
ret = -c@i;
... when != ret = e
return ret;
|
if (ret < 0) { ... return ret; }
)
... when any
}
@r exists@
identifier ret,ok.f,fn;
expression e1,e2,e3,e4,e5,e6,x;
statement S,S1;
position p1,p2,p3;
@@
// identify a case where the return variable is set to a non-negative value
// and then returned in error-handling code
f(...) {
... when any
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != \(ret = e1\|ret++\|ret--\|ret+=e1\|ret-=e1\)
when != &ret
when any
(
if (<+... ret = e5 ...+>) S1
|
if (<+... &ret ...+>) S1
|
if@p2(<+...x = fn(...)...+>)
{
... when != ret = e6
when forall
return@p3 ret;
}
|
break;
|
x = fn(...)
... when != \(ret = e4\|ret++\|ret--\|ret+=e4\|ret-=e4\)
when != &ret
(
if (<+... ret = e3 ...+>) S
|
if (<+... &ret ...+>) S
|
if@p2(<+...\(x != 0\|x < 0\|x == NULL\|IS_ERR(x)\)...+>)
{
... when != ret = e2
when forall
return@p3 ret;
}
)
)
... when any
}
@printer depends on r@
position p;
identifier ok.f,pr;
constant char [] c;
@@
f(...) { <...pr@p(...,c,...)...> }
@bad0 exists@
identifier r.ret,ok.f,g != {ERR_PTR,IS_ERR};
position p != printer.p;
@@
f(...) { ... when any
g@p(...,ret,...)
... when any
}
@bad depends on !bad0 exists@
position r.p1,r.p2;
statement S1,S2;
identifier r.ret;
expression e1;
@@
// ignore the above if there is some path where the variable is set to
// something else
(
if@p1 (\(ret < 0\|ret != 0\)) S1
|
ret@p1 = 0
)
... when any
\(ret = e1\|ret++\|ret--\|ret+=e1\|ret-=e1\|&ret\)
... when any
if@p2(...) S2
@bad1 depends on !bad0 && !bad exists@
position r.p2;
statement S2;
identifier r.ret;
expression e1;
constant c;
@@
ret = -c
... when != \(ret = e1\|ret++\|ret--\|ret+=e1\|ret-=e1\)
when != &ret
when any
if@p2(...) S2
@bad2 depends on !bad0 && !bad && !bad1 exists@
position r.p1,r.p2;
identifier r.ret;
expression e1;
statement S2;
constant c;
@@
// likewise ignore it if there has been an intervening return
ret@p1 = 0
... when != if (...) { ... ret = e1 ... return ret; }
when != if (...) { ... return -c; }
when any
if@p2(...) S2
@script:python depends on !bad0 && !bad && !bad1 && !bad2@
p1 << r.p1;
p2 << r.p2;
p3 << r.p3;
@@
cocci.print_main("",p1)
cocci.print_secs("",p2)
cocci.print_secs("",p3)
//
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/20] mtd: s3c2410: fix misspelling of current function in string
> > diff --git a/drivers/mtd/nand/s3c2410.c b/drivers/mtd/nand/s3c2410.c
> > index 35aef5e..0a9c41f 100644
> > --- a/drivers/mtd/nand/s3c2410.c
> > +++ b/drivers/mtd/nand/s3c2410.c
> > @@ -948,7 +948,7 @@ static int s3c24xx_nand_probe(struct platform_device
> > *pdev)
> >
> > cpu_type = platform_get_device_id(pdev)->driver_data;
> >
> > - pr_debug("s3c2410_nand_probe(%p)\n", pdev);
> > + pr_debug("%s(%p)\n", __func__, pdev);
>
> I think we can drop the line completely.
> We have ftrace to trace function calls...
Should the "initialised ok" at the end of the function be remove as well?
Will it be confusing if this cleanup is done in this function, but not in
others where it may be useful? Perhaps s3c2410_nand_update_chip, for
example?
julia
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/20] fix misspelling of current function in string
On Mon, 8 Dec 2014, Julian Calaby wrote: > Hi Julia, > > On Mon, Dec 8, 2014 at 6:20 AM, Julia Lawall wrote: > > These patches replace what appears to be a reference to the name of the > > current function but is misspelled in some way by either the name of the > > function itself, or by %s and then __func__ in an argument list. > > Would there be any value in doing this for _all_ cases where the > function name is written in a format string? Probably. But there are a lot of them. Even for the misspellings, I have only don about 1/3 of the cases. On the other hand, the misspelling have to be checked carefully, because a misspelling of one thing could be the correct spelling of the thing thst was actually intended. Joe, however, points out that a lot of these prints are just for function tracing, and could be removed. I worked on another semantic patch that tries to do that. It might be better to remove those prints completely, rather than sending one patch to transform them and then one patch to remove them after that. That is why for this series I did only the ones where there was actually a problem. julia -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/20] fix misspelling of current function in string
These patches replace what appears to be a reference to the name of the
current function but is misspelled in some way by either the name of the
function itself, or by %s and then __func__ in an argument list.
//
// sudo apt-get install python-pip
// sudo pip install python-Levenshtein
// spatch requires the argument --in-place
virtual after_start
@initialize:ocaml@
@@
let extensible_functions = ref ([] : string list)
let restarted = ref false
let restart _ =
restarted := true;
let it = new iteration() in
it#add_virtual_rule After_start;
Printf.eprintf "restarting\n";
it#register()
@initialize:python@
@@
import re
from Levenshtein import distance
mindist = 1 // 1 to find only misspellings
maxdist = 2
ignore_leading = True
// -
@r0@
constant char [] c;
identifier f;
@@
f(...,c,...)
@script:ocaml@
c << r0.c;
f << r0.f;
@@
(if not !restarted then restart());
match Str.split_delim (Str.regexp "%") c with
_::_::_ ->
if not (List.mem f !extensible_functions)
then extensible_functions := f :: !extensible_functions
| _ -> ()
// -
@r depends on after_start@
constant char [] c;
position p;
identifier f;
@@
f(...,c@p,...)
@script:python flt@
c << r.c;
p << r.p;
matched;
@@
func = p[0].current_element
wpattern = "[a-zA-Z_][a-zA-Z0-9_]*"
if ignore_leading:
func = func.strip("_")
wpattern = "[a-zA-Z][a-zA-Z0-9_]*"
lf = len(func)
cocci.include_match(False)
// ignore extremely short function names
if lf > 3:
words = [w for w in re.findall(wpattern, c) if abs(len(w) - lf) <= maxdist]
for w in words:
d = distance(w, func)
if mindist <= d and d <= maxdist:
coccinelle.matched = w
cocci.include_match(True)
//print "%s:%d:%s():%d: %s" % (p[0].file, int(p[0].line), func, d, c)
break
@script:ocaml r2@
c << r.c;
f << r.f;
matched << flt.matched;
fixed;
@@
let pieces = Str.split_delim (Str.regexp_string matched) c in
match pieces with
[before;after] ->
let preceeding =
List.length(Str.split (Str.regexp_string "%") before) > 1 in
if preceeding
then Coccilib.include_match false
else
if List.mem f !extensible_functions
then fixed := before ^ "%s" ^ after
else Coccilib.include_match false
| _ -> Coccilib.include_match false
@changed1@
constant char [] r.c;
identifier r2.fixed;
position r.p;
identifier r.f;
@@
f(...,
-c@p
+fixed,__func__
,...)
// ---
@s depends on after_start@
constant char [] c;
position p;
identifier f;
@@
f(...,c@p,...)
@script:python flt2@
c << s.c;
p << s.p;
matched;
@@
func = p[0].current_element
wpattern = "[a-zA-Z_][a-zA-Z0-9_]*"
if ignore_leading:
func = func.strip("_")
wpattern = "[a-zA-Z][a-zA-Z0-9_]*"
lf = len(func)
cocci.include_match(False)
// ignore extremely short function names
if lf > 3:
words = [w for w in re.findall(wpattern, c) if abs(len(w) - lf) <= maxdist]
for w in words:
d = distance(w, func)
if mindist <= d and d <= maxdist:
coccinelle.matched = w
cocci.include_match(True)
//print "%s:%d:%s():%d: %s" % (p[0].file, int(p[0].line), func, d, c)
break
@script:ocaml s2@
c << s.c;
f << s.f;
p << s.p;
matched << flt2.matched;
fixed;
@@
let ce = (List.hd p).current_element in
let pieces = Str.split_delim (Str.regexp_string matched) c in
match pieces with
[before;after] ->
let preceeding =
List.length(Str.split (Str.regexp_string "%") before) > 1 in
if preceeding
then Coccilib.include_match false
else
if List.mem f !extensible_functions
then Coccilib.include_match false
else fixed := before ^ ce ^ after
| _ -> Coccilib.include_match false
@changed2@
constant char [] s.c;
identifier s2.fixed;
position s.p;
identifier s.f;
@@
f(...,
-c@p
+fixed
,...)
//
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/20] mtd: s3c2410: fix misspelling of current function in string
Replace a misspelled function name by %s and then __func__.
s3c2410 is the name of the file, but using the exact function name should
make it easier to connect the error message to the code.
This was done using Coccinelle, including the use of Levenshtein distance,
as proposed by Rasmus Villemoes.
Signed-off-by: Julia Lawall
---
The semantic patch is difficult to summarize, but is available in the cover
letter of this patch series.
drivers/mtd/nand/s3c2410.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/s3c2410.c b/drivers/mtd/nand/s3c2410.c
index 35aef5e..0a9c41f 100644
--- a/drivers/mtd/nand/s3c2410.c
+++ b/drivers/mtd/nand/s3c2410.c
@@ -948,7 +948,7 @@ static int s3c24xx_nand_probe(struct platform_device *pdev)
cpu_type = platform_get_device_id(pdev)->driver_data;
- pr_debug("s3c2410_nand_probe(%p)\n", pdev);
+ pr_debug("%s(%p)\n", __func__, pdev);
info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
if (info == NULL) {
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/5] drm/exynos/ipp: fix error return code
From: Julia Lawall
Propagate the returned error code on failure.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
//
@@
identifier ret; expression e1,e2;
@@
(
if (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
//
Signed-off-by: Julia Lawall
---
drivers/gpu/drm/exynos/exynos_drm_ipp.c |3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
index 00d74b1..d5ad17d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
@@ -426,18 +426,21 @@ int exynos_drm_ipp_set_property(struct drm_device
*drm_dev, void *data,
c_node->start_work = ipp_create_cmd_work();
if (IS_ERR(c_node->start_work)) {
DRM_ERROR("failed to create start work.\n");
+ ret = PTR_ERR(c_node->start_work);
goto err_remove_id;
}
c_node->stop_work = ipp_create_cmd_work();
if (IS_ERR(c_node->stop_work)) {
DRM_ERROR("failed to create stop work.\n");
+ ret = PTR_ERR(c_node->stop_work);
goto err_free_start;
}
c_node->event_work = ipp_create_event_work();
if (IS_ERR(c_node->event_work)) {
DRM_ERROR("failed to create event work.\n");
+ ret = PTR_ERR(c_node->event_work);
goto err_free_stop;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/5] fix error return code
The complate semantic patch that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
//
@ok exists@
identifier f,ret,i;
expression e;
constant c;
@@
// identify a function that returns a negative return value at least once.
f(...) {
... when any
(
return -c@i;
|
ret = -c@i;
... when != ret = e
return ret;
|
if (ret < 0) { ... return ret; }
)
... when any
}
@r exists@
identifier ret,ok.f,fn;
expression e1,e2,e3,e4,e5,e6,x;
statement S,S1;
position p1,p2,p3;
@@
// identify a case where the return variable is set to a non-negative value
// and then returned in error-handling code
f(...) {
... when any
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != \(ret = e1\|ret++\|ret--\|ret+=e1\|ret-=e1\)
when != &ret
when any
(
if (<+... ret = e5 ...+>) S1
|
if (<+... &ret ...+>) S1
|
if@p2(<+...x = fn(...)...+>)
{
... when != ret = e6
when forall
return@p3 ret;
}
|
break;
|
x = fn(...)
... when != \(ret = e4\|ret++\|ret--\|ret+=e4\|ret-=e4\)
when != &ret
(
if (<+... ret = e3 ...+>) S
|
if (<+... &ret ...+>) S
|
if@p2(<+...\(x != 0\|x < 0\|x == NULL\|IS_ERR(x)\)...+>)
{
... when != ret = e2
when forall
return@p3 ret;
}
)
)
... when any
}
@printer depends on r@
position p;
identifier ok.f,pr;
constant char [] c;
@@
f(...) { <...pr@p(...,c,...)...> }
@bad0 exists@
identifier r.ret,ok.f,g != {ERR_PTR,IS_ERR};
position p != printer.p;
@@
f(...) { ... when any
g@p(...,ret,...)
... when any
}
@bad depends on !bad0 exists@
position r.p1,r.p2;
statement S1,S2;
identifier r.ret;
expression e1;
@@
// ignore the above if there is some path where the variable is set to
// something else
(
if@p1 (\(ret < 0\|ret != 0\)) S1
|
ret@p1 = 0
)
... when any
\(ret = e1\|ret++\|ret--\|ret+=e1\|ret-=e1\|&ret\)
... when any
if@p2(...) S2
@bad1 depends on !bad0 && !bad exists@
position r.p2;
statement S2;
identifier r.ret;
expression e1;
constant c;
@@
ret = -c
... when != \(ret = e1\|ret++\|ret--\|ret+=e1\|ret-=e1\)
when != &ret
when any
if@p2(...) S2
@bad2 depends on !bad0 && !bad && !bad1 exists@
position r.p1,r.p2;
identifier r.ret;
expression e1;
statement S2;
constant c;
@@
// likewise ignore it if there has been an intervening return
ret@p1 = 0
... when != if (...) { ... ret = e1 ... return ret; }
when != if (...) { ... return -c; }
when any
if@p2(...) S2
@script:python depends on !bad0 && !bad && !bad1 && !bad2@
p1 << r.p1;
p2 << r.p2;
p3 << r.p3;
@@
cocci.print_main("",p1)
cocci.print_secs("",p2)
cocci.print_secs("",p3)
//
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] thermal: Exynos: Deletion of unnecessary checks before two function calls
On Fri, 21 Nov 2014, SF Markus Elfring wrote:
> From: Markus Elfring
> Date: Fri, 21 Nov 2014 11:06:33 +0100
>
> The functions cpufreq_cooling_unregister() and
> thermal_zone_device_unregister()
> test whether their argument is NULL and then return immediately.
> Thus the test around the call is not needed.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring
> ---
> drivers/thermal/samsung/exynos_thermal_common.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/thermal/samsung/exynos_thermal_common.c
> b/drivers/thermal/samsung/exynos_thermal_common.c
> index 3f5ad25..1cbb9d0 100644
> --- a/drivers/thermal/samsung/exynos_thermal_common.c
> +++ b/drivers/thermal/samsung/exynos_thermal_common.c
> @@ -417,12 +417,10 @@ void exynos_unregister_thermal(struct
> thermal_sensor_conf *sensor_conf)
>
> th_zone = sensor_conf->pzone_data;
>
> - if (th_zone->therm_dev)
> - thermal_zone_device_unregister(th_zone->therm_dev);
> + thermal_zone_device_unregister(th_zone->therm_dev);
>
> for (i = 0; i < th_zone->cool_dev_size; i++) {
> - if (th_zone->cool_dev[i])
> - cpufreq_cooling_unregister(th_zone->cool_dev[i]);
> + cpufreq_cooling_unregister(th_zone->cool_dev[i]);
> }
Now you have unnecessary {}
julia
>
> dev_info(sensor_conf->dev,
> --
> 2.1.3
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/25] fix error return code
These patches fix cases where the return variable is not set to an error code in an error case. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 23/25] thermal: exynos: fix error return code
From: Julia Lawall
Set the return variable to an error code as done elsewhere in the function.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
//
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
//
Signed-off-by: Julia Lawall
---
Not tested.
drivers/thermal/samsung/exynos_tmu.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/thermal/samsung/exynos_tmu.c
b/drivers/thermal/samsung/exynos_tmu.c
index 32f38b9..0d96a51 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -205,6 +205,7 @@ static int exynos_tmu_initialize(struct platform_device
*pdev)
skip_calib_data:
if (pdata->max_trigger_level > MAX_THRESHOLD_LEVS) {
dev_err(&pdev->dev, "Invalid max trigger level\n");
+ ret = -EINVAL;
goto out;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/29] simplify use of devm_ioremap_resource
devm_ioremap_resource often uses the result of a call to
platform_get_resource as its last argument. devm_ioremap_resource does
appropriate error handling on this argument, so error handling can be
removed from the call site. To make the connection between the call to
platform_get_resource and the call to devm_ioremap_resource more clear, the
former is also moved down to be adjacent to the latter.
In many cases, this patch changes the specific error value that is
returned on failure of platform_get_resource.
The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)
//
@@
expression pdev,res,n,e,e1;
expression ret != 0;
identifier l;
@@
(
res = platform_get_resource(pdev, IORESOURCE_MEM, n);
- if (res == NULL) { ... \(goto l;\|return ret;\) }
e = devm_ioremap_resource(e1, res);
|
- res = platform_get_resource(pdev, IORESOURCE_MEM, n);
... when != res
- if (res == NULL) { ... \(goto l;\|return ret;\) }
... when != res
+ res = platform_get_resource(pdev, IORESOURCE_MEM, n);
e = devm_ioremap_resource(e1, res);
)
//
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 12/29] sound/soc/samsung/ac97.c: simplify use of devm_ioremap_resource
From: Julia Lawall
Remove unneeded error handling on the result of a call to
platform_get_resource when the value is passed to devm_ioremap_resource.
Move the call to platform_get_resource adjacent to the call to
devm_ioremap_resource to make the connection between them more clear.
A simplified version of the semantic patch that makes this change is as
follows: (http://coccinelle.lip6.fr/)
//
@@
expression pdev,res,n,e,e1;
expression ret != 0;
identifier l;
@@
- res = platform_get_resource(pdev, IORESOURCE_MEM, n);
... when != res
- if (res == NULL) { ... \(goto l;\|return ret;\) }
... when != res
+ res = platform_get_resource(pdev, IORESOURCE_MEM, n);
e = devm_ioremap_resource(e1, res);
//
Signed-off-by: Julia Lawall
---
sound/soc/samsung/ac97.c |7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/sound/soc/samsung/ac97.c b/sound/soc/samsung/ac97.c
index 2dd623f..c732df9 100644
--- a/sound/soc/samsung/ac97.c
+++ b/sound/soc/samsung/ac97.c
@@ -404,18 +404,13 @@ static int s3c_ac97_probe(struct platform_device *pdev)
return -ENXIO;
}
- mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!mem_res) {
- dev_err(&pdev->dev, "Unable to get register resource\n");
- return -ENXIO;
- }
-
irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
if (!irq_res) {
dev_err(&pdev->dev, "AC97 IRQ not provided!\n");
return -ENXIO;
}
+ mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
s3c_ac97.regs = devm_ioremap_resource(&pdev->dev, mem_res);
if (IS_ERR(s3c_ac97.regs))
return PTR_ERR(s3c_ac97.regs);
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 20/29] watchdog: s3c2410_wdt: simplify use of devm_ioremap_resource
From: Julia Lawall
Remove unneeded error handling on the result of a call to
platform_get_resource when the value is passed to devm_ioremap_resource.
Move the call to platform_get_resource adjacent to the call to
devm_ioremap_resource to make the connection between them more clear.
A simplified version of the semantic patch that makes this change is as
follows: (http://coccinelle.lip6.fr/)
//
@@
expression pdev,res,n,e,e1;
expression ret != 0;
identifier l;
@@
- res = platform_get_resource(pdev, IORESOURCE_MEM, n);
... when != res
- if (res == NULL) { ... \(goto l;\|return ret;\) }
... when != res
+ res = platform_get_resource(pdev, IORESOURCE_MEM, n);
e = devm_ioremap_resource(e1, res);
//
Signed-off-by: Julia Lawall
---
drivers/watchdog/s3c2410_wdt.c |7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 6a22cf5..9fbc993 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -325,12 +325,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
dev = &pdev->dev;
wdt_dev = &pdev->dev;
- wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (wdt_mem == NULL) {
- dev_err(dev, "no memory resource specified\n");
- return -ENOENT;
- }
-
wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
if (wdt_irq == NULL) {
dev_err(dev, "no irq resource specified\n");
@@ -339,6 +333,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
}
/* get the memory region for the watchdog timer */
+ wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
wdt_base = devm_ioremap_resource(dev, wdt_mem);
if (IS_ERR(wdt_base)) {
ret = PTR_ERR(wdt_base);
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: question about arch/arm/mach-s3c24xx/irq.c
On Sun, 24 Feb 2013, Heiko Stübner wrote:
> Am Sonntag, 24. Februar 2013, 14:39:45 schrieb Julia Lawall:
> > [Adding the person who introduced the code]
> >
> > On Sun, 24 Feb 2013, Russell King - ARM Linux wrote:
> > > On Sun, Feb 24, 2013 at 12:45:11PM +0100, Julia Lawall wrote:
> > > > The function s3c24xx_irq_map in arch/arm/mach-s3c24xx/irq.c contains
> > > > the
> > > >
> > > > code:
> > > > parent_irq_data =
> > > > &parent_intc->irqs[irq_data->parent_irq];
> > > >
> > > > if (!irq_data) {
> > > >
> > > > pr_err("irq-s3c24xx: no irq data found for
> > > > hwirq %lu\n",
> > > >
> > > >hw);
> > > >
> > > > goto err;
> > > >
> > > > }
> > > >
> > > > At this point irq_data has already been tested, so the null test on
> > > > irq_data does not look correct. But I wonder if parent_irq_data could
> > > > ever be null here?
> > >
> > > That would be really obscure - because that would require parent_intc to
> > > be a "negative" pointer (to counter-act the indexing by
> > > irq_data->parent_irq). So it looks to me like the above is redundant.
> >
> > Even at its original definition irq_data seems unlikely to be NULL:
> >
> > struct s3c_irq_intc *intc = h->host_data;
> > struct s3c_irq_data *irq_data = &intc->irqs[hw];
> > ...
> > if (!irq_data) {
> > pr_err("irq-s3c24xx: no irq data found for hwirq %lu\n",
> > hw); return -EINVAL;
> > }
> >
> > That is, it could be an invalid value, but whether it actually hits 0
> > would seem to depend on the value hw?
> >
> > Heiko, is NULL really a possibility?
>
> The test you quoted is of course wrong ... it would need to test
> parent_irq_data. But you're also right that the test is not necessary at all.
>
> All the s3c_irq_data arrays used always contain 32 entries to reach all bits
> of the register (which is used differently on each SoC). So if we have found
> the parent_intc at all, it should contain a 32 entries array of irq_data
> structs, so no need to test for the existence of the individual array element.
>
>
> And now that I look at it, I also see another glitch. The code tests for
> parent_irq != 0, which of course won't work if the parent_irq is the 0-hwirq
> of the parent controller.
> The only SoC using such a mapping is the s3c2412 [0], which explains why I
> haven't been bitten by this myself.
Do you want to make all the fixes?
julia
Re: question about arch/arm/mach-s3c24xx/irq.c
[Adding the person who introduced the code]
On Sun, 24 Feb 2013, Russell King - ARM Linux wrote:
> On Sun, Feb 24, 2013 at 12:45:11PM +0100, Julia Lawall wrote:
> > The function s3c24xx_irq_map in arch/arm/mach-s3c24xx/irq.c contains the
> > code:
> >
> > parent_irq_data = &parent_intc->irqs[irq_data->parent_irq];
> > if (!irq_data) {
> > pr_err("irq-s3c24xx: no irq data found for hwirq
> > %lu\n",
> >hw);
> > goto err;
> > }
> >
> > At this point irq_data has already been tested, so the null test on
> > irq_data does not look correct. But I wonder if parent_irq_data could
> > ever be null here?
>
> That would be really obscure - because that would require parent_intc to
> be a "negative" pointer (to counter-act the indexing by
> irq_data->parent_irq). So it looks to me like the above is redundant.
Even at its original definition irq_data seems unlikely to be NULL:
struct s3c_irq_intc *intc = h->host_data;
struct s3c_irq_data *irq_data = &intc->irqs[hw];
...
if (!irq_data) {
pr_err("irq-s3c24xx: no irq data found for hwirq %lu\n", hw);
return -EINVAL;
}
That is, it could be an invalid value, but whether it actually hits 0
would seem to depend on the value hw?
Heiko, is NULL really a possibility?
thanks,
julia
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] arch/arm/mach-s3c24xx/mach-h1940.c: delete double assignment
From: Julia Lawall Delete successive assignments to the same location. A simplified version of the semantic match that finds this problem is as follows: (http://coccinelle.lip6.fr/) // @@ expression i; @@ *i = ...; i = ...; // Signed-off-by: Julia Lawall --- Not compiled, and this may change the behavior of the code. Without this change, check_gpio2 could possibly be used uninitialized later. arch/arm/mach-s3c24xx/mach-h1940.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/mach-s3c24xx/mach-h1940.c b/arch/arm/mach-s3c24xx/mach-h1940.c index bb8d008..48c 100644 --- a/arch/arm/mach-s3c24xx/mach-h1940.c +++ b/arch/arm/mach-s3c24xx/mach-h1940.c @@ -380,7 +380,7 @@ int h1940_led_blink_set(unsigned gpio, int state, default: blink_gpio = S3C2410_GPA(3); check_gpio1 = S3C2410_GPA(1); - check_gpio1 = S3C2410_GPA(7); + check_gpio2 = S3C2410_GPA(7); break; } -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/14] drivers/spi/spi-s3c24xx.c: fix error return code
From: Julia Lawall
Initialize return variable before exiting on an error path.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
//
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
//
Signed-off-by: Julia Lawall
---
Perhaps -EINVAL is not the right value in this case.
drivers/spi/spi-s3c24xx.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/spi/spi-s3c24xx.c b/drivers/spi/spi-s3c24xx.c
index 8ee7d79..a2a080b 100644
--- a/drivers/spi/spi-s3c24xx.c
+++ b/drivers/spi/spi-s3c24xx.c
@@ -611,6 +611,7 @@ static int __devinit s3c24xx_spi_probe(struct
platform_device *pdev)
if (!pdata->set_cs) {
if (pdata->pin_cs < 0) {
dev_err(&pdev->dev, "No chipselect pin\n");
+ err = -EINVAL;
goto err_register;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 14/14] drivers/spi/spi-s3c24xx.c: fix error return code
From: Julia Lawall
Initialize return variable before exiting on an error path.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
//
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
//
Signed-off-by: Julia Lawall
---
Perhaps -EINVAL is not the right value in this case.
drivers/spi/spi-s3c24xx.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/spi/spi-s3c24xx.c b/drivers/spi/spi-s3c24xx.c
index 8ee7d79..a2a080b 100644
--- a/drivers/spi/spi-s3c24xx.c
+++ b/drivers/spi/spi-s3c24xx.c
@@ -611,6 +611,7 @@ static int __devinit s3c24xx_spi_probe(struct
platform_device *pdev)
if (!pdata->set_cs) {
if (pdata->pin_cs < 0) {
dev_err(&pdev->dev, "No chipselect pin\n");
+ err = -EINVAL;
goto err_register;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/10] arch/arm/mach-s5pv210/cpufreq.c: add missing clk_put
From: Julia Lawall
The successive calls to clk_get each call clk_put in the case of failure,
but this is not done for subsequent error handling code. The calls to
clk_get are moved to the end of the function, and appropriate gotos are
added.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
//
@r exists@
expression e1,e2;
statement S;
@@
e1 = clk_get@p1(...);
... when != e1 = e2
when != clk_put(e1)
when any
if (...) { ... when != clk_put(e1)
when != if (...) { ... clk_put(e1) ... }
* return@p3 ...;
} else S
//
Signed-off-by: Julia Lawall
---
arch/arm/mach-s5pv210/cpufreq.c | 25 +
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/arch/arm/mach-s5pv210/cpufreq.c b/arch/arm/mach-s5pv210/cpufreq.c
index 22046e2..97a29e1 100644
--- a/arch/arm/mach-s5pv210/cpufreq.c
+++ b/arch/arm/mach-s5pv210/cpufreq.c
@@ -414,6 +414,7 @@ static int check_mem_type(void __iomem *dmc_reg)
static int __init s5pv210_cpu_init(struct cpufreq_policy *policy)
{
unsigned long mem_type;
+ int ret;
cpu_clk = clk_get(NULL, "armclk");
if (IS_ERR(cpu_clk))
@@ -421,19 +422,20 @@ static int __init s5pv210_cpu_init(struct cpufreq_policy
*policy)
dmc0_clk = clk_get(NULL, "sclk_dmc0");
if (IS_ERR(dmc0_clk)) {
- clk_put(cpu_clk);
- return PTR_ERR(dmc0_clk);
+ ret = PTR_ERR(dmc0_clk);
+ goto out_dmc0;
}
dmc1_clk = clk_get(NULL, "hclk_msys");
if (IS_ERR(dmc1_clk)) {
- clk_put(dmc0_clk);
- clk_put(cpu_clk);
- return PTR_ERR(dmc1_clk);
+ ret = PTR_ERR(dmc1_clk);
+ goto out_dmc1;
}
- if (policy->cpu != 0)
- return -EINVAL;
+ if (policy->cpu != 0) {
+ ret = -EINVAL;
+ goto out_dmc1;
+ }
/*
* check_mem_type : This driver only support LPDDR & LPDDR2.
@@ -443,7 +445,8 @@ static int __init s5pv210_cpu_init(struct cpufreq_policy
*policy)
if ((mem_type != LPDDR) && (mem_type != LPDDR2)) {
printk(KERN_ERR "CPUFreq doesn't support this memory type\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_dmc1;
}
/* Find current refresh counter and frequency each DMC */
@@ -460,6 +463,12 @@ static int __init s5pv210_cpu_init(struct cpufreq_policy
*policy)
policy->cpuinfo.transition_latency = 4;
return cpufreq_frequency_table_cpuinfo(policy, s5pv210_freq_table);
+
+out_dmc1:
+ clk_put(dmc0_clk);
+out_dmc0:
+ clk_put(cpu_clk);
+ return ret;
}
static struct cpufreq_driver s5pv210_driver = {
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
