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] [thread-next>] [day] [month] [year] [list]
Date:	Sun, 17 Feb 2013 15:17:57 +0100
From:	Andi Shyti <andi@...zian.org>
To:	Ruslan Bilovol <ruslan.bilovol@...com>
Cc:	Andi Shyti <andi@...zian.org>, tomi.valkeinen@...com,
	FlorianSchandinat@....de, linux-fbdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-omap@...r.kernel.org
Subject: Re: [PATCH v2 1/1] OMAP4: DSS: Add panel for Blaze Tablet boards

> >> +     char name[30];
> >> +     char buf[50];
> >> +
> >> +     if (size >= sizeof(buf))
> >> +             size = sizeof(buf);
> >
> > what's the point of this?
> 
> This is a way to limit copied from userspace data by available buffer size,
> widely used in current kernel sources. Are you implying there is some
> better (more graceful) way?

No indeed :)
There is no other way, sorry for polluting the review :)

> >> +     if ((pins[2] & 1) || (pins[3] & 1)) {
> >> +             lanes |= (1 << 1);
> >> +             ret |= tc358765_write_register(dssdev, PPI_D0S_CLRSIPOCOUNT,
> >> +                                                     board_data->clrsipo);
> >> +     }
> >> +     if ((pins[4] & 1) || (pins[5] & 1)) {
> >> +             lanes |= (1 << 2);
> >> +             ret |= tc358765_write_register(dssdev, PPI_D1S_CLRSIPOCOUNT,
> >> +                                                     board_data->clrsipo);
> >> +     }
> >> +     if ((pins[6] & 1) || (pins[7] & 1)) {
> >> +             lanes |= (1 << 3);
> >> +             ret |= tc358765_write_register(dssdev, PPI_D2S_CLRSIPOCOUNT,
> >> +                                                     board_data->clrsipo);
> >> +     }
> >> +     if ((pins[8] & 1) || (pins[9] & 1)) {
> >> +             lanes |= (1 << 4);
> >> +             ret |= tc358765_write_register(dssdev, PPI_D3S_CLRSIPOCOUNT,
> >> +                                                     board_data->clrsipo);
> >> +     }
> >
> > Can't this be done in one single multiwrighting command since
> > this registers are consecutive?
> >
> > You build once the array to write and you send it at once.
> 
> In this particular case I disagree. Yes, it will be a little bit
> faster, however:
> 1) we write this for panel initialization only (so no impact in other cases)
> 2) multiwriting of array will make code reading more difficult
> 
> So I would like to leave it as-is
> Same is for next your similar comment.

If the hw is providing us some ways for simplifying the code I
would use it. In this case we are talking about the i2c feature
of multiwriting and multireading.

Let's assume that we want to write on 8 different consecutive
registers. In my opinion this aproach is quite "heavy":

  uX register;

  register = value1;
  i2c_write(REG1, register);

  register = value2;
  i2c_write(REG2, register);

  ...

Usually what I do is this:

  uX register[8];

  for (i = 0; i < 8; i++)
    register |= valuei << i; (or register[i] = valuei or whatever)

  i2c_multi_write(REG, register, 8);

of course this is a simplified example in pseudocode. I think
it's more readable and we are making a better use of the i2c
protocol.

In your case you have some if statement that are making the multi
writing more difficult, but still is not impossible.

At the end it's still a matter of taste, so that you are free to
choose whatever you prefer :)

Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists