Hi Srinidhi, great work!
Just a few quickies, if you fix these it's
Acked-by: Linus Walleij <[email protected]>
> diff --git a/arch/arm/plat-nomadik/include/plat/i2c.h
> b/arch/arm/plat-nomadik/include/plat/i2c.h
> +enum i2c_freq_mode {
> + I2C_FREQ_MODE_STANDARD, /* up to 100 Kb/s */
> + I2C_FREQ_MODE_FAST, /* up to 400 Kb/s */
> + I2C_FREQ_MODE_FAST_PLUS, /* up to 1 Mb/s */
> + I2C_FREQ_MODE_HIGH_SPEED /* up to 3.4 Mb/s */
> +};
> +
> +enum i2c_addr_mode {
> + I2C_7_BIT_ADDRESS = 0x1,
> + I2C_10_BIT_ADDRESS = 0x2
> +};
Which field in the struct will use this? Can't
I2C_FUNC_10BIT_ADDR and I2C_M_TEN from include/linux/i2c.h (and
siblings) be used for this instead? Leftover?
You have this later:
(...)
+ if (unlikely(msgs[i].flags & I2C_M_TEN)) {
+ dev_err(&dev->pdev->dev, "10 bit addressing"
+ "not supported\n");
+ return -EINVAL;
(...)
Hmmmm?
> +
> +struct nmk_i2c_controller {
Please use kerneldoc for these comments.
(see Documentation/kernel-doc-nano-HOWTO.txt)
> + unsigned long clk_freq;
> + unsigned short slsu; /* slave data set up time */
Comment which unit this is in. (Nanosecs I think?)
> + unsigned char tft; /* Tx FIFO Threshold */
> + unsigned char rft; /* Rx FIFO Threshold */
In bytes?
> + unsigned short sm; /* speed mode */
Shouldn't this last element be
enum i2c_freq_mode sm
then?
> +++ b/drivers/i2c/busses/i2c-nmk.c
(...)
> + dev->irq = platform_get_irq(pdev, 0);
> + ret = request_irq(dev->irq, i2c_irq_handler, IRQF_DISABLED,
> + DRIVER_NAME, dev);
Since I2C IRQs can be a bit tedious, could this be converted
to a request_threaded_irq()? OK I know that is probably a bit
intrusive and can very well be done later (so no blocker) but
think about it.
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
