14/04/2021 04:59, Wang, Haiyue:
> From: Xueming Li
[...]
> > +void
> > +auxiliary_on_scan(struct rte_auxiliary_device *dev)
> > +{
> > + struct rte_devargs *devargs;
> > +
> > + devargs = auxiliary_devargs_lookup(dev->name);
> > + dev->device.devargs = devargs;
>
> Can be simple as:
>
> dev->device.devargs = auxiliary_devargs_lookup(dev->name);
>
> > +}
> > +
> > +/*
> > + * Match the auxiliary Driver and Device using driver function.
> > + */
> > +bool
> > +auxiliary_match(const struct rte_auxiliary_driver *auxiliary_drv,
> > + const struct rte_auxiliary_device *auxiliary_dev)
>
> How about these auxiliary variable name style ?
>
> const struct rte_auxiliary_driver *aux_drv,
> const struct rte_auxiliary_device *aux_dev
+1
[...]
> > +static int
> > +rte_auxiliary_probe_one_driver(struct rte_auxiliary_driver *dr,
> > + struct rte_auxiliary_device *dev)
> > +{
> > + int ret;
> > + enum rte_iova_mode iova_mode;
> > +
>
> RCT style ?
> enum rte_iova_mode iova_mode;
> int ret;
I don't see the benefit of reverse christmas tree.
> > + if ((dr->drv_flags & RTE_AUXILIARY_DRV_NEED_IOVA_AS_VA) > 0 &&
>
> '(dr->drv_flags & RTE_AUXILIARY_DRV_NEED_IOVA_AS_VA)' should work, no need '>
> 0'
Yes it's acceptable to consider bit testing as a boolean.
[...]
> > +static int
> > +auxiliary_dma_map(struct rte_device *dev, void *addr, uint64_t iova,
> > size_t len)
> > +{
> > + struct rte_auxiliary_device *adev = RTE_DEV_TO_AUXILIARY(dev);
>
> How about to use 'aux_dev', instead of 'adev' ?
>
> > +
> > + if (!adev || !adev->driver) {
>
> ' RTE_DEV_TO_AUXILIARY' is container of 'dev', so it should check 'dev !=
> NULL',
> not '!adev'. ; -)
Yes and should be explicit NULL comparison.
[...]
> > --- /dev/null
> > +++ b/drivers/bus/auxiliary/linux/auxiliary.c
> ^
> |
> Seems no need to add one more directory 'linux' layer, as the meson said
> "linux only".
I disagree.
Linux sub-directory is more explicit.
And who knows? There could be an implementation on other OSes in future.
