Re: [RFC PATCH v2 8/9] fpga: dfl: migrate dfl_get_feature_by_id() to dfl_feature_dev_data

["Colberg, Peter" <peter.colberg@intel.com>]

On Tue, 2024-04-23 at 23:16 +0800, Xu Yilun wrote:
> On Tue, Apr 09, 2024 at 07:39:41PM -0400, Peter Colberg wrote:
> > This change separates out most of the symbol name changes required by this
> > patch series for the function: dfl_get_feature_by_id(). This is done to
> > split a single monolithic change into multiple, smaller patches at the
> > request of the maintainer.
> > 
> > Signed-off-by: Peter Colberg <peter.colberg@intel.com>
> > ---
> > v2:
> > - Split monolithic patch into series at request of maintainer
> > ---
> >  drivers/fpga/dfl-afu-error.c |  59 +++++++------
> >  drivers/fpga/dfl-afu-main.c  | 166 ++++++++++++++++++-----------------
> >  drivers/fpga/dfl-afu.h       |  26 +++---
> >  drivers/fpga/dfl-fme-error.c |  98 +++++++++++----------
> >  drivers/fpga/dfl-fme-main.c  |  18 ++--
> >  drivers/fpga/dfl-fme-pr.c    |   6 +-
> >  drivers/fpga/dfl.c           |   3 +-
> >  drivers/fpga/dfl.h           |  13 ++-
> >  8 files changed, 203 insertions(+), 186 deletions(-)
> > 
> > diff --git a/drivers/fpga/dfl-afu-error.c b/drivers/fpga/dfl-afu-error.c
> > index ab7be6217368..0f392d1f6d45 100644
> > --- a/drivers/fpga/dfl-afu-error.c
> > +++ b/drivers/fpga/dfl-afu-error.c
> > @@ -28,37 +28,36 @@
> >  #define ERROR_MASK		GENMASK_ULL(63, 0)
> >  
> >  /* mask or unmask port errors by the error mask register. */
> > -static void __afu_port_err_mask(struct device *dev, bool mask)
> > +static void __afu_port_err_mask(struct dfl_feature_dev_data *fdata, bool mask)
> 
> Maybe first replace all "struct device *dev" arguments with "struct
> dfl_feature_platform data *pdata", then you could do simple
> pdata->fdata replacement the same as other patches.

Thank you for the suggestion; this has been done in the patch "fpga:
dfl: pass feature platform data instead of device as argument".
> 
> >  {
> >  	void __iomem *base;
> >  
> > -	base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
> > +	base = dfl_get_feature_ioaddr_by_id(fdata, PORT_FEATURE_ID_ERROR);
> >  
> >  	writeq(mask ? ERROR_MASK : 0, base + PORT_ERROR_MASK);
> >  }
> >  
> >  static void afu_port_err_mask(struct device *dev, bool mask)
> >  {
> > -	struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> > +	struct dfl_feature_dev_data *fdata = to_dfl_feature_dev_data(dev);
> >  
> > -	mutex_lock(&pdata->lock);
> > -	__afu_port_err_mask(dev, mask);
> > -	mutex_unlock(&pdata->lock);
> > +	mutex_lock(&fdata->lock);
> > +	__afu_port_err_mask(fdata, mask);
> > +	mutex_unlock(&fdata->lock);
> >  }
> >  
> >  /* clear port errors. */
> >  static int afu_port_err_clear(struct device *dev, u64 err)
> >  {
> > -	struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> > -	struct platform_device *pdev = to_platform_device(dev);
> > +	struct dfl_feature_dev_data *fdata = to_dfl_feature_dev_data(dev);
> >  	void __iomem *base_err, *base_hdr;
> >  	int enable_ret = 0, ret = -EBUSY;
> >  	u64 v;
> >  
> > -	base_err = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
> > -	base_hdr = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_HEADER);
> > +	base_err = dfl_get_feature_ioaddr_by_id(fdata, PORT_FEATURE_ID_ERROR);
> > +	base_hdr = dfl_get_feature_ioaddr_by_id(fdata, PORT_FEATURE_ID_HEADER);
> >  
> > -	mutex_lock(&pdata->lock);
> > +	mutex_lock(&fdata->lock);
> >  
> >  	/*
> >  	 * clear Port Errors
> > @@ -80,12 +79,12 @@ static int afu_port_err_clear(struct device *dev, u64 err)
> >  	}
> >  
> >  	/* Halt Port by keeping Port in reset */
> > -	ret = __afu_port_disable(pdev);
> > +	ret = __afu_port_disable(fdata);
> >  	if (ret)
> >  		goto done;
> >  
> >  	/* Mask all errors */
> > -	__afu_port_err_mask(dev, true);
> > +	__afu_port_err_mask(fdata, true);
> >  
> >  	/* Clear errors if err input matches with current port errors.*/
> >  	v = readq(base_err + PORT_ERROR);
> > @@ -102,28 +101,28 @@ static int afu_port_err_clear(struct device *dev, u64 err)
> >  	}
> >  
> >  	/* Clear mask */
> > -	__afu_port_err_mask(dev, false);
> > +	__afu_port_err_mask(fdata, false);
> >  
> >  	/* Enable the Port by clearing the reset */
> > -	enable_ret = __afu_port_enable(pdev);
> > +	enable_ret = __afu_port_enable(fdata);
> >  
> >  done:
> > -	mutex_unlock(&pdata->lock);
> > +	mutex_unlock(&fdata->lock);
> >  	return enable_ret ? enable_ret : ret;
> >  }
> >  
> >  static ssize_t errors_show(struct device *dev, struct device_attribute *attr,
> >  			   char *buf)
> >  {
> > -	struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> > +	struct dfl_feature_dev_data *fdata = to_dfl_feature_dev_data(dev);
> >  	void __iomem *base;
> >  	u64 error;
> >  
> > -	base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
> > +	base = dfl_get_feature_ioaddr_by_id(fdata, PORT_FEATURE_ID_ERROR);
> >  
> > -	mutex_lock(&pdata->lock);
> > +	mutex_lock(&fdata->lock);
> >  	error = readq(base + PORT_ERROR);
> > -	mutex_unlock(&pdata->lock);
> > +	mutex_unlock(&fdata->lock);
> >  
> >  	return sprintf(buf, "0x%llx\n", (unsigned long long)error);
> >  }
> > @@ -146,15 +145,15 @@ static DEVICE_ATTR_RW(errors);
> >  static ssize_t first_error_show(struct device *dev,
> >  				struct device_attribute *attr, char *buf)
> >  {
> > -	struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> > +	struct dfl_feature_dev_data *fdata = to_dfl_feature_dev_data(dev);
> >  	void __iomem *base;
> >  	u64 error;
> >  
> > -	base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
> > +	base = dfl_get_feature_ioaddr_by_id(fdata, PORT_FEATURE_ID_ERROR);
> >  
> > -	mutex_lock(&pdata->lock);
> > +	mutex_lock(&fdata->lock);
> >  	error = readq(base + PORT_FIRST_ERROR);
> > -	mutex_unlock(&pdata->lock);
> > +	mutex_unlock(&fdata->lock);
> >  
> >  	return sprintf(buf, "0x%llx\n", (unsigned long long)error);
> >  }
> > @@ -164,16 +163,16 @@ static ssize_t first_malformed_req_show(struct device *dev,
> >  					struct device_attribute *attr,
> >  					char *buf)
> >  {
> > -	struct dfl_feature_platform_data *pdata = dev_get_platdata(dev);
> > +	struct dfl_feature_dev_data *fdata = to_dfl_feature_dev_data(dev);
> >  	void __iomem *base;
> >  	u64 req0, req1;
> >  
> > -	base = dfl_get_feature_ioaddr_by_id(dev, PORT_FEATURE_ID_ERROR);
> > +	base = dfl_get_feature_ioaddr_by_id(fdata, PORT_FEATURE_ID_ERROR);
> >  
> > -	mutex_lock(&pdata->lock);
> > +	mutex_lock(&fdata->lock);
> >  	req0 = readq(base + PORT_MALFORMED_REQ0);
> >  	req1 = readq(base + PORT_MALFORMED_REQ1);
> > -	mutex_unlock(&pdata->lock);
> > +	mutex_unlock(&fdata->lock);
> >  
> >  	return sprintf(buf, "0x%016llx%016llx\n",
> >  		       (unsigned long long)req1, (unsigned long long)req0);
> > @@ -191,12 +190,14 @@ static umode_t port_err_attrs_visible(struct kobject *kobj,
> >  				      struct attribute *attr, int n)
> >  {
> >  	struct device *dev = kobj_to_dev(kobj);
> > +	struct dfl_feature_dev_data *fdata;
> >  
> > +	fdata = to_dfl_feature_dev_data(dev);
> >  	/*
> >  	 * sysfs entries are visible only if related private feature is
> >  	 * enumerated.
> >  	 */
> > -	if (!dfl_get_feature_by_id(dev, PORT_FEATURE_ID_ERROR))
> > +	if (!dfl_get_feature_by_id(fdata, PORT_FEATURE_ID_ERROR))
> >  		return 0;
> >  
> >  	return attr->mode;
> > diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
> > index 61868cdd5b0b..42928cc7e42b 100644
> > --- a/drivers/fpga/dfl-afu-main.c
> > +++ b/drivers/fpga/dfl-afu-main.c
> > @@ -26,7 +26,7 @@
> >  
> >  /**
> >   * __afu_port_enable - enable a port by clear reset
> > - * @pdev: port platform device.
> > + * @fdata: port feature dev data.
> >   *
> >   * Enable Port by clear the port soft reset bit, which is set by default.
> >   * The AFU is unable to respond to any MMIO access while in reset.
> > @@ -35,18 +35,17 @@
> >   *
> >   * The caller needs to hold lock for protection.
> >   */
> > -int __afu_port_enable(struct platform_device *pdev)
> > +int __afu_port_enable(struct dfl_feature_dev_data *fdata)
> 
> Same suggestion. Replace "struct platform_device *pdev" with "struct
> dfl_feature_platform_data *pdata" first. Then do massive pdata->fdata
> replacement.

This has been done in the patch "fpga: dfl: pass feature platform data
instead of device as argument".

Thanks,
Peter

> 
> Thanks,
> Yilun