From: Stuart Yoder <stuart.yoder@nxp.com>
To: Laurentiu Tudor <laurentiu.tudor@nxp.com>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
CC: "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "agraf@suse.de" <agraf@suse.de>, "arnd@arndb.de" <arnd@arndb.de>,
        Leo Li <leoyang.li@nxp.com>, Ioana Ciornei <ioana.ciornei@nxp.com>,
        "Catalin Horghidan" <catalin.horghidan@nxp.com>,
        Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>,
        Roy Pledge <roy.pledge@nxp.com>
Subject: RE: [PATCH v3 3/9] bus: fsl-mc: dpio: add APIs for DPIO objects
Thread-Topic: [PATCH v3 3/9] bus: fsl-mc: dpio: add APIs for DPIO objects
Thread-Index: AQHSTCUbQT0mvsH2lkarEwd+zhdm5qD0haMAgBTCLMA=
Date: Thu, 15 Dec 2016 16:36:43 +0000
Message-ID: <VI1PR0401MB26389555343B2A2AD09541698D9D0@VI1PR0401MB2638.eurprd04.prod.outlook.com>
References: <1480632094-3621-1-git-send-email-stuart.yoder@nxp.com>
 <1480632094-3621-4-git-send-email-stuart.yoder@nxp.com>
 <58415A74.6050604@nxp.com>
In-Reply-To: <58415A74.6050604@nxp.com>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Dec 2016 16:36:43.5848
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0401MB2638
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uBG0G3YJ009851
Content-Length: 1258
Lines: 34

> > +#define DPIO_CMD(id)	((id << DPIO_CMD_ID_OFFSET) | DPIO_CMD_BASE_VERSION)
> 
> Paranthesis around 'id'?

In all cases these are opcode values and will never be an expression.  If
we really need to future proof these definitions, we should do it for all
objects not just DPIO.  I'd like to see consistency across objects and don't
want to see DPIO gratuitously diverge.  So, my suggestion is to have an
offline discussion and if we think the change is needed, submit a patch for
all objects currently supported.

> > +	/* prepare command */
> > +	cmd.header = mc_encode_cmd_header(DPIO_CMDID_OPEN,
> > +					  cmd_flags,
> > +					  0);
> > +	dpio_cmd = (struct dpio_cmd_open *)cmd.params;
> > +	dpio_cmd->dpio_id = cpu_to_le32(dpio_id);
> > +
> > +	/* send command to mc*/
> > +	err = mc_send_command(mc_io, &cmd);
> > +	if (err)
> > +		return err;
> > +
> > +	/* retrieve response parameters */
> > +	*token = mc_cmd_hdr_read_token(&cmd);
> 
> Nit: maybe we should drop these repetitive "prepare / send / retrieve" comments
> as the code is pretty self explanatory.

The 'send' comment certainly isn't needed given that the function
is 'mc_send_command()'.  For the others, I think having some comment
is helpful, even though a bit repetitive.

Stuart