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

[Rangoju, Raju]

On 3/17/2025 7:36 PM, Mark Brown wrote:
> 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.
Sure Mark, we will address this in V2.

> > @@ -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.
Will address in V2.

> > + *
> > + * Copyright (c) 2025, Advanced Micro Devices, Inc.
> > + * All Rights Reserved.
>
> Are you sure?
Yes, we've used the same copyright text in other AMD drivers/files.

> > +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.

The purpose of maintaining error codes is to inform the user space 
application about the reason for the command failure. The user space 
application can then use these error codes to determine the cause of the 
command failure.

> > +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.

Will address in V2.

> > +	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?
Sure, will address in V2.

> > +static int amd_espi_suspend(struct device *dev)
> > +{
> > +	return 0;
> > +}
>
> Remove empty functions, if they can safely be empty the functions will
> be optional.
Will address in V2.

> > +	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().
Will address in V2.

> > +	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.

Sure

> > +
> > +	return 0;
> > +
> > +espidev_free:
> > +	amd_espi_device_remove(amd_espi);
> > +	return ret;
>
> This will return your non-standard error codes to generic code.
Will address in V2

> > +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.
We have moved all device-related operations to a common file espi-amd-dev.c.

Therefore, the device remove callback is invoked from the driver remove 
function during un-initialization.

> > +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.

Since we are not registering the driver with spi framework and going 
with char dev file for user interface, all the dependent functions 
required including open, close and ioctls for char dev are placed in 
this file.