On Tue, Aug 23, 2011 at 08:25:29PM +0100, Jonathan Cameron wrote: > On 08/23/11 12:01, Jonathan Cameron wrote: > > On 08/23/11 01:33, Greg KH wrote: > >> On Wed, Aug 17, 2011 at 05:27:41PM +0100, Jonathan Cameron wrote: > >>> On 08/17/11 11:17, Jonathan Cameron wrote: > >>>> The following is a quick stab at avoiding a hideous work around > >>>> we are currently using in IIO. In some cases we have entire > >>>> attribute groups that are created from descriptions an array > >>>> of struct iio_chan_spec. To keep the reference counts sane > >>>> and cause subdirectories to be created we are currently using > >>>> dummy groups and dummy attribute arrays (provided once in the > >>>> core). This series is an initial probably stupid approach > >>>> to avoiding this. > >>>> > >>>> Greg has expressed some doubts about whether subdirectories are > >>>> ever a good idea, but the problem occurs for the top level > >>>> directory as well (handled by patch 1). > >>>> > >>>> Note, all attributes are created at probe time. Ultimately we > >>>> are just respinning the create_group code to allow us to create > >>>> the attributes from a device description rather than statically > >>>> allocating them in each driver (results in a massive reduction > >>>> in repeated code). > >>>> > >>>> All opinions welcomed. > >>>> > >>>> (this is definitely an rfc, the code isn't even tested yet) > >>> Now tested and looks fine, so I've pushed it to our iio dev tree > >>> (which is re based a few times a day currently so I can always > >>> drop it again ;) > >> > >> Can you show me how you would use this so I could try to understand it a > >> bit better? > > Sorry, should have pointed you at an example. > > > > See our iio-dev tree for the full code (some of it is in your staging tree > > as well). > > http://git.kernel.org/?p=linux/kernel/git/jic23/iio-blue.git;a=summary > > > > Summary: > > > > Sysfs attributes are created from struct iio_chan_spec arrays. These structures > > describe the facilities and characteristics of a physical device channel in > > a compact fashion. Sometimes there are no other attributes thus we currently > > end up with dummy attribute_groups in the core that are registered. The > > purpose of this is to keep the reference counts consistent. > > > > Details: > > > > The core of the matter is that we have channel descriptions for every > > channel on a device. This encodes most of the information about the > > channel in a consistent compact way (there are a few more unusual cases > > we are still working out how to handle). The sysfs control and data reading > > attributes are generated from these struct iio_chan_spec structure arrays > > rather than existing as a static set of attributes as they previously did. > > > > This change (suggested by Arnd) has two big advantages: > > 1) Massive reduction in boilerplate code. > > 2) Enforced consistency of interface across drivers. > > > > The base directory contains simple read attributes for the files and > > stuff like calibration offsets. In many cases there are other elements > > in there not handled by iio_chan_spec. When that happens we register the > > group of other attributes first and then use sysfs_add_file_to_group to > > add the other attributes as they are created from the description. > > A klist keeps track of the attributes added so they can be cleanly > > removed later. > > > > Key function is __iio_add_chan_devattr in industrialio-core.c > > int __iio_add_chan_devattr(const char *postfix, > > const char *group, > > struct iio_chan_spec const *chan, > > ssize_t (*readfunc)(struct device *dev, > > struct device_attribute *attr, > > char *buf), > > ssize_t (*writefunc)(struct device *dev, > > struct device_attribute *attr, > > const char *buf, > > size_t len), > > u64 mask, > > bool generic, > > struct device *dev, > > struct list_head *attr_list) > > { > > int ret; > > struct iio_dev_attr *iio_attr, *t; > > > > iio_attr = kzalloc(sizeof *iio_attr, GFP_KERNEL); > > if (iio_attr == NULL) { > > ret = -ENOMEM; > > goto error_ret; > > } > > ret = __iio_device_attr_init(&iio_attr->dev_attr, > > postfix, chan, > > readfunc, writefunc, generic); > > if (ret) > > goto error_iio_dev_attr_free; > > iio_attr->c = chan; > > iio_attr->address = mask; > > list_for_each_entry(t, attr_list, l) > > if (strcmp(t->dev_attr.attr.name, > > iio_attr->dev_attr.attr.name) == 0) { > > if (!generic) > > dev_err(dev, "tried to double register : %s\n", > > t->dev_attr.attr.name); > > ret = -EBUSY; > > goto error_device_attr_deinit; > > } > > > > ret = sysfs_add_file_to_group(&dev->kobj, > > &iio_attr->dev_attr.attr, group); > > if (ret < 0) > > goto error_device_attr_deinit; > > > > list_add(&iio_attr->l, attr_list); > > > > return 0; > > > > error_device_attr_deinit: > > __iio_device_attr_deinit(&iio_attr->dev_attr); > > error_iio_dev_attr_free: > > kfree(iio_attr); > > error_ret: > > return ret; > > } > > > > > > Call to __iio_device_attr_init builds the attribute name up and then > > it performs checks for double registration (valid to try for 'generic' > > attributes - but not others. When it is marked generic it means > > it applies to a number of channels in_accel_scale for example applies to > > all registered in_accel channels and it simply will not be added if > > already there). > > > > The file is added with sysfs_add_file_to_group. > > > > So ultimately we have a dynamic equivalent of what goes on in > > sysfs_create_group with a const group. We could in theory > > do two passes - first to work out what space is needed and second > > to create a suitable attribute_group, but it's a lot uglier than > > doing it in a single pass through the chan_spec structure array (made > > so by the need to handle those 'generic' attributes.) An alternative > > is to do all but the sysfs_add_file_to_group and then build an > > attribute group from our existing klist of attributes. > > > > Basically I'm happy to do anything that makes this work. If we > > agree there is a reason to have sysfs_add_file_to_group available > > then to my mind we should also allow for empty groups. Right now > > it looks like there are only a few users of that (I count about 6 with > > 2 of them in IIO). > > > > The group add of an empty group is simply to get the reference count > > consistent. > > > > The slightly more involved case is for the scanelements subdirectory. > > This describes the contents of the any buffers that the driver supports > > and is often (but not quite always) generated entirely from the iio_chan_spec > > structure arrays provided by the drivers. It's in a subdirectory as purely > > a means of grouping elements related to a particular task (buffer description) > > rather than any inherent requirement. This case motivates the allowing a group > > with a null pointer in .attrs to avoid having a > > struct attribute *dummy_attrs[] = {NULL}; > > which is ugly. > > > > So what do you recommend? > Having just come across Bart's documentation patches to the driver-model > docs I now understand the userspace issue (misunderstood what you said > the other day). Basically if it isn't done through the groups element > of struct device userspace won't know the attributes have been created. > > Will see if I can rework the registration code to build up a suitable cache > of what will be in the attribute groups for each device. This dynamic > cache can then be used to build up everything needed before the > device_register occurs. It's going to be somewhat clunky but contained > within the IIO core so not too bad. > > So upshot is ignore this patch set. The hack with the dummy groups > will have to stand for 3.1 in IIO. Ok, consider it ignored :) -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html