On Mon, Sep 22, 2014 at 11:59 AM, Wolfram Sang <wsa@xxxxxxxxxxxxx> wrote:
>
>> >> + if (idev->msg_xfrd == 0 && i2c_m_recv_len(msg)) {
>> >> + /*
>> >> + * Check length byte for SMBus block read
>> >> + */
>> >> + if (c <= 0) {
>> >> + idev->msg_err = -EPROTO;
>> >> + i2c_int_disable(idev, ~0);
>> >> + complete(&idev->msg_complete);
>> >> + break;
>> >> + } else if (c > I2C_SMBUS_BLOCK_MAX) {
>> >> + c = I2C_SMBUS_BLOCK_MAX;
>> >
>> > What about returning -EPROTO here as well? I don't think that reading
>> > just a slice of all the data is helpful.
>> >
>>
>> Right, that is probably true. This came from the device I was using to
>> test the block-read operation. This device violated the
>> I2C_SMBUS_BLOCK_MAX limit, so I added this truncation to at least get
>> something out...
>
> Which device was it? I know there are some doing that, yet I like to
> know which ones.
It was a LTC2974, register MFR_FAULT_LOG (0xee).
>
>> >> +{
>> >> + struct axxia_i2c_dev *idev = _dev;
>> >> + u32 status;
>> >> +
>> >> + if (!idev->msg)
>> >> + return IRQ_NONE;
>> >
>> > This is actually not true. There might be interrupt bits set, so there
>> > is an IRQ. There shouldn't be one, right, but that's another case IMO.
>> >
>>
>> You could see it as: there is no interrupt that this handler is
>> interested in serving. I'd like to keep this test as there is some
>> legacy software that could be run on platforms with this driver that
>> access the I2C controller directly. If this happens, this test assures
>> that the user get an informative "unhandled irq" error message
>> (instead of a null pointer dereference).
>
> IRQ_NONE is "this interrupt wasn't by me" so for shared IRQs, the next
> handler can check. You shouldn't remove the check per se. Still, since
> this interrupt was definately from the I2C core, you should return
> IRQ_HANDLED and print an error message to the logs.
>
Ok, I'll do that.
>> >> +static int
>> >> +axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
>> >> +{
>> >> + u32 int_mask = MST_STATUS_ERR | MST_STATUS_SNS;
>> >> + u32 rx_xfer, tx_xfer;
>> >> + u32 addr_1, addr_2;
>> >> + int ret;
>> >> +
>> >> + if (msg->len == 0 || msg->len > 255)
>> >> + return -EINVAL;
>> >
>> > Ouch, really? Maybe we should warn the user here.
>>
>> Yeah, the transfer length register limits the length to 255. I'll add
>> a warning here.
>
> Please also add this information to the Kconfig description and
> somewhere at the top of the source file. This is an important flaw which
> people should easily find out about.
>
Ok.
Thanks,
Anders
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
