From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f42.google.com (mail-wm1-f42.google.com [209.85.128.42]) (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 53AD22D2397 for ; Fri, 7 Aug 2026 14:13:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.42 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786112032; cv=none; b=dfvwVEcUmBYm8j0Id87pXa0ztfkb+k77+Z/hgvejImxfzqhwkmcxg6uSKVIvvyy0oGN2M708raXYp0Kbf/DePtQboOpMeXXY/a9aAkRIJOawU9fnwsJqlltS5EUyk3A7UjCgSIlHOt1H7EPP8+j0tgsTpBk5VoXS+yGUJMVGrDk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786112032; c=relaxed/simple; bh=kCA1jFUs/F/1JO18iKS1aOLmiiVPX0lSRAkM0wfJOR4=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=qbZZBnBnXqnK5AJi6E8WzsMMz4IDJZL7pCfBwY9ckwdWp3aRI6peE7JWy8cN7nl9Sy+xoxy60lxsSCDdHcyRRc8Cxi6BQd3jFFV6ToUyledUVbASc1OHNVhbDW9UnTHT8ut77w6zB/BgWtMAnsbdk/yXR1bGrDFQe2I2LOuZlJk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=GjO7C8cV; arc=none smtp.client-ip=209.85.128.42 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="GjO7C8cV" Received: by mail-wm1-f42.google.com with SMTP id 5b1f17b1804b1-4954a9e8490so12637545e9.1 for ; Fri, 07 Aug 2026 07:13:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1786112021; x=1786716821; darn=vger.kernel.org; h=content-transfer-encoding:content-type:mime-version:references :in-reply-to:message-id:subject:cc:to:from:date:from:to:cc:subject :date:message-id:reply-to:content-type; bh=PIC/yQMkKY4H2MGbW0WBhcLjDZ+KebbmRzTFDny1IYM=; b=GjO7C8cVuTBsz1klpUlOUSlwF0MPPJnycvLlH5PVPhXRD2k7oKsbwM2PEjIsWnn/gd Nyk63F8NF0nYq1h9VmxU7PO43QED6JqfCAwnSMoy8mrRHtDXo+w/RB1i0Hr/WxGIqBrQ Ps/TJLEPf0Wtoi7gMsnHh48sjDLwi57AS6AKjqwepXW7f6onUyNf6dMbcA+tZDeHg1wp 5lXDomqY7PAoPqQRPDCGvKEqN7OKvIyXt03esnUi4ISGaXhEC2cjPOao8EzaS3Bz95KK Hjp/UwwiSK2AsLomZNJ6+oPT6ADZWL0OQLw3C1NvcQxwTZOZc/xpTet2MKZbm21A7Mi2 BOEg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1786112021; x=1786716821; h=content-transfer-encoding:content-type:mime-version:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=PIC/yQMkKY4H2MGbW0WBhcLjDZ+KebbmRzTFDny1IYM=; b=bbd2zh6ugsLjKIji53rva2D3ZIeVH3ZN6CL3z60pYzOrmXKf/Bgo8ZxJE64nAc/YA9 JmSzzQSUX3/K6E6hm6Xrih0bvhNOT+R3X1u8XMlX2+xQko15TFTOsqjPcmj7fRvP+xJj j282mST2AmcLgiu3lG61kbnyqPP0a4Ud6tAh+qdHagD6lcpFJHYqhT6PP/+Y7AMqL4dG ErpDHgBaXaLdc7U14kPRwz09jQKCOkrbhq/yC/4ZvLJ3e9Q5Zt0AMqEN8+ojetyEzln3 IXZINF34u2SC/UU6PK32nJ+TgwmsOKYJaZio4AltRKcd9pdHmAMBKHUlpagbltVD8E9Y HCeg== X-Forwarded-Encrypted: i=1; AHgh+RrFa7XVnqCNWKcNWz9g8tNUrZbK+LI6RMlWfMVdjr3rqNT+pR+0hB1hL8yVHpN2i+RWgvVcVLdVyI7g+iM=@vger.kernel.org X-Gm-Message-State: AOJu0Yxr83qHRnfoyX2AfTrZXFhHW8O8wbAZU8JjE4UBxPhHKf928nKm tDSE6b9IMGHw8AATUpEF6ypdQOSUj7b5+1DPqP1Dl9usAtM/PSd5RO2j X-Gm-Gg: AR+sD12Cu9ynJD92W+Y83aU7OhDsO0R+xuzmR2YDL6FjCZU1O/CNrynbayII7OhtKPx wxfWFuZEye3dISxYXnrgtfKFFgbAwe2NARXt30sySXbDyxyiQtuTHQZ2WvgoxPVbPMcFyBDSx8T FtFFnRUlQDcfPeeK2soAFBFh2dqnxjQGrW0452dUj3ceca+nSam2EZlANrkO4vQX5A+9JJsQqUN T2hgW7FmxZMoIdILqddmkc8PwOuP9nsguQp3VnQIagrzVvCq7JglDFTWkCwPQjItZyo5MUVF6lT RX4VovB9NU1STu5BsQwklhNTk6rKZ83XKS6iXB11LCY5zlc5yB6CYsY8NwHKOlAS4o59bQ0DbPD xCOci1fFE9poJ/kVwXeqQsb3PgbJJixXj5cRRxmCRR6/TTEjKXp2Yt2ro8Dj3ikBVhM7XAmxu73 k/n1T+1RsYHelL1F6YN0YB88FzyDsU/KG+3g0OgFO6xps0+pzoAlhhmhhxgsj8rPtaC734KUunx rTeDi3mBTAbY1bY3Htr1FWDTevTltiH8zSpOcJHqL1InaAUhuL+MeU8VC7spJARq+0Dw5t0PYk1 yJ6uN1s2sfHO7RoTz82+5OxnmBWGTFPEE3hn6RsvmcdPQpPeAlg16O40XDcR5iMvlSc421MfSE/ jnbSUuDOAklWgMKhvRrP6djc2KZ0cZZEpRwL7o/YSdWgHSIlR6Dh6fLk5Y+bJhA9Qp2b3rIE= X-Received: by 2002:a05:600c:a41:b0:499:5f80:83ac with SMTP id 5b1f17b1804b1-49961979a20mr273875e9.7.1786112020910; Fri, 07 Aug 2026 07:13:40 -0700 (PDT) Received: from localhost (90-182-112-124.rcp.o2.cz. [90.182.112.124]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4995427a244sm148058995e9.10.2026.08.07.07.13.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 07 Aug 2026 07:13:40 -0700 (PDT) Date: Fri, 7 Aug 2026 16:13:38 +0200 From: Joshua Crofts To: Kanak Shilledar Cc: "dlechner@baylibre.com" , Henrik Grimler , "nuno.sa@analog.com" , "jean-baptiste.maneyrol@tdk.com" , "robh@kernel.org" , "jic23@kernel.org" , "andy@kernel.org" , "krzk+dt@kernel.org" , "linux-iio@vger.kernel.org" , "conor+dt@kernel.org" , Kernel , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" Subject: Re: [PATCH 2/3] iio: accel: Add support for ICM42370P Message-ID: <20260807161338.00004f48@gmail.com> In-Reply-To: References: <20260806-b4-inv_icm42370p-v1-0-670837f5842f@axis.com> <20260806-b4-inv_icm42370p-v1-2-670837f5842f@axis.com> <20260807120937.00004e3c@gmail.com> X-Mailer: Claws Mail 4.4.0 (GTK 3.24.51; x86_64-w64-mingw32) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On Fri, 7 Aug 2026 13:41:04 +0000 Kanak Shilledar wrote: > Hi Joshua, >=20 > On Fri, 2026-08-07 at 12:09 +0200, Joshua Crofts wrote: > > [You don't often get email from joshua.crofts1@gmail.com. Learn why > > this is important at https://aka.ms/LearnAboutSenderIdentification=A0] > >=20 > > On Thu, 6 Aug 2026 14:46:28 +0200 > > Kanak Shilledar wrote: > > =20 > > > Add support for the Invensense ICM42370P MEMS MotionTracking 3-axis > > > accelerometer with a built-in temperature sensor. Compared to other > > > sensors from the same vendor ICM42370 uses a different way of > > > handling > > > register banks. Although the device supports I2C, SPI, and I3C, > > > implement only I2C support.=A0 Provide basic support for raw sensor > > > reads and a sysfs interface for setting the calibration bias. Keep > > > the > > > embedded temperature sensor enabled because the device design does > > > not > > > allow it to be turned off. > > >=20 > > > Signed-off-by: Kanak Shilledar > > > --- =20 > >=20 > > Hi Kanak, > >=20 > > my comments inline. This is an large driver and I've probably missed > > something. Additionally, Sashiko had some pretty good remarks about > > the scaling math etc. so please check out those: =20 >=20 >=20 > Thanks for the detailed review. >=20 > >=20 > > https://sashiko.dev/#/patchset/20260806-b4-inv_icm42370p-v1-0-670837f58= 42f%40axis.com =20 >=20 > I am going through the sashiko's comments and incorporating them in my > v2. >=20 > > Josh > > =20 > > > +config INV_ICM42370 > > > +=A0=A0=A0=A0 tristate > > > +=A0=A0=A0=A0 select IIO_BUFFER > > > +=A0=A0=A0=A0 select IIO_INV_SENSORS_TIMESTAMP > > > + > > > +config INV_ICM42370_I2C > > > +=A0=A0=A0=A0 tristate "InvenSense ICM-42370 I2C driver" > > > +=A0=A0=A0=A0 depends on I2C > > > +=A0=A0=A0=A0 select INV_ICM42370 > > > +=A0=A0=A0=A0 select REGMAP_I2C > > > +=A0=A0=A0=A0 help > > > +=A0=A0=A0=A0=A0=A0 This driver supports the InvenSense ICM-42730 mot= ion > > > tracking > > > +=A0=A0=A0=A0=A0=A0 devices over I2C. > > > + > > > +=A0=A0=A0=A0=A0=A0 This driver can be built as a module. The module = will be > > > called > > > +=A0=A0=A0=A0=A0=A0 inv_icm42370_i2c. > > > + > > > =A0config KXSD9 > > > =A0=A0=A0=A0=A0 tristate "Kionix KXSD9 Accelerometer Driver" > > > =A0=A0=A0=A0=A0 select IIO_BUFFER > > > diff --git a/drivers/iio/accel/Makefile > > > b/drivers/iio/accel/Makefile > > > index fa440a8592839..6750b03edf518 100644 > > > --- a/drivers/iio/accel/Makefile > > > +++ b/drivers/iio/accel/Makefile > > > @@ -49,6 +49,11 @@ obj-$(CONFIG_HID_SENSOR_ACCEL_3D) +=3D hid-sensor- > > > accel-3d.o > > > =A0obj-$(CONFIG_IIO_KX022A)=A0=A0=A0=A0 +=3D kionix-kx022a.o > > > =A0obj-$(CONFIG_IIO_KX022A_I2C) +=3D kionix-kx022a-i2c.o > > > =A0obj-$(CONFIG_IIO_KX022A_SPI) +=3D kionix-kx022a-spi.o > > > + > > > +obj-$(CONFIG_INV_ICM42370) +=3D inv-icm42370.o > > > +inv-icm42370-y +=3D inv_icm42370_core.o > > > +obj-$(CONFIG_INV_ICM42370_I2C) +=3D inv_icm42370_i2c.o > > > + > > > =A0obj-$(CONFIG_KXCJK1013) +=3D kxcjk-1013.o > > > =A0obj-$(CONFIG_KXSD9)=A0 +=3D kxsd9.o > > > =A0obj-$(CONFIG_KXSD9_SPI)=A0=A0=A0=A0=A0 +=3D kxsd9-spi.o > > > diff --git a/drivers/iio/accel/inv_icm42370.h > > > b/drivers/iio/accel/inv_icm42370.h > > > new file mode 100644 > > > index 0000000000000..9866a5e970dcd > > > --- /dev/null > > > +++ b/drivers/iio/accel/inv_icm42370.h > > > @@ -0,0 +1,365 @@ > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > +/* > > > + * Copyright (C) 2020 Invensense, Inc. > > > + * Copyright (C) 2026 Axis Communications AB > > > + */ > > > + > > > +#ifndef INV_ICM42370_H_ > > > +#define INV_ICM42370_H_ > > > + > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include =20 > >=20 > > Sort these headers alphabetically. Also you're missing types.h. > > =20 > > > +#include > > > +#include =20 > >=20 > > Group the headers separately (check other drivers in > > IIO > > for reference). =20 >=20 > Will sort and group the headers as per the convention. >=20 > > > + > > > +enum inv_icm42370_chip { > > > +=A0=A0=A0=A0 INV_CHIP_INVALID, > > > +=A0=A0=A0=A0 INV_CHIP_ICM42370, > > > +=A0=A0=A0=A0 INV_CHIP_NB, > > > +}; > > > + > > > +/* sensor configuration struct */ > > > +struct inv_icm42370_conf { > > > +=A0=A0=A0=A0 int mode; > > > +=A0=A0=A0=A0 int fs; > > > +=A0=A0=A0=A0 int odr; > > > +=A0=A0=A0=A0 int filter; > > > +}; > > > + > > > +/** > > > + * struct inv_icm42370_data - driver state variables > > > + * @lock:=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 lock for serializing mult= iple register > > > access. > > > + * @name:=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 chip name. > > > + * @map:=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 regmap pointer. > > > + * @vdd_supply:=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 VDD voltage r= egulator for the chip. > > > + * @vddio_supply:=A0=A0=A0 I/O voltage regulator for the chip. > > > + * @indio_accel:=A0=A0=A0=A0 accelerometer IIO device. > > > + * @sensor_state:=A0=A0=A0 per-sensor state tracking (e.g. power, OD= R). > > > + * @buffer:=A0=A0=A0=A0=A0=A0=A0=A0=A0 buffer for reading data regis= ters, aligned > > > for DMA. > > > + * @accel_calibbias: accelerometer calibration bias for X, Y, and > > > Z axes. > > > + * @fifo:=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 FIFO state and configurat= ion. > > > + * @timestamp:=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 interrupt t= imestamp. > > > + * @chip:=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 chip identifier. > > > + * @conf:=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 chip sensors configuratio= ns. > > > + */ > > > +struct inv_icm42370_data { > > > +=A0=A0=A0=A0 struct mutex lock; > > > +=A0=A0=A0=A0 const char *name; > > > +=A0=A0=A0=A0 struct regmap *map; > > > +=A0=A0=A0=A0 struct regulator *vdd_supply; > > > +=A0=A0=A0=A0 struct regulator *vddio_supply; > > > +=A0=A0=A0=A0 struct iio_dev *indio_accel; =20 > >=20 > > You probably don't need this. =20 > Can you please clarify this comment? As I am using `*indio_accel` in > other places inside inv_icm42370_core.c and in many places in > inv_icm42370_buffer.c. Or are you perhaps referring to the > *sensor_state struct below? Yes, apologies, I meant to come back to this comment but forgot (given the size of the driver :)). Reading the cover letter I see that you've taken inspiration from the icm42600, which is an accelerometer and gyroscop= e, which would warrant two different iio_dev structs. However, this chip is an accelerometer only, meaning you can just pass the one iio_dev struct that you initialize in probe to the *_irq_init() function you have and forg= et about it later. --=20 Kind regards, Joshua Crofts