From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Bryan Wu" Subject: Re: [PATCH 3/6] Blackfin I2C/TWI driver: add missing pin mux operation Date: Tue, 25 Mar 2008 11:33:40 +0800 Message-ID: <386072610803242033r2701fddu62ec22976782617a@mail.gmail.com> References: <1205479360-25240-4-git-send-email-cooloney@kernel.org> <20080324204634.69cc87e6@hyperion.delvare> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20080324204634.69cc87e6-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> Content-Disposition: inline List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: i2c-bounces-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org Errors-To: i2c-bounces-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org To: Jean Delvare Cc: i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org List-Id: linux-i2c@vger.kernel.org On Tue, Mar 25, 2008 at 3:46 AM, Jean Delvare wrote: > Hi Bryan, > > > > On Fri, 14 Mar 2008 00:22:37 -0700, Bryan Wu wrote: > > From: Bryan Wu > > > > 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 > > Signed-off-by: Bryan Wu > > --- > > 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 > > > > #include > > +#include > > #include > > > > #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 i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c