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

[Frank Li]

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

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