[LinuxPPS] gpio-pps with echopin

[Rodolfo Giometti]

On Mon, Apr 13, 2015 at 06:36:58PM +0200, f.hiesinger wrote:
> +static void pps_gpio_echo(struct pps_device *pps, int event,
> +		void *data)
> +{
> +		const struct pps_gpio_device_data *info = data;
> +
> +		/* TODO: make polarity of echopin configurable (invertable)
> +		 * TODO: toggle echopin if capture_clear is not set */

Don't use TODO... echo pin polarity can be the same of the source pin.

> +		if (event & PPS_CAPTUREASSERT)
> +			gpio_set_value(info->gpio_echo_pin, SIGNAL);
> +		else
> +			gpio_set_value(info->gpio_echo_pin, NO_SIGNAL);
> +}
>  
>  static unsigned long
>  get_irqf_trigger_flags(const struct pps_gpio_device_data *data)
> @@ -91,6 +113,7 @@ static int pps_gpio_probe(struct platform_device *pdev)
>  {
>  	struct pps_gpio_device_data *data;
>  	const char *gpio_label;
> +	const char *gpio_echo_label;
>  	int ret;
>  	int pps_default_params;
>  	const struct pps_gpio_platform_data *pdata = pdev->dev.platform_data;
> @@ -103,12 +126,20 @@ static int pps_gpio_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	if (pdata) {
> +		/* Non-DT based instantiation */
>  		data->gpio_pin = pdata->gpio_pin;
> +		data->gpio_echo_pin = pdata->gpio_echo_pin;
>  		gpio_label = pdata->gpio_label;
> +		gpio_echo_label = pdata->gpio_echo_label;
>  
>  		data->assert_falling_edge = pdata->assert_falling_edge;
>  		data->capture_clear = pdata->capture_clear;
> +		dev_dbg(&pdev->dev, "1) data->assert_falling_edge = %d , \
> +				 data->capture_clear = %d\n ", data->assert_falling_edge, \
> +				 data->capture_clear);
> +
>  	} else {
> +		/* DT based instantiation */
>  		ret = of_get_gpio(np, 0);
>  		if (ret < 0) {
>  			dev_err(&pdev->dev, "failed to get GPIO from device tree\n");
> @@ -117,8 +148,23 @@ static int pps_gpio_probe(struct platform_device *pdev)
>  		data->gpio_pin = ret;
>  		gpio_label = PPS_GPIO_NAME;
>  
> +		if (of_get_property(np, "echo-enable", NULL)) {

Nak, you can count how much pins has been defined into gpios property
to decide if the echo function is needed.

> +			dev_dbg(&pdev->dev, "'echo-enable'-property found");
> +			ret = of_get_gpio(np, 1);
> +			if (ret < 0) {
> +				dev_err(&pdev->dev, "failed to get Echo-GPIO from device tree\n");
> +				return ret;
> +			}
> +			data->gpio_echo_pin = ret;
> +			gpio_echo_label = "pps-gpio-echo";
> +		}
>  		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;
> +		dev_dbg(&pdev->dev, "2) data->assert_falling_edge = %d , \
> +				data->capture_clear = %d\n ", data->assert_falling_edge, \
> +				data->capture_clear);

What this chunk is for?

>  	}
>  
>  	/* GPIO setup */
> @@ -135,6 +181,21 @@ static int pps_gpio_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	if (of_get_property(np, "echo-enable", NULL)) {

Why using this function again? You can properly set the gpio_echo_pin
to signal if it has been used or not (don't forget to consider the
non-DT based systems).

> +		ret = devm_gpio_request(&pdev->dev, data->gpio_echo_pin, \
> +				gpio_echo_label);
> +			if (ret) {
> +				dev_err(&pdev->dev, "failed to request Echo-GPIO %u\n", \
> +						data->gpio_echo_pin);
> +				return ret;
> +		}
> +		ret = gpio_direction_output(data->gpio_echo_pin, 0);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to set (echo)pin direction\n");
> +			return -EINVAL;
> +		}
> +	}
> +
>  	/* IRQ setup */
>  	ret = gpio_to_irq(data->gpio_pin);
>  	if (ret < 0) {
> @@ -150,13 +211,24 @@ static int pps_gpio_probe(struct platform_device *pdev)
>  		data->info.mode |= PPS_CAPTURECLEAR | PPS_OFFSETCLEAR |
>  			PPS_ECHOCLEAR;
>  	data->info.owner = THIS_MODULE;
> +	dev_dbg(&pdev->dev, "1)data->info.mode = %x\n", data->info.mode);
> +	dev_dbg(&pdev->dev, "1)data->info.echo = %p\n", (void *)data->info.echo);

Nak. Please replace with:

     dev_dbg(&pdev->dev, "data->info.mode = %x\n", data->info.mode);
     dev_dbg(&pdev->dev, "data->info.echo = %p\n", (void *) data->info.echo);

However printing a function address is not so useful...

> +	if (of_get_property(np, "echo-enable", NULL))
> +		/* register echo function */
> +		data->info.echo = pps_gpio_echo;
> +
> +	dev_dbg(&pdev->dev, "2)data->info.echo = %p \n", (void *) data->info.echo);

Nak. Use just one dev_dbg(&pdev->dev, "data->info.echo = ...

>  	snprintf(data->info.name, PPS_MAX_NAME_LEN - 1, "%s.%d",
>  		 pdev->name, pdev->id);
>  
>  	/* register PPS source */
> -	pps_default_params = PPS_CAPTUREASSERT | PPS_OFFSETASSERT;
> +	pps_default_params = PPS_CAPTUREASSERT | PPS_OFFSETASSERT | PPS_ECHOASSERT;
>  	if (data->capture_clear)
> -		pps_default_params |= PPS_CAPTURECLEAR | PPS_OFFSETCLEAR;
> +		pps_default_params |= PPS_CAPTURECLEAR | PPS_OFFSETCLEAR | PPS_ECHOCLEAR;
> +	dev_dbg(&pdev->dev, "1)pps_default_params = %x\n", pps_default_params);

Nak. Please replace with:

dev_dbg(&pdev->dev, "pps_default_params = %x\n", pps_default_params);

>  	data->pps = pps_register_source(&data->info, pps_default_params);
>  	if (data->pps == NULL) {
>  		dev_err(&pdev->dev, "failed to register IRQ %d as PPS source\n",
> @@ -183,7 +255,7 @@ static int pps_gpio_probe(struct platform_device *pdev)
>  static int pps_gpio_remove(struct platform_device *pdev)
>  {
>  	struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
> -
> +	/* TODO: Check if gpio_free(...) is necessary */

Drop TODO... patches must be a complete work.

>  	pps_unregister_source(data->pps);
>  	dev_info(&pdev->dev, "removed IRQ %d as PPS source\n", data->irq);
>  	return 0;
> @@ -208,6 +280,6 @@ static struct platform_driver pps_gpio_driver = {
>  module_platform_driver(pps_gpio_driver);
>  MODULE_AUTHOR("Ricardo Martins <rasm at fe.up.pt>");
>  MODULE_AUTHOR("James Nuss <jamesnuss at nanometrics.ca>");
> -MODULE_DESCRIPTION("Use GPIO pin as PPS source");
> +MODULE_DESCRIPTION("Use GPIO pins as PPS source & echo");
>  MODULE_LICENSE("GPL");
> -MODULE_VERSION("1.0.0");
> +MODULE_VERSION("1.0.3");

The code below is against the Linux vanilla? If not just drop it.

> diff --git a/linux/arch/arm/boot/dts/pps-gpio-overlay.dts b/linux/arch/arm/boot/dts/pps-gpio-overlay.dts
> index 5832dc9..e759605 100644
> --- a/linux/arch/arm/boot/dts/pps-gpio-overlay.dts
> +++ b/linux/arch/arm/boot/dts/pps-gpio-overlay.dts
> @@ -10,10 +10,11 @@
>  				compatible = "pps-gpio";
>  				pinctrl-names = "default";
>  				pinctrl-0 = <&pps_pins>;
> -				gpios = <&gpio 18 0>, <&gpio 21 1>; 
> +				gpios = <&gpio 18 0>, <&gpio 21 1>; /* input, output */ 
>  				status = "okay";
>  				assert-falling-edge;
>  				capture-clear;
> +				echo-enable;
>  			};
>  		};
>  	};
> @@ -32,7 +33,7 @@
>  	__overrides__ {
>  		gpiopin = <&pps>,"gpios:4",
>  			      <&pps_pins>,"brcm,pins:0";
> -		gpioechopin = <&pps>,"gpios:16 ",
> +		gpioechopin = <&pps>,"gpios:16",
>  					  <&pps_pins>,"brcm,pins:4";
>  	};
>  };

Ciao,

Rodolfo

-- 

HCE Engineering                      e-mail: giometti at hce-engineering.com
GNU/Linux Solutions                          giometti at enneenne.com
Linux Device Driver                          giometti at linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Cosino Project - the quick prototyping embedded system - www.cosino.io
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it