Re: [PATCH v4 3/8] i2c: piix4: Export i2c_piix4 driver functions as library

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

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