From: Andi Shyti <andi.shyti@gmail.com> To: Ruslan Bilovol <ruslan.bilovol@ti.com> Cc: Andi Shyti <andi@etezian.org>, tomi.valkeinen@ti.com, FlorianSchandinat@gmx.de, linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org Subject: Re: [PATCH v2 1/1] OMAP4: DSS: Add panel for Blaze Tablet boards Date: Sun, 17 Feb 2013 15:17:57 +0100 [thread overview] Message-ID: <20130217141757.GA5228@jack.whiskey> (raw) In-Reply-To: <CAB=otbSpspZ9Z_fOpBvE1mPtt+JwZydZrgcR2LdkD_4CrGa_eg@mail.gmail.com> > >> + 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
WARNING: multiple messages have this Message-ID (diff)
From: Andi Shyti <andi.shyti@gmail.com> To: Ruslan Bilovol <ruslan.bilovol@ti.com> Cc: Andi Shyti <andi@etezian.org>, tomi.valkeinen@ti.com, FlorianSchandinat@gmx.de, linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org Subject: Re: [PATCH v2 1/1] OMAP4: DSS: Add panel for Blaze Tablet boards Date: Sun, 17 Feb 2013 14:17:57 +0000 [thread overview] Message-ID: <20130217141757.GA5228@jack.whiskey> (raw) In-Reply-To: <CAB=otbSpspZ9Z_fOpBvE1mPtt+JwZydZrgcR2LdkD_4CrGa_eg@mail.gmail.com> > >> + 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
next prev parent reply other threads:[~2013-02-17 14:17 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-02-08 15:43 [PATCH v2 0/1] OMAP4: DSS: Add panel for Blaze Tablet boards Ruslan Bilovol 2013-02-08 15:43 ` Ruslan Bilovol 2013-02-08 15:43 ` [PATCH v2 1/1] " Ruslan Bilovol 2013-02-08 15:43 ` Ruslan Bilovol 2013-02-10 23:51 ` Andi Shyti 2013-02-10 23:51 ` Andi Shyti 2013-02-13 19:23 ` Ruslan Bilovol 2013-02-13 19:23 ` Ruslan Bilovol 2013-02-17 14:17 ` Andi Shyti [this message] 2013-02-17 14:17 ` Andi Shyti 2013-02-18 14:33 ` Ruslan Bilovol 2013-02-18 14:33 ` Ruslan Bilovol 2013-02-17 14:46 ` Andi Shyti 2013-02-13 8:18 ` [PATCH v2 0/1] " Tomi Valkeinen 2013-02-13 8:18 ` Tomi Valkeinen 2013-02-13 8:18 ` Tomi Valkeinen 2013-02-14 0:07 ` Ruslan Bilovol 2013-02-14 0:07 ` Ruslan Bilovol 2013-02-14 9:24 ` Tomi Valkeinen 2013-02-14 9:24 ` Tomi Valkeinen 2013-02-14 9:24 ` Tomi Valkeinen 2013-02-18 14:30 ` Ruslan Bilovol 2013-02-18 14:30 ` Ruslan Bilovol 2013-02-19 11:34 ` Tomi Valkeinen 2013-02-19 11:34 ` Tomi Valkeinen 2013-02-19 11:34 ` Tomi Valkeinen
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20130217141757.GA5228@jack.whiskey \ --to=andi.shyti@gmail.com \ --cc=FlorianSchandinat@gmx.de \ --cc=andi@etezian.org \ --cc=linux-fbdev@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ --cc=ruslan.bilovol@ti.com \ --cc=tomi.valkeinen@ti.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.