Hi Bryan,
On Fri, 2 Nov 2007 14:24:23 +0800, Bryan Wu wrote:
> Blackfin TWI controller hardware pin should be requested from GPIO port
> controller
> Before BF54x, there is no need to do this. But as long as BF54x and BF52x
> are supported by this generic driver, the missing pin mux operation should be
> added.
>
> Signed-off-by: Bryan Wu <[EMAIL PROTECTED]>
> ---
> drivers/i2c/busses/i2c-bfin-twi.c | 24 ++++++++++++++++++++++++
> 1 files changed, 24 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c
> b/drivers/i2c/busses/i2c-bfin-twi.c
> index e495454..727625b 100644
> --- a/drivers/i2c/busses/i2c-bfin-twi.c
> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
> @@ -34,6 +34,7 @@
> #include <linux/platform_device.h>
>
> #include <asm/blackfin.h>
> +#include <asm/portmux.h>
> #include <asm/irq.h>
>
> #define POLL_TIMEOUT (2 * HZ)
> @@ -63,6 +64,7 @@ struct bfin_twi_iface {
> int msg_num;
> int cur_msg;
> void __iomem *regs_base;
> + int bus_num;
> };
>
>
> @@ -89,6 +91,24 @@ DEFINE_TWI_REG(XMT_DATA16, 0x84)
> DEFINE_TWI_REG(RCV_DATA8, 0x88)
> DEFINE_TWI_REG(RCV_DATA16, 0x8C)
>
> +static int setup_pin_mux(int action, struct bfin_twi_iface *iface)
I suggest swapping the order of the parameters - it makes more sense to
pass the object first and the action second than the other way around
IMHO.
Also, all other functions in this driver are named bfin_twi_<something>
so it would be nicer to do the same here.
> +{
> +
> + u16 pin_req[2][3] = {
> + {P_TWI0_SCL, P_TWI0_SDA, 0},
> + {P_TWI1_SCL, P_TWI1_SDA, 0},
> + };
See my previous review: this array should be const and possibly static.
> +
> + if (action) {
> + if (peripheral_request_list(pin_req[iface->bus_num], DRV_NAME))
> + return -EFAULT;
Mike Frysinger suggested that you should return the error code from
peripheral_request_list() and I agree with him.
> + } else {
> + peripheral_free_list(pin_req[iface->bus_num]);
> + }
> +
> + return 0;
> +}
> +
> static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
> {
> unsigned short twi_int_status = read_INT_STAT(iface);
> @@ -640,6 +660,7 @@ static int i2c_bfin_twi_probe(struct platform_device
> *pdev)
> goto out_error_no_irq;
> }
>
> + iface->bus_num = pdev->id;
> init_timer(&(iface->timeout_timer));
> iface->timeout_timer.function = bfin_twi_timeout;
> iface->timeout_timer.data = (unsigned long)iface;
> @@ -652,6 +673,8 @@ static int i2c_bfin_twi_probe(struct platform_device
> *pdev)
> p_adap->class = I2C_CLASS_ALL;
> p_adap->dev.parent = &pdev->dev;
>
> + setup_pin_mux(1, iface);
You must check the error code and handle it.
> +
> rc = request_irq(iface->irq, bfin_twi_interrupt_entry,
> IRQF_DISABLED, pdev->name, iface);
> if (rc) {
> @@ -701,6 +724,7 @@ static int i2c_bfin_twi_remove(struct platform_device
> *pdev)
>
> i2c_del_adapter(&(iface->adap));
> free_irq(iface->irq, iface);
> + setup_pin_mux(0, iface);
If you do this here, then you should also do it in the error path in
i2c_bfin_twi_probe().
>
> return 0;
> }
--
Jean Delvare
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
