2015-06-30 06:43:49

by Fabio Falzoi

[permalink] [raw]
Subject: [PATCH 00/10] Staging: fbtft: flexfb.c file clean up

This patchset aims to refactor the code in flexfb.c, making it more
readable and maintanable.
Various checkpatch.pl issues are fixed.
These patches are created on top of greg-kh's linux-staging tree,
staging-testing branch.

Fabio Falzoi (10):
Staging: fbtft: Remove paragraph about writing to FSF
Staging: fbtft: Remove unnecessary multiple blank lines
Staging: fbtft: Use a struct to describe each LCD controller
Staging: fbtft: Use a helper function to set write_register op
Staging: fbtft: Set bus specific ops using separate functions
Staging: fbtft: Use a helper function to set set_addr_win op
Staging: fbtft: Remove useless newline
Staging: fbtft: Avoid duplicating code to check gpio.dc value
Staging: fbtft: Fix parenthesis alignment coding style issue
Staging: fbtft: Fix spacing coding style issue

drivers/staging/fbtft/fbtft.h | 20 ++
drivers/staging/fbtft/flexfb.c | 485 +++++++++++++++++++++++------------------
2 files changed, 289 insertions(+), 216 deletions(-)

--
2.1.4


2015-06-30 06:43:59

by Fabio Falzoi

[permalink] [raw]
Subject: [PATCH 01/10] Staging: fbtft: Remove paragraph about writing to FSF

Remove paragraph about writing to the Free Software Foundation's
mailing address from GPL notice.
This patch fixes the following checkpatch error:

CHECK:FSF_MAILING_ADDRESS at line 17.

Signed-off-by: Fabio Falzoi <[email protected]>
---
drivers/staging/fbtft/flexfb.c | 4 ----
1 file changed, 4 deletions(-)

diff --git a/drivers/staging/fbtft/flexfb.c b/drivers/staging/fbtft/flexfb.c
index 2c4ce07..80b6620 100644
--- a/drivers/staging/fbtft/flexfb.c
+++ b/drivers/staging/fbtft/flexfb.c
@@ -12,10 +12,6 @@
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/

#include <linux/module.h>
--
2.1.4

2015-06-30 06:46:10

by Fabio Falzoi

[permalink] [raw]
Subject: [PATCH 02/10] Staging: fbtft: Remove unnecessary multiple blank lines

This patch removes some unnecessary multiple blank lines to fix the
following checkpatch errors:

CHECK:LINE_SPACING at lines 29, 67, 131, 287, 299, 312, 326, 351 and
364.

Signed-off-by: Fabio Falzoi <[email protected]>
---
drivers/staging/fbtft/flexfb.c | 11 -----------
1 file changed, 11 deletions(-)

diff --git a/drivers/staging/fbtft/flexfb.c b/drivers/staging/fbtft/flexfb.c
index 80b6620..ed867e7 100644
--- a/drivers/staging/fbtft/flexfb.c
+++ b/drivers/staging/fbtft/flexfb.c
@@ -26,7 +26,6 @@

#define DRVNAME "flexfb"

-
static char *chip;
module_param(chip, charp, 0);
MODULE_PARM_DESC(chip, "LCD controller");
@@ -64,7 +63,6 @@ static bool latched;
module_param(latched, bool, 0);
MODULE_PARM_DESC(latched, "Use with latched 16-bit databus");

-
static int *initp;
static int initp_num;

@@ -128,7 +126,6 @@ static int ssd1351_init[] = { -1, 0xfd, 0x12, -1, 0xfd, 0xb1, -1, 0xae, -1, 0xb3
-1, 0xab, 0x01, -1, 0xb1, 0x32, -1, 0xb4, 0xa0, 0xb5, 0x55, -1, 0xbb, 0x17, -1, 0xbe, 0x05,
-1, 0xc1, 0xc8, 0x80, 0xc8, -1, 0xc7, 0x0f, -1, 0xb6, 0x01, -1, 0xa6, -1, 0xaf, -3 };

-
/* ili9320, ili9325 */
static void flexfb_set_addr_win_1(struct fbtft_par *par,
int xs, int ys, int xe, int ye)
@@ -284,7 +281,6 @@ static int flexfb_probe_common(struct spi_device *sdev,
initp_num = ARRAY_SIZE(st7735r_init);
}

-
} else if (!strcmp(chip, "hx8340bn")) {
if (!width)
width = 176;
@@ -296,7 +292,6 @@ static int flexfb_probe_common(struct spi_device *sdev,
initp_num = ARRAY_SIZE(hx8340bn_init);
}

-
} else if (!strcmp(chip, "ili9225")) {
if (!width)
width = 176;
@@ -309,8 +304,6 @@ static int flexfb_probe_common(struct spi_device *sdev,
initp_num = ARRAY_SIZE(ili9225_init);
}

-
-
} else if (!strcmp(chip, "ili9320")) {
if (!width)
width = 240;
@@ -323,7 +316,6 @@ static int flexfb_probe_common(struct spi_device *sdev,
initp_num = ARRAY_SIZE(ili9320_init);
}

-
} else if (!strcmp(chip, "ili9325")) {
if (!width)
width = 240;
@@ -348,7 +340,6 @@ static int flexfb_probe_common(struct spi_device *sdev,
initp_num = ARRAY_SIZE(ili9341_init);
}

-
} else if (!strcmp(chip, "ssd1289")) {
if (!width)
width = 240;
@@ -361,8 +352,6 @@ static int flexfb_probe_common(struct spi_device *sdev,
initp_num = ARRAY_SIZE(ssd1289_init);
}

-
-
} else if (!strcmp(chip, "ssd1351")) {
if (!width)
width = 128;
--
2.1.4

2015-06-30 06:46:01

by Fabio Falzoi

[permalink] [raw]
Subject: [PATCH 03/10] Staging: fbtft: Use a struct to describe each LCD controller

Use a struct flexfb_lcd_controller to holds chip properties, instead of
relying on a long 'if - else if' chain.
This allows to:
- use a simple linear search to verify if a certain LCD controller
model is supported or not.
- add support for a new LCD chip controller simply defining a new
flexfb_lcd_controller struct.

Signed-off-by: Fabio Falzoi <[email protected]>
---
drivers/staging/fbtft/fbtft.h | 20 ++++
drivers/staging/fbtft/flexfb.c | 212 ++++++++++++++++++++++-------------------
2 files changed, 136 insertions(+), 96 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index 7d817eb..c96c06b 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -256,6 +256,26 @@ struct fbtft_par {
void *extra;
};

+/**
+ * struct flexfb_lcd_controller - Describes the LCD controller properties
+ * @name: Model name of the chip
+ * @width: Width of display in pixels
+ * @height: Height of display in pixels
+ * @setaddrwin: Which set_addr_win() implementation to use
+ * @regwidth: LCD Controller Register width in bits
+ * @init_seq: LCD initialization sequence
+ * @init_seq_sz: Size of LCD initialization sequence
+ */
+struct flexfb_lcd_controller {
+ const char *name;
+ unsigned int width;
+ unsigned int height;
+ unsigned int setaddrwin;
+ unsigned int regwidth;
+ int *init_seq;
+ int init_seq_sz;
+};
+
#define NUMARGS(...) (sizeof((int[]){__VA_ARGS__})/sizeof(int))

#define write_reg(par, ...) \
diff --git a/drivers/staging/fbtft/flexfb.c b/drivers/staging/fbtft/flexfb.c
index ed867e7..25b394d 100644
--- a/drivers/staging/fbtft/flexfb.c
+++ b/drivers/staging/fbtft/flexfb.c
@@ -126,6 +126,89 @@ static int ssd1351_init[] = { -1, 0xfd, 0x12, -1, 0xfd, 0xb1, -1, 0xae, -1, 0xb3
-1, 0xab, 0x01, -1, 0xb1, 0x32, -1, 0xb4, 0xa0, 0xb5, 0x55, -1, 0xbb, 0x17, -1, 0xbe, 0x05,
-1, 0xc1, 0xc8, 0x80, 0xc8, -1, 0xc7, 0x0f, -1, 0xb6, 0x01, -1, 0xa6, -1, 0xaf, -3 };

+static const struct flexfb_lcd_controller flexfb_chip_table[] = {
+ {
+ .name = "st7735r",
+ .width = 120,
+ .height = 160,
+ .init_seq = st7735r_init,
+ .init_seq_sz = ARRAY_SIZE(st7735r_init),
+ },
+ {
+ .name = "hx8340bn",
+ .width = 176,
+ .height = 220,
+ .init_seq = hx8340bn_init,
+ .init_seq_sz = ARRAY_SIZE(hx8340bn_init),
+ },
+ {
+ .name = "ili9225",
+ .width = 176,
+ .height = 220,
+ .regwidth = 16,
+ .init_seq = ili9225_init,
+ .init_seq_sz = ARRAY_SIZE(ili9225_init),
+ },
+ {
+ .name = "ili9225",
+ .width = 176,
+ .height = 220,
+ .regwidth = 16,
+ .init_seq = ili9225_init,
+ .init_seq_sz = ARRAY_SIZE(ili9225_init),
+ },
+ {
+ .name = "ili9225",
+ .width = 176,
+ .height = 220,
+ .regwidth = 16,
+ .init_seq = ili9225_init,
+ .init_seq_sz = ARRAY_SIZE(ili9225_init),
+ },
+ {
+ .name = "ili9320",
+ .width = 240,
+ .height = 320,
+ .setaddrwin = 1,
+ .regwidth = 16,
+ .init_seq = ili9320_init,
+ .init_seq_sz = ARRAY_SIZE(ili9320_init),
+ },
+ {
+ .name = "ili9325",
+ .width = 240,
+ .height = 320,
+ .setaddrwin = 1,
+ .regwidth = 16,
+ .init_seq = ili9325_init,
+ .init_seq_sz = ARRAY_SIZE(ili9325_init),
+ },
+ {
+ .name = "ili9341",
+ .width = 240,
+ .height = 320,
+ .init_seq = ili9341_init,
+ .init_seq_sz = ARRAY_SIZE(ili9341_init),
+ },
+ {
+ .name = "ssd1289",
+ .width = 240,
+ .height = 320,
+ .setaddrwin = 2,
+ .regwidth = 16,
+ .init_seq = ssd1289_init,
+ .init_seq_sz = ARRAY_SIZE(ssd1289_init),
+ },
+ {
+ .name = "ssd1351",
+ .width = 128,
+ .height = 128,
+ .setaddrwin = 3,
+ .init_seq = ssd1351_init,
+ .init_seq_sz = ARRAY_SIZE(ssd1351_init),
+ },
+};
+
/* ili9320, ili9325 */
static void flexfb_set_addr_win_1(struct fbtft_par *par,
int xs, int ys, int xe, int ye)
@@ -248,8 +331,38 @@ static int flexfb_verify_gpios_db(struct fbtft_par *par)
return 0;
}

+static void flexfb_chip_load_param(const struct flexfb_lcd_controller *chip)
+{
+ if (!width)
+ width = chip->width;
+ if (!height)
+ height = chip->height;
+ setaddrwin = chip->setaddrwin;
+ if (chip->regwidth)
+ regwidth = chip->regwidth;
+ if (!init_num) {
+ initp = chip->init_seq;
+ initp_num = chip->init_seq_sz;
+ }
+}
+
static struct fbtft_display flex_display = { };

+static int flexfb_chip_init(const struct device *dev)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(flexfb_chip_table); i++)
+ if (!strcmp(chip, flexfb_chip_table[i].name)) {
+ flexfb_chip_load_param(&flexfb_chip_table[i]);
+ return 0;
+ }
+
+ dev_err(dev, "chip=%s is not supported\n", chip);
+
+ return -EINVAL;
+}
+
static int flexfb_probe_common(struct spi_device *sdev,
struct platform_device *pdev)
{
@@ -270,102 +383,9 @@ static int flexfb_probe_common(struct spi_device *sdev,
sdev ? "'SPI device'" : "'Platform device'");

if (chip) {
-
- if (!strcmp(chip, "st7735r")) {
- if (!width)
- width = 128;
- if (!height)
- height = 160;
- if (init_num == 0) {
- initp = st7735r_init;
- initp_num = ARRAY_SIZE(st7735r_init);
- }
-
- } else if (!strcmp(chip, "hx8340bn")) {
- if (!width)
- width = 176;
- if (!height)
- height = 220;
- setaddrwin = 0;
- if (init_num == 0) {
- initp = hx8340bn_init;
- initp_num = ARRAY_SIZE(hx8340bn_init);
- }
-
- } else if (!strcmp(chip, "ili9225")) {
- if (!width)
- width = 176;
- if (!height)
- height = 220;
- setaddrwin = 0;
- regwidth = 16;
- if (init_num == 0) {
- initp = ili9225_init;
- initp_num = ARRAY_SIZE(ili9225_init);
- }
-
- } else if (!strcmp(chip, "ili9320")) {
- if (!width)
- width = 240;
- if (!height)
- height = 320;
- setaddrwin = 1;
- regwidth = 16;
- if (init_num == 0) {
- initp = ili9320_init;
- initp_num = ARRAY_SIZE(ili9320_init);
- }
-
- } else if (!strcmp(chip, "ili9325")) {
- if (!width)
- width = 240;
- if (!height)
- height = 320;
- setaddrwin = 1;
- regwidth = 16;
- if (init_num == 0) {
- initp = ili9325_init;
- initp_num = ARRAY_SIZE(ili9325_init);
- }
-
- } else if (!strcmp(chip, "ili9341")) {
- if (!width)
- width = 240;
- if (!height)
- height = 320;
- setaddrwin = 0;
- regwidth = 8;
- if (init_num == 0) {
- initp = ili9341_init;
- initp_num = ARRAY_SIZE(ili9341_init);
- }
-
- } else if (!strcmp(chip, "ssd1289")) {
- if (!width)
- width = 240;
- if (!height)
- height = 320;
- setaddrwin = 2;
- regwidth = 16;
- if (init_num == 0) {
- initp = ssd1289_init;
- initp_num = ARRAY_SIZE(ssd1289_init);
- }
-
- } else if (!strcmp(chip, "ssd1351")) {
- if (!width)
- width = 128;
- if (!height)
- height = 128;
- setaddrwin = 3;
- if (init_num == 0) {
- initp = ssd1351_init;
- initp_num = ARRAY_SIZE(ssd1351_init);
- }
- } else {
- dev_err(dev, "chip=%s is not supported\n", chip);
- return -EINVAL;
- }
+ ret = flexfb_chip_init(dev);
+ if (ret)
+ return ret;
}

if (width == 0 || height == 0) {
--
2.1.4

2015-06-30 06:44:16

by Fabio Falzoi

[permalink] [raw]
Subject: [PATCH 04/10] Staging: fbtft: Use a helper function to set write_register op

Use a helper function to set the correct write_register function, based
on the width of the registers.

Signed-off-by: Fabio Falzoi <[email protected]>
---
drivers/staging/fbtft/flexfb.c | 36 ++++++++++++++++++++++--------------
1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/fbtft/flexfb.c b/drivers/staging/fbtft/flexfb.c
index 25b394d..dae092a 100644
--- a/drivers/staging/fbtft/flexfb.c
+++ b/drivers/staging/fbtft/flexfb.c
@@ -363,6 +363,25 @@ static int flexfb_chip_init(const struct device *dev)
return -EINVAL;
}

+static int flexfb_set_regwrite_func(const struct device *dev,
+ struct fbtft_par *par)
+{
+ switch (regwidth) {
+ case 8:
+ par->fbtftops.write_register = fbtft_write_reg8_bus8;
+ break;
+ case 16:
+ par->fbtftops.write_register = fbtft_write_reg16_bus8;
+ break;
+ default:
+ dev_err(dev, "argument 'regwidth': %d is not supported.\n",
+ regwidth);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int flexfb_probe_common(struct spi_device *sdev,
struct platform_device *pdev)
{
@@ -413,20 +432,9 @@ static int flexfb_probe_common(struct spi_device *sdev,
par->init_sequence = initp;
par->fbtftops.init_display = fbtft_init_display;

- /* registerwrite functions */
- switch (regwidth) {
- case 8:
- par->fbtftops.write_register = fbtft_write_reg8_bus8;
- break;
- case 16:
- par->fbtftops.write_register = fbtft_write_reg16_bus8;
- break;
- default:
- dev_err(dev,
- "argument 'regwidth': %d is not supported.\n",
- regwidth);
- return -EINVAL;
- }
+ ret = flexfb_set_regwrite_func(dev, par);
+ if (ret)
+ goto out_release;

/* bus functions */
if (sdev) {
--
2.1.4

2015-06-30 06:44:20

by Fabio Falzoi

[permalink] [raw]
Subject: [PATCH 05/10] Staging: fbtft: Set bus specific ops using separate functions

Use two separate functions for spi and platform bus
(flexfb_set_spi_bus_func and flexfb_set_platform_bus_func,
respectively) to set the appropriate write operations.

This patch corrects the following checkpatch errors:
WARNING:LONG_LiINE at lines 446, 450, 466, 476, 489 and 495.
CHECK:PARENTHESIS_ALIGNMENT at line 459.

Signed-off-by: Fabio Falzoi <[email protected]>
---
drivers/staging/fbtft/flexfb.c | 154 +++++++++++++++++++++++++----------------
1 file changed, 94 insertions(+), 60 deletions(-)

diff --git a/drivers/staging/fbtft/flexfb.c b/drivers/staging/fbtft/flexfb.c
index dae092a..1b833f9 100644
--- a/drivers/staging/fbtft/flexfb.c
+++ b/drivers/staging/fbtft/flexfb.c
@@ -382,6 +382,94 @@ static int flexfb_set_regwrite_func(const struct device *dev,
return 0;
}

+static int flexfb_emulate_spi_8(struct fbtft_par *par, struct spi_device *sdev)
+{
+ struct device *dev = &sdev->dev;
+ int ret;
+
+ dev_warn(dev, "9-bit SPI not available, emulating using 8-bit.\n");
+ sdev->bits_per_word = 8;
+ ret = sdev->master->setup(sdev);
+ if (ret)
+ return ret;
+
+ /* allocate buffer with room for dc bits */
+ par->extra = devm_kzalloc(par->info->device,
+ par->txbuf.len + (par->txbuf.len / 8) + 8,
+ GFP_KERNEL);
+ if (!par->extra)
+ return -ENOMEM;
+ par->fbtftops.write = fbtft_write_spi_emulate_9;
+
+ return 0;
+}
+
+static int flexfb_set_spi_bus_func(struct fbtft_par *par,
+ struct spi_device *sdev)
+{
+ struct device *dev = &sdev->dev;
+ int ret;
+
+ par->fbtftops.write = fbtft_write_spi;
+ switch (buswidth) {
+ case 8:
+ par->fbtftops.write_vmem = fbtft_write_vmem16_bus8;
+ if (!par->startbyte)
+ par->fbtftops.verify_gpios = flexfb_verify_gpios_dc;
+ break;
+ case 9:
+ if (regwidth == 16) {
+ dev_err(dev, "argument 'regwidth': %d is not supported with buswidth=%d and SPI.\n",
+ regwidth, buswidth);
+ return -EINVAL;
+ }
+ par->fbtftops.write_register = fbtft_write_reg8_bus9;
+ par->fbtftops.write_vmem = fbtft_write_vmem16_bus9;
+ sdev->bits_per_word = 9;
+ ret = sdev->master->setup(sdev);
+ if (ret) {
+ ret = flexfb_emulate_spi_8(par, sdev);
+ if (ret)
+ return ret;
+ }
+ break;
+ default:
+ dev_err(dev, "argument 'buswidth': %d is not supported with SPI.\n",
+ buswidth);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int flexfb_set_platform_bus_func(struct fbtft_par *par,
+ struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+
+ par->fbtftops.verify_gpios = flexfb_verify_gpios_db;
+ switch (buswidth) {
+ case 8:
+ par->fbtftops.write = fbtft_write_gpio8_wr;
+ par->fbtftops.write_vmem = fbtft_write_vmem16_bus8;
+ break;
+ case 16:
+ par->fbtftops.write_register = fbtft_write_reg16_bus16;
+ if (latched)
+ par->fbtftops.write = fbtft_write_gpio16_wr_latched;
+ else
+ par->fbtftops.write = fbtft_write_gpio16_wr;
+ par->fbtftops.write_vmem = fbtft_write_vmem16_bus16;
+ break;
+ default:
+ dev_err(dev, "argument 'buswidth': %d is not supported with parallel.\n",
+ buswidth);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int flexfb_probe_common(struct spi_device *sdev,
struct platform_device *pdev)
{
@@ -436,66 +524,12 @@ static int flexfb_probe_common(struct spi_device *sdev,
if (ret)
goto out_release;

- /* bus functions */
- if (sdev) {
- par->fbtftops.write = fbtft_write_spi;
- switch (buswidth) {
- case 8:
- par->fbtftops.write_vmem = fbtft_write_vmem16_bus8;
- if (!par->startbyte)
- par->fbtftops.verify_gpios = flexfb_verify_gpios_dc;
- break;
- case 9:
- if (regwidth == 16) {
- dev_err(dev, "argument 'regwidth': %d is not supported with buswidth=%d and SPI.\n", regwidth, buswidth);
- return -EINVAL;
- }
- par->fbtftops.write_register = fbtft_write_reg8_bus9;
- par->fbtftops.write_vmem = fbtft_write_vmem16_bus9;
- sdev->bits_per_word = 9;
- ret = sdev->master->setup(sdev);
- if (ret) {
- dev_warn(dev,
- "9-bit SPI not available, emulating using 8-bit.\n");
- sdev->bits_per_word = 8;
- ret = sdev->master->setup(sdev);
- if (ret)
- goto out_release;
- /* allocate buffer with room for dc bits */
- par->extra = devm_kzalloc(par->info->device,
- par->txbuf.len + (par->txbuf.len / 8) + 8,
- GFP_KERNEL);
- if (!par->extra) {
- ret = -ENOMEM;
- goto out_release;
- }
- par->fbtftops.write = fbtft_write_spi_emulate_9;
- }
- break;
- default:
- dev_err(dev, "argument 'buswidth': %d is not supported with SPI.\n", buswidth);
- return -EINVAL;
- }
- } else {
- par->fbtftops.verify_gpios = flexfb_verify_gpios_db;
- switch (buswidth) {
- case 8:
- par->fbtftops.write = fbtft_write_gpio8_wr;
- par->fbtftops.write_vmem = fbtft_write_vmem16_bus8;
- break;
- case 16:
- par->fbtftops.write_register = fbtft_write_reg16_bus16;
- if (latched)
- par->fbtftops.write = fbtft_write_gpio16_wr_latched;
- else
- par->fbtftops.write = fbtft_write_gpio16_wr;
- par->fbtftops.write_vmem = fbtft_write_vmem16_bus16;
- break;
- default:
- dev_err(dev, "argument 'buswidth': %d is not supported with parallel.\n", buswidth);
- return -EINVAL;
- }
- }
+ if (sdev)
+ ret = flexfb_set_spi_bus_func(par, sdev);
+ else
+ ret = flexfb_set_platform_bus_func(par, pdev);
+ if (ret)
+ goto out_release;

/* set_addr_win function */
switch (setaddrwin) {
--
2.1.4

2015-06-30 06:45:19

by Fabio Falzoi

[permalink] [raw]
Subject: [PATCH 06/10] Staging: fbtft: Use a helper function to set set_addr_win op

Use a helper function to choose which set_addr_win implementation to
use, based on the value of the setaddrwin module parameter.

Signed-off-by: Fabio Falzoi <[email protected]>
---
drivers/staging/fbtft/flexfb.c | 47 +++++++++++++++++++++++++-----------------
1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/fbtft/flexfb.c b/drivers/staging/fbtft/flexfb.c
index 1b833f9..2a15da1 100644
--- a/drivers/staging/fbtft/flexfb.c
+++ b/drivers/staging/fbtft/flexfb.c
@@ -470,6 +470,31 @@ static int flexfb_set_platform_bus_func(struct fbtft_par *par,
return 0;
}

+static int flexfb_set_addr_win_func(const struct device *dev,
+ struct fbtft_par *par)
+{
+ switch (setaddrwin) {
+ case 0:
+ /* use default */
+ break;
+ case 1:
+ par->fbtftops.set_addr_win = flexfb_set_addr_win_1;
+ break;
+ case 2:
+ par->fbtftops.set_addr_win = flexfb_set_addr_win_2;
+ break;
+ case 3:
+ par->fbtftops.set_addr_win = set_addr_win_3;
+ break;
+ default:
+ dev_err(dev, "argument 'setaddrwin': unknown value %d.\n",
+ setaddrwin);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int flexfb_probe_common(struct spi_device *sdev,
struct platform_device *pdev)
{
@@ -531,25 +556,9 @@ static int flexfb_probe_common(struct spi_device *sdev,
if (ret)
goto out_release;

- /* set_addr_win function */
- switch (setaddrwin) {
- case 0:
- /* use default */
- break;
- case 1:
- par->fbtftops.set_addr_win = flexfb_set_addr_win_1;
- break;
- case 2:
- par->fbtftops.set_addr_win = flexfb_set_addr_win_2;
- break;
- case 3:
- par->fbtftops.set_addr_win = set_addr_win_3;
- break;
- default:
- dev_err(dev, "argument 'setaddrwin': unknown value %d.\n",
- setaddrwin);
- return -EINVAL;
- }
+ ret = flexfb_set_addr_win_func(dev, par);
+ if (ret)
+ goto out_release;

if (!nobacklight)
par->fbtftops.register_backlight = fbtft_register_backlight;
--
2.1.4

2015-06-30 06:45:12

by Fabio Falzoi

[permalink] [raw]
Subject: [PATCH 07/10] Staging: fbtft: Remove useless newline

No newline is needed since checkpatch doesn't complain about line longer
than 80 characters for string literals.

Signed-off-by: Fabio Falzoi <[email protected]>
---
drivers/staging/fbtft/flexfb.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/fbtft/flexfb.c b/drivers/staging/fbtft/flexfb.c
index 2a15da1..cccb0e4 100644
--- a/drivers/staging/fbtft/flexfb.c
+++ b/drivers/staging/fbtft/flexfb.c
@@ -290,8 +290,7 @@ static int flexfb_verify_gpios_dc(struct fbtft_par *par)
fbtft_par_dbg(DEBUG_VERIFY_GPIOS, par, "%s()\n", __func__);

if (par->gpio.dc < 0) {
- dev_err(par->info->device,
- "Missing info about 'dc' gpio. Aborting.\n");
+ dev_err(par->info->device, "Missing info about 'dc' gpio. Aborting.\n");
return -EINVAL;
}

--
2.1.4

2015-06-30 06:45:02

by Fabio Falzoi

[permalink] [raw]
Subject: [PATCH 08/10] Staging: fbtft: Avoid duplicating code to check gpio.dc value

Avoid duplicating code to verify gpios.dc and call
flexfb_verify_gpios_dc instead.

Signed-off-by: Fabio Falzoi <[email protected]>
---
drivers/staging/fbtft/flexfb.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/fbtft/flexfb.c b/drivers/staging/fbtft/flexfb.c
index cccb0e4..ca14919 100644
--- a/drivers/staging/fbtft/flexfb.c
+++ b/drivers/staging/fbtft/flexfb.c
@@ -299,15 +299,14 @@ static int flexfb_verify_gpios_dc(struct fbtft_par *par)

static int flexfb_verify_gpios_db(struct fbtft_par *par)
{
- int i;
+ int i, ret;
int num_db = buswidth;

fbtft_par_dbg(DEBUG_VERIFY_GPIOS, par, "%s()\n", __func__);
+ ret = flexfb_verify_gpios_dc(par);
+ if (ret)
+ return ret;

- if (par->gpio.dc < 0) {
- dev_err(par->info->device, "Missing info about 'dc' gpio. Aborting.\n");
- return -EINVAL;
- }
if (par->gpio.wr < 0) {
dev_err(par->info->device, "Missing info about 'wr' gpio. Aborting.\n");
return -EINVAL;
--
2.1.4

2015-06-30 06:44:53

by Fabio Falzoi

[permalink] [raw]
Subject: [PATCH 09/10] Staging: fbtft: Fix parenthesis alignment coding style issue

This patch fixes the following checkpatch.pl error:

CHECK:PARENTHESIS_ALIGNMENT at line 217.

Signed-off-by: Fabio Falzoi <[email protected]>
---
drivers/staging/fbtft/flexfb.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/fbtft/flexfb.c b/drivers/staging/fbtft/flexfb.c
index ca14919..b161050 100644
--- a/drivers/staging/fbtft/flexfb.c
+++ b/drivers/staging/fbtft/flexfb.c
@@ -213,9 +213,8 @@ static const struct flexfb_lcd_controller flexfb_chip_table[] = {
static void flexfb_set_addr_win_1(struct fbtft_par *par,
int xs, int ys, int xe, int ye)
{
- fbtft_par_dbg(DEBUG_SET_ADDR_WIN, par,
- "%s(xs=%d, ys=%d, xe=%d, ye=%d)\n",
- __func__, xs, ys, xe, ye);
+ fbtft_par_dbg(DEBUG_SET_ADDR_WIN, par, "%s(xs=%d, ys=%d, xe=%d, ye=%d)\n",
+ __func__, xs, ys, xe, ye);
switch (par->info->var.rotate) {
/* R20h = Horizontal GRAM Start Address */
/* R21h = Vertical GRAM Start Address */
@@ -581,8 +580,8 @@ static int flexfb_remove_common(struct device *dev, struct fb_info *info)
return -EINVAL;
par = info->par;
if (par)
- fbtft_par_dbg(DEBUG_DRIVER_INIT_FUNCTIONS, par,
- "%s()\n", __func__);
+ fbtft_par_dbg(DEBUG_DRIVER_INIT_FUNCTIONS, par, "%s()\n",
+ __func__);
fbtft_unregister_framebuffer(info);
fbtft_framebuffer_release(info);

--
2.1.4

2015-06-30 06:44:45

by Fabio Falzoi

[permalink] [raw]
Subject: [PATCH 10/10] Staging: fbtft: Fix spacing coding style issue

This patch fixes the following checkpatch.pl error:

CHECK:SPACING at line 318.

Signed-off-by: Fabio Falzoi <[email protected]>
---
drivers/staging/fbtft/flexfb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/fbtft/flexfb.c b/drivers/staging/fbtft/flexfb.c
index b161050..6cd02b5 100644
--- a/drivers/staging/fbtft/flexfb.c
+++ b/drivers/staging/fbtft/flexfb.c
@@ -315,7 +315,7 @@ static int flexfb_verify_gpios_db(struct fbtft_par *par)
return -EINVAL;
}
if (latched)
- num_db = buswidth/2;
+ num_db = buswidth / 2;
for (i = 0; i < num_db; i++) {
if (par->gpio.db[i] < 0) {
dev_err(par->info->device,
--
2.1.4

2015-06-30 07:42:58

by Dan Carpenter

[permalink] [raw]
Subject: Re: [PATCH 07/10] Staging: fbtft: Remove useless newline

On Tue, Jun 30, 2015 at 08:43:14AM +0200, Fabio Falzoi wrote:
> No newline is needed since checkpatch doesn't complain about line longer
> than 80 characters for string literals.
>
> Signed-off-by: Fabio Falzoi <[email protected]>

The original was correct. There was no need to go over 80 characters
just because of a limitation in checkpatch.

regards,
dan carpenter

2015-06-30 14:55:52

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH 07/10] Staging: fbtft: Remove useless newline

On Tue, 2015-06-30 at 10:42 +0300, Dan Carpenter wrote:
> On Tue, Jun 30, 2015 at 08:43:14AM +0200, Fabio Falzoi wrote:
> > No newline is needed since checkpatch doesn't complain about line longer
> > than 80 characters for string literals.
[]
> The original was correct.

The original was fine.

> There was no need to go over 80 characters
> just because of a limitation in checkpatch.

How is checkpatch not emitting a message on
either form a limitation?

2015-06-30 17:46:06

by Dan Carpenter

[permalink] [raw]
Subject: Re: [PATCH 07/10] Staging: fbtft: Remove useless newline

On Tue, Jun 30, 2015 at 07:55:42AM -0700, Joe Perches wrote:
> On Tue, 2015-06-30 at 10:42 +0300, Dan Carpenter wrote:
> > On Tue, Jun 30, 2015 at 08:43:14AM +0200, Fabio Falzoi wrote:
> > > No newline is needed since checkpatch doesn't complain about line longer
> > > than 80 characters for string literals.
> []
> > The original was correct.
>
> The original was fine.
>
> > There was no need to go over 80 characters
> > just because of a limitation in checkpatch.
>
> How is checkpatch not emitting a message on
> either form a limitation?

It's not an easily solvable limitation, but the original was better than
the new patch. This patch is all like "I've found a way to do something
bad and checkpatch.pl doesn't catch it so woohoo!"

regards,
dan carpenter

2015-06-30 19:03:56

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH 07/10] Staging: fbtft: Remove useless newline

On Tue, 2015-06-30 at 20:45 +0300, Dan Carpenter wrote:
> On Tue, Jun 30, 2015 at 07:55:42AM -0700, Joe Perches wrote:
> > On Tue, 2015-06-30 at 10:42 +0300, Dan Carpenter wrote:
> > > On Tue, Jun 30, 2015 at 08:43:14AM +0200, Fabio Falzoi wrote:
> > > > No newline is needed since checkpatch doesn't complain about line longer
> > > > than 80 characters for string literals.
> > []
> > > The original was correct.
> >
> > The original was fine.
> >
> > > There was no need to go over 80 characters
> > > just because of a limitation in checkpatch.
> >
> > How is checkpatch not emitting a message on
> > either form a limitation?
>
> It's not an easily solvable limitation,

It's not a limitation at all.

> but the original was better than
> the new patch. This patch is all like "I've found a way to do something
> bad and checkpatch.pl doesn't catch it so woohoo!"

I think either form is fine, but because either is fine,
it's not useful/better to change either.

2015-07-15 02:12:53

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 03/10] Staging: fbtft: Use a struct to describe each LCD controller

On Tue, Jun 30, 2015 at 08:43:10AM +0200, Fabio Falzoi wrote:
> Use a struct flexfb_lcd_controller to holds chip properties, instead of
> relying on a long 'if - else if' chain.
> This allows to:
> - use a simple linear search to verify if a certain LCD controller
> model is supported or not.
> - add support for a new LCD chip controller simply defining a new
> flexfb_lcd_controller struct.
>
> Signed-off-by: Fabio Falzoi <[email protected]>
> ---
> drivers/staging/fbtft/fbtft.h | 20 ++++
> drivers/staging/fbtft/flexfb.c | 212 ++++++++++++++++++++++-------------------
> 2 files changed, 136 insertions(+), 96 deletions(-)

I need the maintainers to sign off on this before I can take it, as it's
not just a "simple" coding style fix.

thanks,

greg k-h

2015-07-15 02:13:29

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 04/10] Staging: fbtft: Use a helper function to set write_register op

On Tue, Jun 30, 2015 at 08:43:11AM +0200, Fabio Falzoi wrote:
> Use a helper function to set the correct write_register function, based
> on the width of the registers.
>
> Signed-off-by: Fabio Falzoi <[email protected]>
> ---
> drivers/staging/fbtft/flexfb.c | 36 ++++++++++++++++++++++--------------
> 1 file changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/staging/fbtft/flexfb.c b/drivers/staging/fbtft/flexfb.c
> index 25b394d..dae092a 100644
> --- a/drivers/staging/fbtft/flexfb.c
> +++ b/drivers/staging/fbtft/flexfb.c
> @@ -363,6 +363,25 @@ static int flexfb_chip_init(const struct device *dev)
> return -EINVAL;
> }
>
> +static int flexfb_set_regwrite_func(const struct device *dev,
> + struct fbtft_par *par)
> +{
> + switch (regwidth) {
> + case 8:
> + par->fbtftops.write_register = fbtft_write_reg8_bus8;
> + break;
> + case 16:
> + par->fbtftops.write_register = fbtft_write_reg16_bus8;
> + break;
> + default:
> + dev_err(dev, "argument 'regwidth': %d is not supported.\n",
> + regwidth);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> static int flexfb_probe_common(struct spi_device *sdev,
> struct platform_device *pdev)
> {
> @@ -413,20 +432,9 @@ static int flexfb_probe_common(struct spi_device *sdev,
> par->init_sequence = initp;
> par->fbtftops.init_display = fbtft_init_display;
>
> - /* registerwrite functions */
> - switch (regwidth) {
> - case 8:
> - par->fbtftops.write_register = fbtft_write_reg8_bus8;
> - break;
> - case 16:
> - par->fbtftops.write_register = fbtft_write_reg16_bus8;
> - break;
> - default:
> - dev_err(dev,
> - "argument 'regwidth': %d is not supported.\n",
> - regwidth);
> - return -EINVAL;
> - }
> + ret = flexfb_set_regwrite_func(dev, par);
> + if (ret)
> + goto out_release;
>
> /* bus functions */
> if (sdev) {

Why? You aren't calling this function anywhere else, so why move it?

thanks,

greg k-h

2015-07-15 02:14:03

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 05/10] Staging: fbtft: Set bus specific ops using separate functions

On Tue, Jun 30, 2015 at 08:43:12AM +0200, Fabio Falzoi wrote:
> Use two separate functions for spi and platform bus
> (flexfb_set_spi_bus_func and flexfb_set_platform_bus_func,
> respectively) to set the appropriate write operations.
>
> This patch corrects the following checkpatch errors:
> WARNING:LONG_LiINE at lines 446, 450, 466, 476, 489 and 495.
> CHECK:PARENTHESIS_ALIGNMENT at line 459.
>
> Signed-off-by: Fabio Falzoi <[email protected]>

Can't you fix the line lengths without having to move to separate
functions?

thanks,

greg k-h

2015-07-15 02:14:26

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 06/10] Staging: fbtft: Use a helper function to set set_addr_win op

On Tue, Jun 30, 2015 at 08:43:13AM +0200, Fabio Falzoi wrote:
> Use a helper function to choose which set_addr_win implementation to
> use, based on the value of the setaddrwin module parameter.
>
> Signed-off-by: Fabio Falzoi <[email protected]>
> ---
> drivers/staging/fbtft/flexfb.c | 47 +++++++++++++++++++++++++-----------------
> 1 file changed, 28 insertions(+), 19 deletions(-)

Again, why?

I need a maintainer of the code to ack any of these...

2015-07-23 11:09:39

by Noralf Trønnes

[permalink] [raw]
Subject: Re: [PATCH 03/10] Staging: fbtft: Use a struct to describe each LCD controller


Den 30.06.2015 08:43, skrev Fabio Falzoi:
> Use a struct flexfb_lcd_controller to holds chip properties, instead of
> relying on a long 'if - else if' chain.
> This allows to:
> - use a simple linear search to verify if a certain LCD controller
> model is supported or not.
> - add support for a new LCD chip controller simply defining a new
> flexfb_lcd_controller struct.
>
> Signed-off-by: Fabio Falzoi <[email protected]>
> ---
> drivers/staging/fbtft/fbtft.h | 20 ++++
> drivers/staging/fbtft/flexfb.c | 212 ++++++++++++++++++++++-------------------
> 2 files changed, 136 insertions(+), 96 deletions(-)
>
> diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
> index 7d817eb..c96c06b 100644
> --- a/drivers/staging/fbtft/fbtft.h
> +++ b/drivers/staging/fbtft/fbtft.h
> @@ -256,6 +256,26 @@ struct fbtft_par {
> void *extra;
> };
>
> +/**
> + * struct flexfb_lcd_controller - Describes the LCD controller properties
> + * @name: Model name of the chip
> + * @width: Width of display in pixels
> + * @height: Height of display in pixels
> + * @setaddrwin: Which set_addr_win() implementation to use
> + * @regwidth: LCD Controller Register width in bits
> + * @init_seq: LCD initialization sequence
> + * @init_seq_sz: Size of LCD initialization sequence
> + */
> +struct flexfb_lcd_controller {
> + const char *name;
> + unsigned int width;
> + unsigned int height;
> + unsigned int setaddrwin;
> + unsigned int regwidth;
> + int *init_seq;
> + int init_seq_sz;
> +};
> +

Please put this in flexfb.c since it won't be used outside that file.

> #define NUMARGS(...) (sizeof((int[]){__VA_ARGS__})/sizeof(int))
>
> #define write_reg(par, ...) \
> diff --git a/drivers/staging/fbtft/flexfb.c b/drivers/staging/fbtft/flexfb.c
> index ed867e7..25b394d 100644
> --- a/drivers/staging/fbtft/flexfb.c
> +++ b/drivers/staging/fbtft/flexfb.c
> @@ -126,6 +126,89 @@ static int ssd1351_init[] = { -1, 0xfd, 0x12, -1, 0xfd, 0xb1, -1, 0xae, -1, 0xb3
> -1, 0xab, 0x01, -1, 0xb1, 0x32, -1, 0xb4, 0xa0, 0xb5, 0x55, -1, 0xbb, 0x17, -1, 0xbe, 0x05,
> -1, 0xc1, 0xc8, 0x80, 0xc8, -1, 0xc7, 0x0f, -1, 0xb6, 0x01, -1, 0xa6, -1, 0xaf, -3 };
>
> +static const struct flexfb_lcd_controller flexfb_chip_table[] = {
> + {
> + .name = "st7735r",
> + .width = 120,
> + .height = 160,
> + .init_seq = st7735r_init,
> + .init_seq_sz = ARRAY_SIZE(st7735r_init),
> + },
> + {

Can this be put on one line? }, {

With the struct moved:
Acked-by: Noralf Tr?nnes <[email protected]>

2015-07-23 11:19:54

by Noralf Trønnes

[permalink] [raw]
Subject: Re: [PATCH 06/10] Staging: fbtft: Use a helper function to set set_addr_win op


Den 15.07.2015 04:14, skrev Greg KH:
> On Tue, Jun 30, 2015 at 08:43:13AM +0200, Fabio Falzoi wrote:
>> Use a helper function to choose which set_addr_win implementation to
>> use, based on the value of the setaddrwin module parameter.
>>
>> Signed-off-by: Fabio Falzoi <[email protected]>
>> ---
>> drivers/staging/fbtft/flexfb.c | 47 +++++++++++++++++++++++++-----------------
>> 1 file changed, 28 insertions(+), 19 deletions(-)
> Again, why?
>
> I need a maintainer of the code to ack any of these...
>

I know this code fairly well, but I have to look up the details.
Moving these details from the probe function into small functions
makes it more difficult for me get to those details in a glance.
I would now have to scroll back and forth to see how flexfb is
doing things, especially since flexfb differs from the other drivers
which uses common functions in fbtft-core.c.
So I prefer to keep it as it is.

2015-08-02 14:58:09

by Fabio Falzoi

[permalink] [raw]
Subject: [PATCH v2] Staging: fbtft: Use a struct to describe each LCD controller

Use a struct flexfb_lcd_controller to holds chip properties, instead of
relying on a long 'if - else if' chain.
This allows to:
- use a simple linear search to verify if a certain LCD controller
model is supported or not.
- add support for a new LCD chip controller simply defining a new
flexfb_lcd_controller struct.

Signed-off-by: Fabio Falzoi <[email protected]>
Acked-by: Noralf Trønnes <[email protected]>
---
drivers/staging/fbtft/fbtft.h | 20 ++++
drivers/staging/fbtft/flexfb.c | 203 ++++++++++++++++++++++-------------------
2 files changed, 127 insertions(+), 96 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index 7d817eb..c96c06b 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -256,6 +256,26 @@ struct fbtft_par {
void *extra;
};

+/**
+ * struct flexfb_lcd_controller - Describes the LCD controller properties
+ * @name: Model name of the chip
+ * @width: Width of display in pixels
+ * @height: Height of display in pixels
+ * @setaddrwin: Which set_addr_win() implementation to use
+ * @regwidth: LCD Controller Register width in bits
+ * @init_seq: LCD initialization sequence
+ * @init_seq_sz: Size of LCD initialization sequence
+ */
+struct flexfb_lcd_controller {
+ const char *name;
+ unsigned int width;
+ unsigned int height;
+ unsigned int setaddrwin;
+ unsigned int regwidth;
+ int *init_seq;
+ int init_seq_sz;
+};
+
#define NUMARGS(...) (sizeof((int[]){__VA_ARGS__})/sizeof(int))

#define write_reg(par, ...) \
diff --git a/drivers/staging/fbtft/flexfb.c b/drivers/staging/fbtft/flexfb.c
index ce6e3ae..e70678f 100644
--- a/drivers/staging/fbtft/flexfb.c
+++ b/drivers/staging/fbtft/flexfb.c
@@ -126,6 +126,80 @@ static int ssd1351_init[] = { -1, 0xfd, 0x12, -1, 0xfd, 0xb1, -1, 0xae, -1, 0xb3
-1, 0xab, 0x01, -1, 0xb1, 0x32, -1, 0xb4, 0xa0, 0xb5, 0x55, -1, 0xbb, 0x17, -1, 0xbe, 0x05,
-1, 0xc1, 0xc8, 0x80, 0xc8, -1, 0xc7, 0x0f, -1, 0xb6, 0x01, -1, 0xa6, -1, 0xaf, -3 };

+static const struct flexfb_lcd_controller flexfb_chip_table[] = {
+ {
+ .name = "st7735r",
+ .width = 120,
+ .height = 160,
+ .init_seq = st7735r_init,
+ .init_seq_sz = ARRAY_SIZE(st7735r_init),
+ }, {
+ .name = "hx8340bn",
+ .width = 176,
+ .height = 220,
+ .init_seq = hx8340bn_init,
+ .init_seq_sz = ARRAY_SIZE(hx8340bn_init),
+ }, {
+ .name = "ili9225",
+ .width = 176,
+ .height = 220,
+ .regwidth = 16,
+ .init_seq = ili9225_init,
+ .init_seq_sz = ARRAY_SIZE(ili9225_init),
+ }, {
+ .name = "ili9225",
+ .width = 176,
+ .height = 220,
+ .regwidth = 16,
+ .init_seq = ili9225_init,
+ .init_seq_sz = ARRAY_SIZE(ili9225_init),
+ }, {
+ .name = "ili9225",
+ .width = 176,
+ .height = 220,
+ .regwidth = 16,
+ .init_seq = ili9225_init,
+ .init_seq_sz = ARRAY_SIZE(ili9225_init),
+ }, {
+ .name = "ili9320",
+ .width = 240,
+ .height = 320,
+ .setaddrwin = 1,
+ .regwidth = 16,
+ .init_seq = ili9320_init,
+ .init_seq_sz = ARRAY_SIZE(ili9320_init),
+ }, {
+ .name = "ili9325",
+ .width = 240,
+ .height = 320,
+ .setaddrwin = 1,
+ .regwidth = 16,
+ .init_seq = ili9325_init,
+ .init_seq_sz = ARRAY_SIZE(ili9325_init),
+ }, {
+ .name = "ili9341",
+ .width = 240,
+ .height = 320,
+ .init_seq = ili9341_init,
+ .init_seq_sz = ARRAY_SIZE(ili9341_init),
+ }, {
+ .name = "ssd1289",
+ .width = 240,
+ .height = 320,
+ .setaddrwin = 2,
+ .regwidth = 16,
+ .init_seq = ssd1289_init,
+ .init_seq_sz = ARRAY_SIZE(ssd1289_init),
+ }, {
+ .name = "ssd1351",
+ .width = 128,
+ .height = 128,
+ .setaddrwin = 3,
+ .init_seq = ssd1351_init,
+ .init_seq_sz = ARRAY_SIZE(ssd1351_init),
+ },
+};
+
/* ili9320, ili9325 */
static void flexfb_set_addr_win_1(struct fbtft_par *par,
int xs, int ys, int xe, int ye)
@@ -247,8 +321,38 @@ static int flexfb_verify_gpios_db(struct fbtft_par *par)
return 0;
}

+static void flexfb_chip_load_param(const struct flexfb_lcd_controller *chip)
+{
+ if (!width)
+ width = chip->width;
+ if (!height)
+ height = chip->height;
+ setaddrwin = chip->setaddrwin;
+ if (chip->regwidth)
+ regwidth = chip->regwidth;
+ if (!init_num) {
+ initp = chip->init_seq;
+ initp_num = chip->init_seq_sz;
+ }
+}
+
static struct fbtft_display flex_display = { };

+static int flexfb_chip_init(const struct device *dev)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(flexfb_chip_table); i++)
+ if (!strcmp(chip, flexfb_chip_table[i].name)) {
+ flexfb_chip_load_param(&flexfb_chip_table[i]);
+ return 0;
+ }
+
+ dev_err(dev, "chip=%s is not supported\n", chip);
+
+ return -EINVAL;
+}
+
static int flexfb_probe_common(struct spi_device *sdev,
struct platform_device *pdev)
{
@@ -269,102 +373,9 @@ static int flexfb_probe_common(struct spi_device *sdev,
sdev ? "'SPI device'" : "'Platform device'");

if (chip) {
-
- if (!strcmp(chip, "st7735r")) {
- if (!width)
- width = 128;
- if (!height)
- height = 160;
- if (init_num == 0) {
- initp = st7735r_init;
- initp_num = ARRAY_SIZE(st7735r_init);
- }
-
- } else if (!strcmp(chip, "hx8340bn")) {
- if (!width)
- width = 176;
- if (!height)
- height = 220;
- setaddrwin = 0;
- if (init_num == 0) {
- initp = hx8340bn_init;
- initp_num = ARRAY_SIZE(hx8340bn_init);
- }
-
- } else if (!strcmp(chip, "ili9225")) {
- if (!width)
- width = 176;
- if (!height)
- height = 220;
- setaddrwin = 0;
- regwidth = 16;
- if (init_num == 0) {
- initp = ili9225_init;
- initp_num = ARRAY_SIZE(ili9225_init);
- }
-
- } else if (!strcmp(chip, "ili9320")) {
- if (!width)
- width = 240;
- if (!height)
- height = 320;
- setaddrwin = 1;
- regwidth = 16;
- if (init_num == 0) {
- initp = ili9320_init;
- initp_num = ARRAY_SIZE(ili9320_init);
- }
-
- } else if (!strcmp(chip, "ili9325")) {
- if (!width)
- width = 240;
- if (!height)
- height = 320;
- setaddrwin = 1;
- regwidth = 16;
- if (init_num == 0) {
- initp = ili9325_init;
- initp_num = ARRAY_SIZE(ili9325_init);
- }
-
- } else if (!strcmp(chip, "ili9341")) {
- if (!width)
- width = 240;
- if (!height)
- height = 320;
- setaddrwin = 0;
- regwidth = 8;
- if (init_num == 0) {
- initp = ili9341_init;
- initp_num = ARRAY_SIZE(ili9341_init);
- }
-
- } else if (!strcmp(chip, "ssd1289")) {
- if (!width)
- width = 240;
- if (!height)
- height = 320;
- setaddrwin = 2;
- regwidth = 16;
- if (init_num == 0) {
- initp = ssd1289_init;
- initp_num = ARRAY_SIZE(ssd1289_init);
- }
-
- } else if (!strcmp(chip, "ssd1351")) {
- if (!width)
- width = 128;
- if (!height)
- height = 128;
- setaddrwin = 3;
- if (init_num == 0) {
- initp = ssd1351_init;
- initp_num = ARRAY_SIZE(ssd1351_init);
- }
- } else {
- dev_err(dev, "chip=%s is not supported\n", chip);
- return -EINVAL;
- }
+ ret = flexfb_chip_init(dev);
+ if (ret)
+ return ret;
}

if (width == 0 || height == 0) {
--
2.1.4

2015-08-02 17:54:56

by Noralf Trønnes

[permalink] [raw]
Subject: Re: [PATCH v2] Staging: fbtft: Use a struct to describe each LCD controller


Den 02.08.2015 16:57, skrev Fabio Falzoi:
> Use a struct flexfb_lcd_controller to holds chip properties, instead of
> relying on a long 'if - else if' chain.
> This allows to:
> - use a simple linear search to verify if a certain LCD controller
> model is supported or not.
> - add support for a new LCD chip controller simply defining a new
> flexfb_lcd_controller struct.
>
> Signed-off-by: Fabio Falzoi <[email protected]>
> Acked-by: Noralf Trønnes <[email protected]>
> ---

Seems I wasn't clear enough, you could use my ack if you put
struct flexfb_lcd_controller inside the driver and not in
fbtft.h


Noralf.

2015-08-02 20:17:42

by Fabio Falzoi

[permalink] [raw]
Subject: Re: [PATCH v2] Staging: fbtft: Use a struct to describe each LCD controller

On Sun, Aug 02, 2015 at 07:54:48PM +0200, Noralf Trønnes wrote:
>
> Seems I wasn't clear enough, you could use my ack if you put
> struct flexfb_lcd_controller inside the driver and not in
> fbtft.h
>
>
> Noralf.
>
>

Sorry Noralf, I misunderstood your review.
I will modify the patch and send an updated version.

Thank you,
Fabio

2015-08-02 20:30:32

by Fabio Falzoi

[permalink] [raw]
Subject: [PATCH v3] Staging: fbtft: Use a struct to describe each LCD controller

Use a struct flexfb_lcd_controller to holds chip properties, instead of
relying on a long 'if - else if' chain.
This allows to:
- use a simple linear search to verify if a certain LCD controller
model is supported or not.
- add support for a new LCD chip controller simply defining a new
flexfb_lcd_controller struct.

Signed-off-by: Fabio Falzoi <[email protected]>
Acked-by: Noralf Trønnes <[email protected]>
---
drivers/staging/fbtft/flexfb.c | 232 ++++++++++++++++++++++++-----------------
1 file changed, 136 insertions(+), 96 deletions(-)

diff --git a/drivers/staging/fbtft/flexfb.c b/drivers/staging/fbtft/flexfb.c
index ce6e3ae..b3a68c9 100644
--- a/drivers/staging/fbtft/flexfb.c
+++ b/drivers/staging/fbtft/flexfb.c
@@ -126,6 +126,109 @@ static int ssd1351_init[] = { -1, 0xfd, 0x12, -1, 0xfd, 0xb1, -1, 0xae, -1, 0xb3
-1, 0xab, 0x01, -1, 0xb1, 0x32, -1, 0xb4, 0xa0, 0xb5, 0x55, -1, 0xbb, 0x17, -1, 0xbe, 0x05,
-1, 0xc1, 0xc8, 0x80, 0xc8, -1, 0xc7, 0x0f, -1, 0xb6, 0x01, -1, 0xa6, -1, 0xaf, -3 };

+/**
+ * struct flexfb_lcd_controller - Describes the LCD controller properties
+ * @name: Model name of the chip
+ * @width: Width of display in pixels
+ * @height: Height of display in pixels
+ * @setaddrwin: Which set_addr_win() implementation to use
+ * @regwidth: LCD Controller Register width in bits
+ * @init_seq: LCD initialization sequence
+ * @init_seq_sz: Size of LCD initialization sequence
+ */
+struct flexfb_lcd_controller {
+ const char *name;
+ unsigned int width;
+ unsigned int height;
+ unsigned int setaddrwin;
+ unsigned int regwidth;
+ int *init_seq;
+ int init_seq_sz;
+};
+
+static const struct flexfb_lcd_controller flexfb_chip_table[] = {
+ {
+ .name = "st7735r",
+ .width = 120,
+ .height = 160,
+ .init_seq = st7735r_init,
+ .init_seq_sz = ARRAY_SIZE(st7735r_init),
+ },
+ {
+ .name = "hx8340bn",
+ .width = 176,
+ .height = 220,
+ .init_seq = hx8340bn_init,
+ .init_seq_sz = ARRAY_SIZE(hx8340bn_init),
+ },
+ {
+ .name = "ili9225",
+ .width = 176,
+ .height = 220,
+ .regwidth = 16,
+ .init_seq = ili9225_init,
+ .init_seq_sz = ARRAY_SIZE(ili9225_init),
+ },
+ {
+ .name = "ili9225",
+ .width = 176,
+ .height = 220,
+ .regwidth = 16,
+ .init_seq = ili9225_init,
+ .init_seq_sz = ARRAY_SIZE(ili9225_init),
+ },
+ {
+ .name = "ili9225",
+ .width = 176,
+ .height = 220,
+ .regwidth = 16,
+ .init_seq = ili9225_init,
+ .init_seq_sz = ARRAY_SIZE(ili9225_init),
+ },
+ {
+ .name = "ili9320",
+ .width = 240,
+ .height = 320,
+ .setaddrwin = 1,
+ .regwidth = 16,
+ .init_seq = ili9320_init,
+ .init_seq_sz = ARRAY_SIZE(ili9320_init),
+ },
+ {
+ .name = "ili9325",
+ .width = 240,
+ .height = 320,
+ .setaddrwin = 1,
+ .regwidth = 16,
+ .init_seq = ili9325_init,
+ .init_seq_sz = ARRAY_SIZE(ili9325_init),
+ },
+ {
+ .name = "ili9341",
+ .width = 240,
+ .height = 320,
+ .init_seq = ili9341_init,
+ .init_seq_sz = ARRAY_SIZE(ili9341_init),
+ },
+ {
+ .name = "ssd1289",
+ .width = 240,
+ .height = 320,
+ .setaddrwin = 2,
+ .regwidth = 16,
+ .init_seq = ssd1289_init,
+ .init_seq_sz = ARRAY_SIZE(ssd1289_init),
+ },
+ {
+ .name = "ssd1351",
+ .width = 128,
+ .height = 128,
+ .setaddrwin = 3,
+ .init_seq = ssd1351_init,
+ .init_seq_sz = ARRAY_SIZE(ssd1351_init),
+ },
+};
+
/* ili9320, ili9325 */
static void flexfb_set_addr_win_1(struct fbtft_par *par,
int xs, int ys, int xe, int ye)
@@ -247,8 +350,38 @@ static int flexfb_verify_gpios_db(struct fbtft_par *par)
return 0;
}

+static void flexfb_chip_load_param(const struct flexfb_lcd_controller *chip)
+{
+ if (!width)
+ width = chip->width;
+ if (!height)
+ height = chip->height;
+ setaddrwin = chip->setaddrwin;
+ if (chip->regwidth)
+ regwidth = chip->regwidth;
+ if (!init_num) {
+ initp = chip->init_seq;
+ initp_num = chip->init_seq_sz;
+ }
+}
+
static struct fbtft_display flex_display = { };

+static int flexfb_chip_init(const struct device *dev)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(flexfb_chip_table); i++)
+ if (!strcmp(chip, flexfb_chip_table[i].name)) {
+ flexfb_chip_load_param(&flexfb_chip_table[i]);
+ return 0;
+ }
+
+ dev_err(dev, "chip=%s is not supported\n", chip);
+
+ return -EINVAL;
+}
+
static int flexfb_probe_common(struct spi_device *sdev,
struct platform_device *pdev)
{
@@ -269,102 +402,9 @@ static int flexfb_probe_common(struct spi_device *sdev,
sdev ? "'SPI device'" : "'Platform device'");

if (chip) {
-
- if (!strcmp(chip, "st7735r")) {
- if (!width)
- width = 128;
- if (!height)
- height = 160;
- if (init_num == 0) {
- initp = st7735r_init;
- initp_num = ARRAY_SIZE(st7735r_init);
- }
-
- } else if (!strcmp(chip, "hx8340bn")) {
- if (!width)
- width = 176;
- if (!height)
- height = 220;
- setaddrwin = 0;
- if (init_num == 0) {
- initp = hx8340bn_init;
- initp_num = ARRAY_SIZE(hx8340bn_init);
- }
-
- } else if (!strcmp(chip, "ili9225")) {
- if (!width)
- width = 176;
- if (!height)
- height = 220;
- setaddrwin = 0;
- regwidth = 16;
- if (init_num == 0) {
- initp = ili9225_init;
- initp_num = ARRAY_SIZE(ili9225_init);
- }
-
- } else if (!strcmp(chip, "ili9320")) {
- if (!width)
- width = 240;
- if (!height)
- height = 320;
- setaddrwin = 1;
- regwidth = 16;
- if (init_num == 0) {
- initp = ili9320_init;
- initp_num = ARRAY_SIZE(ili9320_init);
- }
-
- } else if (!strcmp(chip, "ili9325")) {
- if (!width)
- width = 240;
- if (!height)
- height = 320;
- setaddrwin = 1;
- regwidth = 16;
- if (init_num == 0) {
- initp = ili9325_init;
- initp_num = ARRAY_SIZE(ili9325_init);
- }
-
- } else if (!strcmp(chip, "ili9341")) {
- if (!width)
- width = 240;
- if (!height)
- height = 320;
- setaddrwin = 0;
- regwidth = 8;
- if (init_num == 0) {
- initp = ili9341_init;
- initp_num = ARRAY_SIZE(ili9341_init);
- }
-
- } else if (!strcmp(chip, "ssd1289")) {
- if (!width)
- width = 240;
- if (!height)
- height = 320;
- setaddrwin = 2;
- regwidth = 16;
- if (init_num == 0) {
- initp = ssd1289_init;
- initp_num = ARRAY_SIZE(ssd1289_init);
- }
-
- } else if (!strcmp(chip, "ssd1351")) {
- if (!width)
- width = 128;
- if (!height)
- height = 128;
- setaddrwin = 3;
- if (init_num == 0) {
- initp = ssd1351_init;
- initp_num = ARRAY_SIZE(ssd1351_init);
- }
- } else {
- dev_err(dev, "chip=%s is not supported\n", chip);
- return -EINVAL;
- }
+ ret = flexfb_chip_init(dev);
+ if (ret)
+ return ret;
}

if (width == 0 || height == 0) {
--
2.1.4