Hi Jean-Jacques,
I have just found these emails in may draft box, and I thought I have
already sent them. However I didn't find anything relevant about the
driver itslef, but only few nitpick. So either your change are goods,
or I missed the important points. The final judgment belongs ton
Wolfram :)
On 22/11/2013 09:58, jean-jacques hiblot wrote:
> Clean-up properly when a transfer fails for whatever reason.
> Cancel the transfer when the process is signaled.
>
> Signed-off-by: jean-jacques hiblot <jjhiblot@xxxxxxxxx>
> ---
> drivers/i2c/busses/i2c-ibm_iic.c | 144 +++++++++------------------------------
> drivers/i2c/busses/i2c-ibm_iic.h | 2 +-
> 2 files changed, 35 insertions(+), 111 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c
> index 2bb55b3..a3f3f1b 100644
> --- a/drivers/i2c/busses/i2c-ibm_iic.c
> +++ b/drivers/i2c/busses/i2c-ibm_iic.c
> @@ -334,119 +334,25 @@ static irqreturn_t iic_handler(int irq, void *dev_id)
> }
>
> /*
> - * Get master transfer result and clear errors if any.
> - * Returns the number of actually transferred bytes or error (<0)
> - */
> -static int iic_xfer_result(struct ibm_iic_private* dev)
> -{
> - struct iic_regs __iomem *iic = dev->vaddr;
> -
> - if (unlikely(in_8(&iic->sts) & STS_ERR)){
> - DBG(dev, "xfer error, EXTSTS = 0x%02x\n",
> - in_8(&iic->extsts));
> -
> - /* Clear errors and possible pending IRQs */
> - out_8(&iic->extsts, EXTSTS_IRQP | EXTSTS_IRQD |
> - EXTSTS_LA | EXTSTS_ICT | EXTSTS_XFRA);
> -
> - /* Flush master data buffer */
> - out_8(&iic->mdcntl, in_8(&iic->mdcntl) | MDCNTL_FMDB);
> -
> - /* Is bus free?
> - * If error happened during combined xfer
> - * IIC interface is usually stuck in some strange
> - * state, the only way out - soft reset.
> - */
> - if ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE){
> - DBG(dev, "bus is stuck, resetting\n");
> - iic_dev_reset(dev);
> - }
> - return -EREMOTEIO;
> - }
> - else
> - return in_8(&iic->xfrcnt) & XFRCNT_MTC_MASK;
> -}
> -
> -/*
> * Try to abort active transfer.
> */
> -static void iic_abort_xfer(struct ibm_iic_private* dev)
> +static void iic_abort_xfer(struct ibm_iic_private *dev)
> {
> - struct iic_regs __iomem *iic = dev->vaddr;
> - unsigned long x;
> -
> - DBG(dev, "iic_abort_xfer\n");
> + struct device *device = dev->adap.dev.parent;
> + unsigned long end;
>
> - out_8(&iic->cntl, CNTL_HMT);
> + DBG(dev, "aborting transfer\n");
> + /* transfer should be aborted within 10ms */
> + end = jiffies + 10;
> + dev->abort = 1;
>
> - /*
> - * Wait for the abort command to complete.
> - * It's not worth to be optimized, just poll (timeout >= 1 tick)
> - */
> - x = jiffies + 2;
> - while ((in_8(&iic->extsts) & EXTSTS_BCS_MASK) != EXTSTS_BCS_FREE){
> - if (time_after(jiffies, x)){
> - DBG(dev, "abort timeout, resetting...\n");
> - iic_dev_reset(dev);
> - return;
> - }
> + while (time_after(end, jiffies) && !dev->transfer_complete)
> schedule();
> - }
>
> - /* Just to clear errors */
> - iic_xfer_result(dev);
> -}
> -
> -/*
> - * Wait for master transfer to complete.
> - * It puts current process to sleep until we get interrupt or timeout expires.
> - * Returns the number of transferred bytes or error (<0)
> - */
> -static int iic_wait_for_tc(struct ibm_iic_private* dev){
> -
> - struct iic_regs __iomem *iic = dev->vaddr;
> - int ret = 0;
> -
> - if (dev->irq >= 0){
> - /* Interrupt mode */
> - ret = wait_event_interruptible_timeout(dev->wq,
> - !(in_8(&iic->sts) & STS_PT), dev->adap.timeout);
> -
> - if (unlikely(ret < 0))
> - DBG(dev, "wait interrupted\n");
> - else if (unlikely(in_8(&iic->sts) & STS_PT)){
> - DBG(dev, "wait timeout\n");
> - ret = -ETIMEDOUT;
> - }
> - }
> - else {
> - /* Polling mode */
> - unsigned long x = jiffies + dev->adap.timeout;
> -
> - while (in_8(&iic->sts) & STS_PT){
> - if (unlikely(time_after(jiffies, x))){
> - DBG(dev, "poll timeout\n");
> - ret = -ETIMEDOUT;
> - break;
> - }
> -
> - if (unlikely(signal_pending(current))){
> - DBG(dev, "poll interrupted\n");
> - ret = -ERESTARTSYS;
> - break;
> - }
> - schedule();
> - }
> + if (!dev->transfer_complete) {
> + dev_err(device, "abort operation failed\n");
What about using the ERR macro you introduce in the previous patch?
> + iic_dev_reset(dev);
> }
> -
> - if (unlikely(ret < 0))
> - iic_abort_xfer(dev);
> - else
> - ret = iic_xfer_result(dev);
> -
> - DBG2(dev, "iic_wait_for_tc -> %d\n", ret);
> -
> - return ret;
> }
>
> /*
> @@ -470,6 +376,13 @@ static int iic_xfer_bytes(struct ibm_iic_private *dev)
> EXTSTS_ICT | EXTSTS_XFRA);
> out_8(&iic->sts, STS_IRQA | STS_SCMP);
>
> + if (dev->status == -ECANCELED) {
> + DBG(dev, "abort completed\n");
> + dev->transfer_complete = 1;
> + complete(&dev->iic_compl);
> + return dev->status;
> + }
> +
> if ((status & STS_ERR) ||
> (ext_status & (EXTSTS_LA | EXTSTS_ICT | EXTSTS_XFRA))) {
> DBG(dev, "status 0x%x\n", status);
> @@ -571,7 +484,14 @@ static int iic_xfer_bytes(struct ibm_iic_private *dev)
> /* actually start the transfer of the current data chunk */
> out_8(&iic->cntl, cntl | CNTL_PT);
>
> - return 0;
> + /* The transfer must be aborted. */
> + if (dev->abort) {
> + DBG(dev, "aborting\n");
> + out_8(&iic->cntl, CNTL_HMT);
> + dev->status = -ECANCELED;
> + }
> +
> + return dev->status;
> }
>
> /*
> @@ -673,6 +593,7 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> dev->current_byte = dev->current_msg = dev->current_byte_rx = 0;
> dev->transfer_complete = 0;
> dev->status = 0;
> + dev->abort = 0;
> dev->msgs = msgs;
> dev->num_msgs = num;
>
> @@ -710,8 +631,9 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> /* wait for the transfer to complete */
> ret = wait_for_completion_interruptible_timeout(
> &dev->iic_compl, num * HZ);
> - /* mask the interrupts */
> - out_8(&iic->intmsk, 0);
> + /* we don't mask the interrupts here because we may
> + * need them to abort the transfer gracefully
> + */
nitpick:wrong multiline comment style
> }
>
> if (ret == 0) {
> @@ -720,11 +642,15 @@ static int iic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> } else if (ret < 0) {
> if (ret == -ERESTARTSYS)
> ERR(dev, "transfer interrupted\n");
> + iic_abort_xfer(dev);
> } else {
> /* Transfer is complete */
> ret = (dev->status) ? dev->status : num;
> }
>
> + /* mask the interrupts */
> + out_8(&iic->intmsk, 0);
> +
> return ret;
> }
>
> @@ -821,8 +747,6 @@ static int iic_probe(struct platform_device *ofdev)
> goto error_cleanup;
> }
>
> - init_waitqueue_head(&dev->wq);
> -
> dev->irq = iic_request_irq(ofdev, dev);
> if (!dev->irq)
> dev_info(&ofdev->dev, "using polling mode\n");
> diff --git a/drivers/i2c/busses/i2c-ibm_iic.h b/drivers/i2c/busses/i2c-ibm_iic.h
> index 76c476a..0ee28a9 100644
> --- a/drivers/i2c/busses/i2c-ibm_iic.h
> +++ b/drivers/i2c/busses/i2c-ibm_iic.h
> @@ -47,7 +47,6 @@ struct iic_regs {
> struct ibm_iic_private {
> struct i2c_adapter adap;
> struct iic_regs __iomem *vaddr;
> - wait_queue_head_t wq;
> int irq;
> int fast_mode;
> u8 clckdiv;
> @@ -58,6 +57,7 @@ struct ibm_iic_private {
> int current_byte_rx;
> int transfer_complete;
> int status;
> + int abort;
> struct completion iic_compl;
> };
>
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
