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

[Krzysztof Kozlowski]

On 18/04/2023 17:28, nick.hawkins@xxxxxxx wrote:
> 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.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@xxxxxxx>
> ---
>  drivers/gpio/Kconfig    |    9 +
>  drivers/gpio/Makefile   |    1 +
>  drivers/gpio/gpio-gxp.c | 1056 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1066 insertions(+)
>  create mode 100644 drivers/gpio/gpio-gxp.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 13be729710f2..47435307698c 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1235,6 +1235,15 @@ config HTC_EGPIO
>  	  several HTC phones.  It provides basic support for input
>  	  pins, output pins, and IRQs.
>  
> +config GPIO_GXP
> +        tristate "GXP GPIO support"
> +        depends on ARCH_HPE_GXP

|| COMPILE_TEST

(...)

> +
> +	drvdata->chip.parent = &pdev->dev;
> +	ret = devm_gpiochip_add_data(&pdev->dev, &drvdata->chip, NULL);
> +	if (ret < 0)
> +		dev_err(&pdev->dev,
> +			"Could not register gpiochip for fn2, %d\n", ret);
> +	dev_info(&pdev->dev, "HPE GXP FN2 driver loaded.\n");

Drop non-informative probe successes. They are not needed.


> +
> +	return 0;
> +}
> +
> +static int gxp_gpio_pl_init(struct platform_device *pdev)
> +{
> +	struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(&pdev->dev);
> +	struct gpio_irq_chip *girq;
> +	int ret;
> +	unsigned int val;
> +
> +	drvdata->pl_led = gxp_gpio_init_regmap(pdev, "pl-led", 8);
> +	if (IS_ERR(drvdata->pl_led))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->pl_int),
> +				     "failed to map pl_led\n");
> +
> +	drvdata->pl_health = gxp_gpio_init_regmap(pdev, "pl-health", 8);
> +	if (IS_ERR(drvdata->pl_health))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->pl_int),
> +				     "failed to map pl_health\n");
> +
> +	drvdata->pl_int = gxp_gpio_init_regmap(pdev, "pl-int", 8);
> +	if (IS_ERR(drvdata->pl_int))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->pl_int),
> +				     "failed to map pl_int\n");
> +
> +	drvdata->chip = plreg_chip;
> +	drvdata->chip.ngpio = 100;
> +	drvdata->chip.parent = &pdev->dev;
> +
> +	girq = &drvdata->chip.irq;
> +	girq->chip = &gxp_plreg_irqchip;
> +	girq->parent_handler = NULL;
> +	girq->num_parents = 0;
> +	girq->parents = NULL;
> +	girq->default_type = IRQ_TYPE_NONE;
> +	girq->handler = handle_edge_irq;
> +
> +	girq->init_hw = gxp_gpio_irq_init_hw;
> +
> +	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);
> +
> +	regmap_update_bits(drvdata->pl_int, PLREG_INT_HI_PRI_EN,
> +			   BIT(4) | BIT(5), BIT(4) | BIT(5));
> +	regmap_update_bits(drvdata->pl_int, PLREG_INT_GRP_STAT_MASK,
> +			   BIT(4) | BIT(5), 0x00);
> +	regmap_read(drvdata->pl_int, PLREG_INT_HI_PRI_EN, &val);
> +	regmap_read(drvdata->pl_int, PLREG_INT_GRP_STAT_MASK, &val);
> +
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Get irq from platform fail - %d\n", ret);
> +		return ret;

return dev_err_probe

> +	}
> +
> +	drvdata->irq = ret;
> +
> +	ret = devm_request_irq(&pdev->dev, drvdata->irq, gxp_gpio_pl_irq_handle,
> +			       IRQF_SHARED, "gxp-pl", drvdata);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "IRQ handler failed - %d\n", ret);

return dev_err_probe
Actually other applicable places as well...

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id gxp_gpio_of_match[] = {
> +	{ .compatible = "hpe,gxp-gpio"},
> +	{ .compatible = "hpe,gxp-gpio-pl"},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, gxp_gpio_of_match);
> +
> +static int gxp_gpio_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct gxp_gpio_drvdata *drvdata;
> +	struct device *dev = &pdev->dev;
> +	struct device *parent;
> +	const struct of_device_id *match = of_match_device(gxp_gpio_of_match, dev);
> +
> +	if (!match) {
> +		dev_err(dev, "device is not compatible");

It is not possible.

> +		return -EINVAL;
> +	}
> +
> +	parent = dev->parent;
> +	if (!parent) {
> +		dev_err(dev, "no parent\n");

Why do you need this check?

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

sizeof(*)

> +			       GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, drvdata);
> +
> +	if (strcmp(match->compatible, "hpe,gxp-gpio-pl") == 0)

No, use match data. Anyway this is open-coding of OF functions...

> +		ret = gxp_gpio_pl_init(pdev);
> +	else
> +		ret = gxp_gpio_init(pdev);
> +
> +	return ret;

Best regards,
Krzysztof