Hi Stephan,
On Sat, Sep 30, 2023 at 05:28:57PM +0200, Stephan Gerhold wrote:
[...]
> In v2 I have added linux/of.h and linux/mod_devicetable.h, since I'm
> actually using definitions from these two only. Seems like including
> of_device.h is discouraged nowadays, see commit dbce1a7d5dce ("Input:
> Explicitly include correct DT includes").
Apologies for the delayed response and some confusion from my side. What
you have added in v2 is correct, and what I should have suggested in the
first place.
[...]
> > Nit: it would still be nice to preserve as many return values as possible, perhaps
> > as follows:
> >
> > +exit_test_mode:
> > error = i2c_smbus_write_byte_data(...) ? : error;
> >
> > > +power_off:
> > > + hx852x_power_off(hx);
> > > + return error;
> >
> > Similarly, with hx852x_power_off() being promoted to int as suggested above,
> > this could be:
> >
> > return hx852x_power_off(...) ? : error;
> >
> > There are other idiomatic ways to do the same thing based on your preference.
> > Another (perhaps more clear) option would be to move some of these test mode
> > functions into a helper, which would also avoid some goto statements.
> >
>
> I played with this for a bit. A problem of the "? : error" approach is
> that it hides the original error in case the new calls error again.
That's correct; good catch.
>
> Let's assume
>
> error = hx852x_start(hx);
> if (error)
> goto power_off;
>
> fails with error = -ENXIO. We jump to power_off:
>
> power_off:
> return hx852x_power_off(hx) ? : error;
>
> Let's say for whatever reason hx852x_power_off() fails too but returns
> -EINVAL. Then the final return value will be -EINVAL, while with the
> current approach in this patch it would return the original cause
> (-ENXIO). I think that's more clear.
>
> I also played with moving code to a separate function to avoid the
> gotos, but I feel like that makes the fairly focused logic of this
> function (reading the configuration by temporarily entering the test
> mode) just more confusing.
>
> To still fix the error handling I ended up with duplicating the
> "success" code path and the "error" code path (it's just two function
> calls), i.e.:
>
> error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
> if (error)
> goto err_power_off;
>
> return hx852x_power_off(hx);
>
> err_test_mode:
> i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
> err_power_off:
> hx852x_power_off(hx);
> return error;
>
> I hope that's fine too. A bit ugly maybe but in this case I would prefer
> having the main code path (reading the configuration) clearly readable.
>
> Let me know if you have a better suggestion for these (I'll send v2 in a
> bit so that you can see the full diff there).
Maybe we can massage this just a bit more; I have followed up with another
suggestion in v2.
>
> Thanks!
> Stephan
Kind regards,
Jeff LaBundy
