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
signature.asc
Description: PGP signature
