2021-05-27 08:51:30

by Heiko Stuebner

[permalink] [raw]
Subject: Re: [PATCH v5 5/7] gpio/rockchip: support next version gpio controller

Am Donnerstag, 27. Mai 2021, 09:13:45 CEST schrieb Jianqun Xu:
> The next version gpio controller on SoCs like rk3568 have more write
> mask bits for registers.
>
> Signed-off-by: Jianqun Xu <[email protected]>

> @@ -549,15 +677,33 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank)
> return PTR_ERR(bank->reg_base);
>
> bank->irq = irq_of_parse_and_map(bank->of_node, 0);
> -
> - bank->gpio_regs = &gpio_regs_v1;
> - bank->gpio_type = GPIO_TYPE_V1;
> + if (!bank->irq)
> + return -EINVAL;
>
> bank->clk = of_clk_get(bank->of_node, 0);
> - if (!IS_ERR(bank->clk))
> - return clk_prepare(bank->clk);
> + 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);
> +
> + /* If not gpio v2, that is default to v1. */
> + if (id == GPIO_TYPE_V2) {
> + 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");
> + bank->db_clk = NULL;
> + clk_disable(bank->clk);
> + return -EINVAL;
> + }
> + } else {
> + bank->gpio_regs = &gpio_regs_v1;
> + bank->gpio_type = GPIO_TYPE_V1;
> + }
>
> - bank->clk = NULL;
> + clk_disable(bank->clk);

NIT: you could move the "always enable clock for gpio controller" patch
before adding the v2 support. That way you save on not needing to add
the clock handling here.

Otherwise looks nice to me.

Acked-by: Heiko Stuebner <[email protected]>


Heiko


> return 0;
> }
>
> diff --git a/drivers/pinctrl/pinctrl-rockchip.h b/drivers/pinctrl/pinctrl-rockchip.h
> index 1b774b6bbc3e..589d4d2a98c9 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.h
> +++ b/drivers/pinctrl/pinctrl-rockchip.h
> @@ -121,6 +121,7 @@ struct rockchip_drv {
> * @reg_base: register base of the gpio bank
> * @regmap_pull: optional separate register for additional pull settings
> * @clk: clock of the gpio bank
> + * @db_clk: clock of the gpio debounce
> * @irq: interrupt of the gpio bank
> * @saved_masks: Saved content of GPIO_INTEN at suspend time.
> * @pin_base: first pin number
> @@ -146,6 +147,7 @@ struct rockchip_pin_bank {
> void __iomem *reg_base;
> struct regmap *regmap_pull;
> struct clk *clk;
> + struct clk *db_clk;
> int irq;
> u32 saved_masks;
> u32 pin_base;
>





2021-06-18 06:49:33

by Jianqun Xu

[permalink] [raw]
Subject: Re: Re: [PATCH v5 5/7] gpio/rockchip: support next version gpio controller

Hi Heiko

--------------
[email protected]
>Am Donnerstag, 27. Mai 2021, 09:13:45 CEST schrieb Jianqun Xu:
>> The next version gpio controller on SoCs like rk3568 have more write
>> mask bits for registers.
>>
>> Signed-off-by: Jianqun Xu <[email protected]>
>
>> @@ -549,15 +677,33 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank)
>>  return PTR_ERR(bank->reg_base);
>> 
>>  bank->irq = irq_of_parse_and_map(bank->of_node, 0);
>> -
>> - bank->gpio_regs = &gpio_regs_v1;
>> - bank->gpio_type = GPIO_TYPE_V1;
>> + if (!bank->irq)
>> + return -EINVAL;
>> 
>>  bank->clk = of_clk_get(bank->of_node, 0);
>> - if (!IS_ERR(bank->clk))
>> - return clk_prepare(bank->clk);
>> + 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);
>> +
>> + /* If not gpio v2, that is default to v1. */
>> + if (id == GPIO_TYPE_V2) {
>> + 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");
>> + bank->db_clk = NULL;
>> + clk_disable(bank->clk);
>> + return -EINVAL;
>> + }
>> + } else {
>> + bank->gpio_regs = &gpio_regs_v1;
>> + bank->gpio_type = GPIO_TYPE_V1;
>> + }
>> 
>> - bank->clk = NULL;
>> + clk_disable(bank->clk);
>
>NIT: you could move the "always enable clock for gpio controller" patch
>before adding the v2 support. That way you save on not needing to add
>the clock handling here.
>
V6 has reoder the patches, I havn't add acked-by or reviewed-by yet, please take a look again, thanks

>Otherwise looks nice to me.
>
>Acked-by: Heiko Stuebner <[email protected]>
>
>
>Heiko
>
>
>>  return 0;
>>  }
>> 
>> diff --git a/drivers/pinctrl/pinctrl-rockchip.h b/drivers/pinctrl/pinctrl-rockchip.h
>> index 1b774b6bbc3e..589d4d2a98c9 100644
>> --- a/drivers/pinctrl/pinctrl-rockchip.h
>> +++ b/drivers/pinctrl/pinctrl-rockchip.h
>> @@ -121,6 +121,7 @@ struct rockchip_drv {
>>   * @reg_base: register base of the gpio bank
>>   * @regmap_pull: optional separate register for additional pull settings
>>   * @clk: clock of the gpio bank
>> + * @db_clk: clock of the gpio debounce
>>   * @irq: interrupt of the gpio bank
>>   * @saved_masks: Saved content of GPIO_INTEN at suspend time.
>>   * @pin_base: first pin number
>> @@ -146,6 +147,7 @@ struct rockchip_pin_bank {
>>  void __iomem *reg_base;
>>  struct regmap *regmap_pull;
>>  struct clk *clk;
>> + struct clk *db_clk;
>>  int irq;
>>  u32 saved_masks;
>>  u32 pin_base;
>>
>
>
>
>
>
>
>