David Brownell wrote:
Note that the Davinci list isn't the right place to discuss this
particular driver ... and it'll probably reject my posts.
On Thursday 28 August 2008, Sergei Shtylyov wrote:
@@ -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.
Two pointers ... are you positive that's not just eating up some
structure padding?
Pretty much sure.
It's used in container_of(), so storing it
elsewhere would imply a different implementation strategy.
Oh, that must be scary! :-)
Remember that using kmalloc() for such a thing not only creates
I didn't suggesting anything alike, I suggested to simply not have it here
because it:
a) does not convey *any* information to the users of 'struct at24_data';
b) takes up space;
c) does that per device instance ISO only once.
@@ -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.
Why "better"? I see *NO* benefits from that approach. And having
done things like that before, I've observed that having this particular
kind of inter-module dependency makes trouble that's worth avoiding.
OK, I have an alternate suggestion if you want to avoid that.
(One of them is to prevent using those methods from statically linked
code, when the methods are in modules.)
Statically linked code wouldn't be able to call them not knowing what to
pass for the first argument.
+static ssize_t at24_iface_read(struct at24_iface *iface, char *buf,
+ off_t offset, size_t 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...
Again, why "better"?
To avoid the useless inclusion of the instance of 'struct at24_iface' into
'struct at24_data' -- that instance, if you insist on using it, should be a
'static const' variable.
Passing "void *" is generally considered worse
That didn't stop you from using 'void *context' as an argument. ;-)
than passing values with types that the compiler can verify.
Oh yeah, let's pass values that don't make sense just to make compilers
happy. Despite one can always cheat compiler by using a type cast on a
pointer argument or just passing NULL.
People are replacing kernel interfaces which work like that with ones that
have typed pointers.
My own issue is different: since I know at least *NINE* drivers that
could usefully export generic versions of this interface, so I'd rather
see this much more completly decoupled from the at24 driver.
Well, then having an interface structure is well justified.
That could be done later with a simple struct rename, so this is a good start.
Ugh. Don't frighten me with a picture of this container_of() trick repated
in 8 more drivers. :-]
@@ -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...
Only if you (a) are happy to have the other nine drivers each create
the same sort of driver-specific interface,
Why would they, if we use 'void *' to pass the device instance? We could
pass the interface structure as yet another argument, that's all. No useless
inclusion of it per device needed.
and (b) like having that kind of annoying init sequence dependency multiply.
No, I don't. But your way of solving this is certainly not the best one.
+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.
I take it you've not done much C programming... ?
Hah.
It's a really common idiom when dealing with multiple instances of a given type.
In this case, it's more like an idiom turned into idiocy. :-/
I mean *this exact structure*, not a structure representing an instance in
general, so no need to teach me basics. This structure can hardly represent
anything -- aside of by *abusing* it by invoking container_of() to get to the
real enclosing structure.
And for in-kernel examples ... they're all over. Look at the VFS layer
for one case.
I have better things to do... :-)
+ int (*setup)(struct at24_iface *, void *context);
I'm suggesting:
int (*setup)(void *data, void *context);
I can tell you are; but *why* suggest that? As I noted above,
that doesn't seem like an improvement.
To pass the real device instance, not a substitute.
This seems like bikeshedding...
Huh? :-)
- Dave
WBR, Sergei
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
