Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver

[Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>]

On Thu, Oct 8, 2009 at 8:33 AM, Anton Vorontsov
<avorontsov-hkdhdckH98+B+jHODAdFcQ@public.gmane.org> wrote:
> On Thu, Oct 08, 2009 at 04:04:32PM +0200, Wolfram Sang wrote:
>> As Anton introduced archdata support, I wondered if this is a suitable way to
>> handle the platform_data/devicetree_property-dualism (at least for some
>> drivers).
>
> Yes, we handle OF in a similar way for mmc_spi driver. Though,
>
> [...]
>> --- a/drivers/misc/eeprom/at24.c
>> +++ b/drivers/misc/eeprom/at24.c
>> @@ -22,6 +22,9 @@
> [...]
>> +#ifdef CONFIG_OF_I2C
>> +#include <linux/of.h>
>> +#endif
> [..]
>> +#ifdef CONFIG_OF_I2C
>> +static void at24_get_ofdata(struct i2c_client *client, struct at24_platform_data *chip)
>> +{
>> +     const u32 *val;
>> +     struct device_node *node = dev_archdata_get_node(&client->dev.archdata);
>> +
>> +     if (node) {
>> +             if (of_get_property(node, "read-only", NULL))
>> +                     chip->flags |= AT24_FLAG_READONLY;
>> +             val = of_get_property(node, "pagesize", NULL);
>> +             if (val)
>> +                     chip->page_size = *val;
>> +     }
>> +}
>> +#else
>> +static void at24_get_ofdata(struct i2c_client *client, struct at24_platform_data *chip)
>> +{ }
>> +#endif
>
> #ifdefs are ugly in .c files. I'd suggest to move the OF code
> into a separate file. As an example, see

Please don't.  It is such a small amount of code, and I far prefer to
see drivers self contained in a single .c file.  #ifdefs are fine IMHO
when it is a top level block, and not inside a function block.  In the
example you give, I do like the move toward focusing on the pdata
structure; but the patch ads a *lot* of code for something very
simple.  And then we'll need to do the same think for every driver
which will ever be described in the device tree.  It's the right
direction, but still not right.  Driver writers shouldn't have to
write anything more than a tiny function to populate pdata from the
device tree.  Managing that pdata instance needs to be done with
common infrastructure (but I don't have a firm idea about how it
should look yet).  In the mean time I think Wolfram's approach has
lower impact.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.