Re: [PATCH RFC 7/7] spi: spi-fsl-qspi: Add support for RX data sampling point adjustment

Frank Li
2026-03-03T21:02:55Z

From: Frank Li (AI-BOT) <frank.li@nxp.com>

AI bot review and may be useless.

> +#define QUADSPI_SMPR_SDRSMP(x) ((x) << 5)

Macro argument 'x' should be parenthesized: ((x) << 5) is fine, but
consider ((x) << 5) for consistency with kernel style.

> +struct fsl_qspi_chip_data {
> + u8 sdrsmp;
> + u32 rx_sampling_delay_ns;
> + unsigned int rate;
> +};

Consider documenting struct members with kernel-doc comments explaining
the purpose of each field.

> +static void fsl_qspi_update_rx_sampling_delay(struct fsl_qspi *q, u32 sdrsmp)
> +{
> + void __iomem *base = q->iobase;
> + u32 mcr, smpr = QUADSPI_SMPR_SDRSMP(sdrsmp);
> +
> + /* skip if requested value matches cached value */
> + if (q->smpr == smpr)
> + return;
> +
> + /* Disable the module */
> + mcr = qspi_readl(q, base + QUADSPI_MCR);
> + qspi_writel(q, mcr | QUADSPI_MCR_MDIS_MASK, base + QUADSPI_MCR);
> +
> + qspi_writel(q, smpr, base + QUADSPI_SMPR);
> +
> + /* Enable the module */
> + qspi_writel(q, mcr & ~QUADSPI_MCR_MDIS_MASK, base + QUADSPI_MCR);
> +
> + q->smpr = smpr;
> +}

Missing synchronization: if fsl_qspi_exec_op() calls this concurrently
from multiple threads, q->smpr cache may become inconsistent. Consider
protecting with q->lock (already used elsewhere in driver).

> + struct fsl_qspi_chip_data *chip = spi_get_ctldata(mem->spi);
> + void __iomem *base = q->iobase;
> + u32 addr_offset = 0;
> + int err = 0;
> @@ -679,6 +712,8 @@ static int fsl_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
> fsl_qspi_readl_poll_tout(q, base + QUADSPI_SR, (QUADSPI_SR_IP_ACC_MASK |
> QUADSPI_SR_AHB_ACC_MASK), 10, 1000);
>
> + fsl_qspi_update_rx_sampling_delay(q, chip->sdrsmp);

Potential NULL dereference: if fsl_qspi_setup_device() was never called
or failed silently, 'chip' is NULL. Add NULL check or ensure setup is
always called before exec_op.

> +static int fsl_qspi_setup_device(struct spi_device *spi)
> +{
> + struct fsl_qspi_chip_data *chip = spi_get_ctldata(spi);
> +
> + if (!chip) {
> + chip = kzalloc_obj(*chip, GFP_KERNEL);

kzalloc_obj() is non-standard. Use kzalloc(sizeof(*chip), GFP_KERNEL)
or verify kzalloc_obj() is defined in kernel headers.

> + if (!chip)
> + return -ENOMEM;
> + spi_set_ctldata(spi, chip);
> + }
> +
> + return 0;
> +}

setup_device() allows re-entry without freeing old chip. If called