Hi Pavel,
Forgot to add this in last reply: Francisco Iglesias(in cc) was also involved a
lot in net/can QEMU devices and willing to help in the review if needed.
Regards
Vikram
> -----Original Message-----
> From: Vikram Garhwal <fnu.vik...@xilinx.com>
> Sent: Wednesday, September 2, 2020 10:20 PM
> To: Pavel Pisa <p...@cmp.felk.cvut.cz>
> Cc: qemu-devel@nongnu.org; Paolo Bonzini <pbonz...@redhat.com>;
> Stefan Hajnoczi <stefa...@gmail.com>; Konrad Frederic
> <frederic.kon...@adacore.com>; Deniz Eren <deniz.e...@icloud.com>;
> Oliver Hartkopp <socket...@hartkopp.net>; Marek Vasut
> <ma...@denx.de>; Jan Kiszka <jan.kis...@siemens.com>; Oleksij Rempel
> <o.rem...@pengutronix.de>; Markus Armbruster <arm...@redhat.com>;
> Ondrej Ille <ondrej.i...@gmail.com>; Jan Charvat <charv...@fel.cvut.cz>;
> Jiri Novak <jno...@fel.cvut.cz>
> Subject: Re: [PATCH v1 1/6] net/can: Initial host SocketCan support for CAN
> FD.
>
> 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, p...@cmp.felk.cvut.cz
> wrote:
> > > > From: Jan Charvat <charv...@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
> >
> >
