* [PATCH] 2c: i801: Improve handling of chip-specific feature definitions @ 2021-11-08 20:10 Heiner Kallweit 2021-11-18 10:09 ` Jean Delvare 2021-11-18 10:11 ` Jean Delvare 0 siblings, 2 replies; 6+ messages in thread From: Heiner Kallweit @ 2021-11-08 20:10 UTC (permalink / raw) To: Jean Delvare; +Cc: linux-i2c@vger.kernel.org Reduce source code and code size by defining the chip features statically. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- checkpatch complains about the format of the table but I think better readability justifies the formatting. --- drivers/i2c/busses/i2c-i801.c | 191 ++++++++++++---------------------- 1 file changed, 66 insertions(+), 125 deletions(-) diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index f078e75dd..4c96f1b47 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -990,66 +990,72 @@ static const struct i2c_algorithm smbus_algorithm = { .functionality = i801_func, }; +#define DEF_FEATURES (FEATURE_BLOCK_PROC | FEATURE_I2C_BLOCK_READ | \ + FEATURE_IRQ | FEATURE_SMBUS_PEC | \ + FEATURE_BLOCK_BUFFER | FEATURE_HOST_NOTIFY) +#define FEATURES_82801DB (FEATURE_SMBUS_PEC | FEATURE_BLOCK_BUFFER | \ + FEATURE_HOST_NOTIFY) + static const struct pci_device_id i801_ids[] = { - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AA_3) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AB_3) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801BA_2) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801CA_3) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801DB_3) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801EB_3) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ESB_4) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_16) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH7_17) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ESB2_17) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH8_5) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH9_6) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_EP80579_1) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH10_4) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH10_5) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_5_3400_SERIES_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF0) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF1) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF2) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_DH89XXCC_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PANTHERPOINT_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LYNXPOINT_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LYNXPOINT_LP_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_AVOTON_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_WELLSBURG_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_WELLSBURG_SMBUS_MS0) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_WELLSBURG_SMBUS_MS1) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_WELLSBURG_SMBUS_MS2) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_COLETOCREEK_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_GEMINILAKE_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_WILDCATPOINT_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_WILDCATPOINT_LP_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BAYTRAIL_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BRASWELL_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CDF_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_DNV_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_EBG_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BROXTON_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LEWISBURG_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LEWISBURG_SSKU_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_KABYLAKE_PCH_H_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CANNONLAKE_H_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CANNONLAKE_LP_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICELAKE_LP_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICELAKE_N_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_COMETLAKE_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_COMETLAKE_H_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_COMETLAKE_V_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ELKHART_LAKE_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_TIGERLAKE_LP_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_TIGERLAKE_H_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_JASPER_LAKE_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ALDER_LAKE_S_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ALDER_LAKE_P_SMBUS) }, - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ALDER_LAKE_M_SMBUS) }, + { PCI_DEVICE_DATA(INTEL, 82801AA_3, 0 ) }, + { PCI_DEVICE_DATA(INTEL, 82801AB_3, 0 ) }, + { PCI_DEVICE_DATA(INTEL, 82801BA_2, 0 ) }, + { PCI_DEVICE_DATA(INTEL, 82801CA_3, FEATURE_HOST_NOTIFY ) }, + { PCI_DEVICE_DATA(INTEL, 82801DB_3, FEATURES_82801DB ) }, + { PCI_DEVICE_DATA(INTEL, 82801EB_3, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, ESB_4, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, ICH6_16, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, ICH7_17, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, ESB2_17, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, ICH8_5, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, ICH9_6, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, EP80579_1, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, ICH10_4, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, ICH10_5, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, 5_3400_SERIES_SMBUS, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, COUGARPOINT_SMBUS, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, PATSBURG_SMBUS, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, PATSBURG_SMBUS_IDF0, DEF_FEATURES | FEATURE_IDF ) }, + { PCI_DEVICE_DATA(INTEL, PATSBURG_SMBUS_IDF1, DEF_FEATURES | FEATURE_IDF ) }, + { PCI_DEVICE_DATA(INTEL, PATSBURG_SMBUS_IDF2, DEF_FEATURES | FEATURE_IDF ) }, + { PCI_DEVICE_DATA(INTEL, DH89XXCC_SMBUS, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, PANTHERPOINT_SMBUS, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, LYNXPOINT_SMBUS, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, LYNXPOINT_LP_SMBUS, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, AVOTON_SMBUS, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, WELLSBURG_SMBUS, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, WELLSBURG_SMBUS_MS0, DEF_FEATURES | FEATURE_IDF ) }, + { PCI_DEVICE_DATA(INTEL, WELLSBURG_SMBUS_MS1, DEF_FEATURES | FEATURE_IDF ) }, + { PCI_DEVICE_DATA(INTEL, WELLSBURG_SMBUS_MS2, DEF_FEATURES | FEATURE_IDF ) }, + { PCI_DEVICE_DATA(INTEL, COLETOCREEK_SMBUS, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, GEMINILAKE_SMBUS, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, WILDCATPOINT_SMBUS, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, WILDCATPOINT_LP_SMBUS, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, BAYTRAIL_SMBUS, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, BRASWELL_SMBUS, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, SUNRISEPOINT_H_SMBUS, DEF_FEATURES | FEATURE_TCO_SPT ) }, + { PCI_DEVICE_DATA(INTEL, SUNRISEPOINT_LP_SMBUS, DEF_FEATURES | FEATURE_TCO_SPT ) }, + { PCI_DEVICE_DATA(INTEL, CDF_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, + { PCI_DEVICE_DATA(INTEL, DNV_SMBUS, DEF_FEATURES | FEATURE_TCO_SPT ) }, + { PCI_DEVICE_DATA(INTEL, EBG_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, + { PCI_DEVICE_DATA(INTEL, BROXTON_SMBUS, DEF_FEATURES ) }, + { PCI_DEVICE_DATA(INTEL, LEWISBURG_SMBUS, DEF_FEATURES | FEATURE_TCO_SPT ) }, + { PCI_DEVICE_DATA(INTEL, LEWISBURG_SSKU_SMBUS, DEF_FEATURES | FEATURE_TCO_SPT ) }, + { PCI_DEVICE_DATA(INTEL, KABYLAKE_PCH_H_SMBUS, DEF_FEATURES | FEATURE_TCO_SPT ) }, + { PCI_DEVICE_DATA(INTEL, CANNONLAKE_H_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, + { PCI_DEVICE_DATA(INTEL, CANNONLAKE_LP_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, + { PCI_DEVICE_DATA(INTEL, ICELAKE_LP_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, + { PCI_DEVICE_DATA(INTEL, ICELAKE_N_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, + { PCI_DEVICE_DATA(INTEL, COMETLAKE_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, + { PCI_DEVICE_DATA(INTEL, COMETLAKE_H_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, + { PCI_DEVICE_DATA(INTEL, COMETLAKE_V_SMBUS, DEF_FEATURES | FEATURE_TCO_SPT ) }, + { PCI_DEVICE_DATA(INTEL, ELKHART_LAKE_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, + { PCI_DEVICE_DATA(INTEL, TIGERLAKE_LP_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, + { PCI_DEVICE_DATA(INTEL, TIGERLAKE_H_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, + { PCI_DEVICE_DATA(INTEL, JASPER_LAKE_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, + { PCI_DEVICE_DATA(INTEL, ALDER_LAKE_S_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, + { PCI_DEVICE_DATA(INTEL, ALDER_LAKE_P_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, + { PCI_DEVICE_DATA(INTEL, ALDER_LAKE_M_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, { 0, } }; @@ -1678,72 +1684,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) mutex_init(&priv->acpi_lock); priv->pci_dev = dev; - switch (dev->device) { - case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS: - case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS: - case PCI_DEVICE_ID_INTEL_LEWISBURG_SMBUS: - case PCI_DEVICE_ID_INTEL_LEWISBURG_SSKU_SMBUS: - case PCI_DEVICE_ID_INTEL_DNV_SMBUS: - case PCI_DEVICE_ID_INTEL_KABYLAKE_PCH_H_SMBUS: - case PCI_DEVICE_ID_INTEL_COMETLAKE_V_SMBUS: - priv->features |= FEATURE_BLOCK_PROC; - priv->features |= FEATURE_I2C_BLOCK_READ; - priv->features |= FEATURE_IRQ; - priv->features |= FEATURE_SMBUS_PEC; - priv->features |= FEATURE_BLOCK_BUFFER; - priv->features |= FEATURE_TCO_SPT; - priv->features |= FEATURE_HOST_NOTIFY; - break; - - case PCI_DEVICE_ID_INTEL_CANNONLAKE_H_SMBUS: - case PCI_DEVICE_ID_INTEL_CANNONLAKE_LP_SMBUS: - case PCI_DEVICE_ID_INTEL_CDF_SMBUS: - case PCI_DEVICE_ID_INTEL_ICELAKE_LP_SMBUS: - case PCI_DEVICE_ID_INTEL_ICELAKE_N_SMBUS: - case PCI_DEVICE_ID_INTEL_COMETLAKE_SMBUS: - case PCI_DEVICE_ID_INTEL_COMETLAKE_H_SMBUS: - case PCI_DEVICE_ID_INTEL_ELKHART_LAKE_SMBUS: - case PCI_DEVICE_ID_INTEL_TIGERLAKE_LP_SMBUS: - case PCI_DEVICE_ID_INTEL_TIGERLAKE_H_SMBUS: - case PCI_DEVICE_ID_INTEL_JASPER_LAKE_SMBUS: - case PCI_DEVICE_ID_INTEL_EBG_SMBUS: - case PCI_DEVICE_ID_INTEL_ALDER_LAKE_S_SMBUS: - case PCI_DEVICE_ID_INTEL_ALDER_LAKE_P_SMBUS: - case PCI_DEVICE_ID_INTEL_ALDER_LAKE_M_SMBUS: - priv->features |= FEATURE_BLOCK_PROC; - priv->features |= FEATURE_I2C_BLOCK_READ; - priv->features |= FEATURE_IRQ; - priv->features |= FEATURE_SMBUS_PEC; - priv->features |= FEATURE_BLOCK_BUFFER; - priv->features |= FEATURE_TCO_CNL; - priv->features |= FEATURE_HOST_NOTIFY; - break; - - case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF0: - case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF1: - case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF2: - case PCI_DEVICE_ID_INTEL_WELLSBURG_SMBUS_MS0: - case PCI_DEVICE_ID_INTEL_WELLSBURG_SMBUS_MS1: - case PCI_DEVICE_ID_INTEL_WELLSBURG_SMBUS_MS2: - priv->features |= FEATURE_IDF; - fallthrough; - default: - priv->features |= FEATURE_BLOCK_PROC; - priv->features |= FEATURE_I2C_BLOCK_READ; - priv->features |= FEATURE_IRQ; - fallthrough; - case PCI_DEVICE_ID_INTEL_82801DB_3: - priv->features |= FEATURE_SMBUS_PEC; - priv->features |= FEATURE_BLOCK_BUFFER; - fallthrough; - case PCI_DEVICE_ID_INTEL_82801CA_3: - priv->features |= FEATURE_HOST_NOTIFY; - fallthrough; - case PCI_DEVICE_ID_INTEL_82801BA_2: - case PCI_DEVICE_ID_INTEL_82801AB_3: - case PCI_DEVICE_ID_INTEL_82801AA_3: - break; - } + priv->features = id->driver_data; /* Disable features on user request */ for (i = 0; i < ARRAY_SIZE(i801_feature_names); i++) { -- 2.33.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] 2c: i801: Improve handling of chip-specific feature definitions 2021-11-08 20:10 [PATCH] 2c: i801: Improve handling of chip-specific feature definitions Heiner Kallweit @ 2021-11-18 10:09 ` Jean Delvare 2021-11-18 14:03 ` Jarkko Nikula 2021-11-18 10:11 ` Jean Delvare 1 sibling, 1 reply; 6+ messages in thread From: Jean Delvare @ 2021-11-18 10:09 UTC (permalink / raw) To: Heiner Kallweit; +Cc: linux-i2c, Jarkko Nikula Hi Heiner, On Mon, 08 Nov 2021 21:10:12 +0100, Heiner Kallweit wrote: > Reduce source code and code size by defining the chip features > statically. While I don't like the PCI_DEVICE_DATA macro implementation (for it breaks grepping for PCI defines), I generally enjoy more data and less code. So I am fine with this change. Jarkko, you are typically the one adding support for new devices to this driver so this change will affect you. Are you OK with that change? > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > checkpatch complains about the format of the table but I think > better readability justifies the formatting. This approach has the drawback that we would have to modify all lines if we ever need to add one line which is longer (which will inevitably happen if we ever have to add a new feature to the driver). I think you can make the table both readable and compliant. The complaint is only about the space between the last parameter and the closing parenthesis. You can still align the parameters vertically, which is the only thing that really matters IMHO. > --- > drivers/i2c/busses/i2c-i801.c | 191 ++++++++++++---------------------- > 1 file changed, 66 insertions(+), 125 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index f078e75dd..4c96f1b47 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -990,66 +990,72 @@ static const struct i2c_algorithm smbus_algorithm = { > .functionality = i801_func, > }; > > +#define DEF_FEATURES (FEATURE_BLOCK_PROC | FEATURE_I2C_BLOCK_READ | \ Not a good name ("default" isn't descriptive) and not consistent either. I suggest "FEATURES_82801EB" instead, as this is the first chipset which supported all these features. And you can make the definitions of FEATURES_82801DB and FEATURES_82801EB consistent (spacing/alignment). > + FEATURE_IRQ | FEATURE_SMBUS_PEC | \ > + FEATURE_BLOCK_BUFFER | FEATURE_HOST_NOTIFY) > +#define FEATURES_82801DB (FEATURE_SMBUS_PEC | FEATURE_BLOCK_BUFFER | \ > + FEATURE_HOST_NOTIFY) > + > static const struct pci_device_id i801_ids[] = { > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AA_3) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801AB_3) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801BA_2) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801CA_3) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801DB_3) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801EB_3) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ESB_4) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH6_16) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH7_17) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ESB2_17) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH8_5) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH9_6) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_EP80579_1) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH10_4) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH10_5) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_5_3400_SERIES_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF0) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF1) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF2) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_DH89XXCC_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PANTHERPOINT_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LYNXPOINT_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LYNXPOINT_LP_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_AVOTON_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_WELLSBURG_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_WELLSBURG_SMBUS_MS0) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_WELLSBURG_SMBUS_MS1) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_WELLSBURG_SMBUS_MS2) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_COLETOCREEK_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_GEMINILAKE_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_WILDCATPOINT_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_WILDCATPOINT_LP_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BAYTRAIL_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BRASWELL_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CDF_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_DNV_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_EBG_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BROXTON_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LEWISBURG_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LEWISBURG_SSKU_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_KABYLAKE_PCH_H_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CANNONLAKE_H_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CANNONLAKE_LP_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICELAKE_LP_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICELAKE_N_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_COMETLAKE_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_COMETLAKE_H_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_COMETLAKE_V_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ELKHART_LAKE_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_TIGERLAKE_LP_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_TIGERLAKE_H_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_JASPER_LAKE_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ALDER_LAKE_S_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ALDER_LAKE_P_SMBUS) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ALDER_LAKE_M_SMBUS) }, > + { PCI_DEVICE_DATA(INTEL, 82801AA_3, 0 ) }, > + { PCI_DEVICE_DATA(INTEL, 82801AB_3, 0 ) }, > + { PCI_DEVICE_DATA(INTEL, 82801BA_2, 0 ) }, > + { PCI_DEVICE_DATA(INTEL, 82801CA_3, FEATURE_HOST_NOTIFY ) }, > + { PCI_DEVICE_DATA(INTEL, 82801DB_3, FEATURES_82801DB ) }, > + { PCI_DEVICE_DATA(INTEL, 82801EB_3, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, ESB_4, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, ICH6_16, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, ICH7_17, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, ESB2_17, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, ICH8_5, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, ICH9_6, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, EP80579_1, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, ICH10_4, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, ICH10_5, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, 5_3400_SERIES_SMBUS, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, COUGARPOINT_SMBUS, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, PATSBURG_SMBUS, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, PATSBURG_SMBUS_IDF0, DEF_FEATURES | FEATURE_IDF ) }, > + { PCI_DEVICE_DATA(INTEL, PATSBURG_SMBUS_IDF1, DEF_FEATURES | FEATURE_IDF ) }, > + { PCI_DEVICE_DATA(INTEL, PATSBURG_SMBUS_IDF2, DEF_FEATURES | FEATURE_IDF ) }, > + { PCI_DEVICE_DATA(INTEL, DH89XXCC_SMBUS, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, PANTHERPOINT_SMBUS, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, LYNXPOINT_SMBUS, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, LYNXPOINT_LP_SMBUS, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, AVOTON_SMBUS, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, WELLSBURG_SMBUS, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, WELLSBURG_SMBUS_MS0, DEF_FEATURES | FEATURE_IDF ) }, > + { PCI_DEVICE_DATA(INTEL, WELLSBURG_SMBUS_MS1, DEF_FEATURES | FEATURE_IDF ) }, > + { PCI_DEVICE_DATA(INTEL, WELLSBURG_SMBUS_MS2, DEF_FEATURES | FEATURE_IDF ) }, > + { PCI_DEVICE_DATA(INTEL, COLETOCREEK_SMBUS, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, GEMINILAKE_SMBUS, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, WILDCATPOINT_SMBUS, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, WILDCATPOINT_LP_SMBUS, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, BAYTRAIL_SMBUS, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, BRASWELL_SMBUS, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, SUNRISEPOINT_H_SMBUS, DEF_FEATURES | FEATURE_TCO_SPT ) }, > + { PCI_DEVICE_DATA(INTEL, SUNRISEPOINT_LP_SMBUS, DEF_FEATURES | FEATURE_TCO_SPT ) }, > + { PCI_DEVICE_DATA(INTEL, CDF_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, > + { PCI_DEVICE_DATA(INTEL, DNV_SMBUS, DEF_FEATURES | FEATURE_TCO_SPT ) }, > + { PCI_DEVICE_DATA(INTEL, EBG_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, > + { PCI_DEVICE_DATA(INTEL, BROXTON_SMBUS, DEF_FEATURES ) }, > + { PCI_DEVICE_DATA(INTEL, LEWISBURG_SMBUS, DEF_FEATURES | FEATURE_TCO_SPT ) }, > + { PCI_DEVICE_DATA(INTEL, LEWISBURG_SSKU_SMBUS, DEF_FEATURES | FEATURE_TCO_SPT ) }, > + { PCI_DEVICE_DATA(INTEL, KABYLAKE_PCH_H_SMBUS, DEF_FEATURES | FEATURE_TCO_SPT ) }, > + { PCI_DEVICE_DATA(INTEL, CANNONLAKE_H_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, > + { PCI_DEVICE_DATA(INTEL, CANNONLAKE_LP_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, > + { PCI_DEVICE_DATA(INTEL, ICELAKE_LP_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, > + { PCI_DEVICE_DATA(INTEL, ICELAKE_N_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, > + { PCI_DEVICE_DATA(INTEL, COMETLAKE_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, > + { PCI_DEVICE_DATA(INTEL, COMETLAKE_H_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, > + { PCI_DEVICE_DATA(INTEL, COMETLAKE_V_SMBUS, DEF_FEATURES | FEATURE_TCO_SPT ) }, > + { PCI_DEVICE_DATA(INTEL, ELKHART_LAKE_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, > + { PCI_DEVICE_DATA(INTEL, TIGERLAKE_LP_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, > + { PCI_DEVICE_DATA(INTEL, TIGERLAKE_H_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, > + { PCI_DEVICE_DATA(INTEL, JASPER_LAKE_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, > + { PCI_DEVICE_DATA(INTEL, ALDER_LAKE_S_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, > + { PCI_DEVICE_DATA(INTEL, ALDER_LAKE_P_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, > + { PCI_DEVICE_DATA(INTEL, ALDER_LAKE_M_SMBUS, DEF_FEATURES | FEATURE_TCO_CNL ) }, > { 0, } > }; > > @@ -1678,72 +1684,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) > mutex_init(&priv->acpi_lock); > > priv->pci_dev = dev; > - switch (dev->device) { > - case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS: > - case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS: > - case PCI_DEVICE_ID_INTEL_LEWISBURG_SMBUS: > - case PCI_DEVICE_ID_INTEL_LEWISBURG_SSKU_SMBUS: > - case PCI_DEVICE_ID_INTEL_DNV_SMBUS: > - case PCI_DEVICE_ID_INTEL_KABYLAKE_PCH_H_SMBUS: > - case PCI_DEVICE_ID_INTEL_COMETLAKE_V_SMBUS: > - priv->features |= FEATURE_BLOCK_PROC; > - priv->features |= FEATURE_I2C_BLOCK_READ; > - priv->features |= FEATURE_IRQ; > - priv->features |= FEATURE_SMBUS_PEC; > - priv->features |= FEATURE_BLOCK_BUFFER; > - priv->features |= FEATURE_TCO_SPT; > - priv->features |= FEATURE_HOST_NOTIFY; > - break; > - > - case PCI_DEVICE_ID_INTEL_CANNONLAKE_H_SMBUS: > - case PCI_DEVICE_ID_INTEL_CANNONLAKE_LP_SMBUS: > - case PCI_DEVICE_ID_INTEL_CDF_SMBUS: > - case PCI_DEVICE_ID_INTEL_ICELAKE_LP_SMBUS: > - case PCI_DEVICE_ID_INTEL_ICELAKE_N_SMBUS: > - case PCI_DEVICE_ID_INTEL_COMETLAKE_SMBUS: > - case PCI_DEVICE_ID_INTEL_COMETLAKE_H_SMBUS: > - case PCI_DEVICE_ID_INTEL_ELKHART_LAKE_SMBUS: > - case PCI_DEVICE_ID_INTEL_TIGERLAKE_LP_SMBUS: > - case PCI_DEVICE_ID_INTEL_TIGERLAKE_H_SMBUS: > - case PCI_DEVICE_ID_INTEL_JASPER_LAKE_SMBUS: > - case PCI_DEVICE_ID_INTEL_EBG_SMBUS: > - case PCI_DEVICE_ID_INTEL_ALDER_LAKE_S_SMBUS: > - case PCI_DEVICE_ID_INTEL_ALDER_LAKE_P_SMBUS: > - case PCI_DEVICE_ID_INTEL_ALDER_LAKE_M_SMBUS: > - priv->features |= FEATURE_BLOCK_PROC; > - priv->features |= FEATURE_I2C_BLOCK_READ; > - priv->features |= FEATURE_IRQ; > - priv->features |= FEATURE_SMBUS_PEC; > - priv->features |= FEATURE_BLOCK_BUFFER; > - priv->features |= FEATURE_TCO_CNL; > - priv->features |= FEATURE_HOST_NOTIFY; > - break; > - > - case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF0: > - case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF1: > - case PCI_DEVICE_ID_INTEL_PATSBURG_SMBUS_IDF2: > - case PCI_DEVICE_ID_INTEL_WELLSBURG_SMBUS_MS0: > - case PCI_DEVICE_ID_INTEL_WELLSBURG_SMBUS_MS1: > - case PCI_DEVICE_ID_INTEL_WELLSBURG_SMBUS_MS2: > - priv->features |= FEATURE_IDF; > - fallthrough; > - default: > - priv->features |= FEATURE_BLOCK_PROC; > - priv->features |= FEATURE_I2C_BLOCK_READ; > - priv->features |= FEATURE_IRQ; > - fallthrough; > - case PCI_DEVICE_ID_INTEL_82801DB_3: > - priv->features |= FEATURE_SMBUS_PEC; > - priv->features |= FEATURE_BLOCK_BUFFER; > - fallthrough; > - case PCI_DEVICE_ID_INTEL_82801CA_3: > - priv->features |= FEATURE_HOST_NOTIFY; > - fallthrough; > - case PCI_DEVICE_ID_INTEL_82801BA_2: > - case PCI_DEVICE_ID_INTEL_82801AB_3: > - case PCI_DEVICE_ID_INTEL_82801AA_3: > - break; > - } > + priv->features = id->driver_data; > > /* Disable features on user request */ > for (i = 0; i < ARRAY_SIZE(i801_feature_names); i++) { Looks good to me, thanks. -- Jean Delvare SUSE L3 Support ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] 2c: i801: Improve handling of chip-specific feature definitions 2021-11-18 10:09 ` Jean Delvare @ 2021-11-18 14:03 ` Jarkko Nikula 2021-11-18 15:14 ` Jean Delvare 0 siblings, 1 reply; 6+ messages in thread From: Jarkko Nikula @ 2021-11-18 14:03 UTC (permalink / raw) To: Jean Delvare, Heiner Kallweit; +Cc: linux-i2c On 11/18/21 12:09 PM, Jean Delvare wrote: > Hi Heiner, > > On Mon, 08 Nov 2021 21:10:12 +0100, Heiner Kallweit wrote: >> Reduce source code and code size by defining the chip features >> statically. > > While I don't like the PCI_DEVICE_DATA macro implementation (for it > breaks grepping for PCI defines), I generally enjoy more data and less > code. So I am fine with this change. > > Jarkko, you are typically the one adding support for new devices to > this driver so this change will affect you. Are you OK with that change? > I think it makes code more readable and less error prone when adding support for new devices and merging with other upstream changes. I remember one such accident: fd4b204a0971 ("i2c: i801: Bring back Block Process Call support for certain platforms") >> +#define DEF_FEATURES (FEATURE_BLOCK_PROC | FEATURE_I2C_BLOCK_READ | \ > > Not a good name ("default" isn't descriptive) and not consistent > either. I suggest "FEATURES_82801EB" instead, as this is the first > chipset which supported all these features. And you can make the > definitions of FEATURES_82801DB and FEATURES_82801EB consistent > (spacing/alignment). > How about calling default as FEATURES_ICH5 and 82801DB as FEATURES_ICH4? That makes easier to follow comments like "/* ICH4 and later */" in the code. Jarkko ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] 2c: i801: Improve handling of chip-specific feature definitions 2021-11-18 14:03 ` Jarkko Nikula @ 2021-11-18 15:14 ` Jean Delvare 2021-11-18 15:16 ` Heiner Kallweit 0 siblings, 1 reply; 6+ messages in thread From: Jean Delvare @ 2021-11-18 15:14 UTC (permalink / raw) To: Jarkko Nikula; +Cc: Heiner Kallweit, linux-i2c On Thu, 18 Nov 2021 16:03:39 +0200, Jarkko Nikula wrote: > On 11/18/21 12:09 PM, Jean Delvare wrote: > > On Mon, 08 Nov 2021 21:10:12 +0100, Heiner Kallweit wrote: > >> +#define DEF_FEATURES (FEATURE_BLOCK_PROC | FEATURE_I2C_BLOCK_READ | \ > > > > Not a good name ("default" isn't descriptive) and not consistent > > either. I suggest "FEATURES_82801EB" instead, as this is the first > > chipset which supported all these features. And you can make the > > definitions of FEATURES_82801DB and FEATURES_82801EB consistent > > (spacing/alignment). > > > How about calling default as FEATURES_ICH5 and 82801DB as FEATURES_ICH4? > That makes easier to follow comments like "/* ICH4 and later */" in the > code. Good idea :-) -- Jean Delvare SUSE L3 Support ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] 2c: i801: Improve handling of chip-specific feature definitions 2021-11-18 15:14 ` Jean Delvare @ 2021-11-18 15:16 ` Heiner Kallweit 0 siblings, 0 replies; 6+ messages in thread From: Heiner Kallweit @ 2021-11-18 15:16 UTC (permalink / raw) To: Jean Delvare, Jarkko Nikula; +Cc: linux-i2c On 18.11.2021 16:14, Jean Delvare wrote: > On Thu, 18 Nov 2021 16:03:39 +0200, Jarkko Nikula wrote: >> On 11/18/21 12:09 PM, Jean Delvare wrote: >>> On Mon, 08 Nov 2021 21:10:12 +0100, Heiner Kallweit wrote: >>>> +#define DEF_FEATURES (FEATURE_BLOCK_PROC | FEATURE_I2C_BLOCK_READ | \ >>> >>> Not a good name ("default" isn't descriptive) and not consistent >>> either. I suggest "FEATURES_82801EB" instead, as this is the first >>> chipset which supported all these features. And you can make the >>> definitions of FEATURES_82801DB and FEATURES_82801EB consistent >>> (spacing/alignment). >>> >> How about calling default as FEATURES_ICH5 and 82801DB as FEATURES_ICH4? >> That makes easier to follow comments like "/* ICH4 and later */" in the >> code. > > Good idea :-) > Thanks for the reviews. Then I'll incorporate the remarks and send a v2. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] 2c: i801: Improve handling of chip-specific feature definitions 2021-11-08 20:10 [PATCH] 2c: i801: Improve handling of chip-specific feature definitions Heiner Kallweit 2021-11-18 10:09 ` Jean Delvare @ 2021-11-18 10:11 ` Jean Delvare 1 sibling, 0 replies; 6+ messages in thread From: Jean Delvare @ 2021-11-18 10:11 UTC (permalink / raw) To: Heiner Kallweit; +Cc: linux-i2c And I forgot in my review: you have a typo in the subject line ("2c" -> "i2c"). -- Jean Delvare SUSE L3 Support ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-18 15:16 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-11-08 20:10 [PATCH] 2c: i801: Improve handling of chip-specific feature definitions Heiner Kallweit 2021-11-18 10:09 ` Jean Delvare 2021-11-18 14:03 ` Jarkko Nikula 2021-11-18 15:14 ` Jean Delvare 2021-11-18 15:16 ` Heiner Kallweit 2021-11-18 10:11 ` Jean Delvare
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).