Received-SPF: pass (google.com: domain of linux-wireless+bounces-3285-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Kalle Valo <kvalo@kernel.org>
To: David Mosberger-Tang <davidm@egauge.net>
Cc: Alexis =?utf-8?Q?Lothor=C3=A9?= <alexis.lothore@bootlin.com>,
  linux-wireless@vger.kernel.org,  Ajay.Kathat@microchip.com
Subject: Re: [PATCH] [v4] wifi: wilc1000: validate chip id during bus probe
References: <20240127004331.1334804-1-davidm@egauge.net>
	<415a0e6e-5824-44a2-af2a-a75115d5a62e@bootlin.com>
	<706d5fde96f90078f80c7fb4fc88503d1791426f.camel@egauge.net>
Date: Wed, 07 Feb 2024 13:41:01 +0200
In-Reply-To: <706d5fde96f90078f80c7fb4fc88503d1791426f.camel@egauge.net>
	(David Mosberger-Tang's message of "Tue, 06 Feb 2024 21:52:42 -0700")
Message-ID: <871q9oqz76.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

David Mosberger-Tang <davidm@egauge.net> writes:

> On Tue, 2024-01-30 at 10:06 +0100, Alexis Lothor=C3=A9 wrote:
>> On 1/27/24 01:43, David Mosberger-Tang wrote:
>>=20
>>=20
>> > @@ -1142,7 +1170,7 @@ static int wilc_spi_init(struct wilc *wilc, bool=
 resume)
>> >  	}
>> >  	if (ret) {
>> >  		dev_err(&spi->dev, "Failed with CRC7 on and off.\n");
>> > -		return ret;
>> > +		return -ENODEV;
>>=20
>> You are still rewriting error codes here. At a lower level, sure, but st=
ill...
>> When I suggested setting -ENODEV at lower level, I was thinking about pl=
aces
>> where no explicit error code was already in use, but
>> spi_internal_read/spi_internal_write already generate proper error codes=
. Or am
>> I missing a constraint, like the probe chain really needing -ENODEV ?
>
> Lower-level errors are often not meaningful at the higher level.  For
> example, attempting to read a register over SPI may cause a CRC error
> if the device doesn't exist.  That would result in -EINVAL, even though
> there was nothing invalid about the read, it's just that the device
> wasn't there.

Changing the error values makes debugging harder so please avoid doing
it unless absolutely necessary (and then explain the reason in a code
comment).

--=20
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatc=
hes