RE: [PATCH] IOSF: Add interface for the cases requiring fid

[Yang, Fei]

> In subject better to use x86/platform/iosf_mbi prefix.
Will update commit message.

>> Some implementations may require an additional step for setting the 
>> FID
> What FID stands for?
Function ID defined in IOSF specification. Will add more detail in the updated commit message.

>> Change-Id: Ic0227f9e74133a3203aa08e8471939f905d19622
> This should not be here.
Will remove

>> --- a/arch/x86/platform/intel/iosf_mbi.c
>> +++ b/arch/x86/platform/intel/iosf_mbi.c
>> @@ -98,6 +98,24 @@ fail_write:
>> Â	return result;
>> Â}
>> Â
>> +static int iosf_mbi_pci_write_fid(u32 fid)
> Function name should continue already used pattern, i.e.
> â_write_mcrp()
Will change.

>> +{
>> +	int result;
>> +
>> +	if (!mbi_pdev)
>> +		return -ENODEV;
>> +


>> +	result = pci_write_config_dword(mbi_pdev, MBI_MCRP_OFFSET,
>> fid);

> The function of one line.
> So, please, inline it directly where it's used.
Not sure I understand this one, do you meant something like this?
static inline int iosf_mbi_pci_write_fid(u32 fid)


>> +/*
>> + * Some IP blocks require fid to access their registers.
> Any user?
> This API doesn't make much sense without user.
The driver that uses this API is a DRM based display controller driver, which is not currently in the upstream.
But this API is a prerequisite of pushing the display driver upstream.


>> + * fid value is programmed through MCRP register, see above function
>> + * iosf_mbi_pci_write_fid() for details.
>> + */
>> +int iosf_mbi_read_with_fid(u32 fid, u8 port, u8 opcode, u32 offset,
>> u32 *mdr)
> Name kinda fuzzy. How user should know which one to choose? Does fid ==
> 0 work for some cases? We have to think about API and naming.
This is SoC dependent. The drivers accessing registers through IOSF sideband has to
know if fid is required.


>> +	/*Access to the GFX unit is handled by GPU code */
> Spaces.
Will add.

>> +	/*Access to the GFX unit is handled by GPU code */
> Ditto.
Will add.

>> +	mcr = iosf_mbi_form_mcr(opcode, port, offset & MBI_MASK_LO);
>> +	mcrx = offset & MBI_MASK_HI;
>> +
>> +	spin_lock_irqsave(&iosf_mbi_lock, flags);
>> +	ret = iosf_mbi_pci_write_fid(fid);
>> +	if (!ret)
>> +		ret = iosf_mbi_pci_write_mdr(mcrx, mcr, mdr);
>> +	spin_unlock_irqrestore(&iosf_mbi_lock, flags);
> Both of them quite similar to already exist _write()/_read(). Is it possible to combine them out?
Trying to avoid modifying existing code that uses the _read/_write API.