From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932350AbdJ0Pwx (ORCPT ); Fri, 27 Oct 2017 11:52:53 -0400 Received: from esa3.dell-outbound.iphmx.com ([68.232.153.94]:11188 "EHLO esa3.dell-outbound.iphmx.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932321AbdJ0Pwu (ORCPT ); Fri, 27 Oct 2017 11:52:50 -0400 From: X-LoopCount0: from 10.166.132.169 X-IronPort-AV: E=Sophos;i="5.44,304,1505797200"; d="scan'208";a="166185515" X-DLP: DLP_GlobalPCIDSS To: CC: , , , Subject: RE: [PATCH] platform/x86: dell-uart-backlight: new backlight driver for DELL AIO Thread-Topic: [PATCH] platform/x86: dell-uart-backlight: new backlight driver for DELL AIO Thread-Index: AQHTTjAGVCe8H/0xXUqxSQLBpV5fmaL2QlgAgAEH84CAAI/osA== Date: Fri, 27 Oct 2017 15:52:48 +0000 Message-ID: <377b9a237f6448bba0f5a0b83c77d7bd@ausx13mpc124.AMER.DELL.COM> References: <1509004429-1596-1-git-send-email-acelan.kao@canonical.com> <08add166d5b34992ba167a878ca2a808@ausx13mpc124.AMER.DELL.COM> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.143.242.75] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id v9RFqvb5012564 > > > > 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,