[PATCH v6 02/10] mei: late_bind: add late binding component driver
Nilawar, Badal
badal.nilawar at intel.com
Fri Jul 4 11:48:46 UTC 2025
On 04-07-2025 16:04, Greg KH wrote:
> On Fri, Jul 04, 2025 at 03:59:40PM +0530, Nilawar, Badal wrote:
>> On 04-07-2025 10:44, Greg KH wrote:
>>> On Fri, Jul 04, 2025 at 01:00:58AM +0530, Badal Nilawar wrote:
>>>> From: Alexander Usyskin <alexander.usyskin at intel.com>
>>>>
>>>> Add late binding component driver.
>>>> 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 | 1 +
>>>> drivers/misc/mei/Makefile | 1 +
>>>> drivers/misc/mei/late_bind/Kconfig | 13 +
>>>> drivers/misc/mei/late_bind/Makefile | 9 +
>>>> drivers/misc/mei/late_bind/mei_late_bind.c | 272 ++++++++++++++++++++
>>> Why do you have a whole subdir for a single .c file? What's wrong with
>>> just keepign it in drivers/misc/mei/ ?
>> There is separate subdir for each component used by i915/xe, so one was
>> created for late_bind as well. Should we still drop late_bind subdir?
>>
>> cd drivers/misc/mei/
>> gsc_proxy/ hdcp/ late_bind/ pxp/
> For "modules" that are just a single file, yeah, that's silly, don't do
> that.
Another reason to maintain the sub_dir is to accommodate additional
files for future platforms. If you still insist, I'll remove the sub_dir.
>
>>>> +/**
>>>> + * struct csc_heci_late_bind_req - late binding request
>>>> + * @header: @ref mkhi_msg_hdr
>>>> + * @type: type of the late binding payload
>>>> + * @flags: flags to be passed to the firmware
>>>> + * @reserved: reserved field
>>> Reserved for what? Set to what?
>> Reserved by firmware for future use, default value set to 0, I will update
>> above doc.
>>
>>>> + * @payload_size: size of the payload data in bytes
>>>> + * @payload: data to be sent to the firmware
>>>> + */
>>>> +struct csc_heci_late_bind_req {
>>>> + struct mkhi_msg_hdr header;
>>>> + u32 type;
>>>> + u32 flags;
>>>> + u32 reserved[2];
>>>> + u32 payload_size;
>>> As these cross the kernel boundry, they should be the correct type
>>> (__u32), but really, please define the endiness of them (__le32) and use
>>> the proper macros for that.
>> If we go with __le32 then while populating elements of structure
>> csc_heci_late_bind_req I will be using cpu_to_le32().
>>
>> When mapping the response buffer from the firmware with struct
>> csc_heci_late_bind_rsp, there's no need to use le32_to_cpu() since the
>> response will already be in little-endian format.
> How do you know? Where is that defined? Where did the conversion
> happen?
Sorry, I got confused. Conversion is needed when assigning the response
structure elements.
e.g ret = (int)(le32_to_cpu)rsp.status;
>
>> Are you fine with this?
> Please be explicit.
>
>>>> + ret = (int)rsp.status;
>>>> +end:
>>>> + mei_cldev_disable(cldev);
>>>> + kfree(req);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static const struct late_bind_component_ops mei_late_bind_ops = {
>>>> + .owner = THIS_MODULE,
>>> I thought you were going to drop the .owner stuff?
>>>
>>> Or if not, please implement it properly (i.e. by NOT forcing people to
>>> manually set it here.)
>> Somehow I missed this. I will drop it.
> And from the structure definition please.
Sure.
Thanks,
Badal
>
> thanks,
>
> greg k-h
More information about the dri-devel
mailing list