From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.1 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C5AC5C76190 for ; Tue, 23 Jul 2019 22:06:39 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 98872218F0 for ; Tue, 23 Jul 2019 22:06:39 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=lunn.ch header.i=@lunn.ch header.b="UDfC2FY/" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2389302AbfGWWGf (ORCPT ); Tue, 23 Jul 2019 18:06:35 -0400 Received: from vps0.lunn.ch ([185.16.172.187]:60540 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2389193AbfGWWGf (ORCPT ); Tue, 23 Jul 2019 18:06:35 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lunn.ch; s=20171124; h=In-Reply-To:Content-Type:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=E/LUBzLCJzpcb95mBekwKy8gZoYu7sAEuAiBQdoSSiA=; b=UDfC2FY/7QnEVtXmemf42irTpU wU+REj/7meBbBTKOigPdM0rK6mIj3oswO4Bdc7VYYvfx5qdrK5OpFJyfuxReqODGFI0trlm5EdvGr m42/sfxUKYVUo1HAOnCi5EjUPA9+2Ww25yW4X+SG4wGN1XWbm/QhPu4BR/KHE+CwXnPM=; Received: from andrew by vps0.lunn.ch with local (Exim 4.89) (envelope-from ) id 1hq2vU-0003aB-QJ; Wed, 24 Jul 2019 00:06:28 +0200 Date: Wed, 24 Jul 2019 00:06:28 +0200 From: Andrew Lunn To: Evgeny Kolesnikov Cc: Mark Rutland , Jason Cooper , linux-pm@vger.kernel.org, Gregory Clement , Sebastian Reichel , linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Rob Herring , linux-arm-kernel@lists.infradead.org, Sebastian Hesselbarth Subject: Re: [PATCH 0/5] Add support for WD MyCloud EX2 Ultra (+ versatile UART-based restart/poweroff drivers) Message-ID: <20190723220628.GA13517@lunn.ch> References: <20190723015631.GI8972@lunn.ch> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org On Tue, Jul 23, 2019 at 07:48:49PM +0200, Evgeny Kolesnikov wrote: > On 23/07/2019 03:56, Andrew Lunn wrote: > >On Mon, Jul 22, 2019 at 09:53:00PM +0200, Evgeny Kolesnikov wrote: > >> > >>The difference between uart-poweroff and qnap-poweroff is small, but important: > >>uart-poweroff is able to send to an MCU a command of arbitrary length, and the command > >>itself is defined in a DTS file for a specific device/board, thus making this driver > >>applicable to wider range of devices. > > > >There is a lot of replicated code here, and in the original > >qnap-poweroff.c driver. Please consolidate it by extending the current > >driver. It should be easy to add a new compatible string, and turn > >power_off_cfg.cmd into an array. > > Hi, Andrew. > > I've considered extending qnap driver, but I have some doubts about this > approach. > > First of all there is only a poweroff counterpart. As there is no > qnap-restart driver, what should I do with uart-restart? Is it OK to have > xxx-restart-poweroff driver (never saw anything like that)? Hi Evgeny There are a few options. You can refactor all the code into a library and small drivers which wrap around the library. Or you can make the driver handle both, using the compatible string to determine which it should do. > While I can add cmd as a parameter to qnap driver (having it converted > into an array) it should be optional as original qnap relies on two > hardcoded values for its devices. That is not what i meant. You can make the current code more generic by changing the single byte in power_off_cfg to an array. DT should describe the hardware, not bytes you poke into registers. So it is perfectly valid to have the bytes hard coded in the driver. Andrew From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1C09DC76186 for ; Tue, 23 Jul 2019 22:06:46 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E427D2070B for ; Tue, 23 Jul 2019 22:06:45 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="TY+7OWo5"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=lunn.ch header.i=@lunn.ch header.b="UDfC2FY/" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E427D2070B Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=lunn.ch Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=AQcfxvOE95vVTk++fIOmDB+u9GnpbrlBs6ntoOj3VKY=; b=TY+7OWo53BWPzD Iy2UVomfTK99N12Zrl55zto/dadx+IZp1XDOWbGkhQyPfhbFkf5xQWLjwxGW9s0vGxtnaSR9kihr/ DK75T8khhvGX4nOSUB0+tASKL/go0evxgCYyE+nFKUKI7Y2qdg5lV2ImUGwMuPHPPg4NpLUE8RCvb saRRc8ohYGY3IUFeEzaub5XvzrSTLETpaaiBFzM0RhN+0logX9qEp69kMmDeU2+hPWpIbbaqAdvT6 MKzJCtHtG0sJ0q+HRXvdcy/5LMABoofR768CWAqbhVOwrxKHP/MJQVUZsjNgslaDdhS2fRUYAivhq AbToOqMTMq6TZnUZrUew==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1hq2vh-00081I-0L; Tue, 23 Jul 2019 22:06:41 +0000 Received: from vps0.lunn.ch ([185.16.172.187]) by bombadil.infradead.org with esmtps (Exim 4.92 #3 (Red Hat Linux)) id 1hq2vd-0007zX-7F for linux-arm-kernel@lists.infradead.org; Tue, 23 Jul 2019 22:06:38 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lunn.ch; s=20171124; h=In-Reply-To:Content-Type:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=E/LUBzLCJzpcb95mBekwKy8gZoYu7sAEuAiBQdoSSiA=; b=UDfC2FY/7QnEVtXmemf42irTpU wU+REj/7meBbBTKOigPdM0rK6mIj3oswO4Bdc7VYYvfx5qdrK5OpFJyfuxReqODGFI0trlm5EdvGr m42/sfxUKYVUo1HAOnCi5EjUPA9+2Ww25yW4X+SG4wGN1XWbm/QhPu4BR/KHE+CwXnPM=; Received: from andrew by vps0.lunn.ch with local (Exim 4.89) (envelope-from ) id 1hq2vU-0003aB-QJ; Wed, 24 Jul 2019 00:06:28 +0200 Date: Wed, 24 Jul 2019 00:06:28 +0200 From: Andrew Lunn To: Evgeny Kolesnikov Subject: Re: [PATCH 0/5] Add support for WD MyCloud EX2 Ultra (+ versatile UART-based restart/poweroff drivers) Message-ID: <20190723220628.GA13517@lunn.ch> References: <20190723015631.GI8972@lunn.ch> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190723_150637_415185_7284718F X-CRM114-Status: GOOD ( 19.73 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , devicetree@vger.kernel.org, Jason Cooper , linux-pm@vger.kernel.org, Gregory Clement , linux-kernel@vger.kernel.org, Sebastian Reichel , Rob Herring , linux-arm-kernel@lists.infradead.org, Sebastian Hesselbarth Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Jul 23, 2019 at 07:48:49PM +0200, Evgeny Kolesnikov wrote: > On 23/07/2019 03:56, Andrew Lunn wrote: > >On Mon, Jul 22, 2019 at 09:53:00PM +0200, Evgeny Kolesnikov wrote: > >> > >>The difference between uart-poweroff and qnap-poweroff is small, but important: > >>uart-poweroff is able to send to an MCU a command of arbitrary length, and the command > >>itself is defined in a DTS file for a specific device/board, thus making this driver > >>applicable to wider range of devices. > > > >There is a lot of replicated code here, and in the original > >qnap-poweroff.c driver. Please consolidate it by extending the current > >driver. It should be easy to add a new compatible string, and turn > >power_off_cfg.cmd into an array. > > Hi, Andrew. > > I've considered extending qnap driver, but I have some doubts about this > approach. > > First of all there is only a poweroff counterpart. As there is no > qnap-restart driver, what should I do with uart-restart? Is it OK to have > xxx-restart-poweroff driver (never saw anything like that)? Hi Evgeny There are a few options. You can refactor all the code into a library and small drivers which wrap around the library. Or you can make the driver handle both, using the compatible string to determine which it should do. > While I can add cmd as a parameter to qnap driver (having it converted > into an array) it should be optional as original qnap relies on two > hardcoded values for its devices. That is not what i meant. You can make the current code more generic by changing the single byte in power_off_cfg to an array. DT should describe the hardware, not bytes you poke into registers. So it is perfectly valid to have the bytes hard coded in the driver. Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel