Re: 回复: 回复: [PATCH v4 04/20] firmware: scmi: add pin control protocol support to SCMI agent

[Marek Vasut]

On 2/24/25 3:45 AM, Peng Fan wrote:

[...]

>>>>>>> @@ -436,6 +442,11 @@ static int scmi_bind_protocols(struct udevice
>>>> *dev)
>>>>>>>      				drv = DM_DRIVER_GET(scmi_voltage_domain);
>>>>>>>      			}
>>>>>>>      			break;
>>>>>>> +		case SCMI_PROTOCOL_ID_PINCTRL:
>>>>>>> +			if (IS_ENABLED(CONFIG_PINCTRL_IMX_SCMI) &&
>>>>>>
>>>>>> Is this pinctrl protocol really imx specific ?
>>>>>>
>>>>>> If not, this needs to use some other config option to gate access to it.
>>>>>
>>>>> Currently, it is used for some product families of the i.MX9 series products.
>>>> Is the protocol iMX specific or is it generic protocol ?
>>>
>>> SCMI_PROTOCOL_ID_PINCTRL is not unique to iMX, but drivers/pinctrl/nxp/pinctrl-scmi.c (drv = DM_DRIVER_GET(scmi_pinctrl_imx)) is only for iMX.
>> This patch is changing common code, it shouldn't be littered with
>> vendor-specific ifdeffery or if(IS_ENABLED(...))ery . Can this be made fully
>> generic, similar to e.g. regulator protocol ?
> 
> In Linux Kernel, there are two drivers, pinctrl-scmi.c and pinctrl-imx-scmi.c.
> Both follows ARM SCMI 3.2, but pinctrl-imx-scmi has some special settings
> to align with i.mx iomuxc array based settings, mux,input,pad and etc.
> 
> In gerneral, imx part could be merged with pinctrl-scmi.c but that will
> make code not clean.
In that case, why does U-Boot not have pinctrl-imx-scmi.c to contain the 
iMX customization too ? If this protocol is IMX specific, it shouldn't 
be ifdeffed in common code.