From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <556456A1.7030401@arm.com> Date: Tue, 26 May 2015 12:18:57 +0100 From: Sudeep Holla MIME-Version: 1.0 To: "Jon Medhurst (Tixy)" Subject: Re: [PATCH v2 2/5] firmware: add support for ARM System Control and Power Interface(SCPI) protocol References: <1431618155-3132-1-git-send-email-sudeep.holla@arm.com> <1431618155-3132-3-git-send-email-sudeep.holla@arm.com> <1432120636.3231.24.camel@linaro.org> In-Reply-To: <1432120636.3231.24.camel@linaro.org> Cc: Lorenzo Pieralisi , "linux-pm@vger.kernel.org" , Jassi Brar , Liviu Dudau , "linux-kernel@vger.kernel.org" , Sudeep Holla , "linux-clk@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+mturquette=linaro.org@lists.infradead.org List-ID: On 20/05/15 12:17, Jon Medhurst (Tixy) wrote: > On Thu, 2015-05-14 at 16:42 +0100, Sudeep Holla wrote: >> This patch adds support for System Control and Power Interface (SCPI) >> Message Protocol used between the Application Cores(AP) and the System >> Control Processor(SCP). The MHU peripheral provides a mechanism for >> inter-processor communication between SCP's M3 processor and AP. >> >> SCP offers control and management of the core/cluster power states, >> various power domain DVFS including the core/cluster, certain system >> clocks configuration, thermal sensors and many others. >> >> This protocol driver provides interface for all the client drivers using >> SCPI to make use of the features offered by the SCP. >> >> Signed-off-by: Sudeep Holla >> CC: Jassi Brar >> Cc: Liviu Dudau >> Cc: Lorenzo Pieralisi >> Cc: Jon Medhurst (Tixy) >> --- > > Sorry for the delay in looking at this. I have one nitpick below but > anyway, here's a > > Reviewed-by: Jon Medhurst (Tixy) > Thanks! > [...] >> +++ b/drivers/firmware/arm_scpi.c > [...] >> +static void scpi_process_cmd(struct scpi_chan *ch, u32 cmd) >> +{ >> + unsigned long flags; >> + struct scpi_xfer *t, *match = NULL; >> + >> + spin_lock_irqsave(&ch->rx_lock, flags); >> + if (list_empty(&ch->rx_pending)) { >> + spin_unlock_irqrestore(&ch->rx_lock, flags); >> + return; >> + } >> + >> + list_for_each_entry(t, &ch->rx_pending, node) >> + if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) { >> + list_del(&t->node); >> + match = t; >> + break; >> + } >> + /* check if wait_for_completion is in progress or timed-out */ >> + if (match && !completion_done(&match->done)) { >> + struct scpi_shared_mem *mem = ch->rx_payload; >> + int len = min(match->rx_len, CMD_SIZE(cmd)); >> + >> + match->status = le32_to_cpu(mem->status); >> + memcpy_fromio(match->rx_buf, mem->payload, len); >> + if (match->rx_len > len) > > rx_len is unsigned and len is signed and so I had to go refresh my > memory from the C standard before I could convince myself that this if > statement was OK. Might be clearer if len was unsigned, especially as > it's the 'min' of two unsigned values. > Fixed locally for next version. Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel