Re: [PATCH v7 2/9] mei: late_bind: add late binding component driver

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Jul 08, 2025 at 12:42:30AM +0530, Badal Nilawar wrote:
> From: Alexander Usyskin <alexander.usyskin@intel.com>
> 
> Add late binding component driver.

That says what this does, but not why, or even what "late binding"
means.

> It allows pushing the late binding configuration from, for example,
> the Xe graphics driver to the Intel discrete graphics card's CSE device.
> 
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  drivers/misc/mei/Kconfig                    |  11 +
>  drivers/misc/mei/Makefile                   |   1 +
>  drivers/misc/mei/mei_late_bind.c            | 271 ++++++++++++++++++++
>  include/drm/intel/i915_component.h          |   1 +
>  include/drm/intel/late_bind_mei_interface.h |  62 +++++
>  5 files changed, 346 insertions(+)
>  create mode 100644 drivers/misc/mei/mei_late_bind.c
>  create mode 100644 include/drm/intel/late_bind_mei_interface.h
> 
> diff --git a/drivers/misc/mei/Kconfig b/drivers/misc/mei/Kconfig
> index 7575fee96cc6..36569604038c 100644
> --- a/drivers/misc/mei/Kconfig
> +++ b/drivers/misc/mei/Kconfig
> @@ -81,6 +81,17 @@ config INTEL_MEI_VSC
>  	  This driver can also be built as a module. If so, the module
>  	  will be called mei-vsc.
>  
> +config INTEL_MEI_LATE_BIND
> +	tristate "Intel late binding support on ME Interface"
> +	depends on INTEL_MEI_ME
> +	depends on DRM_XE
> +	help
> +	  MEI Support for Late Binding for Intel graphics card.
> +
> +	  Enables the ME FW interfaces for Late Binding feature,
> +	  allowing loading of firmware for the devices like Fan
> +	  Controller by Intel Xe driver.

Where is "Late Binding feature" documented so we know what that is?  Why
wouldn't it just always be enabled and why must it be a config option?

> --- /dev/null
> +++ b/include/drm/intel/late_bind_mei_interface.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright (c) 2025 Intel Corporation
> + */
> +
> +#ifndef _LATE_BIND_MEI_INTERFACE_H_
> +#define _LATE_BIND_MEI_INTERFACE_H_
> +
> +#include <linux/types.h>
> +
> +struct device;
> +struct module;

Not needed.

> +
> +/**
> + * Late Binding flags
> + * Persistent across warm reset

persistent where?

> + */
> +#define CSC_LATE_BINDING_FLAGS_IS_PERSISTENT	BIT(0)
> +
> +/**
> + * xe_late_bind_fw_type - enum to determine late binding fw type
> + */
> +enum late_bind_type {
> +	CSC_LATE_BINDING_TYPE_FAN_CONTROL = 1,
> +};

shouldn't you have mei_ as a prefix for the enum type and the values?

> +
> +/**
> + * Late Binding payload status
> + */
> +enum csc_late_binding_status {

Same here, what is "CSC"?

> +	CSC_LATE_BINDING_STATUS_SUCCESS           = 0,
> +	CSC_LATE_BINDING_STATUS_4ID_MISMATCH      = 1,
> +	CSC_LATE_BINDING_STATUS_ARB_FAILURE       = 2,
> +	CSC_LATE_BINDING_STATUS_GENERAL_ERROR     = 3,
> +	CSC_LATE_BINDING_STATUS_INVALID_PARAMS    = 4,
> +	CSC_LATE_BINDING_STATUS_INVALID_SIGNATURE = 5,
> +	CSC_LATE_BINDING_STATUS_INVALID_PAYLOAD   = 6,
> +	CSC_LATE_BINDING_STATUS_TIMEOUT           = 7,
> +};

This enum type is never used.

> +
> +/**
> + * struct late_bind_component_ops - ops for Late Binding services.
> + * @owner: Module providing the ops
> + * @push_config: Sends a config to FW.
> + */
> +struct late_bind_component_ops {
> +	/**
> +	 * @push_config: Sends a config to FW.

What is "FW"?

> +	 * @dev: device struct corresponding to the mei device

Why not pass in the mei device structure, not a 'struct device' so that
we know this is correct?

> +	 * @type: payload type
> +	 * @flags: payload flags
> +	 * @payload: payload buffer

Where are these defined?  Why are they not enums?

> +	 * @payload_size: payload buffer size

Size in what?

> +	 *
> +	 * Return: 0 success, negative errno value on transport failure,
> +	 *         positive status returned by FW
> +	 */
> +	int (*push_config)(struct device *dev, u32 type, u32 flags,
> +			   const void *payload, size_t payload_size);
> +};
> +
> +#endif /* _LATE_BIND_MEI_INTERFACE_H_ */
> -- 
> 2.34.1
>