[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190723220628.GA13517@lunn.ch>
Date: Wed, 24 Jul 2019 00:06:28 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Evgeny Kolesnikov <evgenyz@...il.com>
Cc: Mark Rutland <mark.rutland@....com>,
Jason Cooper <jason@...edaemon.net>, linux-pm@...r.kernel.org,
Gregory Clement <gregory.clement@...tlin.com>,
Sebastian Reichel <sre@...nel.org>,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
Rob Herring <robh+dt@...nel.org>,
linux-arm-kernel@...ts.infradead.org,
Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
Subject: Re: [PATCH 0/5] Add support for WD MyCloud EX2 Ultra (+ versatile
UART-based restart/poweroff drivers)
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
Powered by blists - more mailing lists