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
