Re: [PATCH v1 1/9] gpio: gxp: Add HPE GXP GPIO

[andy . shevchenko]

Tue, Apr 18, 2023 at 10:28:16AM -0500, nick.hawkins@xxxxxxx kirjoitti:
> From: Nick Hawkins <nick.hawkins@xxxxxxx>
> 
> The GXP SoC supports GPIO on multiple interfaces: Host, CPLD and Soc
> pins. The interface from CPLD and Host are interruptable from vic0
> and vic1. This driver allows support for both of these interfaces
> through the use of different compatible bindings.

Thank you for your contribution. To begin with, I don't believe a simple GPIO
driver needs 1000+ LoCs. But see more comments below.

...

> +config GPIO_GXP
> +        tristate "GXP GPIO support"
> +        depends on ARCH_HPE_GXP
> +        select GPIOLIB_IRQCHIP
> +        help
> +	  Say Y here to support GXP GPIO controllers. It provides
> +	  support for the multiple GPIO interfaces available to be
> +	  available to the Host.

The indentation is rather problematic. Had you run checkpatch.pl?

...

> +#include <linux/gpio.h>

No, new code may not include this header.

...

> +#include <linux/of.h>
> +#include <linux/of_device.h>

Why? I haven't seen much evidence of needing of these. What you need are
rather mod_devicetable.h and property.h (see also below).

...

> +#define GPIDATL		0x40
> +#define GPIDATH		0x60
> +#define GPODATL		0xb0
> +#define GPODATH		0xb4
> +#define GPODAT2L	0xf8
> +#define GPODAT2H	0xfc

Use same size of the values, e.g. 0x0fc

> +#define GPOOWNL		0x110
> +#define GPOOWNH		0x114
> +#define GPOOWN2L	0x118
> +#define GPOOWN2H	0x11c

To me this sounds like a 2 8-lines bank GPIO array. Drop those H and L suffixes
and use proper offset calculation based on the bank number. There are plenty of
existing GPIO drivers that do that way.

Also why gpio-regmap is not in use?

...

> +#define GPIO_DIR_OUT	0
> +#define GPIO_DIR_IN	1

Oh, what is this?! You must not interfere with the GPIO_ namespace.

...

> +#define PGOOD_MASK		1

For mask use BIT() / GENMASK()

> +#define PLREG_INT_GRP_STAT_MASK	0x8

Ditto.

...

> +#define PLREG_INT_HI_PRI_EN	0xC

Is it register offset or value?

> +#define PLREG_INT_GRP5_BASE	0x31
> +#define PLREG_INT_GRP6_BASE	0x35
> +#define PLREG_INT_GRP5_FLAG	0x30
> +#define PLREG_INT_GRP6_FLAG	0x34

These need more expanation what they are for and why these specific values are
being used.

...


> +#define PLREG_INT_GRP5_PIN_BASE	59
> +#define PLREG_INT_GRP6_PIN_BASE	90

What are these for?

...

> +enum plreg_gpio_pn {
> +	RESET = 192,
> +	NMI_OUT = 193,
> +	VPBTN = 210,
> +	PGOOD,
> +	PERST,
> +	POST_COMPLETE,

Why these numbers? If it comes from hardware specification, make _all_ them
explicitly assigned.

> +};

...

> +struct gxp_gpio_drvdata {
> +	struct regmap *csm_map;
> +	void __iomem *fn2_vbtn;
> +	struct regmap *fn2_stat;
> +	struct regmap *vuhc0_map;
> +	void __iomem *vbtn;
> +	struct regmap *pl_led;
> +	struct regmap *pl_health;
> +	struct regmap *pl_int;

> +	struct gpio_chip chip;

Move it to be the first member in the structure. Might save a few bytes in the
code. 

> +	int irq;
> +};

...

> +static int gxp_gpio_csm_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
> +	int ret = 0;

> +	switch (offset) {
> +	case 0 ... 31:
> +		regmap_read(drvdata->csm_map, GPIDATL,	&ret);
> +		ret = (ret & BIT(offset)) ? 1 : 0;
> +		break;
> +	case 32 ... 63:
> +		regmap_read(drvdata->csm_map, GPIDATH,	&ret);
> +		ret = (ret & BIT(offset - 32)) ? 1 : 0;
> +		break;
> +	case 64 ... 95:
> +		regmap_read(drvdata->csm_map, GPODATL,	&ret);
> +		ret = (ret & BIT(offset - 64)) ? 1 : 0;
> +		break;
> +	case 96 ... 127:
> +		regmap_read(drvdata->csm_map, GPODATH,	&ret);
> +		ret = (ret & BIT(offset - 96)) ? 1 : 0;
> +		break;
> +	case 128 ...  159:
> +		regmap_read(drvdata->csm_map, GPODAT2L,	&ret);
> +		ret = (ret & BIT(offset - 128)) ? 1 : 0;
> +		break;
> +	case 160 ... 191:
> +		regmap_read(drvdata->csm_map, GPODAT2H,	&ret);
> +		ret = (ret & BIT(offset - 160)) ? 1 : 0;
> +		break;
> +	case 192:
> +		/* SW_RESET */
> +		regmap_read(drvdata->csm_map, 0x5C, &ret);
> +		ret = (ret & BIT(15)) ? 1 : 0;
> +		break;

Besides redundant " ? 1 : 0" parts, see what I wrote above and calculate offset
properly and then apply. Ditto for other functions.

> +	default:
> +		break;
> +	}
> +
> +	return ret;
> +}

I stopped here, this driver requires a lot more work to be done before
considering for upstream, sorry.

...

> +	girq->default_type = IRQ_TYPE_NONE;
> +	girq->handler = handle_edge_irq;

Should be handle_bad_irq() by default.

...

> +	ret = devm_gpiochip_add_data(&pdev->dev, &drvdata->chip, drvdata);
> +	if (ret < 0)
> +		dev_err(&pdev->dev, "Could not register gpiochip for plreg, %d\n",
> +			ret);

Huh?! No bailing out?

...

> +	regmap_update_bits(drvdata->pl_int, PLREG_INT_HI_PRI_EN,
> +			   BIT(4) | BIT(5), BIT(4) | BIT(5));

GENMASK(), but do it as a defined value with meaningful name.

> +	regmap_update_bits(drvdata->pl_int, PLREG_INT_GRP_STAT_MASK,
> +			   BIT(4) | BIT(5), 0x00);

Ditto.

...

> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {

> +		dev_err(&pdev->dev, "Get irq from platform fail - %d\n", ret);

No. The message is already printed by platform code. You are not supposed to
pollute logs with duplicative noise.

> +		return ret;
> +	}

...

> +static const struct of_device_id gxp_gpio_of_match[] = {
> +	{ .compatible = "hpe,gxp-gpio"},
> +	{ .compatible = "hpe,gxp-gpio-pl"},

Missing spaces in above two lines.

> +	{}
> +};

...

> +	const struct of_device_id *match = of_match_device(gxp_gpio_of_match, dev);

device_get_match_data()

...

> +	drvdata = devm_kzalloc(&pdev->dev, sizeof(struct gxp_gpio_drvdata),
> +			       GFP_KERNEL);

sizeof(*drvdata) and make it one line.

> +	if (!drvdata)
> +		return -ENOMEM;

-- 
With Best Regards,
Andy Shevchenko