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

[Usyskin, Alexander]

> Subject: Re: [PATCH v7 2/9] mei: late_bind: add late binding component driver
> 
> 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.
> 
Will rephrase and add explanations.

> > 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
Will push part of cover letter here for better explanations

> wouldn't it just always be enabled and why must it be a config option?
This will add the module with a driver on MSI bus to the system.
I suppose some people want to have minimal config.

> 
> > --- /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.
Will drop, thx

> 
> > +
> > +/**
> > + * Late Binding flags
> > + * Persistent across warm reset
> 
> persistent where?
Persistent in firmware storage, will rephrase

> 
> > + */
> > +#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?
> 
This is a bridge between mei and graphics drivers, so mei is not a right prefix.
I'll look for better prefix here.

> > +
> > +/**
> > + * 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?
Component consumer only knows this device.
It has no knowledge about mei internal device.

> 
> > +	 * @type: payload type
> > +	 * @flags: payload flags
> > +	 * @payload: payload buffer
> 
> Where are these defined?  Why are they not enums?
It is bitmap, will add this information.
The lone available bit is defined at the beginning of this file
> 
> > +	 * @payload_size: payload buffer size
> 
> Size in what?
In bytes, will specify

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