On 11/10/17 05:38, Vinod Koul wrote:
On Fri, Oct 06, 2017 at 05:51:31PM +0200, srinivas.kandagatla@xxxxxxxxxx wrote:
mutex_init(&ctrl->m_ctrl);
+ spin_lock_init(&ctrl->tx.lock);
+ spin_lock_init(&ctrl->rx.lock);
locks galore :) My assumption is that you want to optimize these? But given
that audio user is going to be serialized do we practically need two locks?
If we remove the locking, It will be issue if we have multiple devices
in a component, which is common atleast with the codec which am looking at.
+
+ ctrl->pending_wr = kcalloc((ctrl->tx.n - 1),
+ sizeof(struct slim_pending),
+ GFP_KERNEL);
+ if (!ctrl->pending_wr) {
+ ret = -ENOMEM;
+ goto wr_alloc_failed;
+ }
+
+ sema_init(&ctrl->tx_sem, (ctrl->tx.n - 1));
i though v5 comment from Arnd was not to use semaphores..
I will revisit this area once again and get rid of this semaphore all
together in next version.
+/* Copyright (c) 2011-2016, The Linux Foundation. All rights reserved.
2017?
Yep.
+int slim_processtxn(struct slim_controller *ctrl,
slim_process_txn seems more readable to me
I can change it in next version.
+ struct slim_msg_txn *txn)
+{
+ int ret, i = 0;
+ unsigned long flags;
+ u8 *buf;
+ bool async = false;
+ struct slim_cb_data cbd;
+ DECLARE_COMPLETION_ONSTACK(done);
+ bool need_tid = slim_tid_txn(txn->mt, txn->mc);
+
+ if (!txn->msg->comp_cb) {
+ txn->msg->comp_cb = slim_sync_default_cb;
+ cbd.comp = &done;
+ txn->msg->ctx = &cbd;
+ } else {
+ async = true;
+ }
+
+ buf = slim_get_tx(ctrl, txn, need_tid);
+ if (!buf)
+ return -ENOMEM;
+
+ if (need_tid) {
+ spin_lock_irqsave(&ctrl->txn_lock, flags);
+ for (i = 0; i < ctrl->last_tid; i++) {
+ if (ctrl->tid_tbl[i] == NULL)
+ break;
+ }
+ if (i >= ctrl->last_tid) {
+ if (ctrl->last_tid == (SLIM_MAX_TIDS - 1)) {
+ spin_unlock_irqrestore(&ctrl->txn_lock, flags);
+ slim_return_tx(ctrl, -ENOMEM);
+ return -ENOMEM;
+ }
+ ctrl->last_tid++;
+ }
+ ctrl->tid_tbl[i] = txn->msg;
+ txn->tid = i;
+ spin_unlock_irqrestore(&ctrl->txn_lock, flags);
+ }
+
+ ret = ctrl->xfer_msg(ctrl, txn, buf);
+
+ if (!ret && !async) { /* sync transaction */
+ /* Fine-tune calculation after bandwidth management */
+ unsigned long ms = txn->rl + 100;
+
+ ret = wait_for_completion_timeout(&done,
+ msecs_to_jiffies(ms));
+ if (!ret)
+ slim_return_tx(ctrl, -ETIMEDOUT);
+
+ ret = cbd.ret;
+ }
+
+ if (ret && need_tid) {
+ spin_lock_irqsave(&ctrl->txn_lock, flags);
+ /* Invalidate the transaction */
+ ctrl->tid_tbl[txn->tid] = NULL;
+ spin_unlock_irqrestore(&ctrl->txn_lock, flags);
+ }
+ if (ret)
+ dev_err(&ctrl->dev, "Tx:MT:0x%x, MC:0x%x, LA:0x%x failed:%d\n",
+ txn->mt, txn->mc, txn->la, ret);
+ if (!async) {
+ txn->msg->comp_cb = NULL;
+ txn->msg->ctx = NULL;
+ }
+ return ret;
+}
+EXPORT_SYMBOL_GPL(slim_processtxn);
that is interesting, I was expecting this to be internal API. So users are
expected to use this which is not very convenient IMO. Can we hide the gory
details and give users simple tx/rx or read/write APIs. FWIW most of the
usage would be thru regmap where people would call regmap_read/write()
Currently the only user of the api is qcom slim controller. May be the
reason for it to use this api is to fit in with all the locking and
sequencing mechanism with in the core.
I will be revisiting the semaphore thingy before I send next version,
hopefully I can to align with your above comments.
+
+static int slim_val_inf_sanity(struct slim_controller *ctrl,
+ struct slim_val_inf *msg, u8 mc)
+{
+ if (!msg || msg->num_bytes > 16 ||
+ (msg->start_offset + msg->num_bytes) > 0xC00)
+ goto reterr;
line break here
I agree.
+ switch (mc) {
+ case SLIM_MSG_MC_REQUEST_VALUE:
+ case SLIM_MSG_MC_REQUEST_INFORMATION:
what does MC refer to?
Message Code.
+ if (msg->rbuf != NULL)
+ return 0;
+ break;
after each break too
Sure, will fix it in next version.
+static u16 slim_slicecodefromsize(u16 req)
hmmm Linux code doesnt prefernamesnames like this :)
Yep, this function is unused, am going to delete this in next version.
+EXPORT_SYMBOL_GPL(slim_request_inf_element);
+
+
unnecessary double space
+struct slim_val_inf {
+ u16 start_offset;
+ u8 num_bytes;
+ u8 *rbuf;
+ const u8 *wbuf;
can we do read and write, if not it can be a buf which maybe rbuf or wbug
based on type
With REQUEST_CHANGE_VALUE single command we can read old value at the
same time we can write new value.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
