[PATCH] mailbox: add ACPI support for mailbox framework

[PATCH] mailbox: add ACPI support for mailbox framework

[Feng Kan]

This will add support for ACPI parsing of the mboxes attribute
when booting with ACPI table. The client will have a attribute
mimic the dts call "mboxes". In the ACPI case, the client will
mark "mboxes" with the ACPI HID of the mbox it wishes to use.

	Name (_DSD, Package () {
		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
		Package () {
			Package (2) {"mboxes", "ACPIHID"},
		}
	})

Signed-off-by: Feng Kan <fkan@apm.com>
---
 drivers/mailbox/mailbox.c          | 121 +++++++++++++++++++++++++++++--------
 include/linux/mailbox_controller.h |   2 +
 2 files changed, 99 insertions(+), 24 deletions(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 19b491d..9f2d0e3 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -9,6 +9,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/acpi.h>
 #include <linux/interrupt.h>
 #include <linux/spinlock.h>
 #include <linux/mutex.h>
@@ -278,6 +279,87 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
 }
 EXPORT_SYMBOL_GPL(mbox_send_message);
 
+static acpi_status
+mbox_acpi_find(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+	struct acpi_device *acpi_dev;
+
+	if (acpi_bus_get_device(handle, &acpi_dev))
+		return AE_OK;
+
+	if (strcmp(acpi_device_hid(acpi_dev), (const char *)context) == 0)
+		*rv = acpi_dev;
+
+	return AE_OK;
+}
+
+static struct mbox_chan *mbox_acpi_parse_chan(struct device *dev, int index)
+{
+	const char *mbox_attr = "mboxes";
+	struct acpi_device *acpi_dev;
+	struct mbox_controller *mbox;
+	struct mbox_chan *chan;
+	int status;
+	char *hid_str;
+
+	status = device_property_read_string(dev, mbox_attr,
+					     (const char **)&hid_str);
+	if (status)
+		return ERR_PTR(-ENODEV);
+
+	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+				     ACPI_UINT32_MAX, mbox_acpi_find, NULL,
+				     (void *)hid_str, (void **)&acpi_dev);
+	if (ACPI_FAILURE(status) || !acpi_dev) {
+		dev_dbg(dev, "mbox: no matching mboxes %s found in ACPI table\n",
+			hid_str);
+		return ERR_PTR(-ENODEV);
+	}
+
+	chan = NULL;
+	list_for_each_entry(mbox, &mbox_cons, node)
+		if (ACPI_COMPANION(mbox->dev) == acpi_dev) {
+			chan = mbox->acpi_xlate(mbox, index);
+			break;
+		}
+
+	return chan;
+}
+
+static struct mbox_chan *mbox_of_parse_chan(struct device *dev, int index)
+{
+	struct of_phandle_args spec;
+	struct mbox_controller *mbox;
+	struct mbox_chan *chan;
+
+	if (of_parse_phandle_with_args(dev->of_node, "mboxes",
+				       "#mbox-cells", index, &spec)) {
+		dev_dbg(dev, "%s: can't parse \"mboxes\" property\n", __func__);
+		return ERR_PTR(-ENODEV);
+	}
+
+	chan = NULL;
+	list_for_each_entry(mbox, &mbox_cons, node)
+		if (mbox->dev->of_node == spec.np) {
+			chan = mbox->of_xlate(mbox, &spec);
+			break;
+		}
+
+	of_node_put(spec.np);
+	return chan;
+}
+
+static struct mbox_chan *mbox_parse_chan(struct device *dev, int index)
+{
+	if (!dev)
+		return ERR_PTR(-ENODEV);
+
+	if (dev->of_node)
+		return mbox_of_parse_chan(dev, index);
+	else
+		return mbox_acpi_parse_chan(dev, index);
+}
+
 /**
  * mbox_request_channel - Request a mailbox channel.
  * @cl: Identity of the client requesting the channel.
@@ -298,36 +380,15 @@ EXPORT_SYMBOL_GPL(mbox_send_message);
 struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
 {
 	struct device *dev = cl->dev;
-	struct mbox_controller *mbox;
-	struct of_phandle_args spec;
 	struct mbox_chan *chan;
 	unsigned long flags;
 	int ret;
 
-	if (!dev || !dev->of_node) {
-		pr_debug("%s: No owner device node\n", __func__);
-		return ERR_PTR(-ENODEV);
-	}
-
 	mutex_lock(&con_mutex);
+	chan = mbox_parse_chan(dev, index);
 
-	if (of_parse_phandle_with_args(dev->of_node, "mboxes",
-				       "#mbox-cells", index, &spec)) {
-		dev_dbg(dev, "%s: can't parse \"mboxes\" property\n", __func__);
-		mutex_unlock(&con_mutex);
-		return ERR_PTR(-ENODEV);
-	}
-
-	chan = NULL;
-	list_for_each_entry(mbox, &mbox_cons, node)
-		if (mbox->dev->of_node == spec.np) {
-			chan = mbox->of_xlate(mbox, &spec);
-			break;
-		}
-
-	of_node_put(spec.np);
-
-	if (!chan || chan->cl || !try_module_get(mbox->dev->driver->owner)) {
+	if (!chan || chan->cl ||
+	    !try_module_get(chan->mbox->dev->driver->owner)) {
 		dev_dbg(dev, "%s: mailbox not free\n", __func__);
 		mutex_unlock(&con_mutex);
 		return ERR_PTR(-EBUSY);
@@ -384,6 +445,15 @@ void mbox_free_channel(struct mbox_chan *chan)
 EXPORT_SYMBOL_GPL(mbox_free_channel);
 
 static struct mbox_chan *
+acpi_mbox_index_xlate(struct mbox_controller *mbox, int chan)
+{
+	if (chan >= mbox->num_chans)
+		return NULL;
+
+	return &mbox->chans[chan];
+}
+
+static struct mbox_chan *
 of_mbox_index_xlate(struct mbox_controller *mbox,
 		    const struct of_phandle_args *sp)
 {
@@ -434,6 +504,9 @@ int mbox_controller_register(struct mbox_controller *mbox)
 	if (!mbox->of_xlate)
 		mbox->of_xlate = of_mbox_index_xlate;
 
+	if (!mbox->acpi_xlate)
+		mbox->acpi_xlate = acpi_mbox_index_xlate;
+
 	mutex_lock(&con_mutex);
 	list_add_tail(&mbox->node, &mbox_cons);
 	mutex_unlock(&con_mutex);
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
index d4cf96f..f61d9d4 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -80,6 +80,8 @@ struct mbox_controller {
 	unsigned txpoll_period;
 	struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox,
 				      const struct of_phandle_args *sp);
+	struct mbox_chan *(*acpi_xlate)(struct mbox_controller *mbox,
+					int chan);
 	/* Internal to API */
 	struct timer_list poll;
 	struct list_head node;
-- 
1.9.1

Re: [PATCH] mailbox: add ACPI support for mailbox framework

[Mika Westerberg]

On Tue, Mar 31, 2015 at 02:18:00PM -0700, Feng Kan wrote:
> This will add support for ACPI parsing of the mboxes attribute
> when booting with ACPI table. The client will have a attribute
> mimic the dts call "mboxes". In the ACPI case, the client will
> mark "mboxes" with the ACPI HID of the mbox it wishes to use.
> 
> 	Name (_DSD, Package () {
> 		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> 		Package () {
> 			Package (2) {"mboxes", "ACPIHID"},
> 		}
> 	})

Instead of this, why not match against the DT compatible property?

	Name (_HID, "PRP0001")

 	Name (_DSD, Package () {
 		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
 		Package () {
 			Package (2) {"compatible", "your-dt-compatible-string"},
 		}
 	})

This all will be done for you automatically with zero changes in the
code.

Re: [PATCH] mailbox: add ACPI support for mailbox framework

[Feng Kan]

On Wed, Apr 1, 2015 at 12:45 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Mar 31, 2015 at 02:18:00PM -0700, Feng Kan wrote:
>> This will add support for ACPI parsing of the mboxes attribute
>> when booting with ACPI table. The client will have a attribute
>> mimic the dts call "mboxes". In the ACPI case, the client will
>> mark "mboxes" with the ACPI HID of the mbox it wishes to use.
>>
>>       Name (_DSD, Package () {
>>               ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>               Package () {
>>                       Package (2) {"mboxes", "ACPIHID"},
>>               }
>>       })
>
> Instead of this, why not match against the DT compatible property?
>
>         Name (_HID, "PRP0001")
>
>         Name (_DSD, Package () {
>                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                 Package () {
>                         Package (2) {"compatible", "your-dt-compatible-string"},
>                 }
>         })
I think my description was not clear enough. The _DSD section is not
meant to identify the mailbox driver, but used by the acpi node that will
request the mailbox channel. The dts version would be as below.

   mailbox: {
   }

   user-mbox: {
      mboxes: <&mailbox 0>
   }

The mboxes attribute in the user of the mbox has to specify the HID of the
mbox in order to retrieve channel pointer.

>
> This all will be done for you automatically with zero changes in the
> code.

Re: [PATCH] mailbox: add ACPI support for mailbox framework

[Mika Westerberg]

On Wed, Apr 01, 2015 at 10:01:45AM -0700, Feng Kan wrote:
> On Wed, Apr 1, 2015 at 12:45 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Tue, Mar 31, 2015 at 02:18:00PM -0700, Feng Kan wrote:
> >> This will add support for ACPI parsing of the mboxes attribute
> >> when booting with ACPI table. The client will have a attribute
> >> mimic the dts call "mboxes". In the ACPI case, the client will
> >> mark "mboxes" with the ACPI HID of the mbox it wishes to use.
> >>
> >>       Name (_DSD, Package () {
> >>               ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >>               Package () {
> >>                       Package (2) {"mboxes", "ACPIHID"},
> >>               }
> >>       })
> >
> > Instead of this, why not match against the DT compatible property?
> >
> >         Name (_HID, "PRP0001")
> >
> >         Name (_DSD, Package () {
> >                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >                 Package () {
> >                         Package (2) {"compatible", "your-dt-compatible-string"},
> >                 }
> >         })
> I think my description was not clear enough. The _DSD section is not
> meant to identify the mailbox driver, but used by the acpi node that will
> request the mailbox channel. The dts version would be as below.
> 
>    mailbox: {
>    }
> 
>    user-mbox: {
>       mboxes: <&mailbox 0>
>    }
> 
> The mboxes attribute in the user of the mbox has to specify the HID of the
> mbox in order to retrieve channel pointer.

Okay, then I think you can use reference instead of _HID, like

	// The mailbox device
	Device (MLBX) {
	}

	// User-mbox device
	Device (USBX) {
		Name (_DSD, Package () {
			ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
			Package () {
				Package () {"mboxes", Package () {^^MLBX, 0}}),
			}
		})
	}

Re: [PATCH] mailbox: add ACPI support for mailbox framework

[Feng Kan]

On Thu, Apr 2, 2015 at 2:07 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, Apr 01, 2015 at 10:01:45AM -0700, Feng Kan wrote:
>> On Wed, Apr 1, 2015 at 12:45 AM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > On Tue, Mar 31, 2015 at 02:18:00PM -0700, Feng Kan wrote:
>> >> This will add support for ACPI parsing of the mboxes attribute
>> >> when booting with ACPI table. The client will have a attribute
>> >> mimic the dts call "mboxes". In the ACPI case, the client will
>> >> mark "mboxes" with the ACPI HID of the mbox it wishes to use.
>> >>
>> >>       Name (_DSD, Package () {
>> >>               ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> >>               Package () {
>> >>                       Package (2) {"mboxes", "ACPIHID"},
>> >>               }
>> >>       })
>> >
>> > Instead of this, why not match against the DT compatible property?
>> >
>> >         Name (_HID, "PRP0001")
>> >
>> >         Name (_DSD, Package () {
>> >                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> >                 Package () {
>> >                         Package (2) {"compatible", "your-dt-compatible-string"},
>> >                 }
>> >         })
>> I think my description was not clear enough. The _DSD section is not
>> meant to identify the mailbox driver, but used by the acpi node that will
>> request the mailbox channel. The dts version would be as below.
>>
>>    mailbox: {
>>    }
>>
>>    user-mbox: {
>>       mboxes: <&mailbox 0>
>>    }
>>
>> The mboxes attribute in the user of the mbox has to specify the HID of the
>> mbox in order to retrieve channel pointer.
>
> Okay, then I think you can use reference instead of _HID, like
>
>         // The mailbox device
>         Device (MLBX) {
>         }
>
>         // User-mbox device
>         Device (USBX) {
>                 Name (_DSD, Package () {
>                         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>                         Package () {
>                                 Package () {"mboxes", Package () {^^MLBX, 0}}),
>                         }
>                 })
>         }

Thanks, will try this. A side question on your previous reply. Would you
prefer a new driver using the PRP0001 or an actual proper HID.

Re: [PATCH] mailbox: add ACPI support for mailbox framework

[Mika Westerberg]

On Thu, Apr 02, 2015 at 11:04:24AM -0700, Feng Kan wrote:
> On Thu, Apr 2, 2015 at 2:07 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Wed, Apr 01, 2015 at 10:01:45AM -0700, Feng Kan wrote:
> >> On Wed, Apr 1, 2015 at 12:45 AM, Mika Westerberg
> >> <mika.westerberg@linux.intel.com> wrote:
> >> > On Tue, Mar 31, 2015 at 02:18:00PM -0700, Feng Kan wrote:
> >> >> This will add support for ACPI parsing of the mboxes attribute
> >> >> when booting with ACPI table. The client will have a attribute
> >> >> mimic the dts call "mboxes". In the ACPI case, the client will
> >> >> mark "mboxes" with the ACPI HID of the mbox it wishes to use.
> >> >>
> >> >>       Name (_DSD, Package () {
> >> >>               ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >> >>               Package () {
> >> >>                       Package (2) {"mboxes", "ACPIHID"},
> >> >>               }
> >> >>       })
> >> >
> >> > Instead of this, why not match against the DT compatible property?
> >> >
> >> >         Name (_HID, "PRP0001")
> >> >
> >> >         Name (_DSD, Package () {
> >> >                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >> >                 Package () {
> >> >                         Package (2) {"compatible", "your-dt-compatible-string"},
> >> >                 }
> >> >         })
> >> I think my description was not clear enough. The _DSD section is not
> >> meant to identify the mailbox driver, but used by the acpi node that will
> >> request the mailbox channel. The dts version would be as below.
> >>
> >>    mailbox: {
> >>    }
> >>
> >>    user-mbox: {
> >>       mboxes: <&mailbox 0>
> >>    }
> >>
> >> The mboxes attribute in the user of the mbox has to specify the HID of the
> >> mbox in order to retrieve channel pointer.
> >
> > Okay, then I think you can use reference instead of _HID, like
> >
> >         // The mailbox device
> >         Device (MLBX) {
> >         }
> >
> >         // User-mbox device
> >         Device (USBX) {
> >                 Name (_DSD, Package () {
> >                         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >                         Package () {
> >                                 Package () {"mboxes", Package () {^^MLBX, 0}}),
> >                         }
> >                 })
> >         }
> 
> Thanks, will try this. A side question on your previous reply. Would you
> prefer a new driver using the PRP0001 or an actual proper HID.

If you have existing DT bindings for this, then PRP0001 is fine.
Otherwise you should use the proper _HID for this particular device.

Re: [PATCH] mailbox: add ACPI support for mailbox framework

[Rafael J. Wysocki]

On Tuesday, April 07, 2015 11:41:43 AM Mika Westerberg wrote:
> On Thu, Apr 02, 2015 at 11:04:24AM -0700, Feng Kan wrote:
> > On Thu, Apr 2, 2015 at 2:07 AM, Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > > On Wed, Apr 01, 2015 at 10:01:45AM -0700, Feng Kan wrote:
> > >> On Wed, Apr 1, 2015 at 12:45 AM, Mika Westerberg
> > >> <mika.westerberg@linux.intel.com> wrote:
> > >> > On Tue, Mar 31, 2015 at 02:18:00PM -0700, Feng Kan wrote:
> > >> >> This will add support for ACPI parsing of the mboxes attribute
> > >> >> when booting with ACPI table. The client will have a attribute
> > >> >> mimic the dts call "mboxes". In the ACPI case, the client will
> > >> >> mark "mboxes" with the ACPI HID of the mbox it wishes to use.
> > >> >>
> > >> >>       Name (_DSD, Package () {
> > >> >>               ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > >> >>               Package () {
> > >> >>                       Package (2) {"mboxes", "ACPIHID"},
> > >> >>               }
> > >> >>       })
> > >> >
> > >> > Instead of this, why not match against the DT compatible property?
> > >> >
> > >> >         Name (_HID, "PRP0001")
> > >> >
> > >> >         Name (_DSD, Package () {
> > >> >                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > >> >                 Package () {
> > >> >                         Package (2) {"compatible", "your-dt-compatible-string"},
> > >> >                 }
> > >> >         })
> > >> I think my description was not clear enough. The _DSD section is not
> > >> meant to identify the mailbox driver, but used by the acpi node that will
> > >> request the mailbox channel. The dts version would be as below.
> > >>
> > >>    mailbox: {
> > >>    }
> > >>
> > >>    user-mbox: {
> > >>       mboxes: <&mailbox 0>
> > >>    }
> > >>
> > >> The mboxes attribute in the user of the mbox has to specify the HID of the
> > >> mbox in order to retrieve channel pointer.
> > >
> > > Okay, then I think you can use reference instead of _HID, like
> > >
> > >         // The mailbox device
> > >         Device (MLBX) {
> > >         }
> > >
> > >         // User-mbox device
> > >         Device (USBX) {
> > >                 Name (_DSD, Package () {
> > >                         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > >                         Package () {
> > >                                 Package () {"mboxes", Package () {^^MLBX, 0}}),
> > >                         }
> > >                 })
> > >         }
> > 
> > Thanks, will try this. A side question on your previous reply. Would you
> > prefer a new driver using the PRP0001 or an actual proper HID.
> 
> If you have existing DT bindings for this, then PRP0001 is fine.
> Otherwise you should use the proper _HID for this particular device.

To be precise, PRP0001 specifically means "Use the 'compatible' property
to find the driver for this device", so if *that* is what you want to do,
you can use PRP0001 as the _HID.  For Windows (and such) compatibility, you
can provide a _CID whith a (list of) proper device ID(s) in addition to that.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] mailbox: add ACPI support for mailbox framework

[Mika Westerberg]

On Tue, Apr 07, 2015 at 01:37:56PM +0200, Rafael J. Wysocki wrote:
> To be precise, PRP0001 specifically means "Use the 'compatible' property
> to find the driver for this device", so if *that* is what you want to do,
> you can use PRP0001 as the _HID.  For Windows (and such) compatibility, you
> can provide a _CID whith a (list of) proper device ID(s) in addition to that.

Thanks for clarification Rafael :-)

Re: [PATCH] mailbox: add ACPI support for mailbox framework

[Feng Kan]

On Tue, Apr 7, 2015 at 4:37 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, April 07, 2015 11:41:43 AM Mika Westerberg wrote:
>> On Thu, Apr 02, 2015 at 11:04:24AM -0700, Feng Kan wrote:
>> > On Thu, Apr 2, 2015 at 2:07 AM, Mika Westerberg
>> > <mika.westerberg@linux.intel.com> wrote:
>> > > On Wed, Apr 01, 2015 at 10:01:45AM -0700, Feng Kan wrote:
>> > >> On Wed, Apr 1, 2015 at 12:45 AM, Mika Westerberg
>> > >> <mika.westerberg@linux.intel.com> wrote:
>> > >> > On Tue, Mar 31, 2015 at 02:18:00PM -0700, Feng Kan wrote:
>> > >> >> This will add support for ACPI parsing of the mboxes attribute
>> > >> >> when booting with ACPI table. The client will have a attribute
>> > >> >> mimic the dts call "mboxes". In the ACPI case, the client will
>> > >> >> mark "mboxes" with the ACPI HID of the mbox it wishes to use.
>> > >> >>
>> > >> >>       Name (_DSD, Package () {
>> > >> >>               ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> > >> >>               Package () {
>> > >> >>                       Package (2) {"mboxes", "ACPIHID"},
>> > >> >>               }
>> > >> >>       })
>> > >> >
>> > >> > Instead of this, why not match against the DT compatible property?
>> > >> >
>> > >> >         Name (_HID, "PRP0001")
>> > >> >
>> > >> >         Name (_DSD, Package () {
>> > >> >                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> > >> >                 Package () {
>> > >> >                         Package (2) {"compatible", "your-dt-compatible-string"},
>> > >> >                 }
>> > >> >         })
>> > >> I think my description was not clear enough. The _DSD section is not
>> > >> meant to identify the mailbox driver, but used by the acpi node that will
>> > >> request the mailbox channel. The dts version would be as below.
>> > >>
>> > >>    mailbox: {
>> > >>    }
>> > >>
>> > >>    user-mbox: {
>> > >>       mboxes: <&mailbox 0>
>> > >>    }
>> > >>
>> > >> The mboxes attribute in the user of the mbox has to specify the HID of the
>> > >> mbox in order to retrieve channel pointer.
>> > >
>> > > Okay, then I think you can use reference instead of _HID, like
>> > >
>> > >         // The mailbox device
>> > >         Device (MLBX) {
>> > >         }
>> > >
>> > >         // User-mbox device
>> > >         Device (USBX) {
>> > >                 Name (_DSD, Package () {
>> > >                         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> > >                         Package () {
>> > >                                 Package () {"mboxes", Package () {^^MLBX, 0}}),
>> > >                         }
>> > >                 })
>> > >         }
>> >
>> > Thanks, will try this. A side question on your previous reply. Would you
>> > prefer a new driver using the PRP0001 or an actual proper HID.
>>
>> If you have existing DT bindings for this, then PRP0001 is fine.
>> Otherwise you should use the proper _HID for this particular device.
>
> To be precise, PRP0001 specifically means "Use the 'compatible' property
> to find the driver for this device", so if *that* is what you want to do,
I am a bit uneasy about this. I understand the application of this. For a system
that is both DT and ACPI compatible, this would open up a flood of PRP0001
device drivers. What is the guideline here? Is this PRP0001 only exist
to support legacy devices that do not have a proper HID.

> you can use PRP0001 as the _HID.  For Windows (and such) compatibility, you
> can provide a _CID whith a (list of) proper device ID(s) in addition to that.
>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] mailbox: add ACPI support for mailbox framework

[Rafael J. Wysocki]

On Tuesday, April 07, 2015 03:58:36 PM Feng Kan wrote:
> On Tue, Apr 7, 2015 at 4:37 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Tuesday, April 07, 2015 11:41:43 AM Mika Westerberg wrote:
> >> On Thu, Apr 02, 2015 at 11:04:24AM -0700, Feng Kan wrote:
> >> > On Thu, Apr 2, 2015 at 2:07 AM, Mika Westerberg
> >> > <mika.westerberg@linux.intel.com> wrote:
> >> > > On Wed, Apr 01, 2015 at 10:01:45AM -0700, Feng Kan wrote:
> >> > >> On Wed, Apr 1, 2015 at 12:45 AM, Mika Westerberg
> >> > >> <mika.westerberg@linux.intel.com> wrote:
> >> > >> > On Tue, Mar 31, 2015 at 02:18:00PM -0700, Feng Kan wrote:
> >> > >> >> This will add support for ACPI parsing of the mboxes attribute
> >> > >> >> when booting with ACPI table. The client will have a attribute
> >> > >> >> mimic the dts call "mboxes". In the ACPI case, the client will
> >> > >> >> mark "mboxes" with the ACPI HID of the mbox it wishes to use.
> >> > >> >>
> >> > >> >>       Name (_DSD, Package () {
> >> > >> >>               ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >> > >> >>               Package () {
> >> > >> >>                       Package (2) {"mboxes", "ACPIHID"},
> >> > >> >>               }
> >> > >> >>       })
> >> > >> >
> >> > >> > Instead of this, why not match against the DT compatible property?
> >> > >> >
> >> > >> >         Name (_HID, "PRP0001")
> >> > >> >
> >> > >> >         Name (_DSD, Package () {
> >> > >> >                 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >> > >> >                 Package () {
> >> > >> >                         Package (2) {"compatible", "your-dt-compatible-string"},
> >> > >> >                 }
> >> > >> >         })
> >> > >> I think my description was not clear enough. The _DSD section is not
> >> > >> meant to identify the mailbox driver, but used by the acpi node that will
> >> > >> request the mailbox channel. The dts version would be as below.
> >> > >>
> >> > >>    mailbox: {
> >> > >>    }
> >> > >>
> >> > >>    user-mbox: {
> >> > >>       mboxes: <&mailbox 0>
> >> > >>    }
> >> > >>
> >> > >> The mboxes attribute in the user of the mbox has to specify the HID of the
> >> > >> mbox in order to retrieve channel pointer.
> >> > >
> >> > > Okay, then I think you can use reference instead of _HID, like
> >> > >
> >> > >         // The mailbox device
> >> > >         Device (MLBX) {
> >> > >         }
> >> > >
> >> > >         // User-mbox device
> >> > >         Device (USBX) {
> >> > >                 Name (_DSD, Package () {
> >> > >                         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >> > >                         Package () {
> >> > >                                 Package () {"mboxes", Package () {^^MLBX, 0}}),
> >> > >                         }
> >> > >                 })
> >> > >         }
> >> >
> >> > Thanks, will try this. A side question on your previous reply. Would you
> >> > prefer a new driver using the PRP0001 or an actual proper HID.
> >>
> >> If you have existing DT bindings for this, then PRP0001 is fine.
> >> Otherwise you should use the proper _HID for this particular device.
> >
> > To be precise, PRP0001 specifically means "Use the 'compatible' property
> > to find the driver for this device", so if *that* is what you want to do,
>
> I am a bit uneasy about this. I understand the application of this. For a system
> that is both DT and ACPI compatible, this would open up a flood of PRP0001
> device drivers. What is the guideline here? Is this PRP0001 only exist
> to support legacy devices that do not have a proper HID.
>

For Windows-compatible firmware it generally would be better to use PRP0001 in
a _CID and make the _HID return a proper device ID that could be matched by a
Windows driver.

Then, the PRP0001 makes it possible to match drivers using the existing DT
device identification without having to add the ACPI device IDs to those
drivers in order to enable them to work with newfangled ACPI tables.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.