| Message ID | 1432817307-28380-1-git-send-email-lambert.quentin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 28/05/2015 15:52, Takashi Iwai wrote: > At Thu, 28 May 2015 14:48:27 +0200, > Quentin Lambert wrote: >> You should use dev_groups instead of the dev_attrs field of struct >> bus_type. This converts the MDIO bus code to use the correct field. >> >> These modifications were made using Coccinelle. >> >> Signed-off-by: Quentin Lambert <lambert.quentin@gmail.com> > What's wrong with dev_attrs? > (Not offending, I'm just curious where the preference for dev_groups > is referred...) This patch is based on commit 4192c74940e23da727eb02b7b4ee779dde2f670, its message indicates that dev_attrs is going away. Quentin
On 28/05/2015 15:52, Takashi Iwai wrote: > At Thu, 28 May 2015 14:48:27 +0200, > Quentin Lambert wrote: >> You should use dev_groups instead of the dev_attrs field of struct >> bus_type. This converts the MDIO bus code to use the correct field. >> >> These modifications were made using Coccinelle. >> >> Signed-off-by: Quentin Lambert <lambert.quentin@gmail.com> > What's wrong with dev_attrs? > (Not offending, I'm just curious where the preference for dev_groups > is referred...) This patch is based on commit 4192c74940e23da727eb02b7b4ee779dde2f670, its message indicates that dev_attrs is going away. Quentin
On 28/05/2015 16:09, Takashi Iwai wrote: > At Thu, 28 May 2015 15:59:50 +0200, > Quentin Lambert wrote: >> >> >> On 28/05/2015 15:52, Takashi Iwai wrote: >>> At Thu, 28 May 2015 14:48:27 +0200, >>> Quentin Lambert wrote: >>>> You should use dev_groups instead of the dev_attrs field of struct >>>> bus_type. This converts the MDIO bus code to use the correct field. >>>> >>>> These modifications were made using Coccinelle. >>>> >>>> Signed-off-by: Quentin Lambert <lambert.quentin@gmail.com> >>> What's wrong with dev_attrs? >>> (Not offending, I'm just curious where the preference for dev_groups >>> is referred...) >> This patch is based on commit 4192c74940e23da727eb02b7b4ee779dde2f670, >> its message indicates that dev_attrs is going away. > OK, makes sense. Then please keep such information in the changelog. > BTW, the code in question isn't MDIO at all. Sorry, about that! > > Also, it'd be better to move ATTRIBUTE_GROUPS(soundbus_dev) into > soundbus/sysfs.c, and make it this global instead of > soundbus_dev_attrs[]. Ok, I need to find a nice way to do that because ATTRIBUTE_GROUPS declares the structure as static. I will send a second version with a better solution. Quentin > > > Takashi
On 28/05/2015 17:01, Takashi Iwai wrote: >>> Also, it'd be better to move ATTRIBUTE_GROUPS(soundbus_dev) into >>> soundbus/sysfs.c, and make it this global instead of >>> soundbus_dev_attrs[]. >> Ok, I need to find a nice way to do that because ATTRIBUTE_GROUPS >> declares the >> structure as static. > > If it results in an ungly code, it's fine with the original code, > too. But, maybe with a comment indicating that xxx_dev_attrs[] is > defined in xxx.c. > > Since sound/aoa/soundbus/sysfs is small, a solution would be to merge sound/aoa/soundbus/sysfs.c and sound/aoa/soundus/core.c. Moreover all 172 other usages of the ATTRIBUTE_GROUPS macro define the struct attribute *xxx_attrs[] in the same file they assign the .dev_groups field. I'm not sure about this change as it seems way more important than adding a comment line as you suggested. Should I send a patch merging these two files? Quentin
At Fri, 29 May 2015 17:49:06 +0200, Quentin Lambert wrote: > > > > On 28/05/2015 17:01, Takashi Iwai wrote: > >>> Also, it'd be better to move ATTRIBUTE_GROUPS(soundbus_dev) into > >>> soundbus/sysfs.c, and make it this global instead of > >>> soundbus_dev_attrs[]. > >> Ok, I need to find a nice way to do that because ATTRIBUTE_GROUPS > >> declares the > >> structure as static. > > > > If it results in an ungly code, it's fine with the original code, > > too. But, maybe with a comment indicating that xxx_dev_attrs[] is > > defined in xxx.c. > > > > > Since sound/aoa/soundbus/sysfs is small, a solution would be > to merge sound/aoa/soundbus/sysfs.c and sound/aoa/soundus/core.c. > Moreover all 172 other usages of the ATTRIBUTE_GROUPS macro > define the struct attribute *xxx_attrs[] in the same file > they assign the .dev_groups field. > > I'm not sure about this change as it seems way more important than > adding a comment line as you suggested. Not "important" but more "radical", I'd say. > Should I send a patch merging these two files? I don't think it's worth. This is a fairly old hardware, thus the code isn't so actively used/maintained. Unless it looks too ugly, we shouldn't touch too many things just for refactoring. So, go for the way to have a minimum change. thanks, Takashi
--- a/sound/aoa/soundbus/core.c +++ b/sound/aoa/soundbus/core.c @@ -150,6 +150,7 @@ static int soundbus_device_resume(struct #endif /* CONFIG_PM */ +ATTRIBUTE_GROUPS(soundbus_dev); static struct bus_type soundbus_bus_type = { .name = "aoa-soundbus", .probe = soundbus_probe, @@ -160,7 +161,7 @@ static struct bus_type soundbus_bus_type .suspend = soundbus_device_suspend, .resume = soundbus_device_resume, #endif - .dev_attrs = soundbus_dev_attrs, + .dev_groups = soundbus_dev_groups, }; int soundbus_add_one(struct soundbus_dev *dev) --- a/sound/aoa/soundbus/soundbus.h +++ b/sound/aoa/soundbus/soundbus.h @@ -199,6 +199,6 @@ struct soundbus_driver { extern int soundbus_register_driver(struct soundbus_driver *drv); extern void soundbus_unregister_driver(struct soundbus_driver *drv); -extern struct device_attribute soundbus_dev_attrs[]; +extern struct attribute *soundbus_dev_attrs[]; #endif /* __SOUNDBUS_H */ --- a/sound/aoa/soundbus/sysfs.c +++ b/sound/aoa/soundbus/sysfs.c @@ -30,13 +30,16 @@ static ssize_t modalias_show(struct devi return length; } +static DEVICE_ATTR_RO(modalias); soundbus_config_of_attr (name, "%s\n"); +static DEVICE_ATTR_RO(name); soundbus_config_of_attr (type, "%s\n"); +static DEVICE_ATTR_RO(type); -struct device_attribute soundbus_dev_attrs[] = { - __ATTR_RO(name), - __ATTR_RO(type), - __ATTR_RO(modalias), - __ATTR_NULL +struct attribute *soundbus_dev_attrs[] = { + &dev_attr_name.attr, + &dev_attr_type.attr, + &dev_attr_modalias.attr, + NULL, };
You should use dev_groups instead of the dev_attrs field of struct bus_type. This converts the MDIO bus code to use the correct field. These modifications were made using Coccinelle. Signed-off-by: Quentin Lambert <lambert.quentin@gmail.com> --- sound/aoa/soundbus/core.c | 3 ++- sound/aoa/soundbus/soundbus.h | 2 +- sound/aoa/soundbus/sysfs.c | 13 ++++++++----- 3 files changed, 11 insertions(+), 7 deletions(-)