[i2c] [PATCH 8/12] drivers: PMC MSP71xx TWI driver

[Marc St-Jean]

Jean Delvare wrote:
> Hi Marc,
> 
> On Tue, 26 Jun 2007 11:09:38 -0700, Marc St-Jean wrote:
>  > Jean Delvare wrote:
>  > > On Fri, 27 Apr 2007 14:08:36 -0600, Marc St-Jean wrote:
>  > >  > +/*
>  > >  > + * Helper routine, converts 'pmctwi_cmd' struct to register format
>  > >  > + */
>  > >  > +static inline u32 pmcmsptwi_cmd_to_reg(const struct 
> pmcmsptwi_cmd *cmd)
>  > >  > +{
>  > >  > +     return (u32)(((cmd->type & 0x3) << 8) |
>  > >  > +                     (((cmd->write_len - 1) & 0x7) << 4) |
>  > >  > +                     (((cmd->read_len - 1) & 0x7) << 0));
>  > >  > +}
>  > >
>  > > What if cmd->write_len or cmd->read_len is 0?
>  >
>  > That's checked in pmcmsptwi_xfer_cmd() before calling 
> pmcmsptwi_cmd_to_reg().
> 
> Not really. The code in pmcmsptwi_xfer_cmd() doesn't check
> cmd->read_len if cmd->type == MSP_TWI_CMD_WRITE, nor cmd->write_len if
> cmd->type == MSP_TWI_CMD_READ. So one of them could still be 0 when you
> call pmcmsptwi_cmd_to_reg() I _guess_ that the resulting bits in the
> cmd register then don't matter, but you may want to double-check.

The HW ignores the read count unless the command is to read and vice-versa
writes. In fact it has to since the register holds the count - 1, so
0 really means 1 byte.

>  > > I seem to understand that the hardware simply doesn't support
>  > > transactions with an arbitrary number of messages. It only supports
>  > > simple reads, simple writes, and combined write + read. Then your
>  > > driver shouldn't attempt to hide this limitation. The hardware only
>  > > supports a subset of the I2C protocol, so be it. Simply return an 
> error
>  > > if requested to do something the hardware doesn't support.
>  > >
>  > > This will make your code much more simple. And in practice it 
> shouldn't
>  > > change anything, because all popular I2C (and SMBus) transactions are
>  > > of one of the 3 supported types.
>  >
>  > I'm concerned about dropping multi-message support and reducing
>  > functionality. We can't be certain that this is not currently used
>  > by devices on our customers' boards. They have been using this driver
>  > on MSP devices for quite a while now.
>  > The debugging output for "SMBus read" (see 4 comments down) hints that
>  > it was used in at least one case.
> 
> I'm not asking that you drop multi-message support entirely. The chip
> supports combined write+read transactions up to 8 bytes per message, so
> the driver should still support that. I'm only asking that you don't
> try to emulate support for transactions of 3 and more messages, as this
> is not correct.

OK, I will drop the emulation.

> Again, I've _never_ seen any I2C chip requiring transactions of more
> than 2 messages, so I'm fairly certain that this won't make a
> difference in practice. What your device supports is sufficient for all
> the SMBus transaction types (except for the limit to 8 bytes per
> message, but there's no way around this), including the one for which
> a debugging message was present. In particular, all the transactions
> used in the LED driver you posted earlier would be supported just fine.
> 
>  > >  > +             if (dual) {
>  > >  > +                     dev_dbg(&adap->dev,
>  > >  > +                             "SMBus read 0x%02x from reg 
> 0x%02x\n",
>  > >  > +                             nmsg->buf[0], cmsg->buf[0]);
>  > >
>  > > This message is only correct for an SMBus Read Byte transaction, while
>  > > there are many other possible combined transactions.
>  >
>  > OK, I've dropped the comment.
> 
> FWIW: enabling CONFIG_I2C_DEBUG_CORE will show all the transactions, if
> you ever need this kind of information.

Thanks, I see several other flags there which I'm also using, see below.

>  > >  > +static int __init pmcmsptwi_init(void)
>  > >  > +{
>  > >  > +     pmcmsptwi_algo_data.iobase = ioremap(MSP_TWI_BASE, 
> MSP_TWI_REG_SIZE);
>  > >
>  > > MSP_TWI_BASE is not defined anywhere.
>  >
>  > It is defined in msp_regs.h included at the top. That file is part of
>  > PATCH 1/12 for the core platform.
> 
> Ah, OK. Sorry, I forgot that this was a series and not a single patch.

No matter since I no longer need to include the file with the platform
driver support.

I have a question related to this, after converting to a platform driver
our chips driver will no longer attempts to find it's devices. The bus
driver first loads successfully. When the chip driver loads, it's
attach_adapter function calls i2c_probe which returns no error. However
i2c_probe never calls the detect function pointer that's passed to it!

There is no debugging output with all flags on. I've been looking at
other bus and chip drivers but see nothing obvious. I'm also working
through the list archive, too bad it's not searchable.

Anyone have an idea if anything new is required once the bus driver
is a platform driver?
Must a custom addr_data structure be passed in to i2c_probe?
Must the .id field of the chip driver be specified?

Thanks,
Marc