From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lj1-f176.google.com (mail-lj1-f176.google.com [209.85.208.176]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B26803EFD0B for ; Thu, 18 Jun 2026 11:06:24 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.208.176 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781780787; cv=none; b=X0nRJ0lyEvATC5SD5d6S95o44U8QBey2TcPbmT3orQ2P9jOnAlzsKGjgXV4fjx/xghhZo3hbeDlxNOSTUKeu8r/GwBiuY0CsUachoqPOMZ6ErBYMZ7Hf1ha64ru3vAcsfxqsFH3roO76g8SPzC9H7LWLQ0NpQ/cRYpe6tvIUt4E= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781780787; c=relaxed/simple; bh=NUicbhiAO5ua8Me7fPWYRCJJaEuoMDgg8YsbHkXxjJI=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=n6o27HAORNIk7EBRMg0HKZ7gEST2UogXKZv1P4j6yFGxYZ8x871fS9CqI0pYWt0rl7LiYs7qyNy3zbkQq+piA3HOB2kQEN5e8//NpmamgDLidYTUdOS27sjviL42E/tyuVSXbhyenaQk4s9B71NfjtzGB4yeyT0riCRLQAkWSM4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=U6zlviGb; arc=none smtp.client-ip=209.85.208.176 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="U6zlviGb" Received: by mail-lj1-f176.google.com with SMTP id 38308e7fff4ca-3967720c093so510181fa.1 for ; Thu, 18 Jun 2026 04:06:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1781780783; x=1782385583; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:references:cc:to:subject :user-agent:mime-version:date:message-id:from:to:cc:subject:date :message-id:reply-to; bh=s0fmq68+3hdiEDF6JrvX9WBO/0vNo5qekexZmlbtfqQ=; b=U6zlviGbR/sHt38EuJJaMSS+Aie2DDZKM3tyJmCDEt3ejeBcDh9CojCqQLvg+12OAG ggcZBfIjj/HEdAJtrZbVQpDYW+qkKay9KeeHPoQt0+NZnimojlYmAxmPDF46dCq33BV2 JGShS1F34j1mLLTq7pDXXb+Lz8ZwjFCvAcSWNY9Q2wwZmVMcfQv20afoWaJCnGsHUTj5 AVqzz/H9wN2o/3H/BdAOqU+txmXhKWioxtfmnWroXG6WhiNal9f3V3A6oeI3KAcE8msN KVBxi2fNdoYhxRv7afrT+JXbRXUT1VToVN/CD5p9wLzn/O+m+6QGagVk0roNAE1zpLva 1shw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1781780783; x=1782385583; h=content-transfer-encoding:in-reply-to:from:references:cc:to:subject :user-agent:mime-version:date:message-id:x-gm-gg:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=s0fmq68+3hdiEDF6JrvX9WBO/0vNo5qekexZmlbtfqQ=; b=aySTv6Oj35lTYGj7aSS9HT1cGlx4Yx2RgkZnUuywKIunIKMtWQmsAPJbBt/mIeIEoW ULbuKfMsCDzVZEb5/faxc3wRuGinhQsfSDGkMQAAZ5B6/qniiYcK9v6x82ilcSowUhLZ cYRV2WF2D4muEisin5njj3/qmfY624+8ML7znpPhkHuHHdVbU7fFLnxMAel3nJ47dcu7 hELaDNl9x7a5pJMpjYQqpjVZJyVj0Hxf4BJH1LicPZ/47mq09E9Nm7/tWWXkU2bs32J2 ybijWkUbCQzbBi7onO6aSDxfPP0ywNothXqMtVT11F3HkmKA3FiRwfhEivtDe4POm9BN XafA== X-Forwarded-Encrypted: i=1; AFNElJ++y4+/776eVfwDCPZvhwPqlKE+3C3pTDYCY1EU3tENZgh+fs3BfHFiNZtzYDNw07z/Btaf3xFuGYDdSBM=@vger.kernel.org X-Gm-Message-State: AOJu0YxgE+3cs2HMiTgsr7XkGPgr1G0FYwvkaoa99Zlcjnem6UBPbhQo Qb/c3FK8lbPGeqRuwpWrcL1pxDeb0CmevBA87kzPTS8yp+PlPhP8fAEp+E3xMNu4UUrtW696PLW ztYEcDxI= X-Gm-Gg: AfdE7clUqL9cA3kWtfCwyWXFtDRxolraslw/mCON3ijvubmJloUnax9Y89xy+3FDkxm u+smRDqinq2OH7Hd47p+PLICBPEssQmqRuGisyJIM3WhcJ9jAn8at/Zo25FaKohSAffpke7HESO NSLhs0qk05wretj7bC5yGNt5AEi9I34vNQmz4Dt8EaCWZ1/F0+3hPCjpNAkcC4/Qx60Q/BkJ6+P xJJmIvb7UkTtqE3IiC1Wr31Yx8WPpItTNEovqDlMIsdVz71BPmAx6p3JuntHOJmsIYXqGbNXzm0 VvN1WK6SRefqpe3Mf89gZxoWNaa4WvwVJFRu88I7OViVzamF+nh3i3Qziszmy09fqoXT2SRFzwu /zmY0oo2Oop/+6eS/tfaeZdTybZBoDX1l4JT4mf+EMIC53l40zN6s6PiuWU/g+7vieSQB2dMJiV vECIe9Y75E5lAql1QG4YfFmpti7nF1sMMAjOuOd1fd8yoEiqMVhX6NT2hjwwoF7hutlEs= X-Received: by 2002:a2e:bc87:0:b0:393:d08b:2a0c with SMTP id 38308e7fff4ca-39969a9269cmr11594201fa.2.1781780782745; Thu, 18 Jun 2026 04:06:22 -0700 (PDT) Received: from [192.168.1.100] (91-159-24-186.elisa-laajakaista.fi. [91.159.24.186]) by smtp.gmail.com with ESMTPSA id 38308e7fff4ca-3997238ea3dsm10911441fa.27.2026.06.18.04.06.21 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 18 Jun 2026 04:06:21 -0700 (PDT) Message-ID: Date: Thu, 18 Jun 2026 14:06:20 +0300 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 2/3] media: i2c: add os02g10 image sensor driver To: Elgin Perumbilly , "sakari.ailus@linux.intel.com" , "laurent.pinchart@ideasonboard.com" Cc: Tarang Raval , Mauro Carvalho Chehab , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Hans Verkuil , Hans de Goede , Mehdi Djait , Sylvain Petinot , Benjamin Mugnier , Bryan O'Donoghue , Himanshu Bhavani , Heimir Thor Sverrisson , Jingjing Xiong , Svyatoslav Ryhel , "linux-media@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" References: <20260424092554.26130-1-elgin.perumbilly@siliconsignals.io> <20260424092554.26130-3-elgin.perumbilly@siliconsignals.io> <421ae63a-88c6-4e81-8478-7f581357676b@linaro.org> From: Vladimir Zapolskiy In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 6/18/26 09:22, Elgin Perumbilly wrote: > Hi Vladimir, > > Thank you for the review. > > I have addressed all of the comments except for two, where I am not entirely > sure about the requested changes. Could you please take a look at the points > below and let me know your opinion? > >> On 4/24/26 12:25, Elgin Perumbilly wrote: >>> Add a v4l2 subdevice driver for the Omnivision os02g10 sensor. >>> >>> The Omnivision os02g10 is a CMOS image sensor with an active array size of >>> 1920 x 1080. >>> >>> The following features are supported: >>> - Manual exposure an gain control support >>> - vblank/hblank control support >>> - vflip/hflip control support >>> - Test pattern control support >>> - Supported resolution: 1920 x 1080 @ 30fps (SBGGR10) >>> >>> Signed-off-by: Elgin Perumbilly >>> Reviewed-by: Tarang Raval > > ... > >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >> >> Please sort the list of includes in alphabetical order, also you >> may consider to shrink the list by removing quite many inherited >> includes. > > Some maintainers prefer the "include what you use" approach, like Andy, > so I added all the headers that are directly used. Should I now remove > any inherited includes? Yes, here opinions may vary, that's why I asked for sorting and to consider to remove some of the redundant headers. In my personal opinion this type of excessive information is not needed, especially if it is justified only by probable and far future trivial clean-up work. >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include > > ... > >>> +static int os02g10_set_framefmt(struct os02g10 *os02g10, >>> + struct v4l2_subdev_state *state) >>> +{ >>> + const struct v4l2_mbus_framefmt *format; >>> + const struct os02g10_mode *mode; >>> + int ret = 0; >>> + >>> + format = v4l2_subdev_state_get_format(state, 0); >>> + mode = v4l2_find_nearest_size(supported_modes, >>> + ARRAY_SIZE(supported_modes), width, >>> + height, format->width, format->height); >>> + >>> + cci_write(os02g10->cci, OS02G10_REG_V_START, mode->y_start, &ret); >>> + cci_write(os02g10->cci, OS02G10_REG_V_SIZE, mode->height, &ret); >>> + cci_write(os02g10->cci, OS02G10_REG_V_SIZE_MIPI, mode->height, &ret); >>> + cci_write(os02g10->cci, OS02G10_REG_H_START, mode->x_start, &ret); >>> + cci_write(os02g10->cci, OS02G10_REG_H_SIZE, mode->width, &ret); >>> + cci_write(os02g10->cci, OS02G10_REG_H_SIZE_MIPI, mode->width, &ret); >>> + >>> + return ret; >> >> Just "return 0" here, and remove the local variable. > > Could you clarify why this should return 0? The local ret is passed to all > cci_write() calls so that any write error is propagated. Returning 0 here > would appear to suppress those errors and always report success. > My bad, yes, here please leave 'return ret' as is, I was confused and misleaded by initialization of the local variable to zero, which is redundant, and I'd suggest to remove this initialization. -- Best wishes, Vladimir