Hi Andi,
On 9/12/2024 13:10, Andi Shyti wrote:
> Hi Shyam,
>
> On Wed, Sep 11, 2024 at 05:24:02PM GMT, Shyam Sundar S K wrote:
>> Export the following i2c_piix4 driver functions as a library so that the
>> AMD ASF driver can utilize these core functionalities from the i2c_piix4
>> driver:
>>
>> - piix4_sb800_region_request(): Request access to a specific SMBus region
>> on the SB800 chipset.
>>
>> - piix4_sb800_region_release(): Release the previously requested SMBus
>> region on the SB800 chipset.
>>
>> - piix4_transaction(): Handle SMBus transactions between the SMBus
>> controller and connected devices.
>>
>> - piix4_sb800_port_sel(): Select the appropriate SMBus port on the SB800
>> chipset.
>>
>> By making these functions available as a library, enable the AMD ASF
>> driver to leverage the established mechanisms in the i2c_piix4 driver,
>> promoting code reuse and consistency across different drivers.
>
> ...
>
>> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
>> index 2c2a466e2f85..174cce254e96 100644
>> --- a/drivers/i2c/busses/i2c-piix4.c
>> +++ b/drivers/i2c/busses/i2c-piix4.c
>> @@ -153,8 +153,7 @@ struct i2c_piix4_adapdata {
>> struct sb800_mmio_cfg mmio_cfg;
>> };
>>
>> -static int piix4_sb800_region_request(struct device *dev,
>> - struct sb800_mmio_cfg *mmio_cfg)
>> +int piix4_sb800_region_request(struct device *dev, struct sb800_mmio_cfg *mmio_cfg)
>
> I’m not entirely happy with this change, or the others above. If
> someone runs a git bisect, they would be confused by not seeing
> this change described in the commit log.
Can you elaborate a bit more on the expectation? The commit message
has the details on the each of the functions that has to be exported.
Can you please take a look at it again?
>
> While it's true that the accepted line length is now 100
> characters, the 80-character limit is still preferred (and
> personally, I prefer 80, though that’s just my opinion).
>
> This change doesn’t seem necessary, so please amend it along with
> the others below in the next version.
Ok. I will move to 80 char length.
Thanks,
Shyam
>
> Thanks,
> Andi
>
>> {
>> if (mmio_cfg->use_mmio) {
>> void __iomem *addr;
>> @@ -192,9 +191,9 @@ static int piix4_sb800_region_request(struct device *dev,
>>
>> return 0;
>> }
>> +EXPORT_SYMBOL_GPL(piix4_sb800_region_request);
>>
>> -static void piix4_sb800_region_release(struct device *dev,
>> - struct sb800_mmio_cfg *mmio_cfg)
>> +void piix4_sb800_region_release(struct device *dev, struct sb800_mmio_cfg *mmio_cfg)
>> {
>> if (mmio_cfg->use_mmio) {
>> iounmap(mmio_cfg->addr);
>> @@ -205,6 +204,7 @@ static void piix4_sb800_region_release(struct device *dev,
>>
>> release_region(SB800_PIIX4_SMB_IDX, SB800_PIIX4_SMB_MAP_SIZE);
>> }
>> +EXPORT_SYMBOL_GPL(piix4_sb800_region_release);
>>
>> static bool piix4_sb800_use_mmio(struct pci_dev *PIIX4_dev)
>> {
>> @@ -514,7 +514,7 @@ static int piix4_setup_aux(struct pci_dev *PIIX4_dev,
>> return piix4_smba;
>> }
>>
>> -static int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short piix4_smba)
>> +int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short piix4_smba)
>> {
>> int temp;
>> int result = 0;
>> @@ -587,6 +587,7 @@ static int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short p
>> inb_p(SMBHSTDAT1));
>> return result;
>> }
>> +EXPORT_SYMBOL_GPL(piix4_transaction);
>>
>> /* Return negative errno on error. */
>> static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
>> @@ -740,7 +741,7 @@ static void piix4_imc_wakeup(void)
>> release_region(KERNCZ_IMC_IDX, 2);
>> }
>>
>> -static int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg)
>> +int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg)
>> {
>> u8 smba_en_lo, val;
>>
>> @@ -762,6 +763,7 @@ static int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg)
>>
>> return (smba_en_lo & piix4_port_mask_sb800);
>> }
>> +EXPORT_SYMBOL_GPL(piix4_sb800_port_sel);
>>
>> /*
>> * Handles access to multiple SMBus ports on the SB800.
>
> ...
