Re: [dpdk-dev] [PATCH v2 4/5] doc: add documentation for szedata2 PMD

[Matej Vido <matejvido@gmail.com>]

Hi John,

Thank you for your comments and advice. I will follow them in the new
version.

Regards,
Matej Vido


2015-10-30 13:16 GMT+01:00 Mcnamara, John <john.mcnamara@intel.com>:

> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Matej Vido
> > Sent: Friday, September 18, 2015 9:33 AM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH v2 4/5] doc: add documentation for szedata2
> PMD
> >
> > Signed-off-by: Matej Vido <matejvido@gmail.com>
> > Reviewed-by: Jan Viktorin <viktorin@rehivetech.com>
>
> Hi,
>
> Thanks for the new PMD and the documentation.
>
> The DPDJ documentation guidelines provide some guidance on how to format
> and build the DPDK documentation:
>
>     http://dpdk.org/doc/guides/contributing/documentation.html
>
> In terms of documentation for a PMD the Mellanox MLX4 docs are a good
> example:
>
>     http://dpdk.org/doc/guides/nics/mlx4.html
>
> Some other comments below:
>
>
>
>
> > +SZEDATA2 PMD
> > +============
>
> No need for an acronym in the title. Follow the MLX4 example and use
> something like " SZEDATA2 poll mode driver library".
>
>
>
> > +SZEDATA2 PMD is virtual PMD which uses sze2 layer to communicate with
> > +COMBO cards (COMBO-80G, COMBO-100G) using interface provided by libsze2
> > library.
>
> The introduction can be brief but should include enough information to
> explain the PMD for someone encountering it for the first time.
>
> Some links would help with this. See the MLX4 doc as an example:
>
>
> > +
> > +.. note::
> > +
> > +   This driver has external dependencies. Therefore it is disabled in
> > default
> > +   configuration files. It can be enabled by setting
> > CONFIG_RTE_LIBRTE_PMD_SZEDATA2=y
>
> Format configuration items as verbatim text with  double backquotes:``
> CONFIG_RTE_LIBRTE_PMD_SZEDATA2=y ``
>
>
> > +
> > +Using PMD
> > +---------
>
> Better: Using the Szedata PMD
>
>
>
> > +
> > +SZEDATA2 PMD can be created by passing --vdev= option to EAL in the
>
> Again format --vdev as ``--vdev``. Also apply to other similar cases.
>
>
>
> > +following format:
> > +
> > +.. code-block:: console
> > +
> > +    --vdev
> >
> 'DEVICE_NAME,dev_path=PATH_TO_SZEDATA2_DEVICE,rx_ifaces=RX_MASK,tx_ifaces=
> > TX_MASK'
>
> This code line exceeds 80 chars and will exceed the page width in the PDF
> docs. If possible see if it can be shortened. Maybe use DEVICE instead of
> DEVICE_NAME and PATH instead of PATH_TO. The next section has an
> explanation anyway.
>
>
> > +
> > +DEVICE_NAME and options dev_path, rx_ifaces, tx_ifaces are mandatory
> > +and must be separated by commas.
>
> Use the ```` quotes again.
>
>
> > +
> > +*   DEVICE_NAME: contains prefix eth_szedata2 followed by numbers or
> > letters,
> > +    must be unique for each virtual device
> > +
> > +*   dev_path: Defines path to szedata2 device.
> > +    Value is valid path to szedata2 device.
> > +
> > +        dev_path=/dev/szedataII0
>
> Code blocks should be prefixed by :: to render them correctly. See the
> guidelines and also view the output from "make doc-guides-html".
>
> Same for the other items below.
>
>
> > +
> > +Example of usage
> > +^^^^^^^^^^^^^^^^
>
> The convention is to use ~~~~~ for third level headings (see the
> guidelines). However, this could probably be a second level heading.
>
>
> > +
> > +Read packets from 0. and 1. receive channel and write them to 0. and 1.
> > +transmit channel
> > +
> > +.. code-block:: console
> > +
> > +    $RTE_TARGET/app/testpmd -c 0xf -n 2 --vdev
> > + 'eth_szedata20,dev_path=/dev/szedataII0,rx_ifaces=0x3,tx_ifaces=0x3'
> > + -- --port-topology=chained --rxq=2 --txq=2 --nb-cores=2
>
>
> Again this code line is >80 chars and won't look good in the PDF. I'd
> suggest the following:
>
> testpmd -c 0xf -n 2 \
>  --vdev 'eth_szedata20,dev_path=/dev/szedataII0,rx_ifaces=3,tx_ifaces=3' \
>  -- --port-topology=chained --rxq=2 --txq=2 --nb-cores=2
>
>
>
> > diff --git a/doc/guides/prog_guide/source_org.rst
> > b/doc/guides/prog_guide/source_org.rst
> > index ae11b3b..2393002 100644
> > --- a/doc/guides/prog_guide/source_org.rst
> > +++ b/doc/guides/prog_guide/source_org.rst
> > @@ -106,6 +106,7 @@ The drivers directory has a *net* subdirectory which
> > contains::
> >      +-- null               # NULL poll mode driver for testing
> >      +-- pcap               # PCAP poll mode driver
> >      +-- ring               # Ring poll mode driver
> > +    +-- szedata2           # szedata2 poll mode driver
> >      +-- virtio             # Virtio poll mode driver
> >      +-- vmxnet3            # VMXNET3 poll mode driver
> >      +-- xenvirt            # Xen virtio poll mode driver
>
> It isn't necessary to update this section. It is meant to be
> representative rather than comprehensive (at least to me). However if you
> leave it in then captilize the name like the others.
>
>
> Thanks,
>
> John
>