On Thu, Jan 21, 2021 at 4:32 PM Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:
>
> Add simple GPIO base pulse counter. This device is used to measure
> rotation speed of some agricultural devices, so no high frequency on the
> counter pin is expected.
>
> The maximal measurement frequency depends on the CPU and system load. On
> the idle iMX6S I was able to measure up to 20kHz without count drops.
...
> +#include <linux/of_gpio.h>
It would be better to see OF agnostic code WRT GPIOs.
...
> +static ssize_t gpio_pulse_count_enable_read(struct counter_device *counter,
> + struct counter_count *count,
> + void *private, char *buf)
> +{
> + struct gpio_pulse_priv *priv = counter->priv;
> +
> + return scnprintf(buf, PAGE_SIZE, "%d\n", priv->enabled);
sysfs_emit()
> +}
> +
> +static ssize_t gpio_pulse_count_enable_write(struct counter_device *counter,
> + struct counter_count *count,
> + void *private,
> + const char *buf, size_t len)
> +{
> + struct gpio_pulse_priv *priv = counter->priv;
> + bool enable;
> + ssize_t ret;
> +
> + ret = kstrtobool(buf, &enable);
> + if (ret)
> + return ret;
> +
> + if (priv->enabled == enable)
> + return len;
> +
> + if (enable)
> + enable_irq(priv->irq);
> + else
> + disable_irq(priv->irq);
Oops, if IRQ happens here, shouldn't we have priv->enabled already set properly?
> + priv->enabled = enable;
> +
> + return len;
> +}
> +
> +static const struct counter_count_ext gpio_pulse_count_ext[] = {
> + {
> + .name = "enable",
> + .read = gpio_pulse_count_enable_read,
> + .write = gpio_pulse_count_enable_write
Leave the comma here.
> + },
> +};
...
> +static struct counter_signal gpio_pulse_signals[] = {
> + {
> + .id = 0,
> + .name = "Channel 0 signal"
Leave the comma.
> + },
> +};
> +
> +static struct counter_synapse gpio_pulse_count_synapses[] = {
> + {
> + .actions_list = gpio_pulse_synapse_actions,
> + .num_actions = ARRAY_SIZE(gpio_pulse_synapse_actions),
> + .signal = &gpio_pulse_signals[0]
Ditto.
> + },
> +};
...
> +static struct counter_count gpio_pulse_counts[] = {
> + {
> + .id = 0,
> + .name = "Channel 1 Count",
> + .functions_list = gpio_pulse_count_functions,
> + .num_functions = ARRAY_SIZE(gpio_pulse_count_functions),
> + .synapses = gpio_pulse_count_synapses,
> + .num_synapses = ARRAY_SIZE(gpio_pulse_count_synapses),
> + .ext = gpio_pulse_count_ext,
> + .num_ext = ARRAY_SIZE(gpio_pulse_count_ext)
Ditto
> + },
> +};
> +
...
> + struct device_node *np = pdev->dev.of_node;
> + if (of_gpio_count(np) != 1) {
> + dev_err(dev, "Error, need exactly 1 gpio for device\n");
> + return -EINVAL;
> + }
gpiod_count() ?
> + priv->gpio = devm_fwnode_gpiod_get(dev, of_fwnode_handle(np),
> + NULL, GPIOD_IN, GPIO_PULSE_NAME);
of node to fwnode, can we avoid dragging this here to there?
Why is devm_gpiod_get() followed by a label setting not good enough?
> + if (IS_ERR(priv->gpio))
> + return dev_err_probe(dev, PTR_ERR(priv->gpio), "failed to get gpio\n");
...
> +static const struct of_device_id gpio_pulse_cnt_of_match[] = {
> + { .compatible = "virtual,gpio-pulse-counter", },
> + {},
No comma needed for real terminator entry.
> +};
--
With Best Regards,
Andy Shevchenko
