Re: [PATCH v2 06/10] soc: fsl: qe: Add support for QMC

Le 11/01/2023 à 16:40, Herve Codina a écrit :

Hi Christophe,
On Wed, 11 Jan 2023 13:58:03 +0000
Christophe Leroy [off-list ref] wrote:

quoted

Le 06/01/2023 à 17:37, Herve Codina a écrit :

quoted

The QMC (QUICC Multichannel Controller) is available on some
PowerQUICC SoC such as the MPC885 or MPC866.

Same, that's QMC for CPM for the time being, should that be made clear
in the subject ?

Also same answer:
Will changing the subject be enough or do I need to move it to new
created drivers/soc/fsl/cpm/ directory ?

quoted

QE also has QMC and this driver might be used for QE QMC as well with
some enhancement, maybe that could be explained here ?

Yes and we plan to have it working on QE too.

What do you think if I add the following:
   The QMC is also available on some Quicc Engine SoC.
   This current version support CPM1 SoC only. Some
   enhancement are needed to support Quicc Engine SoC.

That's look good.

quoted

I see you are using in_be16(), out_be16() etc ....
That's specific to powerpc arch, maybe it is better to use ioread16be,
iowrite16be() etc ....
See commit 3f39f38ea91d ("soc: fsl: qe: replace qe_io{read,write}*
wrappers by generic io{read,write}*") and commit 6ac9b61786cc ("soc:
fsl: qe: introduce qe_io{read,write}* wrappers") for information.

You're right.
I will switch to io{read,write}{16,32}be.

I prefer avoid the use of qe_* wrappers as the driver is not
(yet) compatible with QE.

By the way that was the purpose of commit 3f39f38ea91d ("soc: fsl: qe: 
replace qe_io{read,write}* wrappers by generic io{read,write}*"), 
replace qe_* wrappers by direct use of io{read,write}{16,32}be

So yes, using io{read,write}{16,32}be directly is preferable.

quoted

quoted

It emulates up to 64 channels within one serial controller
using the same TDM physical interface routed from the TSA.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>

In some places, you comments use the network style, not the generic style.

I will fix them.

quoted

Christophe

Thanks for the review,
Hervé