On Mon, 2024-09-23 at 18:06 +0800, Billy Tsai wrote:
> Add low-level operations (llops) to abstract the register access for GPIO
> registers and the coprocessor request/release. With this abstraction
> layer, the driver can separate the hardware and software logic, making it
> easier to extend the driver to support different hardware register
> layouts.
>
> Signed-off-by: Billy Tsai <billy_tsai@xxxxxxxxxxxxxx>
> ---
> drivers/gpio/gpio-aspeed.c | 443 +++++++++++++++++++------------------
> 1 file changed, 233 insertions(+), 210 deletions(-)
>
> diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
> index d20e15b2079d..d3994d833684 100644
> --- a/drivers/gpio/gpio-aspeed.c
> +++ b/drivers/gpio/gpio-aspeed.c
> @@ -39,6 +39,10 @@ struct aspeed_bank_props {
> struct aspeed_gpio_config {
> unsigned int nr_gpios;
> const struct aspeed_bank_props *props;
> + const struct aspeed_gpio_llops *llops;
> + const int *debounce_timers_array;
> + int debounce_timers_num;
> + bool require_dcache;
> };
>
> /*
> @@ -178,6 +182,19 @@ enum aspeed_gpio_reg {
> reg_cmdsrc1,
> };
>
> +struct aspeed_gpio_llops {
> + bool (*copro_request)(struct aspeed_gpio *gpio, unsigned int offset);
> + void (*copro_release)(struct aspeed_gpio *gpio, unsigned int offset);
> + void (*reg_bit_set)(struct aspeed_gpio *gpio, unsigned int offset,
> + const enum aspeed_gpio_reg reg, bool val);
> + bool (*reg_bit_get)(struct aspeed_gpio *gpio, unsigned int offset,
> + const enum aspeed_gpio_reg reg);
> + int (*reg_bank_get)(struct aspeed_gpio *gpio, unsigned int offset,
> + const enum aspeed_gpio_reg reg);
> + void (*privilege_ctrl)(struct aspeed_gpio *gpio, unsigned int offset, int owner);
> + void (*privilege_init)(struct aspeed_gpio *gpio);
I made a request down below, so given that, do you mind re-arranging
the member ordering in this struct? It'd be nice to have some flow to
it. Probably just move the copro_* members down the bottom? So:
struct aspeed_gpio_llops {
void (*reg_bit_set)(struct aspeed_gpio *gpio, unsigned int offset,
const enum aspeed_gpio_reg reg, bool val);
bool (*reg_bit_get)(struct aspeed_gpio *gpio, unsigned int offset,
const enum aspeed_gpio_reg reg);
int (*reg_bank_get)(struct aspeed_gpio *gpio, unsigned int offset,
const enum aspeed_gpio_reg reg);
void (*privilege_ctrl)(struct aspeed_gpio *gpio, unsigned int offset, int owner);
void (*privilege_init)(struct aspeed_gpio *gpio);
bool (*copro_request)(struct aspeed_gpio *gpio, unsigned int offset);
void (*copro_release)(struct aspeed_gpio *gpio, unsigned int offset);
};
*snip*
> @@ -1182,7 +1205,7 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
> if (!gpio_id)
> return -EINVAL;
>
> - gpio->clk = of_clk_get(pdev->dev.of_node, 0);
> + gpio->clk = devm_clk_get_enabled(&pdev->dev, 0);
Nice - however, can you please make this its own patch and add a Fixes:
tag?
Otherwise I think the patch is looking okay. I'll try to get some
testing done.
Andrew
