Re: [PATCH 2/3] staging: most: hdm-usb: do h/w specific synchronization at configuration time

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Oct 27, 2016 at 01:00:47PM +0200, Christian Gromm wrote:
> On Thu, 27 Oct 2016 12:00:28 +0300
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > 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@oracle.com> wrote:
> > > 
> > > > On Tue, Oct 25, 2016 at 05:44:20PM +0200, Christian Gromm wrote:
> > > > > From: Andrey Shvetsov <andrey.shvetsov@k2l.de>
> > > > > 
> > > > > 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@k2l.de>
> > > > > Signed-off-by: Christian Gromm <christian.gromm@microchip.com>
> > > > > ---
> > > > >  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.
> 
> Fine. It would be cool if this patch set will be accepted and I'll be
> sending in a new patch that adds the desired comment.
> 
> Greg, does this make sense to you?

Yes, that's fine.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel