Re: [PATCH v2 3/3] gpio: Add Spreadtrum PMIC EIC driver support

Hi Andy,

On 25 February 2018 at 20:19, Andy Shevchenko [off-list ref] wrote:

On Sat, Feb 24, 2018 at 12:44 PM, Baolin Wang [off-list ref] wrote:

quoted

The Spreadtrum PMIC EIC controller contains only one bank of debounce EIC,
and this bank contains 16 EICs. Each EIC can only be used as input mode,
as well as supporting the debounce and the capability to trigger interrupts
when detecting input signals.

quoted

+/*
+ * These registers are modified under the irq bus lock and cached to avoid
+ * unnecessary writes in bus_sync_unlock.
+ */
+enum { REG_IEV, REG_IE, REG_TRIG, CACHE_NR_REGS };

One item per line.

Sure.

quoted

+static int sprd_pmic_eic_direction_input(struct gpio_chip *chip,
+                                        unsigned int offset)
+{
+       /* EICs are always input, nothing need to do here. */
+       return 0;
+}
+
+static void sprd_pmic_eic_set(struct gpio_chip *chip, unsigned int offset,
+                             int value)
+{
+       /* EICs are always input, nothing need to do here. */
+}

Remove both.

Look at what GPIO core does.

I've checked the GPIO core, we need the
sprd_pmic_eic_direction_input() returns 0, since user can set GPIOD_IN
flag when requesting one GPIO, otherwise it will return errors.
We also need one dummy sprd_pmic_eic_set() when setting debounce for
one GPIO, otherwise it will return errors.

quoted

+       value |= debounce / 1000;

Possible overflow.

OK. I should & SPRD_PMIC_EIC_DBC_MASK.

quoted

+       for (n = 0; n < chip->ngpio; n++) {
+               if (!(BIT(n) & val))

for_each_set_bit().

At some point you may need just to go across lib/ in the kernel and
see what we have there.

I've considered the for_each_set_bit(), it need one 'unsigned long'
type parameter, but we get the value from regmap is 'u32' type. So we
need one extra conversion from 'u32' to 'unsigned long' like:

unsigned long reg = val;

for_each_set_bit(n, &reg, chip->ngpio) {
        .......
}

If you like this conversion, then I can change to use
for_each_set_bit(). Thanks.

-- 
Baolin.wang
Best Regards