Hi Tom, On Sat, Nov 17, 2018 at 2:07 PM Tom Burkart <t...@aussec.com> wrote: > > This patch changes the GPIO access for the pps-gpio driver from the > integer based ABI to the descriptor based ABI. It also adds the > extraction of the device tree capture-clear option.
Is the capture-clear property documented in Documentation/devicetree/bindings/pps/pps-gpio.txt? If not, you should add a binding documentation patch. > Signed-off-by: Tom Burkart <t...@aussec.com> > --- > drivers/pps/clients/pps-gpio.c | 80 > +++++++++++++++++++++--------------------- > include/linux/pps-gpio.h | 3 +- > 2 files changed, 41 insertions(+), 42 deletions(-) > > diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c > index 333ad7d5b45b..d2fbc91dc8fc 100644 > --- a/drivers/pps/clients/pps-gpio.c > +++ b/drivers/pps/clients/pps-gpio.c > @@ -31,7 +31,7 @@ > #include <linux/slab.h> > #include <linux/pps_kernel.h> > #include <linux/pps-gpio.h> > -#include <linux/gpio.h> > +#include <linux/gpio/consumer.h> > #include <linux/list.h> > #include <linux/of_device.h> > #include <linux/of_gpio.h> > @@ -41,9 +41,9 @@ 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; > }; > > /* > @@ -61,18 +61,49 @@ static irqreturn_t pps_gpio_irq_handler(int irq, void > *data) > > info = data; > > - rising_edge = gpio_get_value(info->gpio_pin); > + 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; > + int ret; Unused variable? > + > + if (pdata) { > + data->gpio_pin = pdata->gpio_pin; > + > + data->assert_falling_edge = pdata->assert_falling_edge; > + data->capture_clear = pdata->capture_clear; This is just a matter of personal taste, so feel free to ignore: I would keep the pdata branch in pps_gpio_probe and call this function pps_gpio_dt_setup, to reduce indentation of the OF branch. > + } else { > + data->gpio_pin = devm_gpiod_get(&pdev->dev, > + NULL, /* request "gpios" */ > + 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; Those two should use the of_property_read_bool wrapper. > + } > + return 0; > +} > + > static unsigned long > get_irqf_trigger_flags(const struct pps_gpio_device_data *data) > { > @@ -90,53 +121,23 @@ 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), Could use sizeof(*data) here. Otherwise, Reviewed-by: Philipp Zabel <philipp.za...@gmail.com> regards Philipp