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

2018-10-09 Thread Lee Jones
On Wed, 12 Sep 2018, Radu Pirea wrote:

> 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?

Sorry for the delay, I have been on vacation.

Grep for 'platform_device_add' in drivers/

> 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 :).
> 

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

2018-10-09 Thread Lee Jones
On Wed, 12 Sep 2018, Radu Pirea wrote:

> 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?

Sorry for the delay, I have been on vacation.

Grep for 'platform_device_add' in drivers/

> 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 :).
> 

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

2018-09-12 Thread Radu Pirea
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 :).



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

2018-09-12 Thread Radu Pirea
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 :).



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

2018-09-12 Thread Lee Jones
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! ;)

> > > > 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.

> > 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.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

2018-09-12 Thread Lee Jones
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! ;)

> > > > 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.

> > 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.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

2018-09-12 Thread Alexandre Belloni
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.

> > > 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.

> 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.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com


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

2018-09-12 Thread Alexandre Belloni
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.

> > > 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.

> 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.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com


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

2018-09-12 Thread Lee Jones
On Wed, 12 Sep 2018, Alexandre Belloni wrote:

> On 12/09/2018 11:54:07+0100, Lee Jones wrote:
> > On Wed, 12 Sep 2018, Geert Uytterhoeven wrote:
> > > On Wed, Sep 12, 2018 at 10:41 AM Lee Jones  wrote:
> > > > On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> > > > > On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > > > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > > > > > >
> > > > > > > > USART doc starting p572, registers p621.
> > > > > >
> > > > > > After looking at the datasheet, I don't see any reason why one of 
> > > > > > the
> > > > > > two drivers can't be selected using different compatible strings.
> > > > >
> > > > > Because there is only one IP and we don't use the device tree to 
> > > > > selecet
> > > > > linux specific drivers.
> > > >
> > > > We do it all the time.  There are loads of MFDs (def: same IP, with
> > > > different functions) which have separate compatibles for their various
> > > > functions.  If you wish this IP to operate as an SPI controller, it
> > > > should have an SPI compatible, if you wish it to operate as a U(S)ART,
> > > > then it should have a UART compatible.  It's what we do for most of
> > > > the other MFDs in the kernel.
> > > 
> > > There is a big difference: MFD functions are(more or less) independent
> > > functions, which can be used at the same time. It makes perfect sense for 
> > > a
> > > single IP block that has both SPI and UART interfaces, that can be used at
> > > the same time.
> > > 
> > > In this case, there is a single piece of hardware that can perform
> > > different functions, but not at the same time. Performing a different
> > > function means configuring the hardware for that function, hence using a
> > > different driver (from a different subsystem).
> > 
> > Yes, I can see that PoV.
> > 
> > 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.

> > 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'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.

> I think DT overlays will be the only possible use
> case because on SPI, you'd still have to provide nodes for the connected
> SPI devices.

Since SPI is a function of the USART you should describe is as such
via a child node.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

2018-09-12 Thread Lee Jones
On Wed, 12 Sep 2018, Alexandre Belloni wrote:

> On 12/09/2018 11:54:07+0100, Lee Jones wrote:
> > On Wed, 12 Sep 2018, Geert Uytterhoeven wrote:
> > > On Wed, Sep 12, 2018 at 10:41 AM Lee Jones  wrote:
> > > > On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> > > > > On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > > > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > > > > > >
> > > > > > > > USART doc starting p572, registers p621.
> > > > > >
> > > > > > After looking at the datasheet, I don't see any reason why one of 
> > > > > > the
> > > > > > two drivers can't be selected using different compatible strings.
> > > > >
> > > > > Because there is only one IP and we don't use the device tree to 
> > > > > selecet
> > > > > linux specific drivers.
> > > >
> > > > We do it all the time.  There are loads of MFDs (def: same IP, with
> > > > different functions) which have separate compatibles for their various
> > > > functions.  If you wish this IP to operate as an SPI controller, it
> > > > should have an SPI compatible, if you wish it to operate as a U(S)ART,
> > > > then it should have a UART compatible.  It's what we do for most of
> > > > the other MFDs in the kernel.
> > > 
> > > There is a big difference: MFD functions are(more or less) independent
> > > functions, which can be used at the same time. It makes perfect sense for 
> > > a
> > > single IP block that has both SPI and UART interfaces, that can be used at
> > > the same time.
> > > 
> > > In this case, there is a single piece of hardware that can perform
> > > different functions, but not at the same time. Performing a different
> > > function means configuring the hardware for that function, hence using a
> > > different driver (from a different subsystem).
> > 
> > Yes, I can see that PoV.
> > 
> > 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.

> > 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'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.

> I think DT overlays will be the only possible use
> case because on SPI, you'd still have to provide nodes for the connected
> SPI devices.

Since SPI is a function of the USART you should describe is as such
via a child node.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

2018-09-12 Thread Alexandre Belloni
On 12/09/2018 11:54:07+0100, Lee Jones wrote:
> On Wed, 12 Sep 2018, Geert Uytterhoeven wrote:
> > On Wed, Sep 12, 2018 at 10:41 AM Lee Jones  wrote:
> > > On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> > > > On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > > > > >
> > > > > > > USART doc starting p572, registers p621.
> > > > >
> > > > > After looking at the datasheet, I don't see any reason why one of the
> > > > > two drivers can't be selected using different compatible strings.
> > > >
> > > > Because there is only one IP and we don't use the device tree to selecet
> > > > linux specific drivers.
> > >
> > > We do it all the time.  There are loads of MFDs (def: same IP, with
> > > different functions) which have separate compatibles for their various
> > > functions.  If you wish this IP to operate as an SPI controller, it
> > > should have an SPI compatible, if you wish it to operate as a U(S)ART,
> > > then it should have a UART compatible.  It's what we do for most of
> > > the other MFDs in the kernel.
> > 
> > There is a big difference: MFD functions are(more or less) independent
> > functions, which can be used at the same time. It makes perfect sense for a
> > single IP block that has both SPI and UART interfaces, that can be used at
> > the same time.
> > 
> > In this case, there is a single piece of hardware that can perform
> > different functions, but not at the same time. Performing a different
> > function means configuring the hardware for that function, hence using a
> > different driver (from a different subsystem).
> 
> Yes, I can see that PoV.
> 
> 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.

> 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. I think DT overlays will be the only possible use
case because on SPI, you'd still have to provide nodes for the connected
SPI devices.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com


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

2018-09-12 Thread Alexandre Belloni
On 12/09/2018 11:54:07+0100, Lee Jones wrote:
> On Wed, 12 Sep 2018, Geert Uytterhoeven wrote:
> > On Wed, Sep 12, 2018 at 10:41 AM Lee Jones  wrote:
> > > On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> > > > On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > > > > >
> > > > > > > USART doc starting p572, registers p621.
> > > > >
> > > > > After looking at the datasheet, I don't see any reason why one of the
> > > > > two drivers can't be selected using different compatible strings.
> > > >
> > > > Because there is only one IP and we don't use the device tree to selecet
> > > > linux specific drivers.
> > >
> > > We do it all the time.  There are loads of MFDs (def: same IP, with
> > > different functions) which have separate compatibles for their various
> > > functions.  If you wish this IP to operate as an SPI controller, it
> > > should have an SPI compatible, if you wish it to operate as a U(S)ART,
> > > then it should have a UART compatible.  It's what we do for most of
> > > the other MFDs in the kernel.
> > 
> > There is a big difference: MFD functions are(more or less) independent
> > functions, which can be used at the same time. It makes perfect sense for a
> > single IP block that has both SPI and UART interfaces, that can be used at
> > the same time.
> > 
> > In this case, there is a single piece of hardware that can perform
> > different functions, but not at the same time. Performing a different
> > function means configuring the hardware for that function, hence using a
> > different driver (from a different subsystem).
> 
> Yes, I can see that PoV.
> 
> 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.

> 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. I think DT overlays will be the only possible use
case because on SPI, you'd still have to provide nodes for the connected
SPI devices.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com


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

2018-09-12 Thread Lee Jones
On Wed, 12 Sep 2018, Geert Uytterhoeven wrote:
> On Wed, Sep 12, 2018 at 10:41 AM Lee Jones  wrote:
> > On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> > > On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > > > >
> > > > > > USART doc starting p572, registers p621.
> > > >
> > > > After looking at the datasheet, I don't see any reason why one of the
> > > > two drivers can't be selected using different compatible strings.
> > >
> > > Because there is only one IP and we don't use the device tree to selecet
> > > linux specific drivers.
> >
> > We do it all the time.  There are loads of MFDs (def: same IP, with
> > different functions) which have separate compatibles for their various
> > functions.  If you wish this IP to operate as an SPI controller, it
> > should have an SPI compatible, if you wish it to operate as a U(S)ART,
> > then it should have a UART compatible.  It's what we do for most of
> > the other MFDs in the kernel.
> 
> There is a big difference: MFD functions are(more or less) independent
> functions, which can be used at the same time. It makes perfect sense for a
> single IP block that has both SPI and UART interfaces, that can be used at
> the same time.
> 
> In this case, there is a single piece of hardware that can perform
> different functions, but not at the same time. Performing a different
> function means configuring the hardware for that function, hence using a
> different driver (from a different subsystem).

Yes, I can see that PoV.

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).

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

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

2018-09-12 Thread Lee Jones
On Wed, 12 Sep 2018, Geert Uytterhoeven wrote:
> On Wed, Sep 12, 2018 at 10:41 AM Lee Jones  wrote:
> > On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> > > On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > > > >
> > > > > > USART doc starting p572, registers p621.
> > > >
> > > > After looking at the datasheet, I don't see any reason why one of the
> > > > two drivers can't be selected using different compatible strings.
> > >
> > > Because there is only one IP and we don't use the device tree to selecet
> > > linux specific drivers.
> >
> > We do it all the time.  There are loads of MFDs (def: same IP, with
> > different functions) which have separate compatibles for their various
> > functions.  If you wish this IP to operate as an SPI controller, it
> > should have an SPI compatible, if you wish it to operate as a U(S)ART,
> > then it should have a UART compatible.  It's what we do for most of
> > the other MFDs in the kernel.
> 
> There is a big difference: MFD functions are(more or less) independent
> functions, which can be used at the same time. It makes perfect sense for a
> single IP block that has both SPI and UART interfaces, that can be used at
> the same time.
> 
> In this case, there is a single piece of hardware that can perform
> different functions, but not at the same time. Performing a different
> function means configuring the hardware for that function, hence using a
> different driver (from a different subsystem).

Yes, I can see that PoV.

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).

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

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

2018-09-12 Thread Geert Uytterhoeven
Hi Lee,

On Wed, Sep 12, 2018 at 10:41 AM Lee Jones  wrote:
> On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> > On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > > >
> > > > > USART doc starting p572, registers p621.
> > >
> > > After looking at the datasheet, I don't see any reason why one of the
> > > two drivers can't be selected using different compatible strings.
> >
> > Because there is only one IP and we don't use the device tree to selecet
> > linux specific drivers.
>
> We do it all the time.  There are loads of MFDs (def: same IP, with
> different functions) which have separate compatibles for their various
> functions.  If you wish this IP to operate as an SPI controller, it
> should have an SPI compatible, if you wish it to operate as a U(S)ART,
> then it should have a UART compatible.  It's what we do for most of
> the other MFDs in the kernel.

There is a big difference: MFD functions are(more or less) independent
functions, which can be used at the same time. It makes perfect sense for a
single IP block that has both SPI and UART interfaces, that can be used at
the same time.

In this case, there is a single piece of hardware that can perform
different functions, but not at the same time. Performing a different
function means configuring the hardware for that function, hence using a
different driver (from a different subsystem).

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds


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

2018-09-12 Thread Geert Uytterhoeven
Hi Lee,

On Wed, Sep 12, 2018 at 10:41 AM Lee Jones  wrote:
> On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> > On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > > >
> > > > > USART doc starting p572, registers p621.
> > >
> > > After looking at the datasheet, I don't see any reason why one of the
> > > two drivers can't be selected using different compatible strings.
> >
> > Because there is only one IP and we don't use the device tree to selecet
> > linux specific drivers.
>
> We do it all the time.  There are loads of MFDs (def: same IP, with
> different functions) which have separate compatibles for their various
> functions.  If you wish this IP to operate as an SPI controller, it
> should have an SPI compatible, if you wish it to operate as a U(S)ART,
> then it should have a UART compatible.  It's what we do for most of
> the other MFDs in the kernel.

There is a big difference: MFD functions are(more or less) independent
functions, which can be used at the same time. It makes perfect sense for a
single IP block that has both SPI and UART interfaces, that can be used at
the same time.

In this case, there is a single piece of hardware that can perform
different functions, but not at the same time. Performing a different
function means configuring the hardware for that function, hence using a
different driver (from a different subsystem).

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds


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

2018-09-12 Thread Lee Jones
On Wed, 12 Sep 2018, Alexandre Belloni wrote:

> On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > > 
> > > > USART doc starting p572, registers p621.
> > 
> > After looking at the datasheet, I don't see any reason why one of the
> > two drivers can't be selected using different compatible strings.
> 
> Because there is only one IP and we don't use the device tree to selecet
> linux specific drivers.

We do it all the time.  There are loads of MFDs (def: same IP, with
different functions) which have separate compatibles for their various
functions.  If you wish this IP to operate as an SPI controller, it
should have an SPI compatible, if you wish it to operate as a U(S)ART,
then it should have a UART compatible.  It's what we do for most of
the other MFDs in the kernel.

> If you are not happy having that in MFD, I guess we can move it out
> somewhere else.

My issue isn't pertaining to where the hack lives, it's that there is
a hack in the first place.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

2018-09-12 Thread Lee Jones
On Wed, 12 Sep 2018, Alexandre Belloni wrote:

> On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > > 
> > > > USART doc starting p572, registers p621.
> > 
> > After looking at the datasheet, I don't see any reason why one of the
> > two drivers can't be selected using different compatible strings.
> 
> Because there is only one IP and we don't use the device tree to selecet
> linux specific drivers.

We do it all the time.  There are loads of MFDs (def: same IP, with
different functions) which have separate compatibles for their various
functions.  If you wish this IP to operate as an SPI controller, it
should have an SPI compatible, if you wish it to operate as a U(S)ART,
then it should have a UART compatible.  It's what we do for most of
the other MFDs in the kernel.

> If you are not happy having that in MFD, I guess we can move it out
> somewhere else.

My issue isn't pertaining to where the hack lives, it's that there is
a hack in the first place.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

2018-09-12 Thread Lee Jones
On Wed, 12 Sep 2018, Alexandre Belloni wrote:

> On 11/09/2018 23:43:02+0100, Lee Jones wrote:
> > > I haven't read it, but I believe it's not unlike Renesas SCIF, which is
> > > served by both drivers/tty/serial/sh-sci.c and drivers/spi/spi-sh-sci.c.
> > > But the latter is not used from DT, so we haven't experienced (and solved)
> > > the similar issue yet.
> > > 
> > > Would it work if the UART driver and SPI driver would match against the
> > > same compatible value, but the UART driver would do in its probe()
> > > function:
> > > 
> > > device_property_read_u32(>dev, "atmel,usart-mode", );
> > > if (opmode != AT91_USART_MODE_SERIAL)
> > > return ENODEV;
> > > 
> > > while the SPI driver would do:
> > > 
> > > device_property_read_u32(>dev, "atmel,usart-mode", );
> > > if (opmode != AT91_USART_MODE_SPI)
> > > return ENODEV;
> > > 
> > > ? No MFD driver involved.
> > 
> > I haven't looked at the code in a while, but if memory serves I
> > believe platform code gives up once it has found its first match, so
> > by doing this, one of the drivers will never be matched/probed.
> > 
> > It's midnight here, so cracking out the datasheet isn't going to
> > happen just now, but it's my current belief that if the IP serves 2
> > very different modes of operation, even if the registers are in a
> > shared space, they could have their own compatible strings in DT.
> > 
> > That is what the MFD driver provides after all.  Why would it be okay
> > to allocate different compatible strings from the MFD, but not in the
> > Device Tree?
> > 
> > It would be the easiest solution.
> > 
> > Has Rob commented on this yet?
> 
> V4 of the bindings were acked by Rob and you:
> https://patchwork.kernel.org/patch/10428087/

We didn't Ack the bindings.  We Acked the location change.

I mean, has Rob specifically spoken out about using a compatible
string for each function.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

2018-09-12 Thread Lee Jones
On Wed, 12 Sep 2018, Alexandre Belloni wrote:

> On 11/09/2018 23:43:02+0100, Lee Jones wrote:
> > > I haven't read it, but I believe it's not unlike Renesas SCIF, which is
> > > served by both drivers/tty/serial/sh-sci.c and drivers/spi/spi-sh-sci.c.
> > > But the latter is not used from DT, so we haven't experienced (and solved)
> > > the similar issue yet.
> > > 
> > > Would it work if the UART driver and SPI driver would match against the
> > > same compatible value, but the UART driver would do in its probe()
> > > function:
> > > 
> > > device_property_read_u32(>dev, "atmel,usart-mode", );
> > > if (opmode != AT91_USART_MODE_SERIAL)
> > > return ENODEV;
> > > 
> > > while the SPI driver would do:
> > > 
> > > device_property_read_u32(>dev, "atmel,usart-mode", );
> > > if (opmode != AT91_USART_MODE_SPI)
> > > return ENODEV;
> > > 
> > > ? No MFD driver involved.
> > 
> > I haven't looked at the code in a while, but if memory serves I
> > believe platform code gives up once it has found its first match, so
> > by doing this, one of the drivers will never be matched/probed.
> > 
> > It's midnight here, so cracking out the datasheet isn't going to
> > happen just now, but it's my current belief that if the IP serves 2
> > very different modes of operation, even if the registers are in a
> > shared space, they could have their own compatible strings in DT.
> > 
> > That is what the MFD driver provides after all.  Why would it be okay
> > to allocate different compatible strings from the MFD, but not in the
> > Device Tree?
> > 
> > It would be the easiest solution.
> > 
> > Has Rob commented on this yet?
> 
> V4 of the bindings were acked by Rob and you:
> https://patchwork.kernel.org/patch/10428087/

We didn't Ack the bindings.  We Acked the location change.

I mean, has Rob specifically spoken out about using a compatible
string for each function.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

2018-09-12 Thread Alexandre Belloni
On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > 
> > > USART doc starting p572, registers p621.
> 
> After looking at the datasheet, I don't see any reason why one of the
> two drivers can't be selected using different compatible strings.

Because there is only one IP and we don't use the device tree to selecet
linux specific drivers.

If you are not happy having that in MFD, I guess we can move it out
somewhere else.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com


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

2018-09-12 Thread Alexandre Belloni
On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > 
> > > USART doc starting p572, registers p621.
> 
> After looking at the datasheet, I don't see any reason why one of the
> two drivers can't be selected using different compatible strings.

Because there is only one IP and we don't use the device tree to selecet
linux specific drivers.

If you are not happy having that in MFD, I guess we can move it out
somewhere else.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com


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

2018-09-12 Thread Alexandre Belloni
On 11/09/2018 23:43:02+0100, Lee Jones wrote:
> > I haven't read it, but I believe it's not unlike Renesas SCIF, which is
> > served by both drivers/tty/serial/sh-sci.c and drivers/spi/spi-sh-sci.c.
> > But the latter is not used from DT, so we haven't experienced (and solved)
> > the similar issue yet.
> > 
> > Would it work if the UART driver and SPI driver would match against the
> > same compatible value, but the UART driver would do in its probe()
> > function:
> > 
> > device_property_read_u32(>dev, "atmel,usart-mode", );
> > if (opmode != AT91_USART_MODE_SERIAL)
> > return ENODEV;
> > 
> > while the SPI driver would do:
> > 
> > device_property_read_u32(>dev, "atmel,usart-mode", );
> > if (opmode != AT91_USART_MODE_SPI)
> > return ENODEV;
> > 
> > ? No MFD driver involved.
> 
> I haven't looked at the code in a while, but if memory serves I
> believe platform code gives up once it has found its first match, so
> by doing this, one of the drivers will never be matched/probed.
> 
> It's midnight here, so cracking out the datasheet isn't going to
> happen just now, but it's my current belief that if the IP serves 2
> very different modes of operation, even if the registers are in a
> shared space, they could have their own compatible strings in DT.
> 
> That is what the MFD driver provides after all.  Why would it be okay
> to allocate different compatible strings from the MFD, but not in the
> Device Tree?
> 
> It would be the easiest solution.
> 
> Has Rob commented on this yet?
> 

V4 of the bindings were acked by Rob and you:
https://patchwork.kernel.org/patch/10428087/


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com


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

2018-09-12 Thread Alexandre Belloni
On 11/09/2018 23:43:02+0100, Lee Jones wrote:
> > I haven't read it, but I believe it's not unlike Renesas SCIF, which is
> > served by both drivers/tty/serial/sh-sci.c and drivers/spi/spi-sh-sci.c.
> > But the latter is not used from DT, so we haven't experienced (and solved)
> > the similar issue yet.
> > 
> > Would it work if the UART driver and SPI driver would match against the
> > same compatible value, but the UART driver would do in its probe()
> > function:
> > 
> > device_property_read_u32(>dev, "atmel,usart-mode", );
> > if (opmode != AT91_USART_MODE_SERIAL)
> > return ENODEV;
> > 
> > while the SPI driver would do:
> > 
> > device_property_read_u32(>dev, "atmel,usart-mode", );
> > if (opmode != AT91_USART_MODE_SPI)
> > return ENODEV;
> > 
> > ? No MFD driver involved.
> 
> I haven't looked at the code in a while, but if memory serves I
> believe platform code gives up once it has found its first match, so
> by doing this, one of the drivers will never be matched/probed.
> 
> It's midnight here, so cracking out the datasheet isn't going to
> happen just now, but it's my current belief that if the IP serves 2
> very different modes of operation, even if the registers are in a
> shared space, they could have their own compatible strings in DT.
> 
> That is what the MFD driver provides after all.  Why would it be okay
> to allocate different compatible strings from the MFD, but not in the
> Device Tree?
> 
> It would be the easiest solution.
> 
> Has Rob commented on this yet?
> 

V4 of the bindings were acked by Rob and you:
https://patchwork.kernel.org/patch/10428087/


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com


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

2018-09-11 Thread Lee Jones
On Tue, 11 Sep 2018, Lee Jones wrote:

> On Tue, 11 Sep 2018, Alexandre Belloni wrote:
> 
> > On 11/09/2018 19:39:30+0100, Lee Jones wrote:
> > > On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> > > 
> > > > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> > > >  wrote:
> > > > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > > > >  wrote:
> > > > > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > > > > Radu Pirea (6):
> > > > > > > > >   MAINTAINERS: add at91 usart mfd driver
> > > > > > > > >   dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > > > >   mfd: at91-usart: added mfd driver for usart
> > > > > > > > >   MAINTAINERS: add at91 usart spi driver
> > > > > > > > >   spi: at91-usart: add driver for at91-usart as spi
> > > > > > > > >   tty/serial: atmel: change the driver to work under 
> > > > > > > > > at91-usart mfd
> > > > > > > > >
> > > > > > > > >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> > > > > > > > >  MAINTAINERS   |  16 +
> > > > > > > > >  drivers/mfd/Kconfig   |   9 +
> > > > > > > > >  drivers/mfd/Makefile  |   1 +
> > > > > > > > >  drivers/mfd/at91-usart.c  |  71 +++
> > > > > > > > >  drivers/spi/Kconfig   |   8 +
> > > > > > > > >  drivers/spi/Makefile  |   1 +
> > > > > > > > >  drivers/spi/spi-at91-usart.c  | 432 
> > > > > > > > > ++
> > > > > > > > >  drivers/tty/serial/Kconfig|   1 +
> > > > > > > > >  drivers/tty/serial/atmel_serial.c |  42 +-
> > > > > > > > >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> > > > > > > > >  11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > > > >  rename Documentation/devicetree/bindings/{serial => 
> > > > > > > > > mfd}/atmel-usart.txt (76%)
> > > > > > > > >  create mode 100644 drivers/mfd/at91-usart.c
> > > > > > > > >  create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > > > >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > > > > >
> > > > > > > > Seeing as this patch-set has caused some issues this morning, I 
> > > > > > > > took
> > > > > > > > the liberty to peruse back into its history to figure out where 
> > > > > > > > things
> > > > > > > > started to go wrong.  I also re-reviewed the MFD driver - and 
> > > > > > > > I'm glad
> > > > > > > > I did!
> > > > > > > >
> > > > > > > > My Acked-by has been attached to the MFD portion since v5, 
> > > > > > > > which is
> > > > > > > > why the code hasn't caught my eye before today.  I reviewed the
> > > > > > > > relocation of the *binding document* (serial => mfd with no 
> > > > > > > > changes)
> > > > > > > > in v4 and nothing else.  It appears as though you mistakenly 
> > > > > > > > added it
> > > > > > > > to the *MFD driver* instead.  This explains my confusion in v10 
> > > > > > > > when I
> > > > > > > > told you I'd already reviewed the binding document.
> > > > > > > >
> > > > > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to 
> > > > > > > > say
> > > > > > > > that I do not like what I see.  Besides the missing header file 
> > > > > > > > and
> > > > > > > > the whitespace tabbing errors, I do not agree with the 
> > > > > > > > implementation.
> > > > > > > > Using MFD as a shim to hack around driver selection is not a 
> > > > > > > > valid
> > > > > > > > use-case.
> > > > > > > >
> > > > > > > > What's stopping you from just using the compatible string 
> > > > > > > > directly to
> > > > > > > > select which driver you need to probe?
> > > > > > > >
> > > > > > >
> > > > > > > Then you'd have multiple compatible strings for the same IP which 
> > > > > > > is a
> > > > > > > big no-no.
> > > > > >
> > > > > > It's still the same hardware device, isn't?
> > > > > > What if the SPI or UART slave is not on-board, but on an expansion 
> > > > > > board?
> > > > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > > > >
> > > > > > Hence shouldn't the software derive the hardware mode from the full
> > > > > > hardware description in DT? If that's impossible (I didn't look 
> > > > > > into detail
> > > > > > whether an SPI bus can easily be distinguished from a UART bus), 
> > > > > > perhaps
> > > > > > a mode property should be added?
> > > > >
> > > > > Yes, this is exactly what is done:
> > > > >
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> > > > 
> > > > OK.
> > > > 
> > > > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > > > which thus requires having additional compatible values?
> > > > 
> > > > I think those can just be removed. AFAICS, the SPI and serial drivers 
> > > 

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

2018-09-11 Thread Lee Jones
On Tue, 11 Sep 2018, Lee Jones wrote:

> On Tue, 11 Sep 2018, Alexandre Belloni wrote:
> 
> > On 11/09/2018 19:39:30+0100, Lee Jones wrote:
> > > On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> > > 
> > > > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> > > >  wrote:
> > > > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > > > >  wrote:
> > > > > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > > > > Radu Pirea (6):
> > > > > > > > >   MAINTAINERS: add at91 usart mfd driver
> > > > > > > > >   dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > > > >   mfd: at91-usart: added mfd driver for usart
> > > > > > > > >   MAINTAINERS: add at91 usart spi driver
> > > > > > > > >   spi: at91-usart: add driver for at91-usart as spi
> > > > > > > > >   tty/serial: atmel: change the driver to work under 
> > > > > > > > > at91-usart mfd
> > > > > > > > >
> > > > > > > > >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> > > > > > > > >  MAINTAINERS   |  16 +
> > > > > > > > >  drivers/mfd/Kconfig   |   9 +
> > > > > > > > >  drivers/mfd/Makefile  |   1 +
> > > > > > > > >  drivers/mfd/at91-usart.c  |  71 +++
> > > > > > > > >  drivers/spi/Kconfig   |   8 +
> > > > > > > > >  drivers/spi/Makefile  |   1 +
> > > > > > > > >  drivers/spi/spi-at91-usart.c  | 432 
> > > > > > > > > ++
> > > > > > > > >  drivers/tty/serial/Kconfig|   1 +
> > > > > > > > >  drivers/tty/serial/atmel_serial.c |  42 +-
> > > > > > > > >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> > > > > > > > >  11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > > > >  rename Documentation/devicetree/bindings/{serial => 
> > > > > > > > > mfd}/atmel-usart.txt (76%)
> > > > > > > > >  create mode 100644 drivers/mfd/at91-usart.c
> > > > > > > > >  create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > > > >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > > > > >
> > > > > > > > Seeing as this patch-set has caused some issues this morning, I 
> > > > > > > > took
> > > > > > > > the liberty to peruse back into its history to figure out where 
> > > > > > > > things
> > > > > > > > started to go wrong.  I also re-reviewed the MFD driver - and 
> > > > > > > > I'm glad
> > > > > > > > I did!
> > > > > > > >
> > > > > > > > My Acked-by has been attached to the MFD portion since v5, 
> > > > > > > > which is
> > > > > > > > why the code hasn't caught my eye before today.  I reviewed the
> > > > > > > > relocation of the *binding document* (serial => mfd with no 
> > > > > > > > changes)
> > > > > > > > in v4 and nothing else.  It appears as though you mistakenly 
> > > > > > > > added it
> > > > > > > > to the *MFD driver* instead.  This explains my confusion in v10 
> > > > > > > > when I
> > > > > > > > told you I'd already reviewed the binding document.
> > > > > > > >
> > > > > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to 
> > > > > > > > say
> > > > > > > > that I do not like what I see.  Besides the missing header file 
> > > > > > > > and
> > > > > > > > the whitespace tabbing errors, I do not agree with the 
> > > > > > > > implementation.
> > > > > > > > Using MFD as a shim to hack around driver selection is not a 
> > > > > > > > valid
> > > > > > > > use-case.
> > > > > > > >
> > > > > > > > What's stopping you from just using the compatible string 
> > > > > > > > directly to
> > > > > > > > select which driver you need to probe?
> > > > > > > >
> > > > > > >
> > > > > > > Then you'd have multiple compatible strings for the same IP which 
> > > > > > > is a
> > > > > > > big no-no.
> > > > > >
> > > > > > It's still the same hardware device, isn't?
> > > > > > What if the SPI or UART slave is not on-board, but on an expansion 
> > > > > > board?
> > > > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > > > >
> > > > > > Hence shouldn't the software derive the hardware mode from the full
> > > > > > hardware description in DT? If that's impossible (I didn't look 
> > > > > > into detail
> > > > > > whether an SPI bus can easily be distinguished from a UART bus), 
> > > > > > perhaps
> > > > > > a mode property should be added?
> > > > >
> > > > > Yes, this is exactly what is done:
> > > > >
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> > > > 
> > > > OK.
> > > > 
> > > > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > > > which thus requires having additional compatible values?
> > > > 
> > > > I think those can just be removed. AFAICS, the SPI and serial drivers 
> > > 

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

2018-09-11 Thread Lee Jones
On Tue, 11 Sep 2018, Alexandre Belloni wrote:

> On 11/09/2018 19:39:30+0100, Lee Jones wrote:
> > On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> > 
> > > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> > >  wrote:
> > > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > > >  wrote:
> > > > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > > > Radu Pirea (6):
> > > > > > > >   MAINTAINERS: add at91 usart mfd driver
> > > > > > > >   dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > > >   mfd: at91-usart: added mfd driver for usart
> > > > > > > >   MAINTAINERS: add at91 usart spi driver
> > > > > > > >   spi: at91-usart: add driver for at91-usart as spi
> > > > > > > >   tty/serial: atmel: change the driver to work under at91-usart 
> > > > > > > > mfd
> > > > > > > >
> > > > > > > >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> > > > > > > >  MAINTAINERS   |  16 +
> > > > > > > >  drivers/mfd/Kconfig   |   9 +
> > > > > > > >  drivers/mfd/Makefile  |   1 +
> > > > > > > >  drivers/mfd/at91-usart.c  |  71 +++
> > > > > > > >  drivers/spi/Kconfig   |   8 +
> > > > > > > >  drivers/spi/Makefile  |   1 +
> > > > > > > >  drivers/spi/spi-at91-usart.c  | 432 
> > > > > > > > ++
> > > > > > > >  drivers/tty/serial/Kconfig|   1 +
> > > > > > > >  drivers/tty/serial/atmel_serial.c |  42 +-
> > > > > > > >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> > > > > > > >  11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > > >  rename Documentation/devicetree/bindings/{serial => 
> > > > > > > > mfd}/atmel-usart.txt (76%)
> > > > > > > >  create mode 100644 drivers/mfd/at91-usart.c
> > > > > > > >  create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > > >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > > > >
> > > > > > > Seeing as this patch-set has caused some issues this morning, I 
> > > > > > > took
> > > > > > > the liberty to peruse back into its history to figure out where 
> > > > > > > things
> > > > > > > started to go wrong.  I also re-reviewed the MFD driver - and I'm 
> > > > > > > glad
> > > > > > > I did!
> > > > > > >
> > > > > > > My Acked-by has been attached to the MFD portion since v5, which 
> > > > > > > is
> > > > > > > why the code hasn't caught my eye before today.  I reviewed the
> > > > > > > relocation of the *binding document* (serial => mfd with no 
> > > > > > > changes)
> > > > > > > in v4 and nothing else.  It appears as though you mistakenly 
> > > > > > > added it
> > > > > > > to the *MFD driver* instead.  This explains my confusion in v10 
> > > > > > > when I
> > > > > > > told you I'd already reviewed the binding document.
> > > > > > >
> > > > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > > > that I do not like what I see.  Besides the missing header file 
> > > > > > > and
> > > > > > > the whitespace tabbing errors, I do not agree with the 
> > > > > > > implementation.
> > > > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > > > use-case.
> > > > > > >
> > > > > > > What's stopping you from just using the compatible string 
> > > > > > > directly to
> > > > > > > select which driver you need to probe?
> > > > > > >
> > > > > >
> > > > > > Then you'd have multiple compatible strings for the same IP which 
> > > > > > is a
> > > > > > big no-no.
> > > > >
> > > > > It's still the same hardware device, isn't?
> > > > > What if the SPI or UART slave is not on-board, but on an expansion 
> > > > > board?
> > > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > > >
> > > > > Hence shouldn't the software derive the hardware mode from the full
> > > > > hardware description in DT? If that's impossible (I didn't look into 
> > > > > detail
> > > > > whether an SPI bus can easily be distinguished from a UART bus), 
> > > > > perhaps
> > > > > a mode property should be added?
> > > >
> > > > Yes, this is exactly what is done:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> > > 
> > > OK.
> > > 
> > > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > > which thus requires having additional compatible values?
> > > 
> > > I think those can just be removed. AFAICS, the SPI and serial drivers 
> > > already
> > > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform 
> > > device
> > > names?
> > 
> > The hackish part of this driver is that it's using MFD for something
> > which is clearly not an MFD.  It's a USART device.  Nothing more,
> > 

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

2018-09-11 Thread Lee Jones
On Tue, 11 Sep 2018, Alexandre Belloni wrote:

> On 11/09/2018 19:39:30+0100, Lee Jones wrote:
> > On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> > 
> > > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> > >  wrote:
> > > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > > >  wrote:
> > > > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > > > Radu Pirea (6):
> > > > > > > >   MAINTAINERS: add at91 usart mfd driver
> > > > > > > >   dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > > >   mfd: at91-usart: added mfd driver for usart
> > > > > > > >   MAINTAINERS: add at91 usart spi driver
> > > > > > > >   spi: at91-usart: add driver for at91-usart as spi
> > > > > > > >   tty/serial: atmel: change the driver to work under at91-usart 
> > > > > > > > mfd
> > > > > > > >
> > > > > > > >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> > > > > > > >  MAINTAINERS   |  16 +
> > > > > > > >  drivers/mfd/Kconfig   |   9 +
> > > > > > > >  drivers/mfd/Makefile  |   1 +
> > > > > > > >  drivers/mfd/at91-usart.c  |  71 +++
> > > > > > > >  drivers/spi/Kconfig   |   8 +
> > > > > > > >  drivers/spi/Makefile  |   1 +
> > > > > > > >  drivers/spi/spi-at91-usart.c  | 432 
> > > > > > > > ++
> > > > > > > >  drivers/tty/serial/Kconfig|   1 +
> > > > > > > >  drivers/tty/serial/atmel_serial.c |  42 +-
> > > > > > > >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> > > > > > > >  11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > > >  rename Documentation/devicetree/bindings/{serial => 
> > > > > > > > mfd}/atmel-usart.txt (76%)
> > > > > > > >  create mode 100644 drivers/mfd/at91-usart.c
> > > > > > > >  create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > > >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > > > >
> > > > > > > Seeing as this patch-set has caused some issues this morning, I 
> > > > > > > took
> > > > > > > the liberty to peruse back into its history to figure out where 
> > > > > > > things
> > > > > > > started to go wrong.  I also re-reviewed the MFD driver - and I'm 
> > > > > > > glad
> > > > > > > I did!
> > > > > > >
> > > > > > > My Acked-by has been attached to the MFD portion since v5, which 
> > > > > > > is
> > > > > > > why the code hasn't caught my eye before today.  I reviewed the
> > > > > > > relocation of the *binding document* (serial => mfd with no 
> > > > > > > changes)
> > > > > > > in v4 and nothing else.  It appears as though you mistakenly 
> > > > > > > added it
> > > > > > > to the *MFD driver* instead.  This explains my confusion in v10 
> > > > > > > when I
> > > > > > > told you I'd already reviewed the binding document.
> > > > > > >
> > > > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > > > that I do not like what I see.  Besides the missing header file 
> > > > > > > and
> > > > > > > the whitespace tabbing errors, I do not agree with the 
> > > > > > > implementation.
> > > > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > > > use-case.
> > > > > > >
> > > > > > > What's stopping you from just using the compatible string 
> > > > > > > directly to
> > > > > > > select which driver you need to probe?
> > > > > > >
> > > > > >
> > > > > > Then you'd have multiple compatible strings for the same IP which 
> > > > > > is a
> > > > > > big no-no.
> > > > >
> > > > > It's still the same hardware device, isn't?
> > > > > What if the SPI or UART slave is not on-board, but on an expansion 
> > > > > board?
> > > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > > >
> > > > > Hence shouldn't the software derive the hardware mode from the full
> > > > > hardware description in DT? If that's impossible (I didn't look into 
> > > > > detail
> > > > > whether an SPI bus can easily be distinguished from a UART bus), 
> > > > > perhaps
> > > > > a mode property should be added?
> > > >
> > > > Yes, this is exactly what is done:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> > > 
> > > OK.
> > > 
> > > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > > which thus requires having additional compatible values?
> > > 
> > > I think those can just be removed. AFAICS, the SPI and serial drivers 
> > > already
> > > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform 
> > > device
> > > names?
> > 
> > The hackish part of this driver is that it's using MFD for something
> > which is clearly not an MFD.  It's a USART device.  Nothing more,
> > 

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

2018-09-11 Thread Lee Jones
On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> On Tue, Sep 11, 2018 at 8:40 PM Lee Jones  wrote:
> > On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> > > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> > >  wrote:
> > > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > > >  wrote:
> > > > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > > > Radu Pirea (6):
> > > > > > > >   MAINTAINERS: add at91 usart mfd driver
> > > > > > > >   dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > > >   mfd: at91-usart: added mfd driver for usart
> > > > > > > >   MAINTAINERS: add at91 usart spi driver
> > > > > > > >   spi: at91-usart: add driver for at91-usart as spi
> > > > > > > >   tty/serial: atmel: change the driver to work under at91-usart 
> > > > > > > > mfd
> > > > > > > >
> > > > > > > >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> > > > > > > >  MAINTAINERS   |  16 +
> > > > > > > >  drivers/mfd/Kconfig   |   9 +
> > > > > > > >  drivers/mfd/Makefile  |   1 +
> > > > > > > >  drivers/mfd/at91-usart.c  |  71 +++
> > > > > > > >  drivers/spi/Kconfig   |   8 +
> > > > > > > >  drivers/spi/Makefile  |   1 +
> > > > > > > >  drivers/spi/spi-at91-usart.c  | 432 
> > > > > > > > ++
> > > > > > > >  drivers/tty/serial/Kconfig|   1 +
> > > > > > > >  drivers/tty/serial/atmel_serial.c |  42 +-
> > > > > > > >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> > > > > > > >  11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > > >  rename Documentation/devicetree/bindings/{serial => 
> > > > > > > > mfd}/atmel-usart.txt (76%)
> > > > > > > >  create mode 100644 drivers/mfd/at91-usart.c
> > > > > > > >  create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > > >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > > > >
> > > > > > > Seeing as this patch-set has caused some issues this morning, I 
> > > > > > > took
> > > > > > > the liberty to peruse back into its history to figure out where 
> > > > > > > things
> > > > > > > started to go wrong.  I also re-reviewed the MFD driver - and I'm 
> > > > > > > glad
> > > > > > > I did!
> > > > > > >
> > > > > > > My Acked-by has been attached to the MFD portion since v5, which 
> > > > > > > is
> > > > > > > why the code hasn't caught my eye before today.  I reviewed the
> > > > > > > relocation of the *binding document* (serial => mfd with no 
> > > > > > > changes)
> > > > > > > in v4 and nothing else.  It appears as though you mistakenly 
> > > > > > > added it
> > > > > > > to the *MFD driver* instead.  This explains my confusion in v10 
> > > > > > > when I
> > > > > > > told you I'd already reviewed the binding document.
> > > > > > >
> > > > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > > > that I do not like what I see.  Besides the missing header file 
> > > > > > > and
> > > > > > > the whitespace tabbing errors, I do not agree with the 
> > > > > > > implementation.
> > > > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > > > use-case.
> > > > > > >
> > > > > > > What's stopping you from just using the compatible string 
> > > > > > > directly to
> > > > > > > select which driver you need to probe?
> > > > > > >
> > > > > >
> > > > > > Then you'd have multiple compatible strings for the same IP which 
> > > > > > is a
> > > > > > big no-no.
> > > > >
> > > > > It's still the same hardware device, isn't?
> > > > > What if the SPI or UART slave is not on-board, but on an expansion 
> > > > > board?
> > > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > > >
> > > > > Hence shouldn't the software derive the hardware mode from the full
> > > > > hardware description in DT? If that's impossible (I didn't look into 
> > > > > detail
> > > > > whether an SPI bus can easily be distinguished from a UART bus), 
> > > > > perhaps
> > > > > a mode property should be added?
> > > >
> > > > Yes, this is exactly what is done:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> > >
> > > OK.
> > >
> > > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > > which thus requires having additional compatible values?
> > >
> > > I think those can just be removed. AFAICS, the SPI and serial drivers 
> > > already
> > > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform 
> > > device
> > > names?
> >
> > The hackish part of this driver is that it's using MFD for something
> > which is clearly not an MFD.  It's a USART device.  Nothing more,
> > nothing 

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

2018-09-11 Thread Lee Jones
On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> On Tue, Sep 11, 2018 at 8:40 PM Lee Jones  wrote:
> > On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> > > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> > >  wrote:
> > > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > > >  wrote:
> > > > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > > > Radu Pirea (6):
> > > > > > > >   MAINTAINERS: add at91 usart mfd driver
> > > > > > > >   dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > > >   mfd: at91-usart: added mfd driver for usart
> > > > > > > >   MAINTAINERS: add at91 usart spi driver
> > > > > > > >   spi: at91-usart: add driver for at91-usart as spi
> > > > > > > >   tty/serial: atmel: change the driver to work under at91-usart 
> > > > > > > > mfd
> > > > > > > >
> > > > > > > >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> > > > > > > >  MAINTAINERS   |  16 +
> > > > > > > >  drivers/mfd/Kconfig   |   9 +
> > > > > > > >  drivers/mfd/Makefile  |   1 +
> > > > > > > >  drivers/mfd/at91-usart.c  |  71 +++
> > > > > > > >  drivers/spi/Kconfig   |   8 +
> > > > > > > >  drivers/spi/Makefile  |   1 +
> > > > > > > >  drivers/spi/spi-at91-usart.c  | 432 
> > > > > > > > ++
> > > > > > > >  drivers/tty/serial/Kconfig|   1 +
> > > > > > > >  drivers/tty/serial/atmel_serial.c |  42 +-
> > > > > > > >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> > > > > > > >  11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > > >  rename Documentation/devicetree/bindings/{serial => 
> > > > > > > > mfd}/atmel-usart.txt (76%)
> > > > > > > >  create mode 100644 drivers/mfd/at91-usart.c
> > > > > > > >  create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > > >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > > > >
> > > > > > > Seeing as this patch-set has caused some issues this morning, I 
> > > > > > > took
> > > > > > > the liberty to peruse back into its history to figure out where 
> > > > > > > things
> > > > > > > started to go wrong.  I also re-reviewed the MFD driver - and I'm 
> > > > > > > glad
> > > > > > > I did!
> > > > > > >
> > > > > > > My Acked-by has been attached to the MFD portion since v5, which 
> > > > > > > is
> > > > > > > why the code hasn't caught my eye before today.  I reviewed the
> > > > > > > relocation of the *binding document* (serial => mfd with no 
> > > > > > > changes)
> > > > > > > in v4 and nothing else.  It appears as though you mistakenly 
> > > > > > > added it
> > > > > > > to the *MFD driver* instead.  This explains my confusion in v10 
> > > > > > > when I
> > > > > > > told you I'd already reviewed the binding document.
> > > > > > >
> > > > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > > > that I do not like what I see.  Besides the missing header file 
> > > > > > > and
> > > > > > > the whitespace tabbing errors, I do not agree with the 
> > > > > > > implementation.
> > > > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > > > use-case.
> > > > > > >
> > > > > > > What's stopping you from just using the compatible string 
> > > > > > > directly to
> > > > > > > select which driver you need to probe?
> > > > > > >
> > > > > >
> > > > > > Then you'd have multiple compatible strings for the same IP which 
> > > > > > is a
> > > > > > big no-no.
> > > > >
> > > > > It's still the same hardware device, isn't?
> > > > > What if the SPI or UART slave is not on-board, but on an expansion 
> > > > > board?
> > > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > > >
> > > > > Hence shouldn't the software derive the hardware mode from the full
> > > > > hardware description in DT? If that's impossible (I didn't look into 
> > > > > detail
> > > > > whether an SPI bus can easily be distinguished from a UART bus), 
> > > > > perhaps
> > > > > a mode property should be added?
> > > >
> > > > Yes, this is exactly what is done:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> > >
> > > OK.
> > >
> > > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > > which thus requires having additional compatible values?
> > >
> > > I think those can just be removed. AFAICS, the SPI and serial drivers 
> > > already
> > > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform 
> > > device
> > > names?
> >
> > The hackish part of this driver is that it's using MFD for something
> > which is clearly not an MFD.  It's a USART device.  Nothing more,
> > nothing 

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

2018-09-11 Thread Geert Uytterhoeven
Hi Alexandre,

On Tue, Sep 11, 2018 at 8:58 PM Alexandre Belloni
 wrote:
> On 11/09/2018 19:39:30+0100, Lee Jones wrote:
> > On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> >
> > > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> > >  wrote:
> > > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > > >  wrote:
> > > > > > Then you'd have multiple compatible strings for the same IP which 
> > > > > > is a
> > > > > > big no-no.
> > > > >
> > > > > It's still the same hardware device, isn't?
> > > > > What if the SPI or UART slave is not on-board, but on an expansion 
> > > > > board?
> > > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > > >
> > > > > Hence shouldn't the software derive the hardware mode from the full
> > > > > hardware description in DT? If that's impossible (I didn't look into 
> > > > > detail
> > > > > whether an SPI bus can easily be distinguished from a UART bus), 
> > > > > perhaps
> > > > > a mode property should be added?
> > > >
> > > > Yes, this is exactly what is done:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> > >
> > > OK.
> > >
> > > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > > which thus requires having additional compatible values?
> > >
> > > I think those can just be removed. AFAICS, the SPI and serial drivers 
> > > already
> > > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform 
> > > device
> > > names?
> >
> > The hackish part of this driver is that it's using MFD for something
> > which is clearly not an MFD.  It's a USART device.  Nothing more,
> > nothing less.
> >
> > Does anyone have the datasheet to hand?
>
> It is not a simple usart, it is either a usart or a full blown SPI
> controller with registers changing layout depending on the selected
> mode. Otherwise, I'm not sure how you would get a USART to do SPI.

Note the "S" in USART. SPI is just synchronous serial with a shared clock
for transmit and receive. So the hardware is not that unrelated.

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds


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

2018-09-11 Thread Geert Uytterhoeven
Hi Alexandre,

On Tue, Sep 11, 2018 at 8:58 PM Alexandre Belloni
 wrote:
> On 11/09/2018 19:39:30+0100, Lee Jones wrote:
> > On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> >
> > > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> > >  wrote:
> > > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > > >  wrote:
> > > > > > Then you'd have multiple compatible strings for the same IP which 
> > > > > > is a
> > > > > > big no-no.
> > > > >
> > > > > It's still the same hardware device, isn't?
> > > > > What if the SPI or UART slave is not on-board, but on an expansion 
> > > > > board?
> > > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > > >
> > > > > Hence shouldn't the software derive the hardware mode from the full
> > > > > hardware description in DT? If that's impossible (I didn't look into 
> > > > > detail
> > > > > whether an SPI bus can easily be distinguished from a UART bus), 
> > > > > perhaps
> > > > > a mode property should be added?
> > > >
> > > > Yes, this is exactly what is done:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> > >
> > > OK.
> > >
> > > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > > which thus requires having additional compatible values?
> > >
> > > I think those can just be removed. AFAICS, the SPI and serial drivers 
> > > already
> > > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform 
> > > device
> > > names?
> >
> > The hackish part of this driver is that it's using MFD for something
> > which is clearly not an MFD.  It's a USART device.  Nothing more,
> > nothing less.
> >
> > Does anyone have the datasheet to hand?
>
> It is not a simple usart, it is either a usart or a full blown SPI
> controller with registers changing layout depending on the selected
> mode. Otherwise, I'm not sure how you would get a USART to do SPI.

Note the "S" in USART. SPI is just synchronous serial with a shared clock
for transmit and receive. So the hardware is not that unrelated.

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds


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

2018-09-11 Thread Geert Uytterhoeven
Hi Lee,

On Tue, Sep 11, 2018 at 8:40 PM Lee Jones  wrote:
> On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> >  wrote:
> > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > >  wrote:
> > > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > > Radu Pirea (6):
> > > > > > >   MAINTAINERS: add at91 usart mfd driver
> > > > > > >   dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > >   mfd: at91-usart: added mfd driver for usart
> > > > > > >   MAINTAINERS: add at91 usart spi driver
> > > > > > >   spi: at91-usart: add driver for at91-usart as spi
> > > > > > >   tty/serial: atmel: change the driver to work under at91-usart 
> > > > > > > mfd
> > > > > > >
> > > > > > >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> > > > > > >  MAINTAINERS   |  16 +
> > > > > > >  drivers/mfd/Kconfig   |   9 +
> > > > > > >  drivers/mfd/Makefile  |   1 +
> > > > > > >  drivers/mfd/at91-usart.c  |  71 +++
> > > > > > >  drivers/spi/Kconfig   |   8 +
> > > > > > >  drivers/spi/Makefile  |   1 +
> > > > > > >  drivers/spi/spi-at91-usart.c  | 432 
> > > > > > > ++
> > > > > > >  drivers/tty/serial/Kconfig|   1 +
> > > > > > >  drivers/tty/serial/atmel_serial.c |  42 +-
> > > > > > >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> > > > > > >  11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > >  rename Documentation/devicetree/bindings/{serial => 
> > > > > > > mfd}/atmel-usart.txt (76%)
> > > > > > >  create mode 100644 drivers/mfd/at91-usart.c
> > > > > > >  create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > > >
> > > > > > Seeing as this patch-set has caused some issues this morning, I took
> > > > > > the liberty to peruse back into its history to figure out where 
> > > > > > things
> > > > > > started to go wrong.  I also re-reviewed the MFD driver - and I'm 
> > > > > > glad
> > > > > > I did!
> > > > > >
> > > > > > My Acked-by has been attached to the MFD portion since v5, which is
> > > > > > why the code hasn't caught my eye before today.  I reviewed the
> > > > > > relocation of the *binding document* (serial => mfd with no changes)
> > > > > > in v4 and nothing else.  It appears as though you mistakenly added 
> > > > > > it
> > > > > > to the *MFD driver* instead.  This explains my confusion in v10 
> > > > > > when I
> > > > > > told you I'd already reviewed the binding document.
> > > > > >
> > > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > > that I do not like what I see.  Besides the missing header file and
> > > > > > the whitespace tabbing errors, I do not agree with the 
> > > > > > implementation.
> > > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > > use-case.
> > > > > >
> > > > > > What's stopping you from just using the compatible string directly 
> > > > > > to
> > > > > > select which driver you need to probe?
> > > > > >
> > > > >
> > > > > Then you'd have multiple compatible strings for the same IP which is a
> > > > > big no-no.
> > > >
> > > > It's still the same hardware device, isn't?
> > > > What if the SPI or UART slave is not on-board, but on an expansion 
> > > > board?
> > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > >
> > > > Hence shouldn't the software derive the hardware mode from the full
> > > > hardware description in DT? If that's impossible (I didn't look into 
> > > > detail
> > > > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > > > a mode property should be added?
> > >
> > > Yes, this is exactly what is done:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> >
> > OK.
> >
> > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > which thus requires having additional compatible values?
> >
> > I think those can just be removed. AFAICS, the SPI and serial drivers 
> > already
> > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform 
> > device
> > names?
>
> The hackish part of this driver is that it's using MFD for something
> which is clearly not an MFD.  It's a USART device.  Nothing more,
> nothing less.
>
> Does anyone have the datasheet to hand?

I haven't read it, but I believe it's not unlike Renesas SCIF, which is
served by both drivers/tty/serial/sh-sci.c and drivers/spi/spi-sh-sci.c.
But the latter is not used from DT, so we haven't experienced (and solved)
the similar issue yet.

Would it work if the 

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

2018-09-11 Thread Geert Uytterhoeven
Hi Lee,

On Tue, Sep 11, 2018 at 8:40 PM Lee Jones  wrote:
> On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> >  wrote:
> > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > >  wrote:
> > > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > > Radu Pirea (6):
> > > > > > >   MAINTAINERS: add at91 usart mfd driver
> > > > > > >   dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > >   mfd: at91-usart: added mfd driver for usart
> > > > > > >   MAINTAINERS: add at91 usart spi driver
> > > > > > >   spi: at91-usart: add driver for at91-usart as spi
> > > > > > >   tty/serial: atmel: change the driver to work under at91-usart 
> > > > > > > mfd
> > > > > > >
> > > > > > >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> > > > > > >  MAINTAINERS   |  16 +
> > > > > > >  drivers/mfd/Kconfig   |   9 +
> > > > > > >  drivers/mfd/Makefile  |   1 +
> > > > > > >  drivers/mfd/at91-usart.c  |  71 +++
> > > > > > >  drivers/spi/Kconfig   |   8 +
> > > > > > >  drivers/spi/Makefile  |   1 +
> > > > > > >  drivers/spi/spi-at91-usart.c  | 432 
> > > > > > > ++
> > > > > > >  drivers/tty/serial/Kconfig|   1 +
> > > > > > >  drivers/tty/serial/atmel_serial.c |  42 +-
> > > > > > >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> > > > > > >  11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > >  rename Documentation/devicetree/bindings/{serial => 
> > > > > > > mfd}/atmel-usart.txt (76%)
> > > > > > >  create mode 100644 drivers/mfd/at91-usart.c
> > > > > > >  create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > > >
> > > > > > Seeing as this patch-set has caused some issues this morning, I took
> > > > > > the liberty to peruse back into its history to figure out where 
> > > > > > things
> > > > > > started to go wrong.  I also re-reviewed the MFD driver - and I'm 
> > > > > > glad
> > > > > > I did!
> > > > > >
> > > > > > My Acked-by has been attached to the MFD portion since v5, which is
> > > > > > why the code hasn't caught my eye before today.  I reviewed the
> > > > > > relocation of the *binding document* (serial => mfd with no changes)
> > > > > > in v4 and nothing else.  It appears as though you mistakenly added 
> > > > > > it
> > > > > > to the *MFD driver* instead.  This explains my confusion in v10 
> > > > > > when I
> > > > > > told you I'd already reviewed the binding document.
> > > > > >
> > > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > > that I do not like what I see.  Besides the missing header file and
> > > > > > the whitespace tabbing errors, I do not agree with the 
> > > > > > implementation.
> > > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > > use-case.
> > > > > >
> > > > > > What's stopping you from just using the compatible string directly 
> > > > > > to
> > > > > > select which driver you need to probe?
> > > > > >
> > > > >
> > > > > Then you'd have multiple compatible strings for the same IP which is a
> > > > > big no-no.
> > > >
> > > > It's still the same hardware device, isn't?
> > > > What if the SPI or UART slave is not on-board, but on an expansion 
> > > > board?
> > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > >
> > > > Hence shouldn't the software derive the hardware mode from the full
> > > > hardware description in DT? If that's impossible (I didn't look into 
> > > > detail
> > > > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > > > a mode property should be added?
> > >
> > > Yes, this is exactly what is done:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> >
> > OK.
> >
> > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > which thus requires having additional compatible values?
> >
> > I think those can just be removed. AFAICS, the SPI and serial drivers 
> > already
> > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform 
> > device
> > names?
>
> The hackish part of this driver is that it's using MFD for something
> which is clearly not an MFD.  It's a USART device.  Nothing more,
> nothing less.
>
> Does anyone have the datasheet to hand?

I haven't read it, but I believe it's not unlike Renesas SCIF, which is
served by both drivers/tty/serial/sh-sci.c and drivers/spi/spi-sh-sci.c.
But the latter is not used from DT, so we haven't experienced (and solved)
the similar issue yet.

Would it work if the 

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

2018-09-11 Thread Alexandre Belloni
On 11/09/2018 19:39:30+0100, Lee Jones wrote:
> On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> 
> > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> >  wrote:
> > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > >  wrote:
> > > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > > Radu Pirea (6):
> > > > > > >   MAINTAINERS: add at91 usart mfd driver
> > > > > > >   dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > >   mfd: at91-usart: added mfd driver for usart
> > > > > > >   MAINTAINERS: add at91 usart spi driver
> > > > > > >   spi: at91-usart: add driver for at91-usart as spi
> > > > > > >   tty/serial: atmel: change the driver to work under at91-usart 
> > > > > > > mfd
> > > > > > >
> > > > > > >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> > > > > > >  MAINTAINERS   |  16 +
> > > > > > >  drivers/mfd/Kconfig   |   9 +
> > > > > > >  drivers/mfd/Makefile  |   1 +
> > > > > > >  drivers/mfd/at91-usart.c  |  71 +++
> > > > > > >  drivers/spi/Kconfig   |   8 +
> > > > > > >  drivers/spi/Makefile  |   1 +
> > > > > > >  drivers/spi/spi-at91-usart.c  | 432 
> > > > > > > ++
> > > > > > >  drivers/tty/serial/Kconfig|   1 +
> > > > > > >  drivers/tty/serial/atmel_serial.c |  42 +-
> > > > > > >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> > > > > > >  11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > >  rename Documentation/devicetree/bindings/{serial => 
> > > > > > > mfd}/atmel-usart.txt (76%)
> > > > > > >  create mode 100644 drivers/mfd/at91-usart.c
> > > > > > >  create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > > >
> > > > > > Seeing as this patch-set has caused some issues this morning, I took
> > > > > > the liberty to peruse back into its history to figure out where 
> > > > > > things
> > > > > > started to go wrong.  I also re-reviewed the MFD driver - and I'm 
> > > > > > glad
> > > > > > I did!
> > > > > >
> > > > > > My Acked-by has been attached to the MFD portion since v5, which is
> > > > > > why the code hasn't caught my eye before today.  I reviewed the
> > > > > > relocation of the *binding document* (serial => mfd with no changes)
> > > > > > in v4 and nothing else.  It appears as though you mistakenly added 
> > > > > > it
> > > > > > to the *MFD driver* instead.  This explains my confusion in v10 
> > > > > > when I
> > > > > > told you I'd already reviewed the binding document.
> > > > > >
> > > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > > that I do not like what I see.  Besides the missing header file and
> > > > > > the whitespace tabbing errors, I do not agree with the 
> > > > > > implementation.
> > > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > > use-case.
> > > > > >
> > > > > > What's stopping you from just using the compatible string directly 
> > > > > > to
> > > > > > select which driver you need to probe?
> > > > > >
> > > > >
> > > > > Then you'd have multiple compatible strings for the same IP which is a
> > > > > big no-no.
> > > >
> > > > It's still the same hardware device, isn't?
> > > > What if the SPI or UART slave is not on-board, but on an expansion 
> > > > board?
> > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > >
> > > > Hence shouldn't the software derive the hardware mode from the full
> > > > hardware description in DT? If that's impossible (I didn't look into 
> > > > detail
> > > > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > > > a mode property should be added?
> > >
> > > Yes, this is exactly what is done:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> > 
> > OK.
> > 
> > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > which thus requires having additional compatible values?
> > 
> > I think those can just be removed. AFAICS, the SPI and serial drivers 
> > already
> > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform 
> > device
> > names?
> 
> The hackish part of this driver is that it's using MFD for something
> which is clearly not an MFD.  It's a USART device.  Nothing more,
> nothing less.
> 
> Does anyone have the datasheet to hand?
> 

It is not a simple usart, it is either a usart or a full blown SPI
controller with registers changing layout depending on the selected
mode. Otherwise, I'm not sure how you would get a USART to do SPI.

Datasheet here:


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

2018-09-11 Thread Alexandre Belloni
On 11/09/2018 19:39:30+0100, Lee Jones wrote:
> On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> 
> > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> >  wrote:
> > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > >  wrote:
> > > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > > Radu Pirea (6):
> > > > > > >   MAINTAINERS: add at91 usart mfd driver
> > > > > > >   dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > >   mfd: at91-usart: added mfd driver for usart
> > > > > > >   MAINTAINERS: add at91 usart spi driver
> > > > > > >   spi: at91-usart: add driver for at91-usart as spi
> > > > > > >   tty/serial: atmel: change the driver to work under at91-usart 
> > > > > > > mfd
> > > > > > >
> > > > > > >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> > > > > > >  MAINTAINERS   |  16 +
> > > > > > >  drivers/mfd/Kconfig   |   9 +
> > > > > > >  drivers/mfd/Makefile  |   1 +
> > > > > > >  drivers/mfd/at91-usart.c  |  71 +++
> > > > > > >  drivers/spi/Kconfig   |   8 +
> > > > > > >  drivers/spi/Makefile  |   1 +
> > > > > > >  drivers/spi/spi-at91-usart.c  | 432 
> > > > > > > ++
> > > > > > >  drivers/tty/serial/Kconfig|   1 +
> > > > > > >  drivers/tty/serial/atmel_serial.c |  42 +-
> > > > > > >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> > > > > > >  11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > >  rename Documentation/devicetree/bindings/{serial => 
> > > > > > > mfd}/atmel-usart.txt (76%)
> > > > > > >  create mode 100644 drivers/mfd/at91-usart.c
> > > > > > >  create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > > >
> > > > > > Seeing as this patch-set has caused some issues this morning, I took
> > > > > > the liberty to peruse back into its history to figure out where 
> > > > > > things
> > > > > > started to go wrong.  I also re-reviewed the MFD driver - and I'm 
> > > > > > glad
> > > > > > I did!
> > > > > >
> > > > > > My Acked-by has been attached to the MFD portion since v5, which is
> > > > > > why the code hasn't caught my eye before today.  I reviewed the
> > > > > > relocation of the *binding document* (serial => mfd with no changes)
> > > > > > in v4 and nothing else.  It appears as though you mistakenly added 
> > > > > > it
> > > > > > to the *MFD driver* instead.  This explains my confusion in v10 
> > > > > > when I
> > > > > > told you I'd already reviewed the binding document.
> > > > > >
> > > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > > that I do not like what I see.  Besides the missing header file and
> > > > > > the whitespace tabbing errors, I do not agree with the 
> > > > > > implementation.
> > > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > > use-case.
> > > > > >
> > > > > > What's stopping you from just using the compatible string directly 
> > > > > > to
> > > > > > select which driver you need to probe?
> > > > > >
> > > > >
> > > > > Then you'd have multiple compatible strings for the same IP which is a
> > > > > big no-no.
> > > >
> > > > It's still the same hardware device, isn't?
> > > > What if the SPI or UART slave is not on-board, but on an expansion 
> > > > board?
> > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > >
> > > > Hence shouldn't the software derive the hardware mode from the full
> > > > hardware description in DT? If that's impossible (I didn't look into 
> > > > detail
> > > > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > > > a mode property should be added?
> > >
> > > Yes, this is exactly what is done:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> > 
> > OK.
> > 
> > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > which thus requires having additional compatible values?
> > 
> > I think those can just be removed. AFAICS, the SPI and serial drivers 
> > already
> > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform 
> > device
> > names?
> 
> The hackish part of this driver is that it's using MFD for something
> which is clearly not an MFD.  It's a USART device.  Nothing more,
> nothing less.
> 
> Does anyone have the datasheet to hand?
> 

It is not a simple usart, it is either a usart or a full blown SPI
controller with registers changing layout depending on the selected
mode. Otherwise, I'm not sure how you would get a USART to do SPI.

Datasheet here:


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

2018-09-11 Thread Lee Jones
On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:

> On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
>  wrote:
> > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > >  wrote:
> > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > Radu Pirea (6):
> > > > > >   MAINTAINERS: add at91 usart mfd driver
> > > > > >   dt-bindings: add binding for atmel-usart in SPI mode
> > > > > >   mfd: at91-usart: added mfd driver for usart
> > > > > >   MAINTAINERS: add at91 usart spi driver
> > > > > >   spi: at91-usart: add driver for at91-usart as spi
> > > > > >   tty/serial: atmel: change the driver to work under at91-usart mfd
> > > > > >
> > > > > >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> > > > > >  MAINTAINERS   |  16 +
> > > > > >  drivers/mfd/Kconfig   |   9 +
> > > > > >  drivers/mfd/Makefile  |   1 +
> > > > > >  drivers/mfd/at91-usart.c  |  71 +++
> > > > > >  drivers/spi/Kconfig   |   8 +
> > > > > >  drivers/spi/Makefile  |   1 +
> > > > > >  drivers/spi/spi-at91-usart.c  | 432 
> > > > > > ++
> > > > > >  drivers/tty/serial/Kconfig|   1 +
> > > > > >  drivers/tty/serial/atmel_serial.c |  42 +-
> > > > > >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> > > > > >  11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > >  rename Documentation/devicetree/bindings/{serial => 
> > > > > > mfd}/atmel-usart.txt (76%)
> > > > > >  create mode 100644 drivers/mfd/at91-usart.c
> > > > > >  create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > >
> > > > > Seeing as this patch-set has caused some issues this morning, I took
> > > > > the liberty to peruse back into its history to figure out where things
> > > > > started to go wrong.  I also re-reviewed the MFD driver - and I'm glad
> > > > > I did!
> > > > >
> > > > > My Acked-by has been attached to the MFD portion since v5, which is
> > > > > why the code hasn't caught my eye before today.  I reviewed the
> > > > > relocation of the *binding document* (serial => mfd with no changes)
> > > > > in v4 and nothing else.  It appears as though you mistakenly added it
> > > > > to the *MFD driver* instead.  This explains my confusion in v10 when I
> > > > > told you I'd already reviewed the binding document.
> > > > >
> > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > that I do not like what I see.  Besides the missing header file and
> > > > > the whitespace tabbing errors, I do not agree with the implementation.
> > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > use-case.
> > > > >
> > > > > What's stopping you from just using the compatible string directly to
> > > > > select which driver you need to probe?
> > > > >
> > > >
> > > > Then you'd have multiple compatible strings for the same IP which is a
> > > > big no-no.
> > >
> > > It's still the same hardware device, isn't?
> > > What if the SPI or UART slave is not on-board, but on an expansion board?
> > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > >
> > > Hence shouldn't the software derive the hardware mode from the full
> > > hardware description in DT? If that's impossible (I didn't look into 
> > > detail
> > > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > > a mode property should be added?
> >
> > Yes, this is exactly what is done:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> 
> OK.
> 
> I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> which thus requires having additional compatible values?
> 
> I think those can just be removed. AFAICS, the SPI and serial drivers already
> match against the "at91_usart_spi" resp. "atmel_usart_serial" platform device
> names?

The hackish part of this driver is that it's using MFD for something
which is clearly not an MFD.  It's a USART device.  Nothing more,
nothing less.

Does anyone have the datasheet to hand?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

2018-09-11 Thread Lee Jones
On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:

> On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
>  wrote:
> > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > >  wrote:
> > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > Radu Pirea (6):
> > > > > >   MAINTAINERS: add at91 usart mfd driver
> > > > > >   dt-bindings: add binding for atmel-usart in SPI mode
> > > > > >   mfd: at91-usart: added mfd driver for usart
> > > > > >   MAINTAINERS: add at91 usart spi driver
> > > > > >   spi: at91-usart: add driver for at91-usart as spi
> > > > > >   tty/serial: atmel: change the driver to work under at91-usart mfd
> > > > > >
> > > > > >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> > > > > >  MAINTAINERS   |  16 +
> > > > > >  drivers/mfd/Kconfig   |   9 +
> > > > > >  drivers/mfd/Makefile  |   1 +
> > > > > >  drivers/mfd/at91-usart.c  |  71 +++
> > > > > >  drivers/spi/Kconfig   |   8 +
> > > > > >  drivers/spi/Makefile  |   1 +
> > > > > >  drivers/spi/spi-at91-usart.c  | 432 
> > > > > > ++
> > > > > >  drivers/tty/serial/Kconfig|   1 +
> > > > > >  drivers/tty/serial/atmel_serial.c |  42 +-
> > > > > >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> > > > > >  11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > >  rename Documentation/devicetree/bindings/{serial => 
> > > > > > mfd}/atmel-usart.txt (76%)
> > > > > >  create mode 100644 drivers/mfd/at91-usart.c
> > > > > >  create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > >
> > > > > Seeing as this patch-set has caused some issues this morning, I took
> > > > > the liberty to peruse back into its history to figure out where things
> > > > > started to go wrong.  I also re-reviewed the MFD driver - and I'm glad
> > > > > I did!
> > > > >
> > > > > My Acked-by has been attached to the MFD portion since v5, which is
> > > > > why the code hasn't caught my eye before today.  I reviewed the
> > > > > relocation of the *binding document* (serial => mfd with no changes)
> > > > > in v4 and nothing else.  It appears as though you mistakenly added it
> > > > > to the *MFD driver* instead.  This explains my confusion in v10 when I
> > > > > told you I'd already reviewed the binding document.
> > > > >
> > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > that I do not like what I see.  Besides the missing header file and
> > > > > the whitespace tabbing errors, I do not agree with the implementation.
> > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > use-case.
> > > > >
> > > > > What's stopping you from just using the compatible string directly to
> > > > > select which driver you need to probe?
> > > > >
> > > >
> > > > Then you'd have multiple compatible strings for the same IP which is a
> > > > big no-no.
> > >
> > > It's still the same hardware device, isn't?
> > > What if the SPI or UART slave is not on-board, but on an expansion board?
> > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > >
> > > Hence shouldn't the software derive the hardware mode from the full
> > > hardware description in DT? If that's impossible (I didn't look into 
> > > detail
> > > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > > a mode property should be added?
> >
> > Yes, this is exactly what is done:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> 
> OK.
> 
> I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> which thus requires having additional compatible values?
> 
> I think those can just be removed. AFAICS, the SPI and serial drivers already
> match against the "at91_usart_spi" resp. "atmel_usart_serial" platform device
> names?

The hackish part of this driver is that it's using MFD for something
which is clearly not an MFD.  It's a USART device.  Nothing more,
nothing less.

Does anyone have the datasheet to hand?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

2018-09-11 Thread Geert Uytterhoeven
On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
 wrote:
> On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> >  wrote:
> > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > Radu Pirea (6):
> > > > >   MAINTAINERS: add at91 usart mfd driver
> > > > >   dt-bindings: add binding for atmel-usart in SPI mode
> > > > >   mfd: at91-usart: added mfd driver for usart
> > > > >   MAINTAINERS: add at91 usart spi driver
> > > > >   spi: at91-usart: add driver for at91-usart as spi
> > > > >   tty/serial: atmel: change the driver to work under at91-usart mfd
> > > > >
> > > > >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> > > > >  MAINTAINERS   |  16 +
> > > > >  drivers/mfd/Kconfig   |   9 +
> > > > >  drivers/mfd/Makefile  |   1 +
> > > > >  drivers/mfd/at91-usart.c  |  71 +++
> > > > >  drivers/spi/Kconfig   |   8 +
> > > > >  drivers/spi/Makefile  |   1 +
> > > > >  drivers/spi/spi-at91-usart.c  | 432 
> > > > > ++
> > > > >  drivers/tty/serial/Kconfig|   1 +
> > > > >  drivers/tty/serial/atmel_serial.c |  42 +-
> > > > >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> > > > >  11 files changed, 606 insertions(+), 17 deletions(-)
> > > > >  rename Documentation/devicetree/bindings/{serial => 
> > > > > mfd}/atmel-usart.txt (76%)
> > > > >  create mode 100644 drivers/mfd/at91-usart.c
> > > > >  create mode 100644 drivers/spi/spi-at91-usart.c
> > > > >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > >
> > > > Seeing as this patch-set has caused some issues this morning, I took
> > > > the liberty to peruse back into its history to figure out where things
> > > > started to go wrong.  I also re-reviewed the MFD driver - and I'm glad
> > > > I did!
> > > >
> > > > My Acked-by has been attached to the MFD portion since v5, which is
> > > > why the code hasn't caught my eye before today.  I reviewed the
> > > > relocation of the *binding document* (serial => mfd with no changes)
> > > > in v4 and nothing else.  It appears as though you mistakenly added it
> > > > to the *MFD driver* instead.  This explains my confusion in v10 when I
> > > > told you I'd already reviewed the binding document.
> > > >
> > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > that I do not like what I see.  Besides the missing header file and
> > > > the whitespace tabbing errors, I do not agree with the implementation.
> > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > use-case.
> > > >
> > > > What's stopping you from just using the compatible string directly to
> > > > select which driver you need to probe?
> > > >
> > >
> > > Then you'd have multiple compatible strings for the same IP which is a
> > > big no-no.
> >
> > It's still the same hardware device, isn't?
> > What if the SPI or UART slave is not on-board, but on an expansion board?
> > Then the SoC-specific .dtsi has no idea what mode should be used.
> >
> > Hence shouldn't the software derive the hardware mode from the full
> > hardware description in DT? If that's impossible (I didn't look into detail
> > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > a mode property should be added?
>
> Yes, this is exactly what is done:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33

OK.

I guess the main "hackish" part is that the mfd_cell uses of_compatible,
which thus requires having additional compatible values?

I think those can just be removed. AFAICS, the SPI and serial drivers already
match against the "at91_usart_spi" resp. "atmel_usart_serial" platform device
names?

> Only one compatbile for the IP and a property to know what is the mode.
> That property should indeed be set in the board dts and not the SoC
> dtsi.
>
> the other, less robust alternative was to look for child nodes and
> decide that if some where present it would indicate an SPI bus. But I
> think at some point we may have child nodes under a UART node.

Indeed, using the "new" serial bus.

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds


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

2018-09-11 Thread Geert Uytterhoeven
On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
 wrote:
> On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> >  wrote:
> > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > Radu Pirea (6):
> > > > >   MAINTAINERS: add at91 usart mfd driver
> > > > >   dt-bindings: add binding for atmel-usart in SPI mode
> > > > >   mfd: at91-usart: added mfd driver for usart
> > > > >   MAINTAINERS: add at91 usart spi driver
> > > > >   spi: at91-usart: add driver for at91-usart as spi
> > > > >   tty/serial: atmel: change the driver to work under at91-usart mfd
> > > > >
> > > > >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> > > > >  MAINTAINERS   |  16 +
> > > > >  drivers/mfd/Kconfig   |   9 +
> > > > >  drivers/mfd/Makefile  |   1 +
> > > > >  drivers/mfd/at91-usart.c  |  71 +++
> > > > >  drivers/spi/Kconfig   |   8 +
> > > > >  drivers/spi/Makefile  |   1 +
> > > > >  drivers/spi/spi-at91-usart.c  | 432 
> > > > > ++
> > > > >  drivers/tty/serial/Kconfig|   1 +
> > > > >  drivers/tty/serial/atmel_serial.c |  42 +-
> > > > >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> > > > >  11 files changed, 606 insertions(+), 17 deletions(-)
> > > > >  rename Documentation/devicetree/bindings/{serial => 
> > > > > mfd}/atmel-usart.txt (76%)
> > > > >  create mode 100644 drivers/mfd/at91-usart.c
> > > > >  create mode 100644 drivers/spi/spi-at91-usart.c
> > > > >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > >
> > > > Seeing as this patch-set has caused some issues this morning, I took
> > > > the liberty to peruse back into its history to figure out where things
> > > > started to go wrong.  I also re-reviewed the MFD driver - and I'm glad
> > > > I did!
> > > >
> > > > My Acked-by has been attached to the MFD portion since v5, which is
> > > > why the code hasn't caught my eye before today.  I reviewed the
> > > > relocation of the *binding document* (serial => mfd with no changes)
> > > > in v4 and nothing else.  It appears as though you mistakenly added it
> > > > to the *MFD driver* instead.  This explains my confusion in v10 when I
> > > > told you I'd already reviewed the binding document.
> > > >
> > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > that I do not like what I see.  Besides the missing header file and
> > > > the whitespace tabbing errors, I do not agree with the implementation.
> > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > use-case.
> > > >
> > > > What's stopping you from just using the compatible string directly to
> > > > select which driver you need to probe?
> > > >
> > >
> > > Then you'd have multiple compatible strings for the same IP which is a
> > > big no-no.
> >
> > It's still the same hardware device, isn't?
> > What if the SPI or UART slave is not on-board, but on an expansion board?
> > Then the SoC-specific .dtsi has no idea what mode should be used.
> >
> > Hence shouldn't the software derive the hardware mode from the full
> > hardware description in DT? If that's impossible (I didn't look into detail
> > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > a mode property should be added?
>
> Yes, this is exactly what is done:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33

OK.

I guess the main "hackish" part is that the mfd_cell uses of_compatible,
which thus requires having additional compatible values?

I think those can just be removed. AFAICS, the SPI and serial drivers already
match against the "at91_usart_spi" resp. "atmel_usart_serial" platform device
names?

> Only one compatbile for the IP and a property to know what is the mode.
> That property should indeed be set in the board dts and not the SoC
> dtsi.
>
> the other, less robust alternative was to look for child nodes and
> decide that if some where present it would indicate an SPI bus. But I
> think at some point we may have child nodes under a UART node.

Indeed, using the "new" serial bus.

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds


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

2018-09-11 Thread Alexandre Belloni
On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> Hi Alexandre,
> 
> On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
>  wrote:
> > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > Radu Pirea (6):
> > > >   MAINTAINERS: add at91 usart mfd driver
> > > >   dt-bindings: add binding for atmel-usart in SPI mode
> > > >   mfd: at91-usart: added mfd driver for usart
> > > >   MAINTAINERS: add at91 usart spi driver
> > > >   spi: at91-usart: add driver for at91-usart as spi
> > > >   tty/serial: atmel: change the driver to work under at91-usart mfd
> > > >
> > > >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> > > >  MAINTAINERS   |  16 +
> > > >  drivers/mfd/Kconfig   |   9 +
> > > >  drivers/mfd/Makefile  |   1 +
> > > >  drivers/mfd/at91-usart.c  |  71 +++
> > > >  drivers/spi/Kconfig   |   8 +
> > > >  drivers/spi/Makefile  |   1 +
> > > >  drivers/spi/spi-at91-usart.c  | 432 ++
> > > >  drivers/tty/serial/Kconfig|   1 +
> > > >  drivers/tty/serial/atmel_serial.c |  42 +-
> > > >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> > > >  11 files changed, 606 insertions(+), 17 deletions(-)
> > > >  rename Documentation/devicetree/bindings/{serial => 
> > > > mfd}/atmel-usart.txt (76%)
> > > >  create mode 100644 drivers/mfd/at91-usart.c
> > > >  create mode 100644 drivers/spi/spi-at91-usart.c
> > > >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > >
> > > Seeing as this patch-set has caused some issues this morning, I took
> > > the liberty to peruse back into its history to figure out where things
> > > started to go wrong.  I also re-reviewed the MFD driver - and I'm glad
> > > I did!
> > >
> > > My Acked-by has been attached to the MFD portion since v5, which is
> > > why the code hasn't caught my eye before today.  I reviewed the
> > > relocation of the *binding document* (serial => mfd with no changes)
> > > in v4 and nothing else.  It appears as though you mistakenly added it
> > > to the *MFD driver* instead.  This explains my confusion in v10 when I
> > > told you I'd already reviewed the binding document.
> > >
> > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > that I do not like what I see.  Besides the missing header file and
> > > the whitespace tabbing errors, I do not agree with the implementation.
> > > Using MFD as a shim to hack around driver selection is not a valid
> > > use-case.
> > >
> > > What's stopping you from just using the compatible string directly to
> > > select which driver you need to probe?
> > >
> >
> > Then you'd have multiple compatible strings for the same IP which is a
> > big no-no.
> 
> It's still the same hardware device, isn't?
> What if the SPI or UART slave is not on-board, but on an expansion board?
> Then the SoC-specific .dtsi has no idea what mode should be used.
> 
> Hence shouldn't the software derive the hardware mode from the full
> hardware description in DT? If that's impossible (I didn't look into detail
> whether an SPI bus can easily be distinguished from a UART bus), perhaps
> a mode property should be added?
> 

Yes, this is exactly what is done:

https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33

Only one compatbile for the IP and a property to know what is the mode.
That property should indeed be set in the board dts and not the SoC
dtsi.

the other, less robust alternative was to look for child nodes and
decide that if some where present it would indicate an SPI bus. But I
think at some point we may have child nodes under a UART node.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com


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

2018-09-11 Thread Alexandre Belloni
On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> Hi Alexandre,
> 
> On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
>  wrote:
> > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > Radu Pirea (6):
> > > >   MAINTAINERS: add at91 usart mfd driver
> > > >   dt-bindings: add binding for atmel-usart in SPI mode
> > > >   mfd: at91-usart: added mfd driver for usart
> > > >   MAINTAINERS: add at91 usart spi driver
> > > >   spi: at91-usart: add driver for at91-usart as spi
> > > >   tty/serial: atmel: change the driver to work under at91-usart mfd
> > > >
> > > >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> > > >  MAINTAINERS   |  16 +
> > > >  drivers/mfd/Kconfig   |   9 +
> > > >  drivers/mfd/Makefile  |   1 +
> > > >  drivers/mfd/at91-usart.c  |  71 +++
> > > >  drivers/spi/Kconfig   |   8 +
> > > >  drivers/spi/Makefile  |   1 +
> > > >  drivers/spi/spi-at91-usart.c  | 432 ++
> > > >  drivers/tty/serial/Kconfig|   1 +
> > > >  drivers/tty/serial/atmel_serial.c |  42 +-
> > > >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> > > >  11 files changed, 606 insertions(+), 17 deletions(-)
> > > >  rename Documentation/devicetree/bindings/{serial => 
> > > > mfd}/atmel-usart.txt (76%)
> > > >  create mode 100644 drivers/mfd/at91-usart.c
> > > >  create mode 100644 drivers/spi/spi-at91-usart.c
> > > >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > >
> > > Seeing as this patch-set has caused some issues this morning, I took
> > > the liberty to peruse back into its history to figure out where things
> > > started to go wrong.  I also re-reviewed the MFD driver - and I'm glad
> > > I did!
> > >
> > > My Acked-by has been attached to the MFD portion since v5, which is
> > > why the code hasn't caught my eye before today.  I reviewed the
> > > relocation of the *binding document* (serial => mfd with no changes)
> > > in v4 and nothing else.  It appears as though you mistakenly added it
> > > to the *MFD driver* instead.  This explains my confusion in v10 when I
> > > told you I'd already reviewed the binding document.
> > >
> > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > that I do not like what I see.  Besides the missing header file and
> > > the whitespace tabbing errors, I do not agree with the implementation.
> > > Using MFD as a shim to hack around driver selection is not a valid
> > > use-case.
> > >
> > > What's stopping you from just using the compatible string directly to
> > > select which driver you need to probe?
> > >
> >
> > Then you'd have multiple compatible strings for the same IP which is a
> > big no-no.
> 
> It's still the same hardware device, isn't?
> What if the SPI or UART slave is not on-board, but on an expansion board?
> Then the SoC-specific .dtsi has no idea what mode should be used.
> 
> Hence shouldn't the software derive the hardware mode from the full
> hardware description in DT? If that's impossible (I didn't look into detail
> whether an SPI bus can easily be distinguished from a UART bus), perhaps
> a mode property should be added?
> 

Yes, this is exactly what is done:

https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33

Only one compatbile for the IP and a property to know what is the mode.
That property should indeed be set in the board dts and not the SoC
dtsi.

the other, less robust alternative was to look for child nodes and
decide that if some where present it would indicate an SPI bus. But I
think at some point we may have child nodes under a UART node.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com


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

2018-09-11 Thread Geert Uytterhoeven
Hi Alexandre,

On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
 wrote:
> On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > Radu Pirea (6):
> > >   MAINTAINERS: add at91 usart mfd driver
> > >   dt-bindings: add binding for atmel-usart in SPI mode
> > >   mfd: at91-usart: added mfd driver for usart
> > >   MAINTAINERS: add at91 usart spi driver
> > >   spi: at91-usart: add driver for at91-usart as spi
> > >   tty/serial: atmel: change the driver to work under at91-usart mfd
> > >
> > >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> > >  MAINTAINERS   |  16 +
> > >  drivers/mfd/Kconfig   |   9 +
> > >  drivers/mfd/Makefile  |   1 +
> > >  drivers/mfd/at91-usart.c  |  71 +++
> > >  drivers/spi/Kconfig   |   8 +
> > >  drivers/spi/Makefile  |   1 +
> > >  drivers/spi/spi-at91-usart.c  | 432 ++
> > >  drivers/tty/serial/Kconfig|   1 +
> > >  drivers/tty/serial/atmel_serial.c |  42 +-
> > >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> > >  11 files changed, 606 insertions(+), 17 deletions(-)
> > >  rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt 
> > > (76%)
> > >  create mode 100644 drivers/mfd/at91-usart.c
> > >  create mode 100644 drivers/spi/spi-at91-usart.c
> > >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> >
> > Seeing as this patch-set has caused some issues this morning, I took
> > the liberty to peruse back into its history to figure out where things
> > started to go wrong.  I also re-reviewed the MFD driver - and I'm glad
> > I did!
> >
> > My Acked-by has been attached to the MFD portion since v5, which is
> > why the code hasn't caught my eye before today.  I reviewed the
> > relocation of the *binding document* (serial => mfd with no changes)
> > in v4 and nothing else.  It appears as though you mistakenly added it
> > to the *MFD driver* instead.  This explains my confusion in v10 when I
> > told you I'd already reviewed the binding document.
> >
> > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > that I do not like what I see.  Besides the missing header file and
> > the whitespace tabbing errors, I do not agree with the implementation.
> > Using MFD as a shim to hack around driver selection is not a valid
> > use-case.
> >
> > What's stopping you from just using the compatible string directly to
> > select which driver you need to probe?
> >
>
> Then you'd have multiple compatible strings for the same IP which is a
> big no-no.

It's still the same hardware device, isn't?
What if the SPI or UART slave is not on-board, but on an expansion board?
Then the SoC-specific .dtsi has no idea what mode should be used.

Hence shouldn't the software derive the hardware mode from the full
hardware description in DT? If that's impossible (I didn't look into detail
whether an SPI bus can easily be distinguished from a UART bus), perhaps
a mode property should be added?

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds


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

2018-09-11 Thread Geert Uytterhoeven
Hi Alexandre,

On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
 wrote:
> On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > Radu Pirea (6):
> > >   MAINTAINERS: add at91 usart mfd driver
> > >   dt-bindings: add binding for atmel-usart in SPI mode
> > >   mfd: at91-usart: added mfd driver for usart
> > >   MAINTAINERS: add at91 usart spi driver
> > >   spi: at91-usart: add driver for at91-usart as spi
> > >   tty/serial: atmel: change the driver to work under at91-usart mfd
> > >
> > >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> > >  MAINTAINERS   |  16 +
> > >  drivers/mfd/Kconfig   |   9 +
> > >  drivers/mfd/Makefile  |   1 +
> > >  drivers/mfd/at91-usart.c  |  71 +++
> > >  drivers/spi/Kconfig   |   8 +
> > >  drivers/spi/Makefile  |   1 +
> > >  drivers/spi/spi-at91-usart.c  | 432 ++
> > >  drivers/tty/serial/Kconfig|   1 +
> > >  drivers/tty/serial/atmel_serial.c |  42 +-
> > >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> > >  11 files changed, 606 insertions(+), 17 deletions(-)
> > >  rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt 
> > > (76%)
> > >  create mode 100644 drivers/mfd/at91-usart.c
> > >  create mode 100644 drivers/spi/spi-at91-usart.c
> > >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> >
> > Seeing as this patch-set has caused some issues this morning, I took
> > the liberty to peruse back into its history to figure out where things
> > started to go wrong.  I also re-reviewed the MFD driver - and I'm glad
> > I did!
> >
> > My Acked-by has been attached to the MFD portion since v5, which is
> > why the code hasn't caught my eye before today.  I reviewed the
> > relocation of the *binding document* (serial => mfd with no changes)
> > in v4 and nothing else.  It appears as though you mistakenly added it
> > to the *MFD driver* instead.  This explains my confusion in v10 when I
> > told you I'd already reviewed the binding document.
> >
> > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > that I do not like what I see.  Besides the missing header file and
> > the whitespace tabbing errors, I do not agree with the implementation.
> > Using MFD as a shim to hack around driver selection is not a valid
> > use-case.
> >
> > What's stopping you from just using the compatible string directly to
> > select which driver you need to probe?
> >
>
> Then you'd have multiple compatible strings for the same IP which is a
> big no-no.

It's still the same hardware device, isn't?
What if the SPI or UART slave is not on-board, but on an expansion board?
Then the SoC-specific .dtsi has no idea what mode should be used.

Hence shouldn't the software derive the hardware mode from the full
hardware description in DT? If that's impossible (I didn't look into detail
whether an SPI bus can easily be distinguished from a UART bus), perhaps
a mode property should be added?

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds


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

2018-09-11 Thread Alexandre Belloni
On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> On Tue, 04 Sep 2018, Radu Pirea wrote:
> > Radu Pirea (6):
> >   MAINTAINERS: add at91 usart mfd driver
> >   dt-bindings: add binding for atmel-usart in SPI mode
> >   mfd: at91-usart: added mfd driver for usart
> >   MAINTAINERS: add at91 usart spi driver
> >   spi: at91-usart: add driver for at91-usart as spi
> >   tty/serial: atmel: change the driver to work under at91-usart mfd
> > 
> >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> >  MAINTAINERS   |  16 +
> >  drivers/mfd/Kconfig   |   9 +
> >  drivers/mfd/Makefile  |   1 +
> >  drivers/mfd/at91-usart.c  |  71 +++
> >  drivers/spi/Kconfig   |   8 +
> >  drivers/spi/Makefile  |   1 +
> >  drivers/spi/spi-at91-usart.c  | 432 ++
> >  drivers/tty/serial/Kconfig|   1 +
> >  drivers/tty/serial/atmel_serial.c |  42 +-
> >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> >  11 files changed, 606 insertions(+), 17 deletions(-)
> >  rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt 
> > (76%)
> >  create mode 100644 drivers/mfd/at91-usart.c
> >  create mode 100644 drivers/spi/spi-at91-usart.c
> >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> 
> Seeing as this patch-set has caused some issues this morning, I took
> the liberty to peruse back into its history to figure out where things
> started to go wrong.  I also re-reviewed the MFD driver - and I'm glad
> I did!
> 
> My Acked-by has been attached to the MFD portion since v5, which is
> why the code hasn't caught my eye before today.  I reviewed the
> relocation of the *binding document* (serial => mfd with no changes)
> in v4 and nothing else.  It appears as though you mistakenly added it
> to the *MFD driver* instead.  This explains my confusion in v10 when I
> told you I'd already reviewed the binding document.
> 
> As I said, I have re-reviewed the MFD driver and I'm afraid to say
> that I do not like what I see.  Besides the missing header file and
> the whitespace tabbing errors, I do not agree with the implementation.
> Using MFD as a shim to hack around driver selection is not a valid
> use-case.
> 
> What's stopping you from just using the compatible string directly to
> select which driver you need to probe?
> 

Then you'd have multiple compatible strings for the same IP which is a
big no-no.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com


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

2018-09-11 Thread Alexandre Belloni
On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> On Tue, 04 Sep 2018, Radu Pirea wrote:
> > Radu Pirea (6):
> >   MAINTAINERS: add at91 usart mfd driver
> >   dt-bindings: add binding for atmel-usart in SPI mode
> >   mfd: at91-usart: added mfd driver for usart
> >   MAINTAINERS: add at91 usart spi driver
> >   spi: at91-usart: add driver for at91-usart as spi
> >   tty/serial: atmel: change the driver to work under at91-usart mfd
> > 
> >  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
> >  MAINTAINERS   |  16 +
> >  drivers/mfd/Kconfig   |   9 +
> >  drivers/mfd/Makefile  |   1 +
> >  drivers/mfd/at91-usart.c  |  71 +++
> >  drivers/spi/Kconfig   |   8 +
> >  drivers/spi/Makefile  |   1 +
> >  drivers/spi/spi-at91-usart.c  | 432 ++
> >  drivers/tty/serial/Kconfig|   1 +
> >  drivers/tty/serial/atmel_serial.c |  42 +-
> >  include/dt-bindings/mfd/at91-usart.h  |  17 +
> >  11 files changed, 606 insertions(+), 17 deletions(-)
> >  rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt 
> > (76%)
> >  create mode 100644 drivers/mfd/at91-usart.c
> >  create mode 100644 drivers/spi/spi-at91-usart.c
> >  create mode 100644 include/dt-bindings/mfd/at91-usart.h
> 
> Seeing as this patch-set has caused some issues this morning, I took
> the liberty to peruse back into its history to figure out where things
> started to go wrong.  I also re-reviewed the MFD driver - and I'm glad
> I did!
> 
> My Acked-by has been attached to the MFD portion since v5, which is
> why the code hasn't caught my eye before today.  I reviewed the
> relocation of the *binding document* (serial => mfd with no changes)
> in v4 and nothing else.  It appears as though you mistakenly added it
> to the *MFD driver* instead.  This explains my confusion in v10 when I
> told you I'd already reviewed the binding document.
> 
> As I said, I have re-reviewed the MFD driver and I'm afraid to say
> that I do not like what I see.  Besides the missing header file and
> the whitespace tabbing errors, I do not agree with the implementation.
> Using MFD as a shim to hack around driver selection is not a valid
> use-case.
> 
> What's stopping you from just using the compatible string directly to
> select which driver you need to probe?
> 

Then you'd have multiple compatible strings for the same IP which is a
big no-no.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com


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

2018-09-11 Thread Lee Jones
On Tue, 04 Sep 2018, Radu Pirea wrote:
> Radu Pirea (6):
>   MAINTAINERS: add at91 usart mfd driver
>   dt-bindings: add binding for atmel-usart in SPI mode
>   mfd: at91-usart: added mfd driver for usart
>   MAINTAINERS: add at91 usart spi driver
>   spi: at91-usart: add driver for at91-usart as spi
>   tty/serial: atmel: change the driver to work under at91-usart mfd
> 
>  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
>  MAINTAINERS   |  16 +
>  drivers/mfd/Kconfig   |   9 +
>  drivers/mfd/Makefile  |   1 +
>  drivers/mfd/at91-usart.c  |  71 +++
>  drivers/spi/Kconfig   |   8 +
>  drivers/spi/Makefile  |   1 +
>  drivers/spi/spi-at91-usart.c  | 432 ++
>  drivers/tty/serial/Kconfig|   1 +
>  drivers/tty/serial/atmel_serial.c |  42 +-
>  include/dt-bindings/mfd/at91-usart.h  |  17 +
>  11 files changed, 606 insertions(+), 17 deletions(-)
>  rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt 
> (76%)
>  create mode 100644 drivers/mfd/at91-usart.c
>  create mode 100644 drivers/spi/spi-at91-usart.c
>  create mode 100644 include/dt-bindings/mfd/at91-usart.h

Seeing as this patch-set has caused some issues this morning, I took
the liberty to peruse back into its history to figure out where things
started to go wrong.  I also re-reviewed the MFD driver - and I'm glad
I did!

My Acked-by has been attached to the MFD portion since v5, which is
why the code hasn't caught my eye before today.  I reviewed the
relocation of the *binding document* (serial => mfd with no changes)
in v4 and nothing else.  It appears as though you mistakenly added it
to the *MFD driver* instead.  This explains my confusion in v10 when I
told you I'd already reviewed the binding document.

As I said, I have re-reviewed the MFD driver and I'm afraid to say
that I do not like what I see.  Besides the missing header file and
the whitespace tabbing errors, I do not agree with the implementation.
Using MFD as a shim to hack around driver selection is not a valid
use-case.

What's stopping you from just using the compatible string directly to
select which driver you need to probe?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

2018-09-11 Thread Lee Jones
On Tue, 04 Sep 2018, Radu Pirea wrote:
> Radu Pirea (6):
>   MAINTAINERS: add at91 usart mfd driver
>   dt-bindings: add binding for atmel-usart in SPI mode
>   mfd: at91-usart: added mfd driver for usart
>   MAINTAINERS: add at91 usart spi driver
>   spi: at91-usart: add driver for at91-usart as spi
>   tty/serial: atmel: change the driver to work under at91-usart mfd
> 
>  .../bindings/{serial => mfd}/atmel-usart.txt  |  25 +-
>  MAINTAINERS   |  16 +
>  drivers/mfd/Kconfig   |   9 +
>  drivers/mfd/Makefile  |   1 +
>  drivers/mfd/at91-usart.c  |  71 +++
>  drivers/spi/Kconfig   |   8 +
>  drivers/spi/Makefile  |   1 +
>  drivers/spi/spi-at91-usart.c  | 432 ++
>  drivers/tty/serial/Kconfig|   1 +
>  drivers/tty/serial/atmel_serial.c |  42 +-
>  include/dt-bindings/mfd/at91-usart.h  |  17 +
>  11 files changed, 606 insertions(+), 17 deletions(-)
>  rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt 
> (76%)
>  create mode 100644 drivers/mfd/at91-usart.c
>  create mode 100644 drivers/spi/spi-at91-usart.c
>  create mode 100644 include/dt-bindings/mfd/at91-usart.h

Seeing as this patch-set has caused some issues this morning, I took
the liberty to peruse back into its history to figure out where things
started to go wrong.  I also re-reviewed the MFD driver - and I'm glad
I did!

My Acked-by has been attached to the MFD portion since v5, which is
why the code hasn't caught my eye before today.  I reviewed the
relocation of the *binding document* (serial => mfd with no changes)
in v4 and nothing else.  It appears as though you mistakenly added it
to the *MFD driver* instead.  This explains my confusion in v10 when I
told you I'd already reviewed the binding document.

As I said, I have re-reviewed the MFD driver and I'm afraid to say
that I do not like what I see.  Besides the missing header file and
the whitespace tabbing errors, I do not agree with the implementation.
Using MFD as a shim to hack around driver selection is not a valid
use-case.

What's stopping you from just using the compatible string directly to
select which driver you need to probe?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

2018-09-10 Thread Lee Jones
On Mon, 10 Sep 2018, Nicolas Ferre wrote:

> Hi Lee,
> 
> On 10/09/2018 at 11:48, Lee Jones wrote:
> > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > Well, this is the 12th version of this patch series.
> > > In this version I fixed a warning from kbuild-robot and I have no idea
> > > how I forgot to add static in declaration of that functions. Also I
> > > fixed the example for the SPI driver in bindings.
> > 
> > Okay great.  So which patches still require Acks?
> 
> None I would say: everything's ready to be integrated into your MFD tree as
> you proposed some time ago.
> 
> We are looking forward seeing this series integrated in linux-next as soon
> as possible.

Very well.

I'm just catching up after an extended vacation - only 300 mails to go!

Please bear with me.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

2018-09-10 Thread Lee Jones
On Mon, 10 Sep 2018, Nicolas Ferre wrote:

> Hi Lee,
> 
> On 10/09/2018 at 11:48, Lee Jones wrote:
> > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > Well, this is the 12th version of this patch series.
> > > In this version I fixed a warning from kbuild-robot and I have no idea
> > > how I forgot to add static in declaration of that functions. Also I
> > > fixed the example for the SPI driver in bindings.
> > 
> > Okay great.  So which patches still require Acks?
> 
> None I would say: everything's ready to be integrated into your MFD tree as
> you proposed some time ago.
> 
> We are looking forward seeing this series integrated in linux-next as soon
> as possible.

Very well.

I'm just catching up after an extended vacation - only 300 mails to go!

Please bear with me.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

2018-09-10 Thread Nicolas Ferre

Hi Lee,

On 10/09/2018 at 11:48, Lee Jones wrote:

On Tue, 04 Sep 2018, Radu Pirea wrote:

Well, this is the 12th version of this patch series.
In this version I fixed a warning from kbuild-robot and I have no idea
how I forgot to add static in declaration of that functions. Also I
fixed the example for the SPI driver in bindings.


Okay great.  So which patches still require Acks?


None I would say: everything's ready to be integrated into your MFD tree 
as you proposed some time ago.


We are looking forward seeing this series integrated in linux-next as 
soon as possible.


Best regards,
  Nicolas



Radu Pirea (6):
   MAINTAINERS: add at91 usart mfd driver
   dt-bindings: add binding for atmel-usart in SPI mode
   mfd: at91-usart: added mfd driver for usart
   MAINTAINERS: add at91 usart spi driver
   spi: at91-usart: add driver for at91-usart as spi
   tty/serial: atmel: change the driver to work under at91-usart mfd





--
Nicolas Ferre


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

2018-09-10 Thread Nicolas Ferre

Hi Lee,

On 10/09/2018 at 11:48, Lee Jones wrote:

On Tue, 04 Sep 2018, Radu Pirea wrote:

Well, this is the 12th version of this patch series.
In this version I fixed a warning from kbuild-robot and I have no idea
how I forgot to add static in declaration of that functions. Also I
fixed the example for the SPI driver in bindings.


Okay great.  So which patches still require Acks?


None I would say: everything's ready to be integrated into your MFD tree 
as you proposed some time ago.


We are looking forward seeing this series integrated in linux-next as 
soon as possible.


Best regards,
  Nicolas



Radu Pirea (6):
   MAINTAINERS: add at91 usart mfd driver
   dt-bindings: add binding for atmel-usart in SPI mode
   mfd: at91-usart: added mfd driver for usart
   MAINTAINERS: add at91 usart spi driver
   spi: at91-usart: add driver for at91-usart as spi
   tty/serial: atmel: change the driver to work under at91-usart mfd





--
Nicolas Ferre


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

2018-09-10 Thread Lee Jones
On Tue, 04 Sep 2018, Radu Pirea wrote:
> Well, this is the 12th version of this patch series.
> In this version I fixed a warning from kbuild-robot and I have no idea
> how I forgot to add static in declaration of that functions. Also I
> fixed the example for the SPI driver in bindings.

Okay great.  So which patches still require Acks?

[...]

> Radu Pirea (6):
>   MAINTAINERS: add at91 usart mfd driver
>   dt-bindings: add binding for atmel-usart in SPI mode
>   mfd: at91-usart: added mfd driver for usart
>   MAINTAINERS: add at91 usart spi driver
>   spi: at91-usart: add driver for at91-usart as spi
>   tty/serial: atmel: change the driver to work under at91-usart mfd

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog


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

2018-09-10 Thread Lee Jones
On Tue, 04 Sep 2018, Radu Pirea wrote:
> Well, this is the 12th version of this patch series.
> In this version I fixed a warning from kbuild-robot and I have no idea
> how I forgot to add static in declaration of that functions. Also I
> fixed the example for the SPI driver in bindings.

Okay great.  So which patches still require Acks?

[...]

> Radu Pirea (6):
>   MAINTAINERS: add at91 usart mfd driver
>   dt-bindings: add binding for atmel-usart in SPI mode
>   mfd: at91-usart: added mfd driver for usart
>   MAINTAINERS: add at91 usart spi driver
>   spi: at91-usart: add driver for at91-usart as spi
>   tty/serial: atmel: change the driver to work under at91-usart mfd

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog