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
