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
