Messages in this thread | | | From | "Yang, Fei" <> | Subject | RE: [PATCH] IOSF: Add interface for the cases requiring fid | Date | Tue, 12 Apr 2016 00:18:36 +0000 |
| |
> 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.
| |