From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Brown Subject: Re: [PATCH 3/3] slimbus: Add messaging APIs to slimbus framework Date: Mon, 15 Jun 2015 12:08:37 +0100 Message-ID: <20150615110837.GL18309@sirena.org.uk> References: <1434260958-13732-1-git-send-email-sdharia@codeaurora.org> <1434260958-13732-4-git-send-email-sdharia@codeaurora.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="TnYVF1hk1c8rpHiF" Return-path: Received: from mezzanine.sirena.org.uk ([106.187.55.193]:58341 "EHLO mezzanine.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754083AbbFOLJV (ORCPT ); Mon, 15 Jun 2015 07:09:21 -0400 Content-Disposition: inline In-Reply-To: <1434260958-13732-4-git-send-email-sdharia@codeaurora.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Sagar Dharia 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 --TnYVF1hk1c8rpHiF Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Jun 13, 2015 at 11:49:18PM -0600, Sagar Dharia wrote: > + if (txn->mt =3D=3D SLIM_MSG_MT_CORE && > + (txn->mc =3D=3D SLIM_MSG_MC_REQUEST_INFORMATION || > + txn->mc =3D=3D SLIM_MSG_MC_REQUEST_CLEAR_INFORMATION || > + txn->mc =3D=3D SLIM_MSG_MC_REQUEST_VALUE || > + txn->mc =3D=3D SLIM_MSG_MC_REQUEST_CLEAR_INFORMATION)) { The mc comparison here looks like you meant to write a switch statement. > + ret =3D 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 =3D &txn_stack; > + int ret; > + unsigned long flags; > + u16 sl, cur; > + bool tid_txn, async =3D false; > + DECLARE_COMPLETION_ONSTACK(complete); > + > + ret =3D 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 =3D 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 =3D slim_sync_default_cb; > + msg->ctx =3D &complete; > + } else > + async =3D 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 =3D wait_for_completion_timeout(&complete, HZ); > + if (!ret) > + ret =3D -ETIMEDOUT; > + else > + ret =3D 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 > =20 > +#define SLIM_MAX_TXNS 256 Where did this number come from? --TnYVF1hk1c8rpHiF Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJVfrI0AAoJECTWi3JdVIfQ280H/0dMOmrh4p+oN//f8RnI/HZi AZ90M+QMmYunI6rX9VlFM7u15xKtC/7w7Cit3caSHOXud3xpR9swfxs2JRa+/fH3 +tOd/qAjImPgggcOu2mK1+3t+lxAnF8qQyMUxKJinl6VXuHqUeTNaQJHf9DIBenO hj4omAuqUak3VOkmHLEPm4rPS4IS65UpgSxAHRiHjpj4PGzXIfvb3eI3rmV8bPYB rdImPX9I/xooHovtE++auBTp87ORDf8oMt4UPfl9F10Q/nD3KgbOUtmUQrntI66z KAqGqD9yXMYkAlMR2ssfeGIM5AejSlhhAslvnxeLtktLxPr+6EcwCwgyvIPwR2I= =cSWh -----END PGP SIGNATURE----- --TnYVF1hk1c8rpHiF--