On Thu, Oct 27, 2016 at 10:57:07AM +0200, Christian Gromm wrote:
> On Wed, 26 Oct 2016 17:22:50 +0300
> Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> > On Tue, Oct 25, 2016 at 05:44:20PM +0200, Christian Gromm wrote:
> > > From: Andrey Shvetsov <andrey.shvetsov@xxxxxx>
> > >
> > > This patch puts the synchronization procedure trigger for asynchronous
> > > channels into the function hdm_configure_channel. Likewise, it removes
> > > triggering of hardware specific synchronization for other channel types
> > > from the probe function as it is not required.
> > >
> > > Signed-off-by: Andrey Shvetsov <andrey.shvetsov@xxxxxx>
> > > Signed-off-by: Christian Gromm <christian.gromm@xxxxxxxxxxxxx>
> > > ---
> > > drivers/staging/most/hdm-usb/hdm_usb.c | 17 +++++++++--------
> > > 1 file changed, 9 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/staging/most/hdm-usb/hdm_usb.c b/drivers/staging/most/hdm-usb/hdm_usb.c
> > > index 1a630e1..db11930 100644
> > > --- a/drivers/staging/most/hdm-usb/hdm_usb.c
> > > +++ b/drivers/staging/most/hdm-usb/hdm_usb.c
> > > @@ -695,6 +695,15 @@ static int hdm_configure_channel(struct most_interface *iface, int channel,
> > > - conf->buffer_size;
> > > exit:
> > > mdev->conf[channel] = *conf;
> > > + if (conf->data_type == MOST_CH_ASYNC) {
> > > + u16 ep = mdev->ep_address[channel];
> > > + int err = drci_wr_reg(mdev->usb_device,
> > > + DRCI_REG_BASE + DRCI_COMMAND + ep * 16,
> > > + 1);
> > > +
> > > + if (err < 0)
> > > + dev_warn(dev, "sync for ep%02x failed", ep);
> > > + }
> > > return 0;
> >
> > This code is weird, because we goto exit without checking the
> > frame_size. It looks like it doesn't matter much but it's sort of
> > puzzling what's going on. There weren't any comments to explain it.
> >
>
> The frame size is only needed if we are dealing with synchronous and
> (in some cases) isochronous data. So you're right, the variable
> frame_size is _not_ needed in case we be jumping to the 'exit' label
> and hence, not being checked.
>
> Haven't had the feeling that this is worth a comment. It isn't easy
> to decide what needs a comment and what does not anyway. Then I would
> probably also have to explain why we jump to 'exit' if we have
> isochronous data and a packet_per_transaction value unequal to 0xff.
> (I don't expect anyone to understand what this is supposed mean, unless
> he is familiar with the network interface controller.)
>
> So, let me know if a comment on the frame_size usage can fix the
> confusion.
A comment would be nice, yes.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
