Hi Jeff,
On Mon, Oct 23, 2023 at 07:28:28PM -0500, Jeff LaBundy wrote:
> On Sun, Oct 22, 2023 at 08:49:13PM +0200, Stephan Gerhold wrote:
> > > > +static int hx852x_read_config(struct hx852x *hx)
> > > > +{
> > > > + struct device *dev = &hx->client->dev;
> > > > + struct hx852x_config conf;
> > > > + int x_res, y_res;
> > > > + int error;
> > > > +
> > > > + error = hx852x_power_on(hx);
> > > > + if (error)
> > > > + return error;
> > > > +
> > > > + /* Sensing must be turned on briefly to load the config */
> > > > + error = hx852x_start(hx);
> > > > + if (error)
> > > > + goto err_power_off;
> > > > +
> > > > + error = hx852x_stop(hx);
> > > > + if (error)
> > > > + goto err_power_off;
> > > > +
> > > > + error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH,
> > > > + HX852X_SRAM_SWITCH_TEST_MODE);
> > > > + if (error)
> > > > + goto err_power_off;
> > > > +
> > > > + error = i2c_smbus_write_word_data(hx->client, HX852X_REG_SRAM_ADDR,
> > > > + HX852X_SRAM_ADDR_CONFIG);
> > > > + if (error)
> > > > + goto err_test_mode;
> > > > +
> > > > + error = hx852x_i2c_read(hx, HX852X_REG_FLASH_RPLACE, &conf, sizeof(conf));
> > > > + if (error)
> > > > + goto err_test_mode;
> > > > +
> > > > + x_res = be16_to_cpu(conf.x_res);
> > > > + y_res = be16_to_cpu(conf.y_res);
> > > > + hx->max_fingers = (conf.max_pt & 0xf0) >> 4;
> > > > + dev_dbg(dev, "x res: %u, y res: %u, max fingers: %u\n",
> > > > + x_res, y_res, hx->max_fingers);
> > > > +
> > > > + if (hx->max_fingers > HX852X_MAX_FINGERS) {
> > > > + dev_err(dev, "max supported fingers: %u, found: %u\n",
> > > > + HX852X_MAX_FINGERS, hx->max_fingers);
> > > > + error = -EINVAL;
> > > > + goto err_test_mode;
> > > > + }
> > > > +
> > > > + if (x_res && y_res) {
> > > > + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_X, 0, x_res - 1, 0, 0);
> > > > + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_Y, 0, y_res - 1, 0, 0);
> > > > + }
> > > > +
> > > > + 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;
> > >
> > > Your new version is an improvement, but maybe we can remove duplicate
> > > code by introducing some helper variables:
> > >
> > > int error, error2 = 0, error3;
> > >
> > > /* ... */
> > >
> > > err_test_mode:
> > > error2 = i2c_smbus_write_byte_data(...);
> > >
> > > err_power_off:
> > > error3 = hx852x_power_off(...);
> > >
> > > if (error)
> > > return error;
> > >
> > > return error2 ? : error3;
> > >
> > > In this case we achieve our goal of attempting to return the device to a
> > > safe state in both passing and failing cases. In the event of multiple
> > > errors, we return the first to occur.
> > >
> >
> > Right, this would work as well. Personally I think my solution is
> > slightly easier to read though. In your version my eyes somewhat
> > "stumble" over the multiple "error" variables and then about the purpose
> > of the "? : " construction. This is probably highly subjective. :-)
>
> Agreed, my suggestion is a bit unwieldy, and prone to uninitialized
> variable bugs. However, I feel that duplicate code, especially side
> by side like this, is also confusing and prone to bugs in case the
> sequence must be updated in the future. As a compromise, how about
> something closer to my first idea:
>
> err_test_mode:
> error = i2c_smbus_write_byte_data(...) ? : error;
>
> err_power_off:
> return hx852x_power_off(...) ? : error;
>
> This is nice and compact, and ensures that errors returned by the two
> functions are reported no matter the flow. The only functional change
> is that the _last_ error takes priority; but in practice this does not
> really matter. Normally if one I2C write fails, all subsequent writes
> will fail with the same return code until the hardware is recovered
> somehow.
>
> For the corner case where the code jumps to exit_test_mode with error
> equal to -EINVAL, and i2c_smbus_write_byte_data() then fails and changes
> error to something like -EIO, I don't think we care. After the HW issue
> is fixed and all I2C writes succeed, the developer will then see that
> the number of contacts reported by the FW is invalid anyway :)
>
> Side note: the '? :' syntax is just a shortcut that sets error equal
> to the return value of i2c_smbus_write_byte_data() if true (failure)
> without having to declare a temporary variable. If false (no failure),
> error is assigned to itself. It is perfectly legal.
>
Thanks a lot for your detailed review and explanations!
In v3 I have changed the code to your suggestion above and also
addressed your other comments in the initial reply. :)
Stephan
