RE: [PATCH v3 3/4] fpga: dfl: create a dfl bus type to support DFL devices

[Wu, Hao]

> > > +static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env
> *env)
> > > +{
> > > +struct dfl_device *ddev = to_dfl_dev(dev);
> > > +
> > > +return add_uevent_var(env, "MODALIAS=dfl:t%08Xf%04X",
> > > +      ddev->type, ddev->feature_id);
> >
> > Then we only print 12bit of feature_id will be enough?
> > should we make type shorter as well as feature id?
> 
> I could envision that we need a struct
> 
>  struct dfl_feature_id {
> 	u16 id: 12;
>  }
> 
> for it.
> 
> But it seems more complex and I didn't see the benifit. We don't have to
> worry about the invalid values cause we parse all the ddev->feature_id in
> dfl driver, and ensures it will not be larger than 12bit value.

Ideally type is only 4bits per spec, but looks like it's adding more zero before
the type value, and also the feature id. This may not be a real problem, but
may look a little wired, isn't it?

> 
> > And do you think if we should add a new field for dfl version?
> 
> I think it may not be necessary now. If we support dfl v1 in future, we
> still could try to check uuid first, then fall back to type &
> feature_id.

I think it's all about the uevent and it's user, and for users, they may have to 
deal with different versions, right? As you mentioned, if the event will be
different format for v1 and it's not compatible with v0, anyway we need
the version sooner or later, is my understanding correct?

> 
> Do you have any idea for the potential usage of dfl version.
> 

> > > +/* then add irq resource */
> > > +if (feature->nr_irqs) {
> > > +ddev->irqs = kcalloc(feature->nr_irqs,
> > > +     sizeof(*ddev->irqs), GFP_KERNEL);
> > > +if (!ddev->irqs) {
> > > +ret = -ENOMEM;
> > > +goto put_dev;
> > > +}
> > > +
> > > +for (i = 0; i < feature->nr_irqs; i++)
> > > +ddev->irqs[i] = feature->irq_ctx[i].irq;
> > > +
> > > +ddev->num_irqs = feature->nr_irqs;
> >
> > Do we need to consider using IORESOURCE_IRQ here as well?
> 
> The helper functions for IORESOURCE_IRQ are all for platform_devices,
> We need to define a set of functions similar to them, I think current
> implementation is simpler, for dfl bus and drivers.

If some case that it's going to reuse some platform device driver,
then dfl_device driver will create platform device, it has to pass the mmio
and irq resources to platform device driver, how you plan to do that?

> > > +static int dfl_devs_init(struct platform_device *pdev)
> > > +{
> > > +struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev-
> > > >dev);
> > > +struct dfl_feature *feature;
> > > +struct dfl_device *ddev;
> > > +
> > > +dfl_fpga_dev_for_each_feature(pdata, feature) {
> > > +if (feature->ioaddr || feature->priv)
> >
> > Why checks priv here?
> 
> I want to make sure the dfl_device is not already created.

Is that a valid case that we expected? or sounds like error code needs 
to be returned?

Thanks
Hao