On Mon, Sep 18, 2023 at 01:59:03AM -0500, Jay Monkman wrote:
>
> This adds GPIO support and devicetree configuration for
> the NCN26000 PHY.
Please Cc: the GPIO maintainers. They have specialist knowledge that
netdev reviews like me don't have. You probably want to separate this
out into a patch of its own, since you don't want to be spamming GPIO
people with a MAC driver etc.
> +// clause 45 vendor specific registers
> +#define NCN26000_REG_PHYCFG1_MMD MDIO_MMD_VEND2
Please use MDIO_MMD_VEND2 directly, so it is clear you are in vendor
space.
> +static int ncn26000_gpio_request(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct ncn26000_priv *priv = gpiochip_get_data(gc);
> +
> + if (offset > 2)
> + return -ENODEV;
Can that happen? I would expect the GPIO core to perform this
validation?
> +
> + if (priv->gpiomask & (1 << offset))
> + return 0;
> +
> + return -EBUSY;
Is this function even needed? All it seems to do is validation. No
resources are actually reserved.
> +static void ncn26000_gpio_set(struct gpio_chip *gc, unsigned int offset, int val)
> +{
> + struct ncn26000_priv *priv = gpiochip_get_data(gc);
> + u32 dio_reg;
> +
> + dio_reg = phy_read(priv->phydev, NCN26000_REG_DIO_CONFIG);
> +
> + switch (offset) {
> + case 0:
> + if (!val == !(priv->diocfg & NCN26000_DIO_CFG_VAL0))
> + dio_reg |= NCN26000_DIO_CFG_VAL0;
> + else
> + dio_reg &= ~NCN26000_DIO_CFG_VAL0;
> + break;
> +
> + case 1:
> + if (!val == !(priv->diocfg & NCN26000_DIO_CFG_VAL1))
> + dio_reg |= NCN26000_DIO_CFG_VAL1;
> + else
> + dio_reg &= ~NCN26000_DIO_CFG_VAL1;
> + break;
> +
> + default:
> + dev_err(priv->dev, "invalid GPIO offset: %d\n", offset);
> + return;
> + }
> +
> + phy_write(priv->phydev, NCN26000_REG_DIO_CONFIG, dio_reg);
You are doing a read/modify/write here. How does locking work?
> +static int ncn26000_gpio_get_dir(struct gpio_chip *gc, unsigned int offset)
> +{
> + return GPIO_LINE_DIRECTION_OUT;
> +}
So they are all GPO? No GPI or GPIO?
> +static int ncn26000_gpio_setup(struct ncn26000_priv *priv)
> +{
> + struct gpio_chip *gc = &priv->gpio_chip;
> +
> + gc->request = ncn26000_gpio_request;
> + gc->get_direction = ncn26000_gpio_get_dir;
> + gc->direction_output = ncn26000_gpio_dir_out;
> + gc->set = ncn26000_gpio_set;
> + gc->label = "ncn26000-gpio";
> + gc->base = -1;
> + gc->ngpio = 2;
> + gc->parent = priv->dev;
> + gc->owner = THIS_MODULE;
> +
> + return devm_gpiochip_add_data(priv->dev, gc, priv);
> +}
Am i right in saying that the rest of this patch has nothing to do
with GPIOs? Please split it up into multiple patches.
Andrew
