Messages in this thread | | | Date | Thu, 03 Oct 2019 11:17:39 -0700 | From | mnalajal@codeauro ... | Subject | Re: [PATCH] base: soc: Handle custom soc information sysfs entries |
| |
On 2019-10-03 00:05, Greg KH wrote: > On Wed, Oct 02, 2019 at 05:06:14PM -0700, Murali Nalajala wrote: >> Soc framework exposed sysfs entries are not sufficient for some >> of the h/w platforms. Currently there is no interface where soc >> drivers can expose further information about their SoCs via soc >> framework. This change address this limitation where clients can >> pass their custom entries as attribute group and soc framework >> would expose them as sysfs properties. >> >> Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org> >> --- >> drivers/base/soc.c | 26 ++++++++++++++++++-------- >> include/linux/sys_soc.h | 1 + >> 2 files changed, 19 insertions(+), 8 deletions(-) > > Can you change a soc driver to use this? I don't think that this patch > works because: > >> >> diff --git a/drivers/base/soc.c b/drivers/base/soc.c >> index 7c0c5ca..ec70a58 100644 >> --- a/drivers/base/soc.c >> +++ b/drivers/base/soc.c >> @@ -15,6 +15,8 @@ >> #include <linux/err.h> >> #include <linux/glob.h> >> >> +#define NUM_ATTR_GROUPS 3 >> + >> static DEFINE_IDA(soc_ida); >> >> static ssize_t soc_info_get(struct device *dev, >> @@ -104,11 +106,6 @@ static ssize_t soc_info_get(struct device *dev, >> .is_visible = soc_attribute_mode, >> }; >> >> -static const struct attribute_group *soc_attr_groups[] = { >> - &soc_attr_group, >> - NULL, >> -}; >> - >> static void soc_release(struct device *dev) >> { >> struct soc_device *soc_dev = container_of(dev, struct soc_device, >> dev); >> @@ -121,6 +118,7 @@ static void soc_release(struct device *dev) >> struct soc_device *soc_device_register(struct soc_device_attribute >> *soc_dev_attr) >> { >> struct soc_device *soc_dev; >> + const struct attribute_group **soc_attr_groups = NULL; >> int ret; >> >> if (!soc_bus_type.p) { >> @@ -136,10 +134,20 @@ struct soc_device *soc_device_register(struct >> soc_device_attribute *soc_dev_attr >> goto out1; >> } >> >> + soc_attr_groups = kzalloc(sizeof(*soc_attr_groups) * >> + NUM_ATTR_GROUPS, GFP_KERNEL); >> + if (!soc_attr_groups) { >> + ret = -ENOMEM; >> + goto out2; >> + } >> + soc_attr_groups[0] = &soc_attr_group; >> + soc_attr_groups[1] = soc_dev_attr->custom_attr_group; >> + soc_attr_groups[2] = NULL; > > You set this, but never do anything with it that I can see. What am I > missing? no, since i am using the "soc_attr_groups" name as it here you do not see the assignment below. It is something like this soc_dev->dev.groups = soc_attr_groups; > >> + >> /* Fetch a unique (reclaimable) SOC ID. */ >> ret = ida_simple_get(&soc_ida, 0, 0, GFP_KERNEL); >> if (ret < 0) >> - goto out2; >> + goto out3; >> soc_dev->soc_dev_num = ret; >> >> soc_dev->attr = soc_dev_attr; >> @@ -151,14 +159,16 @@ struct soc_device *soc_device_register(struct >> soc_device_attribute *soc_dev_attr >> >> ret = device_register(&soc_dev->dev); >> if (ret) >> - goto out3; >> + goto out4; >> >> return soc_dev; >> >> -out3: >> +out4: >> ida_simple_remove(&soc_ida, soc_dev->soc_dev_num); >> put_device(&soc_dev->dev); >> soc_dev = NULL; >> +out3: >> + kfree(soc_attr_groups); >> out2: >> kfree(soc_dev); >> out1: > > You don't free it when the soc is removed? agree, will fix it in my next patch. > > thanks, > > greg k-h These changes are verified at my side on SM8250 with mode static compilation and module.
| |