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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3318 bytes --]

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.


-- 
 i.