Re: [PATCH v3] gpio: add Intel WhiskeyCove GPIO driver

From: Andy Shevchenko
Date: Wed Jun 22 2016 - 04:28:31 EST


On Tue, Jun 21, 2016 at 9:54 PM, Bin Gao <bin.gao@xxxxxxxxxxxxxxx> wrote:
> On Tue, Jun 21, 2016 at 02:19:57AM +0300, Andy Shevchenko wrote:

>> > + help
>> > + Support for GPIO pins on Whiskey Cove PMIC.
>> > +
>> > + Say Yes if you have a Intel SoC based tablet with Whiskey Cove PMIC
>>
>> What if I have not a tablet with WC PMIC inside?
>
> This driver is for the GPIO controller in the WC PMIC. If there is no WC PMIC,
> this driver shouldn't be compiled.
> You question is more like:
> If a device doesn't exist, what will its driver do?

My comment regarding to 'tablet' word used wrongly in my opinion.

>> > +#include <linux/seq_file.h>
>> > +#include <linux/bitops.h>
>> > +#include <linux/regmap.h>
>> > +#include <linux/interrupt.h>
>> > +#include <linux/platform_device.h>
>> > +#include <linux/gpio/driver.h>
>> > +#include <linux/mfd/intel_soc_pmic.h>
>>
>> Alphabetical order?
>
> I checked Documentation/CodingStyle but didn't find any alphabetical order
> description. Is this newly enforced?

Not enforced, but it allows you not to duplicate headers and easily
find one in the list.

>
>> > +#define GROUP0_NR_IRQS 7
>> > +#define GROUP1_NR_IRQS 6
>> > +#define IRQ_MASK_BASE 0x4e19
>> > +#define IRQ_STATUS_BASE 0x4e0b
>>
>> Can you define all your bases as a) offsets by value, and b) with
>> _OFFSET suffix instead of _BASE, though second one is up to you.
>
> Strictly speaking, it's called "offset base". I'm really not sure
> it will be more readable to replace BASE with OFFSET here.

OK.

>> > +#define CTLI_INTCNT_DIS (0)
>>
>> (0 << 1) or...
>>
>> > +#define CTLI_INTCNT_NE (1 << 1)
>> > +#define CTLI_INTCNT_PE (2 << 1)
>> > +#define CTLI_INTCNT_BE (3 << 1)
>>
>> ...INTCNT(x) ((x) << 1)
>> ...DIS 0
>> ...NE 1
>>
>> and so on.
>>
>> Same for all similar blocks of constants.
>
> Makes sense.

Please, choose variant that's more suitable and less verbose.

>> > +#define CTLO_DRV_MASK (1 << 4)
>> > +#define CTLO_DRV_OD (0)
>> > +#define CTLO_DRV_CMOS CTLO_DRV_MASK
>> > +
>> > +#define CTLO_DRV_REN (1 << 3)
>> > +
>> > +#define CTLO_RVAL_2KDW (0)
>> > +#define CTLO_RVAL_2KUP (1 << 1)
>> > +#define CTLO_RVAL_50KDW (2 << 1)
>> > +#define CTLO_RVAL_50KUP (3 << 1)
>>
>> Wait, are you sure that's is pure gpio and not a full pinctrl (pinconf) + gpio?
>
> No, we don't have pinctrl for this driver. Those bits are just for pure GPIO setting.

I noticed you are not using bias (yet?). For now I have no strong opinion.
I suppose Linus can share his thoughts.

>> > +static void wcove_bus_lock(struct irq_data *data)
>> > +{
>> > + struct wcove_gpio *wg = gpiochip_get_data(
>> > + irq_data_get_irq_chip_data(data));
>> > +
>> > + mutex_lock(&wg->buslock);
>>
>> I suppose you have to add a hint to static analyzer here and below.
>
> I didn't quite get it. You meant to add some comments for these 2 functions?

One function takes lock, the other releases.
Is static analyzer happy with it?

>> > +}
>> > +
>> > +static void wcove_bus_sync_unlock(struct irq_data *data)
>> > +{
>> > + struct wcove_gpio *wg = gpiochip_get_data(
>> > + irq_data_get_irq_chip_data(data));

>> > +static struct platform_driver wcove_gpio_driver = {
>> > + .driver = {
>> > + .name = "bxt_wcove_gpio",
>>
>> No PM?
>
> Right, no PM for this driver.

Perhaps couple of words in the commit message that the WC GPIO IP is
a) power gated automaticaly, or b) AON, or c) actual case.

--
With Best Regards,
Andy Shevchenko