Re: [PATCH] Staging: silabs si4455 serial driver

[Dan Carpenter]

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