From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id DD863C4332F for ; Tue, 31 Oct 2023 08:53:47 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235349AbjJaIxs (ORCPT ); Tue, 31 Oct 2023 04:53:48 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41466 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235314AbjJaIxr (ORCPT ); Tue, 31 Oct 2023 04:53:47 -0400 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2D409DA; Tue, 31 Oct 2023 01:53:43 -0700 (PDT) Received: from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi [213.243.189.158]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id C46492E4; Tue, 31 Oct 2023 09:53:24 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1698742405; bh=Hy2L6X5L+EpclPfJr2s81XeLbfQHnZfCrcEAXNAvOas=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=gmGVPDzzTNYsrw007x2cWeYTgbxJ+Tyy1zUvHeV8n+yj0Wl5bPAO98Z/In7cBDPWQ +bQNC486sW8UlXDO57Og2t48GsvcEkXbuZR/EJ1sLQ/Ex9Wg5Ki6yaYIxV3/3/YgMH HniHR/GEtvWqcolPYlKxOBQh383Dn5tSc8CXHH2w= Date: Tue, 31 Oct 2023 10:53:47 +0200 From: Laurent Pinchart To: Sakari Ailus Cc: Tommaso Merciai , martin.hecht@avnet.eu, michael.roeder@avnet.eu, mhecht73@gmail.com, linuxfancy@googlegroups.com, Mauro Carvalho Chehab , Liam Girdwood , Mark Brown , Hans de Goede , Hans Verkuil , Tomi Valkeinen , Marco Felsch , Gerald Loacker , Andy Shevchenko , Daniel Scally , Shawn Tu , Linus Walleij , linux-kernel@vger.kernel.org, linux-media@vger.kernel.org Subject: Re: [PATCH v10 3/3] media: i2c: Add support for alvium camera Message-ID: <20231031085347.GH12764@pendragon.ideasonboard.com> References: <20231020141354.2500602-1-tomm.merciai@gmail.com> <20231020141354.2500602-4-tomm.merciai@gmail.com> <20231030233809.GD12764@pendragon.ideasonboard.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org On Tue, Oct 31, 2023 at 06:34:39AM +0000, Sakari Ailus wrote: > On Tue, Oct 31, 2023 at 01:38:09AM +0200, Laurent Pinchart wrote: > > On Mon, Oct 30, 2023 at 10:43:47PM +0000, Sakari Ailus wrote: > > > On Mon, Oct 30, 2023 at 01:26:58PM +0100, Tommaso Merciai wrote: > > > > > > ... > > > > > > > > > +static int alvium_get_host_supp_csi_lanes(struct alvium_dev *alvium) > > > > > > +{ > > > > > > + u64 val; > > > > > > + int ret = 0; > > > > > > + > > > > > > + alvium_read(alvium, REG_BCRM_CSI2_LANE_COUNT_RW, &val, &ret); > > > > > > > > > > Missing error checking before the use of the value. The same pattern > > > > > remains prevalent throughout the driver. > > > > > > > > > > I think it'd be easier if you didn't use a temporary variable for reading, > > > > > but instead had a register width specific access function. You could even > > > > > introduce a helper macro to read this information as I suggested in an > > > > > earlier review. > > > > > > > > oks. > > > > We are moving to use the following macros: > > > > > > > > #define alvium_read_check(alvium, reg, value) \ > > > > { \ > > > > int ret = alvium_read(alvium, reg, value, NULL); \ > > > > if (ret) \ > > > > return ret; \ > > > > } > > > > > > > > > > You could do something like (entirely untested): > > > > > > #define ALVIUM_DECLARE_READ(sign, bits) \ > > > static int > > > alvium_read_ ## sign ## bits(struct alvium_dev *alvium, u32 reg, \ > > > sign ## bits *val, int *err) \ > > > { \ > > > u64 val64; \ > > > int ret; \ > > > \ > > > if (err && *err < 0) \ > > > return *err; \ > > > \ > > > alvium_read(alvium, reg, &val64, &ret); \ > > > if (ret < 0) { \ > > > if (err) \ > > > *err = ret; \ > > > return ret; \ > > > } \ > > > \ > > > *val = val64; \ > > > \ > > > return 0; \ > > > } > > > > > > ALVIUM_DECLARE_READ(u, 32); > > > > > > And then, e.g. instead of (and failing to check ret): > > > > > > u64 val; > > > > > > alvium_read(alvium, REG_BCRM_CONTRAST_VALUE_RW, &val, &ret); > > > alvium->dft_contrast = val; > > > > > > you'd have a single call: > > > > > > alvium_read_u32(alvium, REG_BCRM_CONTRAST_VALUE_RW, > > > &alvium->dft_contrast, &ret); > > > > > > And so on. > > > > > > You can drop sign if you don't need signed reads but some of the struct > > > fields you're writing something appear to be signed. > > > > > > It'd be good to check the register size matches with the size of *val, too. > > > Maybe something like: > > > > > > WARN_ON((CCI_REG ## bits(0) && CCI_REG_WIDTH_MASK) >> CCI_REG_WIDTH_SHIFT > > > != sizeof(sign ## bits)); > > > > I think this could actually be automated, and implemented in v4l2-cci. > > Something like the following: > > > > diff --git a/drivers/media/v4l2-core/v4l2-cci.c b/drivers/media/v4l2-core/v4l2-cci.c > > index bc2dbec019b0..27f1eaa7777d 100644 > > --- a/drivers/media/v4l2-core/v4l2-cci.c > > +++ b/drivers/media/v4l2-core/v4l2-cci.c > > @@ -16,7 +16,7 @@ > > > > #include > > > > -int cci_read(struct regmap *map, u32 reg, u64 *val, int *err) > > +int __cci_read(struct regmap *map, u32 reg, void *val, int *err) > > { > > unsigned int len; > > u8 buf[8]; > > @@ -37,19 +37,19 @@ int cci_read(struct regmap *map, u32 reg, u64 *val, int *err) > > > > switch (len) { > > case 1: > > - *val = buf[0]; > > + *(u8 *)val = buf[0]; > > break; > > case 2: > > - *val = get_unaligned_be16(buf); > > + *(u16 *)val = get_unaligned_be16(buf); > > break; > > case 3: > > - *val = get_unaligned_be24(buf); > > + *(u32 *)val = get_unaligned_be24(buf); > > break; > > case 4: > > - *val = get_unaligned_be32(buf); > > + *(u32 *)val = get_unaligned_be32(buf); > > break; > > case 8: > > - *val = get_unaligned_be64(buf); > > + *(u64 *)val = get_unaligned_be64(buf); > > break; > > default: > > dev_err(regmap_get_device(map), "Error invalid reg-width %u for reg 0x%04x\n", > > @@ -64,7 +64,7 @@ int cci_read(struct regmap *map, u32 reg, u64 *val, int *err) > > > > return ret; > > } > > -EXPORT_SYMBOL_GPL(cci_read); > > +EXPORT_SYMBOL_GPL(__cci_read); > > > > int cci_write(struct regmap *map, u32 reg, u64 val, int *err) > > { > > @@ -119,7 +119,7 @@ int cci_update_bits(struct regmap *map, u32 reg, u64 mask, u64 val, int *err) > > u64 readval; > > int ret; > > > > - ret = cci_read(map, reg, &readval, err); > > + ret = __cci_read(map, reg, &readval, err); > > if (ret) > > return ret; > > > > diff --git a/include/media/v4l2-cci.h b/include/media/v4l2-cci.h > > index 0f6803e4b17e..31223ce8d741 100644 > > --- a/include/media/v4l2-cci.h > > +++ b/include/media/v4l2-cci.h > > @@ -7,6 +7,9 @@ > > #ifndef _V4L2_CCI_H > > #define _V4L2_CCI_H > > > > +#include > > +#include > > +#include > > #include > > > > struct i2c_client; > > @@ -39,6 +42,8 @@ struct cci_reg_sequence { > > #define CCI_REG32(x) ((4 << CCI_REG_WIDTH_SHIFT) | (x)) > > #define CCI_REG64(x) ((8 << CCI_REG_WIDTH_SHIFT) | (x)) > > > > +int __cci_read(struct regmap *map, u32 reg, void *val, int *err); > > + > > /** > > * cci_read() - Read a value from a single CCI register > > * > > @@ -48,9 +53,17 @@ struct cci_reg_sequence { > > * @err: Optional pointer to store errors, if a previous error is set > > * then the read will be skipped > > * > > + * The type of the @val pointer must match the size of the register being read. > > + * Mismatches will result in compile-time errors. > > + * > > * Return: %0 on success or a negative error code on failure. > > */ > > -int cci_read(struct regmap *map, u32 reg, u64 *val, int *err); > > +#define cci_read(map, reg, val, err) ({ \ > > + u32 __reg = (reg); \ > > + u32 __size = FIELD_GET(CCI_REG_WIDTH_MASK, __reg); \ > > + BUILD_BUG_ON(sizeof(*(val)) != roundup_pow_of_two(__size)); \ > > + __cci_read(map, __reg, (void *)(val), err); \ > > +}) > > > > /** > > * cci_write() - Write a value to a single CCI register > > > > The change to cci_update_bits() is obviously wrong, I've hacked that to > > compile-test the rest with the drivers using cci_read(), and I get nice > > build-time errors due to usage of the wrong type :-) > > > > Is this something that would be considered ? Bonus points to anyone who > > would fix cci_update_bits() :-) > > I like the idea of moving this to v4l2-cci. > > I'd prefer _Generic() based solution as we'd have exact types there instead > of just size. E.g. with the above code, reading a value to a long variable > would work on some archs but fail on others. Doesn't _Generic() treat compatible types identically ? -- Regards, Laurent Pinchart