Re: [PATCH 2/3] pinctrl: meson-a1: add pinctrl driver for Meson A1 Soc

[Qianggui Song]

On 2019/9/17 22:07, Jerome Brunet wrote:
> 
> On Tue 17 Sep 2019 at 13:51, Qianggui Song <qianggui.song@xxxxxxxxxxx> wrote:
>>>> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
>>>> index 8bba9d0..885b89d 100644
>>>> --- a/drivers/pinctrl/meson/pinctrl-meson.c
>>>> +++ b/drivers/pinctrl/meson/pinctrl-meson.c
>>>> @@ -688,8 +688,12 @@ static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc,
>>>>  
>>>>  	pc->reg_ds = meson_map_resource(pc, gpio_np, "ds");
>>>>  	if (IS_ERR(pc->reg_ds)) {
>>>> -		dev_dbg(pc->dev, "ds registers not found - skipping\n");
>>>> -		pc->reg_ds = NULL;
>>>> +		if (pc->data->reg_layout == A1_LAYOUT) {
>>>> +			pc->reg_ds = pc->reg_pullen;
>>>
>>> IMO, this kind of ID based init fixup is not going to scale and will
>>> lead to something difficult to maintain in the end.
>>>
>>> The way the different register sets interract with each other is already
>>> pretty complex to follow.
>>>
>>> You could rework this in 2 different ways:
>>> #1 - Have the generic function parse all the register sets and have all
>>> drivers provide a specific (as in gxbb, gxl, axg, etc ...)  function to :
>>>  - Verify the expected sets have been provided
>>>  - Make assignement fixup as above if necessary
>>>
>>> #2 - Rework the driver to have only one single register region
>>>  I think one of your colleague previously mentionned this was not
>>>  possible. It is still unclear to me why ...
>>>
>> Appreciate your advice.  I have an idea based on #1, how about providing
>> only two dt parse function, one is for chips before A1(the old one),
>> another is for A1 and later chips that share the same layout. Assign
>> these two functions to their own driver.
> 
> That's roughly the same thing as your initial proposition with function
> pointer instead of IDs ... IMO, this would still be a quick fix to
> address your immediate topic instead of dealing with the driver as
> whole, which is my concern here.
> 
For #1. It would be like
generic_parse_dt()
{
	1. parse all register regions (mux gpio pull pull_en ds)
	
	2. call  specific function through function pointer in
 	   meson_pinctrl_data.(each platform should have AO and EE two
           specific functions for they are not the same)
	{
		do work you mentioned above
	}
}
right ?
If that so, maybe there are a lot of duplicated codes for most Socs
share the same reg layout. So I guess five specific functions are
enough: AXG and before(ao,ee), G12A(ao,ee) and A1(will place them in
pinctrl_meson.c). Since m8 to AXG are the same register layout for both
ee and ao, G12A with new feature ds and new ao register layout.

Or I misunderstood the #1 ?
>>>> +		} else {
>>>> +			dev_dbg(pc->dev, "ds registers not found - skipping\n");
>>>> +			pc->reg_ds = NULL;
>>>> +		}
>>>>  	}
>>>>  
>>>>  	return 0;
>>>> diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h
>>>> index c696f32..3d0c58d 100644
>>>> --- a/drivers/pinctrl/meson/pinctrl-meson.h
>>>> +++ b/drivers/pinctrl/meson/pinctrl-meson.h
>>>> @@ -80,6 +80,14 @@ enum meson_pinconf_drv {
>>>>  };
>>>>  
>>>>  /**
>>>> + * enum meson_reg_layout - identify two types of reg layout
>>>> + */
>>>> +enum meson_reg_layout {
>>>> +	LEGACY_LAYOUT,
>>>> +	A1_LAYOUT,
>>>> +};
>>>> +
>>>> +/**
>>>>   * struct meson bank
>>>>   *
>>>>   * @name:	bank name
>>>> @@ -114,6 +122,7 @@ struct meson_pinctrl_data {
>>>>  	unsigned int num_banks;
>>>>  	const struct pinmux_ops *pmx_ops;
>>>>  	void *pmx_data;
>>>> +	unsigned int reg_layout;
>>>>  };
>>>>  
>>>>  struct meson_pinctrl {
>>>
>>> .
>>>
> 
> .
>