>>      ret = i2c_smbus_write_byte_data(client, offset, value);
>> -    if (ret < 0) {
>> -            dev_err(ctx->dev, "writebm: %4s[0x%02x] <- 0x%02x\n",
>> -                    sii9234_client_name[id], offset, value);
>> -            ctx->i2c_error = ret;
>> -    }
>> +    if (!ret)
>> +            return 0;
> 
> Ugh.  No.  Don't do success handling on the last if statement.

I find my approach useful in this case.


> Also while I personally prefer testing for non-zero,

I got used to this checking style to some degree.


> the ALSA people got annoyed at you for changing tests for < 0

It seems that involved software developers have got special preferences there.


> but you're doing it again.

I dared to propose such an adjustment once more.
Would you like discuss corresponding reasons any further?


> And it introduces a bug,

Unfortunately, a hiccup in my software development attention …


> although I see now that you fixed it in v2.

Thanks that you noticed also this small update.

https://patchwork.kernel.org/patch/10021767/
https://lkml.kernel.org/r/<2ecf0bb7-7129-40e4-cefc-0bc2d0f7e...@users.sourceforge.net>


> I can't get excited about these sort of risky low value patches.

I try again to point special software improvement opportunities out.


>> +report_failure:
>> +    dev_err(ctx->dev, "writebm: %4s[0x%02x] <- 0x%02x\n",
>> +            sii9234_client_name[id], offset, value);
>> +    ctx->i2c_error = ret;
>>      return ret;
>>  }

How do you think about to move this source code to the end of
this function implementation?

Regards,
Markus
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to