Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1503 bytes --]

On Sun, Jul 15, 2012 at 04:17:16PM +0800, Shawn Guo wrote:
> On Fri, Jul 13, 2012 at 10:22:49AM +0200, Wolfram Sang wrote:
> > > +	/*
> > > +	 * TODO: This is a temporary solution and should be changed
> > > +	 * to use generic DMA binding later when the helpers get in.
> > > +	 */
> > 
> > @Shawn: Any idea when this is going to happen? And why do we need this?
> 
> See thread [1] for current statues.  I'm not sure when it's going to
> happen though.

Phew, [1] is a bit too much too read. I will just assume there are still
issues.

> > AFAICT it will be always channel 6/7 on mx28?
> > 
> Yes, but it might be a different channel on mx23.  Just like we define
> IO region and interrupt number in device tree, dma channel is just
> another resource of hardware block that we choose to define in device
> tree.

What makes me wonder now that I come to think of it (not necessarily a
question for Shawn but to all):

If I have an I2C slave with an interrupt line tied to something, GPIO or
external IRQ from the SoC, it makes perfect sense to define that in the
devicetree.

Yet, if I know the compatible property for the mxs I2C driver, and also
know the CPU type (be it MX23 or MX28), I can deduce from that a lot of
information, including DMA channel. That is fix. Why encode it?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> On Sun, Jul 15, 2012 at 04:17:16PM +0800, Shawn Guo wrote:
> > On Fri, Jul 13, 2012 at 10:22:49AM +0200, Wolfram Sang wrote:
> > > > +	/*
> > > > +	 * TODO: This is a temporary solution and should be changed
> > > > +	 * to use generic DMA binding later when the helpers get in.
> > > > +	 */
> > > 
> > > @Shawn: Any idea when this is going to happen? And why do we need this?
> > 
> > See thread [1] for current statues.  I'm not sure when it's going to
> > happen though.
> 
> Phew, [1] is a bit too much too read. I will just assume there are still
> issues.
> 
> > > AFAICT it will be always channel 6/7 on mx28?
> > 
> > Yes, but it might be a different channel on mx23.  Just like we define
> > IO region and interrupt number in device tree, dma channel is just
> > another resource of hardware block that we choose to define in device
> > tree.
> 
> What makes me wonder now that I come to think of it (not necessarily a
> question for Shawn but to all):
> 
> If I have an I2C slave with an interrupt line tied to something, GPIO or
> external IRQ from the SoC, it makes perfect sense to define that in the
> devicetree.
> 
> Yet, if I know the compatible property for the mxs I2C driver, and also
> know the CPU type (be it MX23 or MX28), I can deduce from that a lot of
> information, including DMA channel. That is fix. Why encode it?

You know the compatible and the "fallback compatible". From the later one, you 
can deduce nothing if that happens to kick in.

btw. the PIO discussion on DT discuss is completely ignored. How shall we 
proceed, this driver is stalled for too long.

> Regards,
> 
>    Wolfram

Best regards,
Marek Vasut

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]


> > Yet, if I know the compatible property for the mxs I2C driver, and also
> > know the CPU type (be it MX23 or MX28), I can deduce from that a lot of
> > information, including DMA channel. That is fix. Why encode it?
> 
> You know the compatible and the "fallback compatible". From the later one, you 
> can deduce nothing if that happens to kick in.

Even if the driver was matched because of an MX23-I2C "compatible"
binding, both devicetree and runtime could provide data that it actually
runs on MX28. That shouldn't be a problem.

> btw. the PIO discussion on DT discuss is completely ignored. How shall we 
> proceed, this driver is stalled for too long.

IIRC I mentioned that a discussion about the bindings won't make the
next merge window. That's why I proposed either module_parameter or
dropping the binding entirely as possible inbetween options.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> > > Yet, if I know the compatible property for the mxs I2C driver, and also
> > > know the CPU type (be it MX23 or MX28), I can deduce from that a lot of
> > > information, including DMA channel. That is fix. Why encode it?
> > 
> > You know the compatible and the "fallback compatible". From the later
> > one, you can deduce nothing if that happens to kick in.
> 
> Even if the driver was matched because of an MX23-I2C "compatible"
> binding, both devicetree and runtime could provide data that it actually
> runs on MX28. That shouldn't be a problem.

You mean like ... cpu_is_mx28() ? We got rid of that in favor of DT.

> > btw. the PIO discussion on DT discuss is completely ignored. How shall we
> > proceed, this driver is stalled for too long.
> 
> IIRC I mentioned that a discussion about the bindings won't make the
> next merge window.

Yet another merge window, I have to mention. And only because very long pauses 
inbetween reviews and very minor nitpicks. I'm being annoyed by this patch so 
much I'm thinking of giving up on this. I wasted too much of my free time on 
this and the result is as is.

> That's why I proposed either module_parameter

Which I explained is not a way to go.

> or
> dropping the binding entirely as possible inbetween options.

Which is not an option either. And this discussion is only further stalling the 
patch.

We're adding fsl,something properties all over the DT all the time, yet this one 
is of concern?

Best regards,
Marek Vasut

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2275 bytes --]


> > Even if the driver was matched because of an MX23-I2C "compatible"
> > binding, both devicetree and runtime could provide data that it actually
> > runs on MX28. That shouldn't be a problem.
> 
> You mean like ... cpu_is_mx28() ? We got rid of that in favor of DT.

Might be. But the information is probably somewhere.

> > IIRC I mentioned that a discussion about the bindings won't make the
> > next merge window.
> 
> Yet another merge window, I have to mention. And only because very long pauses 
> inbetween reviews and very minor nitpicks. I'm being annoyed by this patch so 
> much I'm thinking of giving up on this. I wasted too much of my free time on 
> this and the result is as is.

For you it might be a minor nitpick, for me (as a maintainer) it is not.
You have to deal with just one binding, I have to deal with many. And
since they have to be supported forever, this can easily mess up code
and make the subsystem clumsy and whatnot.

> > That's why I proposed either module_parameter
> 
> Which I explained is not a way to go.

That's why I called it inbetween solution so the patch could go in.
It's fine if you don't like it, I prefer dropping the binding as well.

> > or
> > dropping the binding entirely as possible inbetween options.
> 
> Which is not an option either.

It would enable you to add the binding as an out-of-tree patch.

> And this discussion is only further stalling the 
> patch.


> We're adding fsl,something properties all over the DT all the time, yet this one 
> is of concern?

Yes. Adding all these properties is IMO not the right way, and I have
the impression they often came in because of time pressure like this. If
I think it is wrong for the kernel, I have to reject a patch unless I am
convinced otherwise. Which did not happen yet; as you found out
discussions on devicetree-discuss are slow. Might be another indication
that devictree things happen too much at the time currently. This is not
specific to your patch, there are more which need discussion or had to
be reworked.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Shawn Guo]

On Sat, Jul 21, 2012 at 02:44:06PM +0200, Wolfram Sang wrote:
> Yet, if I know the compatible property for the mxs I2C driver, and also
> know the CPU type (be it MX23 or MX28), I can deduce from that a lot of
> information, including DMA channel. That is fix. Why encode it?
> 
The driver shouldn't know the CPU type but just the IP type/version.
We can definitely have "fsl,imx23-i2c" and "fsl,imx28-i2c" compatible
strings for iMX23 and iMX28 respectively if the I2C controller on these
two SoC differs on the IP inside aspects.  But we shouldn't do that
for IO region, interrupt number, DMA channel such differences at SoC
integration level.

-- 
Regards,
Shawn