Received-SPF: pass (google.com: domain of linux-kernel+bounces-80317-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Sun, 25 Feb 2024 22:52:59 +0100
From: =?utf-8?Q?Ond=C5=99ej?= Jirman <megi@xff.cz>
To: Jonathan Cameron <jic23@kernel.org>
Cc: linux-kernel@vger.kernel.org, Lars-Peter Clausen <lars@metafoo.de>, 
	Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, 
	Conor Dooley <conor+dt@kernel.org>, Andrey Skvortsov <andrej.skvortzov@gmail.com>, 
	Icenowy Zheng <icenowy@aosc.io>, Dalton Durst <dalton@ubports.com>, 
	Shoji Keita <awaittrot@shjk.jp>, 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: <mn2ewn33djqxmghin73l2exq426sws2ymqvajojctyywvosash@an7dzkftkqvc>
Mail-Followup-To: =?utf-8?Q?Ond=C5=99ej?= Jirman <megi@xff.cz>, 
	Jonathan Cameron <jic23@kernel.org>, linux-kernel@vger.kernel.org, 
	Lars-Peter Clausen <lars@metafoo.de>, Rob Herring <robh+dt@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Andrey Skvortsov <andrej.skvortzov@gmail.com>, Icenowy Zheng <icenowy@aosc.io>, 
	Dalton Durst <dalton@ubports.com>, Shoji Keita <awaittrot@shjk.jp>, linux-iio@vger.kernel.org, 
	devicetree@vger.kernel.org
References: <20240222011341.3232645-1-megi@xff.cz>
 <20240222011341.3232645-4-megi@xff.cz>
 <20240225120700.2a0da3f6@jic23-huawei>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240225120700.2a0da3f6@jic23-huawei>

Hello Jonathan,

On Sun, Feb 25, 2024 at 12:07:00PM +0000, Jonathan Cameron wrote:
> On Thu, 22 Feb 2024 02:13:37 +0100
> Ondřej Jirman <megi@xff.cz> wrote:
> 
> > From: Icenowy Zheng <icenowy@aosc.io>
> > 
> > AF8133J is a simple I2C-connected magnetometer, without interrupts.
> > 
> > Add a simple IIO driver for it.
> > 
> > Co-developed-by: Icenowy Zheng <icenowy@aosc.io>
> > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Check patch correct moaned that Icenowy is the author (from:)
> so doesn't need a co-developed.
> 
> > Signed-off-by: Dalton Durst <dalton@ubports.com>
> > Signed-off-by: Shoji Keita <awaittrot@shjk.jp>
> > Co-developed-by: Ondrej Jirman <megi@xff.cz>
> > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> > Reviewed-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> > Tested-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> 
> Hi.
> 
> A few really minor things noticed during a final review.
> I'll tweak them whilst applying.  Diff is

Thank you very much for finishing touches.

> > +static int af8133j_product_check(struct af8133j_data *data)
> > +{
> > +	struct device *dev = &data->client->dev;
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	ret = regmap_read(data->regmap, AF8133J_REG_PCODE, &val);
> > +	if (ret) {
> > +		dev_err(dev, "Error reading product code (%d)\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	if (val != 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 enable
> 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 device.
> 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!

Makes sense. If newer device variant has the same register meanings, but just
a different ID register, this way it can be supported without driver
modifications. I'll keep it in mind when doing ID checks in other drivers.

Thanks for all your detailed notes during the review. I learned a few
new subtleties.

Kind regards,
	o.

> > +	}
> > +
> > +	return 0;
> > +}
> > +
> }
> 
> > +static const int af8133j_scales[][2] = {
> > +	[0] = { 0, 366210 }, // 12 gauss
> > +	[1] = { 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).
> > +};
> 
> 
>