Received: by 2002:a05:7208:9594:b0:7e:5202:c8b4 with SMTP id gs20csp786416rbb; Sun, 25 Feb 2024 04:07:24 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCXvQbqrqMUhmUVm1M9vhFoUeEbRuAHQFAkdv0nojQNeRNpIxlBs0CGuwmKcFQZYMNX8CgVmBX402eAFQUhAa87NJN6wRsHBd9ZFNT1yWQ== X-Google-Smtp-Source: AGHT+IG2dkPWthmQKnbxjeXQd15MduTbfrCoj5UVpsODVExnsUiI7ArqVeMZyLuFw6k/JTkzp5Xc X-Received: by 2002:a17:90a:128d:b0:29a:8398:b0d6 with SMTP id g13-20020a17090a128d00b0029a8398b0d6mr3561423pja.9.1708862843756; Sun, 25 Feb 2024 04:07:23 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1708862843; cv=pass; d=google.com; s=arc-20160816; b=ppYPhJBeds8u2c9AbSJUO+N1s5MKyNzLWVJv+J32zCULU8FqbYm2AJHEufMgEZlIj8 EUVMw2br1xcBpp1zJmD4wJ55FwBMJWV+uuIOTkXW2HxborpFAaeRPxm4kD8mKqnyeafs PPl3vYgUasWT7/h2cDMwlAKupoiT6VGcYOaEuJvCKkW7NgZhhvyV/JSlcSdj2mERCFEs A9QgrohDDzkwVAafF1AG6CnH7rO6BfJi8mDstjb50pAUXEEtoAsFQJxbWGVrv0UAgXqj g3pvr8d1aqY+NdhSCzsMGpzSOYVEfH6DD5cisPK01pys//Y3NdMTLmVv+Q1l21AsR4IL 77Ng== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:in-reply-to:message-id :subject:cc:to:from:date:dkim-signature; bh=VgdjxHTKMDdB6FhlRt13FMWEWVg7NEm7uvS0r8/f43k=; fh=U1jYxVsb6iPjzk5EciiHwdMk3+EzqeaWQrykDM5qbn8=; b=EnFyw/tCZ981Rcxa/SLxJ2C23AF7ZkkprXV9RL4a1rfqzpRO+BVt+A07gp+R45aypl dE3eJiWxTh+1i9x8Uk3sGdLsTV92qK3fBs5wkL0F9T4CwwIGvWtJUsVuYh8F9a67Xriu vmrfeVuPKDRzrbF/KbqR/Ehm8ZOH2limka+beyrFEZTKvyWAIH+nc+iCe4912Des+jnV S/McpIgXOVObl9p+m9ZbAO5ZGaFHj3ilpSovu16PmqTM6y9ZWLX8ghs69c/zF/a3wa0x gxy5MvSVUnjLQS598epDlb7BR94CPTFKKa3mlGUO3Fg3Aj3i9ooIIM7uDfDbRFgp8X5r ZRsw==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=EJXoU2xi; arc=pass (i=1 dkim=pass dkdomain=kernel.org); spf=pass (google.com: domain of linux-kernel+bounces-80053-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-80053-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from sv.mirrors.kernel.org (sv.mirrors.kernel.org. [2604:1380:45e3:2400::1]) by mx.google.com with ESMTPS id pc1-20020a17090b3b8100b0029ab7349999si534374pjb.49.2024.02.25.04.07.23 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 25 Feb 2024 04:07:23 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-80053-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=EJXoU2xi; arc=pass (i=1 dkim=pass dkdomain=kernel.org); spf=pass (google.com: domain of linux-kernel+bounces-80053-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-80053-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sv.mirrors.kernel.org (Postfix) with ESMTPS id 6B9AB281C36 for ; Sun, 25 Feb 2024 12:07:23 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 7F90A10A20; Sun, 25 Feb 2024 12:07:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="EJXoU2xi" Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 9874610949; Sun, 25 Feb 2024 12:07:15 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708862835; cv=none; b=OJNiT+CuFMk7RHSWVlRYwhTdjjrXM5AOfa3OGZmMIrS++gRpQAP7SReyezY5exhk37rdIKqvW91BXFqAmTU3mCOdfK2XFLlnDq0thLquzMVF27QuyanpcLybZsEHdAOfPU6zxzE88BBbhsogwtLOTV4xsZ40irk88wNoyWcge7o= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708862835; c=relaxed/simple; bh=6w8hKlkd2Icuszb9wgQIsvTb5JZNofQSVKO3TItRBrc=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=ePrFLVMoaOwfMj9HsII3NsD3TAsUqmNJd7Sw+zCHyFUZpmzQHUmcDuF7Up1A7iVBYYOI10Q/epmJSbUcqm+Apjp4ngUUctOTokVMJtNjhbn2mxz3vsZy/F6c1KyQEA/WchEk6WvUIjMQk23MzQDv0FIdCcIymsQIX5iRbOcqVwE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=EJXoU2xi; arc=none smtp.client-ip=10.30.226.201 Received: by smtp.kernel.org (Postfix) with ESMTPSA id 35BCBC433C7; Sun, 25 Feb 2024 12:07:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1708862835; bh=6w8hKlkd2Icuszb9wgQIsvTb5JZNofQSVKO3TItRBrc=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=EJXoU2xiLzpCI2NIi2eQFQ65EmDKjEvzkVpmAIuwKKE1n86CFEMAVWlfaMT3hmxka uYtIav0sXdtd1L2zffo+OZOtezUuYpOHfmrTcGdqUdGSBshQsqHxieEvkZJMVghoBg Ak9J6RTCcl4B2gqilL3KNZSLeFiQer/50hK2SjWnTC65NWfJvin7JiEd/n47K2lVNV latp4oEawsqTk8K3PTDiCrmzkG+bLV2bZSOansZSIMW8tbibJg9pdrK2kkDEOk5pXi cLFEkhIFqq+9JG6/rB4WURWcMmyZrR58kEmiZO5fR7XV1IOEKe5E6TdQZRxi2I4gM2 K7ifCiWqIkC+Q== Date: Sun, 25 Feb 2024 12:07:00 +0000 From: Jonathan Cameron To: =?UTF-8?B?T25kxZllag==?= Jirman Cc: linux-kernel@vger.kernel.org, Lars-Peter Clausen , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Andrey Skvortsov , Icenowy Zheng , Dalton Durst , Shoji Keita , linux-iio@vger.kernel.org, devicetree@vger.kernel.org Subject: Re: [PATCH v4 3/4] iio: magnetometer: add a driver for Voltafield AF8133J magnetometer Message-ID: <20240225120700.2a0da3f6@jic23-huawei> In-Reply-To: <20240222011341.3232645-4-megi@xff.cz> References: <20240222011341.3232645-1-megi@xff.cz> <20240222011341.3232645-4-megi@xff.cz> X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) 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=UTF-8 Content-Transfer-Encoding: quoted-printable On Thu, 22 Feb 2024 02:13:37 +0100 Ond=C5=99ej Jirman wrote: > From: Icenowy Zheng >=20 > AF8133J is a simple I2C-connected magnetometer, without interrupts. >=20 > Add a simple IIO driver for it. >=20 > Co-developed-by: Icenowy Zheng > Signed-off-by: Icenowy Zheng Check patch correct moaned that Icenowy is the author (from:) so doesn't need a co-developed. > Signed-off-by: Dalton Durst > Signed-off-by: Shoji Keita > Co-developed-by: Ondrej Jirman > Signed-off-by: Ondrej Jirman > Reviewed-by: Andrey Skvortsov > Tested-by: Andrey Skvortsov Hi. A few really minor things noticed during a final review. I'll tweak them whilst applying. Diff is diff --git a/drivers/iio/magnetometer/af8133j.c b/drivers/iio/magnetometer/= af8133j.c index c75d545e152b..40657a08ce37 100644 --- a/drivers/iio/magnetometer/af8133j.c +++ b/drivers/iio/magnetometer/af8133j.c @@ -40,6 +40,10 @@ static const char * const af8133j_supply_names[] =3D { struct af8133j_data { struct i2c_client *client; struct regmap *regmap; + /* + * Protect device internal state between starting a measurement + * and reading the result. + */ struct mutex mutex; struct iio_mount_matrix orientation; =20 @@ -107,8 +111,8 @@ static int af8133j_product_check(struct af8133j_data *d= ata) } =20 if (val !=3D AF8133J_REG_PCODE_VAL) { - dev_err(dev, "Invalid product code (0x%02x)\n", val); - return -EINVAL; + dev_warn(dev, "Invalid product code (0x%02x)\n", val); + return 0; /* Allow unknown ID so fallback compatibles work = */ } =20 return 0; @@ -237,8 +241,8 @@ static int af8133j_read_measurement(struct af8133j_data= *data, __le16 buf[3]) } =20 static const int af8133j_scales[][2] =3D { - [0] =3D { 0, 366210 }, // 12 gauss - [1] =3D { 0, 671386 }, // 22 gauss + [0] =3D { 0, 366210 }, /* 12 gauss */ + [1] =3D { 0, 671386 }, /* 22 gauss */ }; > diff --git a/drivers/iio/magnetometer/af8133j.c b/drivers/iio/magnetomete= r/af8133j.c > new file mode 100644 > index 000000000000..c75d545e152b > --- /dev/null > +++ b/drivers/iio/magnetometer/af8133j.c > + > +struct af8133j_data { > + struct i2c_client *client; > + struct regmap *regmap; > + struct mutex mutex; I thought checkpatch still moaned that every lock should have documentation. I guess not. However it's still a nice to have. Here it seems this is about protecting device state between triggering a measurement and getting the reading. > + struct iio_mount_matrix orientation; > + > + struct gpio_desc *reset_gpiod; > + struct regulator_bulk_data supplies[ARRAY_SIZE(af8133j_supply_names)]; > + > + u8 range; > +}; > + > +static int af8133j_product_check(struct af8133j_data *data) > +{ > + struct device *dev =3D &data->client->dev; > + unsigned int val; > + int ret; > + > + ret =3D regmap_read(data->regmap, AF8133J_REG_PCODE, &val); > + if (ret) { > + dev_err(dev, "Error reading product code (%d)\n", ret); > + return ret; > + } > + > + if (val !=3D AF8133J_REG_PCODE_VAL) { > + dev_err(dev, "Invalid product code (0x%02x)\n", val); > + return -EINVAL; This should be warn only and we should carry on regardless. The reason behind this is to support fallback compatible values in DT to potentially e= nable a newer device to be supported on an older kernel. Many IIO drivers do this wrong as my understanding on what counted on 'compatible' used to be different. Long discussions on this with the DT maintainers led me to accept that letting ID checks fail was fine, but that a message was appropriate. Often a fail here actually means no devic= e. We have some exceptions to this rule for devices where we know the same FW ids are in use in the wild for devices supported by different Linux drivers - but those are thankfully rare! > + } > + > + return 0; > +} > + } > +static const int af8133j_scales[][2] =3D { > + [0] =3D { 0, 366210 }, // 12 gauss > + [1] =3D { 0, 671386 }, // 22 gauss Trivial so I'll fix it up: IIO comments are /* */ not C++ style (with exception of the SPDX stuff that needs to be). > +};