On Fri, Feb 26, 2021 at 01:14:55PM +0100, Oleksij Rempel wrote: > On Fri, Feb 26, 2021 at 06:45:20PM +0900, William Breathitt Gray wrote: > > On Fri, Feb 26, 2021 at 10:08:30AM +0100, Oleksij Rempel wrote: > > > +static int interrupt_cnt_signal_read(struct counter_device *counter, > > > + struct counter_signal *signal, > > > + enum counter_signal_value *val) > > > +{ > > > + struct interrupt_cnt_priv *priv = counter->priv; > > > + int ret; I forgot about this function. Add a check here to return -EINVAL if we're not dealing with a GPIO: if (!priv->gpio) return -EINVAL; > > > + > > > + ret = gpiod_get_value(priv->gpio); > > > + if (ret < 0) > > > + return ret; > > > + > > > + *val = ret ? COUNTER_SIGNAL_HIGH : COUNTER_SIGNAL_LOW; > > > + > > > + return 0; > > > +} > > > + > > > +static const struct counter_ops interrupt_cnt_ops = { > > > + .action_get = interrupt_cnt_action_get, > > > + .count_read = interrupt_cnt_read, > > > + .count_write = interrupt_cnt_write, > > > + .function_get = interrupt_cnt_function_get, > > > + .signal_read = interrupt_cnt_signal_read, > > > +}; > > > + > > > +static int interrupt_cnt_probe(struct platform_device *pdev) > > > +{ > > > + struct device *dev = &pdev->dev; > > > + struct interrupt_cnt_priv *priv; > > > + int ret; > > > + > > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > > > + if (!priv) > > > + return -ENOMEM; > > > + > > > + priv->irq = platform_get_irq_optional(pdev, 0); > > > + if (priv->irq == -ENXIO) > > > + priv->irq = 0; > > > + else if (priv->irq < 0) > > > + return dev_err_probe(dev, priv->irq, "failed to get IRQ\n"); > > > + > > > + priv->gpio = devm_gpiod_get_optional(dev, NULL, GPIOD_IN); > > > + if (IS_ERR(priv->gpio)) > > > + return dev_err_probe(dev, PTR_ERR(priv->gpio), "failed to get GPIO\n"); > > > + > > > + if (!priv->irq && !priv->gpio) { > > > + dev_err(dev, "IRQ and GPIO are not found. At least one source should be provided\n"); > > > + return -ENODEV; > > > + } > > > + > > > + if (!priv->irq) { > > > + int irq = gpiod_to_irq(priv->gpio); > > > + > > > + if (irq < 0) > > > + return dev_err_probe(dev, irq, "failed to get IRQ from GPIO\n"); > > > + > > > + priv->irq = irq; > > > + } > > > + > > > + if (priv->gpio) { > > > > This if statement can be removed. There's no need to restrict this to > > just GPIO because we're always dealing with an IRQ, so allocate the > > "IRQ #" name unconditionally and set signals/num_signals. > > Your previous suggestion was to no assign signals if there is no gpios. > What should I do? > > Regards, > Oleksij > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | I'm sorry for not being clear. I'm saying there is no need to differentiate here because there will always be a respective IRQ line whether there is a GPIO line or not. So removing the if statement is all you need to do. Instead of: if (priv->gpio) { priv->signals.name = devm_kasprintf(dev, GFP_KERNEL, "IRQ %d", priv->irq); if (!priv->signals.name) return -ENOMEM; priv->counter.signals = &priv->signals; priv->counter.num_signals = 1; } priv->synapses.actions_list = interrupt_cnt_synapse_actionss; priv->synapses.num_actions = ARRAY_SIZE(interrupt_cnt_synapse_actionss); priv->synapses.signal = &priv->signals; ... You can just have those lines execute unconditionally even if there are no gpios: priv->signals.name = devm_kasprintf(dev, GFP_KERNEL, "IRQ %d", priv->irq); if (!priv->signals.name) return -ENOMEM; priv->counter.signals = &priv->signals; priv->counter.num_signals = 1; priv->synapses.actions_list = interrupt_cnt_synapse_actionss; priv->synapses.num_actions = ARRAY_SIZE(interrupt_cnt_synapse_actionss); priv->synapses.signal = &priv->signals; ... William Breathitt Gray
Attachment:
signature.asc
Description: PGP signature