Re: [PATCH 6/6] staging:iio: attrs/event_attrs -> struct attribute * + move to iio_dev.

[Jonathan Cameron <jic23@kernel.org>]

On 01/09/2012 10:14 AM, Lars-Peter Clausen wrote:
> On 01/07/2012 11:25 AM, Jonathan Cameron wrote:
>>
>> diff --git a/drivers/staging/iio/adc/ad7606_core.c
> b/drivers/staging/iio/adc/ad7606_core.c
>> index 97e8d3d..99d91ee 100644
>> --- a/drivers/staging/iio/adc/ad7606_core.c
>> +++ b/drivers/staging/iio/adc/ad7606_core.c
>> @@ -205,30 +205,18 @@ static struct attribute
> *ad7606_attributes_os_and_range[] = {
>>  	NULL,
>>  };
>>
>> -static const struct attribute_group ad7606_attribute_group_os_and_range = {
>> -	.attrs = ad7606_attributes_os_and_range,
>> -};
>> -
>>  static struct attribute *ad7606_attributes_os[] = {
>>  	&iio_dev_attr_oversampling_ratio.dev_attr.attr,
>>  	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
>>  	NULL,
>>  };
>>
>> -static const struct attribute_group ad7606_attribute_group_os = {
>> -	.attrs = ad7606_attributes_os,
>> -};
>> -
>>  static struct attribute *ad7606_attributes_range[] = {
>>  	&iio_dev_attr_in_voltage_range.dev_attr.attr,
>>  	&iio_const_attr_in_voltage_range_available.dev_attr.attr,
>>  	NULL,
>>  };
>>
>> -static const struct attribute_group ad7606_attribute_group_range = {
>> -	.attrs = ad7606_attributes_range,
>> -};
>> -
>>  #define AD7606_CHANNEL(num)				\
>>  	{						\
>>  		.type = IIO_VOLTAGE,			\
>> @@ -429,27 +417,9 @@ static irqreturn_t ad7606_interrupt(int irq, void
> *dev_id)
>>  	return IRQ_HANDLED;
>>  };
>>
>> -static const struct iio_info ad7606_info_no_os_or_range = {
>> -	.driver_module = THIS_MODULE,
>> -	.read_raw = &ad7606_read_raw,
>> -};
>> -
>> -static const struct iio_info ad7606_info_os_and_range = {
>> -	.driver_module = THIS_MODULE,
>> -	.read_raw = &ad7606_read_raw,
>> -	.attrs = &ad7606_attribute_group_os_and_range,
>> -};
>> -
>> -static const struct iio_info ad7606_info_os = {
>> +static const struct iio_info ad7606_info = {
>>  	.driver_module = THIS_MODULE,
>>  	.read_raw = &ad7606_read_raw,
>> -	.attrs = &ad7606_attribute_group_os,
>> -};
>> -
>> -static const struct iio_info ad7606_info_range = {
>> -	.driver_module = THIS_MODULE,
>> -	.read_raw = &ad7606_read_raw,
>> -	.attrs = &ad7606_attribute_group_range,
>>  };
>>
>>  struct iio_dev *ad7606_probe(struct device *dev, int irq,
>> @@ -494,19 +464,16 @@ struct iio_dev *ad7606_probe(struct device *dev, int
> irq,
>>  	st->chip_info = &ad7606_chip_info_tbl[id];
>>
>>  	indio_dev->dev.parent = dev;
>> +	indio_dev->info = &ad7606_info;
>>  	if (gpio_is_valid(st->pdata->gpio_os0) &&
>>  	    gpio_is_valid(st->pdata->gpio_os1) &&
>>  	    gpio_is_valid(st->pdata->gpio_os2)) {
>>  		if (gpio_is_valid(st->pdata->gpio_range))
>> -			indio_dev->info = &ad7606_info_os_and_range;
>> +			indio_dev->attrs = ad7606_attributes_os_and_range;
>>  		else
>> -			indio_dev->info = &ad7606_info_os;
>> -	} else {
>> -		if (gpio_is_valid(st->pdata->gpio_range))
>> -			indio_dev->info = &ad7606_info_range;
>> -		else
>> -			indio_dev->info = &ad7606_info_no_os_or_range;
>> -	}
>> +			indio_dev->attrs = ad7606_attributes_os;
>> +	} else if (gpio_is_valid(st->pdata->gpio_range))
>> +			indio_dev->attrs = ad7606_attributes_range;
>>  	indio_dev->modes = INDIO_DIRECT_MODE;
>>  	indio_dev->name = st->chip_info->name;
>>  	indio_dev->channels = st->chip_info->channels;
> 
> This makes me wonder if we not better add a function which can add a single
> attribute to the attribute list at runtime. Or maybe just use
> device_create_file directly.
Device create file is out I think. It can only be applied after a the
group has been created (so after the iio registration is done)  The
whole issue is that udev doesn't get notified of such creations.  That's
why we jumped through these hoops in the first place.
(I've never entirely understood why this is the case, but Kay and
Greg both assured me it was the case - only reliable option is to
add all files on device registration as here.)  Yes, lots of the
kernel doesn't do that, but they were strongly in favour of it for
any new code.

We could add our own function, but personally I'm against it.  In the
vast majority of cases it is irrelevant and we have this approach for
those where it might be a small clean up.  If these get more common
then I'll come around to such a function with the slight additional
complexity it would need.

So in my view a question for another day!
> - Lars