> 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.
