On Mon, Sep 08, 2014 at 06:57:29PM +0300, Octavian Purdila wrote: > On Mon, Sep 8, 2014 at 5:44 PM, Johan Hovold <johan@xxxxxxxxxx> wrote: > > <snip> > > Hi Johan, > > Again, thanks for the detailed review, I am addressing your review > comments as we speak. Some questions below. > > <snip> > > > > + int ret, len; > > > + struct tx_data { > > > + u8 port; > > > + u8 addr; > > > + u8 mem_addr_len; > > > + __le32 mem_addr; > > > + __le16 buf_len; > > > + u8 buf[DLN2_I2C_MAX_XFER_SIZE]; > > > + } __packed tx; > > > > Allocate these buffers dynamically (possibly at probe). > > > > I double checked this, and DLN2_I2C_MAX_XFER_SIZE should actually be < > 64 as the USB endpoint configuration max packet size is 64. In this > case, can we keep it on the stack? It's better to lift that restriction and allocate it dynamically. Using larger buffers (> EP size) is also more efficient. > <snip> > > > > + int ret, buf_len, rx_len = sizeof(rx); > > > > Again, one declaration per line. > > AFAICS there are many places where declaration on the same line > (initialization included) are used. When did this became a coding > style issue? It's ugly, hurts readability, and can also obfuscate the fact that your function really needs to be refactored. And it's in the CodingStyle: "To this end, use just one data declaration per line (no commas for multiple data declarations)." Johan -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html