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
