> 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 <[email protected]> > > > > 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 <[email protected]> > > Signed-off-by: Badal Nilawar <[email protected]> > > Reviewed-by: Anshuman Gupta <[email protected]> > > --- > > 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 > >
