All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mark Brown <broonie@kernel.org>
To: Sagar Dharia <sdharia@codeaurora.org>
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
	bp@suse.de, poeschel@lemonage.de, santosh.shilimkar@ti.com,
	treding@nvidia.com, gong.chen@linux.intel.com,
	andreas.noever@gmail.com, alan@linux.intel.com,
	mathieu.poirier@linaro.org, daniel@ffwll.ch, oded.gabbay@amd.com,
	jkosina@suse.cz, sharon.dvir1@mail.huji.ac.il, joe@perches.com,
	davem@davemloft.net, james.hogan@imgtec.com,
	michael.opdenacker@free-electrons.com,
	daniel.thompson@linaro.org, nkaje@codeaurora.org,
	mbutler@audience.com, kheitke@audience.com,
	mlocke@codeaurora.org, agross@codeaurora.org,
	linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH 3/3] slimbus: Add messaging APIs to slimbus framework
Date: Mon, 15 Jun 2015 12:08:37 +0100	[thread overview]
Message-ID: <20150615110837.GL18309@sirena.org.uk> (raw)
In-Reply-To: <1434260958-13732-4-git-send-email-sdharia@codeaurora.org>

[-- Attachment #1: Type: text/plain, Size: 2344 bytes --]

On Sat, Jun 13, 2015 at 11:49:18PM -0600, Sagar Dharia wrote:

> +	if (txn->mt == SLIM_MSG_MT_CORE &&
> +		(txn->mc == SLIM_MSG_MC_REQUEST_INFORMATION ||
> +		 txn->mc == SLIM_MSG_MC_REQUEST_CLEAR_INFORMATION ||
> +		 txn->mc == SLIM_MSG_MC_REQUEST_VALUE ||
> +		 txn->mc == SLIM_MSG_MC_REQUEST_CLEAR_INFORMATION)) {

The mc comparison here looks like you meant to write a switch
statement.

> +	ret = ctrl->xfer_msg(ctrl, txn);
> +	return ret;
> +}

No need for ret here.

> +static int slim_xfer_msg(struct slim_controller *ctrl,
> +			struct slim_device *sbdev, struct slim_val_inf *msg,
> +			u8 mc)
> +{
> +	DEFINE_SLIM_LDEST_TXN(txn_stack, mc, 6, sbdev->laddr, msg);
> +	struct slim_msg_txn *txn = &txn_stack;
> +	int ret;
> +	unsigned long flags;
> +	u16 sl, cur;
> +	bool tid_txn, async = false;
> +	DECLARE_COMPLETION_ONSTACK(complete);
> +
> +	ret = slim_val_inf_sanity(msg, mc);
> +	if (ret) {
> +		pr_err("Sanity check failed for msg:offset:0x%x, mc:%d",
> +				msg->start_offset, mc);

dev_err() seems better, and if you're going to print an error on this
why not move the error prints into the sanity check so someone seeing
the error message can tell what went wrong?

> +	sl = slim_slicesize(msg->num_bytes);
> +	dev_err(&ctrl->dev, "SB xfer msg:os:%x, len:%d, MC:%x, sl:%x\n",
> +				msg->start_offset, msg->num_bytes, mc, sl);

Looks like you left osme debug statements in here.

> +	if (!msg->comp_cb && tid_txn) {
> +		msg->comp_cb = slim_sync_default_cb;
> +		msg->ctx = &complete;
> +	} else
> +		async = true;

Coding style: if you have braces on one branch of an if they should be
on both.

> +	/* sync read */
> +	if (!ret && tid_txn && !async) {
> +		ret = wait_for_completion_timeout(&complete, HZ);
> +		if (!ret)
> +			ret = -ETIMEDOUT;
> +		else
> +			ret = 0;
> +	}

Are we sure that HZ is a good timeout here - might it be too short or
too long for some users?

> diff --git a/include/linux/slimbus.h b/include/linux/slimbus.h
> index 61b7c74..1d98c58 100644
> --- a/include/linux/slimbus.h
> +++ b/include/linux/slimbus.h
> @@ -34,6 +34,7 @@ extern struct bus_type slimbus_type;
>  #define SLIM_FRM_SLOTS_PER_SUPERFRAME	16
>  #define SLIM_GDE_SLOTS_PER_SUPERFRAME	2
>  
> +#define SLIM_MAX_TXNS			256

Where did this number come from?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

  reply	other threads:[~2015-06-15 11:09 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-06-14  5:49 [PATCH 0/3] Introduce framework for SLIMbus device drivers Sagar Dharia
2015-06-14  5:49 ` [PATCH 1/3] SLIMbus: Device management on SLIMbus Sagar Dharia
2015-06-15 10:54   ` Mark Brown
2015-06-16 15:22     ` Sagar Dharia
2015-06-17 11:45       ` Mark Brown
2015-06-17 17:10         ` Sagar Dharia
2015-06-18 21:23   ` Srinivas Kandagatla
2015-06-19  3:48     ` Sagar Dharia
2015-06-14  5:49 ` [PATCH 2/3] of/slimbus: OF helper for SLIMbus Sagar Dharia
2015-06-15 10:55   ` Mark Brown
2015-06-14  5:49 ` [PATCH 3/3] slimbus: Add messaging APIs to slimbus framework Sagar Dharia
2015-06-15 11:08   ` Mark Brown [this message]
2015-06-14 15:32 ` [PATCH 0/3] Introduce framework for SLIMbus device drivers Greg KH
2015-06-15 11:27   ` Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150615110837.GL18309@sirena.org.uk \
    --to=broonie@kernel.org \
    --cc=agross@codeaurora.org \
    --cc=alan@linux.intel.com \
    --cc=andreas.noever@gmail.com \
    --cc=bp@suse.de \
    --cc=daniel.thompson@linaro.org \
    --cc=daniel@ffwll.ch \
    --cc=davem@davemloft.net \
    --cc=gong.chen@linux.intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=james.hogan@imgtec.com \
    --cc=jkosina@suse.cz \
    --cc=joe@perches.com \
    --cc=kheitke@audience.com \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mathieu.poirier@linaro.org \
    --cc=mbutler@audience.com \
    --cc=michael.opdenacker@free-electrons.com \
    --cc=mlocke@codeaurora.org \
    --cc=nkaje@codeaurora.org \
    --cc=oded.gabbay@amd.com \
    --cc=poeschel@lemonage.de \
    --cc=santosh.shilimkar@ti.com \
    --cc=sdharia@codeaurora.org \
    --cc=sharon.dvir1@mail.huji.ac.il \
    --cc=treding@nvidia.com \
    --subject='Re: [PATCH 3/3] slimbus: Add messaging APIs to slimbus framework' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.