Re: [PATCH] [v4] wifi: wilc1000: validate chip id during bus probe

[Kalle Valo <kvalo@kernel.org>]

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

> On Tue, 2024-01-30 at 10:06 +0100, Alexis Lothoré wrote:
>> On 1/27/24 01:43, David Mosberger-Tang wrote:
>> 
>> 
>> > @@ -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;
>> 
>> You are still rewriting error codes here. At a lower level, sure, but still...
>> When I suggested setting -ENODEV at lower level, I was thinking about places
>> 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).

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

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches