Re: [PATCH tty v1 72/74] serial: ucc_uart: Use port lock wrappers
On Thu, Sep 14, 2023 at 1:39 PM John Ogness wrote: > Converted with coccinelle. No functional change. > > Signed-off-by: Thomas Gleixner > --- > drivers/tty/serial/ucc_uart.c | 4 ++-- Acked-by: Timur Tabi
Re: [PATCH] Documentation: devices.txt: reconcile serial/ucc_uart minor numers
On Mon, Jul 24, 2023 at 1:33 AM Randy Dunlap wrote:
>
> Reconcile devices.txt with serial/ucc_uart.c regarding device number
> assignments. ucc_uart.c supports 4 ports and uses minor devnums
> 46-49, so update devices.txt with that info.
> Then update ucc_uart.c's reference to the location of the devices.txt
> list in the kernel source tree.
>
> Fixes: d7584ed2b994 ("[POWERPC] qe-uart: add support for Freescale
> QUICCEngine UART")
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Randy Dunlap
> Cc: Timur Tabi
> Cc: Kumar Gala
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Greg Kroah-Hartman
> Cc: Jiri Slaby
> Cc: linux-ser...@vger.kernel.org
> Cc: Jonathan Corbet
> Cc: linux-...@vger.kernel.org
Acked-by: Timur Tabi
One thing does concern me. The UCC UART driver piggy-backs on the CPM
driver's layout (see cpm_uart.h), but apparently CPM UART supports 6
devices, not four:
#define UART_NRfs_uart_nr
where fs_uart_nr is defined in enum fs_uart_id.
Unfortunately, it's been so long since I've touched this code, I'm not
sure whether this means anything.
Re: [PATCH] serial: Use of_property_read_bool() for boolean properties
On Fri, Mar 10, 2023 at 8:48 AM Rob Herring wrote: > > It is preferred to use typed property access functions (i.e. > of_property_read_ functions) rather than low-level > of_get_property/of_find_property functions for reading properties. > Convert reading boolean properties to to of_property_read_bool(). > > Signed-off-by: Rob Herring > --- > drivers/tty/serial/imx.c | 14 +- > drivers/tty/serial/mxs-auart.c | 4 ++-- > drivers/tty/serial/ucc_uart.c | 2 +- ucc_uart.c portion: Acked-by: Timur Tabi
Re: [PATCH v2 1/5] serial: ucc_uart: Remove custom frame size calculation
On Tue, Aug 30, 2022 at 3:49 AM Ilpo Järvinen
wrote:
>
> The number of bits can be calculated using tty_get_frame_size(), no
> need for the driver to do it on its own.
>
> Also remove a comment on number of bits that doesn't match the code nor
> the comment on ucc_uart_pram's rx_length ("minus 1" part differs). That
> comment seems a verbatim copy of that in cpm_uart/cpm_uart_core.c
> anyway so perhaps it was just copied over w/o much thinking.
>
> Reviewed-by: Andy Shevchenko
> Signed-off-by: Ilpo Järvinen
Acked-by: Timur Tabi
Re: [PATCH] tty: serial: Fix refcount leak bug in ucc_uart.c
On Sat, Jun 18, 2022 at 1:09 AM Liang He wrote: > > In soc_info(), of_find_node_by_type() will return a node pointer > with refcount incremented. We should use of_node_put() when it is > not used anymore. > > Signed-off-by: Liang He Acked-by: Timur Tabi
Re: [PATCH -next 9/9] ASoC: fsl_xcvr: check return value after calling platform_get_resource_byname()
On Fri, Jun 11, 2021 at 4:32 AM Yang Yingliang wrote:
> rx_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rxfifo");
> tx_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "txfifo");
> + if (!rx_res || !tx_res) {
> + dev_err(dev, "Invalid resource\n");
> + return -EINVAL;
> + }
If platform_get_resource_byname() returns an error, it's probably
because the name cannot be found. So I think this error message is
more accurate:
"could not find rxfifo or txfifo resource"
Re: [PATCH 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver
On 9/18/20 9:21 AM, Viorel Suman (OSS) wrote:
+static const u32 fsl_xcvr_earc_channels[] = { 1, 2, 8, 16, 32, }; /*
+one bit 6, 12 ? */
What's the meaning of the comments?
Just a thought noted as comment. HDMI2.1 spec defines 6- and 12-channels layout
when
one bit audio stream is transmitted - I was wandering how can this be enforced.
Is a @todo like of comment.
Please don't add comments that other developers could never understand.
The text that you just wrote here would be a great starting point for a
better comment.
Re: [PATCH v2 net-next] net: convert suitable drivers to use phy_do_ioctl_running
On 1/21/20 3:09 PM, Heiner Kallweit wrote: drivers/net/ethernet/qualcomm/emac/emac.c | 14 +- Acked-by: Timur Tabi
Re: [PATCH] evh_bytechan: fix out of bounds accesses
On 1/15/20 2:01 PM, Scott Wood wrote: FWIW I'd rather see the original patch, that keeps the raw asm hcall stuff as simple wrappers in one place. I can live with that.
Re: [PATCH] evh_bytechan: fix out of bounds accesses
On 1/14/20 12:31 AM, Stephen Rothwell wrote: +/** + * ev_byte_channel_send - send characters to a byte stream + * @handle: byte stream handle + * @count: (input) num of chars to send, (output) num chars sent + * @bp: pointer to chars to send + * + * Returns 0 for success, or an error code. + */ +static unsigned int ev_byte_channel_send(unsigned int handle, + unsigned int *count, const char *bp) Well, now you've moved this into the .c file and it is no longer available to other callers. Anything wrong with keeping it in the .h file?
Re: [PATCH] evh_bytechan: fix out of bounds accesses
On 1/14/20 2:29 AM, Segher Boessenkool wrote: You have no working lswx I suppose?:-) I don't know if the P4080 supports lswx, but it does, than that would be an elegant way to fix this bug.
Re: [PATCH] evh_bytechan: fix out of bounds accesses
On 1/14/20 3:18 AM, Laurentiu Tudor wrote: I can offer myself. I'll send a MAINTAINERS patch if nobody is against it. Yes, please do. I'm always available if you have any questions on the code.
Re: [PATCH] MAINTAINERS: Add myself as maintainer of ehv_bytechan tty driver
On 1/14/20 5:00 AM, Laurentiu Tudor wrote: Michael Ellerman made a call for volunteers from NXP to maintain this driver and I offered myself. Signed-off-by: Laurentiu Tudor Acked-by: Timur Tabi
Re: [PATCH] evh_bytechan: fix out of bounds accesses
On 1/13/20 7:10 PM, Timur Tabi wrote: I would prefer that ev_byte_channel_send() is updated to access only 'count' bytes. If that means adding a memcpy to the ev_byte_channel_send() itself, then so be it. Trying to figure out how to stuff n bytes into 4 32-bit registers is probably not worth the effort. Looks like ev_byte_channel_receive() has the same bug, but in reverse.
Re: [PATCH] evh_bytechan: fix out of bounds accesses
On 1/13/20 2:25 PM, Stephen Rothwell wrote: The problem is not really the declaration, the problem is that ev_byte_channel_send always accesses 16 bytes from the buffer and it is not always passed a buffer that long (in one case it is passed a pointer to a single byte). So the alternative to the memcpy approach I have take is to complicate ev_byte_channel_send so that only accesses count bytes from the buffer. Ah, I see now. This is all coming back to me. I would prefer that ev_byte_channel_send() is updated to access only 'count' bytes. If that means adding a memcpy to the ev_byte_channel_send() itself, then so be it. Trying to figure out how to stuff n bytes into 4 32-bit registers is probably not worth the effort.
Re: [PATCH] evh_bytechan: fix out of bounds accesses
On Thu, Jan 9, 2020 at 1:41 AM Stephen Rothwell wrote:
>
> ev_byte_channel_send() assumes that its third argument is a 16 byte array.
> Some places where it is called it may not be (or we can't easily tell
> if it is). Newer compilers have started producing warnings about this,
> so make sure we actually pass a 16 byte array.
...
> +static unsigned int local_ev_byte_channel_send(unsigned int handle,
> +unsigned int *count, const char *p)
> +{
> + char buffer[EV_BYTE_CHANNEL_MAX_BYTES];
> + unsigned int c = *count;
> +
> + if (c < sizeof(buffer)) {
> + memcpy(buffer, p, c);
> + memset(&buffer[c], 0, sizeof(buffer) - c);
> + p = buffer;
> + }
> + return ev_byte_channel_send(handle, count, p);
> +}
Why not simply correct the parameters of ev_byte_channel_send?
static inline unsigned int ev_byte_channel_send(unsigned int handle,
-unsigned int *count, const char buffer[EV_BYTE_CHANNEL_MAX_BYTES])
+unsigned int *count, const char *buffer)
Back then, I probably thought I was just being clever with this code,
but I realize now that it doesn't make sense to do the way I did.
Re: [PATCH] evh_bytechan: fix out of bounds accesses
On 1/13/20 8:34 AM, Laurentiu Tudor wrote: There are a few users that I know of, but I can't tell if that's enough to justify keeping the driver. [1]https://source.codeaurora.org/external/qoriq/qoriq-yocto-sdk/hypervisor/ IIRC, the driver is the only reasonable way to get a serial console from a guest. So if there are users of the hypervisor, then I think there's a good chance at least one is using the byte channel driver.
Re: [PATCH] evh_bytechan: fix out of bounds accesses
On 1/13/20 6:26 AM, Michael Ellerman wrote: I've never heard of it, and I have no idea how to test it. It's not used by qemu, I guess there is/was a Freescale hypervisor that used it. Yes, there is/was a Freescale hypervisor that I and a few others worked on. I've added a couple people on CC that might be able to tell the current disposition of it. But maybe it's time to remove it if it's not being maintained/used by anyone? I wouldn't be completely opposed to that if there really are no more users. There really weren't any users even when I wrote the driver. I haven't had a chance to study the patch, but my first instinct is that there's got to be a better way to fix this than introducing a memcpy.
Re: [PATCH] serial: ucc_uart: remove redundant assignment to pointer bdp
On 12/19/19 6:10 PM, Colin King wrote:
From: Colin Ian King
The variable bdp is being initialized with a value that is never
read and it is being updated later with a new value. The initialization
is redundant and can be removed.
Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King
Acked-by: Timur Tabi
Looks like this bug has been there since day 1.
Re: [PATCH v6 00/49] QUICC Engine support on ARM, ARM64, PPC64
On 11/28/19 8:55 AM, Rasmus Villemoes wrote: There have been several attempts in the past few years to allow building the QUICC engine drivers for platforms other than PPC32. This is yet another attempt. v5 can be found here:https://lore.kernel.org/lkml/20191118112324.22725-1-li...@rasmusvillemoes.dk/ If it helps: Entire series: Acked-by: Timur Tabi I've worked on all code covered by this patchset except for the hdlc driver. I don't know if my ACKs are acceptable to everyone, but you have them regardless.
Re: [PATCH v5 00/48] QUICC Engine support on ARM, ARM64, PPC64
On 11/18/19 5:22 AM, Rasmus Villemoes wrote: There have been several attempts in the past few years to allow building the QUICC engine drivers for platforms other than PPC32. This is yet another attempt. v4 can be found here: https://lore.kernel.org/lkml/20191108130123.6839-1-li...@rasmusvillemoes.dk/ This is excellent work, thank you. All patches: Reviewed-by: Timur Tabi Serial patches: Acked-by: Timur Tabi
Re: [PATCH v4 32/47] serial: ucc_uart: use of_property_read_u32() in ucc_uart_probe()
On 11/15/19 2:01 AM, Rasmus Villemoes wrote: That would be a separate patch, this patch is only concerned with eliminating the implicit assumption of the host being big-endian. And there's already been some pushback to adding arch-specific ifdefs (which I agree with, but as I responded there see as the lesser evil), so unless there's a very good reason to add that complexity, I'd rather not. We don't want to encourage people to introduce device trees that don't have the brg-frequency property in them.
Re: [PATCH v4 45/47] net/wan/fsl_ucc_hdlc: reject muram offsets above 64K
On 11/15/19 1:44 AM, Rasmus Villemoes wrote: I can change it, sure, but it's a matter of taste. To me the above asks "does the value change when it is truncated to a u16" which makes perfect sense when the value is next used with iowrite16be(). Using a comparison to U16_MAX takes more brain cycles for me, because I have to think whether it should be > or >=, and are there some signedness/integer promotion business interfering with that test. Ok.
Re: [PATCH v4 46/47] net: ethernet: freescale: make UCC_GETH explicitly depend on PPC32
On 11/15/19 1:54 AM, Rasmus Villemoes wrote: "Also, the QE Ethernet has never been integrated on any non-PowerPC SoC and most likely will not be in the future." That works for me, thanks.
Re: [PATCH v4 46/47] net: ethernet: freescale: make UCC_GETH explicitly depend on PPC32
On 11/14/19 11:44 PM, Li Yang wrote: Can you add an explanation why we don't want ucc_geth on non-PowerPC platforms? I think it is because the QE Ethernet was never integrated in any non-PowerPC SoC and most likely will not be in the future. We probably can make it compile for other architectures for general code quality but it is not a priority. This explanation belongs in the commit message.
Re: [PATCH v4 07/47] soc: fsl: qe: qe.c: guard use of pvr_version_is() with CONFIG_PPC32
On Fri, Nov 8, 2019 at 7:04 AM Rasmus Villemoes
wrote:
>
> +static bool qe_general4_errata(void)
> +{
> +#ifdef CONFIG_PPC32
> + return pvr_version_is(PVR_VER_836x) || pvr_version_is(PVR_VER_832x);
> +#endif
> + return false;
> +}
> +
> /* Program the BRG to the given sampling rate and multiplier
> *
> * @brg: the BRG, QE_BRG1 - QE_BRG16
> @@ -223,7 +231,7 @@ int qe_setbrg(enum qe_clock brg, unsigned int rate,
> unsigned int multiplier)
> /* Errata QE_General4, which affects some MPC832x and MPC836x SOCs,
> says
>that the BRG divisor must be even if you're not using divide-by-16
>mode. */
Can you also move this comment (and fix the comment formatting so that
it's a proper function comment) to qe_general4_errata()?
Re: [PATCH v4 45/47] net/wan/fsl_ucc_hdlc: reject muram offsets above 64K
On Fri, Nov 8, 2019 at 7:04 AM Rasmus Villemoes
wrote:
> diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
> index 8d13586bb774..f029eaa7cfc0 100644
> --- a/drivers/net/wan/fsl_ucc_hdlc.c
> +++ b/drivers/net/wan/fsl_ucc_hdlc.c
> @@ -245,6 +245,11 @@ static int uhdlc_init(struct ucc_hdlc_private *priv)
> ret = -ENOMEM;
> goto free_riptr;
> }
> + if (riptr != (u16)riptr || tiptr != (u16)tiptr) {
"riptr/tiptr > U16_MAX" is clearer.
Re: [PATCH v4 46/47] net: ethernet: freescale: make UCC_GETH explicitly depend on PPC32
On Fri, Nov 8, 2019 at 7:04 AM Rasmus Villemoes wrote: > > Currently, QUICC_ENGINE depends on PPC32, so this in itself does not > change anything. In order to allow removing the PPC32 dependency from > QUICC_ENGINE and avoid allmodconfig build failures, add this explicit > dependency. Can you add an explanation why we don't want ucc_geth on non-PowerPC platforms?
Re: [PATCH v4 32/47] serial: ucc_uart: use of_property_read_u32() in ucc_uart_probe()
On Fri, Nov 8, 2019 at 7:03 AM Rasmus Villemoes
wrote:
>
> + if (of_property_read_u32(np, "cell-index", &val) &&
> + of_property_read_u32(np, "device-id", &val)) {
I know that this is technically correct, but it's obfuscated IMHO.
'val' is set correctly only when of_property_read_u32(...) is "false",
which is doubly-weird because of_property_read_u32(...) doesn't
actually return a boolean.
I would rather you break this into two if-statements like the original code.
Re: [PATCH v4 32/47] serial: ucc_uart: use of_property_read_u32() in ucc_uart_probe()
On Fri, Nov 8, 2019 at 7:03 AM Rasmus Villemoes
wrote:
>
> - if (*iprop)
> - qe_port->port.uartclk = *iprop;
> + if (val)
> + qe_port->port.uartclk = val;
> else {
> /*
> * Older versions of U-Boot do not initialize the
> brg-frequency
> * property, so in this case we assume the BRG frequency is
> * half the QE bus frequency.
> */
This bug in older U-Boots is definitely PowerPC-specific, so could you
change this so that it reports an error on ARM if brg-frequency is
zero, and displays a warning on PowerPC?
Re: [PATCH v4 30/47] serial: ucc_uart: factor out soft_uart initialization
On Fri, Nov 8, 2019 at 7:03 AM Rasmus Villemoes
wrote:
>
> - /*
> -* Determine if we need Soft-UART mode
> -*/
> if (of_find_property(np, "soft-uart", NULL)) {
> dev_dbg(&ofdev->dev, "using Soft-UART mode\n");
> soft_uart = 1;
> + } else {
> + return 0;
> }
How about:
if (!of_find_property(np, "soft-uart", NULL))
return 0;
And I think you should be able to get rid of the "soft_uart" variable.
Re: [PATCH v4 04/47] soc: fsl: qe: introduce qe_io{read,write}* wrappers
On 11/12/19 1:14 AM, Rasmus Villemoes wrote: but that's because readl and writel by definition work on little-endian registers. I.e., on a BE platform, the readl and writel implementation must themselves contain a swab, so the above would end up doing two swabs on a BE platform. Do you know whether the compiler optimizes-out the double swab? (On PPC, there's a separate definition of mmio_read32be, namely writel_be, which in turn does a out_be32, so on PPC that doesn't actually end up doing two swabs). So ioread32be etc. have well-defined semantics: access a big-endian register and return the result in native endianness. It seems weird that there aren't any cross-arch lightweight endian-specific I/O accessors.
Re: [PATCH v4 04/47] soc: fsl: qe: introduce qe_io{read, write}* wrappers
On Fri, Nov 8, 2019 at 7:03 AM Rasmus Villemoes wrote: > > The QUICC engine drivers use the powerpc-specific out_be32() etc. In > order to allow those drivers to build for other architectures, those > must be replaced by iowrite32be(). However, on powerpc, out_be32() is > a simple inline function while iowrite32be() is out-of-line. So in > order not to introduce a performance regression on powerpc when making > the drivers work on other architectures, introduce qe_io* helpers. Isn't it also true that iowrite32be() assumes a little-endian platform and always does a byte swap?
Re: [alsa-devel] [PATCH] ASoC: cs42xx8: Remove S32_LE in format list
On 3/1/19 12:32 AM, S.j. Wang wrote: This case is covered by S24_LE I think. The S32_LE means the data is 32bit and slot width Is 32bit, this is not in data sheet. The problem is that if you have 32-bit samples in your audio file, and you want to play them, then software (e.g. alsalib) will need to convert the audio to 24-bit before sending it to hardware. This is unnecessary because the hardware can "convert" the sample to 24-bit automatically by ignoring the lower 8 bits. I think a lot of codecs do this already.
Re: [PATCH] ASoC: cs42xx8: Remove S32_LE in format list
On 2/27/19 11:56 PM, S.j. Wang wrote: cs42xx8 is a 24-bit A/D and 24-bit D/A device, so the S32_LE should not be in the supported format list. Signed-off-by: Shengjiu Wang Is the device capable of accepting 32-bit samples, even if it downgrades it to 24-bit internally? If so, then maybe SNDRV_PCM_FMTBIT_S32_LE should stay.
Re: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs
On 3/16/18 6:04 PM, Steve Wise wrote: Anybody understand why the PPC implementation of writeX_relaxed() isn't relaxed? You probably should ask that on the linuxppc-dev@lists.ozlabs.org mailing list. I've always wondered why PowerPC has non-standard I/O accessors. -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v1 00/15] ASoC: fsl_ssi: Clean up - program flow level
On 12/19/17 11:00 AM, Nicolin Chen wrote: This series of patches is the second set to clean up fsl_ssi driver in the program flow level. Any patch here may impact a fundamental test case like playback or record. With Christmas happening over the next two weeks, I don't think I'll be able to review these patches until January.
Re: [PATCH v4 00/11] ASoC: fsl_ssi: Clean up - coding style level
On 12/17/17 8:51 PM, Nicolin Chen wrote: Nicolin Chen (11): ASoC: fsl_ssi: Rename fsl_ssi_private to fsl_ssi ASoC: fsl_ssi: Cache pdev->dev pointer ASoC: fsl_ssi: Refine all comments ASoC: fsl_ssi: Rename registers and fields macros ASoC: fsl_ssi: Refine indentations and wrappings ASoC: fsl_ssi: Refine printk outputs ASoC: fsl_ssi: Rename cpu_dai parameter to dai ASoC: fsl_ssi: Rename scr_val to scr ASoC: fsl_ssi: Replace fsl_ssi_rxtx_reg_val with fsl_ssi_regvals ASoC: fsl_ssi: Rename i2smode to i2s_net ASoC: fsl_ssi: Define ternary macros to simplify code Acked-by: Timur Tabi
Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments
On 12/16/17 11:49 AM, Nicolin Chen wrote: I never said that I don't agree with Timur. Every change here is to simplify things. As long as Timur or any reviewer feels one of new comments is harder to understand, I am totally fine to rework. I respect everyone's opinion, but I hope everyone can respect my effort too by telling me which one needs to rework and why? I do respect it. I apologize if I came across as unduly harsh.
Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments
On 12/16/17 11:30 AM, Caleb Crome wrote: Having come to work on this driver with very little knowledge about kernel programming, and i.MX, I have to agree with Timur. It's an amazingly complex driver (with support of so many variants). By eliminating verbose commentary, it's also wiping away a lot of knowledge. The more sparse commentary makes things harder to understand for newcomers, or really anybody who isn't already steeped in knowledge about the SSI port and linux, and it's interaction with DMA. This is exactly why I wrote the comments the way I did. As I was learning about the hardware and ASoC drivers, whenever I was confused about something and then figured it out, I would add a comment about that. I figured if it wasn't obvious to me, it wouldn't be obvious to anyone else. And since this was one of the first ASoC drivers, and the first to use device tree, it would become a reference driver for years to come. That made it even more important that I document everything I learned. I am pleased that other developers kept up that commenting style. This patch destroys all of that.
Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments
On 12/13/17 5:18 PM, Nicolin Chen wrote:
- /* Used when using fsl-ssi as sound-card. This is only used by ppc and
-* should be replaced with simple-sound-card. */
struct platform_device *pdev;
Is this comment no longer true?
+ * 1) SSI in earlier SoCS has crtical bits in control registers that
critical
-/**
- * fsl_ssi_isr: SSI interrupt handler
- *
- * Although it's possible to use the interrupt handler to send and receive
- * data to/from the SSI, we use the DMA instead. Programming is more
- * complicated, but the performance is much better.
- *
- * This interrupt handler is used only to gather statistics.
- *
- * @irq: IRQ of the SSI device
- * @dev_id: pointer to the fsl_ssi structure for this SSI device
- */
What's wrong with this comment?
-/*
- * Clear RX or TX FIFO to remove samples from the previous
- * stream session which may be still present in the FIFO and
- * may introduce bad samples and/or channel slipping.
- *
- * Note: The SOR is not documented in recent IMX datasheet, but
- * is described in IMX51 reference manual at section 56.3.3.15.
+/**
+ * Clear remaining data in the FIFO to avoid dirty data or channel slipping
I think the original is better, unless there's something untrue about it.
-* We are running on a SoC which does not support online SSI
-* reconfiguration, so we have to enable all necessary flags at once
-* even if we do not use them later (capture and playback configuration)
+* Online configuration is not supported
+* Enable or Disable all necessary bits at once
Ditto
- /*
-* Configure single direction units while the SSI unit is running
-* (online configuration)
-*/
+ /* Online configure single direction while SSI is running */
Ditto
- /*
-* Disabling the necessary flags for one of rx/tx while the
-* other stream is active is a little bit more difficult. We
-* have to disable only those flags that differ between both
-* streams (rx XOR tx) and that are set in the stream that is
-* disabled now. Otherwise we could alter flags of the other
-* stream
-*/
-
- /* These assignments are simply vals without bits set in avals*/
+ /* Exclude necessary bits for the opposite stream */
Ditto
- /*
-* Be sure the Tx FIFO is filled when TE is set.
-* Otherwise, there are some chances to start the
-* playback with some void samples inserted first,
-* generating a channel slip.
-*
-* First, SSIEN must be set, to let the FIFO be filled.
-*
-* Notes:
-* - Limit this fix to the DMA case until FIQ cases can
-* be tested.
-* - Limit the length of the busy loop to not lock the
-* system too long, even if 1-2 loops are sufficient
-* in general.
-*/
What's wrong with this comment?
- /*
-* Note that these below aren't just normal registers.
-* They are a way to disable or enable bits in SACCST
-* register:
-* - writing a '1' bit at some position in SACCEN sets the
-* relevant bit in SACCST,
-* - writing a '1' bit at some position in SACCDIS unsets
-* the relevant bit in SACCST register.
-*
-* The two writes below first disable all channels slots,
-* then enable just slots 3 & 4 ("PCM Playback Left Channel"
-* and "PCM Playback Right Channel").
-*/
+ /* Disable all channel slots */
Ditto.
-* Why are we setting up SACCST everytime we are starting a
-* playback?
-* Some CODECs (like VT1613 CODEC on UDOO board) like to
-* (sometimes) set extra bits in their SLOTREQ requests.
-* When a bit is set in a SLOTREQ request then SSI sets the
-* relevant bit in SACCST automatically (it is enough if a bit was
-* set in a SLOTREQ just once, bits in SACCST are 'sticky').
-* If an extra slot gets enabled that's a disaster for playback
-* because some of normal left or right channel samples are
-* redirected instead to this extra slot.
+* SACCST might be modified via AC Link by a CODEC if it sends
+* extra bits in their SLOTREQ requests, which'll accidentally
+* send valid data to slots other than normal playback slots.
*
-* A workaround implemented in fsl-asoc-card of setting an
-* appropriate CODEC register so that slots 3 & 4 (t
Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments
On 12/16/17 12:10 AM, Nicolin Chen wrote: Hi, I am outside so can't use mutt. Sorry for that. This comment is going to be replaced in the 2nd set anyway because the whole function will be replaced. So you're asking me to review comment changes that will soon be deleted? Can you send out a new patch without changes to comments, so that I can focus on the ones that matter? And please point out all comments that you think I need to rework. I am totally fine to do that. I don't think every single one is bad. And this patch has to go in as it also adds a lot of new comments. Ok.
Re: [PATCH v3 00/11] ASoC: fsl_ssi: Clean up - coding style level
On 12/13/17 5:18 PM, Nicolin Chen wrote: Additionally, in order to fix/work-around hardware bugs and design flaws, the driver made a lot of compromise so now its program flow looks very complicated and it's getting hard to maintain or update. So I am going to clean up the driver on both coding style level and program flow level. I'm okay with everything except patch #3.
Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments
On 12/13/17 5:18 PM, Nicolin Chen wrote: -* We are running on a SoC which does not support online SSI -* reconfiguration, so we have to enable all necessary flags at once -* even if we do not use them later (capture and playback configuration) +* Online configuration is not supported +* Enable or Disable all necessary bits at once This is an example of a bad change, IMHO. The original was written in elegant prose. The new version is just two short sentences.
Re: [PATCH 0/3] tty/serial/ucc_uart: Adjustments for two functions
On 12/06/2017 03:10 PM, SF Markus Elfring wrote: Three update suggestions were taken into account from static source code analysis. Markus Elfring (3): Delete an error message for a failed memory allocation in ucc_uart_probe() Improve a size determination in ucc_uart_probe() Fix a typo in a comment line All three: Acked-by: Timur Tabi
Re: [PATCH 04/10] ASoC: fsl_ssi: Refine all comments
On 12/4/17 2:46 PM, Nicolin Chen wrote: This patch refines the comments by: 1) Removing all out-of-date comments 2) Removing all not-so-useful comments 3) Unifying the styles of all comments 4) Simplifying over-descriptive comments 5) Adding comments to improve code readablity 6) Moving all register related comments to fsl_ssi.h 7) Adding comments to all register and field defines Even after adding dozens of lines in fsl_ssi.h, this patch reduces 100 lines totally. I'll review the other patches later, but I'm not keen on your removal of some of the comments in this patch. I don't see why line count is so important, and you're removing some informative text. I can see removing trivial comments and outdated ones, but "no-so-useful" and "over-descriptive" are subjective.
Re: [PATCH 05/11] serial: uuc_uart: constify uart_ops structures
On 8/13/17 1:21 AM, Julia Lawall wrote: These uart_ops structures are only stored in the ops field of a uart_port structure and this fields is const, so the uart_ops structures can also be const. Done with the help of Coccinelle. Signed-off-by: Julia Lawall Acked-by: Timur Tabi
Re: [PATCH] sound: fsl_dma: remove dma_object path member
On 7/18/17 4:43 PM, Rob Herring wrote: - dev_err(&pdev->dev, "could not determine resources for %s\n", - ssi_np->full_name); + dev_err(&pdev->dev, "could not determine resources for %pOF\n", + ssi_np); I think you meant to include this in the other patch.
Re: [PATCH] ASoC : fsl_ssi : Correct the condition to check AC97 mode
Andreas Schwab wrote: > + return !!(ssi_private->dai_fmt & SND_SOC_DAIFMT_AC97) == > + SND_SOC_DAIFMT_AC97; This is never true. I think the right parenthesis should be at the end of the expression.
Re: [PATCH 1/1] serial/uuc_uart: Set shutdown timeout to CONFIG_HZ independent 2ms
Alexander Stein wrote: Okay, I was just wondering why the timeout is dependant on the timer tick. That didn't seem obvious to me. Rethinking about this, I would rather replace those lines with msleep instead. What's wrong with leaving it as-is? The code is five years old, and Freescale/NXP barely uses the QE any more. I don't have access to any hardware to test any changes you would propose.
Re: [PATCH 1/1] serial/uuc_uart: Set shutdown timeout to CONFIG_HZ independent 2ms
Alexander Stein wrote:
- schedule_timeout(2);
+ schedule_timeout(msecs_to_jiffies(2));
NACK.
So I don't remember why I wrote this code, but I don't think I was
expecting it to be 2ms. Instead, I think I just wanted it to be some
delay, but I believed that schedule_timeout(1) was too short or would be
"optimized" out somehow.
Note that right below this, I do:
if (qe_port->wait_closing) {
/* Wait a bit longer */
set_current_state(TASK_UNINTERRUPTIBLE);
schedule_timeout(qe_port->wait_closing);
}
And wait_closing is a number of jiffies, so I knew that
schedule_timeout() took jiffies as a parameter.
So I think I'm going to NACK this patch, since I believe I knew what I
was doing when I wrote it five years ago.
Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults
Maciej S. Szmigiero wrote:
On 17.01.2016 15:16, Maciej S. Szmigiero wrote:
On 17.01.2016 06:16, Timur Tabi wrote:
Maciej S. Szmigiero wrote:
This is because (at least according to the datasheet) imx21-class SSI
registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so
reading them for cache initialization may not be safe.
Also, a "MXC 91221 only" comment before these regs in FSL tree
(drivers/mxc/ssi/registers.h) seems to confirm that these registers
aren't present at least on some SSI (or SoC) models.
Can't we just mark them as precious or something, so that we don't have to have
two structures?
Looks like it can be done with just one static regmap config struct
used then as template - I will post updated patch.
Updated patch:
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 40dfd8a36484..105de76dd2fc 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -112,20 +112,6 @@ struct fsl_ssi_rxtx_reg_val {
struct fsl_ssi_reg_val tx;
};
-static const struct reg_default fsl_ssi_reg_defaults[] = {
- {CCSR_SSI_SCR, 0x},
- {CCSR_SSI_SIER,0x3003},
- {CCSR_SSI_STCR,0x0200},
- {CCSR_SSI_SRCR,0x0200},
- {CCSR_SSI_STCCR, 0x0004},
- {CCSR_SSI_SRCCR, 0x0004},
- {CCSR_SSI_SACNT, 0x},
- {CCSR_SSI_STMSK, 0x},
- {CCSR_SSI_SRMSK, 0x},
- {CCSR_SSI_SACCEN, 0x},
- {CCSR_SSI_SACCDIS, 0x},
-};
-
static bool fsl_ssi_readable_reg(struct device *dev, unsigned int reg)
{
switch (reg) {
@@ -190,8 +176,7 @@ static const struct regmap_config fsl_ssi_regconfig = {
.val_bits = 32,
.reg_stride = 4,
.val_format_endian = REGMAP_ENDIAN_NATIVE,
- .reg_defaults = fsl_ssi_reg_defaults,
- .num_reg_defaults = ARRAY_SIZE(fsl_ssi_reg_defaults),
+ .num_reg_defaults_raw = CCSR_SSI_SACCDIS / 4 + 1,
Replace "4" with "sizeof(uint32_t).
.readable_reg = fsl_ssi_readable_reg,
.volatile_reg = fsl_ssi_volatile_reg,
.precious_reg = fsl_ssi_precious_reg,
@@ -201,6 +186,7 @@ static const struct regmap_config fsl_ssi_regconfig = {
struct fsl_ssi_soc_data {
bool imx;
+ bool imx21regs;
Please add a comment explaining why this is needed.
bool offline_config;
u32 sisr_write_mask;
};
@@ -295,6 +281,7 @@ struct fsl_ssi_private {
static struct fsl_ssi_soc_data fsl_ssi_mpc8610 = {
.imx = false,
+ .imx21regs = false,
This is unnecessary. The default is already 0 (false).
.offline_config = true,
.sisr_write_mask = CCSR_SSI_SISR_RFRC | CCSR_SSI_SISR_TFRC |
CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 |
@@ -303,12 +290,14 @@ static struct fsl_ssi_soc_data fsl_ssi_mpc8610 = {
static struct fsl_ssi_soc_data fsl_ssi_imx21 = {
.imx = true,
+ .imx21regs = true,
.offline_config = true,
.sisr_write_mask = 0,
};
static struct fsl_ssi_soc_data fsl_ssi_imx35 = {
.imx = true,
+ .imx21regs = false,
Same here.
.offline_config = true,
.sisr_write_mask = CCSR_SSI_SISR_RFRC | CCSR_SSI_SISR_TFRC |
CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 |
@@ -317,6 +306,7 @@ static struct fsl_ssi_soc_data fsl_ssi_imx35 = {
static struct fsl_ssi_soc_data fsl_ssi_imx51 = {
.imx = true,
+ .imx21regs = false,
.offline_config = false,
.sisr_write_mask = CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 |
CCSR_SSI_SISR_TUE0 | CCSR_SSI_SISR_TUE1,
@@ -586,8 +576,11 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi_private
*ssi_private)
*/
regmap_write(regs, CCSR_SSI_SACNT,
CCSR_SSI_SACNT_AC97EN | CCSR_SSI_SACNT_FV);
- regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
- regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
+
+ if (!ssi_private->soc->imx21regs) {
+ regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
+ regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
+ }
This needs a comment.
/*
* Enable SSI, Transmit and Receive. AC97 has to communicate with the
@@ -1397,6 +1390,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
struct resource *res;
void __iomem *iomem;
char name[64];
+ struct regmap_config regconfig = fsl_ssi_regconfig;
of_id = of_match_device(fsl_ssi_ids, &pdev->dev);
if (!of_id || !of_id->data)
@@ -1444,15 +1438,22 @@ static int fsl_ssi_probe(struct platform_device *pdev)
return PTR_ERR(iomem);
ssi_private->ssi_phys = res->start;
+ if (ssi_private->soc->imx21regs) {
+ /* According to datasheet imx21-class SSI have less regs */
First of all, it would be "fewer regs", but even better would be to say
that c
Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults
Maciej S. Szmigiero wrote:
This is because (at least according to the datasheet) imx21-class SSI
registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so
reading them for cache initialization may not be safe.
Also, a "MXC 91221 only" comment before these regs in FSL tree
(drivers/mxc/ssi/registers.h) seems to confirm that these registers
aren't present at least on some SSI (or SoC) models.
Can't we just mark them as precious or something, so that we don't have
to have two structures?
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults
Maciej S. Szmigiero wrote:
+static const struct regmap_config fsl_ssi_regconfig_imx21 = {
+ .max_register = CCSR_SSI_SRMSK,
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+ .val_format_endian = REGMAP_ENDIAN_NATIVE,
+ .num_reg_defaults_raw = CCSR_SSI_SRMSK / 4 + 1,
+ .readable_reg = fsl_ssi_readable_reg,
+ .volatile_reg = fsl_ssi_volatile_reg,
+ .precious_reg = fsl_ssi_precious_reg,
+ .writeable_reg = fsl_ssi_writeable_reg,
+ .cache_type = REGCACHE_RBTREE,
+};
+
static const struct regmap_config fsl_ssi_regconfig = {
.max_register = CCSR_SSI_SACCDIS,
.reg_bits = 32,
.val_bits = 32,
.reg_stride = 4,
.val_format_endian = REGMAP_ENDIAN_NATIVE,
- .reg_defaults = fsl_ssi_reg_defaults,
- .num_reg_defaults = ARRAY_SIZE(fsl_ssi_reg_defaults),
+ .num_reg_defaults_raw = CCSR_SSI_SACCDIS / 4 + 1,
.readable_reg = fsl_ssi_readable_reg,
.volatile_reg = fsl_ssi_volatile_reg,
.precious_reg = fsl_ssi_precious_reg,
Is this really necessary? Why do we need separate register configs for
one specific SOC? There are already too many "if
(some_stupid_imx_variant)" blocks in this driver.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] ASoC: fsl: select SND_SOC_FSL_SAI or SND_SOC_FSL_SSI depending on SoC type
Lothar Waßmann wrote: Why? If more than one of the IMX6 SoCs are selected, both interfaces may be selected at the same time without any harm. Oh, ok. I thought the point behind the patch was that you *souldn't* enable the the SSI driver on an i.MX6UL. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] ASoC: fsl: select SND_SOC_FSL_SAI or SND_SOC_FSL_SSI depending on SoC type
Lothar Waßmann wrote: - select SND_SOC_FSL_SSI + select SND_SOC_FSL_SAI if SOC_IMX6UL + select SND_SOC_FSL_SSI if SOC_IMX6Q || SOC_IMX6SL || SOC_IMX6SX I don't think this is compatible with a multiarch kernel. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults
Mark Brown wrote: Quite possibly (it'll be more efficient and it's intended for such use cases) but as I said in my other reply that then has the issue that it implicitly gives default values to all the registers so I'd expect we still need to handle the cache initialisation explicitly (or alternatively the hardware sync with the cache on startup). Why does REGCACHE_FLAT assume that all registers have a default value of 0? Shouldn't it have the same behavior w.r.t. cache values as REGCACHE_RBTREE? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults
Mark Brown wrote: regcache handles this fine, it's perfectly happy to just go and allocate the cache as registers get used (this is why the code that's doing the allocation exists...). What is causing problems here is that the first access to the register is happening in interrupt context so we can't do a GFP_KERNEL allocation for it. Considering how small and not-sparse the SSI register space is, would using REGCACHE_FLAT be appropriate? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults
Mark Brown wrote: That's possibly problematic because the flat cache will of necessity end up with defaults (of 0 from the kzalloc()) for all the registers. You'll still have default values in the cache, though some of the behaviour around optimising syncs does change without them explicitly given. It does deal with the allocation issue but given that the issue was incorrect defaults I'd be a bit concerned. Ok, I'm confused. Granted, all of this regcache stuff was added after I stopped working on this driver, so I'm out of the loop. But it appears that the regcache cannot properly handle an uninitialized cache. I would expect it to know to perform hard reads of any registers that are uninitialized. If the regcache wants to have an initialized cache, then it should automatically perform reads an all non-volatile, non-precious registers at initialization. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults
Maciej S. Szmigiero wrote:
There is no guarantee that on fsl_ssi module load
SSI registers will have their power-on-reset values.
In fact, if the driver is reloaded the values in
registers will be whatever they were set to previously.
This fixes hard lockup on fsl_ssi module reload,
at least in AC'97 mode.
Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support MEGA
Fast")
Signed-off-by: Maciej S. Szmigiero
Acked-by: Timur Tabi
I'm surprised that we're actually encouraging drivers to contain
hard-coded register values.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/3] ASoC: fsl_ssi: mark some registers precious
Maciej S. Szmigiero wrote: Mark some registers precious since their reads have side effects (like clearing flags). Signed-off-by: Maciej S. Szmigiero Acked-by: Timur Tabi ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3] ASoC: fsl_ssi: mark SACNT register volatile
Maciej S. Szmigiero wrote: + regmap_write(regs, CCSR_SSI_SACNT, + ssi_private->regcache_sacnt); So I'm not familiar with all of the regcache features, but I understand this patch. I was wondering if it makes sense to write the same exact value that was read previously. Isn't it possible for the WR or RD bits to change between fsl_ssi_suspend() and fsl_ssi_resume()? That is, should we be doing this instead? u32 temp; regmap_read(regs, CCSR_SSI_SACNT, &temp); temp &= 0x18; // preserve WR and RD regmap_write(regs, CCSR_SSI_SACNT, (ssi_private->regcache_sacnt & ~0x18) | temp); ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH 1/3] ASoC: fsl_ssi: mark SACNT register volatile
On Sun, Dec 20, 2015 at 2:30 PM, Maciej S. Szmigiero
wrote:
> SACNT register should be marked volatile since
> its WR and RD bits are cleared by SSI after
> completing the relevant operation.
> This unbreaks AC'97 register access.
>
> Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support
> MEGA Fast")
>
> Signed-off-by: Maciej S. Szmigiero
These patches seem okay, but can we hold off merging them until I get
back from vacation and have a chance to review them?
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [net-next v5 0/8] dpaa_eth: Add the Freescale DPAA Ethernet driver
On Thu, Dec 3, 2015 at 6:08 AM, <> wrote: > From: Madalin Bucur > > This patch series adds the Ethernet driver for the Freescale > QorIQ Data Path Acceleration Architecture (DPAA). Please fix your git-send-email configuration, so that your emails are formatted properly. This is the From: header: From: <> -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/2] net/fsl_pq_mdio: check TBI address for consistency with mapped range
Gerlando Falauto wrote: Change-Id: If1e7d8931f440ea9259726c36d3df797dda016fb You need to remove these from patches that are emailed, and fix the pointer type comparison. Otherwise, Acked-by: Timur Tabi ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/3] powerpc/512x: add LocalPlus Bus FIFO device driver
On 09/30/2015 04:24 PM, Alexander Popov wrote: Can you test for "!cs" here instead? +e = -EFAULT; +goto err_param; +} Unfortunately no: 0 is a valid value for Chip Select. Is it OK to leave it like that? Yes. +lpbfifo.ram_bus_addr = sg_dma_address(&sg); /* For freeing later */ +sg_dma_len(&sg) = lpbfifo.req->size; I don't think sg_dma_len() is meant to be used as an lvalue. I've double-checked and found many cases of such usage of this macro. It seems that I can't avoid it too. Ok. Driver code that has to parse #address-cells or #size-cells is usually wrong. I would not call it "parsing", I just check whether the dts-file is good. Anyway, could you give me a clue how to do better? You should use of_n_size_cells() and of_n_addr_cells(). -- ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 2/3] powerpc/512x: add a device tree binding for LocalPlus Bus FIFO
Alexander Popov wrote: I've just followed devicetree/bindings/dma/dma.txt... This "rx-tx" doesn't mean much but it could show that LocalPlus Bus FIFO uses a single DMA read-write channel. Should I really drop it? Hmmm, I'm not sure. Is there anything else (besides your driver) that parses this device tree node? dma.txt says this: "The specific strings that can be used are defined in the binding of the DMA client device." So this looks like it's driver-specific, but it is a required property. I guess you should keep it, but I think you should get a second opinion. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/3] powerpc/512x: add LocalPlus Bus FIFO device driver
Alexander Popov wrote: The only question I have: why calling dma_unmap_single() from within a spinlock is a bad practice? I don't know, but usually functions that allocate or free memory cannot be called from within a spinlock. You need to check that. Since the MPC5121 is a single-core CPU, you might not notice if you're doing something wrong. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/3] powerpc/512x: add LocalPlus Bus FIFO device driver
Alexander Popov wrote:
+struct mpc512x_lpbfifo_request {
+ phys_addr_t bus_phys; /* physical address of some device on LPB */
Is this a phys_addr_t or a dma_addr_t? It can't be both a physical
address and a bus address.
+ void *ram_virt; /* virtual address of some region in RAM */
+ u32 size;
+ enum lpb_dev_portsize portsize;
+ enum mpc512x_lpbfifo_req_dir dir;
+ void (*callback)(struct mpc512x_lpbfifo_request *);
+};
+
+int mpc512x_lpbfifo_submit(struct mpc512x_lpbfifo_request *req);
+
#endif /* __ASM_POWERPC_MPC5121_H__ */
diff --git a/arch/powerpc/platforms/512x/Kconfig
b/arch/powerpc/platforms/512x/Kconfig
index 48bf38d..f09016f 100644
--- a/arch/powerpc/platforms/512x/Kconfig
+++ b/arch/powerpc/platforms/512x/Kconfig
@@ -10,6 +10,12 @@ config PPC_MPC512x
select USB_EHCI_BIG_ENDIAN_MMIO if USB_EHCI_HCD
select USB_EHCI_BIG_ENDIAN_DESC if USB_EHCI_HCD
+config MPC512x_LPBFIFO
+ tristate "MPC512x LocalPlus Bus FIFO driver"
+ depends on PPC_MPC512x && MPC512X_DMA
+ help
+ Enable support for Freescale MPC512x LocalPlus Bus FIFO (SCLPC).
+
config MPC5121_ADS
bool "Freescale MPC5121E ADS"
depends on PPC_MPC512x
diff --git a/arch/powerpc/platforms/512x/Makefile
b/arch/powerpc/platforms/512x/Makefile
index 0169312..f47d422 100644
--- a/arch/powerpc/platforms/512x/Makefile
+++ b/arch/powerpc/platforms/512x/Makefile
@@ -5,4 +5,5 @@ obj-$(CONFIG_COMMON_CLK)+= clock-commonclk.o
obj-y += mpc512x_shared.o
obj-$(CONFIG_MPC5121_ADS) += mpc5121_ads.o mpc5121_ads_cpld.o
obj-$(CONFIG_MPC512x_GENERIC) += mpc512x_generic.o
+obj-$(CONFIG_MPC512x_LPBFIFO) += mpc512x_lpbfifo.o
obj-$(CONFIG_PDM360NG)+= pdm360ng.o
diff --git a/arch/powerpc/platforms/512x/mpc512x_lpbfifo.c
b/arch/powerpc/platforms/512x/mpc512x_lpbfifo.c
new file mode 100644
index 000..e6f2c6b
--- /dev/null
+++ b/arch/powerpc/platforms/512x/mpc512x_lpbfifo.c
@@ -0,0 +1,560 @@
+/*
+ * The driver for Freescale MPC512x LocalPlus Bus FIFO
+ * (called SCLPC in the Reference Manual).
+ *
+ * Copyright (C) 2013-2015 Alexander Popov.
+ *
+ * This file is released under the GPLv2.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DRV_NAME "mpc512x_lpbfifo"
+
+struct cs_range {
+ u32 csnum;
+ u32 base; /* must be zero */
+ u32 addr;
+ u32 size;
+};
+
+static struct lpbfifo_data {
+ spinlock_t lock; /* for protecting lpbfifo_data */
+ phys_addr_t regs_phys;
+ resource_size_t regs_size;
+ struct mpc512x_lpbfifo __iomem *regs;
+ int irq;
+ struct cs_range *cs_ranges;
+ size_t cs_n;
+ struct dma_chan *chan;
+ struct mpc512x_lpbfifo_request *req;
+ dma_addr_t ram_bus_addr;
+ bool wait_lpbfifo_irq;
+ bool wait_lpbfifo_callback;
+} lpbfifo;
+
+/*
+ * A data transfer from RAM to some device on LPB is finished
+ * when both mpc512x_lpbfifo_irq() and mpc512x_lpbfifo_callback()
+ * have been called. We execute the callback registered in
+ * mpc512x_lpbfifo_request just after that.
+ * But for a data transfer from some device on LPB to RAM we don't enable
+ * LPBFIFO interrupt because clearing MPC512X_SCLPC_SUCCESS interrupt flag
+ * automatically disables LPBFIFO reading request to the DMA controller
+ * and the data transfer hangs. So the callback registered in
+ * mpc512x_lpbfifo_request is executed at the end of
mpc512x_lpbfifo_callback().
+ */
+
+/*
+ * mpc512x_lpbfifo_irq - IRQ handler for LPB FIFO
+ */
+static irqreturn_t mpc512x_lpbfifo_irq(int irq, void *param)
+{
+ struct device *d = (struct device *)param;
Please use the variable name 'dev' instead of 'd', for consistency.
+ struct mpc512x_lpbfifo_request *req = lpbfifo.req;
+ unsigned long flags;
+ u32 status;
+
+ spin_lock_irqsave(&lpbfifo.lock, flags);
+
+ if (!lpbfifo.regs)
+ goto end0;
+
+ if (!req || req->dir == MPC512X_LPBFIFO_REQ_DIR_READ) {
+ dev_err(d, "bogus LPBFIFO IRQ\n");
+ goto end0;
+ }
So this might be an obscure bug. The code says that "req = lpbfifo.req"
above. However, it doesn't know that this block is in a critical
section, so it will initialize 'req' not on the top line of the
function, but rather right here. That means that although you think
that you're initializing 'req' before the spin_lock_irqsave(), the code
might actually initialize it after the spin_lock_irqsave().
My suggestion is that you explicitly initialize 'req' after
spin_lock_irqsave(). Or better yet, don't use 'req' and explicitly
reference lpbfifo.req every time.
+
+ status = in_be32(&lpbfifo.regs->status);
+ if (status != MPC512X_SCLPC_SUCCESS) {
+ dev_err(d, "DMA transfer between RAM and peripheral fa
Re: [PATCH v3 2/3] powerpc/512x: add a device tree binding for LocalPlus Bus FIFO
Alexander Popov wrote: +- dma-names: should be "rx-tx"; Why bother, if it can only be one value? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 3/3] dmaengine: mpc512x: initialize with subsys_initcall()
Alexander Popov wrote: Initialize Freescale MPC512x DMA driver with subsys_initcall() to allow the depending drivers to call dma_request_slave_channel() during their probe. Signed-off-by: Alexander Popov Is there any way we can use -EPROBEDEFER instead? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2] video: fbdev: fsl: Fix the sleep function for FSL DIU module
Wang Dongsheng wrote: Thanks Timur. @Scott, Could you apply this patch? You need to ask the fbdev maintainer to apply it, because it has to go through his tree. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2] video: fbdev: fsl: Fix the sleep function for FSL DIU module
Dongsheng Wang wrote: For deep sleep, the diu module will power off, when wake up from the deep sleep, the registers need to be reinitialized. Signed-off-by: Jason Jin Signed-off-by: Wang Dongsheng Acked-by: Timur Tabi ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel][PATCH 1/2] ASoC: fsl_ssi: Add driver suspend and resume to support MEGA Fast
On Jul 6, 2015, at 4:49 AM, Zidan Wang wrote:
> +static bool fsl_ssi_readable_reg(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case CCSR_SSI_STX0:
> + case CCSR_SSI_STX1:
> + case CCSR_SSI_SRX0:
> + case CCSR_SSI_SRX1:
> + case CCSR_SSI_SCR:
> + case CCSR_SSI_SISR:
> + case CCSR_SSI_SIER:
> + case CCSR_SSI_STCR:
> + case CCSR_SSI_SRCR:
> + case CCSR_SSI_STCCR:
> + case CCSR_SSI_SRCCR:
> + case CCSR_SSI_SFCSR:
> + case CCSR_SSI_STR:
> + case CCSR_SSI_SOR:
> + case CCSR_SSI_SACNT:
> + case CCSR_SSI_SACADD:
> + case CCSR_SSI_SACDAT:
> + case CCSR_SSI_SATAG:
> + case CCSR_SSI_STMSK:
> + case CCSR_SSI_SRMSK:
> + case CCSR_SSI_SACCST:
> + case CCSR_SSI_SACCEN:
> + case CCSR_SSI_SACCDIS:
> + return true;
> + default:
> + return false;
> + }
> +}
This should be the other way around: return true by default, and false it is
one of the few registers that is not readable.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel][PATCH 2/2] ASoC: fsl_ssi: sound is wrong after suspend/resume
On Jul 6, 2015, at 4:49 AM, Zidan Wang wrote: > The register SFCSR is volatile, but some bits in it need to be > recovered after suspend/resume. > > Signed-off-by: Zidan Wang Shouldn't this be part of patch #1? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/6] ASoC: fsl_ssi: enable AC'97 asymmetric rates
Maciej S. Szmigiero wrote:
/* Are the RX and the TX clocks locked? */
if (!of_find_property(np, "fsl,ssi-asynchronous", NULL)) {
- ssi_private->cpu_dai_drv.symmetric_rates = 1;
+ if (!fsl_ssi_is_ac97(ssi_private))
+ ssi_private->cpu_dai_drv.symmetric_rates = 1;
+
Is this necessary? Why not just add fsl,ssi-asynchronous to the AC97
device tree node?
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] ASoC: fsl_ssi: fix AC'97 mode
Maciej S. Szmigiero wrote: If there isn't going to be new platforms added with old bindings then this won't be needed - I'll remove it. I would love it if someone would port those original device trees to the new binding, so that we can get rid of the old one. But we should definitely not allow new device trees with the old binding. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] ASoC: fsl_ssi: fix AC'97 mode
Maciej S. Szmigiero wrote:
+ if (newbinding && fsl_ssi_is_ac97(ssi_private)) {
Is the "newbinding" necessary? I thought only the original PowerPC
device trees were the only one that have the old binding, and they never
supported AC97.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] ASoC: fsl: Add dedicated DMA buffer size for each cpu dai
Nicolin Chen wrote: >As the ssi is not the only cpu dai, there are esai, spdif, sai. >and imx_pcm_dma can be used by all of them. Especially ESAI need >a larger DMA buffer size. So Add dedicated DMA buffer for each cpu >dai. > >Signed-off-by: Shengjiu Wang Acked-by: Nicolin Chen Acked-by: Timur Tabi ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: new way of writing defconfigs for freescale's powerpc platforms
On 04/21/2015 12:55 PM, Scott Wood wrote: > >Ok, then define a new Kconfig option that merge_config.sh will look for. > So in p1_defconfig, there will be this line: > >CONFIG_OTHER_DEFCONFIGS=fsl_basic_config If you want to do that go ahead. In the meantime we'll use the mechanism that already exists. Dude, you are not making any sense. If there is a mechanism that already exists, then what are we talking about? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: new way of writing defconfigs for freescale's powerpc platforms
Scott Wood wrote: We want single-name config targets to still work from the user's perspective, but we want to reduce the (often imperfect) duplication under the hood. Ok, then define a new Kconfig option that merge_config.sh will look for. So in p1_defconfig, there will be this line: CONFIG_OTHER_DEFCONFIGS=fsl_basic_config -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: new way of writing defconfigs for freescale's powerpc platforms
Scott Wood wrote: > >Why do you need a powerpc-specific way to use merge_config.sh? Other >architectures have the same problem with defconfigs. What are you perceiving as "powerpc-specific" about what we're proposing? Well, there's the subject of this thread, which is "new way of writing defconfigs for freescale's powerpc platforms". > Are you complaining about the actual content of which fragments to use to produce which defconfigs going in arch/powerpc? No, I'm just trying to figure out what's powerpc-specific about Lijun's proposal. >Besides, wouldn't it make more sense to define a new defconfig type, >like p1_defconfig.merge, and if you do "make p1_defconfig.merge" it >knows to call merge_config.sh? That's already there. "make .config". Ok, so I'm definitely confused now. I have no idea what's actually being proposed, since apparently the ability to have merge configs already exists. Wouldn't it just be simpler to pass multiple defconfigs to 'make', and then 'make' will know to call merge_config.sh on them? So instead of make ./scripts/kconfig/merge_config.sh arch/powerpc/configs/fsl_basic_config p1_defconfig make we can just do make fsl_basic_config p1_defconfig make -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: new way of writing defconfigs for freescale's powerpc platforms
On Mon, Apr 20, 2015 at 3:31 PM, Scott Wood wrote: > > > The ability to merge configs is already there. We're just talking about > using that functionality. Why do you need a powerpc-specific way to use merge_config.sh? Other architectures have the same problem with defconfigs. Besides, wouldn't it make more sense to define a new defconfig type, like p1_defconfig.merge, and if you do "make p1_defconfig.merge" it knows to call merge_config.sh? -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: new way of writing defconfigs for freescale's powerpc platforms
On Fri, Apr 17, 2015 at 11:53 PM, Lijun Pan wrote: > Have just sent out a patch considering the previous discussion. > http://patchwork.ozlabs.org/patch/462249/ > [PATCH] powerpc/defconfig: new way of writing defconfig The ability to merge defconfigs is a feature that's been requested by many people for a long time. Any solution should definitely NOT be PowerPC-only. -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 05/35 linux-next] tty: constify of_device_id array
On 03/16/2015 02:17 PM, Fabian Frederick wrote: of_device_id is always used as const. (See driver.of_match_table and open firmware functions) Signed-off-by: Fabian Frederick ... drivers/tty/serial/ucc_uart.c | 2 +- For this driver: Acked-by: Timur Tabi ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
On 12/01/2014 02:40 PM, Fabio Estevam wrote: >Hm... that's new. But it's not really a driver issue anymore if it is done >in the core. So I guess for now just use platform_get_irq() and ignore the >other issue. With the suggested changes below, the removal of the driver works fine on a mx6: Would the mapping continue to exist after the driver is unloaded? Can you try multiple loads/unloads and see if interrupts still work? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
On 12/01/2014 02:01 PM, Arnd Bergmann wrote: >Does this mean that fsl_ssi.c should not be calling >irq_of_parse_and_map? How else should it get the IRQ? platform_get_irq() Ok, but that function also calls irq_create_of_mapping(). So it still appears that the only way to get the IRQ is to map it, but then we can't use devm_request_irq(). ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
On 12/01/2014 01:56 PM, Arnd Bergmann wrote: All other drivers that call irq_of_parse_and_map and pass that into devm_request_irq just never unmap, and their interrupts are already mapped by the platform code, so I think it's not even a leak. Does this mean that fsl_ssi.c should not be calling irq_of_parse_and_map? How else should it get the IRQ? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
On 12/01/2014 10:49 AM, Lars-Peter Clausen wrote: The driver creates the mapping by calling irq_of_parse_and_map(), so it also has to dispose the mapping. I agree with Markus, this does seem weird. It sounds like you're saying that irq_of_parse_and_map() and devm_request_irq() are incompatible. A quick grep shows the following drivers that call both functions: ata/pata_mpc52xx.c built-in.o cpufreq/exynos5440-cpufreq.c crypto/omap-sham.c dma/moxart-dma.c edac/mpc85xx_edac.c hsi/clients/nokia-modem.c i2c/busses/i2c-wmt.c input/serio/apbps2.c mmc/host/omap_hsmmc.c mmc/host/moxart-mmc.c mtd/nand/mpc5121_nfc.c net/ethernet/arc/emac_main.c net/ethernet/moxa/moxart_ether.c pci/host/pcie-rcar.c pinctrl/samsung/pinctrl-exynos5440.c pinctrl/samsung/pinctrl-exynos.c pinctrl/pinctrl-bcm2835.c spi/spi-bcm2835.c spi/spi-mpc512x-psc.c staging/xillybus/xillybus_of.c thermal/samsung/exynos_tmu.c ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()
On 12/01/2014 10:49 AM, Lars-Peter Clausen wrote: The driver creates the mapping by calling irq_of_parse_and_map(), so it also has to dispose the mapping. But the easy way out is to simply use platform_get_irq() instead of irq_of_parse_map(). In this case the mapping is not managed by the device but by the of core, so the device has not to dispose the mapping. Is this a problem unique to the SSI driver? Maybe devm_free_irq() should also dispose of the mapping? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3] qe-uart: modify qe-uart to adapt both powerpc and arm
On Fri, Oct 10, 2014 at 1:05 PM, Scott Wood wrote:
> There are many changes in here that ought to be separate patches with
> separate justification.
>
> Also, some of the QE changes seem to be reasonable cleanup, but not
> related to making the code work on ARM.
I agree with Scott. This patch already makes significant code
changes, so you should have one patch that just makes the
out_be32->iowrite32be changes. Changes to the QE library should NOT
be in the same patch as changes to ucc_uart.c.
In addition, changes like this:
- iprop = of_get_property(np, "port-number", NULL);
- if (!iprop) {
+ ret = of_property_read_u32_index(np, "port-number", 0, &val);
+ if (ret) {
should be changed to remove the OF dependency. If you're going to
replace of_get_property, replace it with device_property_read_u32(),
to remove the OF dependency.
>> diff --git a/arch/arm/include/asm/delay.h b/arch/arm/include/asm/delay.h
>> index dff714d..a932f99 100644
>> --- a/arch/arm/include/asm/delay.h
>> +++ b/arch/arm/include/asm/delay.h
>> @@ -57,6 +57,22 @@ extern void __bad_udelay(void);
>> __const_udelay((n) * UDELAY_MULT)) :\
>> __udelay(n))
>>
>> +#define spin_event_timeout(condition, timeout, delay)
>>\
>> +({
>>\
>> + typeof(condition) __ret;
>> \
>> + int i = 0;
>> \
>> + while (!(__ret = (condition)) && (i++ < timeout)) {
>> \
>> + if (delay)
>> \
>> + udelay(delay);
>> \
>> + else
>> \
>> + cpu_relax();
>> \
>> + udelay(1);
>> \
>> + }
>> \
>
> This will delay too long if "delay" is used.
Shouldn't ARM have a version of tb_ticks_since() by now?
>> + if (!__ret)
>> \
>> + __ret = (condition);
>> \
>> + __ret;
>> \
>
> Timur, do you remember why that final "if (!__ret) __ret = (condition);"
> is needed?
powerpc: Fix spin_event_timeout() to be robust over context switches
Current implementation of spin_event_timeout can be interrupted by an
IRQ or context switch after testing the condition, but before checking
the timeout. This can cause the loop to report a timeout when the
condition actually became true in the middle.
This patch adds one final check of the condition upon exit of the loop
if the last test of the condition was still false.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V2] ASoC: fsl_ssi: refine ipg clock usage in this module
Shengjiu Wang wrote: + ret = clk_prepare_enable(ssi_private->clk); + if (ret) + return ret; Will this work on PowerPC, where ssi_private->clk is always NULL? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module
On 09/09/2014 03:27 PM, Nicolin Chen wrote: I guess Mark's comment is merely against the check for clk validation because if talking about clk validation, we should check IS_ERR(clk) rather than check !=NULL directly. Ah, that makes sense now. However, my approach doesn't need any check. The open() or pm_resume() can just call clk_prepare_enable() directly. The __clk_enable() will then handle the 'clk == NULL' case: Yes, I was thinking the same thing. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module
On 09/09/2014 02:59 PM, Nicolin Chen wrote: + /* +* Initially mark the clock to NULL for all platforms so that later +* clk_prepare_enable() will ignore and return 0 for non-clock cases. +*/ + ssi_private->clk = NULL; According to Mark, NULL is a valid clock, so this should be instead: ssi_private->clk = PTR_ERR(-EINVAL); although that doesn't sit well with me. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module
On 09/09/2014 01:38 PM, Nicolin Chen wrote: make sure to have the call for imx only because it seems that the other platforms do not depend on the clock. Although I doubt anyone will every add support for clocks to PowerPC "side" of this driver, I would prefer to avoid IMX-specific changes. Instead, the code should check if a clock is available. That's why I suggested this change: - if (ssi_private->soc->imx) + if (!IS_ERR(ssi_private->clk)) ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module
On 09/09/2014 10:21 AM, Mark Brown wrote: if (ssi_private->clk) >clk_prepare_enable(ssi_private->clk); Should be a !IS_ERR() - NULL is a valid clock. In that case, ssi_private->clk needs to be initialized to -EINVAL or something, so that the check works on systems that don't have any clocks. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module
Shengjiu Wang wrote: + if (ssi_private->soc->imx) + clk_prepare_enable(ssi_private->clk); How about this instead? if (ssi_private->clk) clk_prepare_enable(ssi_private->clk); ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: qe: move qe from arch/powerpc to drivers
On 08/07/2014 03:11 PM, Scott Wood wrote: > >Do you have a better suggestion? > >Leave it where it is? We need it on ARM as well. In that case, drivers/soc is the least-bad option. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: qe: move qe from arch/powerpc to drivers
On 08/07/2014 03:08 PM, Scott Wood wrote: >Scott suggests drivers/soc. I'm not crazy about that, but I don't >maintain that code any more. Do you have a better suggestion? Leave it where it is? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: qe: move qe from arch/powerpc to drivers
On Wed, Aug 6, 2014 at 3:53 AM, qiang.z...@freescale.com wrote: > > Actually qe is a kind of IP block, so in my opinion, it is proper to put it > under driver/(just in my opinion). The QE library is not a driver, however. It doesn't register as a driver with the kernel. Scott suggests drivers/soc. I'm not crazy about that, but I don't maintain that code any more. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
