Hi Elad,
first of all it's very nice to see code so well documented :-)
Thanks for the effort you put in it.
...
> + /*
> + * Start with arbitration lost soft reset enabled as to false.
> + * Try to locate the necessary items in the device tree to
> + * make this feature work.
> + * Only after we verify that the device tree contains all of
> + * the needed information and that it is sound and usable,
> + * then we enable this flag.
> + * This information should be defined, but the driver maintains
> + * backward compatibility with old dts files, so it will not fail
> + * the probe in case these are missing.
> + */
> + drv_data->soft_reset = false;
> + pc = pinctrl_get(&pd->dev);
I'm not against using devm_pinctrl_get(), in my previous comments
I was questioning whether it should be placed in the probe
function (as you did).
Placed there, iirc, it needed explicit release. Here, I don't
think it does if you use the managed version.
> + if (!IS_ERR(pc)) {
> + drv_data->pc = pc;
> + drv_data->i2c_mpp_state =
> + pinctrl_lookup_state(pc, "default");
> + drv_data->i2c_gpio_state =
> + pinctrl_lookup_state(pc, "gpio");
> + drv_data->scl_gpio =
> + of_get_named_gpio(pd->dev.of_node, "scl-gpios", 0);
> + drv_data->sda_gpio =
> + of_get_named_gpio(pd->dev.of_node, "sda-gpios", 0);
please use devm_gpio_get(...).
> + if (!IS_ERR(drv_data->i2c_gpio_state) &&
> + !IS_ERR(drv_data->i2c_mpp_state) &&
> + gpio_is_valid(drv_data->scl_gpio) &&
> + gpio_is_valid(drv_data->sda_gpio)) {
...
> + if (!IS_ERR(drv_data->pc))
> + pinctrl_put(drv_data->pc);
> + if (drv_data->soft_reset) {
> + devm_gpiod_put(drv_data->adapter.dev.parent,
> + gpio_to_desc(drv_data->scl_gpio));
> + devm_gpiod_put(drv_data->adapter.dev.parent,
> + gpio_to_desc(drv_data->sda_gpio));
if it's managed it doesn't need to be explicitely removed.
Thanks,
Andi
