Re: [PATCH v6 4/7] pinctrl: s32: convert the driver into an mfd cell

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On 19/11/2024 10:57, Andrei Stefanescu wrote:
> Hi Krzysztof,
> 
>>> +	if (npins < 0)
>>> +		return dev_err_probe(dev, -EINVAL,
>>> +				     "Failed to read 'pinmux' in node %s\n",
>>> +				     grp->data.name);
>>
>> I do not see how this change is related. Looks you are mixing cleanups
>> with refactoring into MFD cell. These are two different things.
> 
> Yes, I also included some small refactoring changes. I didn't think they were
> important enough to include them in a separate commit. Would you like me to separate
> them in another commit?

You cannot include such changes along other, meaningful changes. This
does not apply to this patch only but all contributions. There is a
clear policy how cleanups, bugs and new things are being upstreamed:
https://elixir.bootlin.com/linux/v6.12/source/Documentation/process/submitting-patches.rst#L168

Please read above document very carefully. This is v6 and we still
circle around absolute basics.

> 
>>> -	if (mem_regions == 0 || mem_regions >= 10000) {
>>> -		dev_err(&pdev->dev, "mem_regions is invalid: %u\n", mem_regions);
>>> -		return -EINVAL;
>>> -	}
>>> +	/* one MSCR and one IMCR region per SIUL2 module */
>>
>> How is this related to converion into MFD cell?
> 
> We no longer parse the device tree to configure the regmaps, we instead
> get them from the mfd driver. This is the main point of converting this
> driver into an mfd cell. 
> 
>>
>> Still looks like an ABI break.
>>
> 
> Yes, the driver no longer adheres to the nxp,s32g2-siul2-pinctrl.yaml binding.

I did not find in commit msg explanation that this is ABI break and why
it is allowed. I asked for it.

> 
> The intention is to deprecate that binding since it doesn't correctly describe
> the hardware. I am not sure on how to do this. I thought that changing this
> driver and removing the old compatible would be enough.

No, you cannot break the ABI. Either you deprecate this or just don't touch.

> 
> Would it be better to add another file which would contain the old probing
> function(and match the old binding) so clients would be able to select the
> old implementation?

I don't understand that. Your driver is supposed to keep ABI. Not
through some selection but just as is.

Best regards,
Krzysztof