Sergei Shtylyov <[EMAIL PROTECTED]> writes:
> Hello.
>
> Kevin Hilman wrote:
>
>> This patch adds an interface by which other kernel code can read/write
>> detected EEPROM.
>>
>> The platform code registers a 'setup' callback with the
>> at24_platform_data. When the at24 driver detects an EEPROM, it fills
>> out the read and write functions of at24_iface and calls the setup
>> callback. The platform code can then use the read/write functions in
>> the at24_iface struct for reading and writing the EEPROM.
>>
>> Original idea, review and updates by David Brownell <[EMAIL PROTECTED]>
>>
>
> This seems over-engineered (and suboptimal) to me.
I'll let you take this up with Dave. We went over this a few times,
and the goal here is to have a generic enough type of interface that
it could be used for SPI EEPROMS, NVRAM etc.
I will likely put this into DaVinci git until something is finalized
on the i2c list.
Kevin
>> Signed-off-by: Kevin Hilman <[EMAIL PROTECTED]>
>>
> [...]
>> diff --git a/drivers/i2c/chips/at24.c b/drivers/i2c/chips/at24.c
>> index 2a4acb2..ce6423d 100644
>> --- a/drivers/i2c/chips/at24.c
>> +++ b/drivers/i2c/chips/at24.c
>> @@ -53,6 +53,7 @@
>> struct at24_data {
>> struct at24_platform_data chip;
>> + struct at24_iface iface;
>>
>
> Holding this structure (with always the same member values) here
> seems a waste of memory.
>
>> bool use_smbus;
>> /*
>> @@ -264,13 +265,6 @@ static ssize_t at24_bin_read(struct kobject *kobj,
>> struct bin_attribute *attr,
>> /*
>> - * REVISIT: export at24_bin{read,write}() to let other kernel code use
>> - * eeprom data. For example, it might hold a board's Ethernet address, or
>> - * board-specific calibration data generated on the manufacturing floor.
>> - */
>> -
>> -
>> -/*
>>
>
> I think it would have been better to export the read()/write()
> methods directly.
>
>> @@ -386,6 +380,30 @@ static ssize_t at24_bin_write(struct kobject *kobj,
>> struct bin_attribute *attr,
>>
>> /*-------------------------------------------------------------------------*/
>> +/*
>> + * This lets other kernel code access the eeprom data. For example, it
>> + * might hold a board's Ethernet address, or board-specific calibration
>> + * data generated on the manufacturing floor.
>> + */
>> +
>> +static ssize_t at24_iface_read(struct at24_iface *iface, char *buf,
>> + off_t offset, size_t count)
>> +{
>> + struct at24_data *at24 = container_of(iface, struct at24_data, iface);
>> +
>> + return at24_eeprom_read(at24, buf, offset, count);
>> +}
>> +
>> +static ssize_t at24_iface_write(struct at24_iface *iface, char *buf,
>> + off_t offset, size_t count)
>> +{
>> + struct at24_data *at24 = container_of(iface, struct at24_data, iface);
>> +
>> + return at24_eeprom_write(at24, buf, offset, count);
>> +}
>> +
>>
>
> I think it would have been better to pass 'void *' to these methods,
> which they could cast to 'struct at24_data *', than to waste memory on
> storing 'struct at24_iface' within 'struct at24_data' just to make use
> of container_of() here...
>
>> @@ -521,6 +545,10 @@ static int at24_probe(struct i2c_client *client, const
>> struct i2c_device_id *id)
>> at24->write_max,
>> use_smbus ? ", use_smbus" : "");
>> + /* export data to kernel code */
>> + if (chip.setup)
>> + chip.setup(&at24->iface, chip.context);
>>
>
> Could just pass 'at24' itself here...
>
>> diff --git a/include/linux/i2c/at24.h b/include/linux/i2c/at24.h
>> index f6edd52..50008cc 100644
>> --- a/include/linux/i2c/at24.h
>> +++ b/include/linux/i2c/at24.h
>> @@ -15,6 +15,13 @@
>> * is bigger than what the chip actually supports!
>> */
>> +struct at24_iface {
>> + ssize_t (*read)(struct at24_iface *, char *buf, off_t offset,
>> + size_t count);
>> + ssize_t (*write)(struct at24_iface *, char *buf, off_t offset,
>> + size_t count);
>> +};
>> +
>>
>
> Ugh, passing a pointer to this structure to the read()/write()
> methods does look... strange.
>
>> struct at24_platform_data {
>> u32 byte_len; /* size (sum of all addr) */
>> u16 page_size; /* for writes */
>> @@ -23,6 +30,9 @@ struct at24_platform_data {
>> #define AT24_FLAG_READONLY 0x40 /* sysfs-entry will be read-only */
>> #define AT24_FLAG_IRUGO 0x20 /* sysfs-entry will be
>> world-readable */
>> #define AT24_FLAG_TAKE8ADDR 0x10 /* take always 8 addresses (24c00) */
>> +
>> + int (*setup)(struct at24_iface *, void *context);
>>
>
> I'm suggesting:
>
> int (*setup)(void *data, void *context);
>
>> + void *context;
>> };
>> #endif /* _LINUX_AT24_H */
>>
>
> WBR, Sergei
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
