Hi Jean,
nearly a month since your reply, I am sorry. Work was shifting in a
different direction meanwhile.
(Everything I did not comment on is already fixed as suggested)
>> +/* Read/Write functions for different register alignments */
>> +
>> +static int i2c_pca_pf_readbyte8(void *pd, int reg)
>> +{
>> + struct i2c_pca_pf_data *i2c = pd;
>> + return ioread8(i2c->reg_base + reg);
>> +}
>> +
>> +static int i2c_pca_pf_readbyte16(void *pd, int reg)
>> +{
>> + struct i2c_pca_pf_data *i2c = pd;
>> + return ioread8(i2c->reg_base + reg * 2);
>
> Shouldn't this be ioread16?
I don't think so. The registers get scattered differently in the
address-room, but their size itself is still 8 bit. I expect that
ioread8 gives me just those 8 bits I want, hiding that some busses
actually do 32-bit accesses only. Am I wrong?
>> + if (i2c->irq) {
>> + ret = wait_event_interruptible(i2c->wait,
>> + i2c->algo_data.read_byte(i2c, I2C_PCA_CON)
>> + & I2C_PCA_CON_SI);
>> + } else {
>> + while ((i2c->algo_data.read_byte(i2c, I2C_PCA_CON)
>> + & I2C_PCA_CON_SI) == 0)
>> + udelay(100);
>
> No timeout?
You got a point there. The thing is that the underlying pca-algorithm
does not have a timeout when expecting this condition (interrupt bit
set). Even when using interrupts instead of polling, it will just sleep
forever, if there goes something _really_ wrong. I could copy at least
this behaviour by replacing the udelay(100); with an msleep(x) with x
being a module parameter. I hope this is good enough, connecting the IRQ
is preferred anyhow. (I am afraid that I don't have the time to add a
sane and tested timeout mechanism to the algorithm)
> The i2c_clock gets to be copied to the driver data structure, but for
> the gpio you have to fetch it from the platform device data? Not very
> consistent.
ACK.
>> + i2c->adap.owner = THIS_MODULE;
> No alignment in code please.
Really? Well, if you say so...
> That's a bit confusing given that the driver isn't named i2c-pca9564.
> Other drivers usually include the address to distinguish between
> multiple device for example (..., "PCA9564 adapter at %04lx",
> res->start).
Okay, will look for something better.
>> +struct i2c_pca9564_pf_platform_data {
> "pf" is redundant with "platform", isn't it?
My intention was that 'i2c_pca9564_pf_' is the prefix for everything
related to this platform-driver, and platform_data happens to be
...well... the platform data :)
>> + int timeout; /* timeout = this value * 10us */
> A rather curious time unit if you ask me.
Given by the algorithm. I didn't question it.
Best wishes,
Wolfram
--
Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
Pengutronix - Linux Solutions for Science and Industry
_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c
