From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 671E02F31 for ; Fri, 10 Feb 2023 11:35:00 +0000 (UTC) Received: from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi [213.243.189.158]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id C762127C; Fri, 10 Feb 2023 12:34:57 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1676028898; bh=Ffl1cyU5TLZfkGB7NTCX097mcL2f4GPZl9iPOEmdqVY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Vt4tH43Lm3dlKnYL3SlOnPbRObdSC+g8WnGq9YQwPNt1sJRTuNHX/E0oi38n25OV4 9QnulthSZkUFU8gahY5NdSvswr+kT/fJuaGgrKkS8QoGSxr0VQZmk5L5QOdE3ZdYGA OiE5Kfb7Jovq6sWyilP0iAj+5dLUn35Zsl6oPY5Y= Date: Fri, 10 Feb 2023 13:34:56 +0200 From: Laurent Pinchart To: Sakari Ailus Cc: Hans de Goede , Mauro Carvalho Chehab , Tsuchiya Yuto , Andy Shevchenko , Yury Luneff , Nable , andrey.i.trufanov@gmail.com, Fabio Aiuto , linux-media@vger.kernel.org, linux-staging@lists.linux.dev Subject: Re: [PATCH 28/57] media: Add ovxxxx_16bit_addr_reg_helpers.h Message-ID: References: <20230123125205.622152-1-hdegoede@redhat.com> <20230123125205.622152-29-hdegoede@redhat.com> <026272d3-88d7-a67f-4942-5cba6c3eab86@redhat.com> Precedence: bulk X-Mailing-List: linux-staging@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Hi Sakari, On Fri, Feb 10, 2023 at 01:18:12PM +0200, Sakari Ailus wrote: > On Fri, Feb 10, 2023 at 01:04:48PM +0200, Laurent Pinchart wrote: > > > > > > > > Also, may I > > > > > > > > suggest to have a look at drivers/media/i2c/imx290.c for an example of > > > > > > > > how registers of different sizes can be handled in a less error-prone > > > > > > > > way, using single read/write functions that adapt to the size > > > > > > > > automatically ? > > > > > > > > > > > > > > Yes I have seen this pattern in drivers/media/i2c/ov5693.c too > > > > > > > (at least I assume it is the same pattern you are talking about). > > > > > > > > > > > > Correct. Can we use something like that to merge all the ov*_write_reg() > > > > > > variants into a single function ? Having to select the size manually in > > > > > > each call (either by picking the function variant, or by passing a size > > > > > > as a function parameter) is error-prone. Encoding the size in the > > > > > > register macro is much safer, easing both development and review. > > > > > > > > > > I think so, too. > > > > > > > > > > That doesn't mean we shouldn't have function variants for specific register > > > > > sizes (taking just register addresses) though. > > > > > > > > I don't see why we should have multiple APIs when a single one works. > > > > > > Yes, it "works", but the purpose of the API is to avoid driver code. A > > > driver accessing fixed width registers is likely to use a helper function > > > with an API that requires encoding the width into the register address. > > > > Why not ? I don't see anything wrong with having that as a single API, > > it doesn't make life more complicated for driver authors or reviewers. > > Given that the reviewers (at least me) haven't had noteworthy issues when > each driver implements their own register access functions, I'm not > concerned having ~ six register read functions instead of one or two. > Driver authors should pick the one that fits the purpose best, and not be > required to implement wrappers in drivers --- which is exactly the > situation we have today. It's of course always technically possibly to have more functions, but I don't see any practical advantage. -- Regards, Laurent Pinchart