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

[Stephen Boyd <sboyd@codeaurora.org>]

On 12/10/13 00:10, Bjorn Andersson wrote:
> On Fri 06 Dec 13:40 PST 2013, Stephen Boyd wrote:
>>> +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...

We're going to need to build everything but the essentials as modules
for the multi-platform kernel because we're going to run into the
problem where the kernel can't link anymore. Data heavy drivers such as
this are  targets for that because they waste more space being built-in
and they're not absolutely necessary to boot into an initrd that holds
the modules for a particular device.

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

Isn't this the same?

+     val = readl(pctrl->regs + g->intr_cfg_reg);
+     val |= BIT(g->intr_raw_status_bit);
+
+     detection_mask = BIT(g->intr_detection_width) - 1;
+     val &= ~(detection_mask << g->intr_detection_bit);
+     val &= ~BIT(g->intr_polarity_bit);
+
+     if (type & IRQ_TYPE_EDGE_RISING)
+             val |= BIT(g->intr_detection_bit);
+
+     if (type & IRQ_TYPE_EDGE_FALLING)
+             val |= g->intr_detection_width << g->intr_detection_bit;
+
+     if (!(type & IRQ_TYPE_LEVEL_LOW))
+             val |= BIT(g->intr_polarity_bit);
+

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

We should be able to get away with forward declaring the structs we care
about. C files that include this header should be including the
pinctrl.h header file anyway, no?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation