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

andy.shevchenko
2023-05-15T09:45:29Z

Mon, May 15, 2023 at 02:31:57PM +0800, Jiawen Wu kirjoitti:
> Register GPIO chip and handle GPIO IRQ for SFP socket.

...

> +static int txgbe_gpio_direction_out(struct gpio_chip *chip, unsigned int offset,
> + int val)
> +{
> + struct wx *wx = gpiochip_get_data(chip);
> + unsigned long flags;
> + u32 set;
> +
> + spin_lock_irqsave(&wx->gpio_lock, flags);

> + set = val ? BIT(offset) : 0;

Why is this under the lock?

> + wr32m(wx, WX_GPIO_DR, BIT(offset), set);
> + wr32m(wx, WX_GPIO_DDR, BIT(offset), BIT(offset));
> + spin_unlock_irqrestore(&wx->gpio_lock, flags);
> +
> + return 0;
> +}

...

> +static void txgbe_toggle_trigger(struct gpio_chip *gc, unsigned int offset)

> +{
> + struct wx *wx = gpiochip_get_data(gc);
> + u32 pol;
> + int val;
> +
> + pol = rd32(wx, WX_GPIO_POLARITY);

> + val = gc->get(gc, offset);

Can't you use directly the respective rd32()?

> + if (val)
> + pol &= ~BIT(offset);
> + else
> + pol |= BIT(offset);
> +
> + wr32(wx, WX_GPIO_POLARITY, pol);
> +}

...

> +static void txgbe_irq_handler(struct irq_desc *desc)
> +{
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct wx *wx = irq_desc_get_handler_data(desc);
> + struct txgbe *txgbe = wx->priv;
> + irq_hw_number_t hwirq;
> + unsigned long gpioirq;
> + struct gpio_chip *gc;
> +
> + chained_irq_enter(chip, desc);

Seems spin_lock() call is missing in this function.

> + gpioirq = rd32(wx, WX_GPIO_INTSTATUS);
> +
> + gc = txgbe->gpio;
> + for_each_set_bit(hwirq, &gpioirq, gc->ngpio) {
> + int gpio = irq_find_mapping(gc->irq.domain, hwirq);
> + u32 irq_type = irq_get_trigger_type(gpio);
> +
> + generic_handle_irq(gpio);

Can generic_handle_domain_irq() be used here?

> + if ((irq_type & IRQ_TYPE_SENSE_MASK) == IRQ_TYPE_EDGE_BOTH)
> + txgbe_toggle_trigger(gc, hwirq);
> + }
> +
> + chained_irq_exit(chip, desc);
> +
> + /* unmask interrupt */
> + wx_intr_enable(wx, TXGBE_INTR_MISC(wx));
> +}

...

> +static int txgbe_gpio_init(struct txgbe *txgbe)
> +{
> + struct gpio_irq_chip *girq;
> + struct wx *wx = txgbe->wx;
> + struct gpio_chip *gc;
> + struct device *dev;
> + int ret;

> + dev = &wx->pdev->dev;

This can be united with the defintion above.

struct device *dev = &wx->pdev->dev;

> + gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
> + if (!gc)
> + return -ENOMEM;
> +
> + gc->label = devm_kasprintf(dev, GFP_KERNEL, "txgbe_gpio-%x",
> + (wx->pdev->bus->number << 8) | wx->pdev->devfn);
> + gc->base = -1;
> + gc->ngpio = 6;
> + gc->owner = THIS_MODULE;
> + gc->parent = dev;
> + gc->fwnode = software_node_fwnode(txgbe->nodes.group[SWNODE_GPIO]);

Looking at the I²C case, I'm wondering if gpio-regmap can be used for this piece.

> + gc->get = txgbe_gpio_get;
> + gc->get_direction = txgbe_gpio_get_direction;
> + gc->direction_input = txgbe_gpio_direction_in;
> + gc->direction_output = txgbe_gpio_direction_out;
> +
> + girq = &gc->irq;
> + gpio_irq_chip_set_chip(girq, &txgbe_gpio_irq_chip);
> + girq->parent_handler = txgbe_irq_handler;
> + girq->parent_handler_data = wx;
> + girq->num_parents = 1;
> + girq->parents = devm_kcalloc(dev, girq->num_parents,
> + sizeof(*girq->parents), GFP_KERNEL);
> + if (!girq->parents)
> + return -ENOMEM;
> + girq->parents[0] = wx->msix_entries[wx->num_q_vectors].vector;
> + girq->default_type = IRQ_TYPE_NONE;
> + girq->handler = handle_bad_irq;
> +
> + ret = devm_gpiochip_add_data(dev, gc, wx);
> + if (ret)
> + return ret;

> + spin_lock_init(&wx->gpio_lock);

Isn't it too late?

> + txgbe->gpio = gc;
> +
> + return 0;
> +}

--
With Best Regards,
Andy Shevchenko