Re: [PATCH 6/6] gpio: rockchip: support acpi

[Johan Jonker <jbx6244@xxxxxxxxx>]

On 9/13/22 03:12, jay.xu@xxxxxxxxxxxxxx wrote:
> Hi Johan
> 
> --------------
> jay.xu@xxxxxxxxxxxxxx
>> Hi Jianqun,
>>
>> Some comments about clocks. Have a look if it's usefull.
>>
>> On 9/9/22 11:05, Jianqun Xu wrote:
>>> The gpio driver for rockchip gpio controller is seperated from rockchip
>>> pinctrl driver, at the first version, it keeps things original to make
>>> the patch easy to be reviewed, such as the gpio driver must work with
>>> the pinctrl dt node to be its parent node.
>>>
>>> This patch wants to fix driver to support acpi since gpio controller
>>> should work well during acpi is enabled. But during upstream, driver is
>>> better to fix other thing together includes:
>>>   - add 'clock-names' to allow driver to get clocks by devm_clk_get().
>>>   - get io resource and irq by platform common apis.
>>>   - use fwnode instead of of_node from device structure.
>>>
>>> Signed-off-by: Jianqun Xu <jay.xu@xxxxxxxxxxxxxx>
>>> ---
>>>   drivers/gpio/gpio-rockchip.c | 197 ++++++++++++++++++++++-------------
>>>   1 file changed, 122 insertions(+), 75 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
>>> index ebb50c25a461..ca012cf199a6 100644
>>> --- a/drivers/gpio/gpio-rockchip.c
>>> +++ b/drivers/gpio/gpio-rockchip.c
>>> @@ -6,9 +6,9 @@
>>>    * Copyright (c) 2021 Rockchip Electronics Co. Ltd.
>>>    */
>>>  
>>> +#include <linux/acpi.h>
>>>   #include <linux/bitops.h>
>>>   #include <linux/clk.h>
>>> -#include <linux/device.h>
>>>   #include <linux/err.h>
>>>   #include <linux/gpio/driver.h>
>>>   #include <linux/init.h>
>>> @@ -16,10 +16,9 @@
>>>   #include <linux/io.h>
>>>   #include <linux/module.h>
>>>   #include <linux/of.h>
>>> -#include <linux/of_address.h>
>>> -#include <linux/of_device.h>
>>> -#include <linux/of_irq.h>
>>> +#include <linux/platform_device.h>
>>>   #include <linux/pinctrl/pinconf-generic.h>
>>> +#include <linux/property.h>
>>>   #include <linux/regmap.h>
>>>  
>>>   #include "../pinctrl/core.h"
>>> @@ -29,6 +28,8 @@
>>>   #define GPIO_TYPE_V2	(0x01000C2B)  /* GPIO Version ID 0x01000C2B */
>>>   #define GPIO_TYPE_V2_1	(0x0101157C)  /* GPIO Version ID 0x0101157C */
>>>  
>>> +#define GPIO_MAX_PINS	(32)
>>> +
>>>   static const struct rockchip_gpio_regs gpio_regs_v1 = {
>>>   .port_dr = 0x00,
>>>   .port_ddr = 0x04,
>>> @@ -200,6 +201,9 @@ static int rockchip_gpio_set_debounce(struct gpio_chip *gc,
>>>   if (bank->gpio_type == GPIO_TYPE_V2 && !IS_ERR(bank->db_clk)) {
>>>   div_debounce_support = true;
>>>   freq = clk_get_rate(bank->db_clk);
>>> +	if (!freq)
>>> +	return -EINVAL;
>>> +
>>>   max_debounce = (GENMASK(23, 0) + 1) * 2 * 1000000 / freq;
>>>   if (debounce > max_debounce)
>>>   return -EINVAL;
>>> @@ -507,15 +511,16 @@ static int rockchip_interrupts_register(struct rockchip_pin_bank *bank)
>>>   struct irq_chip_generic *gc;
>>>   int ret;
>>>  
>>> -	bank->domain = irq_domain_add_linear(bank->of_node, 32,
>>> -	&irq_generic_chip_ops, NULL);
>>> +	bank->domain = irq_domain_create_linear(dev_fwnode(bank->dev),
>>> +	GPIO_MAX_PINS,
>>> +	&irq_generic_chip_ops, NULL);
>>>   if (!bank->domain) {
>>>   dev_warn(bank->dev, "could not init irq domain for bank %s\n",
>>>   bank->name);
>>>   return -EINVAL;
>>>   }
>>>  
>>> -	ret = irq_alloc_domain_generic_chips(bank->domain, 32, 1,
>>> +	ret = irq_alloc_domain_generic_chips(bank->domain, GPIO_MAX_PINS, 1,
>>>        "rockchip_gpio_irq",
>>>        handle_level_irq,
>>>        clr, 0, 0);
>>> @@ -565,7 +570,8 @@ static int rockchip_interrupts_register(struct rockchip_pin_bank *bank)
>>>   return 0;
>>>   }
>>>  
>>> -static int rockchip_gpiolib_register(struct rockchip_pin_bank *bank)
>>> +static int rockchip_gpiolib_register(struct rockchip_pin_bank *bank,
>>> +	     struct pinctrl_dev *pctldev)
>>>   {
>>>   struct gpio_chip *gc;
>>>   int ret;
>>> @@ -578,6 +584,17 @@ static int rockchip_gpiolib_register(struct rockchip_pin_bank *bank)
>>>   gc->label = bank->name;
>>>   gc->parent = bank->dev;
>>>  
>>> +	if (!gc->base)
>>> +	gc->base = GPIO_MAX_PINS * bank->bank_num;
>>> +	if (!gc->ngpio)
>>> +	gc->ngpio = GPIO_MAX_PINS;
>>> +	if (!gc->label) {
>>> +	gc->label = devm_kasprintf(bank->dev, GFP_KERNEL, "gpio%d",
>>> +	   bank->bank_num);
>>> +	if (!gc->label)
>>> +	return -ENOMEM;
>>> +	}
>>> +
>>>   ret = gpiochip_add_data(gc, bank);
>>>   if (ret) {
>>>   dev_err(bank->dev, "failed to add gpiochip %s, %d\n",
>>> @@ -595,17 +612,7 @@ static int rockchip_gpiolib_register(struct rockchip_pin_bank *bank)
>>>   * files which don't set the "gpio-ranges" property or systems that
>>>   * utilize ACPI the driver has to call gpiochip_add_pin_range().
>>>   */
>>> -	if (!of_property_read_bool(bank->of_node, "gpio-ranges")) {
>>> -	struct device_node *pctlnp = of_get_parent(bank->of_node);
>>> -	struct pinctrl_dev *pctldev = NULL;
>>> -
>>> -	if (!pctlnp)
>>> -	return -ENODATA;
>>> -
>>> -	pctldev = of_pinctrl_get(pctlnp);
>>> -	if (!pctldev)
>>> -	return -ENODEV;
>>> -
>>> +	if (!device_property_read_bool(bank->dev, "gpio-ranges") && pctldev) {
>>>   ret = gpiochip_add_pin_range(gc, dev_name(pctldev->dev), 0,
>>>        gc->base, gc->ngpio);
>>>   if (ret) {
>>> @@ -628,45 +635,49 @@ static int rockchip_gpiolib_register(struct rockchip_pin_bank *bank)
>>>   return ret;
>>>   }
>>>  
>>> -static int rockchip_get_bank_data(struct rockchip_pin_bank *bank)
>>> +static void rockchip_gpio_set_regs(struct rockchip_pin_bank *bank)
>>>   {
>>> -	struct resource res;
>>> -	int id = 0;
>>> -
>>> -	if (of_address_to_resource(bank->of_node, 0, &res)) {
>>> -	dev_err(bank->dev, "cannot find IO resource for bank\n");
>>> -	return -ENOENT;
>>> -	}
>>> -
>>> -	bank->reg_base = devm_ioremap_resource(bank->dev, &res);
>>> -	if (IS_ERR(bank->reg_base))
>>> -	return PTR_ERR(bank->reg_base);
>>> -
>>> -	bank->irq = irq_of_parse_and_map(bank->of_node, 0);
>>> -	if (!bank->irq)
>>> -	return -EINVAL;
>>> -
>>> -	bank->clk = of_clk_get(bank->of_node, 0);
>>> -	if (IS_ERR(bank->clk))
>>> -	return PTR_ERR(bank->clk);
>>> -
>>> -	clk_prepare_enable(bank->clk);
>>> -	id = readl(bank->reg_base + gpio_regs_v2.version_id);
>>> +	int id = readl(bank->reg_base + gpio_regs_v2.version_id);
>>>  
>>>   /* If not gpio v2, that is default to v1. */
>>>   if (id == GPIO_TYPE_V2 || id == GPIO_TYPE_V2_1) {
>>>   bank->gpio_regs = &gpio_regs_v2;
>>>   bank->gpio_type = GPIO_TYPE_V2;
>>> -	bank->db_clk = of_clk_get(bank->of_node, 1);
>>> -	if (IS_ERR(bank->db_clk)) {
>>> -	dev_err(bank->dev, "cannot find debounce clk\n");
>>> -	clk_disable_unprepare(bank->clk);
>>> -	return -EINVAL;
>>> -	}
>>>   } else {
>>>   bank->gpio_regs = &gpio_regs_v1;
>>>   bank->gpio_type = GPIO_TYPE_V1;
>>>   }
>>> +}
>>> +
>>> +static int rockchip_gpio_get_clocks(struct rockchip_pin_bank *bank)
>>> +{
>>> +	struct device *dev = bank->dev;
>>> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
>>> +	int ret;
>>> +
>>> +	if (!is_of_node(fwnode))
>>> +	return 0;
>>> +
>>
>>> +	bank->clk = devm_clk_get(dev, "bus");
>>
>> .. but existing DTs also come without clock-name and must still work.
>>

> Could some one can share me that does the driver should work well with the old dt files?
> I think that if some one update the kernel, they should update the driver and the dt files ?
> the new driver with a new core dt files, now if i update the dt files and driver once time, should
> the driver still need to work with old dt files ?

There's one or more comments in the past by Heiko which I just can't find in the "lore archive" about that right now.

Because it's a too abrupt change. Kernel DT changes/bindings have a more long term commitment in it.
We shouldn't be in a situation that someones kernel doesn't work any more when he/she upgrades and that just happened after your patch was applied.
The use of clocks without clock-names was a valid case in the past. When adding new futures that same functionality should continue to be available as expected. 

> 
>> Check clock-names first else fallback to clock index or keep using of_clk_get().
>> Up to the GPIO maintainer of course.
>> Not sure if a clk index function for devm exist, else maybe use: ???
>>
>> 	bank->clk = clk_get(dev, "bus");
>> vs.
>> 	bank->clk = of_clk_get_by_name(np, "bus");
>>
>>
>> 	if (IS_ERR(bank->clk)) {
>> 	bank->clk = of_clk_get(bank->of_node, 0);
>> 	if (IS_ERR(bank->clk))
>> 	return PTR_ERR(bank->clk);
>>
>> Could you check if this still works?
>>
>> ====
>>
>> Previous comments:
>>
>>>> You can't mix devm_ with non-devm_ calls.
>>>>
>>> Okay, I add 'clock-names' property for all existed related dt files and fix driver to only use
>>> devm_clk_get(), I push a version please help to review
>>
>> ====
>>
>>> +	if (IS_ERR(bank->clk)) {
>>> +	dev_err(dev, "fail to get apb clock\n");
>>> +	return PTR_ERR(bank->clk);
>>> +	}
>>> +
>>
>>> +	ret = clk_prepare_enable(bank->clk);
>>> +	if (ret < 0)
>>> +	return ret;
>>> +
>>
>> Block order: first find all your clocks then enable.
>> Get your resources first.
>>
>>> +	bank->db_clk = devm_clk_get(dev, "db");
>>> +	if (IS_ERR(bank->db_clk)) {
>>> +	bank->db_clk = NULL;
>>> +	}
>>
>> The clock-name "db" is only available in rk356x.dtsi
>>
>>> +
>>> +	ret = clk_prepare_enable(bank->db_clk);
>>> +	if (ret < 0) {
>>
>> It is not an error if it doesn't exists.
>> One might use the function devm_clk_get_optional().
>>
>>> +	clk_disable_unprepare(bank->clk);
>>> +	return ret;
>>> +	}
>>>  
>>>   return 0;
>>>   }
>>> @@ -690,57 +701,86 @@ rockchip_gpio_find_bank(struct pinctrl_dev *pctldev, int id)
>>>   return found ? bank : NULL;
>>>   }
>>>  
>>> -static int rockchip_gpio_probe(struct platform_device *pdev)
>>> +static int rockchip_gpio_get_bank_id(struct device *dev)
>>>   {
>>> -	struct device *dev = &pdev->dev;
>>> -	struct device_node *np = dev->of_node;
>>> -	struct device_node *pctlnp = of_get_parent(np);
>>> -	struct pinctrl_dev *pctldev = NULL;
>>> -	struct rockchip_pin_bank *bank = NULL;
>>> -	struct rockchip_pin_deferred *cfg;
>>> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
>>> +	int bank_id = -EINVAL;
>>> +	u64 uid;
>>>   static int gpio;
>>> -	int id, ret;
>>>  
>>> -	if (!np || !pctlnp)
>>> -	return -ENODEV;
>>> +	if (is_acpi_node(fwnode)) {
>>> +	if (!acpi_dev_uid_to_integer(ACPI_COMPANION(dev), &uid))
>>> +	bank_id = (int)uid;
>>> +	} else {
>>> +	bank_id = of_alias_get_id(to_of_node(fwnode), "gpio");
>>> +	if (bank_id < 0)
>>> +	bank_id = gpio++;
>>> +	}
>>>  
>>> -	pctldev = of_pinctrl_get(pctlnp);
>>> -	if (!pctldev)
>>> -	return -EPROBE_DEFER;
>>> +	return bank_id;
>>> +}
>>>  
>>> -	id = of_alias_get_id(np, "gpio");
>>> -	if (id < 0)
>>> -	id = gpio++;
>>> +static int rockchip_gpio_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct rockchip_pin_bank *bank;
>>> +	struct pinctrl_dev *pctldev;
>>> +	int bank_id;
>>> +	int ret;
>>>  
>>> -	bank = rockchip_gpio_find_bank(pctldev, id);
>>> -	if (!bank)
>>> -	return -EINVAL;
>>> +	bank_id = rockchip_gpio_get_bank_id(dev);
>>> +	if (bank_id < 0)
>>> +	return bank_id;
>>>  
>>> +	pctldev = get_pinctrl_dev_from_devname("pinctrl");
>>> +	if (pctldev) {
>>> +	bank = rockchip_gpio_find_bank(pctldev, bank_id);
>>> +	if (!bank)
>>> +	return -ENODEV;
>>> +	} else {
>>> +	bank = devm_kzalloc(dev, sizeof(*bank), GFP_KERNEL);
>>> +	if (!bank)
>>> +	return -ENOMEM;
>>> +	}
>>> +
>>> +	bank->bank_num = bank_id;
>>>   bank->dev = dev;
>>> -	bank->of_node = np;
>>>  
>>> -	raw_spin_lock_init(&bank->slock);
>>> +	bank->reg_base = devm_platform_ioremap_resource(pdev, 0);
>>> +	if (IS_ERR(bank->reg_base))
>>> +	return PTR_ERR(bank->reg_base);
>>>  
>>> -	ret = rockchip_get_bank_data(bank);
>>> +	bank->irq = platform_get_irq(pdev, 0);
>>> +	if (bank->irq < 0)
>>> +	return bank->irq;
>>> +
>>> +	ret = rockchip_gpio_get_clocks(bank);
>>>   if (ret)
>>>   return ret;
>>>  
>>> +	raw_spin_lock_init(&bank->slock);
>>> +	rockchip_gpio_set_regs(bank);
>>> +
>>>   /*
>>>   * Prevent clashes with a deferred output setting
>>>   * being added right at this moment.
>>>   */
>>>   mutex_lock(&bank->deferred_lock);
>>>  
>>> -	ret = rockchip_gpiolib_register(bank);
>>> +	ret = rockchip_gpiolib_register(bank, pctldev);
>>>   if (ret) {
>>> -	clk_disable_unprepare(bank->clk);
>>> -	mutex_unlock(&bank->deferred_lock);
>>> -	return ret;
>>> +	dev_err(bank->dev, "Failed to register gpio %d\n", ret);
>>> +	goto err_unlock;
>>>   }
>>>  
>>>   while (!list_empty(&bank->deferred_pins)) {
>>> +	struct rockchip_pin_deferred *cfg;
>>> +
>>>   cfg = list_first_entry(&bank->deferred_pins,
>>>          struct rockchip_pin_deferred, head);
>>> +	if (!cfg)
>>> +	break;
>>> +
>>>   list_del(&cfg->head);
>>>  
>>>   switch (cfg->param) {
>>> @@ -765,9 +805,15 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
>>>   mutex_unlock(&bank->deferred_lock);
>>>  
>>>   platform_set_drvdata(pdev, bank);
>>> -	dev_info(dev, "probed %pOF\n", np);
>>> +	dev_info(dev, "probed %pfw\n", dev_fwnode(dev));
>>>  
>>>   return 0;
>>> +err_unlock:
>>> +	mutex_unlock(&bank->deferred_lock);
>>> +	clk_disable_unprepare(bank->clk);
>>> +	clk_disable_unprepare(bank->db_clk);
>>> +
>>> +	return ret;
>>>   }
>>>  
>>>   static int rockchip_gpio_remove(struct platform_device *pdev)
>>> @@ -775,6 +821,7 @@ static int rockchip_gpio_remove(struct platform_device *pdev)
>>>   struct rockchip_pin_bank *bank = platform_get_drvdata(pdev);
>>>  
>>>   clk_disable_unprepare(bank->clk);
>>> +	clk_disable_unprepare(bank->db_clk);
>>>   gpiochip_remove(&bank->gpio_chip);
>>>  
>>>   return 0;