On Wed 18 Oct 09:39 PDT 2017, Srinivas Kandagatla wrote:
Thanks for Review Comments,
On 18/10/17 07:15, Bjorn Andersson wrote:
On Fri 06 Oct 08:51 PDT 2017, srinivas.kandagatla@linaro.org wrote:
[..]
- if (!async) {
- txn->msg->comp_cb = NULL;
- txn->msg->ctx = NULL;
I believe txn->msg is always required, so you don't need to do this contidionally.
I don't get this, why do you want to set comp_cb to NULL unconditionally?
I'm just not happy about the complexity of this function, but perhaps it's confusing to always set them, regardless of them being used. Feel free to keep it.
[..]
+void slim_return_tx(struct slim_controller *ctrl, int err) +{
- unsigned long flags;
- int idx;
- struct slim_pending cur;
- spin_lock_irqsave(&ctrl->tx.lock, flags);
- idx = ctrl->tx.head;
- ctrl->tx.head = (ctrl->tx.head + 1) % ctrl->tx.n;
- cur = ctrl->pending_wr[idx];
Why is this doing struct copy?
Not sure, do you see any issue with this?
It's a rarely used feature and I don't see a reason for using it here.
It's probably better to make a copy of cur.cb and cur.ctx to make their use after the spin-unlock more obvious (but should be fine as the spinlock is for the pending_wr array.
- spin_unlock_irqrestore(&ctrl->tx.lock, flags);
- if (!cur.cb)
- dev_err(&ctrl->dev, "NULL Transaction or completion");
- else
- cur.cb(cur.ctx, err);
- up(&ctrl->tx_sem);
+} +EXPORT_SYMBOL_GPL(slim_return_tx);
[..]
+/**
- struct slim_ctrl_buf: circular buffer used by contoller for TX, RX
- @base: virtual base address for this buffer
- @phy: physical address for this buffer (this is useful if controller can
- DMA the buffers for TX and RX to/from controller hardware
- @lock: lock protecting head and tail
- @head: index where buffer is returned back
- @tail: index from where buffer is consumed
- @sl_sz: byte-size of each slot in this buffer
- @n: number of elements in this circular ring, note that this needs to be
- 1 more than actual buffers to allow for one open slot
- */
Is this ringbuffer mechanism defined in the slimbus specification? Looks like something specific to the Qualcomm controller, rather than something that should be enforced in the framework.
Yes, this is not part of the slimbus specs, but Qcom SOCs have concept of Message Queues.
Are you suggesting that this buffer handling has to be moved out of core into controller driver?
The fact that this seems to describe a physical ring buffer, with some set of properties that are related to how a ring buffer works in the Qualcomm hardware and it carries a notion of physical mapping, all indicates to me that this describes some Qualcomm hardware interface.
I believe this is a hardware implementation detail that should reside in the hardware part of the implementation (i.e. the Qualcomm driver).
+struct slim_ctrl_buf {
- void *base;
- phys_addr_t phy;
- spinlock_t lock;
- int head;
- int tail;
- int sl_sz;
- int n;
+};
Regards, Bjorn