Hi Sean

Just looking at the GPIO handling at the moment.

> +     /* Reset whole chip through gpio pin or
> +      * memory-mapped registers for different
> +      * type of hardware
> +      */
> +     if (priv->mcm) {
> +             regmap_update_bits(priv->ethsys, SYSC_REG_RSTCTRL,
> +                                RESET_MCM, RESET_MCM);
> +             usleep_range(1000, 1100);
> +             regmap_update_bits(priv->ethsys, SYSC_REG_RSTCTRL,
> +                                RESET_MCM, ~RESET_MCM);
> +     } else {
> +             gpio_direction_output(priv->reset, 0);
> +             usleep_range(1000, 1100);
> +             gpio_set_value(priv->reset, 1);
> +     }

....

> +     /* Not MCM that indicates switch works as the remote standalone
> +      * integrated circuit so the GPIO pin would be used to complete
> +      * the reset, otherwise memory-mapped register accessing used
> +      * through syscon provides in the case of MCM.
> +      */
> +     if (!priv->mcm) {
> +             priv->reset = of_get_named_gpio(dn, "mediatek,reset-pin", 0);
> +             if (!gpio_is_valid(priv->reset))
> +                     return priv->reset;
> +
> +             ret = devm_gpio_request_one(&mdiodev->dev,
> +                                         priv->reset, GPIOF_OUT_INIT_LOW,
> +                                         "mediatek,reset-pin");
> +             if (ret < 0) {
> +                     dev_err(&mdiodev->dev,
> +                             "fail to devm_gpio_request reset\n");
> +                     return ret;
> +             }
> +     }

You are not handling the flags part of the GPIO binding. It is better
to use devm_gpiod_ API calls, which will handle the active low flags
for you.

    Andrew

Reply via email to