Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <562c1505-d3bc-6422-9598-15c399e6fbba@kernel.org>
Date:   Thu, 24 Nov 2022 07:53:50 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.1
Content-Language: en-US
To:     Tony Lindgren <tony@atomide.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Andy Shevchenko <andriy.shevchenko@intel.com>,
        =?UTF-8?Q?Ilpo_J=c3=a4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        Jiri Slaby <jirislaby@kernel.org>,
        Johan Hovold <johan@kernel.org>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        linux-serial@vger.kernel.org, linux-omap@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220615062455.15490-1-tony@atomide.com>
 <Yrmfr3GfXYhclKXA@kroah.com> <Yrm1HaUtjTMcSIE+@atomide.com>
From:   Jiri Slaby <jirislaby@kernel.org>
Subject: Re: [PATCH v2 1/1] serial: core: Start managing serial controllers to
 enable runtime PM
In-Reply-To: <Yrm1HaUtjTMcSIE+@atomide.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

I am returning to v2, as I managed to read only v3 and only now. But 
here was already the point below.

On 27. 06. 22, 15:48, Tony Lindgren wrote:
>>> Considering the above, let's improve the serial core layer so we can
>>> manage the serial port controllers better. Let's register the controllers
>>> with the serial core layer in addition to the serial ports.
>>
>> Why can't controllers be a device as well?
> 
> The controllers are devices already probed by the serial port drivers.
> What's missing is mapping the ports (as devices based on the comments
> above) to the controller devices. I don't think we need another struct
> device for the serial controller in addition to the serial port driver
> device and it's child port devices.

To be honest, I don't like the patch (even v3). We have uart_state which 
I already hate and now we have another structure holding *some* other 
info about a serial device (apart from uart_port). It's mess already and 
hard to follow, esp. to newcomers.

AFAIU, what Greg suggests would be:

PCI/platform/acpi/whatever struct dev
   -> serial controller 1 struct dev
      -> serial port 1 struct dev (tty_port instance exists for this)
      -> serial port 2 struct dev (tty_port instance exists for this)
      -> ...
   -> serial controller 2 struct dev
      -> serial port 1 struct dev (tty_port instance exists for this)
      -> serial port 2 struct dev (tty_port instance exists for this)
      -> ...

And you are objecting that mostly (or in all cases?), there will never 
be "serial controller 2"?

But given your description, I believe you need it anyway -- side note: 
does really the PM layer/or you need it or would you be fine with 
"serial port N" dev children? But provided you don't have the 
controller, you work around it by struct serial_controller. So what's 
actually the point of the workaround instead of sticking to proper 
driver model? With the workaround you seem you have to implement all the 
binding, lookup and such yourself anyway. And that renders the serial 
even worse :P. Let's do the reverse instead.

The only thing I am not sure about, whether tty_port should be struct 
dev too -- and if it should have serial port 1 as a parent. But likely 
so. And then with pure tty (i.e. tty_driver's, not uart_driver's), it 
would have PCI/platform/acpi/whatever as a parent directly.

In sum, the above structure makes perfect sense to me. There has only 
been noone to do the real work yet. And having tty_port was a hard 
prerequisite for this to happen. And that happened long time ago. All 
this would need a lot of work initially¹⁾, but it paid off a lot in long 
term.

¹⁾I know what I am writing about -- I converted HID. After all, the core 
was only 1000 lines patch (cf 85cdaf524b7d) + patches to convert all the 
drivers incrementally (like 8c19a51591).

thanks,
-- 
js
suse labs