Hi Simon,
On 10/09/2024 10:17, Simon Horman wrote:
> W=1 builds flag that some accessor functions for ALE fields are unused.
>
> Address this by splitting up the macros used to define these
> accessors to allow only those that are used to be declared.
>
> The warnings are verbose, but for example, the mcast_state case is
> flagged by clang-18 as:
>
> .../cpsw_ale.c:220:1: warning: unused function 'cpsw_ale_get_mcast_state' [-Wunused-function]
> 220 | DEFINE_ALE_FIELD(mcast_state, 62, 2)
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> .../cpsw_ale.c:145:19: note: expanded from macro 'DEFINE_ALE_FIELD'
> 145 | static inline int cpsw_ale_get_##name(u32 *ale_entry) \
> | ^~~~~~~~~~~~~~~~~~~
> <scratch space>:196:1: note: expanded from here
> 196 | cpsw_ale_get_mcast_state
> | ^~~~~~~~~~~~~~~~~~~~~~~~
>
> Compile tested only.
> No functional change intended.
>
> Signed-off-by: Simon Horman <horms@xxxxxxxxxx>
> ---
> drivers/net/ethernet/ti/cpsw_ale.c | 30 +++++++++++++++++++++---------
> 1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
> index 64bf22cd860c..d37b4ddd6787 100644
> --- a/drivers/net/ethernet/ti/cpsw_ale.c
> +++ b/drivers/net/ethernet/ti/cpsw_ale.c
> @@ -141,27 +141,39 @@ static inline void cpsw_ale_set_field(u32 *ale_entry, u32 start, u32 bits,
> ale_entry[idx] |= (value << start);
> }
>
> -#define DEFINE_ALE_FIELD(name, start, bits) \
> +#define DEFINE_ALE_FIELD_GET(name, start, bits) \
> static inline int cpsw_ale_get_##name(u32 *ale_entry) \
> { \
> return cpsw_ale_get_field(ale_entry, start, bits); \
> -} \
> +}
> +
> +#define DEFINE_ALE_FIELD_SET(name, start, bits) \
> static inline void cpsw_ale_set_##name(u32 *ale_entry, u32 value) \
> { \
> cpsw_ale_set_field(ale_entry, start, bits, value); \
> }
>
> -#define DEFINE_ALE_FIELD1(name, start) \
> +#define DEFINE_ALE_FIELD(name, start, bits) \
> +DEFINE_ALE_FIELD_GET(name, start, bits) \
> +DEFINE_ALE_FIELD_SET(name, start, bits)
> +
> +#define DEFINE_ALE_FIELD1_GET(name, start) \
> static inline int cpsw_ale_get_##name(u32 *ale_entry, u32 bits) \
> { \
> return cpsw_ale_get_field(ale_entry, start, bits); \
> -} \
> +}
> +
> +#define DEFINE_ALE_FIELD1_SET(name, start) \
> static inline void cpsw_ale_set_##name(u32 *ale_entry, u32 value, \
> u32 bits) \
> { \
> cpsw_ale_set_field(ale_entry, start, bits, value); \
> }
>
> +#define DEFINE_ALE_FIELD1(name, start) \
> +DEFINE_ALE_FIELD1_GET(name, start) \
> +DEFINE_ALE_FIELD1_SET(name, start)
> +
> enum {
> ALE_ENT_VID_MEMBER_LIST = 0,
> ALE_ENT_VID_UNREG_MCAST_MSK,
> @@ -217,14 +229,14 @@ static const struct ale_entry_fld vlan_entry_k3_cpswxg[] = {
>
> DEFINE_ALE_FIELD(entry_type, 60, 2)
> DEFINE_ALE_FIELD(vlan_id, 48, 12)
> -DEFINE_ALE_FIELD(mcast_state, 62, 2)
> +DEFINE_ALE_FIELD_SET(mcast_state, 62, 2)
I don't understand why we need separate macros for GET and SET.
The original intent was to use one macro for both.
Otherwise we will have to add DEFINE_ALE_FIELD/1_SET to all the fields.
> DEFINE_ALE_FIELD1(port_mask, 66)
> DEFINE_ALE_FIELD(super, 65, 1)
> DEFINE_ALE_FIELD(ucast_type, 62, 2)
> -DEFINE_ALE_FIELD1(port_num, 66)
> -DEFINE_ALE_FIELD(blocked, 65, 1)
> -DEFINE_ALE_FIELD(secure, 64, 1)
> -DEFINE_ALE_FIELD(mcast, 40, 1)
> +DEFINE_ALE_FIELD1_SET(port_num, 66)
> +DEFINE_ALE_FIELD_SET(blocked, 65, 1)
> +DEFINE_ALE_FIELD_SET(secure, 64, 1)
> +DEFINE_ALE_FIELD_GET(mcast, 40, 1)
>
> #define NU_VLAN_UNREG_MCAST_IDX 1
>
>
--
cheers,
-roger
