Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Mon, 18 Sep 2023 18:18:00 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Jay Monkman <jtm@lopingdog.com>
Cc:     devicetree@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Piergiorgio Beruto <piergiorgio.beruto@gmail.com>,
        Arndt Schuebel <Arndt.Schuebel@onsemi.com>
Subject: Re: [PATCH 3/4] net: phy: Add GPIO and DT support to NCN26000
Message-ID: <4242c63c-eec2-4168-ae59-b4fac8cdea3e@lunn.ch>
References: <ZQf1Nx+2amtUZulX@lopingdog.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZQf1Nx+2amtUZulX@lopingdog.com>
Precedence: bulk

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