Re: [Linaro-acpi] [PATCH v6 1/2] Mailbox: Add support for Platform Communication Channel

Hi Lv,

On 4 September 2014 21:56, Zheng, Lv lv.zheng@intel.com wrote:

...

• spin_lock_irqsave(&chan->lock, flags);
  

• chan->msg_free = 0;
  

• chan->msg_count = 0;
  

• chan->active_req = NULL;
  

• chan->cl = cl;
  

• init_completion(&chan->tx_complete);
  

• if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
  

The tab should be a space here?

Not sure where you're seeing the tab.

...

+static bool pcc_tx_done(struct mbox_chan *chan) +{

• struct acpi_pcct_subspace *pcct_ss = chan->con_priv;
  

• struct acpi_pcct_shared_memory *generic_comm_base =
  

•         (struct acpi_pcct_shared_memory *) pcct_ss->base_address;
  

• u16 cmd_delay = pcct_ss->min_turnaround_time;
  

Should these parameters be acquired with chan-lock held? Is that possible to change them during runtime?

No. These parameters don't change.

...

+/* Channel lock is already held by mbox controller code. */ +static int pcc_send_data(struct mbox_chan *chan, void *data) +{

• struct acpi_pcct_subspace *pcct_ss = chan->con_priv;
  

• struct acpi_pcct_shared_memory *generic_comm_base =
  

•         (struct acpi_pcct_shared_memory *) pcct_ss->base_address;
  

Ditto.

Same as above.

...

+out_err:

• return ACPI_SUCCESS(status) ? 1 : 0;
  

Should the return value of 1 here be -EINVAL?

I dont think it really matters, the caller returns -EINVAL anyway.

Thanks, Ashwin