On Tue, Jul 08, 2025 at 12:42:30AM +0530, Badal Nilawar wrote:
> From: Alexander Usyskin <alexander.usyskin at 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 at intel.com>
> Signed-off-by: Badal Nilawar <badal.nilawar at intel.com>
> Reviewed-by: Anshuman Gupta <anshuman.gupta at 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
>