On 12/09/2024 11:59, Simon Horman wrote: > On Thu, Sep 12, 2024 at 10:07:27AM +0300, Roger Quadros wrote: >> 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. > > Hi Roger, > > Sorry for not being clearer. > > My intent was to avoid declaring functions that are never used. > Perhaps it is best explained by some examples. > > In the case of mcast_state, the compiler flags that the get accessor is > never used. The intent is of this patch addresses that by declaring the set > accessor for mcast_state. Likewise for other similar cases. > > OTOH, in the case of, f.e. vlan_id, the set and get accessor functions are > both used, and DEFINE_ALE_FIELD continues to be used to define them both. > DEFINE_ALE_FIELD is implemented as the combination of _SET and _GET. > Thanks for the explanation Simon. I understand now. Would using __maybe_unused__ be preferable to get rid of the warnings? That way we don't need to care if both set/get helpers are used or not and don't have to touch the below code ever again except to add new 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