David Mosberger-Tang <davidm@xxxxxxxxxx> 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
