On Tue, Mar 25, 2008 at 3:46 AM, Jean Delvare <[EMAIL PROTECTED]> wrote:
> Hi Bryan,
>
>
>
> On Fri, 14 Mar 2008 00:22:37 -0700, Bryan Wu wrote:
> > From: Bryan Wu <[EMAIL PROTECTED]>
> >
> > 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]>
> > Signed-off-by: Bryan Wu <[EMAIL PROTECTED]>
> > ---
> > 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;
> > }
>
>
You are right, I will update this patch. It will be more simple and better.
Thanks
-Bryan
_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c
