Re: [PATCH v1 1/6] net/can: Initial host SocketCan support for CAN FD.

[Vikram Garhwal <fnu.vikram@xilinx.com>]

On Wed, Sep 02, 2020 at 09:51:44AM +0200, Pavel Pisa wrote:
Hi Pavel,
> Hello Vikram,
>
> thanks much for the patches review.
>
> On Tuesday 01 of September 2020 22:01:26 Vikram Garhwal wrote:
> > Hi Jan,
> > A couple of comments on this patch.
> >
> > On Tue, Jul 14, 2020 at 02:20:14PM +0200, pisa@cmp.felk.cvut.cz wrote:
> > > From: Jan Charvat <charvj10@fel.cvut.cz>
> > > @@ -185,13 +204,34 @@ static void can_host_socketcan_connect(CanHostState
> > > *ch, Error **errp) addr.can_family = AF_CAN;
> > >      memset(&ifr.ifr_name, 0, sizeof(ifr.ifr_name));
> > >      strcpy(ifr.ifr_name, c->ifname);
> > > +    /* check if the frame fits into the CAN netdevice */
> > >      if (ioctl(s, SIOCGIFINDEX, &ifr) < 0) {
> > >          error_setg_errno(errp, errno,
> > > -                         "SocketCAN host interface %s not available",
> > > c->ifname); +                         "SocketCAN host interface %s not
> > > available", +                         c->ifname);
> >
> > May be this formatting change in a different patch? As this is not related
> > to CANFD.
> > > @@ -232,7 +272,8 @@ static char *can_host_socketcan_get_if(Object *obj,
> > > Error **errp) return g_strdup(c->ifname);
> > >  }
> > >
> > > -static void can_host_socketcan_set_if(Object *obj, const char *value,
> > > Error **errp) +static void can_host_socketcan_set_if(Object *obj, const
> > > char *value,
> > > +                                      Error **errp)
> >
> > This one also not relevant change for CANFD. Rest of the patch looks good.
>
>
> I am responsible for mentioned lines change in net/can/can_socketcan.c.
> When I have reviewed patches after Jan Charvat theses submittion,
> I have done another bunch of rounds to check that the patches as well
> as the whole net/can and hw/net/can are checkpatch clean. I am not sure
> if the incorrect formatting sneaked in in my 2018 submission or patcheck
> became more strict last years.
>
> I can separate these changes changes into separate patch if required.
May be we can keep them in same patch. I was just referring to "Don't include irrelevant changes" section on this page about patches: https://wiki.qemu.org/Contribute/SubmitAPatch.
>
> By the way, if you or other of your colleagues is willing to participate
> in net/can and or  hw/net/can patches reviews, I would be happy if you
> join my attempt and we can record that we are available to take care
> abut these in MAINTAINERS file.
Given that I spent good amount of time working with net/can, I am willing to review the patches. Thanks!
>
> Best wishes,
>
> Pavel
>
>