RE: [PATCH] platform/x86: dell-uart-backlight: new backlight driver for DELL AIO

[<Mario.Limonciello@dell.com>]

> >
> > Looking at the comment for serial8250_get_port, it sounds like the serial driver
> > doesn't really want other drivers using this function outside of suspend/resume
> callback.
> >
> > /**
> >  * serial8250_get_port - retrieve struct uart_8250_port
> >  * @line: serial line number
> >  *
> >  * This function retrieves struct uart_8250_port for the specific line.
> >  * This struct *must* *not* be used to perform a 8250 or serial core operation
> >  * which is not accessible otherwise. Its only purpose is to make the struct
> >  * accessible to the runtime-pm callbacks for context suspend/restore.
> >  * The lock assumption made here is none because runtime-pm suspend/resume
> >  * callbacks should not be invoked if there is any operation performed on the
> >  * port.
> >  */
> > http://elixir.free-
> electrons.com/linux/v4.13/source/drivers/tty/serial/8250/8250_core.c#L409
> >
> > If serial core doesn't provide an API, have you already discussed with serial
> maintainer
> > to review your approach?  It might make sense to work with serial maintainer to
> provide
> > an API for this type of usecase if it doesn't exist.
> Yes, I know the pointer returned from serial8250_get_port() becomes
> unavailable after S3.
> So, I provide a suspend/resume function try to workaround it.
> But just like the comment described, it's risky to perform serial
> operations by using the structure,
> and there is a new interface to deal with this kind of requirement, serdev.
> But serdev is introduced after v4.12, to make it work before serdev, I
> can't find other good solutions for it.

Right, I know you have a need for this to work on older kernel too, so I think
this interface as you did it makes sense on older kernel, but please do use serdev
on newer kernel and as this is upstreamed.

> 
> >> +     /* unregister acpi backlight interface */
> >> +     acpi_video_set_dmi_backlight_type(acpi_backlight_vendor);
> >
> > Since you change this when the driver is loaded, you should cache the old
> > value and restore it when the driver is unloaded too.
> 
> In this case, acpi backlight doesn't work, all the backlight
> operations would go through UART commands.
> So, it safe to remove it.

OK.
I think it would be good to include a command about this so that folks who
look at this later understand this.

Thanks,