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:

quoted

On Fri, Feb 26, 2021 at 10:08:30AM +0100, Oleksij Rempel wrote:

quoted

+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;

quoted

quoted

+
+	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