Re: [PATCH 3/6] Blackfin I2C/TWI driver: add missing pin mux operation

[Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>]

Hi Bryan,

On Fri, 14 Mar 2008 00:22:37 -0700, Bryan Wu wrote:
> From: Bryan Wu <bryan.wu-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> 
> 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 <bryan.wu-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Bryan Wu <cooloney-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-bfin-twi.c |   29 +++++++++++++++++++++++++++++
>  1 files changed, 29 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
> index 515fe08..674c053 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,23 @@ DEFINE_TWI_REG(XMT_DATA16, 0x84)
>  DEFINE_TWI_REG(RCV_DATA8, 0x88)
>  DEFINE_TWI_REG(RCV_DATA16, 0x8C)
>  
> +static u16 pin_req[2][3] = {
> +	{P_TWI0_SCL, P_TWI0_SDA, 0},
> +	{P_TWI1_SCL, P_TWI1_SDA, 0},
> +};

Could be const.

> +
> +static int setup_pin_mux(struct bfin_twi_iface *iface, int action)
> +{
> +	int rc = 0;
> +
> +	if (action)
> +		rc = peripheral_request_list(pin_req[iface->bus_num], "i2c-bfin-twi");
> +	else
> +		peripheral_free_list(pin_req[iface->bus_num]);
> +
> +	return rc;
> +}

It didn't strike me the first time I reviewed this patch, but this
function doesn't make much sense. Calling peripheral_request_list() and
peripheral_free_list() directly would make the code more simple. But
well, that's your driver, so that's your call. I can only hope that gcc
properly optimizes away the additional costs.

> +
>  static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
>  {
>  	unsigned short twi_int_status = read_INT_STAT(iface);
> @@ -640,6 +659,7 @@ static int i2c_bfin_twi_probe(struct platform_device *pdev)
>  		goto out_error_no_irq;
>  	}
>  
> +	iface->bus_num = pdev->id;

I fail to see why you duplicate this value... why don't you use
pdev->id directly?

>  	init_timer(&(iface->timeout_timer));
>  	iface->timeout_timer.function = bfin_twi_timeout;
>  	iface->timeout_timer.data = (unsigned long)iface;
> @@ -653,6 +673,12 @@ static int i2c_bfin_twi_probe(struct platform_device *pdev)
>  	p_adap->class = I2C_CLASS_ALL;
>  	p_adap->dev.parent = &pdev->dev;
>  
> +	rc = setup_pin_mux(iface, 1);
> +	if (rc) {
> +		dev_err(&pdev->dev, "Can't setup Pin Mux!\n");

s/Pin Mux/pin mux/

> +		goto out_error_pin_mux;
> +	}
> +
>  	rc = request_irq(iface->irq, bfin_twi_interrupt_entry,
>  		IRQF_DISABLED, pdev->name, iface);
>  	if (rc) {
> @@ -690,6 +716,8 @@ out_error_add_adapter:
>  	free_irq(iface->irq, iface);
>  out_error_req_irq:
>  out_error_no_irq:
> +	setup_pin_mux(iface, 0);
> +out_error_pin_mux:
>  	iounmap(iface->regs_base);
>  out_error_ioremap:
>  out_error_get_res:
> @@ -706,6 +734,7 @@ static int i2c_bfin_twi_remove(struct platform_device *pdev)
>  
>  	i2c_del_adapter(&(iface->adap));
>  	free_irq(iface->irq, iface);
> +	setup_pin_mux(iface, 0);
>  
>  	return 0;
>  }


-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c