Message-ID: <a724814a-2517-4855-a183-e80117ab4b01@collabora.com>
Date: Thu, 18 Sep 2025 12:34:53 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>
Cc: sboyd@...nel.org, jic23@...nel.org, dlechner@...libre.com,
 nuno.sa@...log.com, andy@...nel.org, arnd@...db.de,
 gregkh@...uxfoundation.org, srini@...nel.org, vkoul@...nel.org,
 kishon@...nel.org, sre@...nel.org, krzysztof.kozlowski@...aro.org,
 linux-arm-msm@...r.kernel.org, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org,
 linux-pm@...r.kernel.org, kernel@...labora.com, wenst@...omium.org,
 casey.connolly@...aro.org, Jonathan Cameron <jonathan.cameron@...wei.com>,
 Neil Armstrong <neil.armstrong@...aro.org>
Subject: Re: [PATCH v4 1/7] spmi: Implement spmi_subdevice_alloc_and_add() and
 devm variant

Il 17/09/25 16:57, Uwe Kleine-König ha scritto:
> Hello AngeloGioacchino,
> 
> On Wed, Sep 17, 2025 at 01:41:40PM +0200, AngeloGioacchino Del Regno wrote:
>> Il 16/09/25 15:25, Uwe Kleine-König ha scritto:
>>> Hello AngeloGioacchino,
>>>
>>> On Tue, Sep 16, 2025 at 10:44:39AM +0200, AngeloGioacchino Del Regno wrote:
>>>> +/**
>>>> + * spmi_subdevice_alloc_and_add(): Allocate and add a new SPMI sub-device
>>>> + * @sparent:	SPMI parent device with previously registered SPMI controller
>>>> + *
>>>> + * Returns:
>>>> + * Pointer to newly allocated SPMI sub-device for success or negative ERR_PTR.
>>>> + */
>>>> +struct spmi_subdevice *spmi_subdevice_alloc_and_add(struct spmi_device *sparent)
>>>> +{
>>>> +	struct spmi_subdevice *sub_sdev;
>>>> +	struct spmi_device *sdev;
>>>> +	int ret;
>>>> +
>>>> +	sub_sdev = kzalloc(sizeof(*sub_sdev), GFP_KERNEL);
>>>> +	if (!sub_sdev)
>>>> +		return ERR_PTR(-ENOMEM);
>>>> +
>>>> +	ret = ida_alloc(&spmi_subdevice_ida, GFP_KERNEL);
>>>> +	if (ret < 0) {
>>>> +		kfree(sub_sdev);
>>>> +		return ERR_PTR(ret);
>>>> +	}
>>>> +
>>>> +	sdev = &sub_sdev->sdev;
>>>> +	sdev->ctrl = sparent->ctrl;
>>>> +	device_initialize(&sdev->dev);
>>>> +	sdev->dev.parent = &sparent->dev;
>>>> +	sdev->dev.bus = &spmi_bus_type;
>>>> +	sdev->dev.type = &spmi_subdev_type;
>>>> +
>>>> +	sub_sdev->devid = ret;
>>>> +	sdev->usid = sparent->usid;
>>>> +
>>>> +	ret = dev_set_name(&sdev->dev, "%d-%02x.%d.auto",
>>>> +			   sdev->ctrl->nr, sdev->usid, sub_sdev->devid);
>>>
>>> If I understand correctly sub_sdev->devid is globally unique. I wonder
>>> if a namespace that is specific to the parent spmi device would be more
>>> sensible?!
>>
>> Only in the context of the children of sdev. I'm not sure of what you're proposing
>> here, looks like it would complicate the code for no big reason - unless I am
>> misunderstanding something here.
> 
> The thing that I wondered about is: Why use sdev->usid if
> sub_sdev->devid is already a unique description of the subdevice? And
> for other device types (platform devices, mfd) the device identifiers
> are not globally unique. So I just wondered why spmi is different here.
> 

That gives a clear representation of the tree of devices on a SPMI bus, more
or less like it's done for some other addressable (or discoverable) busses
where you may have a tree like controller->hub->device (just as a fast example eh).

The SPMI devices are anyway already following this naming even before my changes,
as those are simply "%d-%02x" (ctrlid-devaddr), so what I wrote here is aimed to
  1. Not reinvent the wheel
  2. Be consistent with previous naming
  3. Be nice to whoever is trying to understand "where" a device is
     3a. ...And make it immediately easy to see that
     3b. ...And make it easier to debug, in case it's needed
  4. Not exclude a possible future where SPMI may become discoverable somehow
     (...which is questionably kind of a thing already, but then for multiple
      reasons it's not really feasible right now)

...I would be able to go on with more reasons, but let's not open this loophole :-)

>>>> +	if (ret)
>>>> +		goto err_put_dev;
>>>> +
>>>> +	ret = device_add(&sdev->dev);
>>>> +	if (ret) {
>>>> +		dev_err(&sdev->dev, "Can't add %s, status %d\n",
>>>
>>> I'd use %pe instead of %d here.
>>>
>>
>> The only reason why I am using %d is for consistency with the rest of the code that
>> is in SPMI - there is another device_add() call in spmi_device_add() which prints
>> the same error in the very same way as I'm doing here.
>>
>> I agree that using %pe makes error prints more readable, but perhaps that should be
>> done as a later cleanup to keep prints consistent (and perhaps that should not be
>> done only in SPMI anyway).
>>
>> If you have really strong opinions about doing that right now I can do it, but I
>> anyway prefer seeing that as a later commit doing that in the entire SPMI codebase.
> 
> My approach would be to first convert the driver to use %pe and then
> add the new code. But I don't feel strong.
> 

That'd be right, but %pe being better is (imo..) just an opinion and, while I agree
with you in that it's nicer, it'd be great if this doesn't become a blocker and if
we could get this patch picked in the current merge window.

This is because otherwise the other patches (of which, two are not yet ready as
they need some Kconfig fixes) would need immutable branches between multiple
trees (as those are touching nvmem, power, phy, misc and iio), and that would
get a bit complicated.

Of course while resending the other patches, I can add a %pe conversion for the
whole SPMI framework code. One more line isn't the end of the world anyway.

Mind you - the main reason for this patch is that I am using it for new drivers
for MediaTek PMICs (and a SPMI v2 bus implementation for the new MTK SPMI Arbiter)
that I will introduce after this gets picked.

Cheers,
Angelo

Powered by blists - more mailing lists