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

[andy.shevchenko@xxxxxxxxx]

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