From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754204AbeDZHEc (ORCPT ); Thu, 26 Apr 2018 03:04:32 -0400 Received: from mail-wr0-f175.google.com ([209.85.128.175]:36713 "EHLO mail-wr0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754038AbeDZHE2 (ORCPT ); Thu, 26 Apr 2018 03:04:28 -0400 X-Google-Smtp-Source: AIpwx49qD1e06JMFSIsVdOHTqwLi7882oiB6PrlgQw8mrk07dSvysI1Emu3ujYZovkceg6WEBMeyGg== Subject: Re: [PATCH v4 2/2] media: Add a driver for the ov7251 camera sensor To: Sakari Ailus Cc: mchehab@kernel.org, hverkuil@xs4all.nl, laurent.pinchart@ideasonboard.com, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org References: <1524673246-14175-1-git-send-email-todor.tomov@linaro.org> <1524673246-14175-3-git-send-email-todor.tomov@linaro.org> <20180426065010.a67iqsaicpgu7m5b@valkosipuli.retiisi.org.uk> From: Todor Tomov Message-ID: Date: Thu, 26 Apr 2018 10:04:25 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180426065010.a67iqsaicpgu7m5b@valkosipuli.retiisi.org.uk> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Sakari, On 26.04.2018 09:50, Sakari Ailus wrote: > Hi Todor, > > On Wed, Apr 25, 2018 at 07:20:46PM +0300, Todor Tomov wrote: > ... >> +static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val) >> +{ >> + u8 regbuf[3]; >> + int ret; >> + >> + regbuf[0] = reg >> 8; >> + regbuf[1] = reg & 0xff; >> + regbuf[2] = val; >> + >> + ret = i2c_master_send(ov7251->i2c_client, regbuf, 3); >> + if (ret < 0) { >> + dev_err(ov7251->dev, "%s: write reg error %d: reg=%x, val=%x\n", >> + __func__, ret, reg, val); >> + return ret; >> + } >> + >> + return 0; > > How about: > > return ov7251_write_seq_regs(ov7251, reg, &val, 1); > > And put the function below ov2751_write_seq_regs(). I'm not sure... It will calculate message length each time and then check that it is not greater than 5, which it is. Seems redundant. > >> +} >> + >> +static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val, >> + u8 num) >> +{ >> + const u8 maxregbuf = 5; >> + u8 regbuf[maxregbuf]; >> + u8 nregbuf = sizeof(reg) + num * sizeof(*val); >> + int ret = 0; >> + >> + if (nregbuf > maxregbuf) >> + return -EINVAL; >> + >> + regbuf[0] = reg >> 8; >> + regbuf[1] = reg & 0xff; >> + >> + memcpy(regbuf + 2, val, num); >> + >> + ret = i2c_master_send(ov7251->i2c_client, regbuf, nregbuf); >> + if (ret < 0) { >> + dev_err(ov7251->dev, "%s: write seq regs error %d: first reg=%x\n", > > This line is over 80... Yes indeed. Somehow checkpatch does not report this line, I don't know why. > > If you're happy with these, I can make the changes, too; they're trivial. Only the second one? Thanks :) > >> + __func__, ret, reg); >> + return ret; >> + } >> + >> + return 0; >> +} > -- Best regards, Todor Tomov