Re: [PATCH v5 09/28] fpga: dfl: add feature device infrastructure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jun 05, 2018 at 04:14:43PM -0500, Alan Tull wrote:
> On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@xxxxxxxxx> wrote:
> 
> Hi Hao,
> 
> Some minor things, otherwise looks fine.
> 
> > From: Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
> >
> > This patch abstracts the common operations of the sub features, and defines
> > the feature_ops data structure, including init, uinit and ioctl function
> > pointers. And this patch adds some common helper functions for FME and AFU
> > drivers, e.g dfl_feature_dev_use_begin/end which are used to ensure
> > exclusive usage of the feature device file.
> >
> > Signed-off-by: Tim Whisonant <tim.whisonant@xxxxxxxxx>
> > Signed-off-by: Enno Luebbers <enno.luebbers@xxxxxxxxx>
> > Signed-off-by: Shiva Rao <shiva.rao@xxxxxxxxx>
> > Signed-off-by: Christopher Rauer <christopher.rauer@xxxxxxxxx>
> > Signed-off-by: Kang Luwei <luwei.kang@xxxxxxxxx>
> > Signed-off-by: Zhang Yi <yi.z.zhang@xxxxxxxxx>
> > Signed-off-by: Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
> > Signed-off-by: Wu Hao <hao.wu@xxxxxxxxx>
> Acked-by: Alan Tull <atull@xxxxxxxxxx>
> 
> > ---
> > v2: rebased
> > v3: use const for feature_ops.
> >     replace pci related function.
> > v4: rebase and add more comments in code.
> > v5: remove useless WARN_ON().
> >     reorder declarations in functions per suggestion from Moritz.
> >     add "dfl_" prefix to functions and data structure.
> > ---
> >  drivers/fpga/dfl.c | 57 +++++++++++++++++++++++++++++++++++++
> >  drivers/fpga/dfl.h | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 138 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index 1e06efb..c4c47d6 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -74,6 +74,63 @@ static enum dfl_id_type feature_dev_id_type(struct platform_device *pdev)
> >         return DFL_ID_MAX;
> >  }
> >
> > +void dfl_fpga_dev_feature_uinit(struct platform_device *pdev)
> > +{
> > +       struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > +       struct dfl_feature *feature;
> > +
> > +       dfl_fpga_dev_for_each_feature(pdata, feature)
> > +               if (feature->ops) {
> > +                       feature->ops->uinit(pdev, feature);
> > +                       feature->ops = NULL;
> > +               }
> > +}
> > +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit);
> 
> Please add kernel-doc for functions that are exported to recommend and
> guide their usage.

Sorry, I missed uinit and init functions. will add them in v6.

> 
> > +
> > +static int dfl_feature_instance_init(struct platform_device *pdev,
> > +                                    struct dfl_feature_platform_data *pdata,
> > +                                    struct dfl_feature *feature,
> > +                                    struct dfl_feature_driver *drv)
> > +{
> > +       int ret;
> > +
> > +       ret = drv->ops->init(pdev, feature);
> > +       if (ret)
> > +               return ret;
> > +
> > +       feature->ops = drv->ops;
> > +
> > +       return ret;
> > +}
> > +
> > +int dfl_fpga_dev_feature_init(struct platform_device *pdev,
> > +                             struct dfl_feature_driver *feature_drvs)
> > +{
> > +       struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > +       struct dfl_feature_driver *drv = feature_drvs;
> > +       struct dfl_feature *feature;
> > +       int ret;
> > +
> > +       while (drv->ops) {
> > +               dfl_fpga_dev_for_each_feature(pdata, feature) {
> > +                       /* match feature and drv using id */
> > +                       if (feature->id == drv->id) {
> > +                               ret = dfl_feature_instance_init(pdev, pdata,
> > +                                                               feature, drv);
> > +                               if (ret)
> > +                                       goto exit;
> > +                       }
> > +               }
> > +               drv++;
> > +       }
> > +
> > +       return 0;
> > +exit:
> > +       dfl_fpga_dev_feature_uinit(pdev);
> > +       return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init);
> > +
> >  struct dfl_chardev_info {
> >         const char *name;
> >         dev_t devt;
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > index 2b6aaef..27f7a74 100644
> > --- a/drivers/fpga/dfl.h
> > +++ b/drivers/fpga/dfl.h
> > @@ -132,6 +132,17 @@
> >  #define PORT_CTRL_SFTRST_ACK   BIT_ULL(4)              /* HW ack for reset */
> >
> >  /**
> > + * struct dfl_feature_driver - sub feature's driver
> > + *
> > + * @id: sub feature id.
> > + * @ops: ops of this sub feature.
> > + */
> > +struct dfl_feature_driver {
> > +       u64 id;
> > +       const struct dfl_feature_ops *ops;
> > +};
> > +
> > +/**
> >   * struct dfl_feature - sub feature of the feature devices
> >   *
> >   * @id: sub feature id.
> > @@ -139,13 +150,17 @@
> >   *                 this index is used to find its mmio resource from the
> >   *                 feature dev (platform device)'s reources.
> >   * @ioaddr: mapped mmio resource address.
> > + * @ops: ops of this sub feature.
> >   */
> >  struct dfl_feature {
> >         u64 id;
> >         int resource_index;
> >         void __iomem *ioaddr;
> > +       const struct dfl_feature_ops *ops;
> >  };
> >
> > +#define DEV_STATUS_IN_USE      0
> > +
> >  /**
> >   * struct dfl_feature_platform_data - platform data for feature devices
> >   *
> > @@ -156,6 +171,8 @@ struct dfl_feature {
> >   * @dfl_cdev: ptr to container device.
> >   * @disable_count: count for port disable.
> >   * @num: number for sub features.
> > + * @dev_status: dev status (e.g DEV_STATUS_IN_USE).
> > + * @private: ptr to feature dev private data.
> >   * @features: sub features of this feature dev.
> >   */
> >  struct dfl_feature_platform_data {
> > @@ -165,11 +182,49 @@ struct dfl_feature_platform_data {
> >         struct platform_device *dev;
> >         struct dfl_fpga_cdev *dfl_cdev;
> >         unsigned int disable_count;
> > -
> > +       unsigned long dev_status;
> > +       void *private;
> >         int num;
> >         struct dfl_feature features[0];
> >  };
> >
> > +static inline
> > +int dfl_feature_dev_use_begin(struct dfl_feature_platform_data *pdata)
> > +{
> > +       /* Test and set IN_USE flags to ensure file is exclusively used */
> > +       if (test_and_set_bit_lock(DEV_STATUS_IN_USE, &pdata->dev_status))
> > +               return -EBUSY;
> > +
> > +       return 0;
> > +}
> > +
> > +static inline
> > +void dfl_feature_dev_use_end(struct dfl_feature_platform_data *pdata)
> > +{
> > +       clear_bit_unlock(DEV_STATUS_IN_USE, &pdata->dev_status);
> > +}
> > +
> > +static inline
> > +void dfl_fpga_pdata_set_private(struct dfl_feature_platform_data *pdata,
> > +                               void *private)
> > +{
> > +       pdata->private = private;
> > +}
> > +
> > +static inline
> > +void *dfl_fpga_pdata_get_private(struct dfl_feature_platform_data *pdata)
> > +{
> > +       return pdata->private;
> > +}
> > +
> > +struct dfl_feature_ops {
> > +       int (*init)(struct platform_device *pdev, struct dfl_feature *feature);
> > +       void (*uinit)(struct platform_device *pdev,
> > +                     struct dfl_feature *feature);
> > +       long (*ioctl)(struct platform_device *pdev, struct dfl_feature *feature,
> > +                     unsigned int cmd, unsigned long arg);
> > +};
> > +
> >  #define DFL_FPGA_FEATURE_DEV_FME               "dfl-fme"
> >  #define DFL_FPGA_FEATURE_DEV_PORT              "dfl-port"
> 
> Please move these to the same place as other things that will need to
> be added to as feature devices are added as noted in the other reviews
> today.

as these two strings are used as platform device name, so I think we need to
keep them in the dfl.h file, because platform driver could reuse the same.
But I will add detailed comments to guide others to put name string for new
feature device (platform device) in the dfl.h file together with above ones.

Thanks a lot for the review.
Hao
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux