On Thu, Jan 21, 2021 at 4:32 PM Oleksij Rempel [off-list ref] 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