Re: [PATCH 6/7] spi: cadence: Add Marvell IP modification changes

[Mark Brown <broonie@kernel.org>]

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

On Mon, Dec 19, 2022 at 06:42:53AM -0800, Witold Sadowski wrote:
> Add support for Marvell IP modification - clock divider,
> and PHY config, and IRQ clearing.
> Clock divider block is build into Cadence XSPI controller
> and is connected directly to 800MHz clock.
> As PHY config is not set directly in IP block, driver can
> load custom PHY configuration values.

What is a PHY in the context of a SPI controller?

> +config SPI_CADENCE_MRVL_XSPI
> +	tristate "Marvell mods for XSPI controller"
> +	depends on SPI_CADENCE_XSPI
> +
> +	help

Extra blank line (does this work?).  It's not clear to me that there's
enough code here to justify a Kconfig.

> +	/*Reset DLL*/

Please follow the kernel coding style.

> @@ -328,6 +468,9 @@ static int cdns_xspi_controller_init(struct cdns_xspi_dev *cdns_xspi)
>  		return -EIO;
>  	}
>  
> +	writel(FIELD_PREP(CDNS_XSPI_CTRL_WORK_MODE, CDNS_XSPI_WORK_MODE_STIG),
> +	       cdns_xspi->iobase + CDNS_XSPI_CTRL_CONFIG_REG);
> +

This is done unconditionally, will other instances in the IP be OK with
it?  Should it be a separate commit since it's affecting everything?

> +#if IS_ENABLED(CONFIG_SPI_CADENCE_MRVL_XSPI)
> +	writel(CDNS_MSIX_CLEAR_IRQ, cdns_xspi->auxbase + CDNS_XSPI_SPIX_INTR_AUX);
> +#endif

This is not how we do support for variants of an IP, we need to support
a single kernel image for many different systems so variant handling
needs to be done with runtime selection not build time selection.
Please handle this in a similar way to how other drivers handle support
for multiple devices.

> +#if IS_ENABLED(CONFIG_SPI_CADENCE_MRVL_XSPI)
> +static int cdns_xspi_setup(struct spi_device *spi_dev)
> +{
> +	struct cdns_xspi_dev *cdns_xspi = spi_master_get_devdata(spi_dev->master);
> +
> +	cdns_xspi_setup_clock(cdns_xspi, spi_dev->max_speed_hz);
> +
> +	return 0;
> +}
> +#endif

Note that setup() might be called while other transfers are in progress
and should not affect them.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]