Re: [PATCH 01/10] spi: espi_amd: Add AMD eSPI controller driver support

[Mark Brown]

On Fri, Mar 14, 2025 at 12:04:31AM +0530, Raju Rangoju wrote:

> @@ -159,6 +159,8 @@ obj-$(CONFIG_SPI_XTENSA_XTFPGA)		+= spi-xtensa-xtfpga.o
>  obj-$(CONFIG_SPI_ZYNQ_QSPI)		+= spi-zynq-qspi.o
>  obj-$(CONFIG_SPI_ZYNQMP_GQSPI)		+= spi-zynqmp-gqspi.o
>  obj-$(CONFIG_SPI_AMD)			+= spi-amd.o
> +obj-$(CONFIG_SPI_AMD_ESPI)		+= espi-amd.o
> +espi-amd-objs				:= espi-amd-core.o espi-amd-dev.o
>  

Please keep these files sorted.

> @@ -0,0 +1,883 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * AMD eSPI controller driver

Please make the entire comment block a C++ one to make things look more
consistent.

> + *
> + * Copyright (c) 2025, Advanced Micro Devices, Inc.
> + * All Rights Reserved.

Are you sure?

> +static int amd_espi_check_error_status(struct amd_espi *amd_espi, u32 status)
> +{
> +	int ret = CB_SUCCESS;
> +
> +	if (!(status & ESPI_DNCMD_INT)) {
> +		ret =  ESPI_DNCMD_INT;
> +		dev_err(amd_espi->dev, "eSPI downstream command completion failure\n");
> +	} else if (status & ESPI_BUS_ERR_INT) {
> +		ret = ESPI_BUS_ERR_INT;
> +		dev_err(amd_espi->dev, "%s\n", espi_error_codes[POS_BUS_TIMING]);

Can we really only have one error flagged at once?  The whole
espi_error_codes thing also seems like unneeded complexity and fagility,
this function is the only place they're used and there's nothing
ensuring that the defines for indexing into the array have anything to
do with the strings in there.

> +int amd_espi_set_iomode(struct amd_espi *amd_espi, u32 *slave_config, u32 *ctrlr_config,
> +			u8 io_mode)
> +{
> +	struct espi_master *master = amd_espi->master;

There's a lot of outdated terminology like this in the driver - while
sometimes it's unavoidable due to the register map it's better to use
more modern terms like controller and device when it's just pure
software things.

> +	ret = amd_espi_get_config(amd_espi, ESPI_SLAVE_PERIPH_CFG, &slave_config);
> +	if (ret != CB_SUCCESS)
> +		return ret;
> +
> +	/* Check if PC is already enabled */
> +	if (slave_config & ESPI_SLAVE_CHANNEL_ENABLE)
> +		return CB_SUCCESS;

Is there any great reason to use these non-standard CB_ return codes?

> +static int amd_espi_suspend(struct device *dev)
> +{
> +	return 0;
> +}

Remove empty functions, if they can safely be empty the functions will
be optional.

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -EOPNOTSUPP;
> +
> +	amd_espi->io_remap_addr = devm_ioremap_resource(dev, res);

dev_platform_get_and_ioremap_resource().

> +	amd_espi_control_reg_init(amd_espi);
> +	ret = amd_espi_init_slave(amd_espi);
> +	if (ret != CB_SUCCESS)
> +		goto espidev_free;
> +
> +	dev_info(dev, "AMD ESPI device initialization completed\n");

This is just noise, remove it.

> +
> +	return 0;
> +
> +espidev_free:
> +	amd_espi_device_remove(amd_espi);
> +	return ret;

This will return your non-standard error codes to generic code.

> +static void amd_espi_remove(struct platform_device *pdev)
> +{
> +	struct amd_espi *amd_espi = platform_get_drvdata(pdev);
> +
> +	amd_espi_device_remove(amd_espi);
> +}

There's no need for this wrapper function, there's exactly one place we
can call remove from.

> +static int amd_espi_open(struct inode *inode, struct file *filp)
> +{
> +	struct amd_espi *espi;
> +	int status = -ENXIO;
> +
> +	guard(mutex)(&device_list_lock);

Whatever this userspace ABI is for it should be added in a separate
patch, most likely it shouldn't be here at all and standard interfaces
should be used.  Currently it doesn't seem to actually do anything.

Attachment: signature.asc
Description: PGP signature