Re: [PATCH v8 2/4] pps: descriptor-based gpio, capture-clear addition

[tom burkart]

Hi Philipp,

Quoting Philipp Zabel <philipp.zabel@xxxxxxxxx>:

> On Sat, Nov 17, 2018 at 2:07 PM Tom Burkart <tom@xxxxxxxxxx> 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.

Yes, that is in patch 1/4

> > Signed-off-by: Tom Burkart <tom@xxxxxxxxxx>
> > ---
> >  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?

Oops, yes, in this patch (2/4), but not in patch 4/4

> > +
> > +       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.

Ok, I am happy to agree as it makes sense.

> > +       } 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.

Thanks.

> > +       }
> > +       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,

Fine with me.

> Reviewed-by: Philipp Zabel <philipp.zabel@xxxxxxxxx>

Is this for patch 2/4 only or the others as well?

Will generate v10 of the patch and post it again.