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

[Jie Luo <quic_luoj@quicinc.com>]

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));