diff mbox

[RFC,06/10] spi: add support for ACPI reconfigure notifications

Message ID 1459417026-6697-7-git-send-email-octavian.purdila@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Octavian Purdila March 31, 2016, 9:37 a.m. UTC
This allows the SPI core to enumerate devices from ACPI tables that
are dynamically loaded after the SPI master device has been probed.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/spi/spi.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

Comments

Mark Brown March 31, 2016, 5:29 p.m. UTC | #1
On Thu, Mar 31, 2016 at 12:37:02PM +0300, Octavian Purdila wrote:

> +#if IS_ENABLED(CONFIG_ACPI)
> +static int acpi_spi_table_load(struct device *dev, const void *data)
> +{
> +	struct spi_master *master = container_of(dev, struct spi_master, dev);
> +
> +	acpi_register_spi_devices(master);
> +	return 0;
> +}

Why do we have a separate code path for this coompared to the initial
startup?  The handling appears to be identical so it seems we should
drive this from the ACPI code so we don't have to add this to every
single bus with ACPI bindings.
Octavian Purdila April 1, 2016, 10:54 a.m. UTC | #2
On Thu, Mar 31, 2016 at 8:29 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Mar 31, 2016 at 12:37:02PM +0300, Octavian Purdila wrote:
>
>> +#if IS_ENABLED(CONFIG_ACPI)
>> +static int acpi_spi_table_load(struct device *dev, const void *data)
>> +{
>> +     struct spi_master *master = container_of(dev, struct spi_master, dev);
>> +
>> +     acpi_register_spi_devices(master);
>> +     return 0;
>> +}
>
> Why do we have a separate code path for this coompared to the initial
> startup?  The handling appears to be identical so it seems we should
> drive this from the ACPI code so we don't have to add this to every
> single bus with ACPI bindings.

Hi Mark,

I probably don't fully understand your question, but I don't see a way
of how we can create a new SPI device from generic ACPI code. For
example, in acpi_spi_add_device() we need the spi_master node so that
we can allocate the spi device.

The handling is identical because we don't have yet have a way to
identify what where the new nodes added when a new ACPI table /
overlay has been loaded, so we have to rescan the ACPI namespace under
each controller.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown April 1, 2016, 2:08 p.m. UTC | #3
On Fri, Apr 01, 2016 at 01:54:28PM +0300, Octavian Purdila wrote:

> I probably don't fully understand your question, but I don't see a way
> of how we can create a new SPI device from generic ACPI code. For
> example, in acpi_spi_add_device() we need the spi_master node so that
> we can allocate the spi device.

Right, but the same applies to initial enumeration so we also have to
manually instantiate ACPI devices on startup.  Why do we need to do
that?

> The handling is identical because we don't have yet have a way to
> identify what where the new nodes added when a new ACPI table /
> overlay has been loaded, so we have to rescan the ACPI namespace under
> each controller.

That's not the point.  The point is that since the handling is identical
why are we handling it through exactly the same code?
Rafael J. Wysocki April 1, 2016, 7:26 p.m. UTC | #4
On Fri, Apr 1, 2016 at 4:08 PM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Apr 01, 2016 at 01:54:28PM +0300, Octavian Purdila wrote:
>
>> I probably don't fully understand your question, but I don't see a way
>> of how we can create a new SPI device from generic ACPI code. For
>> example, in acpi_spi_add_device() we need the spi_master node so that
>> we can allocate the spi device.
>
> Right, but the same applies to initial enumeration so we also have to
> manually instantiate ACPI devices on startup.  Why do we need to do
> that?
>
>> The handling is identical because we don't have yet have a way to
>> identify what where the new nodes added when a new ACPI table /
>> overlay has been loaded, so we have to rescan the ACPI namespace under
>> each controller.
>
> That's not the point.  The point is that since the handling is identical
> why are we handling it through exactly the same code?

I think that during the initial enumeration the controller driver's
probe walks the children and creates device objects for them.  When a
table is loaded later, the controller driver has been probed already
and there needs to be a way to trigger a walk over the (new) children
from it.

Or a hook somewhere around acpi_platform_notify() is needed.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown April 2, 2016, 4:24 p.m. UTC | #5
On Fri, Apr 01, 2016 at 09:26:38PM +0200, Rafael J. Wysocki wrote:
> On Fri, Apr 1, 2016 at 4:08 PM, Mark Brown <broonie@kernel.org> wrote:

> > That's not the point.  The point is that since the handling is identical
> > why are we handling it through exactly the same code?

> I think that during the initial enumeration the controller driver's
> probe walks the children and creates device objects for them.  When a
> table is loaded later, the controller driver has been probed already
> and there needs to be a way to trigger a walk over the (new) children
> from it.

> Or a hook somewhere around acpi_platform_notify() is needed.

What I don't understand is why the flow on inital probe isn't simply to
register the controller which then triggers the walk of the children.
That way any bus that supports initial probe also supports hotplug
without needing to go and manually add a second code path.
Octavian Purdila April 4, 2016, 10:25 a.m. UTC | #6
On Sat, Apr 2, 2016 at 7:24 PM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Apr 01, 2016 at 09:26:38PM +0200, Rafael J. Wysocki wrote:
>> On Fri, Apr 1, 2016 at 4:08 PM, Mark Brown <broonie@kernel.org> wrote:
>
>> > That's not the point.  The point is that since the handling is identical
>> > why are we handling it through exactly the same code?
>
>> I think that during the initial enumeration the controller driver's
>> probe walks the children and creates device objects for them.  When a
>> table is loaded later, the controller driver has been probed already
>> and there needs to be a way to trigger a walk over the (new) children
>> from it.
>
>> Or a hook somewhere around acpi_platform_notify() is needed.
>
> What I don't understand is why the flow on inital probe isn't simply to
> register the controller which then triggers the walk of the children.
> That way any bus that supports initial probe also supports hotplug
> without needing to go and manually add a second code path.

Do you mean register the notifier per controller instead of per
subsystem? Either way we need changes at the subsystem level and I
choose to follow the device tree implementation for consistency.

The other reason is that (pending other ACPICA changes) we can add
other notification events in the future such as node added or removed
(just like device tree), and in that case the probe and hotplug
handling would be different (and a bit more efficient).
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown April 4, 2016, 4:03 p.m. UTC | #7
On Mon, Apr 04, 2016 at 01:25:56PM +0300, Octavian Purdila wrote:
> On Sat, Apr 2, 2016 at 7:24 PM, Mark Brown <broonie@kernel.org> wrote:

> > What I don't understand is why the flow on inital probe isn't simply to
> > register the controller which then triggers the walk of the children.
> > That way any bus that supports initial probe also supports hotplug
> > without needing to go and manually add a second code path.

> Do you mean register the notifier per controller instead of per
> subsystem? Either way we need changes at the subsystem level and I
> choose to follow the device tree implementation for consistency.

No!  I mean use the exact same callback you've got now for everything.

> The other reason is that (pending other ACPICA changes) we can add
> other notification events in the future such as node added or removed
> (just like device tree), and in that case the probe and hotplug
> handling would be different (and a bit more efficient).

Why is probe different to hotplug?  We don't need to do that in the
normal driver model.
Octavian Purdila April 4, 2016, 7:34 p.m. UTC | #8
On Mon, Apr 4, 2016 at 7:03 PM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Apr 04, 2016 at 01:25:56PM +0300, Octavian Purdila wrote:
>> On Sat, Apr 2, 2016 at 7:24 PM, Mark Brown <broonie@kernel.org> wrote:
>
>> > What I don't understand is why the flow on inital probe isn't simply to
>> > register the controller which then triggers the walk of the children.
>> > That way any bus that supports initial probe also supports hotplug
>> > without needing to go and manually add a second code path.
>
>> Do you mean register the notifier per controller instead of per
>> subsystem? Either way we need changes at the subsystem level and I
>> choose to follow the device tree implementation for consistency.
>
> No!  I mean use the exact same callback you've got now for everything.
>
>> The other reason is that (pending other ACPICA changes) we can add
>> other notification events in the future such as node added or removed
>> (just like device tree), and in that case the probe and hotplug
>> handling would be different (and a bit more efficient).
>
> Why is probe different to hotplug?  We don't need to do that in the
> normal driver model.

There might be some confusion with the term, I am referring to slave
hotplug, not controller hotplug.

The way I see it, there are two logical operations: probe of a
controller and the associated enumeration of the SPI slaves for that
bus and "hotplug" of new SPI slaves and the enumeration of those
particular slaves.

When we probe the controller we search DT/ACPI and enumerate all the
slaves for *that* controller.

When a slave hotplug happens for device tree we get a device node
notification and we can instantiate the SPI slave based on that info.
In case of ACPI, (at this point) we get a global callback and in that
callback we need to iterate through *all* controllers.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki April 4, 2016, 9:18 p.m. UTC | #9
On Mon, Apr 4, 2016 at 9:34 PM, Octavian Purdila
<octavian.purdila@intel.com> wrote:
> On Mon, Apr 4, 2016 at 7:03 PM, Mark Brown <broonie@kernel.org> wrote:
>> On Mon, Apr 04, 2016 at 01:25:56PM +0300, Octavian Purdila wrote:
>>> On Sat, Apr 2, 2016 at 7:24 PM, Mark Brown <broonie@kernel.org> wrote:
>>
>>> > What I don't understand is why the flow on inital probe isn't simply to
>>> > register the controller which then triggers the walk of the children.
>>> > That way any bus that supports initial probe also supports hotplug
>>> > without needing to go and manually add a second code path.
>>
>>> Do you mean register the notifier per controller instead of per
>>> subsystem? Either way we need changes at the subsystem level and I
>>> choose to follow the device tree implementation for consistency.
>>
>> No!  I mean use the exact same callback you've got now for everything.
>>
>>> The other reason is that (pending other ACPICA changes) we can add
>>> other notification events in the future such as node added or removed
>>> (just like device tree), and in that case the probe and hotplug
>>> handling would be different (and a bit more efficient).
>>
>> Why is probe different to hotplug?  We don't need to do that in the
>> normal driver model.
>
> There might be some confusion with the term, I am referring to slave
> hotplug, not controller hotplug.
>
> The way I see it, there are two logical operations: probe of a
> controller and the associated enumeration of the SPI slaves for that
> bus and "hotplug" of new SPI slaves and the enumeration of those
> particular slaves.
>
> When we probe the controller we search DT/ACPI and enumerate all the
> slaves for *that* controller.
>
> When a slave hotplug happens for device tree we get a device node
> notification and we can instantiate the SPI slave based on that info.
> In case of ACPI, (at this point) we get a global callback and in that
> callback we need to iterate through *all* controllers.

Is that really necessary?

The namespace rescan could notify the parent of a new node in
acpi_default_enumeration(), couldn't it?
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Octavian Purdila April 5, 2016, 11:49 a.m. UTC | #10
On Tue, Apr 5, 2016 at 12:18 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Mon, Apr 4, 2016 at 9:34 PM, Octavian Purdila
> <octavian.purdila@intel.com> wrote:
>> On Mon, Apr 4, 2016 at 7:03 PM, Mark Brown <broonie@kernel.org> wrote:
>>> On Mon, Apr 04, 2016 at 01:25:56PM +0300, Octavian Purdila wrote:
>>>> On Sat, Apr 2, 2016 at 7:24 PM, Mark Brown <broonie@kernel.org> wrote:
>>>
>>>> > What I don't understand is why the flow on inital probe isn't simply to
>>>> > register the controller which then triggers the walk of the children.
>>>> > That way any bus that supports initial probe also supports hotplug
>>>> > without needing to go and manually add a second code path.
>>>
>>>> Do you mean register the notifier per controller instead of per
>>>> subsystem? Either way we need changes at the subsystem level and I
>>>> choose to follow the device tree implementation for consistency.
>>>
>>> No!  I mean use the exact same callback you've got now for everything.
>>>
>>>> The other reason is that (pending other ACPICA changes) we can add
>>>> other notification events in the future such as node added or removed
>>>> (just like device tree), and in that case the probe and hotplug
>>>> handling would be different (and a bit more efficient).
>>>
>>> Why is probe different to hotplug?  We don't need to do that in the
>>> normal driver model.
>>
>> There might be some confusion with the term, I am referring to slave
>> hotplug, not controller hotplug.
>>
>> The way I see it, there are two logical operations: probe of a
>> controller and the associated enumeration of the SPI slaves for that
>> bus and "hotplug" of new SPI slaves and the enumeration of those
>> particular slaves.
>>
>> When we probe the controller we search DT/ACPI and enumerate all the
>> slaves for *that* controller.
>>
>> When a slave hotplug happens for device tree we get a device node
>> notification and we can instantiate the SPI slave based on that info.
>> In case of ACPI, (at this point) we get a global callback and in that
>> callback we need to iterate through *all* controllers.
>
> Is that really necessary?
>
> The namespace rescan could notify the parent of a new node in
> acpi_default_enumeration(), couldn't it?

We could do that, with some complexity, but I am not sure if it is
worth it - see below. And in either case I still don't see how we can
avoid two enumeration paths: one from the probe of the controller ->
spi_register_master() -> acpi_register_spi_devices()  and one from the
notification handler -> acpi_register_spi_devices(). Both will end up
are calling the same enumeration function but there are still two
paths.

Even right now, in the case of dynamic DT, we have the two enumeration paths.

If we really want to have a single path for ACPI enumeration we could
do that by using an ACPI SPI bridge driver or scan handlers after
extending the matching mechanisms. But we would still need to modify
the SPI subsystem and I don't think its worth it just to save a call
to acpi_register_spi_devices() from spi_register_master().

Now for parent notification complexity: in the case of SPI we can
easily to this because current ACPI SPI enumeration supports only
direct children as slaves. However, on I2C we can have an unrelated
node as a slave - that is why the I2C scanning searches the whole
namespaces for references to a particular i2c_adapter. So, we would
need to retrieve the parent node from the namespace node information
which means that we will do SPI specific stuff in ACPI generic code. I
don't think it is a big issue, because we already treat SPI / I2C
special, right?

Once we have the parent handle we would still have to iterate through
all controllers to get the spi_master matching the ACPI handle. It is
a bit more efficient then global notification, but probably won't
matter in practice.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown April 5, 2016, 6:24 p.m. UTC | #11
On Mon, Apr 04, 2016 at 10:34:56PM +0300, Octavian Purdila wrote:
> On Mon, Apr 4, 2016 at 7:03 PM, Mark Brown <broonie@kernel.org> wrote:

> > Why is probe different to hotplug?  We don't need to do that in the
> > normal driver model.

> There might be some confusion with the term, I am referring to slave
> hotplug, not controller hotplug.

That's what I was talking about too.

> The way I see it, there are two logical operations: probe of a
> controller and the associated enumeration of the SPI slaves for that
> bus and "hotplug" of new SPI slaves and the enumeration of those
> particular slaves.

I don't see a distinction here.  The firmware finds some new slaves to
tell the framework about.  Quite why it decided to go looking shouldn't
matter.

> When a slave hotplug happens for device tree we get a device node
> notification and we can instantiate the SPI slave based on that info.
> In case of ACPI, (at this point) we get a global callback and in that
> callback we need to iterate through *all* controllers.

That's not really helping me understand why you need every bus to open
code enumeration twice?
Mark Brown April 5, 2016, 6:32 p.m. UTC | #12
On Tue, Apr 05, 2016 at 02:49:13PM +0300, Octavian Purdila wrote:

> If we really want to have a single path for ACPI enumeration we could
> do that by using an ACPI SPI bridge driver or scan handlers after
> extending the matching mechanisms. But we would still need to modify
> the SPI subsystem and I don't think its worth it just to save a call
> to acpi_register_spi_devices() from spi_register_master().

It's not specifically for SPI, it's the fact that you're asking every
single bus type which might be described in ACPI to handle both hotplug
and coldplug paths separately.  Given that the code that's being added
just seems like trivial boilerplate it seems like we're doing this
wrong, we should be factoring this out so there's nothing bus types can
get wrong.

> Now for parent notification complexity: in the case of SPI we can
> easily to this because current ACPI SPI enumeration supports only
> direct children as slaves. However, on I2C we can have an unrelated
> node as a slave - that is why the I2C scanning searches the whole
> namespaces for references to a particular i2c_adapter. So, we would
> need to retrieve the parent node from the namespace node information
> which means that we will do SPI specific stuff in ACPI generic code. I
> don't think it is a big issue, because we already treat SPI / I2C
> special, right?

Or perhaps the issue is that we can't make our mind up if the bus
specific code should go in the bus or in the ACPI core?
Octavian Purdila April 5, 2016, 7:16 p.m. UTC | #13
On Tue, Apr 5, 2016 at 9:32 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Apr 05, 2016 at 02:49:13PM +0300, Octavian Purdila wrote:
>
>> If we really want to have a single path for ACPI enumeration we could
>> do that by using an ACPI SPI bridge driver or scan handlers after
>> extending the matching mechanisms. But we would still need to modify
>> the SPI subsystem and I don't think its worth it just to save a call
>> to acpi_register_spi_devices() from spi_register_master().
>
> It's not specifically for SPI, it's the fact that you're asking every
> single bus type which might be described in ACPI to handle both hotplug
> and coldplug paths separately.  Given that the code that's being added
> just seems like trivial boilerplate it seems like we're doing this
> wrong, we should be factoring this out so there's nothing bus types can
> get wrong.
>

AFAICS this is exactly the same case for DT: one code path for
coldplug and one for hotplug.

Which makes me think that it is not possible to have a single path for
both, or maybe its not worth it. Do I miss something obvious?
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown April 5, 2016, 9:20 p.m. UTC | #14
On Tue, Apr 05, 2016 at 10:16:16PM +0300, Octavian Purdila wrote:
> On Tue, Apr 5, 2016 at 9:32 PM, Mark Brown <broonie@kernel.org> wrote:

> > It's not specifically for SPI, it's the fact that you're asking every
> > single bus type which might be described in ACPI to handle both hotplug
> > and coldplug paths separately.  Given that the code that's being added
> > just seems like trivial boilerplate it seems like we're doing this
> > wrong, we should be factoring this out so there's nothing bus types can
> > get wrong.

> AFAICS this is exactly the same case for DT: one code path for
> coldplug and one for hotplug.

In the DT case the DT code understands a bit more of what's going on
with the firmware parsing which makes things better (the code in the two
paths is not identical, unlike ACPI) but yes, that's another aspect of
why I'm not thrilled with this - if it's not the hotplug and coldplug
that should be sharing code then perhaps it's along the firmware
interface axis that we need to be sharing things.

> Which makes me think that it is not possible to have a single path for
> both, or maybe its not worth it. Do I miss something obvious?

I think at a very high level what I'm looking at here is that we're
having to write too much boilerplate per firmware interface.  Perhaps
the hotplug vs coldplug path isn't it, it looked like it from your
patch, but there must be something here we're spending too much time on.
diff mbox

Patch

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 47eff80..8751f02 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1619,7 +1619,8 @@  static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level,
 
 	if (acpi_bus_get_device(handle, &adev))
 		return AE_OK;
-	if (acpi_bus_get_status(adev) || !adev->status.present)
+	if (acpi_bus_get_status(adev) || !adev->status.present ||
+	    acpi_device_enumerated(adev))
 		return AE_OK;
 
 	spi = spi_alloc_device(master);
@@ -1645,6 +1646,8 @@  static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level,
 	if (spi->irq < 0)
 		spi->irq = acpi_dev_gpio_irq_get(adev, 0);
 
+	adev->flags.visited = true;
+
 	adev->power.flags.ignore_parent = true;
 	strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias));
 	if (spi_add_device(spi)) {
@@ -2698,6 +2701,35 @@  static struct notifier_block spi_of_notifier = {
 extern struct notifier_block spi_of_notifier;
 #endif /* IS_ENABLED(CONFIG_OF_DYNAMIC) */
 
+#if IS_ENABLED(CONFIG_ACPI)
+static int acpi_spi_table_load(struct device *dev, const void *data)
+{
+	struct spi_master *master = container_of(dev, struct spi_master, dev);
+
+	acpi_register_spi_devices(master);
+	return 0;
+}
+
+static int acpi_spi_notify(struct notifier_block *nb, unsigned long value,
+			   void *arg)
+{
+	switch (value) {
+	case ACPI_RECONFIG_TABLE_LOAD:
+		class_find_device(&spi_master_class, NULL, NULL,
+				  acpi_spi_table_load);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block spi_acpi_notifier = {
+	.notifier_call = acpi_spi_notify,
+};
+#else
+extern struct notifier_block spi_acpi_notifier;
+#endif
+
 static int __init spi_init(void)
 {
 	int	status;
@@ -2718,6 +2750,8 @@  static int __init spi_init(void)
 
 	if (IS_ENABLED(CONFIG_OF_DYNAMIC))
 		WARN_ON(of_reconfig_notifier_register(&spi_of_notifier));
+	if (IS_ENABLED(CONFIG_ACPI))
+		WARN_ON(acpi_reconfig_notifier_register(&spi_acpi_notifier));
 
 	return 0;