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

[Russ Weight]

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@xxxxxxxxx>
>>>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
>>>> ---
>>>>  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