1. Patchew
  2. linux
  3. [PATCH v3 0/6] Add FIELD_MODIFY() helper
  4. View patch

[PATCH v3 2/6] coccinelle: misc: Add field_modify script

Luo Jie posted 6 patches 8 months ago
There is a newer version of this series
[PATCH v3 2/6] coccinelle: misc: Add field_modify script
Posted by Luo Jie 8 months ago 
This script finds and suggests conversions of opencoded field
modify patterns with the wrapper FIELD_MODIFY() API defined in
include/linux/bitfield.h for better readability.

Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
---
 scripts/coccinelle/misc/field_modify.cocci | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/scripts/coccinelle/misc/field_modify.cocci b/scripts/coccinelle/misc/field_modify.cocci
new file mode 100644
index 000000000000..9022c1b23291
--- /dev/null
+++ b/scripts/coccinelle/misc/field_modify.cocci
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
+/// - reg &= ~MASK;
+/// - reg |= FIELD_PREP(MASK, val);
+//
+// Confidence: High
+// Author: Luo Jie <quic_luoj@quicinc.com>
+// Copyright (C) 2025 Qualcomm Innovation Center, Inc.
+// URL: https://coccinelle.gitlabpages.inria.fr/website
+// Keywords: FIELD_PREP, FIELD_MODIFY
+// Options: --include-headers
+
+virtual patch
+
+@depends on patch@
+identifier reg, val;
+constant mask;
+symbol FIELD_PREP, FIELD_MODIFY;
+@@
+
+- reg &= ~mask;
+- reg |= FIELD_PREP(mask, val);
++ FIELD_MODIFY(mask, &reg, val);

-- 
2.34.1
Re: [cocci] [PATCH v3 2/6] coccinelle: misc: Add field_modify script
Posted by Markus Elfring 7 months, 3 weeks ago 
> This script finds and suggests conversions of opencoded field
> modify patterns with the wrapper FIELD_MODIFY() API defined in
> include/linux/bitfield.h for better readability.

See also:
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.15-rc3#n94


…
> +++ b/scripts/coccinelle/misc/field_modify.cocci
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +///

I suggest to omit a blank comment line here.


> +/// Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)

Replace?


…
> +// Copyright (C) 2025 Qualcomm Innovation Center, Inc.

Copyright: ?


> +// URL: https://coccinelle.gitlabpages.inria.fr/website

I suggest to omit such information here.


…
> +virtual patch

How do you think about to support additional operation modes?


…
> +- reg &= ~mask;
> +- reg |= FIELD_PREP(mask, val);
> ++ FIELD_MODIFY(mask, &reg, val);

Would you like to integrate the following SmPL code variant?

-reg &= ~mask;
-reg |= FIELD_PREP
+       FIELD_MODIFY
                  (mask,
+                  &reg,
                   val
                  );


Regards,
Markus
Re: [cocci] [PATCH v3 2/6] coccinelle: misc: Add field_modify script
Posted by Jie Luo 7 months, 3 weeks ago 

On 4/23/2025 7:01 PM, Markus Elfring wrote:
>> This script finds and suggests conversions of opencoded field
>> modify patterns with the wrapper FIELD_MODIFY() API defined in
>> include/linux/bitfield.h for better readability.
> 
> See also:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.15-rc3#n94

OK, I will update the commit message with the imperative mood.

> 
> 
> …
>> +++ b/scripts/coccinelle/misc/field_modify.cocci
>> @@ -0,0 +1,24 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +///
> 
> I suggest to omit a blank comment line here.

OK.

> 
> 
>> +/// Replaced below code with the wrapper FIELD_MODIFY(MASK, &reg, val)
> 
> Replace?

I will correct it.

> 
> 
> …
>> +// Copyright (C) 2025 Qualcomm Innovation Center, Inc.
> 
> Copyright: ?

I will fix it.

> 
> 
>> +// URL: https://coccinelle.gitlabpages.inria.fr/website
> 
> I suggest to omit such information here.

OK.

> 
> 
> …
>> +virtual patch
> 
> How do you think about to support additional operation modes?

Sure, I will update the patch to support other operation modes.

> 
> 
> …
>> +- reg &= ~mask;
>> +- reg |= FIELD_PREP(mask, val);
>> ++ FIELD_MODIFY(mask, &reg, val);
> 
> Would you like to integrate the following SmPL code variant?
> 
> -reg &= ~mask;
> -reg |= FIELD_PREP
> +       FIELD_MODIFY
>                    (mask,
> +                  &reg,
>                     val
>                    );
> 
> 
> Regards,
> Markus
> 

With this code variant, there is no space prior to &reg, here is the
example code changes generated by the SmPL code as below, is this
expected?

Thanks for the review comments.

--- a/drivers/phy/starfive/phy-jh7110-dphy-tx.c
+++ b/drivers/phy/starfive/phy-jh7110-dphy-tx.c
@@ -244,8 +244,7 @@ static int stf_dphy_configure(struct phy
         i = stf_dphy_get_config_index(bitrate);

         tmp = readl(dphy->topsys + STF_DPHY_APBIFSAIF_SYSCFG(100));
-       tmp &= ~STF_DPHY_REFCLK_IN_SEL;
-       tmp |= FIELD_PREP(STF_DPHY_REFCLK_IN_SEL, STF_DPHY_REFCLK_12M);
+       FIELD_MODIFY(STF_DPHY_REFCLK_IN_SEL,&tmp, STF_DPHY_REFCLK_12M);
         writel(tmp, dphy->topsys + STF_DPHY_APBIFSAIF_SYSCFG(100));
Re: [cocci] [PATCH v3 2/6] coccinelle: misc: Add field_modify script
Posted by Markus Elfring 7 months, 3 weeks ago 
…
>> -reg &= ~mask;
>> -reg |= FIELD_PREP
>> +       FIELD_MODIFY
>>                    (mask,
>> +                  &reg,
>>                     val
>>                    );
…
> With this code variant, there is no space prior to &reg, here is the
> example code changes generated by the SmPL code as below, is this expected?
…
> +++ b/drivers/phy/starfive/phy-jh7110-dphy-tx.c
> @@ -244,8 +244,7 @@ static int stf_dphy_configure(struct phy
>         i = stf_dphy_get_config_index(bitrate);
> 
>         tmp = readl(dphy->topsys + STF_DPHY_APBIFSAIF_SYSCFG(100));
> -       tmp &= ~STF_DPHY_REFCLK_IN_SEL;
> -       tmp |= FIELD_PREP(STF_DPHY_REFCLK_IN_SEL, STF_DPHY_REFCLK_12M);
> +       FIELD_MODIFY(STF_DPHY_REFCLK_IN_SEL,&tmp, STF_DPHY_REFCLK_12M);
>         writel(tmp, dphy->topsys + STF_DPHY_APBIFSAIF_SYSCFG(100));

The Coccinelle software is still evolving somehow.
Thus your test result can trigger further development considerations.
I hope that clarifications and corresponding improvements can be achieved
also according to such source code layout concerns.

Regards,
Markus
Re: [cocci] [PATCH v3 2/6] coccinelle: misc: Add field_modify script
Posted by Luo Jie 6 months, 4 weeks ago 

On 4/24/2025 12:35 AM, Markus Elfring wrote:
> …
>>> -reg &= ~mask;
>>> -reg |= FIELD_PREP
>>> +       FIELD_MODIFY
>>>                     (mask,
>>> +                  &reg,
>>>                      val
>>>                     );
> …
>> With this code variant, there is no space prior to &reg, here is the
>> example code changes generated by the SmPL code as below, is this expected?
> …
>> +++ b/drivers/phy/starfive/phy-jh7110-dphy-tx.c
>> @@ -244,8 +244,7 @@ static int stf_dphy_configure(struct phy
>>          i = stf_dphy_get_config_index(bitrate);
>>
>>          tmp = readl(dphy->topsys + STF_DPHY_APBIFSAIF_SYSCFG(100));
>> -       tmp &= ~STF_DPHY_REFCLK_IN_SEL;
>> -       tmp |= FIELD_PREP(STF_DPHY_REFCLK_IN_SEL, STF_DPHY_REFCLK_12M);
>> +       FIELD_MODIFY(STF_DPHY_REFCLK_IN_SEL,&tmp, STF_DPHY_REFCLK_12M);
>>          writel(tmp, dphy->topsys + STF_DPHY_APBIFSAIF_SYSCFG(100));
> 
> The Coccinelle software is still evolving somehow.
> Thus your test result can trigger further development considerations.
> I hope that clarifications and corresponding improvements can be achieved
> also according to such source code layout concerns.
> 
> Regards,
> Markus

OK, understand. I will keep the original patch as below for now, as we
need to ensure the script generates code with the expected style.
+- reg &= ~mask;
+- reg |= FIELD_PREP(mask, val);
++ FIELD_MODIFY(mask, &reg, val);

In addition, below case is filed to request this improvement on the
coccinelle project.
https://github.com/coccinelle/coccinelle/issues/397

Thanks.
Re: [cocci] [PATCH v3 2/6] coccinelle: misc: Add field_modify script
Posted by Julia Lawall 6 months, 4 weeks ago 

On Mon, 19 May 2025, Luo Jie wrote:

>
>
> On 4/24/2025 12:35 AM, Markus Elfring wrote:
> > …
> > > > -reg &= ~mask;
> > > > -reg |= FIELD_PREP
> > > > +       FIELD_MODIFY
> > > >                     (mask,
> > > > +                  &reg,
> > > >                      val
> > > >                     );
> > …
> > > With this code variant, there is no space prior to &reg, here is the
> > > example code changes generated by the SmPL code as below, is this
> > > expected?
> > …
> > > +++ b/drivers/phy/starfive/phy-jh7110-dphy-tx.c
> > > @@ -244,8 +244,7 @@ static int stf_dphy_configure(struct phy
> > >          i = stf_dphy_get_config_index(bitrate);
> > >
> > >          tmp = readl(dphy->topsys + STF_DPHY_APBIFSAIF_SYSCFG(100));
> > > -       tmp &= ~STF_DPHY_REFCLK_IN_SEL;
> > > -       tmp |= FIELD_PREP(STF_DPHY_REFCLK_IN_SEL, STF_DPHY_REFCLK_12M);
> > > +       FIELD_MODIFY(STF_DPHY_REFCLK_IN_SEL,&tmp, STF_DPHY_REFCLK_12M);
> > >          writel(tmp, dphy->topsys + STF_DPHY_APBIFSAIF_SYSCFG(100));
> >
> > The Coccinelle software is still evolving somehow.
> > Thus your test result can trigger further development considerations.
> > I hope that clarifications and corresponding improvements can be achieved
> > also according to such source code layout concerns.
> >
> > Regards,
> > Markus
>
> OK, understand. I will keep the original patch as below for now, as we
> need to ensure the script generates code with the expected style.
> +- reg &= ~mask;
> +- reg |= FIELD_PREP(mask, val);
> ++ FIELD_MODIFY(mask, &reg, val);
>
> In addition, below case is filed to request this improvement on the
> coccinelle project.
> https://github.com/coccinelle/coccinelle/issues/397

Thanks for the report.

julia


>
> Thanks.
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.