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 <frederic.danis.oss@xxxxxxxxx>
> >> ---
> >> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
