Re: [PATCHv2] w1: omap-hdq: Simplify driver with PM runtime autosuspend

[Adam Ford]

On Mon, Dec 16, 2019 at 10:43 AM Tony Lindgren <tony@xxxxxxxxxxx> wrote:
>
> * Tony Lindgren <tony@xxxxxxxxxxx> [191216 14:54]:
> > Vignesh, any comments on the ti,mode = "1w" and removal of the call to
> > hdq_disable_interrupt()? Is there some specific section where we need
> > to have interrupts disabled and then re-enabled?
>
> OK I got "1w" mode working too now. We need to clear the irqstatus before
> calling wait_event_timeout() on it, and we're now missing it in the
> hdq_read_byte().
>
> Looks like we should not tinker with the actual irstatus register though,
> that's up to the hdq_isr() to manage.
>
> So the following helper is probably what we want to do additionally.
> I'll be posting v3 of the $subject patch.

I manually attempted to apply the patches.  I 'think' I did it right.

For testing, I ran:
  watch cat /sys/devices/w1_bus_master1/01-000000000000/power_supply/bq27000-battery/voltage_now

I inserted and removed the power cable several times and observed the
voltage readings with the battery connected.
The numbers looked reasonable.

I then put the board to suspend, waited a few seconds and woke the
board from sleep.  I resumed my 'watch' function from above and it
worked just fine.
If there is nothing else you want me to test, go ahead and add:

Tested-by: Adam Ford <aford173@xxxxxxxxx> #logicpd-torpedo-37xx-devkit

adam
>
> Regards,
>
> Tony
>
> 8< ----------------------
> diff --git a/drivers/w1/masters/omap_hdq.c b/drivers/w1/masters/omap_hdq.c
> --- a/drivers/w1/masters/omap_hdq.c
> +++ b/drivers/w1/masters/omap_hdq.c
> @@ -119,22 +119,24 @@ static int hdq_wait_for_flag(struct hdq_data *hdq_data, u32 offset,
>         return ret;
>  }
>
> +/* Clear saved irqstatus after using an interrupt */
> +static void hdq_reset_irqstatus(struct hdq_data *hdq_data)
> +{
> +       unsigned long irqflags;
> +
> +       spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
> +       hdq_data->hdq_irqstatus = 0;
> +       spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
> +}
> +
>  /* write out a byte and fill *status with HDQ_INT_STATUS */
>  static int hdq_write_byte(struct hdq_data *hdq_data, u8 val, u8 *status)
>  {
>         int ret;
>         u8 tmp_status;
> -       unsigned long irqflags;
>
>         *status = 0;
>
> -       spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
> -       /* clear interrupt flags via a dummy read */
> -       hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
> -       /* ISR loads it with new INT_STATUS */
> -       hdq_data->hdq_irqstatus = 0;
> -       spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
> -
>         hdq_reg_out(hdq_data, OMAP_HDQ_TX_DATA, val);
>
>         /* set the GO bit */
> @@ -168,6 +170,7 @@ static int hdq_write_byte(struct hdq_data *hdq_data, u8 val, u8 *status)
>         }
>
>  out:
> +       hdq_reset_irqstatus(hdq_data);
>         return ret;
>  }
>
> @@ -219,7 +222,6 @@ static int omap_hdq_break(struct hdq_data *hdq_data)
>  {
>         int ret = 0;
>         u8 tmp_status;
> -       unsigned long irqflags;
>
>         ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
>         if (ret < 0) {
> @@ -228,13 +230,6 @@ static int omap_hdq_break(struct hdq_data *hdq_data)
>                 goto rtn;
>         }
>
> -       spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
> -       /* clear interrupt flags via a dummy read */
> -       hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
> -       /* ISR loads it with new INT_STATUS */
> -       hdq_data->hdq_irqstatus = 0;
> -       spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
> -
>         /* set the INIT and GO bit */
>         hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS,
>                 OMAP_HDQ_CTRL_STATUS_INITIALIZATION | OMAP_HDQ_CTRL_STATUS_GO,
> @@ -283,6 +278,7 @@ static int omap_hdq_break(struct hdq_data *hdq_data)
>                         " return to zero, %x", tmp_status);
>
>  out:
> +       hdq_reset_irqstatus(hdq_data);
>         mutex_unlock(&hdq_data->hdq_mutex);
>  rtn:
>         return ret;
> @@ -330,6 +326,7 @@ static int hdq_read_byte(struct hdq_data *hdq_data, u8 *val)
>         /* the data is ready. Read it in! */
>         *val = hdq_reg_in(hdq_data, OMAP_HDQ_RX_DATA);
>  out:
> +       hdq_reset_irqstatus(hdq_data);
>         mutex_unlock(&hdq_data->hdq_mutex);
>  rtn:
>         return ret;
> @@ -363,7 +360,6 @@ static u8 omap_w1_triplet(void *_hdq, u8 bdir)
>                 goto rtn;
>         }
>
> -       hdq_data->hdq_irqstatus = 0;
>         /* read id_bit */
>         hdq_reg_merge(_hdq, OMAP_HDQ_CTRL_STATUS,
>                       ctrl | OMAP_HDQ_CTRL_STATUS_DIR, mask);
> @@ -377,7 +373,9 @@ static u8 omap_w1_triplet(void *_hdq, u8 bdir)
>         }
>         id_bit = (hdq_reg_in(_hdq, OMAP_HDQ_RX_DATA) & 0x01);
>
> -       hdq_data->hdq_irqstatus = 0;
> +       /* Must clear irqstatus for another RXCOMPLETE interrupt */
> +       hdq_reset_irqstatus(hdq_data);
> +
>         /* read comp_bit */
>         hdq_reg_merge(_hdq, OMAP_HDQ_CTRL_STATUS,
>                       ctrl | OMAP_HDQ_CTRL_STATUS_DIR, mask);
> @@ -420,6 +418,7 @@ static u8 omap_w1_triplet(void *_hdq, u8 bdir)
>                       OMAP_HDQ_CTRL_STATUS_SINGLE);
>
>  out:
> +       hdq_reset_irqstatus(hdq_data);
>         mutex_unlock(&hdq_data->hdq_mutex);
>  rtn:
>         pm_runtime_mark_last_busy(hdq_data->dev);
> @@ -460,7 +459,7 @@ static u8 omap_w1_read_byte(void *_hdq)
>         if (ret < 0) {
>                 pm_runtime_put_noidle(hdq_data->dev);
>
> -               return ret;
> +               return -1;
>         }
>
>         ret = hdq_read_byte(hdq_data, &val);
> --
> 2.24.1