Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <e28a7788-e4b5-d777-4775-d57a00b0fbcc@linaro.org>
Date:   Tue, 18 Apr 2023 19:02:23 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v1 1/9] gpio: gxp: Add HPE GXP GPIO
Content-Language: en-US
To:     nick.hawkins@hpe.com, verdun@hpe.com, linus.walleij@linaro.org,
        brgl@bgdev.pl, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, jdelvare@suse.com,
        linux@roeck-us.net, linux@armlinux.org.uk,
        linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
References: <20230418152824.110823-1-nick.hawkins@hpe.com>
 <20230418152824.110823-2-nick.hawkins@hpe.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230418152824.110823-2-nick.hawkins@hpe.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 18/04/2023 17:28, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> 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@hpe.com>
> ---
>  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