On Wed, Jul 04, 2018 at 11:39:11AM +0300, Matti Vaittinen wrote:
> On Tue, Jul 03, 2018 at 11:26:00AM +0200, Enric Balletbo Serra wrote:
> > Missatge de Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx> del
> > dia dv., 29 de juny 2018 a les 11:47:
> >
> > Now that you use devm calls and you don't need to unwind things I
> > think is better to use plain returns. So,
> >
> > return -ENOMEM;
>
> I have never really understood why use of gotos in error handling is
> discouraged. Personally I would always choose single point of exit from
> a function when it is simple enough to achieve (like in this case). I've
> written and fixed way too many functions which leak resources or
> accidentally keep a lock when exiting from error branches. But I know
> many colleagues like you who prefer not to have gotos but in place returns
> instead. So I guess I'll leave the final call on this to the one who is
> maintainer for this code. And it is true there is no things to unwind
> now - which does not mean that next updater won't add such. But as I
> said, I know plenty of people share your view - and even though I rather
> maintain code with only one exit the final call is on subsystem maintainer
> here.
Actually, If it was completely my call the probe would look something
like this:
+static int bd71837_i2c_probe(struct i2c_client *i2c,
+ const struct i2c_device_id *id)
+{
+ struct bd71837 *bd71837;
+ struct bd71837_board *board_info;
+ int gpio_intr = 0;
+
+ const char *errstr = "No IRQ configured";
+ int ret = -EINVAL;
+
+ bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
+
+ if (bd71837 == NULL)
+ return -ENOMEM;
+
+ board_info = dev_get_platdata(&i2c->dev);
+
+ if (!board_info)
+ gpio_intr = i2c->irq;
+ else
+ gpio_intr = board_info->gpio_intr;
+
+ if (!gpio_intr)
+ goto err_out;
+
+ i2c_set_clientdata(i2c, bd71837);
+ bd71837->dev = &i2c->dev;
+ bd71837->i2c_client = i2c;
+ bd71837->chip_irq = gpio_intr;
+
+ errstr = "regmap initialization failed";
+
+ bd71837->regmap = devm_regmap_init_i2c(i2c, &bd71837_regmap_config);
+ ret = PTR_ERR(bd71837->regmap);
+ if (IS_ERR(bd71837->regmap))
+ goto err_out;
+
+ errstr = "Read BD71837_REG_DEVICE failed";
+ ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
+ if (ret < 0)
+ goto err_out;
+
+ errstr = "Failed to add irq_chip";
+ ret = devm_regmap_add_irq_chip(&i2c->dev, bd71837->regmap,
+ bd71837->chip_irq, IRQF_ONESHOT, 0,
+ &bd71837_irq_chip, &bd71837->irq_data);
+ if (ret < 0)
+ goto err_out;
+
+ errstr = "Failed to configure button short press timeout";
+ ret = regmap_update_bits(bd71837->regmap,
+ BD71837_REG_PWRONCONFIG0,
+ BD718XX_PWRBTN_PRESS_DURATION_MASK,
+ BD718XX_PWRBTN_SHORT_PRESS_10MS);
+ if (ret < 0)
+ goto err_out;
+
+ /* According to BD71847 datasheet the HW default for long press
+ * detection is 10ms. So lets change it to 10 sec so we can actually
+ * get the short push and allow gracefull shut down
+ */
+ ret = regmap_update_bits(bd71837->regmap,
+ BD71837_REG_PWRONCONFIG1,
+ BD718XX_PWRBTN_PRESS_DURATION_MASK,
+ BD718XX_PWRBTN_LONG_PRESS_10S);
+
+ errstr = "Failed to configure button long press timeout";
+ if (ret < 0)
+ goto err_out;
+
+ btns[0].irq = regmap_irq_get_virq(bd71837->irq_data,
+ BD71837_INT_PWRBTN_S);
+
+ errstr = "Failed to get the IRQ";
+ ret = btns[0].irq;
+ if (btns[0].irq < 0)
+ goto err_out;
+
+ errstr = "Failed to create subdevices";
+ ret = devm_mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
+ bd71837_mfd_cells,
+ ARRAY_SIZE(bd71837_mfd_cells), NULL, 0,
+ regmap_irq_get_domain(bd71837->irq_data));
+ if (ret) {
+err_out:
+ if (errstr)
+ dev_err(&i2c->dev, "%s (%d)\n", errstr, ret);
+ }
+
+ return ret;
+}
What do you think of this? To my eye it is nice. It keeps single point of
exit and introduces only simple if-statements without the need of curly
brackets. And finally the error prints string works as a comment too.
I've seen bunch of constructs like this on the networking side but I
have no idea if this is frowned on this subsystem =) Oh, and probe abowe
is just to illustrate the idea, I did not even try compiling it yet.
Best Regards
Matti Vaittinen
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
