lkml.org 
[lkml]   [2023]   [Oct]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 2/2] Input: add Himax HX852x(ES) touchscreen driver
    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

    \
     
     \ /
      Last update: 2023-10-24 20:36    [W:4.773 / U:0.196 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site