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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Fri, Sep 16, 2022 at 04:43:47PM +0800, Jianqun Xu wrote:
> The gpio driver for rockchip gpio controller is seperated from rockchip

GPIO

Rockchip (?)

GPIO

> 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

GPIO

> the pinctrl dt node to be its parent node.
> 
> This patch wants to fix driver to support acpi since gpio controller

the driver
ACPI
GPIO

> should work well during acpi is enabled. But during upstream, driver is

ACPI

> better to fix other thing together includes:

Consider splitting to a few patches, if possible.

>  - only get clocks when is_of_node.
>  - get io resource and irq by platform common apis.
>  - use fwnode instead of of_node from device structure.
>  - drop pinctrl related codes.

...

> +struct rockchip_pin_bank {
> +	struct device			*dev;
> +	void __iomem			*reg_base;
> +	struct clk			*clk;
> +	struct clk			*db_clk;
> +	int				irq;
> +	u32				saved_masks;
> +	u32				pin_base;

> +	u8				nr_pins;
> +	char				*name;
> +	u8				bank_num;

Have you ran pahole against this data type? I believe it wastes a lot of bytes
for peanuts on 64-bit machines (due to padding). Grouping u8:s may help.

> +	struct device_node		*of_node;
> +	struct irq_domain		*domain;

> +	struct gpio_chip		gpio_chip;

I would suggest to move this to be first member of the struct as it may save
some bytes of the code (but this needs to be checked by bloat-o-meter to
support or be against the proposal) due to pointer arithmetics.

> +	raw_spinlock_t			slock;
> +	const struct rockchip_gpio_regs	*gpio_regs;
> +	u32				gpio_type;
> +	u32				toggle_edge_mode;
> +};

...

> +#ifdef CONFIG_OF_GPIO
> +	gc->of_node = of_node_get(bank->dev->of_node);
> +#endif

No, please fill the gc->fwnode.

...

> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
> +	int version_id;
> +	int ret;
>  
> +	if (is_of_node(fwnode)) {
> +		bank->clk = of_clk_get(to_of_node(fwnode), 0);
> +		if (IS_ERR(bank->clk))
> +			return PTR_ERR(bank->clk);
>  
> +		ret = clk_prepare_enable(bank->clk);
> +		if (ret < 0)

Where the clock will be put?

> +			return ret;
> +	}
>  
> +	version_id = readl(bank->reg_base + gpio_regs_v2.version_id);
> +	if (version_id == GPIO_TYPE_V2 || version_id == GPIO_TYPE_V2_1) {
>  		bank->gpio_regs = &gpio_regs_v2;
>  		bank->gpio_type = GPIO_TYPE_V2;
>  	} else {
>  		bank->gpio_regs = &gpio_regs_v1;
>  		bank->gpio_type = GPIO_TYPE_V1;
>  	}
>  
> +	if (bank->gpio_type == GPIO_TYPE_V2 && is_of_node(fwnode)) {
> +		bank->db_clk = of_clk_get(to_of_node(fwnode), 1);
> +		if (IS_ERR(bank->db_clk))

Where the clock will be unprepared, disabled, and put?

> +			return PTR_ERR(bank->db_clk);
> +
> +		ret = clk_prepare_enable(bank->db_clk);
> +		if (ret < 0)

Where the clock will be put?

> +			return ret;
> +	}


...

>  	ret = rockchip_gpiolib_register(bank);
>  	if (ret) {
> +		dev_err(bank->dev, "Failed to register gpio %d\n", ret);
>  		clk_disable_unprepare(bank->clk);
> +		clk_disable_unprepare(bank->db_clk);
>  		return ret;
>  	}

See above questions.

-- 
With Best Regards,
Andy Shevchenko



-- 
With Best Regards,
Andy Shevchenko