Add ACPI support for serial attached devices.
Currently, serial devices are not set as enumerated during ACPI scan for SPI
or i2c buses (but not for UART). This should also be done for UART serial
devices.
I renamed *spi_i2c_slave* to *serial_bus_slave* to reflect this.
For hci_bcm, this needs Hans de Goede's "Bluetooth: hci_bcm: Add (runtime)pm
support to the serdev drv" patches to work.
Tested on T100TA with Broadcom BCM2E39.
Since RFC:
- Add or reword commit messages
- Rename *serial_slave* to *serial_bus_slave*
- Add specific check for Apple in acpi_is_serial_bus_slave(), thanks to
Lukas Wunner
- Update comment in acpi_default_enumeration()
- Remove patch 3 "Bluetooth: hci_bcm: Add ACPI serdev support for BCM2E39"
in favor of patches from Hans de Goede
Frédéric Danis (2):
serdev: Add ACPI support
ACPI / scan: Fix enumeration for special UART devices
drivers/acpi/scan.c | 37 ++++++++----------
drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
include/acpi/acpi_bus.h | 2 +-
3 files changed, 112 insertions(+), 26 deletions(-)
--
2.7.4
T24gMTEgT2N0b2JlciAyMDE3IGF0IDAzOjM2LCBKb2hhbiBIb3ZvbGQgPGpvaGFuQGtlcm5lbC5v
cmc+IHdyb3RlOg0KPiBPbiBUdWUsIE9jdCAxMCwgMjAxNyBhdCAxMDoyMjoxOUFNICswMjAwLCBN
YXJjZWwgSG9sdG1hbm4gd3JvdGU6DQo+Pg0KPj4gV2hhdCBJIHdhcyB3b25kZXJpbmcgdGhlIG90
aGVyIGRheSBpcyBpZiB3ZSBuZWVkIGEgbHNzZXJkZXYgdG9vbCBvcg0KPj4gc29tZSBpbnRlZ3Jh
dGlvbiBpbiBsc2h3IHRvIGJlIGFibGUgdG8gZGVidWcgd2hhdCBzZXJkZXYgZGV2aWNlcyBhbmQN
Cj4+IElEIGFyZSBwcmVzZW50LiBUaGUgbHN1c2IgYW5kIC9zeXMva2VybmVsL2RlYnVnL3VzYi9k
ZXZpY2VzIGlzIGp1c3QNCj4+IHN1cGVyIHBvd2VyZnVsIGFuZCBlYXN5IHdoZW4gaXQgY29tZXMg
dG8gZmlndXJpbmcgb3V0IHdoYXQgcGVvcGxlIGhhdmUNCj4+IGluIHRoZWlyIHN5c3RlbS4gTWF5
YmUgL3N5cy9rZXJuZWwvZGVidWcvc2VyZGV2L2RldmljZXMgY291bGQgYmUNCj4+IGhlbHBmdWwg
YXMgd2VsbC4gSnVzdCB0aGlua2luZyBvdXQgbG91ZCBoZXJlLg0KPg0KPiBZZWFoLCBtYXliZS4g
U2luY2UgeW91J2QgdHlwaWNhbGx5IG9ubHkgaGF2ZSBzbWFsbCBudW1iZXIgb2Ygc2VyZGV2DQo+
IGRldmljZXMgKHNheSwgbWF4IDQpLCB1c2luZyAvc3lzL2J1cy9zZXJpYWwvZGV2aWNlcyBkaXJl
Y3RseSBzaG91bGQgbm90DQo+IGJlIHRvbyBiYWQgbWVhbndoaWxlLiBOb3QgdGhhdCBtdWNoIGNv
bW1vbiBpbmZvcm1hdGlvbiB3ZSBjYW4gZXhwb3NlDQo+IGVpdGhlciwgYXQgbGVhc3Qgbm90IGlu
IGNvbXBhcmlzb24gdG8gVVNCLiBCdXQgSSdsbCBrZWVwIGl0IG1pbmQuIDopDQo+DQoNClllcyBp
biB0aGUgaW50ZXJpbSwgaWYgeW91IGhhdmUgJ3RyZWUnIGluc3RhbGxlZCB0aGVuIGZvciBleGFt
cGxlDQoNCiQgYWxpYXMgbHNzZXJkZXY9J3RyZWUgL3N5cy9idXMvc2VyaWFsL2RldmljZXMvKi0w
Jw0KJCBsc3NlcmRldg0KL3N5cy9idXMvc2VyaWFsL2RldmljZXMvc2VyaWFsMC0wDQrilJzilIDi
lIAgYmx1ZXRvb3RoDQrilIIgICDilJTilIDilIAgaGNpMA0K4pSCICAgICAgIOKUnOKUgOKUgCBk
ZXZpY2UgLT4gLi4vLi4vLi4vc2VyaWFsMC0wDQrilIIgICAgICAg4pSc4pSA4pSAIHBvd2VyDQri
lIIgICAgICAg4pSCICAg4pSc4pSA4pSAIGFzeW5jDQrilIIgICAgICAg4pSCICAg4pSc4pSA4pSA
IGF1dG9zdXNwZW5kX2RlbGF5X21zDQrilIIgICAgICAg4pSCICAg4pSc4pSA4pSAIGNvbnRyb2wN
CuKUgiAgICAgICDilIIgICDilJzilIDilIAgcnVudGltZV9hY3RpdmVfa2lkcw0K4pSCICAgICAg
IOKUgiAgIOKUnOKUgOKUgCBydW50aW1lX2FjdGl2ZV90aW1lDQrilIIgICAgICAg4pSCICAg4pSc
4pSA4pSAIHJ1bnRpbWVfZW5hYmxlZA0K4pSCICAgICAgIOKUgiAgIOKUnOKUgOKUgCBydW50aW1l
X3N0YXR1cw0K4pSCICAgICAgIOKUgiAgIOKUnOKUgOKUgCBydW50aW1lX3N1c3BlbmRlZF90aW1l
DQrilIIgICAgICAg4pSCICAg4pSU4pSA4pSAIHJ1bnRpbWVfdXNhZ2UNCuKUgiAgICAgICDilJzi
lIDilIAgcmZraWxsMA0K4pSCICAgICAgIOKUgiAgIOKUnOKUgOKUgCBkZXZpY2UgLT4gLi4vLi4v
aGNpMA0K4pSCICAgICAgIOKUgiAgIOKUnOKUgOKUgCBoYXJkDQrilIIgICAgICAg4pSCICAg4pSc
4pSA4pSAIGluZGV4DQrilIIgICAgICAg4pSCICAg4pSc4pSA4pSAIG5hbWUNCuKUgiAgICAgICDi
lIIgICDilJzilIDilIAgcGVyc2lzdGVudA0K4pSCICAgICAgIOKUgiAgIOKUnOKUgOKUgCBwb3dl
cg0K4pSCICAgICAgIOKUgiAgIOKUgiAgIOKUnOKUgOKUgCBhc3luYw0K4pSCICAgICAgIOKUgiAg
IOKUgiAgIOKUnOKUgOKUgCBhdXRvc3VzcGVuZF9kZWxheV9tcw0K4pSCICAgICAgIOKUgiAgIOKU
giAgIOKUnOKUgOKUgCBjb250cm9sDQrilIIgICAgICAg4pSCICAg4pSCICAg4pSc4pSA4pSAIHJ1
bnRpbWVfYWN0aXZlX2tpZHMNCuKUgiAgICAgICDilIIgICDilIIgICDilJzilIDilIAgcnVudGlt
ZV9hY3RpdmVfdGltZQ0K4pSCICAgICAgIOKUgiAgIOKUgiAgIOKUnOKUgOKUgCBydW50aW1lX2Vu
YWJsZWQNCuKUgiAgICAgICDilIIgICDilIIgICDilJzilIDilIAgcnVudGltZV9zdGF0dXMNCuKU
giAgICAgICDilIIgICDilIIgICDilJzilIDilIAgcnVudGltZV9zdXNwZW5kZWRfdGltZQ0K4pSC
ICAgICAgIOKUgiAgIOKUgiAgIOKUlOKUgOKUgCBydW50aW1lX3VzYWdlDQrilIIgICAgICAg4pSC
ICAg4pSc4pSA4pSAIHNvZnQNCuKUgiAgICAgICDilIIgICDilJzilIDilIAgc3RhdGUNCuKUgiAg
ICAgICDilIIgICDilJzilIDilIAgc3Vic3lzdGVtIC0+IC4uLy4uLy4uLy4uLy4uLy4uLy4uLy4u
L2NsYXNzL3Jma2lsbA0K4pSCICAgICAgIOKUgiAgIOKUnOKUgOKUgCB0eXBlDQrilIIgICAgICAg
4pSCICAg4pSU4pSA4pSAIHVldmVudA0K4pSCICAgICAgIOKUnOKUgOKUgCBzdWJzeXN0ZW0gLT4g
Li4vLi4vLi4vLi4vLi4vLi4vLi4vY2xhc3MvYmx1ZXRvb3RoDQrilIIgICAgICAg4pSU4pSA4pSA
IHVldmVudA0K4pSc4pSA4pSAIGRyaXZlciAtPiAuLi8uLi8uLi8uLi8uLi9idXMvc2VyaWFsL2Ry
aXZlcnMvaGNpX3VhcnRfYmNtDQrilJzilIDilIAgZmlybXdhcmVfbm9kZSAtPg0KLi4vLi4vLi4v
Li4vTE5YU1lTVE06MDAvTE5YU1lCVVM6MDAvUE5QMEEwODowMC84MDg2MjI4QTowMC9CQ00yRUE0
OjAwDQrilJzilIDilIAgbW9kYWxpYXMNCuKUnOKUgOKUgCBwb3dlcg0K4pSCICAg4pSc4pSA4pSA
IGFzeW5jDQrilIIgICDilJzilIDilIAgYXV0b3N1c3BlbmRfZGVsYXlfbXMNCuKUgiAgIOKUnOKU
gOKUgCBjb250cm9sDQrilIIgICDilJzilIDilIAgcnVudGltZV9hY3RpdmVfa2lkcw0K4pSCICAg
4pSc4pSA4pSAIHJ1bnRpbWVfYWN0aXZlX3RpbWUNCuKUgiAgIOKUnOKUgOKUgCBydW50aW1lX2Vu
YWJsZWQNCuKUgiAgIOKUnOKUgOKUgCBydW50aW1lX3N0YXR1cw0K4pSCICAg4pSc4pSA4pSAIHJ1
bnRpbWVfc3VzcGVuZGVkX3RpbWUNCuKUgiAgIOKUlOKUgOKUgCBydW50aW1lX3VzYWdlDQrilJzi
lIDilIAgc3Vic3lzdGVtIC0+IC4uLy4uLy4uLy4uLy4uL2J1cy9zZXJpYWwNCuKUlOKUgOKUgCB1
ZXZlbnQNCg0KMTMgZGlyZWN0b3JpZXMsIDM4IGZpbGVzDQokDQoNCkl0IGlzIGEgYml0IHNoYWJi
eSB3aGVuIHRoZXJlIGlzIG5vdGhpbmcgdG8gcmVwb3J0DQoNCiQgbHNzZXJkZXYNCi9zeXMvYnVz
L3NlcmlhbC9kZXZpY2VzLyotMCBbZXJyb3Igb3BlbmluZyBkaXJdDQoNCjAgZGlyZWN0b3JpZXMs
IDAgZmlsZXMNCiQNCg==
On Tue, Oct 10, 2017 at 10:22:19AM +0200, Marcel Holtmann wrote:
> > Some of the above are minor nits that can be addressed by a follow-up
> > patch indeed, but the handling of nodes with no child devices needs to
> > be correct (or you could end up breaking normal serial-port support).
>
> that sounds good to me as well. Lets get this feature into the ACPI
> tree since I am going to push Hans’ patches into bluetooth-next as
> soon as he re-submits the missing ID change. That hopefully gives us
> then fully serdev support for ACPI and DT when it comes to Broadcom
> Bluetooth controllers.
But with Hans's work these devices should continue to function using the
platform-device hacks until the ACPI patch eventually lands in Linus's
tree (via the acpi tree).
> Next step is to convert the Intel driver to also use ACPI based serdev
> :)
>
> And for the brave, I think there are Realtek based systems using ACPI
> as well.
>
> What I was wondering the other day is if we need a lsserdev tool or
> some integration in lshw to be able to debug what serdev devices and
> ID are present. The lsusb and /sys/kernel/debug/usb/devices is just
> super powerful and easy when it comes to figuring out what people have
> in their system. Maybe /sys/kernel/debug/serdev/devices could be
> helpful as well. Just thinking out loud here.
Yeah, maybe. Since you'd typically only have small number of serdev
devices (say, max 4), using /sys/bus/serial/devices directly should not
be too bad meanwhile. Not that much common information we can expose
either, at least not in comparison to USB. But I'll keep it mind. :)
Johan
Hi Johan,
>>>> This patch allows SerDev module to manage serial devices declared as
>>>> attached to an UART in ACPI table.
>>>>
>>>> acpi_serdev_add_device() callback will only take into account entries
>>>> without enumerated flag set. This flags is set for all entries during
>>>> ACPI scan, except for SPI and I2C serial devices, and for UART with
>>>> 2nd patch in the series.
>>>>
>>>> Signed-off-by: Frédéric Danis <[email protected]>
>>>> ---
>>>> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
>>>> 1 file changed, 94 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
>>>> index c68fb3a..104777d 100644
>>>> --- a/drivers/tty/serdev/core.c
>>>> +++ b/drivers/tty/serdev/core.c
>
>>>> +static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
>>>> +{
>>>> + acpi_status status;
>>>> + acpi_handle handle;
>>>> +
>>>> + handle = ACPI_HANDLE(ctrl->dev.parent);
>>>> + if (!handle)
>>>> + return -ENODEV;
>>>> +
>>>> + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
>>>> + acpi_serdev_add_device, NULL, ctrl, NULL);
>>>> + if (ACPI_FAILURE(status)) {
>>>> + dev_warn(&ctrl->dev, "failed to enumerate Serial slaves\n");
>>>
>>> s/Serial/serdev/
>>>
>>>> + return -ENODEV;
>>>> + }
>>>> +
>>>> + return 0;
>>>
>>> What if there are no slaves defined? I'm not very familiar with the ACPI
>>> helpers, but from a quick look it seems you'd then end up returning zero
>>> here which would cause the serdev controller to be registered instead of
>>> the tty-class device.
>>>
>>> [ And if I'm mistaken, you do want to suppress that error message for
>>> when there are no slaves defined. ]
>
>>>> - ret = of_serdev_register_devices(ctrl);
>>>> - if (ret)
>>>> + ret_of = of_serdev_register_devices(ctrl);
>>>> + ret_acpi = acpi_serdev_register_devices(ctrl);
>>>> + if (ret_of && ret_acpi) {
>>>> + dev_dbg(&ctrl->dev, "serdev%d no devices registered: of:%d acpi:%d\n",
>>>
>>> "serdev%d" is redundant here as you're using dev_dbg (which will print
>>> the device name).
>>>
>>>> + ctrl->nr, ret_of, ret_acpi);
>>>> + ret = -ENODEV;
>>>> goto out_dev_del;
>>>> + }
>>>>
>>>> dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
>>>> ctrl->nr, &ctrl->dev);
>>>
>>> Hmm, I see it's already used here. No need to follow that example
>>> though.
>>
>> lets have an extra patch on top of it that fixes these.
>
> Some of the above are minor nits that can be addressed by a follow-up
> patch indeed, but the handling of nodes with no child devices needs to
> be correct (or you could end up breaking normal serial-port support).
that sounds good to me as well. Lets get this feature into the ACPI tree since I am going to push Hans’ patches into bluetooth-next as soon as he re-submits the missing ID change. That hopefully gives us then fully serdev support for ACPI and DT when it comes to Broadcom Bluetooth controllers.
Next step is to convert the Intel driver to also use ACPI based serdev :)
And for the brave, I think there are Realtek based systems using ACPI as well.
What I was wondering the other day is if we need a lsserdev tool or some integration in lshw to be able to debug what serdev devices and ID are present. The lsusb and /sys/kernel/debug/usb/devices is just super powerful and easy when it comes to figuring out what people have in their system. Maybe /sys/kernel/debug/serdev/devices could be helpful as well. Just thinking out loud here.
Regards
Marcel
On Tue, Oct 10, 2017 at 10:10:58AM +0200, Marcel Holtmann wrote:
> Hi Johan,
>
> >> This patch allows SerDev module to manage serial devices declared as
> >> attached to an UART in ACPI table.
> >>
> >> acpi_serdev_add_device() callback will only take into account entries
> >> without enumerated flag set. This flags is set for all entries during
> >> ACPI scan, except for SPI and I2C serial devices, and for UART with
> >> 2nd patch in the series.
> >>
> >> Signed-off-by: Fr?d?ric Danis <[email protected]>
> >> ---
> >> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
> >> 1 file changed, 94 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> >> index c68fb3a..104777d 100644
> >> --- a/drivers/tty/serdev/core.c
> >> +++ b/drivers/tty/serdev/core.c
> >> +static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
> >> +{
> >> + acpi_status status;
> >> + acpi_handle handle;
> >> +
> >> + handle = ACPI_HANDLE(ctrl->dev.parent);
> >> + if (!handle)
> >> + return -ENODEV;
> >> +
> >> + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> >> + acpi_serdev_add_device, NULL, ctrl, NULL);
> >> + if (ACPI_FAILURE(status)) {
> >> + dev_warn(&ctrl->dev, "failed to enumerate Serial slaves\n");
> >
> > s/Serial/serdev/
> >
> >> + return -ENODEV;
> >> + }
> >> +
> >> + return 0;
> >
> > What if there are no slaves defined? I'm not very familiar with the ACPI
> > helpers, but from a quick look it seems you'd then end up returning zero
> > here which would cause the serdev controller to be registered instead of
> > the tty-class device.
> >
> > [ And if I'm mistaken, you do want to suppress that error message for
> > when there are no slaves defined. ]
> >> - ret = of_serdev_register_devices(ctrl);
> >> - if (ret)
> >> + ret_of = of_serdev_register_devices(ctrl);
> >> + ret_acpi = acpi_serdev_register_devices(ctrl);
> >> + if (ret_of && ret_acpi) {
> >> + dev_dbg(&ctrl->dev, "serdev%d no devices registered: of:%d acpi:%d\n",
> >
> > "serdev%d" is redundant here as you're using dev_dbg (which will print
> > the device name).
> >
> >> + ctrl->nr, ret_of, ret_acpi);
> >> + ret = -ENODEV;
> >> goto out_dev_del;
> >> + }
> >>
> >> dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
> >> ctrl->nr, &ctrl->dev);
> >
> > Hmm, I see it's already used here. No need to follow that example
> > though.
>
> lets have an extra patch on top of it that fixes these.
Some of the above are minor nits that can be addressed by a follow-up
patch indeed, but the handling of nodes with no child devices needs to
be correct (or you could end up breaking normal serial-port support).
Johan
Hi Johan,
>> This patch allows SerDev module to manage serial devices declared as
>> attached to an UART in ACPI table.
>>
>> acpi_serdev_add_device() callback will only take into account entries
>> without enumerated flag set. This flags is set for all entries during
>> ACPI scan, except for SPI and I2C serial devices, and for UART with
>> 2nd patch in the series.
>>
>> Signed-off-by: Frédéric Danis <[email protected]>
>> ---
>> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 94 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
>> index c68fb3a..104777d 100644
>> --- a/drivers/tty/serdev/core.c
>> +++ b/drivers/tty/serdev/core.c
>
>> @@ -385,6 +401,74 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
>> return 0;
>> }
>>
>> +#ifdef CONFIG_ACPI
>> +static acpi_status acpi_serdev_register_device(struct serdev_controller *ctrl,
>> + struct acpi_device *adev)
>> +{
>> + struct serdev_device *serdev = NULL;
>> + int err;
>> +
>> + if (acpi_bus_get_status(adev) || !adev->status.present ||
>> + acpi_device_enumerated(adev))
>> + return AE_OK;
>> +
>> + serdev = serdev_device_alloc(ctrl);
>> + if (!serdev) {
>> + dev_err(&ctrl->dev, "failed to allocate Serial device for %s\n",
>
> s/Serial/serdev/
>
>> + dev_name(&adev->dev));
>> + return AE_NO_MEMORY;
>> + }
>> +
>> + ACPI_COMPANION_SET(&serdev->dev, adev);
>> + acpi_device_set_enumerated(adev);
>> +
>> + err = serdev_device_add(serdev);
>> + if (err) {
>> + dev_err(&serdev->dev,
>> + "failure adding ACPI device. status %d\n", err);
>
> s/ACPI/ACPI serdev/?
>
>> + serdev_device_put(serdev);
>> + }
>> +
>> + return AE_OK;
>> +}
>> +
>> +static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level,
>> + void *data, void **return_value)
>> +{
>> + struct serdev_controller *ctrl = data;
>> + struct acpi_device *adev;
>> +
>> + if (acpi_bus_get_device(handle, &adev))
>> + return AE_OK;
>> +
>> + return acpi_serdev_register_device(ctrl, adev);
>> +}
>> +
>> +static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
>> +{
>> + acpi_status status;
>> + acpi_handle handle;
>> +
>> + handle = ACPI_HANDLE(ctrl->dev.parent);
>> + if (!handle)
>> + return -ENODEV;
>> +
>> + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
>> + acpi_serdev_add_device, NULL, ctrl, NULL);
>> + if (ACPI_FAILURE(status)) {
>> + dev_warn(&ctrl->dev, "failed to enumerate Serial slaves\n");
>
> s/Serial/serdev/
>
>> + return -ENODEV;
>> + }
>> +
>> + return 0;
>
> What if there are no slaves defined? I'm not very familiar with the ACPI
> helpers, but from a quick look it seems you'd then end up returning zero
> here which would cause the serdev controller to be registered instead of
> the tty-class device.
>
> [ And if I'm mistaken, you do want to suppress that error message for
> when there are no slaves defined. ]
>
>> +}
>> +#else
>> +static inline int acpi_serdev_register_devices(struct serdev_controller *ctlr)
>
> s/ctlr/ctrl/
>
>> +{
>> + return -ENODEV;
>> +}
>> +#endif /* CONFIG_ACPI */
>> +
>> /**
>> * serdev_controller_add() - Add an serdev controller
>> * @ctrl: controller to be registered.
>> @@ -394,7 +478,7 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
>> */
>> int serdev_controller_add(struct serdev_controller *ctrl)
>> {
>> - int ret;
>> + int ret_of, ret_acpi, ret;
>>
>> /* Can't register until after driver model init */
>> if (WARN_ON(!is_registered))
>> @@ -404,9 +488,14 @@ int serdev_controller_add(struct serdev_controller *ctrl)
>> if (ret)
>> return ret;
>>
>> - ret = of_serdev_register_devices(ctrl);
>> - if (ret)
>> + ret_of = of_serdev_register_devices(ctrl);
>> + ret_acpi = acpi_serdev_register_devices(ctrl);
>> + if (ret_of && ret_acpi) {
>> + dev_dbg(&ctrl->dev, "serdev%d no devices registered: of:%d acpi:%d\n",
>
> "serdev%d" is redundant here as you're using dev_dbg (which will print
> the device name).
>
>> + ctrl->nr, ret_of, ret_acpi);
>> + ret = -ENODEV;
>> goto out_dev_del;
>> + }
>>
>> dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
>> ctrl->nr, &ctrl->dev);
>
> Hmm, I see it's already used here. No need to follow that example
> though.
lets have an extra patch on top of it that fixes these.
Regards
Marcel
On Mon, Oct 09, 2017 at 08:09:19PM +0200, Marcel Holtmann wrote:
> Hi Johan,
>
> >>>>>> UART devices is expected to be enumerated by SerDev subsystem.
> >>>>>>
> >>>>>> During ACPI scan, serial devices behind SPI, I2C or UART buses are not
> >>>>>> enumerated, allowing them to be enumerated by their respective parents.
> >>>>>>
> >>>>>> Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
> >>>>>> devices on serial buses (SPI, I2C or UART).
> >>>>>>
> >>>>>> On Macs an empty ResourceTemplate is returned for uart slaves.
> >>>>>> Instead the device properties "baud", "parity", "dataBits", "stopBits" are
> >>>>>> provided. Add a check for "baud" in acpi_is_serial_bus_slave().
> >>>>>>
> >>>>>> Signed-off-by: Fr?d?ric Danis <[email protected]>
> >>>>>
> >>>>> So just to reiterate what I just mentioned in a comment to one of Hans's
> >>>>> hci_bcm patches:
> >>>>>
> >>>>> This one would silently break PM for such devices on any system which
> >>>>> does not have serdev enabled (as the corresponding platform devices
> >>>>> would no longer be registered). And with serdev enabled, hciattach
> >>>>> (btattach) would start failing as the tty device would no longer be
> >>>>> registered (but I assume everyone is aware of that, and fine with it, by
> >>>>> now).
> >>>>>
> >>>>> Perhaps the hci_bcm driver should start depending on
> >>>>> SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
> >>>>
> >>>> ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
> >>>> since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
> >>>> for serdev. If any other controller is implemented that one could
> >>>> also be used.
> >>>
> >>> Not for hci_bcm, right? This particular driver specifically depends on
> >>> SERIAL_DEV_CTRL_TTYPORT for the ACPI devices and not just any (future)
> >>> serdev controller (or currently working systems soon breaks silently).
> >>>
> >>> I don't think the same is true for the DT case where we do not already
> >>> have child nodes defined in firmware (and in fact, this driver did not
> >>> really support DT before serdev).
> >>
> >> The serdev ACPI support has been added to the core and not to
> >> the ttyport and the hci_bcm driver only uses functions from the
> >> core. As far as I can see the ACPI part would also work fine with
> >> a different serdev controller.
> >
> > Indeed, but you need SERIAL_DEV_CTRL_TTYPORT to avoid silently breaking
> > current ACPI setups which breaks when this patch is applied (as these
> > devices all hang off of common serial ports managed by serial core).
> >
> >> Of course DT and ACPI currently require SERIAL_DEV_CTRL_TTYPORT,
> >> since it's the only serdev controller implementation. Also it
> >> covers most use cases. When SERIAL_DEV_BUS is selected it's
> >> very likely, that you also want SERIAL_DEV_CTRL_TTYPORT.
> >
> >>>> I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
> >>>> it together with SERDEV. I suspect that we won't see any other
> >>>> controller (it would be a UART device, that is not registered as
> >>>> tty device) in the next few years and the extra option seems to
> >>>> confuse people.
> >>>
> >>> I agree that it is somewhat confusing. But now that we have both,
> >>> perhaps simply having SERIAL_DEV_CTRL_TTYPORT default to "y" when
> >>> SERIAL_DEV_BUS is selected could be a compromise. The Kconfig entry
> >>> might need to be amended as well (e.g. if only to mention that you
> >>> need to select a controller as well).
> >>
> >> I think we should at least add a default "y" if SERIAL_DEV_BUS.
> >
> > I'm preparing a patch.
>
> yes, please prepare a patch since the discussion spans multiple email
> threads now. Lets get this back on track and find a patch that we are
> all happy with.
I submitted a patch amending the serdev Kconfig entries and making
SERIAL_DEV_CTRL_TTYPORT default to Y when serdev support has been
chosen.
While that hopefully reduces the confusion regarding the Kconfig options
somewhat, it's not really a fix for the potential silent hci_bcm
regression that could result from this patch.
[ The problem being that hci-attach would still succeed, but PM would be
broken, when SERIAL_DEV_CTRL_TTYPORT is not set. ]
I mentioned that making HCI_BCM depend on SERIAL_DEV_CTRL_TTYPORT might
be used to avoid this situation, although it is on some level is
conceptually wrong to describe runtime dependencies in Kconfig (cf.
having a USB device driver, depend on a particular host controller
rather than just USB support).
I'll write something like that up in a patch for Bluetooth and you can
decide if you want to apply it to remedy this particular situation,
though.
Johan
On Wed, Oct 4, 2017 at 10:51 AM, Fr=C3=A9d=C3=A9ric Danis
<[email protected]> wrote:
> Add ACPI support for serial attached devices.
>
> Currently, serial devices are not set as enumerated during ACPI scan for =
SPI
> or i2c buses (but not for UART). This should also be done for UART serial
> devices.
> I renamed *spi_i2c_slave* to *serial_bus_slave* to reflect this.
>
> For hci_bcm, this needs Hans de Goede's "Bluetooth: hci_bcm: Add (runtime=
)pm
> support to the serdev drv" patches to work.
>
> Tested on T100TA with Broadcom BCM2E39.
>
> Since RFC:
> - Add or reword commit messages
> - Rename *serial_slave* to *serial_bus_slave*
> - Add specific check for Apple in acpi_is_serial_bus_slave(), thanks to
> Lukas Wunner
> - Update comment in acpi_default_enumeration()
> - Remove patch 3 "Bluetooth: hci_bcm: Add ACPI serdev support for BCM2E=
39"
> in favor of patches from Hans de Goede
>
> Fr=C3=A9d=C3=A9ric Danis (2):
> serdev: Add ACPI support
> ACPI / scan: Fix enumeration for special UART devices
Some review comments you received on this series should be addressed
IMO, so I'm expecting an update of it.
When sending the next version, please add all of the tags you've
already received on it.
Thanks,
Rafael
Hi Johan,
>>>>>> UART devices is expected to be enumerated by SerDev subsystem.
>>>>>>
>>>>>> During ACPI scan, serial devices behind SPI, I2C or UART buses are not
>>>>>> enumerated, allowing them to be enumerated by their respective parents.
>>>>>>
>>>>>> Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
>>>>>> devices on serial buses (SPI, I2C or UART).
>>>>>>
>>>>>> On Macs an empty ResourceTemplate is returned for uart slaves.
>>>>>> Instead the device properties "baud", "parity", "dataBits", "stopBits" are
>>>>>> provided. Add a check for "baud" in acpi_is_serial_bus_slave().
>>>>>>
>>>>>> Signed-off-by: Frédéric Danis <[email protected]>
>>>>>
>>>>> So just to reiterate what I just mentioned in a comment to one of Hans's
>>>>> hci_bcm patches:
>>>>>
>>>>> This one would silently break PM for such devices on any system which
>>>>> does not have serdev enabled (as the corresponding platform devices
>>>>> would no longer be registered). And with serdev enabled, hciattach
>>>>> (btattach) would start failing as the tty device would no longer be
>>>>> registered (but I assume everyone is aware of that, and fine with it, by
>>>>> now).
>>>>>
>>>>> Perhaps the hci_bcm driver should start depending on
>>>>> SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
>>>>
>>>> ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
>>>> since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
>>>> for serdev. If any other controller is implemented that one could
>>>> also be used.
>>>
>>> Not for hci_bcm, right? This particular driver specifically depends on
>>> SERIAL_DEV_CTRL_TTYPORT for the ACPI devices and not just any (future)
>>> serdev controller (or currently working systems soon breaks silently).
>>>
>>> I don't think the same is true for the DT case where we do not already
>>> have child nodes defined in firmware (and in fact, this driver did not
>>> really support DT before serdev).
>>
>> The serdev ACPI support has been added to the core and not to
>> the ttyport and the hci_bcm driver only uses functions from the
>> core. As far as I can see the ACPI part would also work fine with
>> a different serdev controller.
>
> Indeed, but you need SERIAL_DEV_CTRL_TTYPORT to avoid silently breaking
> current ACPI setups which breaks when this patch is applied (as these
> devices all hang off of common serial ports managed by serial core).
>
>> Of course DT and ACPI currently require SERIAL_DEV_CTRL_TTYPORT,
>> since it's the only serdev controller implementation. Also it
>> covers most use cases. When SERIAL_DEV_BUS is selected it's
>> very likely, that you also want SERIAL_DEV_CTRL_TTYPORT.
>
>>>> I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
>>>> it together with SERDEV. I suspect that we won't see any other
>>>> controller (it would be a UART device, that is not registered as
>>>> tty device) in the next few years and the extra option seems to
>>>> confuse people.
>>>
>>> I agree that it is somewhat confusing. But now that we have both,
>>> perhaps simply having SERIAL_DEV_CTRL_TTYPORT default to "y" when
>>> SERIAL_DEV_BUS is selected could be a compromise. The Kconfig entry
>>> might need to be amended as well (e.g. if only to mention that you
>>> need to select a controller as well).
>>
>> I think we should at least add a default "y" if SERIAL_DEV_BUS.
>
> I'm preparing a patch.
yes, please prepare a patch since the discussion spans multiple email threads now. Lets get this back on track and find a patch that we are all happy with.
Regards
Marcel
On Mon, Oct 09, 2017 at 10:55:34AM +0200, Sebastian Reichel wrote:
> Hi,
>
> On Mon, Oct 09, 2017 at 09:35:26AM +0200, Johan Hovold wrote:
> > On Sun, Oct 08, 2017 at 12:53:11AM +0200, Sebastian Reichel wrote:
> > > On Sat, Oct 07, 2017 at 05:19:34PM +0200, Johan Hovold wrote:
> > > > On Wed, Oct 04, 2017 at 10:51:30AM +0200, Fr?d?ric Danis wrote:
> > > > > UART devices is expected to be enumerated by SerDev subsystem.
> > > > >
> > > > > During ACPI scan, serial devices behind SPI, I2C or UART buses are not
> > > > > enumerated, allowing them to be enumerated by their respective parents.
> > > > >
> > > > > Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
> > > > > devices on serial buses (SPI, I2C or UART).
> > > > >
> > > > > On Macs an empty ResourceTemplate is returned for uart slaves.
> > > > > Instead the device properties "baud", "parity", "dataBits", "stopBits" are
> > > > > provided. Add a check for "baud" in acpi_is_serial_bus_slave().
> > > > >
> > > > > Signed-off-by: Fr?d?ric Danis <[email protected]>
> > > >
> > > > So just to reiterate what I just mentioned in a comment to one of Hans's
> > > > hci_bcm patches:
> > > >
> > > > This one would silently break PM for such devices on any system which
> > > > does not have serdev enabled (as the corresponding platform devices
> > > > would no longer be registered). And with serdev enabled, hciattach
> > > > (btattach) would start failing as the tty device would no longer be
> > > > registered (but I assume everyone is aware of that, and fine with it, by
> > > > now).
> > > >
> > > > Perhaps the hci_bcm driver should start depending on
> > > > SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
> > >
> > > ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
> > > since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
> > > for serdev. If any other controller is implemented that one could
> > > also be used.
> >
> > Not for hci_bcm, right? This particular driver specifically depends on
> > SERIAL_DEV_CTRL_TTYPORT for the ACPI devices and not just any (future)
> > serdev controller (or currently working systems soon breaks silently).
> >
> > I don't think the same is true for the DT case where we do not already
> > have child nodes defined in firmware (and in fact, this driver did not
> > really support DT before serdev).
>
> The serdev ACPI support has been added to the core and not to
> the ttyport and the hci_bcm driver only uses functions from the
> core. As far as I can see the ACPI part would also work fine with
> a different serdev controller.
Indeed, but you need SERIAL_DEV_CTRL_TTYPORT to avoid silently breaking
current ACPI setups which breaks when this patch is applied (as these
devices all hang off of common serial ports managed by serial core).
> Of course DT and ACPI currently require SERIAL_DEV_CTRL_TTYPORT,
> since it's the only serdev controller implementation. Also it
> covers most use cases. When SERIAL_DEV_BUS is selected it's
> very likely, that you also want SERIAL_DEV_CTRL_TTYPORT.
> > > I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
> > > it together with SERDEV. I suspect that we won't see any other
> > > controller (it would be a UART device, that is not registered as
> > > tty device) in the next few years and the extra option seems to
> > > confuse people.
> >
> > I agree that it is somewhat confusing. But now that we have both,
> > perhaps simply having SERIAL_DEV_CTRL_TTYPORT default to "y" when
> > SERIAL_DEV_BUS is selected could be a compromise. The Kconfig entry
> > might need to be amended as well (e.g. if only to mention that you
> > need to select a controller as well).
>
> I think we should at least add a default "y" if SERIAL_DEV_BUS.
I'm preparing a patch.
> > And the bluetooth uart drivers already depend on SERIAL_DEV_BUS.
>
> Yes and that's the correct dependency. They only need the serdev
> core and controller. The only reason they do not work without
> SERIAL_DEV_CTRL_TTYPORT is, that there won't be any serdev
> controller.
In general, yes. Again, the only exception would be hci_bcm to avoid
breaking current setups without people noticing.
> Note, that the default "y" if SERIAL_DEV_BUS in SERIAL_DEV_CTRL_TTYPORT's
> config entry is only a partial fix. There is still the problem,
> that SERIAL_DEV_CTRL_TTYPORT can only be enabled if SERIAL_DEV_BUS
> is configured builtin. This is a limitation of the ttyport
> implementation, that hooks into builtin TTY core code.
I'm not saying it's a fix, but it is a sane default. I'm preparing a
patch also amending the Kconfig entries, and we can take it from there.
Johan
Hi,
On Sun, Oct 08, 2017 at 10:51:52AM +0200, Marcel Holtmann wrote:
> Hi Sebastian,
>
> >>> UART devices is expected to be enumerated by SerDev subsystem.
> >>>
> >>> During ACPI scan, serial devices behind SPI, I2C or UART buses are not
> >>> enumerated, allowing them to be enumerated by their respective parents.
> >>>
> >>> Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
> >>> devices on serial buses (SPI, I2C or UART).
> >>>
> >>> On Macs an empty ResourceTemplate is returned for uart slaves.
> >>> Instead the device properties "baud", "parity", "dataBits", "stopBits" are
> >>> provided. Add a check for "baud" in acpi_is_serial_bus_slave().
> >>>
> >>> Signed-off-by: Fr?d?ric Danis <[email protected]>
> >>
> >> So just to reiterate what I just mentioned in a comment to one of Hans's
> >> hci_bcm patches:
> >>
> >> This one would silently break PM for such devices on any system which
> >> does not have serdev enabled (as the corresponding platform devices
> >> would no longer be registered). And with serdev enabled, hciattach
> >> (btattach) would start failing as the tty device would no longer be
> >> registered (but I assume everyone is aware of that, and fine with it, by
> >> now).
> >>
> >> Perhaps the hci_bcm driver should start depending on
> >> SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
> >
> > ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
> > since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
> > for serdev. If any other controller is implemented that one could
> > also be used.
> >
> > I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
> > it together with SERDEV. I suspect that we won't see any other
> > controller (it would be a UART device, that is not registered as
> > tty device) in the next few years and the extra option seems to
> > confuse people.
>
> I wonder if we should just default SERIAL_DEV_CTRL_TTYPORT=y when
> DT or ACPI is enabled. Then no driver would have to select or
> depend on it.
This is not related to DT/ACPI. SERIAL_DEV_CTRL_TTYPORT is a serdev
controller driver, that registers serdev controller devices for each
tty. It will also be used by clients, that are registered via board
code.
-- Sebastian
Hi,
On Mon, Oct 09, 2017 at 09:35:26AM +0200, Johan Hovold wrote:
> On Sun, Oct 08, 2017 at 12:53:11AM +0200, Sebastian Reichel wrote:
> > On Sat, Oct 07, 2017 at 05:19:34PM +0200, Johan Hovold wrote:
> > > On Wed, Oct 04, 2017 at 10:51:30AM +0200, Fr?d?ric Danis wrote:
> > > > UART devices is expected to be enumerated by SerDev subsystem.
> > > >
> > > > During ACPI scan, serial devices behind SPI, I2C or UART buses are not
> > > > enumerated, allowing them to be enumerated by their respective parents.
> > > >
> > > > Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
> > > > devices on serial buses (SPI, I2C or UART).
> > > >
> > > > On Macs an empty ResourceTemplate is returned for uart slaves.
> > > > Instead the device properties "baud", "parity", "dataBits", "stopBits" are
> > > > provided. Add a check for "baud" in acpi_is_serial_bus_slave().
> > > >
> > > > Signed-off-by: Fr?d?ric Danis <[email protected]>
> > >
> > > So just to reiterate what I just mentioned in a comment to one of Hans's
> > > hci_bcm patches:
> > >
> > > This one would silently break PM for such devices on any system which
> > > does not have serdev enabled (as the corresponding platform devices
> > > would no longer be registered). And with serdev enabled, hciattach
> > > (btattach) would start failing as the tty device would no longer be
> > > registered (but I assume everyone is aware of that, and fine with it, by
> > > now).
> > >
> > > Perhaps the hci_bcm driver should start depending on
> > > SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
> >
> > ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
> > since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
> > for serdev. If any other controller is implemented that one could
> > also be used.
>
> Not for hci_bcm, right? This particular driver specifically depends on
> SERIAL_DEV_CTRL_TTYPORT for the ACPI devices and not just any (future)
> serdev controller (or currently working systems soon breaks silently).
>
> I don't think the same is true for the DT case where we do not already
> have child nodes defined in firmware (and in fact, this driver did not
> really support DT before serdev).
The serdev ACPI support has been added to the core and not to
the ttyport and the hci_bcm driver only uses functions from the
core. As far as I can see the ACPI part would also work fine with
a different serdev controller.
Of course DT and ACPI currently require SERIAL_DEV_CTRL_TTYPORT,
since it's the only serdev controller implementation. Also it
covers most use cases. When SERIAL_DEV_BUS is selected it's
very likely, that you also want SERIAL_DEV_CTRL_TTYPORT.
> > I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
> > it together with SERDEV. I suspect that we won't see any other
> > controller (it would be a UART device, that is not registered as
> > tty device) in the next few years and the extra option seems to
> > confuse people.
>
> I agree that it is somewhat confusing. But now that we have both,
> perhaps simply having SERIAL_DEV_CTRL_TTYPORT default to "y" when
> SERIAL_DEV_BUS is selected could be a compromise. The Kconfig entry
> might need to be amended as well (e.g. if only to mention that you
> need to select a controller as well).
I think we should at least add a default "y" if SERIAL_DEV_BUS.
> And the bluetooth uart drivers already depend on SERIAL_DEV_BUS.
Yes and that's the correct dependency. They only need the serdev
core and controller. The only reason they do not work without
SERIAL_DEV_CTRL_TTYPORT is, that there won't be any serdev
controller.
Note, that the default "y" if SERIAL_DEV_BUS in SERIAL_DEV_CTRL_TTYPORT's
config entry is only a partial fix. There is still the problem,
that SERIAL_DEV_CTRL_TTYPORT can only be enabled if SERIAL_DEV_BUS
is configured builtin. This is a limitation of the ttyport
implementation, that hooks into builtin TTY core code.
-- Sebastian
On Sun, Oct 08, 2017 at 12:53:11AM +0200, Sebastian Reichel wrote:
> Hi,
>
> On Sat, Oct 07, 2017 at 05:19:34PM +0200, Johan Hovold wrote:
> > On Wed, Oct 04, 2017 at 10:51:30AM +0200, Fr?d?ric Danis wrote:
> > > UART devices is expected to be enumerated by SerDev subsystem.
> > >
> > > During ACPI scan, serial devices behind SPI, I2C or UART buses are not
> > > enumerated, allowing them to be enumerated by their respective parents.
> > >
> > > Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
> > > devices on serial buses (SPI, I2C or UART).
> > >
> > > On Macs an empty ResourceTemplate is returned for uart slaves.
> > > Instead the device properties "baud", "parity", "dataBits", "stopBits" are
> > > provided. Add a check for "baud" in acpi_is_serial_bus_slave().
> > >
> > > Signed-off-by: Fr?d?ric Danis <[email protected]>
> >
> > So just to reiterate what I just mentioned in a comment to one of Hans's
> > hci_bcm patches:
> >
> > This one would silently break PM for such devices on any system which
> > does not have serdev enabled (as the corresponding platform devices
> > would no longer be registered). And with serdev enabled, hciattach
> > (btattach) would start failing as the tty device would no longer be
> > registered (but I assume everyone is aware of that, and fine with it, by
> > now).
> >
> > Perhaps the hci_bcm driver should start depending on
> > SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
>
> ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
> since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
> for serdev. If any other controller is implemented that one could
> also be used.
Not for hci_bcm, right? This particular driver specifically depends on
SERIAL_DEV_CTRL_TTYPORT for the ACPI devices and not just any (future)
serdev controller (or currently working systems soon breaks silently).
I don't think the same is true for the DT case where we do not already
have child nodes defined in firmware (and in fact, this driver did not
really support DT before serdev).
> I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
> it together with SERDEV. I suspect that we won't see any other
> controller (it would be a UART device, that is not registered as
> tty device) in the next few years and the extra option seems to
> confuse people.
I agree that it is somewhat confusing. But now that we have both,
perhaps simply having SERIAL_DEV_CTRL_TTYPORT default to "y" when
SERIAL_DEV_BUS is selected could be a compromise. The Kconfig entry
might need to be amended as well (e.g. if only to mention that you need
to select a controller as well).
And the bluetooth uart drivers already depend on SERIAL_DEV_BUS.
Johan
Hi Sebastian,
>>> UART devices is expected to be enumerated by SerDev subsystem.
>>>
>>> During ACPI scan, serial devices behind SPI, I2C or UART buses are not
>>> enumerated, allowing them to be enumerated by their respective parents.
>>>
>>> Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
>>> devices on serial buses (SPI, I2C or UART).
>>>
>>> On Macs an empty ResourceTemplate is returned for uart slaves.
>>> Instead the device properties "baud", "parity", "dataBits", "stopBits" are
>>> provided. Add a check for "baud" in acpi_is_serial_bus_slave().
>>>
>>> Signed-off-by: Frédéric Danis <[email protected]>
>>
>> So just to reiterate what I just mentioned in a comment to one of Hans's
>> hci_bcm patches:
>>
>> This one would silently break PM for such devices on any system which
>> does not have serdev enabled (as the corresponding platform devices
>> would no longer be registered). And with serdev enabled, hciattach
>> (btattach) would start failing as the tty device would no longer be
>> registered (but I assume everyone is aware of that, and fine with it, by
>> now).
>>
>> Perhaps the hci_bcm driver should start depending on
>> SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
>
> ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
> since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
> for serdev. If any other controller is implemented that one could
> also be used.
>
> I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
> it together with SERDEV. I suspect that we won't see any other
> controller (it would be a UART device, that is not registered as
> tty device) in the next few years and the extra option seems to
> confuse people.
I wonder if we should just default SERIAL_DEV_CTRL_TTYPORT=y when DT or ACPI is enabled. Then no driver would have to select or depend on it.
Regards
Marcel
Hi,
On Sat, Oct 07, 2017 at 05:19:34PM +0200, Johan Hovold wrote:
> On Wed, Oct 04, 2017 at 10:51:30AM +0200, Fr?d?ric Danis wrote:
> > UART devices is expected to be enumerated by SerDev subsystem.
> >
> > During ACPI scan, serial devices behind SPI, I2C or UART buses are not
> > enumerated, allowing them to be enumerated by their respective parents.
> >
> > Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
> > devices on serial buses (SPI, I2C or UART).
> >
> > On Macs an empty ResourceTemplate is returned for uart slaves.
> > Instead the device properties "baud", "parity", "dataBits", "stopBits" are
> > provided. Add a check for "baud" in acpi_is_serial_bus_slave().
> >
> > Signed-off-by: Fr?d?ric Danis <[email protected]>
>
> So just to reiterate what I just mentioned in a comment to one of Hans's
> hci_bcm patches:
>
> This one would silently break PM for such devices on any system which
> does not have serdev enabled (as the corresponding platform devices
> would no longer be registered). And with serdev enabled, hciattach
> (btattach) would start failing as the tty device would no longer be
> registered (but I assume everyone is aware of that, and fine with it, by
> now).
>
> Perhaps the hci_bcm driver should start depending on
> SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
for serdev. If any other controller is implemented that one could
also be used.
I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
it together with SERDEV. I suspect that we won't see any other
controller (it would be a UART device, that is not registered as
tty device) in the next few years and the extra option seems to
confuse people.
-- Sebastian
On Wed, Oct 04, 2017 at 10:51:30AM +0200, Fr?d?ric Danis wrote:
> UART devices is expected to be enumerated by SerDev subsystem.
>
> During ACPI scan, serial devices behind SPI, I2C or UART buses are not
> enumerated, allowing them to be enumerated by their respective parents.
>
> Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
> devices on serial buses (SPI, I2C or UART).
>
> On Macs an empty ResourceTemplate is returned for uart slaves.
> Instead the device properties "baud", "parity", "dataBits", "stopBits" are
> provided. Add a check for "baud" in acpi_is_serial_bus_slave().
>
> Signed-off-by: Fr?d?ric Danis <[email protected]>
So just to reiterate what I just mentioned in a comment to one of Hans's
hci_bcm patches:
This one would silently break PM for such devices on any system which
does not have serdev enabled (as the corresponding platform devices
would no longer be registered). And with serdev enabled, hciattach
(btattach) would start failing as the tty device would no longer be
registered (but I assume everyone is aware of that, and fine with it, by
now).
Perhaps the hci_bcm driver should start depending on
SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
Johan
On Fri, Oct 06, 2017 at 07:36:27PM +0200, Fr?d?ric Danis wrote:
> I took a look at dmesg.txt and I did not find any trace related to serdev.
> On the T100, where ttyS4 is used for Bluetooth, I can see the following
> traces:
> ? [?? 11.732347] serial serial0: allocated controller
> 0xffff880036229000 id 0
> ? [?? 11.732470] serial serial0-0: device serial0-0 registered
> ? [?? 11.732475] serial serial0: serdev0 registered: dev:ffff880036229000
> ? [?? 11.732478] serial serial0: tty port ttyS4 registered
>
> If serdev registration failed you should at least get something like:
> ??? serdev0 no devices registered: of:<> acpi:<>
Only with debugging enabled (it's a dev_dbg).
Johan
On Wed, Oct 04, 2017 at 10:51:29AM +0200, Fr?d?ric Danis wrote:
> This patch allows SerDev module to manage serial devices declared as
> attached to an UART in ACPI table.
>
> acpi_serdev_add_device() callback will only take into account entries
> without enumerated flag set. This flags is set for all entries during
> ACPI scan, except for SPI and I2C serial devices, and for UART with
> 2nd patch in the series.
>
> Signed-off-by: Fr?d?ric Danis <[email protected]>
> ---
> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 94 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index c68fb3a..104777d 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -385,6 +401,74 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
> return 0;
> }
>
> +#ifdef CONFIG_ACPI
> +static acpi_status acpi_serdev_register_device(struct serdev_controller *ctrl,
> + struct acpi_device *adev)
> +{
> + struct serdev_device *serdev = NULL;
> + int err;
> +
> + if (acpi_bus_get_status(adev) || !adev->status.present ||
> + acpi_device_enumerated(adev))
> + return AE_OK;
> +
> + serdev = serdev_device_alloc(ctrl);
> + if (!serdev) {
> + dev_err(&ctrl->dev, "failed to allocate Serial device for %s\n",
s/Serial/serdev/
> + dev_name(&adev->dev));
> + return AE_NO_MEMORY;
> + }
> +
> + ACPI_COMPANION_SET(&serdev->dev, adev);
> + acpi_device_set_enumerated(adev);
> +
> + err = serdev_device_add(serdev);
> + if (err) {
> + dev_err(&serdev->dev,
> + "failure adding ACPI device. status %d\n", err);
s/ACPI/ACPI serdev/?
> + serdev_device_put(serdev);
> + }
> +
> + return AE_OK;
> +}
> +
> +static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level,
> + void *data, void **return_value)
> +{
> + struct serdev_controller *ctrl = data;
> + struct acpi_device *adev;
> +
> + if (acpi_bus_get_device(handle, &adev))
> + return AE_OK;
> +
> + return acpi_serdev_register_device(ctrl, adev);
> +}
> +
> +static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
> +{
> + acpi_status status;
> + acpi_handle handle;
> +
> + handle = ACPI_HANDLE(ctrl->dev.parent);
> + if (!handle)
> + return -ENODEV;
> +
> + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> + acpi_serdev_add_device, NULL, ctrl, NULL);
> + if (ACPI_FAILURE(status)) {
> + dev_warn(&ctrl->dev, "failed to enumerate Serial slaves\n");
s/Serial/serdev/
> + return -ENODEV;
> + }
> +
> + return 0;
What if there are no slaves defined? I'm not very familiar with the ACPI
helpers, but from a quick look it seems you'd then end up returning zero
here which would cause the serdev controller to be registered instead of
the tty-class device.
[ And if I'm mistaken, you do want to suppress that error message for
when there are no slaves defined. ]
> +}
> +#else
> +static inline int acpi_serdev_register_devices(struct serdev_controller *ctlr)
s/ctlr/ctrl/
> +{
> + return -ENODEV;
> +}
> +#endif /* CONFIG_ACPI */
> +
> /**
> * serdev_controller_add() - Add an serdev controller
> * @ctrl: controller to be registered.
> @@ -394,7 +478,7 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
> */
> int serdev_controller_add(struct serdev_controller *ctrl)
> {
> - int ret;
> + int ret_of, ret_acpi, ret;
>
> /* Can't register until after driver model init */
> if (WARN_ON(!is_registered))
> @@ -404,9 +488,14 @@ int serdev_controller_add(struct serdev_controller *ctrl)
> if (ret)
> return ret;
>
> - ret = of_serdev_register_devices(ctrl);
> - if (ret)
> + ret_of = of_serdev_register_devices(ctrl);
> + ret_acpi = acpi_serdev_register_devices(ctrl);
> + if (ret_of && ret_acpi) {
> + dev_dbg(&ctrl->dev, "serdev%d no devices registered: of:%d acpi:%d\n",
"serdev%d" is redundant here as you're using dev_dbg (which will print
the device name).
> + ctrl->nr, ret_of, ret_acpi);
> + ret = -ENODEV;
> goto out_dev_del;
> + }
>
> dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
> ctrl->nr, &ctrl->dev);
Hmm, I see it's already used here. No need to follow that example
though.
Johan
Hi,
On Wed, Oct 04, 2017 at 10:51:30AM +0200, Fr?d?ric Danis wrote:
> UART devices is expected to be enumerated by SerDev subsystem.
>
> During ACPI scan, serial devices behind SPI, I2C or UART buses are not
> enumerated, allowing them to be enumerated by their respective parents.
>
> Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
> devices on serial buses (SPI, I2C or UART).
>
> On Macs an empty ResourceTemplate is returned for uart slaves.
> Instead the device properties "baud", "parity", "dataBits", "stopBits" are
> provided. Add a check for "baud" in acpi_is_serial_bus_slave().
>
> Signed-off-by: Fr?d?ric Danis <[email protected]>
> ---
Reviewed-by: Sebastian Reichel <[email protected]>
-- Sebastian
> drivers/acpi/scan.c | 37 +++++++++++++++++--------------------
> include/acpi/acpi_bus.h | 2 +-
> 2 files changed, 18 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 602f8ff..860b698 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1505,41 +1505,38 @@ static void acpi_init_coherency(struct acpi_device *adev)
> adev->flags.coherent_dma = cca;
> }
>
> -static int acpi_check_spi_i2c_slave(struct acpi_resource *ares, void *data)
> +static int acpi_check_serial_bus_slave(struct acpi_resource *ares, void *data)
> {
> - bool *is_spi_i2c_slave_p = data;
> + bool *is_serial_bus_slave_p = data;
>
> if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
> return 1;
>
> - /*
> - * devices that are connected to UART still need to be enumerated to
> - * platform bus
> - */
> - if (ares->data.common_serial_bus.type != ACPI_RESOURCE_SERIAL_TYPE_UART)
> - *is_spi_i2c_slave_p = true;
> + *is_serial_bus_slave_p = true;
>
> /* no need to do more checking */
> return -1;
> }
>
> -static bool acpi_is_spi_i2c_slave(struct acpi_device *device)
> +static bool acpi_is_serial_bus_slave(struct acpi_device *device)
> {
> struct list_head resource_list;
> - bool is_spi_i2c_slave = false;
> + bool is_serial_bus_slave = false;
>
> /* Macs use device properties in lieu of _CRS resources */
> if (x86_apple_machine &&
> (fwnode_property_present(&device->fwnode, "spiSclkPeriod") ||
> - fwnode_property_present(&device->fwnode, "i2cAddress")))
> + fwnode_property_present(&device->fwnode, "i2cAddress") ||
> + fwnode_property_present(&device->fwnode, "baud")))
> return true;
>
> INIT_LIST_HEAD(&resource_list);
> - acpi_dev_get_resources(device, &resource_list, acpi_check_spi_i2c_slave,
> - &is_spi_i2c_slave);
> + acpi_dev_get_resources(device, &resource_list,
> + acpi_check_serial_bus_slave,
> + &is_serial_bus_slave);
> acpi_dev_free_resource_list(&resource_list);
>
> - return is_spi_i2c_slave;
> + return is_serial_bus_slave;
> }
>
> void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
> @@ -1557,7 +1554,7 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
> acpi_bus_get_flags(device);
> device->flags.match_driver = false;
> device->flags.initialized = true;
> - device->flags.spi_i2c_slave = acpi_is_spi_i2c_slave(device);
> + device->flags.serial_bus_slave = acpi_is_serial_bus_slave(device);
> acpi_device_clear_enumerated(device);
> device_initialize(&device->dev);
> dev_set_uevent_suppress(&device->dev, true);
> @@ -1841,10 +1838,10 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used,
> static void acpi_default_enumeration(struct acpi_device *device)
> {
> /*
> - * Do not enumerate SPI/I2C slaves as they will be enumerated by their
> - * respective parents.
> + * Do not enumerate SPI/I2C/UART slaves as they will be enumerated by
> + * their respective parents.
> */
> - if (!device->flags.spi_i2c_slave) {
> + if (!device->flags.serial_bus_slave) {
> acpi_create_platform_device(device, NULL);
> acpi_device_set_enumerated(device);
> } else {
> @@ -1941,7 +1938,7 @@ static void acpi_bus_attach(struct acpi_device *device)
> return;
>
> device->flags.match_driver = true;
> - if (ret > 0 && !device->flags.spi_i2c_slave) {
> + if (ret > 0 && !device->flags.serial_bus_slave) {
> acpi_device_set_enumerated(device);
> goto ok;
> }
> @@ -1950,7 +1947,7 @@ static void acpi_bus_attach(struct acpi_device *device)
> if (ret < 0)
> return;
>
> - if (!device->pnp.type.platform_id && !device->flags.spi_i2c_slave)
> + if (!device->pnp.type.platform_id && !device->flags.serial_bus_slave)
> acpi_device_set_enumerated(device);
> else
> acpi_default_enumeration(device);
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index fa15052..f849be2 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -211,7 +211,7 @@ struct acpi_device_flags {
> u32 of_compatible_ok:1;
> u32 coherent_dma:1;
> u32 cca_seen:1;
> - u32 spi_i2c_slave:1;
> + u32 serial_bus_slave:1;
> u32 reserved:19;
> };
>
> --
> 2.7.4
>
Hi,
On Wed, Oct 04, 2017 at 10:51:29AM +0200, Fr?d?ric Danis wrote:
> This patch allows SerDev module to manage serial devices declared as
> attached to an UART in ACPI table.
>
> acpi_serdev_add_device() callback will only take into account entries
> without enumerated flag set. This flags is set for all entries during
> ACPI scan, except for SPI and I2C serial devices, and for UART with
> 2nd patch in the series.
>
> Signed-off-by: Fr?d?ric Danis <[email protected]>
Reviewed-by: Sebastian Reichel <[email protected]>
-- Sebastian
> ---
> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 94 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index c68fb3a..104777d 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -14,6 +14,7 @@
> * GNU General Public License for more details.
> */
>
> +#include <linux/acpi.h>
> #include <linux/errno.h>
> #include <linux/idr.h>
> #include <linux/kernel.h>
> @@ -49,13 +50,22 @@ static const struct device_type serdev_ctrl_type = {
>
> static int serdev_device_match(struct device *dev, struct device_driver *drv)
> {
> - /* TODO: ACPI and platform matching */
> + /* TODO: platform matching */
> + if (acpi_driver_match_device(dev, drv))
> + return 1;
> +
> return of_driver_match_device(dev, drv);
> }
>
> static int serdev_uevent(struct device *dev, struct kobj_uevent_env *env)
> {
> - /* TODO: ACPI and platform modalias */
> + int rc;
> +
> + /* TODO: platform modalias */
> + rc = acpi_device_uevent_modalias(dev, env);
> + if (rc != -ENODEV)
> + return rc;
> +
> return of_device_uevent_modalias(dev, env);
> }
>
> @@ -260,6 +270,12 @@ static int serdev_drv_remove(struct device *dev)
> static ssize_t modalias_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> + int len;
> +
> + len = acpi_device_modalias(dev, buf, PAGE_SIZE - 1);
> + if (len != -ENODEV)
> + return len;
> +
> return of_device_modalias(dev, buf, PAGE_SIZE);
> }
> DEVICE_ATTR_RO(modalias);
> @@ -385,6 +401,74 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
> return 0;
> }
>
> +#ifdef CONFIG_ACPI
> +static acpi_status acpi_serdev_register_device(struct serdev_controller *ctrl,
> + struct acpi_device *adev)
> +{
> + struct serdev_device *serdev = NULL;
> + int err;
> +
> + if (acpi_bus_get_status(adev) || !adev->status.present ||
> + acpi_device_enumerated(adev))
> + return AE_OK;
> +
> + serdev = serdev_device_alloc(ctrl);
> + if (!serdev) {
> + dev_err(&ctrl->dev, "failed to allocate Serial device for %s\n",
> + dev_name(&adev->dev));
> + return AE_NO_MEMORY;
> + }
> +
> + ACPI_COMPANION_SET(&serdev->dev, adev);
> + acpi_device_set_enumerated(adev);
> +
> + err = serdev_device_add(serdev);
> + if (err) {
> + dev_err(&serdev->dev,
> + "failure adding ACPI device. status %d\n", err);
> + serdev_device_put(serdev);
> + }
> +
> + return AE_OK;
> +}
> +
> +static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level,
> + void *data, void **return_value)
> +{
> + struct serdev_controller *ctrl = data;
> + struct acpi_device *adev;
> +
> + if (acpi_bus_get_device(handle, &adev))
> + return AE_OK;
> +
> + return acpi_serdev_register_device(ctrl, adev);
> +}
> +
> +static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
> +{
> + acpi_status status;
> + acpi_handle handle;
> +
> + handle = ACPI_HANDLE(ctrl->dev.parent);
> + if (!handle)
> + return -ENODEV;
> +
> + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> + acpi_serdev_add_device, NULL, ctrl, NULL);
> + if (ACPI_FAILURE(status)) {
> + dev_warn(&ctrl->dev, "failed to enumerate Serial slaves\n");
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +#else
> +static inline int acpi_serdev_register_devices(struct serdev_controller *ctlr)
> +{
> + return -ENODEV;
> +}
> +#endif /* CONFIG_ACPI */
> +
> /**
> * serdev_controller_add() - Add an serdev controller
> * @ctrl: controller to be registered.
> @@ -394,7 +478,7 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
> */
> int serdev_controller_add(struct serdev_controller *ctrl)
> {
> - int ret;
> + int ret_of, ret_acpi, ret;
>
> /* Can't register until after driver model init */
> if (WARN_ON(!is_registered))
> @@ -404,9 +488,14 @@ int serdev_controller_add(struct serdev_controller *ctrl)
> if (ret)
> return ret;
>
> - ret = of_serdev_register_devices(ctrl);
> - if (ret)
> + ret_of = of_serdev_register_devices(ctrl);
> + ret_acpi = acpi_serdev_register_devices(ctrl);
> + if (ret_of && ret_acpi) {
> + dev_dbg(&ctrl->dev, "serdev%d no devices registered: of:%d acpi:%d\n",
> + ctrl->nr, ret_of, ret_acpi);
> + ret = -ENODEV;
> goto out_dev_del;
> + }
>
> dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
> ctrl->nr, &ctrl->dev);
> --
> 2.7.4
>
On Wed, Oct 04, 2017 at 10:51:29AM +0200, Fr?d?ric Danis wrote:
> This patch allows SerDev module to manage serial devices declared as
> attached to an UART in ACPI table.
>
> acpi_serdev_add_device() callback will only take into account entries
> without enumerated flag set. This flags is set for all entries during
> ACPI scan, except for SPI and I2C serial devices, and for UART with
> 2nd patch in the series.
>
> Signed-off-by: Fr?d?ric Danis <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
On Sat, Oct 07, 2017 at 02:31:05AM +0200, Marcel Holtmann wrote:
> Hi Rafael,
>
> >>>> This patch allows SerDev module to manage serial devices declared as
> >>>> attached to an UART in ACPI table.
> >>>>
> >>>> acpi_serdev_add_device() callback will only take into account entries
> >>>> without enumerated flag set. This flags is set for all entries during
> >>>> ACPI scan, except for SPI and I2C serial devices, and for UART with
> >>>> 2nd patch in the series.
> >>>>
> >>>> Signed-off-by: Fr?d?ric Danis <[email protected]>
> >>>> ---
> >>>> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
> >>>> 1 file changed, 94 insertions(+), 5 deletions(-)
> >>>
> >>> Reviewed-by: Rob Herring <[email protected]>
> >>
> >> so how do we get these changes upstream? If the serdev changes alone do not cause any harm, I am almost proposing taking them through bluetooth-next tree and only leave only the ACPI change to the ACPI maintainers.
> >
> > That would be fine by me. I can take the serdev patch too, though.
>
> having both patches go via ACPI tree might be simplest. Greg, any objections from you?
None from me, let me go ack the serdev patch...
thanks,
greg k-h
On 10/7/17 4:36 AM, Frédéric Danis wrote:
> Hi Ian,
>
<snip>
>
> Which tty is used for btattach?
> Is this tty existing in /dev?
Yes - /dev/ttyS4
>
> I took a look at dmesg.txt and I did not find any trace related to serdev.
> On the T100, where ttyS4 is used for Bluetooth, I can see the following traces:
> [ 11.732347] serial serial0: allocated controller 0xffff880036229000 id 0
> [ 11.732470] serial serial0-0: device serial0-0 registered
> [ 11.732475] serial serial0: serdev0 registered: dev:ffff880036229000
> [ 11.732478] serial serial0: tty port ttyS4 registered
>
> If serdev registration failed you should at least get something like:
> serdev0 no devices registered: of:<> acpi:<>
>
> So, just to be sure, is SERIAL_DEV_BUS and SERIAL_DEV_CTRL_TTYPORT enabled in your kernel?
>
> Regards,
>
> Fred
>
Hi Fred,
My config actually had SERIAL_DEV_BUS set as 'm' so SERIAL_DEV_CTRL_TTYPORT was (forced) unset. If I set them both everything now works (see attached dmesg).
I've submitted a new patch to update drivers/tty/serdev/Kconfig for ACPI support as the current Kconfig for serdev is not really compatible when adding ACPI support. If SERIAL_DEV_BUS is set as 'm' then the ACPI support does not work when built as a module as it requires config SERIAL_DEV_CTRL_TTYPORT to be set as you stated. If you force SERIAL_DEV_CTRL_TTYPORT to be set then a compilation error results. So I suggest that if SERIAL_DEV_BUS is selected then serdev is compiled into the kernel so that config SERIAL_DEV_CTRL_TTYPORT can be selected and set if requiring ACPI support.
Regards,
Ian
Hi Rafael,
>>>> This patch allows SerDev module to manage serial devices declared as
>>>> attached to an UART in ACPI table.
>>>>
>>>> acpi_serdev_add_device() callback will only take into account entries
>>>> without enumerated flag set. This flags is set for all entries during
>>>> ACPI scan, except for SPI and I2C serial devices, and for UART with
>>>> 2nd patch in the series.
>>>>
>>>> Signed-off-by: Frédéric Danis <[email protected]>
>>>> ---
>>>> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
>>>> 1 file changed, 94 insertions(+), 5 deletions(-)
>>>
>>> Reviewed-by: Rob Herring <[email protected]>
>>
>> so how do we get these changes upstream? If the serdev changes alone do not cause any harm, I am almost proposing taking them through bluetooth-next tree and only leave only the ACPI change to the ACPI maintainers.
>
> That would be fine by me. I can take the serdev patch too, though.
having both patches go via ACPI tree might be simplest. Greg, any objections from you?
Regards
Marcel
On Fri, Oct 6, 2017 at 8:32 PM, Marcel Holtmann <[email protected]> wrote=
:
> Hi Rob,
>
>>> This patch allows SerDev module to manage serial devices declared as
>>> attached to an UART in ACPI table.
>>>
>>> acpi_serdev_add_device() callback will only take into account entries
>>> without enumerated flag set. This flags is set for all entries during
>>> ACPI scan, except for SPI and I2C serial devices, and for UART with
>>> 2nd patch in the series.
>>>
>>> Signed-off-by: Fr=C3=A9d=C3=A9ric Danis <[email protected]>
>>> ---
>>> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++=
++++---
>>> 1 file changed, 94 insertions(+), 5 deletions(-)
>>
>> Reviewed-by: Rob Herring <[email protected]>
>
> so how do we get these changes upstream? If the serdev changes alone do n=
ot cause any harm, I am almost proposing taking them through bluetooth-next=
tree and only leave only the ACPI change to the ACPI maintainers.
That would be fine by me. I can take the serdev patch too, though.
Thanks,
Rafael
Hi Rob,
>> This patch allows SerDev module to manage serial devices declared as
>> attached to an UART in ACPI table.
>>
>> acpi_serdev_add_device() callback will only take into account entries
>> without enumerated flag set. This flags is set for all entries during
>> ACPI scan, except for SPI and I2C serial devices, and for UART with
>> 2nd patch in the series.
>>
>> Signed-off-by: Frédéric Danis <[email protected]>
>> ---
>> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 94 insertions(+), 5 deletions(-)
>
> Reviewed-by: Rob Herring <[email protected]>
so how do we get these changes upstream? If the serdev changes alone do not cause any harm, I am almost proposing taking them through bluetooth-next tree and only leave only the ACPI change to the ACPI maintainers.
Greg, any thoughts?
Regards
Marcel
Hi Ian,
Le 06/10/2017 à 16:47, Ian W MORRISON a écrit :
> <snip>
>> It seems normal to me that BCM2EA4 is no more enumerated at ACPI level as
>> this is moved to serdev.
>> When removing 'if (ares->data.common_serial_bus.type !=
>> ACPI_RESOURCE_SERIAL_TYPE_UART)' you stop the serdev module finding the
>> Serial UART information. In this case it will not register the device and it
>> will fall back to previous behavior needing to use btattach to setup
>> Bluetooth.
>>
>> Can you share:
>> - btattach you are currently using,
>> - dmesg with with dynamic debug enabled for serdev and hci_uart modules
>> during boot (with Hans's patches, your MINIX Z83-4 patches and mine
>> patches).
>>
>> Regards,
>>
>> Fred
> Hi Fred,
>
> I've attached four (text) files:
>
> 1. btattach.txt - Details of the 'bluez' package that contains the
> 'btattach' I'm using.
> 2. dmesg.txt - 'dmesg' with with dynamic debug enabled for serdev and
> hci_uart modules. This doesn't seem to show much so have I provided
> what you wanted?
> 3. gitlog.txt - First few commits from the git log showing the kernel
> patches used to build the kernel (sent just for clarity).
> 4. working.txt - An extract from 'dmesg' when BCM2EA4 is enumerated
> from a kernel patched with the 'if' statement refered to above.
>
> Regards,
> Ian
Which tty is used for btattach?
Is this tty existing in /dev?
I took a look at dmesg.txt and I did not find any trace related to serdev.
On the T100, where ttyS4 is used for Bluetooth, I can see the following
traces:
[ 11.732347] serial serial0: allocated controller
0xffff880036229000 id 0
[ 11.732470] serial serial0-0: device serial0-0 registered
[ 11.732475] serial serial0: serdev0 registered: dev:ffff880036229000
[ 11.732478] serial serial0: tty port ttyS4 registered
If serdev registration failed you should at least get something like:
serdev0 no devices registered: of:<> acpi:<>
So, just to be sure, is SERIAL_DEV_BUS and SERIAL_DEV_CTRL_TTYPORT
enabled in your kernel?
Regards,
Fred
On 6 October 2017 at 19:16, Frédéric Danis <[email protected]> wrote:
> Hi Ian,
>
> Le 06/10/2017 à 09:33, Ian W MORRISON a écrit :
>
<snip>
>
> It seems normal to me that BCM2EA4 is no more enumerated at ACPI level as
> this is moved to serdev.
> When removing 'if (ares->data.common_serial_bus.type !=
> ACPI_RESOURCE_SERIAL_TYPE_UART)' you stop the serdev module finding the
> Serial UART information. In this case it will not register the device and it
> will fall back to previous behavior needing to use btattach to setup
> Bluetooth.
>
> Can you share:
> - btattach you are currently using,
> - dmesg with with dynamic debug enabled for serdev and hci_uart modules
> during boot (with Hans's patches, your MINIX Z83-4 patches and mine
> patches).
>
> Regards,
>
> Fred
Hi Fred,
I've attached four (text) files:
1. btattach.txt - Details of the 'bluez' package that contains the
'btattach' I'm using.
2. dmesg.txt - 'dmesg' with with dynamic debug enabled for serdev and
hci_uart modules. This doesn't seem to show much so have I provided
what you wanted?
3. gitlog.txt - First few commits from the git log showing the kernel
patches used to build the kernel (sent just for clarity).
4. working.txt - An extract from 'dmesg' when BCM2EA4 is enumerated
from a kernel patched with the 'if' statement refered to above.
Regards,
Ian
On Wed, Oct 4, 2017 at 3:51 AM, Fr=C3=A9d=C3=A9ric Danis
<[email protected]> wrote:
> This patch allows SerDev module to manage serial devices declared as
> attached to an UART in ACPI table.
>
> acpi_serdev_add_device() callback will only take into account entries
> without enumerated flag set. This flags is set for all entries during
> ACPI scan, except for SPI and I2C serial devices, and for UART with
> 2nd patch in the series.
>
> Signed-off-by: Fr=C3=A9d=C3=A9ric Danis <[email protected]>
> ---
> drivers/tty/serdev/core.c | 99 +++++++++++++++++++++++++++++++++++++++++=
+++---
> 1 file changed, 94 insertions(+), 5 deletions(-)
Reviewed-by: Rob Herring <[email protected]>
Hi Ian,
Le 06/10/2017 à 09:33, Ian W MORRISON a écrit :
> On 10/6/17 2:21 AM, Marcel Holtmann wrote:
>> Hi Fred,
>>
>>> Add ACPI support for serial attached devices.
>>>
>>> Currently, serial devices are not set as enumerated during ACPI scan for SPI
>>> or i2c buses (but not for UART). This should also be done for UART serial
>>> devices.
>>> I renamed *spi_i2c_slave* to *serial_bus_slave* to reflect this.
>>>
>>> For hci_bcm, this needs Hans de Goede's "Bluetooth: hci_bcm: Add (runtime)pm
>>> support to the serdev drv" patches to work.
>>>
>>> Tested on T100TA with Broadcom BCM2E39.
>>>
>>> Since RFC:
>>> - Add or reword commit messages
>>> - Rename *serial_slave* to *serial_bus_slave*
>>> - Add specific check for Apple in acpi_is_serial_bus_slave(), thanks to
>>> Lukas Wunner
>>> - Update comment in acpi_default_enumeration()
>>> - Remove patch 3 "Bluetooth: hci_bcm: Add ACPI serdev support for BCM2E39"
>>> in favor of patches from Hans de Goede
>> I already applied Hans’ patch series to bluetooth-next tree. And I have no objections or comments for these changes. They should be reviewed and acked by the appropriate maintainers as well, but from my side this looks good.
>>
>> Acked-by: Marcel Holtmann <[email protected]>
>>
>> What we should discuss on how we get them upstream since besides the Bluetooth subsystem, they cover ACPI and TTY subsystems.
>>
>> Regards
>>
>> Marcel
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
> Hi Marcel,
>
> I've tested these two patches along with the nine patches (plus revert patch) from Hans's series but cannot get bluetooth to work on some of my devices using these two patches as is, specifically on MINIX Z83-4 based PCs.
>
> In order for bluetooth to function on these devices I submitted the patch 'Bluetooth: BCM: Add support for MINIX Z83-4 based devices' as they require the trigger type IRQF_TRIGGER_FALLING. With Hans's new patch series I've had to modify the patch which I've resubmitted as version 2. However when I apply the second patch from Fred's series 'ACPI / scan: Fix enumeration for special UART devices' I have found the bluetooth device (BCM2EA4) is no longer enumerated. Specifically I've found that the removal of the check 'if (ares->data.common_serial_bus.type != ACPI_RESOURCE_SERIAL_TYPE_UART)' causes this failure and if I re-add the 'if' statement then the device is loaded. However a 'btattach' is still required for functioning bluetooth.
It seems normal to me that BCM2EA4 is no more enumerated at ACPI level
as this is moved to serdev.
When removing 'if (ares->data.common_serial_bus.type !=
ACPI_RESOURCE_SERIAL_TYPE_UART)' you stop the serdev module finding the
Serial UART information. In this case it will not register the device
and it will fall back to previous behavior needing to use btattach to
setup Bluetooth.
Can you share:
- btattach you are currently using,
- dmesg with with dynamic debug enabled for serdev and hci_uart modules
during boot (with Hans's patches, your MINIX Z83-4 patches and mine
patches).
Regards,
Fred
On 10/6/17 2:21 AM, Marcel Holtmann wrote:
> Hi Fred,
>
>> Add ACPI support for serial attached devices.
>>
>> Currently, serial devices are not set as enumerated during ACPI scan for SPI
>> or i2c buses (but not for UART). This should also be done for UART serial
>> devices.
>> I renamed *spi_i2c_slave* to *serial_bus_slave* to reflect this.
>>
>> For hci_bcm, this needs Hans de Goede's "Bluetooth: hci_bcm: Add (runtime)pm
>> support to the serdev drv" patches to work.
>>
>> Tested on T100TA with Broadcom BCM2E39.
>>
>> Since RFC:
>> - Add or reword commit messages
>> - Rename *serial_slave* to *serial_bus_slave*
>> - Add specific check for Apple in acpi_is_serial_bus_slave(), thanks to
>> Lukas Wunner
>> - Update comment in acpi_default_enumeration()
>> - Remove patch 3 "Bluetooth: hci_bcm: Add ACPI serdev support for BCM2E39"
>> in favor of patches from Hans de Goede
>
> I already applied Hans’ patch series to bluetooth-next tree. And I have no objections or comments for these changes. They should be reviewed and acked by the appropriate maintainers as well, but from my side this looks good.
>
> Acked-by: Marcel Holtmann <[email protected]>
>
> What we should discuss on how we get them upstream since besides the Bluetooth subsystem, they cover ACPI and TTY subsystems.
>
> Regards
>
> Marcel
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Hi Marcel,
I've tested these two patches along with the nine patches (plus revert patch) from Hans's series but cannot get bluetooth to work on some of my devices using these two patches as is, specifically on MINIX Z83-4 based PCs.
In order for bluetooth to function on these devices I submitted the patch 'Bluetooth: BCM: Add support for MINIX Z83-4 based devices' as they require the trigger type IRQF_TRIGGER_FALLING. With Hans's new patch series I've had to modify the patch which I've resubmitted as version 2. However when I apply the second patch from Fred's series 'ACPI / scan: Fix enumeration for special UART devices' I have found the bluetooth device (BCM2EA4) is no longer enumerated. Specifically I've found that the removal of the check 'if (ares->data.common_serial_bus.type != ACPI_RESOURCE_SERIAL_TYPE_UART)' causes this failure and if I re-add the 'if' statement then the device is loaded. However a 'btattach' is still required for functioning bluetooth.
In terms on applying the patches I have some questions.
Like Hans I too would like one of my patches to be applied to v4.14. A lot of people have been waiting to get bluetooth working on the MINIX Z83-4 family of devices so is it possible to get my original (resend) patch for bluetooth on MINIX Z83-4 based devices applied to v4.14? If so and assuming the nine patches from Hans go into v4.15 I can then send an additional patch to extend his second patch (Bluetooth: hci_bcm: Fix setting of irq trigger type) to delete the extra line '.driver_data = &acpi_active_low,'.
If the original (resend) version of my patch for MINIX Z83-4 cannot be applied to v4.14 then I would like to request that my second version of my patch for MINIX Z83-4 based devices be applied to bluetooth-next tree so that they are included in v4.15.
Finally I would like to request some help in addressing the issue caused by Fred's 'Fix enumeration for special UART devices' patch so as to prevent a regression.
Regards,
Ian
Hi Fred,
> Add ACPI support for serial attached devices.
>
> Currently, serial devices are not set as enumerated during ACPI scan for SPI
> or i2c buses (but not for UART). This should also be done for UART serial
> devices.
> I renamed *spi_i2c_slave* to *serial_bus_slave* to reflect this.
>
> For hci_bcm, this needs Hans de Goede's "Bluetooth: hci_bcm: Add (runtime)pm
> support to the serdev drv" patches to work.
>
> Tested on T100TA with Broadcom BCM2E39.
>
> Since RFC:
> - Add or reword commit messages
> - Rename *serial_slave* to *serial_bus_slave*
> - Add specific check for Apple in acpi_is_serial_bus_slave(), thanks to
> Lukas Wunner
> - Update comment in acpi_default_enumeration()
> - Remove patch 3 "Bluetooth: hci_bcm: Add ACPI serdev support for BCM2E39"
> in favor of patches from Hans de Goede
I already applied Hans’ patch series to bluetooth-next tree. And I have no objections or comments for these changes. They should be reviewed and acked by the appropriate maintainers as well, but from my side this looks good.
Acked-by: Marcel Holtmann <[email protected]>
What we should discuss on how we get them upstream since besides the Bluetooth subsystem, they cover ACPI and TTY subsystems.
Regards
Marcel
UART devices is expected to be enumerated by SerDev subsystem.
During ACPI scan, serial devices behind SPI, I2C or UART buses are not
enumerated, allowing them to be enumerated by their respective parents.
Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
devices on serial buses (SPI, I2C or UART).
On Macs an empty ResourceTemplate is returned for uart slaves.
Instead the device properties "baud", "parity", "dataBits", "stopBits" are
provided. Add a check for "baud" in acpi_is_serial_bus_slave().
Signed-off-by: Frédéric Danis <[email protected]>
---
drivers/acpi/scan.c | 37 +++++++++++++++++--------------------
include/acpi/acpi_bus.h | 2 +-
2 files changed, 18 insertions(+), 21 deletions(-)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 602f8ff..860b698 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1505,41 +1505,38 @@ static void acpi_init_coherency(struct acpi_device *adev)
adev->flags.coherent_dma = cca;
}
-static int acpi_check_spi_i2c_slave(struct acpi_resource *ares, void *data)
+static int acpi_check_serial_bus_slave(struct acpi_resource *ares, void *data)
{
- bool *is_spi_i2c_slave_p = data;
+ bool *is_serial_bus_slave_p = data;
if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
return 1;
- /*
- * devices that are connected to UART still need to be enumerated to
- * platform bus
- */
- if (ares->data.common_serial_bus.type != ACPI_RESOURCE_SERIAL_TYPE_UART)
- *is_spi_i2c_slave_p = true;
+ *is_serial_bus_slave_p = true;
/* no need to do more checking */
return -1;
}
-static bool acpi_is_spi_i2c_slave(struct acpi_device *device)
+static bool acpi_is_serial_bus_slave(struct acpi_device *device)
{
struct list_head resource_list;
- bool is_spi_i2c_slave = false;
+ bool is_serial_bus_slave = false;
/* Macs use device properties in lieu of _CRS resources */
if (x86_apple_machine &&
(fwnode_property_present(&device->fwnode, "spiSclkPeriod") ||
- fwnode_property_present(&device->fwnode, "i2cAddress")))
+ fwnode_property_present(&device->fwnode, "i2cAddress") ||
+ fwnode_property_present(&device->fwnode, "baud")))
return true;
INIT_LIST_HEAD(&resource_list);
- acpi_dev_get_resources(device, &resource_list, acpi_check_spi_i2c_slave,
- &is_spi_i2c_slave);
+ acpi_dev_get_resources(device, &resource_list,
+ acpi_check_serial_bus_slave,
+ &is_serial_bus_slave);
acpi_dev_free_resource_list(&resource_list);
- return is_spi_i2c_slave;
+ return is_serial_bus_slave;
}
void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
@@ -1557,7 +1554,7 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
acpi_bus_get_flags(device);
device->flags.match_driver = false;
device->flags.initialized = true;
- device->flags.spi_i2c_slave = acpi_is_spi_i2c_slave(device);
+ device->flags.serial_bus_slave = acpi_is_serial_bus_slave(device);
acpi_device_clear_enumerated(device);
device_initialize(&device->dev);
dev_set_uevent_suppress(&device->dev, true);
@@ -1841,10 +1838,10 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used,
static void acpi_default_enumeration(struct acpi_device *device)
{
/*
- * Do not enumerate SPI/I2C slaves as they will be enumerated by their
- * respective parents.
+ * Do not enumerate SPI/I2C/UART slaves as they will be enumerated by
+ * their respective parents.
*/
- if (!device->flags.spi_i2c_slave) {
+ if (!device->flags.serial_bus_slave) {
acpi_create_platform_device(device, NULL);
acpi_device_set_enumerated(device);
} else {
@@ -1941,7 +1938,7 @@ static void acpi_bus_attach(struct acpi_device *device)
return;
device->flags.match_driver = true;
- if (ret > 0 && !device->flags.spi_i2c_slave) {
+ if (ret > 0 && !device->flags.serial_bus_slave) {
acpi_device_set_enumerated(device);
goto ok;
}
@@ -1950,7 +1947,7 @@ static void acpi_bus_attach(struct acpi_device *device)
if (ret < 0)
return;
- if (!device->pnp.type.platform_id && !device->flags.spi_i2c_slave)
+ if (!device->pnp.type.platform_id && !device->flags.serial_bus_slave)
acpi_device_set_enumerated(device);
else
acpi_default_enumeration(device);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index fa15052..f849be2 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -211,7 +211,7 @@ struct acpi_device_flags {
u32 of_compatible_ok:1;
u32 coherent_dma:1;
u32 cca_seen:1;
- u32 spi_i2c_slave:1;
+ u32 serial_bus_slave:1;
u32 reserved:19;
};
--
2.7.4
This patch allows SerDev module to manage serial devices declared as
attached to an UART in ACPI table.
acpi_serdev_add_device() callback will only take into account entries
without enumerated flag set. This flags is set for all entries during
ACPI scan, except for SPI and I2C serial devices, and for UART with
2nd patch in the series.
Signed-off-by: Frédéric Danis <[email protected]>
---
drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 94 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index c68fb3a..104777d 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -14,6 +14,7 @@
* GNU General Public License for more details.
*/
+#include <linux/acpi.h>
#include <linux/errno.h>
#include <linux/idr.h>
#include <linux/kernel.h>
@@ -49,13 +50,22 @@ static const struct device_type serdev_ctrl_type = {
static int serdev_device_match(struct device *dev, struct device_driver *drv)
{
- /* TODO: ACPI and platform matching */
+ /* TODO: platform matching */
+ if (acpi_driver_match_device(dev, drv))
+ return 1;
+
return of_driver_match_device(dev, drv);
}
static int serdev_uevent(struct device *dev, struct kobj_uevent_env *env)
{
- /* TODO: ACPI and platform modalias */
+ int rc;
+
+ /* TODO: platform modalias */
+ rc = acpi_device_uevent_modalias(dev, env);
+ if (rc != -ENODEV)
+ return rc;
+
return of_device_uevent_modalias(dev, env);
}
@@ -260,6 +270,12 @@ static int serdev_drv_remove(struct device *dev)
static ssize_t modalias_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
+ int len;
+
+ len = acpi_device_modalias(dev, buf, PAGE_SIZE - 1);
+ if (len != -ENODEV)
+ return len;
+
return of_device_modalias(dev, buf, PAGE_SIZE);
}
DEVICE_ATTR_RO(modalias);
@@ -385,6 +401,74 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
return 0;
}
+#ifdef CONFIG_ACPI
+static acpi_status acpi_serdev_register_device(struct serdev_controller *ctrl,
+ struct acpi_device *adev)
+{
+ struct serdev_device *serdev = NULL;
+ int err;
+
+ if (acpi_bus_get_status(adev) || !adev->status.present ||
+ acpi_device_enumerated(adev))
+ return AE_OK;
+
+ serdev = serdev_device_alloc(ctrl);
+ if (!serdev) {
+ dev_err(&ctrl->dev, "failed to allocate Serial device for %s\n",
+ dev_name(&adev->dev));
+ return AE_NO_MEMORY;
+ }
+
+ ACPI_COMPANION_SET(&serdev->dev, adev);
+ acpi_device_set_enumerated(adev);
+
+ err = serdev_device_add(serdev);
+ if (err) {
+ dev_err(&serdev->dev,
+ "failure adding ACPI device. status %d\n", err);
+ serdev_device_put(serdev);
+ }
+
+ return AE_OK;
+}
+
+static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level,
+ void *data, void **return_value)
+{
+ struct serdev_controller *ctrl = data;
+ struct acpi_device *adev;
+
+ if (acpi_bus_get_device(handle, &adev))
+ return AE_OK;
+
+ return acpi_serdev_register_device(ctrl, adev);
+}
+
+static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
+{
+ acpi_status status;
+ acpi_handle handle;
+
+ handle = ACPI_HANDLE(ctrl->dev.parent);
+ if (!handle)
+ return -ENODEV;
+
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+ acpi_serdev_add_device, NULL, ctrl, NULL);
+ if (ACPI_FAILURE(status)) {
+ dev_warn(&ctrl->dev, "failed to enumerate Serial slaves\n");
+ return -ENODEV;
+ }
+
+ return 0;
+}
+#else
+static inline int acpi_serdev_register_devices(struct serdev_controller *ctlr)
+{
+ return -ENODEV;
+}
+#endif /* CONFIG_ACPI */
+
/**
* serdev_controller_add() - Add an serdev controller
* @ctrl: controller to be registered.
@@ -394,7 +478,7 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
*/
int serdev_controller_add(struct serdev_controller *ctrl)
{
- int ret;
+ int ret_of, ret_acpi, ret;
/* Can't register until after driver model init */
if (WARN_ON(!is_registered))
@@ -404,9 +488,14 @@ int serdev_controller_add(struct serdev_controller *ctrl)
if (ret)
return ret;
- ret = of_serdev_register_devices(ctrl);
- if (ret)
+ ret_of = of_serdev_register_devices(ctrl);
+ ret_acpi = acpi_serdev_register_devices(ctrl);
+ if (ret_of && ret_acpi) {
+ dev_dbg(&ctrl->dev, "serdev%d no devices registered: of:%d acpi:%d\n",
+ ctrl->nr, ret_of, ret_acpi);
+ ret = -ENODEV;
goto out_dev_del;
+ }
dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
ctrl->nr, &ctrl->dev);
--
2.7.4