Re: [PATCH net-next v7 6/9] net: txgbe: Support GPIO to SFP socket

[andy.shevchenko@gmail.com]

Tue, May 09, 2023 at 10:27:31AM +0800, Jiawen Wu kirjoitti:
> Register GPIO chip and handle GPIO IRQ for SFP socket.

...

> +#include <linux/gpio/consumer.h>

What for?

> +#include <linux/gpio/machine.h>
> +#include <linux/gpio/driver.h>

...

> +static int txgbe_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct wx *wx = gpiochip_get_data(chip);
> +	struct txgbe *txgbe = wx->priv;
> +	int val;
> +
> +	val = rd32m(wx, WX_GPIO_EXT, BIT(offset));
> +
> +	txgbe->gpio_orig &= ~BIT(offset);
> +	txgbe->gpio_orig |= val;

This can use standard pattern in conjunction with simple rd32() call:

	txgbe->gpio_orig = (txgbe->gpio_orig & ~BIT(offset)) | (val & BIT(offset));

otherwise it's not immediately obvious that val can have only one bit set.

> +	return !!(val & BIT(offset));
> +}

...

> +static int txgbe_gpio_direction_out(struct gpio_chip *chip, unsigned int offset,
> +				    int val)
> +{
> +	struct wx *wx = gpiochip_get_data(chip);
> +	u32 mask;
> +
> +	mask = BIT(offset) | BIT(offset - 1);
> +	if (val)
> +		wr32m(wx, WX_GPIO_DR, mask, mask);
> +	else
> +		wr32m(wx, WX_GPIO_DR, mask, 0);
> +
> +	wr32m(wx, WX_GPIO_DDR, BIT(offset), BIT(offset));

Can you explain, what prevents to have this flow to be interleaved by other API
calls, like ->direction_in()? Didn't you missed proper locking schema?

> +	return 0;
> +}

...

> +	switch (type) {
> +	case IRQ_TYPE_EDGE_BOTH:
> +		level |= BIT(hwirq);
> +		break;
> +	case IRQ_TYPE_EDGE_RISING:
> +		level |= BIT(hwirq);
> +		polarity |= BIT(hwirq);
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		level |= BIT(hwirq);

> +		polarity &= ~BIT(hwirq);

This...

> +		break;
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		level &= ~BIT(hwirq);

...and this can be done outside of the switch-case. Then you simply set certain
bits where it's needed.

> +		polarity |= BIT(hwirq);
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		level &= ~BIT(hwirq);
> +		polarity &= ~BIT(hwirq);
> +		break;

default?

> +	}

...

> +	/* workaround for hysteretic gpio interrupts */

GPIO

...

> +	gc->can_sleep = false;

Useless, kzalloc() already sets this to 0.

...

> +	girq->num_parents = 1;
> +	girq->parents = devm_kcalloc(&pdev->dev, 1, sizeof(*girq->parents),

Use girq->num_parents instead of explicit 1 in this call.

> +				     GFP_KERNEL);

Also with 

	struct device *dev = &pdev->dev;

this (and others) can be modified as

	girq->parents = devm_kcalloc(dev, girq->num_parents, sizeof(*girq->parents),

> +	if (!girq->parents)
> +		return -ENOMEM;


...

> +#define TXGBE_PX_MISC_IEN_MASK ( \
> +				TXGBE_PX_MISC_ETH_LKDN | \
> +				TXGBE_PX_MISC_DEV_RST | \
> +				TXGBE_PX_MISC_ETH_EVENT | \
> +				TXGBE_PX_MISC_ETH_LK | \
> +				TXGBE_PX_MISC_ETH_AN | \
> +				TXGBE_PX_MISC_INT_ERR | \
> +				TXGBE_PX_MISC_GPIO)

Wouldn't be better

#define TXGBE_PX_MISC_IEN_MASK				  \
	(TXGBE_PX_MISC_ETH_LKDN | TXGBE_PX_MISC_ETH_LK |  \
	 TXGBE_PX_MISC_ETH_EVENT | TXGBE_PX_MISC_ETH_AN | \
	 TXGBE_PX_MISC_DEV_RST | TXGBE_PX_MISC_INT_ERR |  \
	 TXGBE_PX_MISC_GPIO)

?

-- 
With Best Regards,
Andy Shevchenko