linux-kernel - Re: [PATCH] Staging: silabs si4455 serial driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20201210080006.GK2767@kadam>
Date:   Thu, 10 Dec 2020 11:00:06 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     József Horváth <info@...istro.hu>
Cc:     Jérôme Pouiller <jerome.pouiller@...abs.com>,
        devel@...verdev.osuosl.org, devicetree@...r.kernel.org,
        "'Greg Kroah-Hartman'" <gregkh@...uxfoundation.org>,
        driverdev-devel@...uxdriverproject.org,
        linux-kernel@...r.kernel.org, "'Rob Herring'" <robh+dt@...nel.org>
Subject: Re: [PATCH] Staging: silabs si4455 serial driver

On Wed, Dec 09, 2020 at 07:41:53PM +0000, József Horváth wrote:
> > > +static int si4455_get_part_info(struct uart_port *port,
> > > +                               struct si4455_part_info *result)
> > > +{
> > > +       int ret;
> > > +       u8 dataOut[] = { SI4455_CMD_ID_PART_INFO };
> > > +       u8 dataIn[SI4455_CMD_REPLY_COUNT_PART_INFO];
> > > +
> > > +       ret = si4455_send_command_get_response(port,
> > > +                                               sizeof(dataOut),
> > > +                                               dataOut,
> > > +                                               sizeof(dataIn),
> > > +                                               dataIn);
> > 
> > Why not:
> > 
> 
> I changed all like this in my code already. I test it, and I'll send it again.
> 
> Ps.: For my eyes is better to read line or list, reading table is harder :)
> 
> 	line(arg1, arg2, arg3, arg4);
> 
> 	list(arg1,
> 		arg2,
> 		arg3,
> 		arg4);
> 
> 	table(arg1, arg2,
> 		arg3, arg4);
> 

Use spaces to make arguments have to line up properly.
`checkpatch.pl --strict` will complain if it's not done.

	table(arg1, arg2,
	      arg_whatver, foo);
[tab][space x 7]arg_whaver, foo);

But I think Jérôme's main point was to get rid of the dataOut buffer and
use "result" directly.

> 
> >        ret = si4455_send_command_get_response(port,
> >                                               sizeof(*result), result,
> >                                               sizeof(dataIn), dataIn);
> > 
> > > +       if (ret == 0) {
> > > +               result->CHIPREV = dataIn[0];
> > > +               memcpy(&result->PART, &dataIn[1],sizeof(result->PART));
> > > +               result->PBUILD = dataIn[3];
> > > +               memcpy(&result->ID, &dataIn[4], sizeof(result->ID));
> > > +               result->CUSTOMER = dataIn[6];
> > > +               result->ROMID = dataIn[7];
> > > +               result->BOND = dataIn[8];
> > 
> > ... it would avoid all these lines.
> > 

regards,
dan carpenter