* [PATCH v7 0/4] PPS: pps-gpio PPS ECHO implementation @ 2018-11-14 12:54 Tom Burkart 2018-11-14 12:54 ` [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition Tom Burkart 0 siblings, 1 reply; 16+ messages in thread From: Tom Burkart @ 2018-11-14 12:54 UTC (permalink / raw) To: Linux kernel mailing list; +Cc: Tom Burkart Hi all, please find attached the PPS-GPIO PPS ECHO implementation patch. The driver claims to have echo functionality in the sysfs interface but this functionality is not present. This patch provides this functionality. Parts 1 and 2 of the patch change the original driver from the number based GPIO ABI to the descriptor based ABI. This has been modified to not break backward compatibility. Parts 3 and 4 then add the PPS ECHO functionality. This is enabled if a "echo-gpios" entry is found in the devicetree. Changes in v4: There is a bugfix in part 2 that was returning an uninitialised variable and juggling some code that speeds up the probe by not running code when not using PPS ECHO. Changes in v5: Meet all requirements of checkpatch Changes in v6: Cosmetic changes to code layout Changes in v7: Also accept the DEPRECATED "gpios" devicetree entry to ensure backward compatibility. On the linuxpps mailing list it was suggested to use a hrtimer for resetting the GPIO ECHO active state to the inactive state. Please also comment on whether a hrtimer is necessary/desirable for the purpose of resetting the echo pin active state. I am happy to implement it if there is a need. Please install, test and comment as it is now a quite major change to the driver. Please do send suggestions for improvement. Tom Burkart Tom Burkart (4): dt-bindings: pps: descriptor-based gpio, capture-clear addition pps: descriptor-based gpio, capture-clear addition dt-bindings: pps: pps-gpio PPS ECHO implementation pps: pps-gpio pps-echo implementation Documentation/devicetree/bindings/pps/pps-gpio.txt | 17 +- drivers/pps/clients/pps-gpio.c | 225 +++++++++++++++++---- include/linux/pps-gpio.h | 6 +- 3 files changed, 203 insertions(+), 45 deletions(-) -- 2.12.3 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition 2018-11-14 12:54 [PATCH v7 0/4] PPS: pps-gpio PPS ECHO implementation Tom Burkart @ 2018-11-14 12:54 ` Tom Burkart 2018-11-14 12:54 ` [PATCH v7 2/4] " Tom Burkart 2018-11-16 22:49 ` [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition Rob Herring 0 siblings, 2 replies; 16+ messages in thread From: Tom Burkart @ 2018-11-14 12:54 UTC (permalink / raw) To: Linux kernel mailing list; +Cc: Tom Burkart, devicetree This patch changes the devicetree bindings for the pps-gpio driver from the integer based ABI to the descriptor based ABI. It also adds documentation for the device tree capture-clear option. The legacy device tree entry for the GPIO pin is supported. Signed-off-by: Tom Burkart <tom@aussec.com> --- Documentation/devicetree/bindings/pps/pps-gpio.txt | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt b/Documentation/devicetree/bindings/pps/pps-gpio.txt index 3683874832ae..6c9fc0998d94 100644 --- a/Documentation/devicetree/bindings/pps/pps-gpio.txt +++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt @@ -5,19 +5,23 @@ a GPIO pin. Required properties: - compatible: should be "pps-gpio" -- gpios: one PPS GPIO in the format described by ../gpio/gpio.txt +- pps-gpios: one PPS GPIO in the format described by ../gpio/gpio.txt +Alternatively (DEPRECATED), instead of pps-gpios above, it may have: +- gpios: one PPS GPIO as above Optional properties: - assert-falling-edge: when present, assert is indicated by a falling edge (instead of by a rising edge) +- capture-clear: when present, also capture the PPS clear event Example: pps { pinctrl-names = "default"; pinctrl-0 = <&pinctrl_pps>; - gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>; + pps-gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>; assert-falling-edge; + capture-clear; compatible = "pps-gpio"; }; -- 2.12.3 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v7 2/4] pps: descriptor-based gpio, capture-clear addition 2018-11-14 12:54 ` [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition Tom Burkart @ 2018-11-14 12:54 ` Tom Burkart 2018-11-14 12:54 ` [PATCH 3/4] dt-bindings: pps: pps-gpio PPS ECHO implementation Tom Burkart 2018-11-16 22:49 ` [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition Rob Herring 1 sibling, 1 reply; 16+ messages in thread From: Tom Burkart @ 2018-11-14 12:54 UTC (permalink / raw) To: Linux kernel mailing list; +Cc: Tom Burkart This patch changes the GPIO access for the pps-gpio driver from the integer based ABI to the descriptor based ABI. It also adds documentation for the device tree capture-clear option and device tree capture-clear extraction. The legacy device tree entry for the GPIO pin is supported. Signed-off-by: Tom Burkart <tom@aussec.com> --- drivers/pps/clients/pps-gpio.c | 124 ++++++++++++++++++++++++++++------------- include/linux/pps-gpio.h | 3 +- 2 files changed, 86 insertions(+), 41 deletions(-) diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c index 333ad7d5b45b..aa8b8fb0e9ab 100644 --- a/drivers/pps/clients/pps-gpio.c +++ b/drivers/pps/clients/pps-gpio.c @@ -31,7 +31,8 @@ #include <linux/slab.h> #include <linux/pps_kernel.h> #include <linux/pps-gpio.h> -#include <linux/gpio.h> +#include <linux/gpio/consumer.h> /* GPIO descriptor ABI */ +#include <linux/gpio.h> /* GPIO integer ABI */ #include <linux/list.h> #include <linux/of_device.h> #include <linux/of_gpio.h> @@ -41,9 +42,11 @@ struct pps_gpio_device_data { int irq; /* IRQ used as PPS source */ struct pps_device *pps; /* PPS source device */ struct pps_source_info info; /* PPS source information */ + struct gpio_desc *gpio_pin; /* GPIO port descriptors */ bool assert_falling_edge; bool capture_clear; - unsigned int gpio_pin; + bool legacy_int_gpio; /* flag for which ABI to use */ + unsigned int gpio_pin_int; /* GPIO integer ABI */ }; /* @@ -61,18 +64,87 @@ static irqreturn_t pps_gpio_irq_handler(int irq, void *data) info = data; - rising_edge = gpio_get_value(info->gpio_pin); + if (info->legacy_int_gpio) + rising_edge = gpio_get_value(info->gpio_pin_int); + else + rising_edge = gpiod_get_value(info->gpio_pin); if ((rising_edge && !info->assert_falling_edge) || (!rising_edge && info->assert_falling_edge)) pps_event(info->pps, &ts, PPS_CAPTUREASSERT, NULL); else if (info->capture_clear && ((rising_edge && info->assert_falling_edge) || - (!rising_edge && !info->assert_falling_edge))) + (!rising_edge && !info->assert_falling_edge))) pps_event(info->pps, &ts, PPS_CAPTURECLEAR, NULL); return IRQ_HANDLED; } +static int pps_gpio_setup(struct platform_device *pdev) +{ + struct pps_gpio_device_data *data = platform_get_drvdata(pdev); + const struct pps_gpio_platform_data *pdata = pdev->dev.platform_data; + struct device_node *np = pdev->dev.of_node; + const char *gpio_label; + int ret; + + if (pdata) { + data->gpio_pin_int = pdata->gpio_pin_int; + data->gpio_pin = pdata->gpio_pin; + gpio_label = pdata->gpio_label; + + data->assert_falling_edge = pdata->assert_falling_edge; + data->capture_clear = pdata->capture_clear; + } else { + if (of_get_property(np, "gpios", NULL)) { /* integer GPIO */ + data->legacy_int_gpio = true; + + ret = of_get_named_gpio(np, "gpios", 0); + if (ret < 0) { + dev_err(&pdev->dev, + "failed to get GPIO from device tree\n"); + return ret; + } + data->gpio_pin_int = ret; + gpio_label = PPS_GPIO_NAME; + + } else { /* descriptor GPIO */ + data->gpio_pin = devm_gpiod_get(&pdev->dev, + "pps", + GPIOD_IN); + if (IS_ERR(data->gpio_pin)) { + dev_err(&pdev->dev, + "failed to request PPS GPIO\n"); + return PTR_ERR(data->gpio_pin); + } + } + + if (of_get_property(np, "assert-falling-edge", NULL)) + data->assert_falling_edge = true; + + if (of_get_property(np, "capture-clear", NULL)) + data->capture_clear = true; + } + + /* GPIO setup */ + if (data->legacy_int_gpio) { + ret = devm_gpio_request(&pdev->dev, + data->gpio_pin_int, + gpio_label); + if (ret) { + dev_err(&pdev->dev, "failed to request GPIO %u\n", + data->gpio_pin_int); + return ret; + } + + ret = gpio_direction_input(data->gpio_pin_int); + if (ret) { + dev_err(&pdev->dev, "failed to set pin as input\n"); + return -EINVAL; + } + } + return 0; +} + static unsigned long get_irqf_trigger_flags(const struct pps_gpio_device_data *data) { @@ -90,53 +162,26 @@ get_irqf_trigger_flags(const struct pps_gpio_device_data *data) static int pps_gpio_probe(struct platform_device *pdev) { struct pps_gpio_device_data *data; - const char *gpio_label; int ret; int pps_default_params; - const struct pps_gpio_platform_data *pdata = pdev->dev.platform_data; - struct device_node *np = pdev->dev.of_node; /* allocate space for device info */ data = devm_kzalloc(&pdev->dev, sizeof(struct pps_gpio_device_data), GFP_KERNEL); if (!data) return -ENOMEM; - - if (pdata) { - data->gpio_pin = pdata->gpio_pin; - gpio_label = pdata->gpio_label; - - data->assert_falling_edge = pdata->assert_falling_edge; - data->capture_clear = pdata->capture_clear; - } else { - ret = of_get_gpio(np, 0); - if (ret < 0) { - dev_err(&pdev->dev, "failed to get GPIO from device tree\n"); - return ret; - } - data->gpio_pin = ret; - gpio_label = PPS_GPIO_NAME; - - if (of_get_property(np, "assert-falling-edge", NULL)) - data->assert_falling_edge = true; - } + platform_set_drvdata(pdev, data); /* GPIO setup */ - ret = devm_gpio_request(&pdev->dev, data->gpio_pin, gpio_label); - if (ret) { - dev_err(&pdev->dev, "failed to request GPIO %u\n", - data->gpio_pin); - return ret; - } - - ret = gpio_direction_input(data->gpio_pin); - if (ret) { - dev_err(&pdev->dev, "failed to set pin direction\n"); + ret = pps_gpio_setup(pdev); + if (ret) return -EINVAL; - } /* IRQ setup */ - ret = gpio_to_irq(data->gpio_pin); + if (data->legacy_int_gpio) + ret = gpio_to_irq(data->gpio_pin_int); + else + ret = gpiod_to_irq(data->gpio_pin); if (ret < 0) { dev_err(&pdev->dev, "failed to map GPIO to IRQ: %d\n", ret); return -EINVAL; @@ -173,7 +218,6 @@ static int pps_gpio_probe(struct platform_device *pdev) return -EINVAL; } - platform_set_drvdata(pdev, data); dev_info(data->pps->dev, "Registered IRQ %d as PPS source\n", data->irq); @@ -209,4 +253,4 @@ MODULE_AUTHOR("Ricardo Martins <rasm@fe.up.pt>"); MODULE_AUTHOR("James Nuss <jamesnuss@nanometrics.ca>"); MODULE_DESCRIPTION("Use GPIO pin as PPS source"); MODULE_LICENSE("GPL"); -MODULE_VERSION("1.0.0"); +MODULE_VERSION("1.1.0"); diff --git a/include/linux/pps-gpio.h b/include/linux/pps-gpio.h index 56f35dd3d01d..86e2081dc7d1 100644 --- a/include/linux/pps-gpio.h +++ b/include/linux/pps-gpio.h @@ -23,9 +23,10 @@ #define _PPS_GPIO_H struct pps_gpio_platform_data { + struct gpio_desc *gpio_pin; bool assert_falling_edge; bool capture_clear; - unsigned int gpio_pin; + unsigned int gpio_pin_int; const char *gpio_label; }; -- 2.12.3 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/4] dt-bindings: pps: pps-gpio PPS ECHO implementation 2018-11-14 12:54 ` [PATCH v7 2/4] " Tom Burkart @ 2018-11-14 12:54 ` Tom Burkart 2018-11-14 12:54 ` [PATCH v7 4/4] pps: pps-gpio pps-echo implementation Tom Burkart 0 siblings, 1 reply; 16+ messages in thread From: Tom Burkart @ 2018-11-14 12:54 UTC (permalink / raw) To: Linux kernel mailing list; +Cc: Tom Burkart, devicetree, Lukas Senger This patch implements the device tree changes required for the pps echo functionality for pps-gpio, that sysfs claims is available already. This patch was originally written by Lukas Senger as part of a masters thesis project and modified for inclusion into the linux kernel by Tom Burkart. Signed-off-by: Lukas Senger <lukas@fridolin.com> Signed-off-by: Tom Burkart <tom@aussec.com> --- Documentation/devicetree/bindings/pps/pps-gpio.txt | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt b/Documentation/devicetree/bindings/pps/pps-gpio.txt index 6c9fc0998d94..f85dbe4ce0b3 100644 --- a/Documentation/devicetree/bindings/pps/pps-gpio.txt +++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt @@ -9,10 +9,15 @@ Required properties: Alternatively (DEPRECATED), instead of pps-gpios above, it may have: - gpios: one PPS GPIO as above +Additional required properties for the PPS ECHO functionality: +- echo-gpios: one PPS ECHO GPIO in the format described by ../gpio/gpio.txt +- echo-active-ms: duration in ms of the active portion of the echo pulse + Optional properties: - assert-falling-edge: when present, assert is indicated by a falling edge (instead of by a rising edge) - capture-clear: when present, also capture the PPS clear event +- invert-pps-echo: when present, invert the PPS ECHO pulse Example: pps { @@ -23,5 +28,9 @@ Example: assert-falling-edge; capture-clear; + echo-gpios = <&gpio1 27 GPIO_ACTIVE_HIGH>; + echo-active-ms = <100>; + invert-pps-echo; + compatible = "pps-gpio"; }; -- 2.12.3 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v7 4/4] pps: pps-gpio pps-echo implementation 2018-11-14 12:54 ` [PATCH 3/4] dt-bindings: pps: pps-gpio PPS ECHO implementation Tom Burkart @ 2018-11-14 12:54 ` Tom Burkart 0 siblings, 0 replies; 16+ messages in thread From: Tom Burkart @ 2018-11-14 12:54 UTC (permalink / raw) To: Linux kernel mailing list; +Cc: Tom Burkart, Lukas Senger This patch implements the pps echo functionality for pps-gpio, that sysfs claims is available already. Configuration is done via device tree bindings. This patch was originally written by Lukas Senger as part of a masters thesis project and modified for inclusion into the linux kernel by Tom Burkart. Signed-off-by: Lukas Senger <lukas@fridolin.com> Signed-off-by: Tom Burkart <tom@aussec.com> --- drivers/pps/clients/pps-gpio.c | 103 +++++++++++++++++++++++++++++++++++++++-- include/linux/pps-gpio.h | 3 ++ 2 files changed, 103 insertions(+), 3 deletions(-) diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c index aa8b8fb0e9ab..ea035399d56d 100644 --- a/drivers/pps/clients/pps-gpio.c +++ b/drivers/pps/clients/pps-gpio.c @@ -36,6 +36,8 @@ #include <linux/list.h> #include <linux/of_device.h> #include <linux/of_gpio.h> +#include <linux/timer.h> +#include <linux/jiffies.h> /* Info for each registered platform device */ struct pps_gpio_device_data { @@ -43,10 +45,16 @@ struct pps_gpio_device_data { struct pps_device *pps; /* PPS source device */ struct pps_source_info info; /* PPS source information */ struct gpio_desc *gpio_pin; /* GPIO port descriptors */ + struct gpio_desc *echo_pin; + struct timer_list echo_timer; /* timer to reset echo active state */ bool assert_falling_edge; bool capture_clear; + bool enable_pps_echo; + bool invert_pps_echo; bool legacy_int_gpio; /* flag for which ABI to use */ + unsigned int echo_active_ms; /* PPS echo active duration */ unsigned int gpio_pin_int; /* GPIO integer ABI */ + unsigned long echo_timeout; /* timer timeout value in jiffies */ }; /* @@ -70,15 +78,54 @@ static irqreturn_t pps_gpio_irq_handler(int irq, void *data) rising_edge = gpiod_get_value(info->gpio_pin); if ((rising_edge && !info->assert_falling_edge) || (!rising_edge && info->assert_falling_edge)) - pps_event(info->pps, &ts, PPS_CAPTUREASSERT, NULL); + pps_event(info->pps, &ts, PPS_CAPTUREASSERT, data); else if (info->capture_clear && ((rising_edge && info->assert_falling_edge) || (!rising_edge && !info->assert_falling_edge))) - pps_event(info->pps, &ts, PPS_CAPTURECLEAR, NULL); + pps_event(info->pps, &ts, PPS_CAPTURECLEAR, data); return IRQ_HANDLED; } +static void pps_gpio_echo(struct pps_device *pps, int event, void *data) +{ + /* add_timer() needs to write into info->echo_timer */ + struct pps_gpio_device_data *info; + + info = data; + + switch (event) { + case PPS_CAPTUREASSERT: + if (pps->params.mode & PPS_ECHOASSERT) + gpiod_set_value(info->echo_pin, + info->invert_pps_echo ? 0 : 1); + break; + + case PPS_CAPTURECLEAR: + if (pps->params.mode & PPS_ECHOCLEAR) + gpiod_set_value(info->echo_pin, + info->invert_pps_echo ? 0 : 1); + break; + } + + /* fire the timer */ + if (info->pps->params.mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) { + info->echo_timer.expires = jiffies + info->echo_timeout; + add_timer(&info->echo_timer); + } +} + +/* Timer callback to reset the echo pin to the inactive state */ +static void pps_gpio_echo_timer_callback(struct timer_list *t) +{ + const struct pps_gpio_device_data *info; + + info = from_timer(info, t, echo_timer); + + gpiod_set_value(info->echo_pin, + info->invert_pps_echo ? 1 : 0); +} + static int pps_gpio_setup(struct platform_device *pdev) { struct pps_gpio_device_data *data = platform_get_drvdata(pdev); @@ -86,14 +133,20 @@ static int pps_gpio_setup(struct platform_device *pdev) struct device_node *np = pdev->dev.of_node; const char *gpio_label; int ret; + u32 value; if (pdata) { data->gpio_pin_int = pdata->gpio_pin_int; data->gpio_pin = pdata->gpio_pin; + data->echo_pin = pdata->echo_pin; + if (pdata->echo_pin != NULL) + data->enable_pps_echo = true; gpio_label = pdata->gpio_label; data->assert_falling_edge = pdata->assert_falling_edge; data->capture_clear = pdata->capture_clear; + data->invert_pps_echo = pdata->invert_pps_echo; + data->echo_active_ms = pdata->echo_active_ms; } else { if (of_get_property(np, "gpios", NULL)) { /* integer GPIO */ data->legacy_int_gpio = true; @@ -118,11 +171,45 @@ static int pps_gpio_setup(struct platform_device *pdev) } } + if (of_get_property(np, "echo-gpios", NULL)) { + data->enable_pps_echo = true; + + data->echo_pin = devm_gpiod_get(&pdev->dev, + "echo", + GPIOD_OUT_LOW); + if (IS_ERR(data->echo_pin)) { + dev_err(&pdev->dev, "failed to request ECHO GPIO\n"); + return PTR_ERR(data->echo_pin); + } + + ret = of_property_read_u32(np, + "echo-active-ms", + &value); + if (ret) { + dev_err(&pdev->dev, + "failed to get echo-active-ms from OF\n"); + return ret; + } + data->echo_active_ms = value; + } + + if (of_get_property(np, "assert-falling-edge", NULL)) data->assert_falling_edge = true; if (of_get_property(np, "capture-clear", NULL)) data->capture_clear = true; + + if (of_get_property(np, "invert-pps-echo", NULL)) + data->invert_pps_echo = true; + } + /* sanity check on echo_active_ms */ + if (data->enable_pps_echo + && (!data->echo_active_ms || data->echo_active_ms > 999)) { + dev_err(&pdev->dev, + "echo-active-ms: %u - bad value from OF\n", + data->echo_active_ms); + return -EINVAL; } /* GPIO setup */ @@ -197,6 +284,11 @@ static int pps_gpio_probe(struct platform_device *pdev) data->info.owner = THIS_MODULE; snprintf(data->info.name, PPS_MAX_NAME_LEN - 1, "%s.%d", pdev->name, pdev->id); + if (data->enable_pps_echo) { + data->info.echo = pps_gpio_echo; + data->echo_timeout = msecs_to_jiffies(data->echo_active_ms); + timer_setup(&data->echo_timer, pps_gpio_echo_timer_callback, 0); + } /* register PPS source */ pps_default_params = PPS_CAPTUREASSERT | PPS_OFFSETASSERT; @@ -229,6 +321,11 @@ static int pps_gpio_remove(struct platform_device *pdev) struct pps_gpio_device_data *data = platform_get_drvdata(pdev); pps_unregister_source(data->pps); + if (data->enable_pps_echo) { + del_timer_sync(&data->echo_timer); + /* reset echo pin in any case */ + gpiod_set_value(data->echo_pin, data->invert_pps_echo ? 1 : 0); + } dev_info(&pdev->dev, "removed IRQ %d as PPS source\n", data->irq); return 0; } @@ -253,4 +350,4 @@ MODULE_AUTHOR("Ricardo Martins <rasm@fe.up.pt>"); MODULE_AUTHOR("James Nuss <jamesnuss@nanometrics.ca>"); MODULE_DESCRIPTION("Use GPIO pin as PPS source"); MODULE_LICENSE("GPL"); -MODULE_VERSION("1.1.0"); +MODULE_VERSION("1.2.0"); diff --git a/include/linux/pps-gpio.h b/include/linux/pps-gpio.h index 86e2081dc7d1..21eceb90187d 100644 --- a/include/linux/pps-gpio.h +++ b/include/linux/pps-gpio.h @@ -24,8 +24,11 @@ struct pps_gpio_platform_data { struct gpio_desc *gpio_pin; + struct gpio_desc *echo_pin; bool assert_falling_edge; bool capture_clear; + bool invert_pps_echo; + unsigned int echo_active_ms; unsigned int gpio_pin_int; const char *gpio_label; }; -- 2.12.3 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition 2018-11-14 12:54 ` [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition Tom Burkart 2018-11-14 12:54 ` [PATCH v7 2/4] " Tom Burkart @ 2018-11-16 22:49 ` Rob Herring 2018-11-17 10:35 ` tom burkart 1 sibling, 1 reply; 16+ messages in thread From: Rob Herring @ 2018-11-16 22:49 UTC (permalink / raw) To: Tom Burkart; +Cc: Linux kernel mailing list, devicetree On Wed, Nov 14, 2018 at 11:54:29PM +1100, Tom Burkart wrote: > This patch changes the devicetree bindings for the pps-gpio driver > from the integer based ABI to the descriptor based ABI. ? That has nothing to do with DT. > It also adds > documentation for the device tree capture-clear option. The legacy > device tree entry for the GPIO pin is supported. > > Signed-off-by: Tom Burkart <tom@aussec.com> > --- > Documentation/devicetree/bindings/pps/pps-gpio.txt | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt b/Documentation/devicetree/bindings/pps/pps-gpio.txt > index 3683874832ae..6c9fc0998d94 100644 > --- a/Documentation/devicetree/bindings/pps/pps-gpio.txt > +++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt > @@ -5,19 +5,23 @@ a GPIO pin. > > Required properties: > - compatible: should be "pps-gpio" > -- gpios: one PPS GPIO in the format described by ../gpio/gpio.txt > +- pps-gpios: one PPS GPIO in the format described by ../gpio/gpio.txt > +Alternatively (DEPRECATED), instead of pps-gpios above, it may have: > +- gpios: one PPS GPIO as above > > Optional properties: > - assert-falling-edge: when present, assert is indicated by a falling edge > (instead of by a rising edge) > +- capture-clear: when present, also capture the PPS clear event Is this a h/w thing? or driver configuration? > > Example: > pps { > pinctrl-names = "default"; > pinctrl-0 = <&pinctrl_pps>; > > - gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>; > + pps-gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>; > assert-falling-edge; > + capture-clear; > > compatible = "pps-gpio"; > }; > -- > 2.12.3 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition 2018-11-16 22:49 ` [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition Rob Herring @ 2018-11-17 10:35 ` tom burkart 2018-11-17 14:22 ` Rob Herring 0 siblings, 1 reply; 16+ messages in thread From: tom burkart @ 2018-11-17 10:35 UTC (permalink / raw) To: Rob Herring; +Cc: Linux kernel mailing list, devicetree Quoting Rob Herring <robh@kernel.org>: > On Wed, Nov 14, 2018 at 11:54:29PM +1100, Tom Burkart wrote: >> This patch changes the devicetree bindings for the pps-gpio driver >> from the integer based ABI to the descriptor based ABI. > ? That has nothing to do with DT. I believe it does, as the change in ABI forces a rename in the DT naming convention. This is due to the descriptor based ABI appending "-gpio" or "-gpios" (see Documentation/gpio/base.txt.) Admittedly, I may have called it by the wrong name due to ignorance, my apologies. >> It also adds >> documentation for the device tree capture-clear option. The legacy >> device tree entry for the GPIO pin is supported. >> >> Signed-off-by: Tom Burkart <tom@aussec.com> >> --- >> Documentation/devicetree/bindings/pps/pps-gpio.txt | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt >> b/Documentation/devicetree/bindings/pps/pps-gpio.txt >> index 3683874832ae..6c9fc0998d94 100644 >> --- a/Documentation/devicetree/bindings/pps/pps-gpio.txt >> +++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt >> @@ -5,19 +5,23 @@ a GPIO pin. >> >> Required properties: >> - compatible: should be "pps-gpio" >> -- gpios: one PPS GPIO in the format described by ../gpio/gpio.txt >> +- pps-gpios: one PPS GPIO in the format described by ../gpio/gpio.txt >> +Alternatively (DEPRECATED), instead of pps-gpios above, it may have: >> +- gpios: one PPS GPIO as above >> >> Optional properties: >> - assert-falling-edge: when present, assert is indicated by a falling edge >> (instead of by a rising edge) >> +- capture-clear: when present, also capture the PPS clear event > > Is this a h/w thing? or driver configuration? Driver configuration. Most of the code was present in the driver, yet it was not documented, or usable due to a two line (code) omission (the value was not being fetched from DT). >> >> Example: >> pps { >> pinctrl-names = "default"; >> pinctrl-0 = <&pinctrl_pps>; >> >> - gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>; >> + pps-gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>; >> assert-falling-edge; >> + capture-clear; >> >> compatible = "pps-gpio"; >> }; >> -- >> 2.12.3 >> > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition 2018-11-17 10:35 ` tom burkart @ 2018-11-17 14:22 ` Rob Herring 2018-11-18 0:35 ` tom burkart 0 siblings, 1 reply; 16+ messages in thread From: Rob Herring @ 2018-11-17 14:22 UTC (permalink / raw) To: Tom Burkart; +Cc: linux-kernel, devicetree On Sat, Nov 17, 2018 at 4:35 AM tom burkart <tom@aussec.com> wrote: > > Quoting Rob Herring <robh@kernel.org>: > > > On Wed, Nov 14, 2018 at 11:54:29PM +1100, Tom Burkart wrote: > >> This patch changes the devicetree bindings for the pps-gpio driver > >> from the integer based ABI to the descriptor based ABI. > > ? That has nothing to do with DT. > > I believe it does, as the change in ABI forces a rename in the DT > naming convention. > This is due to the descriptor based ABI appending "-gpio" or "-gpios" (see > Documentation/gpio/base.txt.) > Admittedly, I may have called it by the wrong name due to ignorance, > my apologies. If what you say is correct, then you can't change this driver. You'll break compatibility with any existing DT. Changing the binding reasoning should purely be that this is the preferred form. Bindings must be independent from changing kernel APIs. > > >> It also adds > >> documentation for the device tree capture-clear option. The legacy > >> device tree entry for the GPIO pin is supported. > >> > >> Signed-off-by: Tom Burkart <tom@aussec.com> > >> --- > >> Documentation/devicetree/bindings/pps/pps-gpio.txt | 8 ++++++-- > >> 1 file changed, 6 insertions(+), 2 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt > >> b/Documentation/devicetree/bindings/pps/pps-gpio.txt > >> index 3683874832ae..6c9fc0998d94 100644 > >> --- a/Documentation/devicetree/bindings/pps/pps-gpio.txt > >> +++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt > >> @@ -5,19 +5,23 @@ a GPIO pin. > >> > >> Required properties: > >> - compatible: should be "pps-gpio" > >> -- gpios: one PPS GPIO in the format described by ../gpio/gpio.txt > >> +- pps-gpios: one PPS GPIO in the format described by ../gpio/gpio.txt > >> +Alternatively (DEPRECATED), instead of pps-gpios above, it may have: > >> +- gpios: one PPS GPIO as above > >> > >> Optional properties: > >> - assert-falling-edge: when present, assert is indicated by a falling edge > >> (instead of by a rising edge) > >> +- capture-clear: when present, also capture the PPS clear event > > > > Is this a h/w thing? or driver configuration? > > Driver configuration. Most of the code was present in the driver, yet > it was not documented, or usable due to a two line (code) omission > (the value was not being fetched from DT). So what determines how you want to configure this? If the user will want to change it, then it should be a sysfs attr and exposed to userspace. If it depends on h/w config for a board then it can be in DT. Rob ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition 2018-11-17 14:22 ` Rob Herring @ 2018-11-18 0:35 ` tom burkart 2018-11-26 19:39 ` Rob Herring 0 siblings, 1 reply; 16+ messages in thread From: tom burkart @ 2018-11-18 0:35 UTC (permalink / raw) To: Rob Herring; +Cc: linux-kernel, devicetree Quoting Rob Herring <robh@kernel.org>: > On Sat, Nov 17, 2018 at 4:35 AM tom burkart <tom@aussec.com> wrote: >> >> Quoting Rob Herring <robh@kernel.org>: >> >> > On Wed, Nov 14, 2018 at 11:54:29PM +1100, Tom Burkart wrote: >> >> This patch changes the devicetree bindings for the pps-gpio driver >> >> from the integer based ABI to the descriptor based ABI. >> > ? That has nothing to do with DT. >> >> I believe it does, as the change in ABI forces a rename in the DT >> naming convention. >> This is due to the descriptor based ABI appending "-gpio" or "-gpios" (see >> Documentation/gpio/base.txt.) >> Admittedly, I may have called it by the wrong name due to ignorance, >> my apologies. > > If what you say is correct, then you can't change this driver. You'll > break compatibility with any existing DT. > > Changing the binding reasoning should purely be that this is the > preferred form. Bindings must be independent from changing kernel > APIs. See comments from Philip Zabel. I misread the documentation and this has now been corrected in v8 of the patch. I hope that eliminates all comments made above. >> >> It also adds >> >> documentation for the device tree capture-clear option. The legacy >> >> device tree entry for the GPIO pin is supported. >> >> >> >> Signed-off-by: Tom Burkart <tom@aussec.com> >> >> --- >> >> Documentation/devicetree/bindings/pps/pps-gpio.txt | 8 ++++++-- >> >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> b/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> index 3683874832ae..6c9fc0998d94 100644 >> >> --- a/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> +++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> @@ -5,19 +5,23 @@ a GPIO pin. >> >> >> >> Required properties: >> >> - compatible: should be "pps-gpio" >> >> -- gpios: one PPS GPIO in the format described by ../gpio/gpio.txt >> >> +- pps-gpios: one PPS GPIO in the format described by ../gpio/gpio.txt >> >> +Alternatively (DEPRECATED), instead of pps-gpios above, it may have: >> >> +- gpios: one PPS GPIO as above >> >> >> >> Optional properties: >> >> - assert-falling-edge: when present, assert is indicated by a >> falling edge >> >> (instead of by a rising edge) >> >> +- capture-clear: when present, also capture the PPS clear event >> > >> > Is this a h/w thing? or driver configuration? >> >> Driver configuration. Most of the code was present in the driver, yet >> it was not documented, or usable due to a two line (code) omission >> (the value was not being fetched from DT). > > So what determines how you want to configure this? If the user will > want to change it, then it should be a sysfs attr and exposed to > userspace. If it depends on h/w config for a board then it can be in > DT. Sorry, I misled you somewhat. If the PPS pulse active transition from the hardware is on the falling edge, this flag is required to get the OS to use that as the active transition. This would not change at the user's whim but rather it is dependent on connected hardware. Tom ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition 2018-11-18 0:35 ` tom burkart @ 2018-11-26 19:39 ` Rob Herring 2018-11-27 3:57 ` tom burkart 0 siblings, 1 reply; 16+ messages in thread From: Rob Herring @ 2018-11-26 19:39 UTC (permalink / raw) To: Tom Burkart; +Cc: linux-kernel, devicetree On Sat, Nov 17, 2018 at 6:35 PM tom burkart <tom@aussec.com> wrote: > > Quoting Rob Herring <robh@kernel.org>: > > > On Sat, Nov 17, 2018 at 4:35 AM tom burkart <tom@aussec.com> wrote: > >> > >> Quoting Rob Herring <robh@kernel.org>: > >> > >> > On Wed, Nov 14, 2018 at 11:54:29PM +1100, Tom Burkart wrote: > >> >> This patch changes the devicetree bindings for the pps-gpio driver > >> >> from the integer based ABI to the descriptor based ABI. > >> > ? That has nothing to do with DT. > >> > >> I believe it does, as the change in ABI forces a rename in the DT > >> naming convention. > >> This is due to the descriptor based ABI appending "-gpio" or "-gpios" (see > >> Documentation/gpio/base.txt.) > >> Admittedly, I may have called it by the wrong name due to ignorance, > >> my apologies. > > > > If what you say is correct, then you can't change this driver. You'll > > break compatibility with any existing DT. > > > > Changing the binding reasoning should purely be that this is the > > preferred form. Bindings must be independent from changing kernel > > APIs. > > See comments from Philip Zabel. I misread the documentation and this > has now been corrected in v8 of the patch. I hope that eliminates all > comments made above. > > >> >> It also adds > >> >> documentation for the device tree capture-clear option. The legacy > >> >> device tree entry for the GPIO pin is supported. > >> >> > >> >> Signed-off-by: Tom Burkart <tom@aussec.com> > >> >> --- > >> >> Documentation/devicetree/bindings/pps/pps-gpio.txt | 8 ++++++-- > >> >> 1 file changed, 6 insertions(+), 2 deletions(-) > >> >> > >> >> diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt > >> >> b/Documentation/devicetree/bindings/pps/pps-gpio.txt > >> >> index 3683874832ae..6c9fc0998d94 100644 > >> >> --- a/Documentation/devicetree/bindings/pps/pps-gpio.txt > >> >> +++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt > >> >> @@ -5,19 +5,23 @@ a GPIO pin. > >> >> > >> >> Required properties: > >> >> - compatible: should be "pps-gpio" > >> >> -- gpios: one PPS GPIO in the format described by ../gpio/gpio.txt > >> >> +- pps-gpios: one PPS GPIO in the format described by ../gpio/gpio.txt > >> >> +Alternatively (DEPRECATED), instead of pps-gpios above, it may have: > >> >> +- gpios: one PPS GPIO as above > >> >> > >> >> Optional properties: > >> >> - assert-falling-edge: when present, assert is indicated by a > >> falling edge > >> >> (instead of by a rising edge) > >> >> +- capture-clear: when present, also capture the PPS clear event > >> > > >> > Is this a h/w thing? or driver configuration? > >> > >> Driver configuration. Most of the code was present in the driver, yet > >> it was not documented, or usable due to a two line (code) omission > >> (the value was not being fetched from DT). > > > > So what determines how you want to configure this? If the user will > > want to change it, then it should be a sysfs attr and exposed to > > userspace. If it depends on h/w config for a board then it can be in > > DT. > > Sorry, I misled you somewhat. If the PPS pulse active transition from > the hardware is on the falling edge, this flag is required to get the > OS to use that as the active transition. This would not change at the > user's whim but rather it is dependent on connected hardware. This description sounds more like 'assert-falling-edge' than 'capture-clear'. I'm still not clear on what 'capture-clear' is. Rob ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition 2018-11-26 19:39 ` Rob Herring @ 2018-11-27 3:57 ` tom burkart 2018-11-28 17:13 ` Rob Herring 0 siblings, 1 reply; 16+ messages in thread From: tom burkart @ 2018-11-27 3:57 UTC (permalink / raw) To: Rob Herring; +Cc: linux-kernel, devicetree Hi Rob, Quoting Rob Herring <robh@kernel.org>: > On Sat, Nov 17, 2018 at 6:35 PM tom burkart <tom@aussec.com> wrote: >> >> Quoting Rob Herring <robh@kernel.org>: >> >> > On Sat, Nov 17, 2018 at 4:35 AM tom burkart <tom@aussec.com> wrote: >> >> >> >> Quoting Rob Herring <robh@kernel.org>: >> >> >> >> > On Wed, Nov 14, 2018 at 11:54:29PM +1100, Tom Burkart wrote: >> >> >> This patch changes the devicetree bindings for the pps-gpio driver >> >> >> from the integer based ABI to the descriptor based ABI. >> >> > ? That has nothing to do with DT. >> >> >> >> I believe it does, as the change in ABI forces a rename in the DT >> >> naming convention. >> >> This is due to the descriptor based ABI appending "-gpio" or >> "-gpios" (see >> >> Documentation/gpio/base.txt.) >> >> Admittedly, I may have called it by the wrong name due to ignorance, >> >> my apologies. >> > >> > If what you say is correct, then you can't change this driver. You'll >> > break compatibility with any existing DT. >> > >> > Changing the binding reasoning should purely be that this is the >> > preferred form. Bindings must be independent from changing kernel >> > APIs. >> >> See comments from Philip Zabel. I misread the documentation and this >> has now been corrected in v8 of the patch. I hope that eliminates all >> comments made above. >> >> >> >> It also adds >> >> >> documentation for the device tree capture-clear option. The legacy >> >> >> device tree entry for the GPIO pin is supported. >> >> >> >> >> >> Signed-off-by: Tom Burkart <tom@aussec.com> >> >> >> --- >> >> >> Documentation/devicetree/bindings/pps/pps-gpio.txt | 8 ++++++-- >> >> >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> >> >> >> >> diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> >> b/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> >> index 3683874832ae..6c9fc0998d94 100644 >> >> >> --- a/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> >> +++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> >> @@ -5,19 +5,23 @@ a GPIO pin. >> >> >> >> >> >> Required properties: >> >> >> - compatible: should be "pps-gpio" >> >> >> -- gpios: one PPS GPIO in the format described by ../gpio/gpio.txt >> >> >> +- pps-gpios: one PPS GPIO in the format described by ../gpio/gpio.txt >> >> >> +Alternatively (DEPRECATED), instead of pps-gpios above, it may have: >> >> >> +- gpios: one PPS GPIO as above >> >> >> >> >> >> Optional properties: >> >> >> - assert-falling-edge: when present, assert is indicated by a >> >> falling edge >> >> >> (instead of by a rising edge) >> >> >> +- capture-clear: when present, also capture the PPS clear event >> >> > >> >> > Is this a h/w thing? or driver configuration? >> >> >> >> Driver configuration. Most of the code was present in the driver, yet >> >> it was not documented, or usable due to a two line (code) omission >> >> (the value was not being fetched from DT). >> > >> > So what determines how you want to configure this? If the user will >> > want to change it, then it should be a sysfs attr and exposed to >> > userspace. If it depends on h/w config for a board then it can be in >> > DT. >> >> Sorry, I misled you somewhat. If the PPS pulse active transition from >> the hardware is on the falling edge, this flag is required to get the >> OS to use that as the active transition. This would not change at the >> user's whim but rather it is dependent on connected hardware. > > This description sounds more like 'assert-falling-edge' than 'capture-clear'. > > I'm still not clear on what 'capture-clear' is. Ignoring my patch for a minute, the pps_gpio_irq_handler will only report a pps PPS_CAPTURECLEAR event if 'capture-clear' is set. As the current pps-gpio driver is not able to set this flag, it cannot ever report a PPS_CAPTURECLEAR event. My patch adds the ability to set this flag and adds the documentation to go with it. Admittedly, I do not require this functionality for what I want, but working with the code, I noticed the omission and decided to add it for someone else to use it, if they need it. I am happy to remove this out of my patch, if you feel this to be the best way forward. Tom ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition 2018-11-27 3:57 ` tom burkart @ 2018-11-28 17:13 ` Rob Herring 2018-11-29 0:29 ` tom burkart 2018-11-29 2:05 ` tom burkart 0 siblings, 2 replies; 16+ messages in thread From: Rob Herring @ 2018-11-28 17:13 UTC (permalink / raw) To: Tom Burkart; +Cc: linux-kernel, devicetree On Mon, Nov 26, 2018 at 9:57 PM tom burkart <tom@aussec.com> wrote: > > Hi Rob, > > Quoting Rob Herring <robh@kernel.org>: > > > On Sat, Nov 17, 2018 at 6:35 PM tom burkart <tom@aussec.com> wrote: > >> > >> Quoting Rob Herring <robh@kernel.org>: > >> > >> > On Sat, Nov 17, 2018 at 4:35 AM tom burkart <tom@aussec.com> wrote: > >> >> > >> >> Quoting Rob Herring <robh@kernel.org>: > >> >> > >> >> > On Wed, Nov 14, 2018 at 11:54:29PM +1100, Tom Burkart wrote: > >> >> >> This patch changes the devicetree bindings for the pps-gpio driver > >> >> >> from the integer based ABI to the descriptor based ABI. > >> >> > ? That has nothing to do with DT. > >> >> > >> >> I believe it does, as the change in ABI forces a rename in the DT > >> >> naming convention. > >> >> This is due to the descriptor based ABI appending "-gpio" or > >> "-gpios" (see > >> >> Documentation/gpio/base.txt.) > >> >> Admittedly, I may have called it by the wrong name due to ignorance, > >> >> my apologies. > >> > > >> > If what you say is correct, then you can't change this driver. You'll > >> > break compatibility with any existing DT. > >> > > >> > Changing the binding reasoning should purely be that this is the > >> > preferred form. Bindings must be independent from changing kernel > >> > APIs. > >> > >> See comments from Philip Zabel. I misread the documentation and this > >> has now been corrected in v8 of the patch. I hope that eliminates all > >> comments made above. > >> > >> >> >> It also adds > >> >> >> documentation for the device tree capture-clear option. The legacy > >> >> >> device tree entry for the GPIO pin is supported. > >> >> >> > >> >> >> Signed-off-by: Tom Burkart <tom@aussec.com> > >> >> >> --- > >> >> >> Documentation/devicetree/bindings/pps/pps-gpio.txt | 8 ++++++-- > >> >> >> 1 file changed, 6 insertions(+), 2 deletions(-) > >> >> >> > >> >> >> diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt > >> >> >> b/Documentation/devicetree/bindings/pps/pps-gpio.txt > >> >> >> index 3683874832ae..6c9fc0998d94 100644 > >> >> >> --- a/Documentation/devicetree/bindings/pps/pps-gpio.txt > >> >> >> +++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt > >> >> >> @@ -5,19 +5,23 @@ a GPIO pin. > >> >> >> > >> >> >> Required properties: > >> >> >> - compatible: should be "pps-gpio" > >> >> >> -- gpios: one PPS GPIO in the format described by ../gpio/gpio.txt > >> >> >> +- pps-gpios: one PPS GPIO in the format described by ../gpio/gpio.txt > >> >> >> +Alternatively (DEPRECATED), instead of pps-gpios above, it may have: > >> >> >> +- gpios: one PPS GPIO as above > >> >> >> > >> >> >> Optional properties: > >> >> >> - assert-falling-edge: when present, assert is indicated by a > >> >> falling edge > >> >> >> (instead of by a rising edge) > >> >> >> +- capture-clear: when present, also capture the PPS clear event > >> >> > > >> >> > Is this a h/w thing? or driver configuration? > >> >> > >> >> Driver configuration. Most of the code was present in the driver, yet > >> >> it was not documented, or usable due to a two line (code) omission > >> >> (the value was not being fetched from DT). > >> > > >> > So what determines how you want to configure this? If the user will > >> > want to change it, then it should be a sysfs attr and exposed to > >> > userspace. If it depends on h/w config for a board then it can be in > >> > DT. > >> > >> Sorry, I misled you somewhat. If the PPS pulse active transition from > >> the hardware is on the falling edge, this flag is required to get the > >> OS to use that as the active transition. This would not change at the > >> user's whim but rather it is dependent on connected hardware. > > > > This description sounds more like 'assert-falling-edge' than 'capture-clear'. > > > > I'm still not clear on what 'capture-clear' is. > > Ignoring my patch for a minute, the pps_gpio_irq_handler will only > report a pps PPS_CAPTURECLEAR event if 'capture-clear' is set. As the > current pps-gpio driver is not able to set this flag, it cannot ever > report a PPS_CAPTURECLEAR event. > > My patch adds the ability to set this flag and adds the documentation > to go with it. > Admittedly, I do not require this functionality for what I want, but > working with the code, I noticed the omission and decided to add it > for someone else to use it, if they need it. > > I am happy to remove this out of my patch, if you feel this to be the > best way forward. I found this prior discussion on adding this[1]. Seems to me this should be userspace configurable if the GPIO line can interrupt on both edges. We shouldn't need a DT property to determine that. Rob [1] https://lore.kernel.org/patchwork/patch/557781/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition 2018-11-28 17:13 ` Rob Herring @ 2018-11-29 0:29 ` tom burkart 2018-11-29 2:05 ` tom burkart 1 sibling, 0 replies; 16+ messages in thread From: tom burkart @ 2018-11-29 0:29 UTC (permalink / raw) To: Rob Herring; +Cc: linux-kernel, devicetree Quoting Rob Herring <robh@kernel.org>: > On Mon, Nov 26, 2018 at 9:57 PM tom burkart <tom@aussec.com> wrote: >> >> Quoting Rob Herring <robh@kernel.org>: >> >> > On Sat, Nov 17, 2018 at 6:35 PM tom burkart <tom@aussec.com> wrote: >> >> >> >> Quoting Rob Herring <robh@kernel.org>: >> >> >> >> > On Sat, Nov 17, 2018 at 4:35 AM tom burkart <tom@aussec.com> wrote: >> >> >> >> >> >> Quoting Rob Herring <robh@kernel.org>: >> >> >> >> >> >> > On Wed, Nov 14, 2018 at 11:54:29PM +1100, Tom Burkart wrote: >> >> >> >> This patch changes the devicetree bindings for the pps-gpio driver >> >> >> >> from the integer based ABI to the descriptor based ABI. >> >> >> > ? That has nothing to do with DT. >> >> >> >> >> >> I believe it does, as the change in ABI forces a rename in the DT >> >> >> naming convention. >> >> >> This is due to the descriptor based ABI appending "-gpio" or >> >> "-gpios" (see >> >> >> Documentation/gpio/base.txt.) >> >> >> Admittedly, I may have called it by the wrong name due to ignorance, >> >> >> my apologies. >> >> > >> >> > If what you say is correct, then you can't change this driver. You'll >> >> > break compatibility with any existing DT. >> >> > >> >> > Changing the binding reasoning should purely be that this is the >> >> > preferred form. Bindings must be independent from changing kernel >> >> > APIs. >> >> >> >> See comments from Philip Zabel. I misread the documentation and this >> >> has now been corrected in v8 of the patch. I hope that eliminates all >> >> comments made above. >> >> >> >> >> >> It also adds >> >> >> >> documentation for the device tree capture-clear option. The legacy >> >> >> >> device tree entry for the GPIO pin is supported. >> >> >> >> >> >> >> >> Signed-off-by: Tom Burkart <tom@aussec.com> >> >> >> >> --- >> >> >> >> Documentation/devicetree/bindings/pps/pps-gpio.txt | 8 ++++++-- >> >> >> >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> >> >> >> >> >> >> diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> >> >> b/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> >> >> index 3683874832ae..6c9fc0998d94 100644 >> >> >> >> --- a/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> >> >> +++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> >> >> @@ -5,19 +5,23 @@ a GPIO pin. >> >> >> >> >> >> >> >> Required properties: >> >> >> >> - compatible: should be "pps-gpio" >> >> >> >> -- gpios: one PPS GPIO in the format described by ../gpio/gpio.txt >> >> >> >> +- pps-gpios: one PPS GPIO in the format described by >> ../gpio/gpio.txt >> >> >> >> +Alternatively (DEPRECATED), instead of pps-gpios above, >> it may have: >> >> >> >> +- gpios: one PPS GPIO as above >> >> >> >> >> >> >> >> Optional properties: >> >> >> >> - assert-falling-edge: when present, assert is indicated by a >> >> >> falling edge >> >> >> >> (instead of by a rising edge) >> >> >> >> +- capture-clear: when present, also capture the PPS clear event >> >> >> > >> >> >> > Is this a h/w thing? or driver configuration? >> >> >> >> >> >> Driver configuration. Most of the code was present in the driver, yet >> >> >> it was not documented, or usable due to a two line (code) omission >> >> >> (the value was not being fetched from DT). >> >> > >> >> > So what determines how you want to configure this? If the user will >> >> > want to change it, then it should be a sysfs attr and exposed to >> >> > userspace. If it depends on h/w config for a board then it can be in >> >> > DT. >> >> >> >> Sorry, I misled you somewhat. If the PPS pulse active transition from >> >> the hardware is on the falling edge, this flag is required to get the >> >> OS to use that as the active transition. This would not change at the >> >> user's whim but rather it is dependent on connected hardware. >> > >> > This description sounds more like 'assert-falling-edge' than >> 'capture-clear'. >> > >> > I'm still not clear on what 'capture-clear' is. >> >> Ignoring my patch for a minute, the pps_gpio_irq_handler will only >> report a pps PPS_CAPTURECLEAR event if 'capture-clear' is set. As the >> current pps-gpio driver is not able to set this flag, it cannot ever >> report a PPS_CAPTURECLEAR event. >> >> My patch adds the ability to set this flag and adds the documentation >> to go with it. >> Admittedly, I do not require this functionality for what I want, but >> working with the code, I noticed the omission and decided to add it >> for someone else to use it, if they need it. >> >> I am happy to remove this out of my patch, if you feel this to be the >> best way forward. > > I found this prior discussion on adding this[1]. Seems to me this > should be userspace configurable if the GPIO line can interrupt on > both edges. We shouldn't need a DT property to determine that. > > Rob > > [1] https://lore.kernel.org/patchwork/patch/557781/ Patch v11 has just been sent that has no changes to the capture-clear DT option. Tom ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition 2018-11-28 17:13 ` Rob Herring 2018-11-29 0:29 ` tom burkart @ 2018-11-29 2:05 ` tom burkart 2018-11-29 18:00 ` Rob Herring 1 sibling, 1 reply; 16+ messages in thread From: tom burkart @ 2018-11-29 2:05 UTC (permalink / raw) To: Rob Herring; +Cc: linux-kernel, devicetree Quoting Rob Herring <robh@kernel.org>: > On Mon, Nov 26, 2018 at 9:57 PM tom burkart <tom@aussec.com> wrote: >> >> Quoting Rob Herring <robh@kernel.org>: >> >> > On Sat, Nov 17, 2018 at 6:35 PM tom burkart <tom@aussec.com> wrote: >> >> >> >> Quoting Rob Herring <robh@kernel.org>: >> >> >> >> > On Sat, Nov 17, 2018 at 4:35 AM tom burkart <tom@aussec.com> wrote: >> >> >> >> >> >> Quoting Rob Herring <robh@kernel.org>: >> >> >> >> >> >> > On Wed, Nov 14, 2018 at 11:54:29PM +1100, Tom Burkart wrote: >> >> >> >> This patch changes the devicetree bindings for the pps-gpio driver >> >> >> >> from the integer based ABI to the descriptor based ABI. >> >> >> > ? That has nothing to do with DT. >> >> >> >> >> >> I believe it does, as the change in ABI forces a rename in the DT >> >> >> naming convention. >> >> >> This is due to the descriptor based ABI appending "-gpio" or >> >> "-gpios" (see >> >> >> Documentation/gpio/base.txt.) >> >> >> Admittedly, I may have called it by the wrong name due to ignorance, >> >> >> my apologies. >> >> > >> >> > If what you say is correct, then you can't change this driver. You'll >> >> > break compatibility with any existing DT. >> >> > >> >> > Changing the binding reasoning should purely be that this is the >> >> > preferred form. Bindings must be independent from changing kernel >> >> > APIs. >> >> >> >> See comments from Philip Zabel. I misread the documentation and this >> >> has now been corrected in v8 of the patch. I hope that eliminates all >> >> comments made above. >> >> >> >> >> >> It also adds >> >> >> >> documentation for the device tree capture-clear option. The legacy >> >> >> >> device tree entry for the GPIO pin is supported. >> >> >> >> >> >> >> >> Signed-off-by: Tom Burkart <tom@aussec.com> >> >> >> >> --- >> >> >> >> Documentation/devicetree/bindings/pps/pps-gpio.txt | 8 ++++++-- >> >> >> >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> >> >> >> >> >> >> diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> >> >> b/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> >> >> index 3683874832ae..6c9fc0998d94 100644 >> >> >> >> --- a/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> >> >> +++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> >> >> @@ -5,19 +5,23 @@ a GPIO pin. >> >> >> >> >> >> >> >> Required properties: >> >> >> >> - compatible: should be "pps-gpio" >> >> >> >> -- gpios: one PPS GPIO in the format described by ../gpio/gpio.txt >> >> >> >> +- pps-gpios: one PPS GPIO in the format described by >> ../gpio/gpio.txt >> >> >> >> +Alternatively (DEPRECATED), instead of pps-gpios above, >> it may have: >> >> >> >> +- gpios: one PPS GPIO as above >> >> >> >> >> >> >> >> Optional properties: >> >> >> >> - assert-falling-edge: when present, assert is indicated by a >> >> >> falling edge >> >> >> >> (instead of by a rising edge) >> >> >> >> +- capture-clear: when present, also capture the PPS clear event >> >> >> > >> >> >> > Is this a h/w thing? or driver configuration? >> >> >> >> >> >> Driver configuration. Most of the code was present in the driver, yet >> >> >> it was not documented, or usable due to a two line (code) omission >> >> >> (the value was not being fetched from DT). >> >> > >> >> > So what determines how you want to configure this? If the user will >> >> > want to change it, then it should be a sysfs attr and exposed to >> >> > userspace. If it depends on h/w config for a board then it can be in >> >> > DT. >> >> >> >> Sorry, I misled you somewhat. If the PPS pulse active transition from >> >> the hardware is on the falling edge, this flag is required to get the >> >> OS to use that as the active transition. This would not change at the >> >> user's whim but rather it is dependent on connected hardware. >> > >> > This description sounds more like 'assert-falling-edge' than >> 'capture-clear'. >> > >> > I'm still not clear on what 'capture-clear' is. >> >> Ignoring my patch for a minute, the pps_gpio_irq_handler will only >> report a pps PPS_CAPTURECLEAR event if 'capture-clear' is set. As the >> current pps-gpio driver is not able to set this flag, it cannot ever >> report a PPS_CAPTURECLEAR event. >> >> My patch adds the ability to set this flag and adds the documentation >> to go with it. >> Admittedly, I do not require this functionality for what I want, but >> working with the code, I noticed the omission and decided to add it >> for someone else to use it, if they need it. >> >> I am happy to remove this out of my patch, if you feel this to be the >> best way forward. > > I found this prior discussion on adding this[1]. Seems to me this > should be userspace configurable if the GPIO line can interrupt on > both edges. We shouldn't need a DT property to determine that. > > Rob > > [1] https://lore.kernel.org/patchwork/patch/557781/ Hi Rob, the fact that prior knowledge of board/CPU/SOC specifics is required is the most compelling argument for this to be in the DT. This is not something a user should need to know or remember. Userspace is already asking for what they need via the time_pps_setparams call, but to do that they have to first call time_pps_getparams. Time_pps_getparams will not return PPS_CAPTURECLEAR as it is never set in the driver due to it being hardware specific/unable to be set. Tom ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition 2018-11-29 2:05 ` tom burkart @ 2018-11-29 18:00 ` Rob Herring 2018-12-01 22:24 ` tom burkart 0 siblings, 1 reply; 16+ messages in thread From: Rob Herring @ 2018-11-29 18:00 UTC (permalink / raw) To: Tom Burkart; +Cc: linux-kernel, devicetree On Wed, Nov 28, 2018 at 8:05 PM tom burkart <tom@aussec.com> wrote: > > Quoting Rob Herring <robh@kernel.org>: > > > On Mon, Nov 26, 2018 at 9:57 PM tom burkart <tom@aussec.com> wrote: > >> > >> Quoting Rob Herring <robh@kernel.org>: > >> > >> > On Sat, Nov 17, 2018 at 6:35 PM tom burkart <tom@aussec.com> wrote: > >> >> > >> >> Quoting Rob Herring <robh@kernel.org>: > >> >> > >> >> > On Sat, Nov 17, 2018 at 4:35 AM tom burkart <tom@aussec.com> wrote: > >> >> >> > >> >> >> Quoting Rob Herring <robh@kernel.org>: > >> >> >> > >> >> >> > On Wed, Nov 14, 2018 at 11:54:29PM +1100, Tom Burkart wrote: > >> >> >> >> This patch changes the devicetree bindings for the pps-gpio driver > >> >> >> >> from the integer based ABI to the descriptor based ABI. > >> >> >> > ? That has nothing to do with DT. > >> >> >> > >> >> >> I believe it does, as the change in ABI forces a rename in the DT > >> >> >> naming convention. > >> >> >> This is due to the descriptor based ABI appending "-gpio" or > >> >> "-gpios" (see > >> >> >> Documentation/gpio/base.txt.) > >> >> >> Admittedly, I may have called it by the wrong name due to ignorance, > >> >> >> my apologies. > >> >> > > >> >> > If what you say is correct, then you can't change this driver. You'll > >> >> > break compatibility with any existing DT. > >> >> > > >> >> > Changing the binding reasoning should purely be that this is the > >> >> > preferred form. Bindings must be independent from changing kernel > >> >> > APIs. > >> >> > >> >> See comments from Philip Zabel. I misread the documentation and this > >> >> has now been corrected in v8 of the patch. I hope that eliminates all > >> >> comments made above. > >> >> > >> >> >> >> It also adds > >> >> >> >> documentation for the device tree capture-clear option. The legacy > >> >> >> >> device tree entry for the GPIO pin is supported. > >> >> >> >> > >> >> >> >> Signed-off-by: Tom Burkart <tom@aussec.com> > >> >> >> >> --- > >> >> >> >> Documentation/devicetree/bindings/pps/pps-gpio.txt | 8 ++++++-- > >> >> >> >> 1 file changed, 6 insertions(+), 2 deletions(-) > >> >> >> >> > >> >> >> >> diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt > >> >> >> >> b/Documentation/devicetree/bindings/pps/pps-gpio.txt > >> >> >> >> index 3683874832ae..6c9fc0998d94 100644 > >> >> >> >> --- a/Documentation/devicetree/bindings/pps/pps-gpio.txt > >> >> >> >> +++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt > >> >> >> >> @@ -5,19 +5,23 @@ a GPIO pin. > >> >> >> >> > >> >> >> >> Required properties: > >> >> >> >> - compatible: should be "pps-gpio" > >> >> >> >> -- gpios: one PPS GPIO in the format described by ../gpio/gpio.txt > >> >> >> >> +- pps-gpios: one PPS GPIO in the format described by > >> ../gpio/gpio.txt > >> >> >> >> +Alternatively (DEPRECATED), instead of pps-gpios above, > >> it may have: > >> >> >> >> +- gpios: one PPS GPIO as above > >> >> >> >> > >> >> >> >> Optional properties: > >> >> >> >> - assert-falling-edge: when present, assert is indicated by a > >> >> >> falling edge > >> >> >> >> (instead of by a rising edge) > >> >> >> >> +- capture-clear: when present, also capture the PPS clear event > >> >> >> > > >> >> >> > Is this a h/w thing? or driver configuration? > >> >> >> > >> >> >> Driver configuration. Most of the code was present in the driver, yet > >> >> >> it was not documented, or usable due to a two line (code) omission > >> >> >> (the value was not being fetched from DT). > >> >> > > >> >> > So what determines how you want to configure this? If the user will > >> >> > want to change it, then it should be a sysfs attr and exposed to > >> >> > userspace. If it depends on h/w config for a board then it can be in > >> >> > DT. > >> >> > >> >> Sorry, I misled you somewhat. If the PPS pulse active transition from > >> >> the hardware is on the falling edge, this flag is required to get the > >> >> OS to use that as the active transition. This would not change at the > >> >> user's whim but rather it is dependent on connected hardware. > >> > > >> > This description sounds more like 'assert-falling-edge' than > >> 'capture-clear'. > >> > > >> > I'm still not clear on what 'capture-clear' is. > >> > >> Ignoring my patch for a minute, the pps_gpio_irq_handler will only > >> report a pps PPS_CAPTURECLEAR event if 'capture-clear' is set. As the > >> current pps-gpio driver is not able to set this flag, it cannot ever > >> report a PPS_CAPTURECLEAR event. > >> > >> My patch adds the ability to set this flag and adds the documentation > >> to go with it. > >> Admittedly, I do not require this functionality for what I want, but > >> working with the code, I noticed the omission and decided to add it > >> for someone else to use it, if they need it. > >> > >> I am happy to remove this out of my patch, if you feel this to be the > >> best way forward. > > > > I found this prior discussion on adding this[1]. Seems to me this > > should be userspace configurable if the GPIO line can interrupt on > > both edges. We shouldn't need a DT property to determine that. > > > > Rob > > > > [1] https://lore.kernel.org/patchwork/patch/557781/ > > Hi Rob, > the fact that prior knowledge of board/CPU/SOC specifics is required > is the most compelling argument for this to be in the DT. This is not > something a user should need to know or remember. > > Userspace is already asking for what they need via the > time_pps_setparams call, but to do that they have to first call > time_pps_getparams. Time_pps_getparams will not return > PPS_CAPTURECLEAR as it is never set in the driver due to it being > hardware specific/unable to be set. My understanding is it is a 2 step process. The capability is exposed to the user if the hardware supports it, but it is still a user decision whether or not to enable it. So as far as DT bindings are concerned, we're only concerned with the first part. Whether the hardware supports capture clear in the GPIO PPS case just depends on whether the GPIO controller can support both edge interrupts (or emulate that by switching between rising/falling edge interrupts). Do we agree on that part? Needing to know GPIO interrupt capabilities is hardly specific to PPS. There is not any explicit property in DT for this capability because it is implicit based on the GPIO controller's compatible string (or driver capabilities if emulated). Therefore, it is up to the GPIO driver and API to expose this information to client drivers like PPS. Rob ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition 2018-11-29 18:00 ` Rob Herring @ 2018-12-01 22:24 ` tom burkart 0 siblings, 0 replies; 16+ messages in thread From: tom burkart @ 2018-12-01 22:24 UTC (permalink / raw) To: Rob Herring; +Cc: linux-kernel, devicetree Quoting Rob Herring <robh@kernel.org>: > On Wed, Nov 28, 2018 at 8:05 PM tom burkart <tom@aussec.com> wrote: >> >> Quoting Rob Herring <robh@kernel.org>: >> >> > On Mon, Nov 26, 2018 at 9:57 PM tom burkart <tom@aussec.com> wrote: >> >> >> >> Quoting Rob Herring <robh@kernel.org>: >> >> >> >> > On Sat, Nov 17, 2018 at 6:35 PM tom burkart <tom@aussec.com> wrote: >> >> >> >> >> >> Quoting Rob Herring <robh@kernel.org>: >> >> >> >> >> >> > On Sat, Nov 17, 2018 at 4:35 AM tom burkart <tom@aussec.com> wrote: >> >> >> >> >> >> >> >> Quoting Rob Herring <robh@kernel.org>: >> >> >> >> >> >> >> >> > On Wed, Nov 14, 2018 at 11:54:29PM +1100, Tom Burkart wrote: >> >> >> >> >> This patch changes the devicetree bindings for the >> pps-gpio driver >> >> >> >> >> from the integer based ABI to the descriptor based ABI. >> >> >> >> > ? That has nothing to do with DT. >> >> >> >> >> >> >> >> I believe it does, as the change in ABI forces a rename in the DT >> >> >> >> naming convention. >> >> >> >> This is due to the descriptor based ABI appending "-gpio" or >> >> >> "-gpios" (see >> >> >> >> Documentation/gpio/base.txt.) >> >> >> >> Admittedly, I may have called it by the wrong name due to >> ignorance, >> >> >> >> my apologies. >> >> >> > >> >> >> > If what you say is correct, then you can't change this >> driver. You'll >> >> >> > break compatibility with any existing DT. >> >> >> > >> >> >> > Changing the binding reasoning should purely be that this is the >> >> >> > preferred form. Bindings must be independent from changing kernel >> >> >> > APIs. >> >> >> >> >> >> See comments from Philip Zabel. I misread the documentation and this >> >> >> has now been corrected in v8 of the patch. I hope that eliminates all >> >> >> comments made above. >> >> >> >> >> >> >> >> It also adds >> >> >> >> >> documentation for the device tree capture-clear option. >> The legacy >> >> >> >> >> device tree entry for the GPIO pin is supported. >> >> >> >> >> >> >> >> >> >> Signed-off-by: Tom Burkart <tom@aussec.com> >> >> >> >> >> --- >> >> >> >> >> Documentation/devicetree/bindings/pps/pps-gpio.txt | 8 ++++++-- >> >> >> >> >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> >> >> >> >> >> >> >> >> diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> >> >> >> b/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> >> >> >> index 3683874832ae..6c9fc0998d94 100644 >> >> >> >> >> --- a/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> >> >> >> +++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt >> >> >> >> >> @@ -5,19 +5,23 @@ a GPIO pin. >> >> >> >> >> >> >> >> >> >> Required properties: >> >> >> >> >> - compatible: should be "pps-gpio" >> >> >> >> >> -- gpios: one PPS GPIO in the format described by >> ../gpio/gpio.txt >> >> >> >> >> +- pps-gpios: one PPS GPIO in the format described by >> >> ../gpio/gpio.txt >> >> >> >> >> +Alternatively (DEPRECATED), instead of pps-gpios above, >> >> it may have: >> >> >> >> >> +- gpios: one PPS GPIO as above >> >> >> >> >> >> >> >> >> >> Optional properties: >> >> >> >> >> - assert-falling-edge: when present, assert is indicated by a >> >> >> >> falling edge >> >> >> >> >> (instead of by a rising edge) >> >> >> >> >> +- capture-clear: when present, also capture the PPS clear event >> >> >> >> > >> >> >> >> > Is this a h/w thing? or driver configuration? >> >> >> >> >> >> >> >> Driver configuration. Most of the code was present in the >> driver, yet >> >> >> >> it was not documented, or usable due to a two line (code) omission >> >> >> >> (the value was not being fetched from DT). >> >> >> > >> >> >> > So what determines how you want to configure this? If the user will >> >> >> > want to change it, then it should be a sysfs attr and exposed to >> >> >> > userspace. If it depends on h/w config for a board then it can be in >> >> >> > DT. >> >> >> >> >> >> Sorry, I misled you somewhat. If the PPS pulse active transition from >> >> >> the hardware is on the falling edge, this flag is required to get the >> >> >> OS to use that as the active transition. This would not change at the >> >> >> user's whim but rather it is dependent on connected hardware. >> >> > >> >> > This description sounds more like 'assert-falling-edge' than >> >> 'capture-clear'. >> >> > >> >> > I'm still not clear on what 'capture-clear' is. >> >> >> >> Ignoring my patch for a minute, the pps_gpio_irq_handler will only >> >> report a pps PPS_CAPTURECLEAR event if 'capture-clear' is set. As the >> >> current pps-gpio driver is not able to set this flag, it cannot ever >> >> report a PPS_CAPTURECLEAR event. >> >> >> >> My patch adds the ability to set this flag and adds the documentation >> >> to go with it. >> >> Admittedly, I do not require this functionality for what I want, but >> >> working with the code, I noticed the omission and decided to add it >> >> for someone else to use it, if they need it. >> >> >> >> I am happy to remove this out of my patch, if you feel this to be the >> >> best way forward. >> > >> > I found this prior discussion on adding this[1]. Seems to me this >> > should be userspace configurable if the GPIO line can interrupt on >> > both edges. We shouldn't need a DT property to determine that. >> > >> > Rob >> > >> > [1] https://lore.kernel.org/patchwork/patch/557781/ >> >> Hi Rob, >> the fact that prior knowledge of board/CPU/SOC specifics is required >> is the most compelling argument for this to be in the DT. This is not >> something a user should need to know or remember. >> >> Userspace is already asking for what they need via the >> time_pps_setparams call, but to do that they have to first call >> time_pps_getparams. Time_pps_getparams will not return >> PPS_CAPTURECLEAR as it is never set in the driver due to it being >> hardware specific/unable to be set. > > My understanding is it is a 2 step process. The capability is exposed > to the user if the hardware supports it, but it is still a user > decision whether or not to enable it. So as far as DT bindings are > concerned, we're only concerned with the first part. Whether the > hardware supports capture clear in the GPIO PPS case just depends on > whether the GPIO controller can support both edge interrupts (or > emulate that by switching between rising/falling edge interrupts). Do > we agree on that part? Yes and no. While it is desirable to have a complete separation of the two parts as described above, we do live in a world that isn't perfect and we do need to compromise to maximise the benefit to all. See explanation below. > Needing to know GPIO interrupt capabilities is hardly specific to PPS. > There is not any explicit property in DT for this capability because > it is implicit based on the GPIO controller's compatible string (or > driver capabilities if emulated). Therefore, it is up to the GPIO > driver and API to expose this information to client drivers like PPS. Let me pose the following scenario: Say, the GPIO driver/API tell the PPS driver that both edges are indeed supported. This then means that the PPS driver will blindly configure itself for returning data for both the ASSERT and DEASSERT edges of the PPS pulse. However, say, the user only wants the ASSERT edge (as would be the case in most instances). As the kernel currently stands, there is no way for the PPS driver to reconfigure itself after the pps_gpio_probe function exits. This then means that you will automatically be servicing two interrupts per PPS cycle, even though the user only wants one (for PPS this is probably not a big deal...) The reason for the PPS driver to have to blindly configure itself is that there is always going to be a delay between the driver being loaded and the userspace program becoming active and actually requesting what it wants. On a modern x86 system this may be in the order of milliseconds, but on an embedded system this would be in the order of seconds as some systems load all configured modules at boot, before userspace gets control. In the ideal case, it would be nice for the PPS driver to load and only configure itself after the time_pps_setparams function has been called by the user. Here is the compromise: It is usually the hardware vendor/integrator who has a good idea (hopefully) as to what capabilities are going to be used by the system. Hence the devicetree would be the most logical place to put a flag that embodies the capabilities of the GPIO subsystem as well as the GPS or other peripheral device creating the PPS pulses. To me, that is what capture_clear is. Anyway, v11 of my patch now excludes all changes made previously to the capture_clear flag. Tom ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-12-01 22:26 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-14 12:54 [PATCH v7 0/4] PPS: pps-gpio PPS ECHO implementation Tom Burkart 2018-11-14 12:54 ` [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition Tom Burkart 2018-11-14 12:54 ` [PATCH v7 2/4] " Tom Burkart 2018-11-14 12:54 ` [PATCH 3/4] dt-bindings: pps: pps-gpio PPS ECHO implementation Tom Burkart 2018-11-14 12:54 ` [PATCH v7 4/4] pps: pps-gpio pps-echo implementation Tom Burkart 2018-11-16 22:49 ` [PATCH v7 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition Rob Herring 2018-11-17 10:35 ` tom burkart 2018-11-17 14:22 ` Rob Herring 2018-11-18 0:35 ` tom burkart 2018-11-26 19:39 ` Rob Herring 2018-11-27 3:57 ` tom burkart 2018-11-28 17:13 ` Rob Herring 2018-11-29 0:29 ` tom burkart 2018-11-29 2:05 ` tom burkart 2018-11-29 18:00 ` Rob Herring 2018-12-01 22:24 ` tom burkart
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).