Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 14 Nov 2022 09:58:51 +0800
From:   Xu Yilun <yilun.xu@intel.com>
To:     Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
Cc:     linux-fpga@vger.kernel.org, Wu Hao <hao.wu@intel.com>,
        Tom Rix <trix@redhat.com>, Moritz Fischer <mdf@kernel.org>,
        Lee Jones <lee@kernel.org>,
        Matthew Gerlach <matthew.gerlach@linux.intel.com>,
        Russ Weight <russell.h.weight@intel.com>,
        Tianfei zhang <tianfei.zhang@intel.com>,
        Mark Brown <broonie@kernel.org>,
        Greg KH <gregkh@linuxfoundation.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 02/12] mfd: intel-m10-bmc: Create m10bmc_platform_info
 for type specific info
Message-ID: <Y3Gg29pGm4DwjOgI@yilunxu-OptiPlex-7050>
References: <20221108144305.45424-1-ilpo.jarvinen@linux.intel.com>
 <20221108144305.45424-3-ilpo.jarvinen@linux.intel.com>
 <Y24gJ7fIsUPmhzY2@yilunxu-OptiPlex-7050>
 <752a1dc-fae6-4431-41cf-a6deaf157ad3@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <752a1dc-fae6-4431-41cf-a6deaf157ad3@linux.intel.com>
Precedence: bulk

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