Re: [PATCH v7 2/2] counter: add IRQ or GPIO based counter

[William Breathitt Gray <vilhelm.gray@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4181 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]