diff mbox

[RFC,1/2] ACPI / PNP: Don't add "enumeration_by_parent" devices

Message ID 1524218846-169934-2-git-send-email-john.garry@huawei.com (mailing list archive)
State RFC, archived
Headers show

Commit Message

John Garry April 20, 2018, 10:07 a.m. UTC
For ACPI devices with the enumeration_by_parent flag set,
we expect the parent device to enumerate the device after
the ACPI scan.

This patch does partially the same for devices which are
enumerated as PNP devices.

We still want PNP scan code to create the per-ACPI device
PNP device, but hold off adding the device to allow the
parent to do this optionally.

Flag acpi_device.driver_data is used as temp store as a
reference to the PNP device for the parent.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/pnp/pnpacpi/core.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Mika Westerberg April 20, 2018, 1:07 p.m. UTC | #1
On Fri, Apr 20, 2018 at 06:07:25PM +0800, John Garry wrote:
> +	} else {
> +		device->driver_data = dev;

I think this deserves a comment explaining why we (ab)use driver_data
like this.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
John Garry April 20, 2018, 1:24 p.m. UTC | #2
Hi Mika,

On 20/04/2018 14:07, Mika Westerberg wrote:
> On Fri, Apr 20, 2018 at 06:07:25PM +0800, John Garry wrote:
>> +	} else {
>> +		device->driver_data = dev;
>
> I think this deserves a comment explaining why we (ab)use driver_data
> like this.

Sure, could add. I didn't see any other way for the acpi_device 
structure to reference the derived PNP device.

TBH, This overall approach is not good since we are creating the PNP 
device in the scan, and then leaving the device in limbo, waiting for 
the parent to add it, if at all. There's no rule for this.

So I'm looking for ideas on how to improve this.

Thanks,
John

>
> .
>


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mika Westerberg April 20, 2018, 1:52 p.m. UTC | #3
On Fri, Apr 20, 2018 at 02:24:18PM +0100, John Garry wrote:
> Hi Mika,
> 
> On 20/04/2018 14:07, Mika Westerberg wrote:
> > On Fri, Apr 20, 2018 at 06:07:25PM +0800, John Garry wrote:
> > > +	} else {
> > > +		device->driver_data = dev;
> > 
> > I think this deserves a comment explaining why we (ab)use driver_data
> > like this.
> 
> Sure, could add. I didn't see any other way for the acpi_device structure to
> reference the derived PNP device.
> 
> TBH, This overall approach is not good since we are creating the PNP device
> in the scan, and then leaving the device in limbo, waiting for the parent to
> add it, if at all. There's no rule for this.
> 
> So I'm looking for ideas on how to improve this.

One idea is to make pnpacpi_add_device() available outside of PNP and
call it directly (or some variation) in hisi_lpc.c when it walks over
its children.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
John Garry April 20, 2018, 2:09 p.m. UTC | #4
On 20/04/2018 14:52, Mika Westerberg wrote:
> On Fri, Apr 20, 2018 at 02:24:18PM +0100, John Garry wrote:
>> Hi Mika,
>>
>> On 20/04/2018 14:07, Mika Westerberg wrote:
>>> On Fri, Apr 20, 2018 at 06:07:25PM +0800, John Garry wrote:
>>>> +	} else {
>>>> +		device->driver_data = dev;
>>>
>>> I think this deserves a comment explaining why we (ab)use driver_data
>>> like this.
>>
>> Sure, could add. I didn't see any other way for the acpi_device structure to
>> reference the derived PNP device.
>>
>> TBH, This overall approach is not good since we are creating the PNP device
>> in the scan, and then leaving the device in limbo, waiting for the parent to
>> add it, if at all. There's no rule for this.
>>
>> So I'm looking for ideas on how to improve this.
>

Hi Mika,

> One idea is to make pnpacpi_add_device() available outside of PNP and
> call it directly (or some variation) in hisi_lpc.c when it walks over
> its children.
>

I did consider this initially and it seems quite straightforward.

However I think the problem is that we would need to modify the 
acpi_device child resources from FW with kernel-specific resources, 
which does not seem right (I think that is what you meant). As I see, 
this is one reason that we went in the direction of modelling the host 
as an MFD, as we could set the resources of the mfd_cells.

So adding a variant of pnpacpi_add_device() could work, or modifying 
pnpacpi_add_device() to accept a callback to translate the resources. 
But this feature is specific to our very special requirement...

Thanks,
John

> .
>


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index 3a4c1aa..92f9d6f 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -285,10 +285,14 @@  static int __init pnpacpi_add_device(struct acpi_device *device)
 	if (!dev->active)
 		pnp_init_resources(dev);
 
-	error = pnp_add_device(dev);
-	if (error) {
-		put_device(&dev->dev);
-		return error;
+	if (!device->flags.enumeration_by_parent) {
+		error = pnp_add_device(dev);
+		if (error) {
+			put_device(&dev->dev);
+			return error;
+		}
+	} else {
+		device->driver_data = dev;
 	}
 
 	num++;