From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752865AbbHZRI7 (ORCPT <rfc822;w@1wt.eu>);
	Wed, 26 Aug 2015 13:08:59 -0400
Received: from relay1.mentorg.com ([192.94.38.131]:62504 "EHLO
	relay1.mentorg.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751393AbbHZRI6 (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 26 Aug 2015 13:08:58 -0400
Subject: Re: [PATCH 2/4] remoteproc: Supply controller driver for ST's Remote
 Processors
To: Lee Jones <lee.jones@linaro.org>
References: <1440594483-29601-1-git-send-email-lee.jones@linaro.org>
 <1440594483-29601-3-git-send-email-lee.jones@linaro.org>
CC: <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
        <ohad@wizery.com>, <devicetree@vger.kernel.org>, <kernel@stlinux.com>
From: Nathan Lynch <Nathan_Lynch@mentor.com>
Message-ID: <55DDF252.2010901@mentor.com>
Date: Wed, 26 Aug 2015 12:07:30 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.1.0
MIME-Version: 1.0
In-Reply-To: <1440594483-29601-3-git-send-email-lee.jones@linaro.org>
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 08/26/2015 08:08 AM, Lee Jones wrote:
> --- /dev/null
> +++ b/drivers/remoteproc/st_remoteproc.c
> @@ -0,0 +1,300 @@
> +/*
> + * ST's Remote Processor Control Driver
> + *
> + * Copyright (C) 2015 STMicroelectronics - All Rights Reserved
> + *
> + * Author: Ludovic Barre <ludovic.barre@st.com>

When submitting code you didn't write, I'd say it's better practice to
clearly indicate its provenance in the commit message.  E.g. something
like "Driver based on code authored by Ludovic Barre for ST".  And
obtain signoffs etc if possible.


> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License.

Please review the wording here.  It's unclear whether this is intended
to be v2-only or v2 or later.


> +static int st_rproc_stop(struct rproc *rproc)
> +{
> +	struct st_rproc *st_rproc = rproc->priv;
> +	int err = 0;
> +
> +	if (st_rproc->config->sw_reset) {
> +		err = reset_control_assert(st_rproc->sw_reset);
> +		if (err)
> +			dev_warn(&rproc->dev, "Failed to assert S/W Reset\n");
> +	}
> +
> +	if (st_rproc->config->pwr_reset) {
> +		err = reset_control_assert(st_rproc->pwr_reset);
> +		if (err)
> +			dev_warn(&rproc->dev, "Failed to assert Power Reset\n");
> +	}
> +
> +	clk_disable(st_rproc->clk);
> +
> +	return 0;
> +}

Seems like st_rproc_stop should propagate errors back to its caller
instead of always returning 0.