diff mbox series

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

Message ID 20240911115407.1090046-4-Shyam-sundar.S-k@amd.com
State Changes Requested
Delegated to: Andi Shyti
Headers show
Series Introduce initial AMD ASF Controller driver support | expand

Commit Message

Shyam Sundar S K Sept. 11, 2024, 11:54 a.m. UTC 
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.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i2c/busses/i2c-piix4.c | 14 ++++++++------
 drivers/i2c/busses/i2c-piix4.h |  5 +++++
 2 files changed, 13 insertions(+), 6 deletions(-)

Comments

Andi Shyti Sept. 12, 2024, 7:40 a.m. UTC | #1 
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.

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.

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.

...
Shyam Sundar S K Sept. 13, 2024, 6:24 a.m. UTC | #2 
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.
> 
> ...
Andi Shyti Sept. 13, 2024, 8:36 a.m. UTC | #3 
Hi Shyam,

On Fri, Sep 13, 2024 at 11:54:55AM GMT, Shyam Sundar S K wrote:
> On 9/12/2024 13:10, Andi Shyti wrote:
> > 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?

The change I am referring to is that style change I described
here below, that consists in putting in one line the functions.

You are describing the logical changes, but there is no mention
of the fact that you are moving the second parameter of the
function in the same line with the other.

> > 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,
Andi
diff mbox series

Patch

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)
 {
 	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.
diff --git a/drivers/i2c/busses/i2c-piix4.h b/drivers/i2c/busses/i2c-piix4.h
index c4c20edacb00..9a5faac3eedd 100644
--- a/drivers/i2c/busses/i2c-piix4.h
+++ b/drivers/i2c/busses/i2c-piix4.h
@@ -37,4 +37,9 @@  struct sb800_mmio_cfg {
 	bool use_mmio;
 };
 
+int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg);
+int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short piix4_smba);
+int piix4_sb800_region_request(struct device *dev, struct sb800_mmio_cfg *mmio_cfg);
+void piix4_sb800_region_release(struct device *dev, struct sb800_mmio_cfg *mmio_cfg);
+
 #endif /* I2C_PIIX4_H */