On Fri, 25 Apr 2008, Jean Delvare wrote:
> > +static void smbus_write_data(u8 *src, u16 *dst, int len)
> > +{
> > + int i, j;
> > +
> > + if (len == 1)
> > + *dst = *src << 8;
> > + else {
> > + j = 0;
> > + for (i = 0; i < len; i += 2) {
> > + *(dst + j) = *(src + i) << 8 | *(src + i + 1);
> > + j++;
> > + }
> > + }
> > +}
for (; len > 1; len -= 2) {
*dst++ = be16_to_cpup((u16*)src);
src += 2;
}
if (len)
*dst = *src << 8;
> > +static void smbus_read_data(u16 *src, u8 *dst, int len)
> > +{
> > + int i, j;
> > +
> > + if (len == 1)
> > + *dst = *src >> 8;
> > + else {
> > + j = 0;
> > + for (i = 0; i < len; i += 2) {
> > + *(dst + i) = *(src + j) >> 8;
> > + *(dst + i + 1) = *(src + j) & 0x00ff;
> > + j++;
> > + }
> > + }
> > +}
>
> If I read the code above correctly, you are merely converting 16-bit
> words from cpu-endian to long-endian and back, so using be16_to_cpu and
> cpu_to_be16 would perform better. If the Highlander is big-endian, you
> should be able to let the compiler optimize out most of the code.
for (; len > 1; len -= 2) {
*(u16*)dst = cpu_to_be16p(src++);
dst += 2;
}
if (len)
*dst = *src >> 8;
> > +static void highlander_i2c_command(struct highlander_i2c_dev *dev, u8
> > command, int len)
> > +{
> > + u16 cmd[32];
> > + int i, j;
> > +
> > + j = 0;
> > + if (len == 1)
> > + cmd[j++] = (command << 8);
> > + else
> > + for (i = 0; i < len; i += 2)
> > + cmd[j++] = (command << 8) | command;
> > +
> > + for (i = 0; i < j; i++) {
> > + iowrite16(cmd[i], dev->base + SMSADR + (i * sizeof(u16)));
> > + dev_dbg(dev->dev, "command data[%x] 0x%04x\n", i, cmd[i]);
> > + }
> > +}
Is there a reason to have a 32 element array for cmd? Each element is the
same.
unsigned int i;
u16 cmd = (command << 8) | command;
for (i = 0; i < len; i += 2) {
if (len - i == 1)
cmd = command << 8;
iowrite16(cmd, dev->base + SMSADR + i);
dev_dbg(dev->dev, "command data[%x] 0x%04x\n", i/2, cmd);
}
These versions will work for odd values of len. If the hardware can't
handle this, except when len == 1, it would probably make more sense to
catch the error sooner and never call them with unsupported values of len.
> > +static int highlander_i2c_wait_for_bbsy(struct highlander_i2c_dev *dev)
> > +{
> > + unsigned long timeout;
> > +
> > + timeout = jiffies + msecs_to_jiffies(iic_timeout);
> > + while (ioread16(dev->base + SMCR) & SMCR_BBSY) {
If there is some delay here (interrupt or kernel preemption for example),
then the timeout could be triggered incorrectly.
> > + if (time_after(jiffies, timeout)) {
> > + dev_warn(dev->dev, "timeout waiting for bus ready\n");
> > + return -ETIMEDOUT;
> > + }
> > +
> > + msleep(1);
> > + }
> > +
> > + return 0;
> > +}
> > + /*
> > + * The R0P7780LC0011RL FPGA on the SH7780-based Highlanders
> > + * needs a significant delay in the read path. SH7785 Highlanders
> > + * don't have this issue, so restrict it entirely to those.
> > + */
> > + if (mach_is_r7780rp() || mach_is_r7780mp())
> > + mdelay(1000);
>
> A one second busy-wait is nasty. Can't you use msleep here instead?
>
> Having to wait for 1 second after each read makes this driver pretty
> unusable on these machines anyway, doesn't it? :(
Does it need to wait 1 sec after each read, or does it really need a 1 sec
delay _between_ reads? If it's the later, you would be much better off
storing the time of the last read, and delaying one second from this time
before starting a new read. If you're only trying to poll a sensor chip
every second, you won't need to busy wait at all this way.
_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c
