Re: [PATCH 2/2] misc: keba: Add basic KEBA CP500 system FPGA support

[Greg KH <gregkh@linuxfoundation.org>]

On Sat, Jun 01, 2024 at 09:28:46PM +0200, Gerhard Engleder wrote:
> From: Gerhard Engleder <eg@keba.com>
> 
> The KEBA CP500 system FPGA is a PCIe device, which consists of multiple
> IP cores. Every IP core has its own platform driver. The cp500 driver
> registers a platform device for each device and the corresponding
> drivers are loaded by the Linux driver infrastructure.

Please use the aux bus code for this, not the platform driver code.
That's what the aux bus is explicitly for, platform devices are NOT
meant to hang off of a PCIe device at all.

> Currently 3 variants of this device exists. Every variant has its own
> PCI device ID, which is used to determine the list of available IP
> cores. In this first version only the platform device for the I2C
> controller is registered.
> 
> Besides the platform device registration some other basic functions of
> the FPGA are implemented; e.g, FPGA version sysfs file, keep FPGA
> configuration on reset sysfs file, error message for errors on the
> internal AXI bus of the FPGA.
> 
> Signed-off-by: Gerhard Engleder <eg@keba.com>
> ---
>  drivers/misc/Kconfig       |   1 +
>  drivers/misc/Makefile      |   1 +
>  drivers/misc/keba/Kconfig  |  12 +
>  drivers/misc/keba/Makefile |   3 +
>  drivers/misc/keba/cp500.c  | 433 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 450 insertions(+)
>  create mode 100644 drivers/misc/keba/Kconfig
>  create mode 100644 drivers/misc/keba/Makefile
>  create mode 100644 drivers/misc/keba/cp500.c

You create sysfs files for this driver, yet there is no
Documentation/ABI/ entries for it?  Please do so in your next version of
this series.

> +static ssize_t keep_cfg_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct cp500 *cp500 = dev_get_drvdata(dev);
> +	unsigned long keep_cfg = 1;
> +
> +	/* FPGA configuration stream is kept during reset when RECONFIG bit is
> +	 * zero
> +	 */
> +	if (ioread8(cp500->system_startup_addr + CP500_RECONFIG_REG) &
> +		CP500_RECFG_REQ)
> +		keep_cfg = 0;
> +
> +	return sprintf(buf, "%lu\n", keep_cfg);

sysfs_emit() for all sysfs show functions please.

thanks,

greg k-h