Thanks for reviewing. >-----Original Message----- >From: Borislav Petkov <bp@xxxxxxxxx> >Sent: 13 September 2024 17:41 >To: Shiju Jose <shiju.jose@xxxxxxxxxx> >Cc: linux-edac@xxxxxxxxxxxxxxx; linux-cxl@xxxxxxxxxxxxxxx; linux- >acpi@xxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; >tony.luck@xxxxxxxxx; rafael@xxxxxxxxxx; lenb@xxxxxxxxxx; >mchehab@xxxxxxxxxx; dan.j.williams@xxxxxxxxx; dave@xxxxxxxxxxxx; Jonathan >Cameron <jonathan.cameron@xxxxxxxxxx>; dave.jiang@xxxxxxxxx; >alison.schofield@xxxxxxxxx; vishal.l.verma@xxxxxxxxx; ira.weiny@xxxxxxxxx; >david@xxxxxxxxxx; Vilas.Sridharan@xxxxxxx; leo.duran@xxxxxxx; >Yazen.Ghannam@xxxxxxx; rientjes@xxxxxxxxxx; jiaqiyan@xxxxxxxxxx; >Jon.Grimm@xxxxxxx; dave.hansen@xxxxxxxxxxxxxxx; >naoya.horiguchi@xxxxxxx; james.morse@xxxxxxx; jthoughton@xxxxxxxxxx; >somasundaram.a@xxxxxxx; erdemaktas@xxxxxxxxxx; pgonda@xxxxxxxxxx; >duenwen@xxxxxxxxxx; mike.malvestuto@xxxxxxxxx; gthelen@xxxxxxxxxx; >wschwartz@xxxxxxxxxxxxxxxxxxx; dferguson@xxxxxxxxxxxxxxxxxxx; >wbs@xxxxxxxxxxxxxxxxxxxxxx; nifan.cxl@xxxxxxxxx; jgroves@xxxxxxxxxx; >vsalve@xxxxxxxxxx; tanxiaofei <tanxiaofei@xxxxxxxxxx>; Zengtao (B) ><prime.zeng@xxxxxxxxxxxxx>; Roberto Sassu <roberto.sassu@xxxxxxxxxx>; >kangkang.shen@xxxxxxxxxxxxx; wanghuiqiang <wanghuiqiang@xxxxxxxxxx>; >Linuxarm <linuxarm@xxxxxxxxxx> >Subject: Re: [PATCH v12 01/17] EDAC: Add support for EDAC device features >control > >On Wed, Sep 11, 2024 at 10:04:30AM +0100, shiju.jose@xxxxxxxxxx wrote: >> +/** >> + * edac_dev_feature_init - Init a RAS feature >> + * @parent: client device. >> + * @dev_data: pointer to the edac_dev_data structure, which contains >> + * client device specific info. >> + * @feat: pointer to struct edac_dev_feature. >> + * @attr_groups: pointer to attribute group's container. >> + * >> + * Returns number of scrub features attribute groups on success, > >Not "scrub" - this is an interface initializing a generic feature. Will correct. > >> + * error otherwise. >> + */ >> +static int edac_dev_feat_init(struct device *parent, >> + struct edac_dev_data *dev_data, >> + const struct edac_dev_feature *ras_feat, >> + const struct attribute_group **attr_groups) { >> + int num; >> + >> + switch (ras_feat->ft_type) { >> + case RAS_FEAT_SCRUB: >> + dev_data->scrub_ops = ras_feat->scrub_ops; >> + dev_data->private = ras_feat->ctx; >> + return 1; >> + case RAS_FEAT_ECS: >> + num = ras_feat->ecs_info.num_media_frus; >> + dev_data->ecs_ops = ras_feat->ecs_ops; >> + dev_data->private = ras_feat->ctx; >> + return num; >> + case RAS_FEAT_PPR: >> + dev_data->ppr_ops = ras_feat->ppr_ops; >> + dev_data->private = ras_feat->ctx; >> + return 1; >> + default: >> + return -EINVAL; >> + } >> +} > >And why does this function even exist and has kernel-doc comments when all it >does is assign a couple of values? And it gets called exactly once? > >Just merge its body into the call site. There you can reuse the switch-case there >too. No need for too much noodling around. edac_dev_feat_init () function is updated with feature specific function call() etc in subsequent EDAC feature specific patches. Thus added a separate function. > >> diff --git a/include/linux/edac.h b/include/linux/edac.h index >> b4ee8961e623..b337254cf5b8 100644 >> --- a/include/linux/edac.h >> +++ b/include/linux/edac.h >> @@ -661,4 +661,59 @@ static inline struct dimm_info >> *edac_get_dimm(struct mem_ctl_info *mci, >> >> return mci->dimms[index]; >> } >> + >> +/* EDAC device features */ >> + >> +#define EDAC_FEAT_NAME_LEN 128 >> + >> +/* RAS feature type */ >> +enum edac_dev_feat { >> + RAS_FEAT_SCRUB, >> + RAS_FEAT_ECS, >> + RAS_FEAT_PPR, >> + RAS_FEAT_MAX > >I still don't know what ECS or PPR is. I will add comment/documentation here with a short explanation of features if that make sense? Each feature is described in the subsequent EDAC feature specific patches. > >-- >Regards/Gruss, > Boris. > >https://people.kernel.org/tglx/notes-about-netiquette Thanks, Shiju