| Message ID | 20221226175849.13056-2-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | intel-m10-bmc: Split BMC to core and SPI parts & add PMCI+N6000 support | expand |
On Mon, 26 Dec 2022, Ilpo Järvinen wrote: > BMC type specific info is currently set by a switch/case block. The > size of this info is expected to grow as more dev types and features > are added which would have made the switch block bloaty. > > Store type specific info into struct and place them into .driver_data > instead because it makes things a bit cleaner. > > The m10bmc_type enum can be dropped as the differentiation is now > fully handled by the platform info. > > The info member of struct intel_m10bmc that is added here is not used > yet in this change but its addition logically still belongs to this > change. The CSR map change that comes after this change needs to have > the info member. > > Reviewed-by: Russ Weight <russell.h.weight@intel.com> > Reviewed-by: Xu Yilun <yilun.xu@intel.com> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > drivers/mfd/intel-m10-bmc.c | 53 ++++++++++++++----------------- > include/linux/mfd/intel-m10-bmc.h | 12 +++++++ > 2 files changed, 36 insertions(+), 29 deletions(-) > > diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c > index 7e3319e5b22f..12c522c16d83 100644 > --- a/drivers/mfd/intel-m10-bmc.c > +++ b/drivers/mfd/intel-m10-bmc.c > @@ -13,12 +13,6 @@ > #include <linux/regmap.h> > #include <linux/spi/spi.h> > > -enum m10bmc_type { > - M10_N3000, > - M10_D5005, > - M10_N5010, > -}; > - > static struct mfd_cell m10bmc_d5005_subdevs[] = { > { .name = "d5005bmc-hwmon" }, > { .name = "d5005bmc-sec-update" } > @@ -162,15 +156,17 @@ static int check_m10bmc_version(struct intel_m10bmc *ddata) > static int intel_m10_bmc_spi_probe(struct spi_device *spi) > { > const struct spi_device_id *id = spi_get_device_id(spi); > + const struct intel_m10bmc_platform_info *info; > struct device *dev = &spi->dev; > - struct mfd_cell *cells; > struct intel_m10bmc *ddata; > - int ret, n_cell; > + int ret; > > ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL); > if (!ddata) > return -ENOMEM; > > + info = (struct intel_m10bmc_platform_info *)id->driver_data; > + ddata->info = info; Why are you keeping it? > ddata->dev = dev; > > ddata->regmap = > @@ -189,24 +185,8 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi) > return ret; > } > > - switch (id->driver_data) { > - case M10_N3000: > - cells = m10bmc_pacn3000_subdevs; > - n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs); > - break; > - case M10_D5005: > - cells = m10bmc_d5005_subdevs; > - n_cell = ARRAY_SIZE(m10bmc_d5005_subdevs); > - break; > - case M10_N5010: > - cells = m10bmc_n5010_subdevs; > - n_cell = ARRAY_SIZE(m10bmc_n5010_subdevs); > - break; > - default: > - return -ENODEV; > - } > - > - ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells, n_cell, > + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, > + info->cells, info->n_cells, > NULL, 0, NULL); > if (ret) > dev_err(dev, "Failed to register sub-devices: %d\n", ret); > @@ -214,10 +194,25 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi) > return ret; > } > > +static const struct intel_m10bmc_platform_info m10bmc_spi_n3000 = { > + .cells = m10bmc_pacn3000_subdevs, > + .n_cells = ARRAY_SIZE(m10bmc_pacn3000_subdevs), > +}; Not seeing how adding a whole new structure and swapping out 4 lines to describe a device for a different 4 lines per device is better? I'm not necessarily against it. Just seems like a bit of a pointless exercise. > +static const struct intel_m10bmc_platform_info m10bmc_spi_d5005 = { > + .cells = m10bmc_d5005_subdevs, > + .n_cells = ARRAY_SIZE(m10bmc_d5005_subdevs), > +}; > + > +static const struct intel_m10bmc_platform_info m10bmc_spi_n5010 = { > + .cells = m10bmc_n5010_subdevs, > + .n_cells = ARRAY_SIZE(m10bmc_n5010_subdevs), > +}; > + > static const struct spi_device_id m10bmc_spi_id[] = { > - { "m10-n3000", M10_N3000 }, > - { "m10-d5005", M10_D5005 }, > - { "m10-n5010", M10_N5010 }, > + { "m10-n3000", (kernel_ulong_t)&m10bmc_spi_n3000 }, > + { "m10-d5005", (kernel_ulong_t)&m10bmc_spi_d5005 }, > + { "m10-n5010", (kernel_ulong_t)&m10bmc_spi_n5010 }, > { } > }; > MODULE_DEVICE_TABLE(spi, m10bmc_spi_id); > diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h > index f0044b14136e..725b51ea4aee 100644 > --- a/include/linux/mfd/intel-m10-bmc.h > +++ b/include/linux/mfd/intel-m10-bmc.h > @@ -118,14 +118,26 @@ > /* Address of 4KB inverted bit vector containing staging area FLASH count */ > #define STAGING_FLASH_COUNT 0x17ffb000 > > +/** > + * struct intel_m10bmc_platform_info - Intel MAX 10 BMC platform specific information > + * @cells: MFD cells > + * @n_cells: MFD cells ARRAY_SIZE() > + */ > +struct intel_m10bmc_platform_info { > + struct mfd_cell *cells; > + int n_cells; > +}; > + > /** > * struct intel_m10bmc - Intel MAX 10 BMC parent driver data structure > * @dev: this device > * @regmap: the regmap used to access registers by m10bmc itself > + * @info: the platform information for MAX10 BMC > */ > struct intel_m10bmc { > struct device *dev; > struct regmap *regmap; > + const struct intel_m10bmc_platform_info *info; > }; > > /* > -- > 2.30.2 >
On Mon, 9 Jan 2023, Lee Jones wrote: > On Mon, 26 Dec 2022, Ilpo Järvinen wrote: > > > BMC type specific info is currently set by a switch/case block. The > > size of this info is expected to grow as more dev types and features > > are added which would have made the switch block bloaty. > > > > Store type specific info into struct and place them into .driver_data > > instead because it makes things a bit cleaner. > > > > The m10bmc_type enum can be dropped as the differentiation is now > > fully handled by the platform info. > > > > The info member of struct intel_m10bmc that is added here is not used > > yet in this change but its addition logically still belongs to this > > change. The CSR map change that comes after this change needs to have > > the info member. > > > > Reviewed-by: Russ Weight <russell.h.weight@intel.com> > > Reviewed-by: Xu Yilun <yilun.xu@intel.com> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > --- > > drivers/mfd/intel-m10-bmc.c | 53 ++++++++++++++----------------- > > include/linux/mfd/intel-m10-bmc.h | 12 +++++++ > > 2 files changed, 36 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c > > index 7e3319e5b22f..12c522c16d83 100644 > > --- a/drivers/mfd/intel-m10-bmc.c > > +++ b/drivers/mfd/intel-m10-bmc.c > > @@ -13,12 +13,6 @@ > > #include <linux/regmap.h> > > #include <linux/spi/spi.h> > > > > -enum m10bmc_type { > > - M10_N3000, > > - M10_D5005, > > - M10_N5010, > > -}; > > - > > static struct mfd_cell m10bmc_d5005_subdevs[] = { > > { .name = "d5005bmc-hwmon" }, > > { .name = "d5005bmc-sec-update" } > > @@ -162,15 +156,17 @@ static int check_m10bmc_version(struct intel_m10bmc *ddata) > > static int intel_m10_bmc_spi_probe(struct spi_device *spi) > > { > > const struct spi_device_id *id = spi_get_device_id(spi); > > + const struct intel_m10bmc_platform_info *info; > > struct device *dev = &spi->dev; > > - struct mfd_cell *cells; > > struct intel_m10bmc *ddata; > > - int ret, n_cell; > > + int ret; > > > > ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL); > > if (!ddata) > > return -ENOMEM; > > > > + info = (struct intel_m10bmc_platform_info *)id->driver_data; > > + ddata->info = info; > > Why are you keeping it? There are plenty of users starting from patch 04. There will more users and members in the changes not included into this series. Thus, storing csr_map instead of info would not be forward-looking enough. > > ddata->dev = dev; > > > > ddata->regmap = > > @@ -189,24 +185,8 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi) > > return ret; > > } > > > > - switch (id->driver_data) { > > - case M10_N3000: > > - cells = m10bmc_pacn3000_subdevs; > > - n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs); > > - break; > > - case M10_D5005: > > - cells = m10bmc_d5005_subdevs; > > - n_cell = ARRAY_SIZE(m10bmc_d5005_subdevs); > > - break; > > - case M10_N5010: > > - cells = m10bmc_n5010_subdevs; > > - n_cell = ARRAY_SIZE(m10bmc_n5010_subdevs); > > - break; > > - default: > > - return -ENODEV; > > - } > > - > > - ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells, n_cell, > > + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, > > + info->cells, info->n_cells, > > NULL, 0, NULL); > > if (ret) > > dev_err(dev, "Failed to register sub-devices: %d\n", ret); > > @@ -214,10 +194,25 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi) > > return ret; > > } > > > > +static const struct intel_m10bmc_platform_info m10bmc_spi_n3000 = { > > + .cells = m10bmc_pacn3000_subdevs, > > + .n_cells = ARRAY_SIZE(m10bmc_pacn3000_subdevs), > > +}; > > Not seeing how adding a whole new structure and swapping out 4 lines to > describe a device for a different 4 lines per device is better? > > I'm not necessarily against it. Just seems like a bit of a pointless > exercise. After the BMC core/SPI split in a later patch in this series, there will be an init func in m10bmc core that will be called from spi side and after PMCI is added, from there too. With a structure, only a pointer to that will have to be passed to the init func rather than n parameters (there will be more members added into the info structure too both by changes in this series and in the ones not included to this series). Thanks for the review.
On Mon, 09 Jan 2023, Ilpo Järvinen wrote: > On Mon, 9 Jan 2023, Lee Jones wrote: > > > On Mon, 26 Dec 2022, Ilpo Järvinen wrote: > > > > > BMC type specific info is currently set by a switch/case block. The > > > size of this info is expected to grow as more dev types and features > > > are added which would have made the switch block bloaty. > > > > > > Store type specific info into struct and place them into .driver_data > > > instead because it makes things a bit cleaner. > > > > > > The m10bmc_type enum can be dropped as the differentiation is now > > > fully handled by the platform info. > > > > > > The info member of struct intel_m10bmc that is added here is not used > > > yet in this change but its addition logically still belongs to this > > > change. The CSR map change that comes after this change needs to have > > > the info member. > > > > > > Reviewed-by: Russ Weight <russell.h.weight@intel.com> > > > Reviewed-by: Xu Yilun <yilun.xu@intel.com> > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > --- > > > drivers/mfd/intel-m10-bmc.c | 53 ++++++++++++++----------------- > > > include/linux/mfd/intel-m10-bmc.h | 12 +++++++ > > > 2 files changed, 36 insertions(+), 29 deletions(-) > > > > > > diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c > > > index 7e3319e5b22f..12c522c16d83 100644 > > > --- a/drivers/mfd/intel-m10-bmc.c > > > +++ b/drivers/mfd/intel-m10-bmc.c > > > @@ -13,12 +13,6 @@ > > > #include <linux/regmap.h> > > > #include <linux/spi/spi.h> > > > > > > -enum m10bmc_type { > > > - M10_N3000, > > > - M10_D5005, > > > - M10_N5010, > > > -}; > > > - > > > static struct mfd_cell m10bmc_d5005_subdevs[] = { > > > { .name = "d5005bmc-hwmon" }, > > > { .name = "d5005bmc-sec-update" } > > > @@ -162,15 +156,17 @@ static int check_m10bmc_version(struct intel_m10bmc *ddata) > > > static int intel_m10_bmc_spi_probe(struct spi_device *spi) > > > { > > > const struct spi_device_id *id = spi_get_device_id(spi); > > > + const struct intel_m10bmc_platform_info *info; > > > struct device *dev = &spi->dev; > > > - struct mfd_cell *cells; > > > struct intel_m10bmc *ddata; > > > - int ret, n_cell; > > > + int ret; > > > > > > ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL); > > > if (!ddata) > > > return -ENOMEM; > > > > > > + info = (struct intel_m10bmc_platform_info *)id->driver_data; > > > + ddata->info = info; > > > > Why are you keeping it? > > There are plenty of users starting from patch 04. There will more users > and members in the changes not included into this series. Thus, storing > csr_map instead of info would not be forward-looking enough. > > > > ddata->dev = dev; > > > > > > ddata->regmap = > > > @@ -189,24 +185,8 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi) > > > return ret; > > > } > > > > > > - switch (id->driver_data) { > > > - case M10_N3000: > > > - cells = m10bmc_pacn3000_subdevs; > > > - n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs); > > > - break; > > > - case M10_D5005: > > > - cells = m10bmc_d5005_subdevs; > > > - n_cell = ARRAY_SIZE(m10bmc_d5005_subdevs); > > > - break; > > > - case M10_N5010: > > > - cells = m10bmc_n5010_subdevs; > > > - n_cell = ARRAY_SIZE(m10bmc_n5010_subdevs); > > > - break; > > > - default: > > > - return -ENODEV; > > > - } > > > - > > > - ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells, n_cell, > > > + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, > > > + info->cells, info->n_cells, > > > NULL, 0, NULL); > > > if (ret) > > > dev_err(dev, "Failed to register sub-devices: %d\n", ret); > > > @@ -214,10 +194,25 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi) > > > return ret; > > > } > > > > > > +static const struct intel_m10bmc_platform_info m10bmc_spi_n3000 = { > > > + .cells = m10bmc_pacn3000_subdevs, > > > + .n_cells = ARRAY_SIZE(m10bmc_pacn3000_subdevs), > > > +}; > > > > Not seeing how adding a whole new structure and swapping out 4 lines to > > describe a device for a different 4 lines per device is better? > > > > I'm not necessarily against it. Just seems like a bit of a pointless > > exercise. > > After the BMC core/SPI split in a later patch in this series, there will > be an init func in m10bmc core that will be called from spi side and > after PMCI is added, from there too. > > With a structure, only a pointer to that will have to be passed to the > init func rather than n parameters (there will be more members added into > the info structure too both by changes in this series and in the ones not > included to this series). Very well. Please consider these review comments as tentative, until I get a chance to dig deeper into the patch-set.
diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c index 7e3319e5b22f..12c522c16d83 100644 --- a/drivers/mfd/intel-m10-bmc.c +++ b/drivers/mfd/intel-m10-bmc.c @@ -13,12 +13,6 @@ #include <linux/regmap.h> #include <linux/spi/spi.h> -enum m10bmc_type { - M10_N3000, - M10_D5005, - M10_N5010, -}; - static struct mfd_cell m10bmc_d5005_subdevs[] = { { .name = "d5005bmc-hwmon" }, { .name = "d5005bmc-sec-update" } @@ -162,15 +156,17 @@ static int check_m10bmc_version(struct intel_m10bmc *ddata) static int intel_m10_bmc_spi_probe(struct spi_device *spi) { const struct spi_device_id *id = spi_get_device_id(spi); + const struct intel_m10bmc_platform_info *info; struct device *dev = &spi->dev; - struct mfd_cell *cells; struct intel_m10bmc *ddata; - int ret, n_cell; + int ret; ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL); if (!ddata) return -ENOMEM; + info = (struct intel_m10bmc_platform_info *)id->driver_data; + ddata->info = info; ddata->dev = dev; ddata->regmap = @@ -189,24 +185,8 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi) return ret; } - switch (id->driver_data) { - case M10_N3000: - cells = m10bmc_pacn3000_subdevs; - n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs); - break; - case M10_D5005: - cells = m10bmc_d5005_subdevs; - n_cell = ARRAY_SIZE(m10bmc_d5005_subdevs); - break; - case M10_N5010: - cells = m10bmc_n5010_subdevs; - n_cell = ARRAY_SIZE(m10bmc_n5010_subdevs); - break; - default: - return -ENODEV; - } - - ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells, n_cell, + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, + info->cells, info->n_cells, NULL, 0, NULL); if (ret) dev_err(dev, "Failed to register sub-devices: %d\n", ret); @@ -214,10 +194,25 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi) return ret; } +static const struct intel_m10bmc_platform_info m10bmc_spi_n3000 = { + .cells = m10bmc_pacn3000_subdevs, + .n_cells = ARRAY_SIZE(m10bmc_pacn3000_subdevs), +}; + +static const struct intel_m10bmc_platform_info m10bmc_spi_d5005 = { + .cells = m10bmc_d5005_subdevs, + .n_cells = ARRAY_SIZE(m10bmc_d5005_subdevs), +}; + +static const struct intel_m10bmc_platform_info m10bmc_spi_n5010 = { + .cells = m10bmc_n5010_subdevs, + .n_cells = ARRAY_SIZE(m10bmc_n5010_subdevs), +}; + static const struct spi_device_id m10bmc_spi_id[] = { - { "m10-n3000", M10_N3000 }, - { "m10-d5005", M10_D5005 }, - { "m10-n5010", M10_N5010 }, + { "m10-n3000", (kernel_ulong_t)&m10bmc_spi_n3000 }, + { "m10-d5005", (kernel_ulong_t)&m10bmc_spi_d5005 }, + { "m10-n5010", (kernel_ulong_t)&m10bmc_spi_n5010 }, { } }; MODULE_DEVICE_TABLE(spi, m10bmc_spi_id); diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h index f0044b14136e..725b51ea4aee 100644 --- a/include/linux/mfd/intel-m10-bmc.h +++ b/include/linux/mfd/intel-m10-bmc.h @@ -118,14 +118,26 @@ /* Address of 4KB inverted bit vector containing staging area FLASH count */ #define STAGING_FLASH_COUNT 0x17ffb000 +/** + * struct intel_m10bmc_platform_info - Intel MAX 10 BMC platform specific information + * @cells: MFD cells + * @n_cells: MFD cells ARRAY_SIZE() + */ +struct intel_m10bmc_platform_info { + struct mfd_cell *cells; + int n_cells; +}; + /** * struct intel_m10bmc - Intel MAX 10 BMC parent driver data structure * @dev: this device * @regmap: the regmap used to access registers by m10bmc itself + * @info: the platform information for MAX10 BMC */ struct intel_m10bmc { struct device *dev; struct regmap *regmap; + const struct intel_m10bmc_platform_info *info; }; /*