Hi Jean, some more comments while working on the driver:
> > +static struct i2c_client *at24_ee_address(struct at24_data *at24, u16
> > *addr,
> > + unsigned *offset)
> > +{
> > + struct at24_platform_data *chip = &at24->chip;
> > + unsigned i;
> > +
> > + if (*offset >= chip->byte_len)
> > + return NULL;
> > +
> > + if (chip->flags & AT24_EE_ADDR2) {
> > + i = *offset >> 16;
> > + *offset &= 0xffff;
> > + } else {
> > + i = *offset >> 8;
> > + *offset &= 0xff;
> > + }
> > +
> > + if (unlikely(i > chip->i2c_addr_mask)) {
> > + dev_err(&at24->client[0]->dev,
> > + "requested client %u not available!\n", i);
> > + return NULL;
> > + }
>
> That's not just unlikely... that would be a bug in the driver, right?
> This could be protected by #ifdef DEBUG, to not slow down the driver
> uselessly.
This error case can happen, if the supplied 'byte_len' and
'i2c_addr_mask' don't fit. I will put this check to the probe-function,
this should also do. Calculating i2c_addr_mask from byte_len still gives
me headaches, although I'd also like this to happen (see later).
> > + /*
> > + * When we have a better choice than SMBus calls, use a combined
> > + * I2C message. Write address; then read up to io_limit data bytes.
> > + * Note that read page rollover helps us here (unlike writes).
> > + */
> > + msg[0].buf = addr;
> > + addr[1] = (u8) offset;
>
> Please use a proper mask instead of a cast, it's clearer what you're
> doing.
>
> > + addr[0] = (u8) (offset >> 8);
>
> Cast is not needed.
I tend to change it like this:
offset_be = cpu_to_be16(offset);
msg[0].buf = (u8 *) &offset_be;
> > +static ssize_t at24_ee_write(struct at24_data *at24, char *buf,
> > loff_t off, + size_t count) +{ + struct
> > i2c_client *client; + struct i2c_msg msg;
>
> Doubled space.
>
> > + unsigned offset = (unsigned) off;
>
> Why don't you simply make the offset parameter an unsigned int, if
> that's what you want?
I changed the parameter to unsigned. But I think I need to have another
look, not that we are bitten by some cast-sideeffects.
> > + /* buffer big enough to stick the address at the beginning */
> > + at24->writebuf = kmalloc(at24->write_max + 2, GFP_KERNEL);
> > + if (!at24->writebuf) {
> > + retval = -ENOMEM;
> > + count = 0;
> > + }
>
> You could move this to before taking the mutex (using a temporary
> pointer), so that you can return with an error immediately if
> allocation fails. But more importantly: do you really need to allocate
> and free a new buffer for each write? You serialize the writes, and the
> size of the buffer doesn't depend on the actual write operation, so you
> might as well allocate the buffer as part of at24_probe(), this will
> make write operations faster and will avoid useless memory
> fragmentation.
>
> If you are really worried about the memory waste in case the user
> doesn't actually write to the EEPROM, you could alternatively allocate
> the buffer on the first write, and free it in the .remove() method.
Now, I allocate it in probe when the EEPROM is selected to be writable.
> > + /* Use I2C operations unless we're stuck with SMBus extensions. */
> > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> > + if (chip->flags & AT24_EE_ADDR2) {
> > + err = -ECOMM;
> > + goto fail;
> > + }
> > + if (!i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_I2C_BLOCK)) {
> > + err = -EBADMSG;
> > + goto fail;
> > + }
>
> These are definitely not the correct error values. I also see no
> reason to return different error values for the two cases as the
> problem is exactly the same: underlying adapter doesn't support the
> transaction types needed to talk to the EEPROM. What about -EOPNOTSUPP?
> Or -ENODEV.
Ah, errno-fun. -EPFNOSUPPORT?
> > + /* use dummy devices for multiple-address chips */
> > + if (chip->i2c_addr_mask) {
> > + for (i = 1; i <= chip->i2c_addr_mask; i++) {
> > + c = i2c_new_dummy(client->adapter, client->addr + i);
> > + if (!c) {
> > + dev_err(&client->dev, "addr %d unavail\n",
>
> Please spell words in error messages completely. Address should be
> printed in hexadecimal, that's what developers and users are used to.
>
> > + client->addr + i);
> > + err = -ENOCSI;
>
> Again a funky error value, completely unrelated with the error that
> occurred. -EBUSY?
-EADDRINUSE?
> > +cleanup:
> > + if (chip->i2c_addr_mask) {
>
> Note that this test is not needed: the for loop below won't do anything
> if i2c_addr_mask == 0.
I thought it would be cleaner to skip the for-loop if it is not needed.
Otherwise it may look like a bug easily. Then again, a comment would
also help.
>
> > + for (i = 1; i <= chip->i2c_addr_mask; i++) {
> > + c = at24->client[i];
> > + if (c)
> > + i2c_unregister_device(c);
> > + * - What write page size does it support?
> > + */
> > +
> > +struct at24_platform_data {
> > + u32 byte_len; /* size (sum of all addr) */
> > + u16 page_size; /* for writes */
> > + u8 i2c_addr_mask; /* for multi-addr chips */
>
> This notion of i2c_addr_mask seems more restrictive and easy to get
> wrong than it needs to be. What you really have is a number of slave
> I2C addresses, that's more intuitive IMHO, and using this would save a
> couple "+ 1" in the code. As a matter of fact, that's what you
> described in the comment above.
>
> Oh, BTW, can't you compute this value yourself from byte_len and (flags
> & AT24_EE_ADDR2)? I think so...
There is at least one exception already (24c00) which covers eight
addresses but actually just needs one. This spoils the calculation of
i2c_addr_mask (and if there is one case, there will be others) :( I
agree, that num_addresses might be more apropriate than i2c_addr_mask.
Kind regards,
Wolfram
--
Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
Pengutronix - Linux Solutions for Science and Industry
signature.asc
Description: Digital signature
_______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
