Re: [PATCH 01/14] spi: spi-fsl-dspi: Define regmaps per device

[Vladimir Oltean <olteanv@gmail.com>]

On Fri, May 09, 2025 at 12:05:48PM +0100, James Clark wrote:
>  static const struct fsl_dspi_devtype_data devtype_data[] = {
>  	[VF610] = {
>  		.trans_mode		= DSPI_DMA_MODE,
>  		.max_clock_factor	= 2,
>  		.fifo_size		= 4,
> +		.regmap			= &dspi_regmap_config[DSPI_REGMAP]

Comma at the end, please. Just like you didn't have to modify the
previous line to add this new assignment, so shouldn't any future
contributor. The comment applies throughout the entire patch set.

>  	},
>  	[LS1021A] = {
>  		/* Has A-011218 DMA erratum */
>  		.trans_mode		= DSPI_XSPI_MODE,
>  		.max_clock_factor	= 8,
>  		.fifo_size		= 4,
> +		.regmap			= &dspi_regmap_config[DSPI_XSPI_REGMAP]
>  	},
>  	[LS1012A] = {
>  		/* Has A-011218 DMA erratum */
>  		.trans_mode		= DSPI_XSPI_MODE,
>  		.max_clock_factor	= 8,
>  		.fifo_size		= 16,
> +		.regmap			= &dspi_regmap_config[DSPI_XSPI_REGMAP]
>  	},
>  	[LS1028A] = {
>  		.trans_mode		= DSPI_XSPI_MODE,
>  		.max_clock_factor	= 8,
>  		.fifo_size		= 4,
> +		.regmap			= &dspi_regmap_config[DSPI_XSPI_REGMAP]
>  	},
>  	[LS1043A] = {
>  		/* Has A-011218 DMA erratum */
>  		.trans_mode		= DSPI_XSPI_MODE,
>  		.max_clock_factor	= 8,
>  		.fifo_size		= 16,
> +		.regmap			= &dspi_regmap_config[DSPI_XSPI_REGMAP]
>  	},
>  	[LS1046A] = {
>  		/* Has A-011218 DMA erratum */
>  		.trans_mode		= DSPI_XSPI_MODE,
>  		.max_clock_factor	= 8,
>  		.fifo_size		= 16,
> +		.regmap			= &dspi_regmap_config[DSPI_XSPI_REGMAP]
>  	},
>  	[LS2080A] = {
>  		.trans_mode		= DSPI_XSPI_MODE,
>  		.max_clock_factor	= 8,
>  		.fifo_size		= 4,
> +		.regmap			= &dspi_regmap_config[DSPI_XSPI_REGMAP]
>  	},
>  	[LS2085A] = {
>  		.trans_mode		= DSPI_XSPI_MODE,
>  		.max_clock_factor	= 8,
>  		.fifo_size		= 4,
> +		.regmap			= &dspi_regmap_config[DSPI_XSPI_REGMAP]
>  	},
>  	[LX2160A] = {
>  		.trans_mode		= DSPI_XSPI_MODE,
>  		.max_clock_factor	= 8,
>  		.fifo_size		= 4,
> +		.regmap			= &dspi_regmap_config[DSPI_XSPI_REGMAP]
>  	},
>  	[MCF5441X] = {
>  		.trans_mode		= DSPI_DMA_MODE,
>  		.max_clock_factor	= 8,
>  		.fifo_size		= 16,
> +		.regmap			= &dspi_regmap_config[DSPI_REGMAP]
>  	},
>  };
>  
> @@ -1167,54 +1231,6 @@ static int dspi_resume(struct device *dev)
>  
>  static SIMPLE_DEV_PM_OPS(dspi_pm, dspi_suspend, dspi_resume);
>  
> -static const struct regmap_range dspi_volatile_ranges[] = {
> -	regmap_reg_range(SPI_MCR, SPI_TCR),
> -	regmap_reg_range(SPI_SR, SPI_SR),
> -	regmap_reg_range(SPI_PUSHR, SPI_RXFR3),
> -};
> -
> -static const struct regmap_access_table dspi_volatile_table = {
> -	.yes_ranges	= dspi_volatile_ranges,
> -	.n_yes_ranges	= ARRAY_SIZE(dspi_volatile_ranges),
> -};
> -
> -static const struct regmap_config dspi_regmap_config = {
> -	.reg_bits	= 32,
> -	.val_bits	= 32,
> -	.reg_stride	= 4,
> -	.max_register	= 0x88,
> -	.volatile_table	= &dspi_volatile_table,
> -};
> -
> -static const struct regmap_range dspi_xspi_volatile_ranges[] = {
> -	regmap_reg_range(SPI_MCR, SPI_TCR),
> -	regmap_reg_range(SPI_SR, SPI_SR),
> -	regmap_reg_range(SPI_PUSHR, SPI_RXFR3),
> -	regmap_reg_range(SPI_SREX, SPI_SREX),
> -};
> -
> -static const struct regmap_access_table dspi_xspi_volatile_table = {
> -	.yes_ranges	= dspi_xspi_volatile_ranges,
> -	.n_yes_ranges	= ARRAY_SIZE(dspi_xspi_volatile_ranges),
> -};
> -
> -static const struct regmap_config dspi_xspi_regmap_config[] = {
> -	{
> -		.reg_bits	= 32,
> -		.val_bits	= 32,
> -		.reg_stride	= 4,
> -		.max_register	= 0x13c,
> -		.volatile_table	= &dspi_xspi_volatile_table,
> -	},
> -	{
> -		.name		= "pushr",
> -		.reg_bits	= 16,
> -		.val_bits	= 16,
> -		.reg_stride	= 2,
> -		.max_register	= 0x2,
> -	},
> -};
> -
>  static int dspi_init(struct fsl_dspi *dspi)
>  {
>  	unsigned int mcr;
> @@ -1272,7 +1288,6 @@ static int dspi_target_abort(struct spi_controller *host)
>  static int dspi_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> -	const struct regmap_config *regmap_config;
>  	struct fsl_dspi_platform_data *pdata;
>  	struct spi_controller *ctlr;
>  	int ret, cs_num, bus_num = -1;
> @@ -1355,11 +1370,7 @@ static int dspi_probe(struct platform_device *pdev)
>  		goto out_ctlr_put;
>  	}
>  
> -	if (dspi->devtype_data->trans_mode == DSPI_XSPI_MODE)
> -		regmap_config = &dspi_xspi_regmap_config[0];
> -	else
> -		regmap_config = &dspi_regmap_config;
> -	dspi->regmap = devm_regmap_init_mmio(&pdev->dev, base, regmap_config);
> +	dspi->regmap = devm_regmap_init_mmio(&pdev->dev, base, dspi->devtype_data->regmap);

I know there are other coding conventions floating around, but for this
driver please try to stick to a limit of ~80 characters limit for lines
which don't contain strings.

>  	if (IS_ERR(dspi->regmap)) {
>  		dev_err(&pdev->dev, "failed to init regmap: %ld\n",
>  				PTR_ERR(dspi->regmap));
> @@ -1370,7 +1381,7 @@ static int dspi_probe(struct platform_device *pdev)
>  	if (dspi->devtype_data->trans_mode == DSPI_XSPI_MODE) {
>  		dspi->regmap_pushr = devm_regmap_init_mmio(
>  			&pdev->dev, base + SPI_PUSHR,
> -			&dspi_xspi_regmap_config[1]);
> +			&dspi_regmap_config[DSPI_PUSHR]);
>  		if (IS_ERR(dspi->regmap_pushr)) {
>  			dev_err(&pdev->dev,
>  				"failed to init pushr regmap: %ld\n",
> 
> -- 
> 2.34.1
> 

With the change request addressed, please add my tag and keep it in
subsequent submissions.

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>