linux-kernel - Re: [PATCH v12 0/6] Driver for at91 usart in spi mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a143edd3c0de2ba98b64e642b4384edf4323be2b.camel@upb.ro>
Date:   Wed, 12 Sep 2018 22:36:40 +0300
From:   Radu Pirea <radu_nicolae.pirea@....ro>
To:     Lee Jones <lee.jones@...aro.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc:     Geert Uytterhoeven <geert@...ux-m68k.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Mark Brown <broonie@...nel.org>, Jiri Slaby <jslaby@...e.com>,
        Richard Genoud <richard.genoud@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Arnd Bergmann <arnd@...db.de>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        linux-spi <linux-spi@...r.kernel.org>
Subject: Re: [PATCH v12 0/6] Driver for at91 usart in spi mode

On Wed, 2018-09-12 at 14:12 +0100, Lee Jones wrote:
> On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> 
> > On 12/09/2018 12:43:52+0100, Lee Jones wrote:
> > > > > But ... we can't have it both ways.  *Either* it's a true
> > > > > MFD, in
> > > > > which case it can/should have 2 separate compatible strings
> > > > > which can
> > > > > be specified directly from the DT.  *Or* it's not an MFD.  In
> > > > > the
> > > > > latter case, which I think we're all agreeing on (else we'd
> > > > > have 2
> > > > > compatible strings), MFD is not the place to handle this (my
> > > > > original
> > > > > point).
> > > > > 
> > > > 
> > > > If that is what bothers you, then let's move it out of mfd.
> > > 
> > > As I've already mentioned.  I don't just want it moved out of MFD
> > > and
> > > shoved somewhere else.  My aim is to fix this properly.
> > > 
> > 
> > If it is out of MFD, then I'm not sure why you would care too much
> > about
> > it as you won't be maintaining that code. And I still this what was
> > done
> > was correct but I'm open to test what you suggest.
> 
> I care for the kernel in general, not just the areas I'm responsible
> for.  I guess I'm just that kinda guy! ;)

Well, Lee, like you, I think this driver should not be a MFD driver,
but Alex has a good point of view. 

> 
> > > > > So ... this is a USART device which can do SPI, right?
> > > > > 
> > > > > My current thinking is that; as this is a USART device first
> > > > > &
> > > > > foremost, the USART should be probed in the first instance
> > > > > regardless,
> > > > > then if SPI mode is specified it (the USART driver) registers
> > > > > the SPI
> > > > > platform driver (as MFD does currently) and exits gracefully,
> > > > > allowing
> > > > > the SPI driver to take over.
> > > > > 
> > > > > Spanner in the works: is it physically possible to change the
> > > > > mode at
> > > > > run-time?  :s
> > > > 
> > > > Yes it is possible but on Linux that will not happen without
> > > > probing
> > > > the drivers again.
> > > 
> > > Not sure I understand what you mean.
> > 
> > I was just commenting on changing the mode at runtime.
> 
> Oh I see.  My question was relating to whether the H/W is physically
> capable of changing modes on-the-fly, rather than how Linux would
> handle that.  If this is something we'd wish to support, then it
> would
> have to be a single driver, which is why I was asking.  By separating
> the drivers this way, we are blocking that as a
> possibility.  Although
> I guess the OP has already thought about that and made the decision
> not to support it.

Is possible to change modes on-the-fly, but you have no reason to do
that. On the PCB you will have a SPI slave or a serial console :)
Anyway, the current form of the driver, and through this I want to say
"this ugly hack", allows the user to switch from serial to SPI mode by
adding only one property to the device tree node of USART. If the
driver were in his first form, a simple SPI driver, how you will make a
dtsi file for an IP like this? You will add two nodes for the same IP
in dtsi and will take care to enable correct node in dts?
I think this driver is only a tradeoff between having an ugly hack in
kernel or having an messy device tree.

> 
> > > I'm suggesting that you use the same platform_* interfaces MFD
> > > uses to
> > > register the SPI driver if SPI mode has been selected.  Only do
> > > so
> > > from the appropriate driver i.e. USART.
> > 
> > Yeah, I understood that but I didn't comment because I'm not sure
> > this
> > will work yet.
> 
> Other drivers already do this.

Can you give me an example please?
I am open to suggestions.

Sorry for that acked-by. There was a lot of "reviewed-by", "acked-by",
etc in a single version and I messed up :).