Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Oleksii Moisieiev <Oleksii_Moisieiev@epam.com>
To:     "andy.shevchenko@gmail.com" <andy.shevchenko@gmail.com>
CC:     "sudeep.holla@arm.com" <sudeep.holla@arm.com>,
        Cristian Marussi <cristian.marussi@arm.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>
Subject: Re: [PATCH v3 3/4] pinctrl: Implementation of the generic
 scmi-pinctrl driver
Thread-Topic: [PATCH v3 3/4] pinctrl: Implementation of the generic
 scmi-pinctrl driver
Thread-Index: AQHZmJMWFmItseqHB0qRWKbh+9Pp/K9+8aOAgEPuLAA=
Date:   Thu, 20 Jul 2023 13:40:03 +0000
Message-ID: <87sf9ihfc7.fsf@epam.com>
References: <cover.1686063941.git.oleksii_moisieiev@epam.com>
 <43109a0f2f362222fca79e2afd15c46ed9a32977.1686063941.git.oleksii_moisieiev@epam.com>
 <ZIAxLdexyKBnMOmU@surfacebook>
In-Reply-To: <ZIAxLdexyKBnMOmU@surfacebook>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?iso-8859-1?Q?v/W0LAfuZkQ0Q9wOuFURznfWO+g7ZVBLsmA5vRpFLrmp9NuYj8zt++kLOc?=
 =?iso-8859-1?Q?2gkjf53Et4FRzVMZ19W1yBROD0sOx9jykztG1luxNe3aO4v+fsv6L85xD+?=
 =?iso-8859-1?Q?kaZRaP0ncqN6pCwaZW8bNPfHnujpNmF50vRlAnOnGFt/IbPQhUqWcZZbO9?=
 =?iso-8859-1?Q?FGvMUd0VjxlBKgHnjOJlXpl+C8jAq6KQfWdd2Zq19atPXDRgpXyaoltOki?=
 =?iso-8859-1?Q?nMob8Znju6Z3H/yMlWI2F1ka6pqL0SD9UGdJme3qReECXP9I98LSf6frC4?=
 =?iso-8859-1?Q?O5b2Dl0h3xOLw93PzKEGTGvDO59jZqOB/X33pfI/I6sROAFuNLiso0Vjdm?=
 =?iso-8859-1?Q?Kuvq1UpSPExqXSBNed7tsxSQAmqQ/SaUUBb7e97Am/5kKrDAkdoa7ZVQkO?=
 =?iso-8859-1?Q?A/2zlyMD2QL7VizyfhMF0XNRuS35PNgj2envwgo5LgmZSwZqw2GQRU+rRG?=
 =?iso-8859-1?Q?Fclgf/dfZ0rav9IGUoFxStYDuDILSs1AJel4PdiJzY8WbjuSiSgeDD/Oaj?=
 =?iso-8859-1?Q?TKX7bRcQhL0g8EjOWI4w4Lt7vem/Zar+IrrSamdIvq1ECQGmvPv0vIuw51?=
 =?iso-8859-1?Q?YTll+tEjVmmpMkxmMqhHCqmc/PUmvYeN76GvYQ/lTYBcY3yn5PTVceuT57?=
 =?iso-8859-1?Q?TYXOHw3JdxVCkJdgbNLp4CfpPALoeIfD3Dx3dYqae8Tv7w2yP0L7LXtor5?=
 =?iso-8859-1?Q?NtlOkLEuQIojR2JF4C/Dsqomj4MOWIsvoatl4zBQjHIzCK/a7UaCXQ8ROP?=
 =?iso-8859-1?Q?V8AB/jdG483dvyxKVMQY0pmkfPloofAqMwaEQ9BVEFpSu1ABVVTfMD9k1X?=
 =?iso-8859-1?Q?q7tZrUWAXxh0zGxpbDFX1PqMql5jXxiXV/pmukVI3BnbknhrMWwXiPX0Lh?=
 =?iso-8859-1?Q?RTX/eoNKe6+Vw5jV0znqA44ARvvNRUQX4g3azhPmPl9OdU0J7ou0vi7o8q?=
 =?iso-8859-1?Q?+o0qkMQgnYLjvOj20yj1Pn/OF1Q+phhoNT/s3vrWZCaObLPoCd0PkzewKe?=
 =?iso-8859-1?Q?xKWfN/05U5oUiRxRf69OQ7/PVf9aXfn1ke4Ws1MP/p1B1vGMr6l/U89Hep?=
 =?iso-8859-1?Q?qhvJ0qGkJ7BKnT4ABXqDsC0lXf+FwjH2KYkh69lqbptIxx5BY72KSL5dXf?=
 =?iso-8859-1?Q?gReqzrs00TDlEZh4Rs6TnK5SMMhdaDH4FGJYE4XJ2MSyQjUv4LCS7hUxRQ?=
 =?iso-8859-1?Q?5neWqWK9zj9NJGyJdhlzlJDFD634Iiyx3uFZ4ZiNJnWiIXKCOyf80o2paI?=
 =?iso-8859-1?Q?TQ1P4oDv7s8zlNFDRehlXA8uoPhB0g+D6uc0RN3qXUs0Y9ycnGg0Yl/oA6?=
 =?iso-8859-1?Q?PgmQ3oZ1xvot+r0DabBFhuItElC6kMH8ITx/Kzg887kVv8uv5bP1f7unsH?=
 =?iso-8859-1?Q?hqlj3BmJpVYbgnKQF1XZP8mzQ8Wnqcg0bOFr8p2dW851MlflAgR+pUrk2o?=
 =?iso-8859-1?Q?SXIK7IpJOfWCKTV2Rw7r3jY4Uf3ffkIqy6fchfHVWf4zdtcljIHn7gFzJi?=
 =?iso-8859-1?Q?I55qd66sZJpP0zzjn9LPGoYoOci3geGU1Y1kpHDxjFKFMTBQeqUG5VULvE?=
 =?iso-8859-1?Q?XUYICmzQDnsHRYbd1LR6PqDs0EIXU4xe9fMclqDEeo6hdDDdY1R9MZCaCX?=
 =?iso-8859-1?Q?T8FbrhUm2nSOu17N8lPpibx4uFY5E7j8KOxuAYP5bbYJks8BjSUtVekQ?=
 =?iso-8859-1?Q?=3D=3D?=
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: epam.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PA4PR03MB7136.eurprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9b4b8f8a-1742-4487-61fe-08db8926d237
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Jul 2023 13:40:03.1320
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b41b72d0-4e9f-4c26-8a69-f949f367c91d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: AtB1YI0gnslAWas55OCZxW3GFuvwalRavLOULEepXUW0OGrg3Ao2pLY8YH+9eK41dMoyTkqlQTN2OkTUU/TdC5FRUqg1o9yq3NFU/7fJkCA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: GV1PR03MB8381
X-Proofpoint-ORIG-GUID: akm6161UO6vh71FCwdshpDHl0EP-BG7A
X-Proofpoint-GUID: akm6161UO6vh71FCwdshpDHl0EP-BG7A
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26
 definitions=2023-07-20_06,2023-07-20_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 suspectscore=0 malwarescore=0
 impostorscore=0 mlxscore=0 adultscore=0 phishscore=0 lowpriorityscore=0
 mlxlogscore=999 priorityscore=1501 clxscore=1015 spamscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2306200000
 definitions=main-2307200115
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


Hi Andy,

andy.shevchenko@gmail.com writes:

> Tue, Jun 06, 2023 at 04:22:28PM +0000, Oleksii Moisieiev kirjoitti:
>> scmi-pinctrl driver implements pinctrl driver interface and using
>> SCMI protocol to redirect messages from pinctrl subsystem SDK to
>> SCP firmware, which does the changes in HW.
>>=20
>> This setup expects SCP firmware (or similar system, such as ATF)
>> to be installed on the platform, which implements pinctrl driver
>> for the specific platform.
>>=20
>> SCMI-Pinctrl driver should be configured from the device-tree and uses
>> generic device-tree mappings for the configuration.

[snip]

> ...
>
>> +error:
>
> Labels shoud be self-explanatory, i.e. they should tell what _will_ be wh=
en goto.
>
>> +	devm_kfree(pmx->dev, pmx->functions[selector].groups);
>
> Red Flag. Please, elaborate.
>

Thank you for the review.
I did some research regarding this and now I'm confused. Could you
please explain to me why it's a red flag?
IIUC devm_alloc/free functions are the calls to the resource-managed
alloc/free command, which is bound to the device.
pinctrl-scmi driver does devm_pinctrl_register_and_init which does
devres_alloc and doesn't open devres_group like
scmi_alloc_init_protocol_instance (thanks to Cristian detailed
explanation).

As was mentioned in Documentation/driver-api/driver-model/devres.rst:

```
No matter what, all devres entries are released on driver detach.  On
release, the associated release function is invoked and then the
devres entry is freed.
```

Also there is devm_pinctrl_get call listed in the managed interfaces.

My understanding is that all resources, bound to the particular device
will be freed on driver detach.

Also I found some examples of using devm_alloc/free like from dt_node_to_ma=
p
call in pinctrl-simple.c driver.

I agree that I need to implement .remove callback with proper cleanup,
but why can't I use devm_* here?
Maybe I've misunderstood your point.

--=20
Thanks,
Oleksii=