Re: [PATCH net-next 4/4] net-next: dsa: add dsa support for Mediatek MT7530 switch

Andrew Lunn
2017-03-13T16:41:40Z

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