diff mbox series

[02/12] mfd: intel-m10-bmc: Create m10bmc_platform_info for type specific info

Message ID 20221108144305.45424-3-ilpo.jarvinen@linux.intel.com
State New
Headers
Series intel-m10-bmc: Split BMC to core and SPI parts & add PMCI+N6000 support |

Commit Message

Ilpo Järvinen Nov. 8, 2022, 2:42 p.m. UTC 
  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.

Reviewed-by: Russ Weight <russell.h.weight@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/mfd/intel-m10-bmc.c       | 50 +++++++++++++++++--------------
 include/linux/mfd/intel-m10-bmc.h | 14 +++++++++
 2 files changed, 41 insertions(+), 23 deletions(-)

Comments

Xu Yilun Nov. 11, 2022, 10:12 a.m. UTC | #1 
On 2022-11-08 at 16:42:55 +0200, 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.
> 
> Reviewed-by: Russ Weight <russell.h.weight@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/mfd/intel-m10-bmc.c       | 50 +++++++++++++++++--------------
>  include/linux/mfd/intel-m10-bmc.h | 14 +++++++++
>  2 files changed, 41 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
> index ee167c5dcd29..762808906380 100644
> --- a/drivers/mfd/intel-m10-bmc.c
> +++ b/drivers/mfd/intel-m10-bmc.c
> @@ -156,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;

Where to use the ddata->info?

>  	ddata->dev = dev;
>  
>  	ddata->regmap =
> @@ -183,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);
> @@ -208,10 +194,28 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
>  	return ret;
>  }
>  
> +static const struct intel_m10bmc_platform_info m10bmc_m10_n3000 = {
> +	.type = M10_N3000,

Is the type enum still useful? Found no usage.

Thanks,
Yilun

> +	.cells = m10bmc_pacn3000_subdevs,
> +	.n_cells = ARRAY_SIZE(m10bmc_pacn3000_subdevs),
> +};
> +
> +static const struct intel_m10bmc_platform_info m10bmc_m10_d5005 = {
> +	.type = M10_D5005,
> +	.cells = m10bmc_d5005_subdevs,
> +	.n_cells = ARRAY_SIZE(m10bmc_d5005_subdevs),
> +};
> +
> +static const struct intel_m10bmc_platform_info m10bmc_m10_n5010 = {
> +	.type = M10_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_m10_n3000 },
> +	{ "m10-d5005", (kernel_ulong_t)&m10bmc_m10_d5005 },
> +	{ "m10-n5010", (kernel_ulong_t)&m10bmc_m10_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 d77383a8a410..e58115f514b9 100644
> --- a/include/linux/mfd/intel-m10-bmc.h
> +++ b/include/linux/mfd/intel-m10-bmc.h
> @@ -124,14 +124,28 @@ enum m10bmc_type {
>  /* 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
> + * @type: the type of MAX10 BMC
> + * @cells: MFD cells
> + * @n_cells: MFD cells ARRAY_SIZE()
> + */
> +struct intel_m10bmc_platform_info {
> +	enum m10bmc_type type;
> +	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
>
Ilpo Järvinen Nov. 11, 2022, 11:49 a.m. UTC | #2 
On Fri, 11 Nov 2022, Xu Yilun wrote:

> On 2022-11-08 at 16:42:55 +0200, 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.
> > 
> > Reviewed-by: Russ Weight <russell.h.weight@intel.com>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >  drivers/mfd/intel-m10-bmc.c       | 50 +++++++++++++++++--------------
> >  include/linux/mfd/intel-m10-bmc.h | 14 +++++++++
> >  2 files changed, 41 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
> > index ee167c5dcd29..762808906380 100644
> > --- a/drivers/mfd/intel-m10-bmc.c
> > +++ b/drivers/mfd/intel-m10-bmc.c
> > @@ -156,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;
> 
> Where to use the ddata->info?

In patch 5/12 there are many these constructs:
const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;

Now that I look though, this particular line is altered by the split patch 
4/12 so it would be not strictly necessary to do it here. I'd prefer, 
however, still to add it here even if it's technically not used until 
after the split 5/12 patch because it very much logically belongs to this 
change.

> >  	ddata->dev = dev;
> >  
> >  	ddata->regmap =
> > @@ -183,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);
> > @@ -208,10 +194,28 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
> >  	return ret;
> >  }
> >  
> > +static const struct intel_m10bmc_platform_info m10bmc_m10_n3000 = {
> > +	.type = M10_N3000,
> 
> Is the type enum still useful? Found no usage.

There's no use within context of this patch series. However, I think there 
might have been something depending on it in the changes that are not part 
of this series so I left it in place for now.
Xu Yilun Nov. 14, 2022, 1:58 a.m. UTC | #3 
On 2022-11-11 at 13:49:38 +0200, Ilpo Järvinen wrote:
> On Fri, 11 Nov 2022, Xu Yilun wrote:
> 
> > On 2022-11-08 at 16:42:55 +0200, 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.
> > > 
> > > Reviewed-by: Russ Weight <russell.h.weight@intel.com>
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > ---
> > >  drivers/mfd/intel-m10-bmc.c       | 50 +++++++++++++++++--------------
> > >  include/linux/mfd/intel-m10-bmc.h | 14 +++++++++
> > >  2 files changed, 41 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
> > > index ee167c5dcd29..762808906380 100644
> > > --- a/drivers/mfd/intel-m10-bmc.c
> > > +++ b/drivers/mfd/intel-m10-bmc.c
> > > @@ -156,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;
> > 
> > Where to use the ddata->info?
> 
> In patch 5/12 there are many these constructs:
> const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
> 
> Now that I look though, this particular line is altered by the split patch 
> 4/12 so it would be not strictly necessary to do it here. I'd prefer, 
> however, still to add it here even if it's technically not used until 
> after the split 5/12 patch because it very much logically belongs to this 
> change.

It's good to me.

> 
> > >  	ddata->dev = dev;
> > >  
> > >  	ddata->regmap =
> > > @@ -183,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);
> > > @@ -208,10 +194,28 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
> > >  	return ret;
> > >  }
> > >  
> > > +static const struct intel_m10bmc_platform_info m10bmc_m10_n3000 = {
> > > +	.type = M10_N3000,
> > 
> > Is the type enum still useful? Found no usage.
> 
> There's no use within context of this patch series. However, I think there 
> might have been something depending on it in the changes that are not part 
> of this series so I left it in place for now.

I'm not sure how it would be used later. This patch is to eliminate the
"switch (board type) case" block, but similar code is still to be added
later?

Thanks,
Yilun
Russ Weight Nov. 15, 2022, 1:17 a.m. UTC | #4 
On 11/13/22 17:58, Xu Yilun wrote:
> On 2022-11-11 at 13:49:38 +0200, Ilpo Järvinen wrote:
>> On Fri, 11 Nov 2022, Xu Yilun wrote:
>>
>>> On 2022-11-08 at 16:42:55 +0200, 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.
>>>>
>>>> Reviewed-by: Russ Weight <russell.h.weight@intel.com>
>>>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>>> ---
>>>>  drivers/mfd/intel-m10-bmc.c       | 50 +++++++++++++++++--------------
>>>>  include/linux/mfd/intel-m10-bmc.h | 14 +++++++++
>>>>  2 files changed, 41 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
>>>> index ee167c5dcd29..762808906380 100644
>>>> --- a/drivers/mfd/intel-m10-bmc.c
>>>> +++ b/drivers/mfd/intel-m10-bmc.c
>>>> @@ -156,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;
>>> Where to use the ddata->info?
>> In patch 5/12 there are many these constructs:
>> const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
>>
>> Now that I look though, this particular line is altered by the split patch 
>> 4/12 so it would be not strictly necessary to do it here. I'd prefer, 
>> however, still to add it here even if it's technically not used until 
>> after the split 5/12 patch because it very much logically belongs to this 
>> change.
> It's good to me.
>
>>>>  	ddata->dev = dev;
>>>>  
>>>>  	ddata->regmap =
>>>> @@ -183,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);
>>>> @@ -208,10 +194,28 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
>>>>  	return ret;
>>>>  }
>>>>  
>>>> +static const struct intel_m10bmc_platform_info m10bmc_m10_n3000 = {
>>>> +	.type = M10_N3000,
>>> Is the type enum still useful? Found no usage.
>> There's no use within context of this patch series. However, I think there 
>> might have been something depending on it in the changes that are not part 
>> of this series so I left it in place for now.
> I'm not sure how it would be used later. This patch is to eliminate the
> "switch (board type) case" block, but similar code is still to be added
> later?

Unfortunately, these will be needed later. Consider the following (future)
function that has to account for a field that was moved from one register
to another:

    static int
    m10bmc_sec_status(struct m10bmc_sec *sec, u32 *status)
    {
            u32 reg_offset, reg_value;
            int ret;

            reg_offset = (sec->type == N6000BMC_SEC) ?
                    auth_result_reg(sec->m10bmc) : doorbell_reg(sec->m10bmc);

            ret = m10bmc_sys_read(sec->m10bmc, reg_offset, &reg_value);
            if (ret)
                    return ret;

            *status = rsu_stat(reg_value);

            return 0;
    }

With this patch-set, most conditionals are removed, but there will still
be some cases where it is needed. If you prefer, we could wait and add
the type in when we are ready to use it.

- Russ

>
> Thanks,
> Yilun
Xu Yilun Nov. 15, 2022, 1:55 a.m. UTC | #5 
On 2022-11-14 at 17:17:06 -0800, Russ Weight wrote:
> 
> 
> On 11/13/22 17:58, Xu Yilun wrote:
> > On 2022-11-11 at 13:49:38 +0200, Ilpo Järvinen wrote:
> >> On Fri, 11 Nov 2022, Xu Yilun wrote:
> >>
> >>> On 2022-11-08 at 16:42:55 +0200, 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.
> >>>>
> >>>> Reviewed-by: Russ Weight <russell.h.weight@intel.com>
> >>>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >>>> ---
> >>>>  drivers/mfd/intel-m10-bmc.c       | 50 +++++++++++++++++--------------
> >>>>  include/linux/mfd/intel-m10-bmc.h | 14 +++++++++
> >>>>  2 files changed, 41 insertions(+), 23 deletions(-)
> >>>>
> >>>> diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
> >>>> index ee167c5dcd29..762808906380 100644
> >>>> --- a/drivers/mfd/intel-m10-bmc.c
> >>>> +++ b/drivers/mfd/intel-m10-bmc.c
> >>>> @@ -156,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;
> >>> Where to use the ddata->info?
> >> In patch 5/12 there are many these constructs:
> >> const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
> >>
> >> Now that I look though, this particular line is altered by the split patch 
> >> 4/12 so it would be not strictly necessary to do it here. I'd prefer, 
> >> however, still to add it here even if it's technically not used until 
> >> after the split 5/12 patch because it very much logically belongs to this 
> >> change.
> > It's good to me.
> >
> >>>>  	ddata->dev = dev;
> >>>>  
> >>>>  	ddata->regmap =
> >>>> @@ -183,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);
> >>>> @@ -208,10 +194,28 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
> >>>>  	return ret;
> >>>>  }
> >>>>  
> >>>> +static const struct intel_m10bmc_platform_info m10bmc_m10_n3000 = {
> >>>> +	.type = M10_N3000,
> >>> Is the type enum still useful? Found no usage.
> >> There's no use within context of this patch series. However, I think there 
> >> might have been something depending on it in the changes that are not part 
> >> of this series so I left it in place for now.
> > I'm not sure how it would be used later. This patch is to eliminate the
> > "switch (board type) case" block, but similar code is still to be added
> > later?
> 
> Unfortunately, these will be needed later. Consider the following (future)
> function that has to account for a field that was moved from one register
> to another:
> 
>     static int
>     m10bmc_sec_status(struct m10bmc_sec *sec, u32 *status)
>     {
>             u32 reg_offset, reg_value;
>             int ret;
> 
>             reg_offset = (sec->type == N6000BMC_SEC) ?
>                     auth_result_reg(sec->m10bmc) : doorbell_reg(sec->m10bmc);
> 
>             ret = m10bmc_sys_read(sec->m10bmc, reg_offset, &reg_value);
>             if (ret)
>                     return ret;
> 
>             *status = rsu_stat(reg_value);
> 
>             return 0;
>     }
> 
> With this patch-set, most conditionals are removed, but there will still
> be some cases where it is needed. If you prefer, we could wait and add

Why this condition can't be handled in the same manner? I actually hope
all board type difference been handled in the same way, either by the
core mfd driver or each subdev driver, but not a mix of the two.

Thanks,
Yilun

> the type in when we are ready to use it.
> 
> - Russ
> 
> >
> > Thanks,
> > Yilun
>
Ilpo Järvinen Nov. 15, 2022, 8:56 a.m. UTC | #6 
On Tue, 15 Nov 2022, Xu Yilun wrote:

> On 2022-11-14 at 17:17:06 -0800, Russ Weight wrote:
> > 
> > 
> > On 11/13/22 17:58, Xu Yilun wrote:
> > > On 2022-11-11 at 13:49:38 +0200, Ilpo Järvinen wrote:
> > >> On Fri, 11 Nov 2022, Xu Yilun wrote:
> > >>
> > >>> On 2022-11-08 at 16:42:55 +0200, 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.
> > >>>>
> > >>>> Reviewed-by: Russ Weight <russell.h.weight@intel.com>
> > >>>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > >>>> ---
> > >>>>  drivers/mfd/intel-m10-bmc.c       | 50 +++++++++++++++++--------------
> > >>>>  include/linux/mfd/intel-m10-bmc.h | 14 +++++++++
> > >>>>  2 files changed, 41 insertions(+), 23 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
> > >>>> index ee167c5dcd29..762808906380 100644
> > >>>> --- a/drivers/mfd/intel-m10-bmc.c
> > >>>> +++ b/drivers/mfd/intel-m10-bmc.c
> > >>>> @@ -208,10 +194,28 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
> > >>>>  	return ret;
> > >>>>  }
> > >>>>  
> > >>>> +static const struct intel_m10bmc_platform_info m10bmc_m10_n3000 = {
> > >>>> +	.type = M10_N3000,
> > >>> Is the type enum still useful? Found no usage.
> > >> There's no use within context of this patch series. However, I think there 
> > >> might have been something depending on it in the changes that are not part 
> > >> of this series so I left it in place for now.
> > > I'm not sure how it would be used later. This patch is to eliminate the
> > > "switch (board type) case" block, but similar code is still to be added
> > > later?
> > 
> > Unfortunately, these will be needed later. Consider the following (future)
> > function that has to account for a field that was moved from one register
> > to another:
> > 
> >     static int
> >     m10bmc_sec_status(struct m10bmc_sec *sec, u32 *status)
> >     {
> >             u32 reg_offset, reg_value;
> >             int ret;
> > 
> >             reg_offset = (sec->type == N6000BMC_SEC) ?
> >                     auth_result_reg(sec->m10bmc) : doorbell_reg(sec->m10bmc);
> > 
> >             ret = m10bmc_sys_read(sec->m10bmc, reg_offset, &reg_value);
> >             if (ret)
> >                     return ret;
> > 
> >             *status = rsu_stat(reg_value);
> > 
> >             return 0;
> >     }
> > 
> > With this patch-set, most conditionals are removed, but there will still
> > be some cases where it is needed. If you prefer, we could wait and add
> 
> Why this condition can't be handled in the same manner? I actually hope
> all board type difference been handled in the same way, either by the
> core mfd driver or each subdev driver, but not a mix of the two.

I agree. It is already a TODO item on my list to make that oddity part of 
CSR map.

Also, this is anyway not the same type (mfd and sec have their own set of 
types). I'll just drop the mfd type field for now and might end up doing 
the same for the sec one too.
diff mbox series

Patch

diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
index ee167c5dcd29..762808906380 100644
--- a/drivers/mfd/intel-m10-bmc.c
+++ b/drivers/mfd/intel-m10-bmc.c
@@ -156,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 =
@@ -183,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);
@@ -208,10 +194,28 @@  static int intel_m10_bmc_spi_probe(struct spi_device *spi)
 	return ret;
 }
 
+static const struct intel_m10bmc_platform_info m10bmc_m10_n3000 = {
+	.type = M10_N3000,
+	.cells = m10bmc_pacn3000_subdevs,
+	.n_cells = ARRAY_SIZE(m10bmc_pacn3000_subdevs),
+};
+
+static const struct intel_m10bmc_platform_info m10bmc_m10_d5005 = {
+	.type = M10_D5005,
+	.cells = m10bmc_d5005_subdevs,
+	.n_cells = ARRAY_SIZE(m10bmc_d5005_subdevs),
+};
+
+static const struct intel_m10bmc_platform_info m10bmc_m10_n5010 = {
+	.type = M10_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_m10_n3000 },
+	{ "m10-d5005", (kernel_ulong_t)&m10bmc_m10_d5005 },
+	{ "m10-n5010", (kernel_ulong_t)&m10bmc_m10_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 d77383a8a410..e58115f514b9 100644
--- a/include/linux/mfd/intel-m10-bmc.h
+++ b/include/linux/mfd/intel-m10-bmc.h
@@ -124,14 +124,28 @@  enum m10bmc_type {
 /* 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
+ * @type: the type of MAX10 BMC
+ * @cells: MFD cells
+ * @n_cells: MFD cells ARRAY_SIZE()
+ */
+struct intel_m10bmc_platform_info {
+	enum m10bmc_type type;
+	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;
 };
 
 /*