Received: by 2002:a05:7412:f589:b0:e2:908c:2ebd with SMTP id eh9csp53889rdb; Mon, 30 Oct 2023 23:35:08 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFcxbvaCBt8RkFxhEdgaPd08wUTDTKzij5cFI7Qly3VbToWukq1CuoIn0jInboA+IAuDKrd X-Received: by 2002:a05:6830:270b:b0:6bb:1049:6919 with SMTP id j11-20020a056830270b00b006bb10496919mr16014921otu.12.1698734108169; Mon, 30 Oct 2023 23:35:08 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698734108; cv=none; d=google.com; s=arc-20160816; b=fIAt3ErOoHYDHu29o4IrCtvfRw745iK+SVxGcYMREZhVBjM6h4QSeB10z+Xt9uq78K 0mYC2XDfjEoPW+2UvARA2lwmxDRz49KS93iuIpIPWTPUwxKyp8SfVkFfanUyT4UPdkq2 fpJyo3FRs2iYyiQ3DEt1o8mF1f8S3VpZWtPDdHeXPCuNXqcmkxHvr8Bs8lVQOXqr/LLe RAGYxv491zqoknpGIFA/kCuunLapbGA9Ep+2WVmZJZ/BkMXN+NVhqs1KHuC+42S4M7pM 85lRX7GNVKiNdzaNZVSQxAAVuR7KmVNLL3/uVQ+zvyUaq8+P2RLEl1lNaO3LOCHGvQGL qHuw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=OUPRPSzhKpnIhKkapnk5CqIMimZaAxoKZ7eWTdcDChw=; fh=UW8/FRJc0nvUXYCUzZN1+2G+wUlNUzZ3Qq5myDGD9eg=; b=wgJHMuuFfLJAaECbkg/ffeZT8OEGsaWySlpoDjperrzBGpE1BZBs9noVL3NoW5o7E0 QT0cWOkTWO1uXp+zNPB+x0QPQBJlHiMFtfxldt9e6yWIvlQ0O0VJcMQU7/4bRX8AyLcP 42aeOHtYgL/k6t+hiz8hf0r5DYtXI5SvXVLLhU0LGCR/SltmILBX57eukBK+R97jsU48 1siH417R709+fPsf/9ho1lgFemEy8YDyH/lsEEzPvMppx+okc50iDYRI6CGzkIgjE4n2 NCvXkwTi/4TK65bH0IXE6v8N021Etxkklhmo1ezSTvmlnEY7uBN/FKaEV4pm5MI8IUu3 wilg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=WH8rkCH3; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from lipwig.vger.email (lipwig.vger.email. [23.128.96.33]) by mx.google.com with ESMTPS id l64-20020a639143000000b005898db9d676si579484pge.260.2023.10.30.23.35.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Oct 2023 23:35:08 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=WH8rkCH3; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by lipwig.vger.email (Postfix) with ESMTP id 79EB6803BAA6; Mon, 30 Oct 2023 23:35:05 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at lipwig.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236073AbjJaGev (ORCPT + 99 others); Tue, 31 Oct 2023 02:34:51 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37158 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233629AbjJaGeu (ORCPT ); Tue, 31 Oct 2023 02:34:50 -0400 Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.126]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C51A6B3; Mon, 30 Oct 2023 23:34:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1698734087; x=1730270087; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=xI1KDUbCultnDPUeItxneVNFdyGSh0uPi9N2KqUlUKM=; b=WH8rkCH3/MtpFNhf/a/EDIkM9nwmY2HUlhUrTyAu0MfNCVOVk4BiBjOq BTG/kJcmYFUGtPdhsEu1aoiyC3/VLcdSXh/jmqrJPLxWvBv4KzBiNkBrM lV4MXkyiATeIQuuCshguswrgm/MaDWWHgw77vAFuYXESLDwE/Q+xSgVt6 e/VH9jbOePz/P4CRKPB8zxqtznTPuaDTgBl1ejQZlKYZKR8LD0lzSCCw/ 75XYK0NN8XKdHgd/QihkZMCIBIbfON4z5LDaAi5RUtPmmp9gs427rOeap zlvneq8D/E25zk6O9ZYi9ieWH+JvnuWgYnbrnIBjVkLu+BrOEMh7nRkIl w==; X-IronPort-AV: E=McAfee;i="6600,9927,10879"; a="373275912" X-IronPort-AV: E=Sophos;i="6.03,265,1694761200"; d="scan'208";a="373275912" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Oct 2023 23:34:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10879"; a="904240645" X-IronPort-AV: E=Sophos;i="6.03,265,1694761200"; d="scan'208";a="904240645" Received: from turnipsi.fi.intel.com (HELO kekkonen.fi.intel.com) ([10.237.72.44]) by fmsmga001-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Oct 2023 23:34:42 -0700 Received: from kekkonen.localdomain (localhost [127.0.0.1]) by kekkonen.fi.intel.com (Postfix) with SMTP id 5A51D11F894; Tue, 31 Oct 2023 08:34:39 +0200 (EET) Date: Tue, 31 Oct 2023 06:34:39 +0000 From: Sakari Ailus To: Laurent Pinchart 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: 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=us-ascii Content-Disposition: inline In-Reply-To: <20231030233809.GD12764@pendragon.ideasonboard.com> X-Spam-Status: No, score=-1.2 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Mon, 30 Oct 2023 23:35:05 -0700 (PDT) Hi Laurent, On Tue, Oct 31, 2023 at 01:38:09AM +0200, Laurent Pinchart wrote: > Hello, > > Hans, there's a question for you below. > > On Mon, Oct 30, 2023 at 10:43:47PM +0000, Sakari Ailus wrote: > > Hi Tommaso, > > > > 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. -- Regards, Sakari Ailus