On 13/10/23 22:34, Andi Shyti wrote: > Hi Chris, > > ... > >> static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = { >> @@ -1083,6 +1084,10 @@ mv64xxx_i2c_probe(struct platform_device *pd) >> if (drv_data->irq < 0) >> return drv_data->irq; >> >> + drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "reset", GPIOD_OUT_HIGH); >> + if (IS_ERR(drv_data->reset_gpio)) >> + return PTR_ERR(drv_data->reset_gpio); >> >> if this optional why are we returning in case of error? >> >> gpiod_get_optional() will return NULL if the property is not present. The main >> error I care about here is -EPROBE_DEFER but I figure other errors are also >> relevant. This same kind of pattern is used in other drivers. > we already discussed about this, I don't have a strong opinion, > you can leave it as it is... I recon this is a matter of pure > taste. I think in this case it would actually make things uglier because I'd have to check for -EPROBE_DEFER. So something like drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "reset", GPIOD_OUT_HIGH); if (IS_ERR(drv_data->reset_gpio) && PTR_ERR(drv_data->reset_gpio) == -EPROBE_DEFER) return PTR_ERR(drv_data->reset_gpio); else drv_data->reset_gpio = NULL; I could probably come up with something less ugly with a local variable or two but nothing as tidy as just returning on error. > > Would you just mind adding an error message using > dev_err_probe()? Yep sure. Will include in the next round.