Re: [PATCH v2 1/3] pinctrl: Add Qualcomm TLMM driver

[Bjorn Andersson <bjorn.andersson@sonymobile.com>]

On Fri 06 Dec 13:40 PST 2013, Stephen Boyd wrote:

> General nitpick: There seems to be a lot of checks for invalid input in
> the op functions. I hope that they're all unnecessary debugging that can
> be removed.
> 

Most of them checks that a gpio number is not larger than the number of pingroups.
This would be much cleaner to just replace with a validation in probe().

...
> >
> > +config PINCTRL_MSM
> > +     bool
> 
> Why not tristate?
> 

I have a hard time seeing an situation where you would like to build a system
with this driver as a module; it would force almost the entire system to be
loaded at a later time...

> > +/**
> > + * struct msm_pinctrl - state for a pinctrl-msm device
> > + * @dev:            device handle.
> > + * @pctrl:          pinctrl handle.
> > + * @domain:         irqdomain handle.
> > + * @chip:           gpiochip handle.
> > + * @irq:            parent irq for the TLMM irq_chip.
> > + * @lock:           Spinlock to protect register resources as well
> > + *                  as msm_pinctrl data structures.
> > + * @enabled_irqs:   Bitmap of currently enabled irqs.
> > + * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
> > + *                  detection.
> > + * @wake_irqs:      Bitmap of irqs with requested as wakeup source.
> > + * @soc;            Reference to soc_data of platform specific data.
> > + * @regs:           Base address for the TLMM register map.
> > + */
> > +struct msm_pinctrl {
> > +     struct device *dev;
> > +     struct pinctrl_dev *pctrl;
> > +     struct irq_domain *domain;
> > +     struct gpio_chip chip;
> > +     unsigned irq;
> > +
> > +     spinlock_t lock;
> > +
> > +     unsigned long *enabled_irqs;
> > +     unsigned long *dual_edge_irqs;
> > +     unsigned long *wake_irqs;
> 
> In the gpio driver we went with a static upper limit on the bitmap. That
> allowed us to avoid small allocations fragmenting the heap. Please do
> the same here (look at gpio-msm-v2.c).
> 

Sounds reasonable.

> > +static struct pinctrl_ops msm_pinctrl_ops = {
> 
> const?
> 

Of course.

> > +static struct pinmux_ops msm_pinmux_ops = {
> 
> const?
> 

Of course.

> > +static struct pinconf_ops msm_pinconf_ops = {
> 
> const?
> 

Of course.

> > +static int msm_gpio_direction_output(struct gpio_chip *chip, unsigned offset, int value)
> > +{
> > +     const struct msm_pingroup *g;
> > +     struct msm_pinctrl *pctrl = container_of(chip, struct msm_pinctrl, chip);
> > +     unsigned long flags;
> > +     u32 val;
> > +
> > +     if (WARN_ON(offset >= pctrl->soc->ngroups))
> > +             return -EINVAL;
> 
> How is this possible?
> 

If the soc specific ngpios is greater than ngroups this would happen. But it's much better
to catch that earlier!

> > +static void msm_gpio_dbg_show_one(struct seq_file *s,
> > +                               struct pinctrl_dev *pctldev,
> > +                               struct gpio_chip *chip,
> > +                               unsigned offset,
> > +                               unsigned gpio)
> > +{
> > +     const struct msm_pingroup *g;
> > +     struct msm_pinctrl *pctrl = container_of(chip, struct msm_pinctrl, chip);
> > +     unsigned func;
> > +     int is_out;
> > +     int drive;
> > +     int pull;
> > +     u32 ctl_reg;
> > +
> > +     const char *pulls[] = {
> 
> static const char * const ?
> 

Makes sense.
> > +             "no pull",
> > +             "pull down",
> > +             "keeper",
> > +             "pull up"
> > +     };
> > +
> > +     g = &pctrl->soc->groups[offset];
> > +     ctl_reg = readl(pctrl->regs + g->ctl_reg);
> > +
> > +     is_out = !!(ctl_reg & BIT(g->oe_bit));
> > +     func = (ctl_reg >> g->mux_bit) & 7;
> > +     drive = (ctl_reg >> g->drv_bit) & 7;
> > +     pull = (ctl_reg >> g->pull_bit) & 3;
> > +
> > +     seq_printf(s, " %-8s: %-3s %d", g->name, is_out ? "out" : "in", func);
> > +     seq_printf(s, " %dmA", msm_regval_to_drive[drive]);
> > +     seq_printf(s, " %s", pulls[pull]);
> > +}
> > +
> > +static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> > +{
> > +     unsigned gpio = chip->base;
> > +     unsigned i;
> > +
> > +     for (i = 0; i < chip->ngpio; i++, gpio++) {
> > +             msm_gpio_dbg_show_one(s, NULL, chip, i, gpio);
> > +             seq_printf(s, "\n");
> 
> seq_puts()?
> 

OK.

> > +static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> > +{
> > +     const struct msm_pingroup *g;
> > +     struct msm_pinctrl *pctrl;
> > +     unsigned long flags;
> > +     u32 val;
> > +
> > +     pctrl = irq_data_get_irq_chip_data(d);
> > +     if (!pctrl)
> > +             return -EINVAL;
> 
> How is this possible?
> 

As long as probe() is single threaded this should never be an issue, so I think
it makes sense to drop it.

> > +     val = readl(pctrl->regs + g->intr_cfg_reg);
> > +     val |= BIT(g->intr_raw_status_bit);
> > +     if (g->intr_detection_width == 2) {
> > +             val &= ~(3 << g->intr_detection_bit);
> > +             val &= ~(1 << g->intr_polarity_bit);
> > +             switch (type) {
> > +             case IRQ_TYPE_EDGE_RISING:
> > +                     val |= 1 << g->intr_detection_bit;
> > +                     val |= BIT(g->intr_polarity_bit);
> > +                     break;
> > +             case IRQ_TYPE_EDGE_FALLING:
> > +                     val |= 2 << g->intr_detection_bit;
> > +                     val |= BIT(g->intr_polarity_bit);
> > +                     break;
> > +             case IRQ_TYPE_EDGE_BOTH:
> > +                     val |= 3 << g->intr_detection_bit;
> > +                     val |= BIT(g->intr_polarity_bit);
> > +                     break;
> > +             case IRQ_TYPE_LEVEL_LOW:
> > +                     break;
> > +             case IRQ_TYPE_LEVEL_HIGH:
> > +                     val |= BIT(g->intr_polarity_bit);
> > +                     break;
> > +             }
> > +     } else if (g->intr_detection_width == 1) {
> > +             val &= ~(1 << g->intr_detection_bit);
> > +             val &= ~(1 << g->intr_polarity_bit);
> > +             switch (type) {
> > +             case IRQ_TYPE_EDGE_RISING:
> > +                     val |= BIT(g->intr_detection_bit);
> > +                     val |= BIT(g->intr_polarity_bit);
> > +                     break;
> > +             case IRQ_TYPE_EDGE_FALLING:
> > +                     val |= BIT(g->intr_detection_bit);
> > +                     break;
> > +             case IRQ_TYPE_EDGE_BOTH:
> > +                     val |= BIT(g->intr_detection_bit);
> > +                     break;
> > +             case IRQ_TYPE_LEVEL_LOW:
> > +                     break;
> > +             case IRQ_TYPE_LEVEL_HIGH:
> > +                     val |= BIT(g->intr_polarity_bit);
> > +                     break;
> > +             }
> > +     } else {
> > +             BUG();
> > +     }
> 
> This would be better written as a collection of ifs so that
> IRQ_TYPE_EDGE_BOTH doesn't have to be tested and we duplicate less code.
> 

I've rewritten this numerous times and this is the cleanest way I can find
to do this in. Yes, there's some duplication but it has a cleaner structure
and easier to follow than the nested if/elseif/else statements.

So I would like to keep it as is.

> > +     /*
> > +      * Each pin have it's own IRQ status register, so use
> 
> s/have/has/
> 

Thanks.

> > +     /* No interrutps where flagged */
> 
> s/where/were/
> s/interrutps/interrupts/

Thanks.

> > +     pctrl->enabled_irqs = devm_kzalloc(pctrl->dev,
> > +                                        sizeof(unsigned long) * BITS_TO_LONGS(chip->ngpio),
> > +                                        GFP_KERNEL);
> 
> If you don't agree with how gpio-msm-v2.c does it, please use
> devm_kcalloc().
> 

If we're to cause churn I think it's better to go with your suggested
approach.

> > +     pctrl->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> 
> Why not use platform_get_irq()?
> 

I just followed suit, but as I have *pdev here there's no reason to call
irq_of_parse_and_map yet again.

> > +int msm_pinctrl_remove(struct platform_device *pdev)
> > +{
> > +     struct msm_pinctrl *pctrl = platform_get_drvdata(pdev);
> > +     int ret;
> > +
> > +     irq_set_chained_handler(pctrl->irq, NULL);
> > +     irq_domain_remove(pctrl->domain);
> > +     ret = gpiochip_remove(&pctrl->chip);
> > +     pinctrl_unregister(pctrl->pctrl);
> > +
> > +     return 0;
> 
> return ret?
> 

I was intentionally ignoring the return value of gpiochip_remove, but that's of
course incorrect. I'll restructure this to make sense, and care about
gpiochip_remove returning e.g EBUSY.

> > +#include <linux/pinctrl/pinctrl.h>
> > +#include <linux/pinctrl/pinmux.h>
> > +#include <linux/pinctrl/pinconf.h>
> > +#include <linux/pinctrl/machine.h>
> 
> Are any of these includes actually necessary? Can't we just forward
> declare struct pinctrl_pin_desc?
> 

None of them are needed in the current set of patches, as these are already
included in the c-files before including this.

But the right set should be: platform_device.h and pinctrl.h.

> > +
> >
> > +struct msm_pingroup {
> > +     const char *name;
> > +     const unsigned *pins;
> > +     unsigned npins;
> > +
> > +     unsigned funcs[8];
> > +
> > +     s16 ctl_reg;
> > +     s16 io_reg;
> > +     s16 intr_cfg_reg;
> > +     s16 intr_status_reg;
> > +     s16 intr_target_reg;
> 
> Why are these signed? Because the macros assign -1 to them? Perhaps make
> them unsigned and make the macro assign ~0U?
> 

Only reason is that I prefer to have invalid values falling outside the
possibly valid memory range.


Thanks for you review Stephen.

Regards,
Bjorn