mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH 0/5] video: ssd1307fb: Add SPI support
@ 2021-12-17 18:22 Michael Tretter
  2021-12-17 18:22 ` [PATCH 1/5] video: ssd1307fb: pass par instead of i2c client to write Michael Tretter
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Michael Tretter @ 2021-12-17 18:22 UTC (permalink / raw)
  To: barebox

Hello,

The Solomon single-chip CMOS OLED/PLED driver with controller can be connected
to I2C or SPI. The driver already supports I2C. This series adds support for
SPI connected displays to the driver.

Unfortunately, the bindings for the SPI connected display are not documented.
This driver uses the (undocumented) solomon,ssd1306 compatible of the staging
driver in Linux, but uses properties defined for the solomon,ssd1306fb-i2c
compatible of the I2C driver.

While the driver allows to use SPI and I2C, which would be a use case for
regmap, the driver does not use regmap, because the controller does not
actually expose registers, but simply accepts commands or data. This does not
match the regmap API. Therefore, the driver uses its own abstraction for the
bus.

The updated driver also allows to disable either SPI or I2C and uses #if
statements in the driver code, which I don't really like. I considered it
better than making the driver dependent on SPI and I2C, but if there is a
better way to handle this either/or dependency, I will gladly update the
driver accordingly.

Patches 1-4 refactor the driver to have fewer locations that refer to I2C to
simplify disabling the I2C support.

Patch 5 actually adds the SPI support and makes I2C optional.

Michael

Michael Tretter (5):
  video: ssd1307fb: pass par instead of i2c client to write
  video: ssd1307fb: don't use i2c client for logging
  video: ssd1307fb: move i2c setup to single place
  video: ssd1307fb: use function pointer for write
  video: ssd1307fb: add spi support

 drivers/video/Kconfig     |   2 +-
 drivers/video/ssd1307fb.c | 180 ++++++++++++++++++++++++++------------
 2 files changed, 125 insertions(+), 57 deletions(-)

-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/5] video: ssd1307fb: pass par instead of i2c client to write
  2021-12-17 18:22 [PATCH 0/5] video: ssd1307fb: Add SPI support Michael Tretter
@ 2021-12-17 18:22 ` Michael Tretter
  2021-12-17 18:22 ` [PATCH 2/5] video: ssd1307fb: don't use i2c client for logging Michael Tretter
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Michael Tretter @ 2021-12-17 18:22 UTC (permalink / raw)
  To: barebox

By pushing the dependency to i2c down into the write function, the
remaining driver is less dependent on i2c. This allows to delay the
decision to use i2c until the actual bus write.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/video/ssd1307fb.c | 69 ++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 34 deletions(-)

diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
index 5d160ddf338a..61d0e083a3f7 100644
--- a/drivers/video/ssd1307fb.c
+++ b/drivers/video/ssd1307fb.c
@@ -93,9 +93,10 @@ static struct ssd1307fb_array *ssd1307fb_alloc_array(u32 len, u8 type)
 	return array;
 }
 
-static int ssd1307fb_write_array(struct i2c_client *client,
+static int ssd1307fb_write_array(struct ssd1307fb_par *par,
 				 struct ssd1307fb_array *array, u32 len)
 {
+	struct i2c_client *client = par->client;
 	int ret;
 
 	len += sizeof(struct ssd1307fb_array);
@@ -109,7 +110,7 @@ static int ssd1307fb_write_array(struct i2c_client *client,
 	return 0;
 }
 
-static inline int ssd1307fb_write_cmd(struct i2c_client *client, u8 cmd)
+static inline int ssd1307fb_write_cmd(struct ssd1307fb_par *par, u8 cmd)
 {
 	struct ssd1307fb_array *array;
 	int ret;
@@ -120,7 +121,7 @@ static inline int ssd1307fb_write_cmd(struct i2c_client *client, u8 cmd)
 
 	array->data[0] = cmd;
 
-	ret = ssd1307fb_write_array(client, array, 1);
+	ret = ssd1307fb_write_array(par, array, 1);
 	kfree(array);
 
 	return ret;
@@ -181,20 +182,20 @@ static void ssd1307fb_update_display(struct ssd1307fb_par *par)
 		}
 	}
 
-	ssd1307fb_write_array(par->client, array, par->width * par->height / 8);
+	ssd1307fb_write_array(par, array, par->width * par->height / 8);
 	kfree(array);
 }
 
 static void ssd1307fb_enable(struct fb_info *info)
 {
 	struct ssd1307fb_par *par = info->priv;
-	ssd1307fb_write_cmd(par->client, SSD1307FB_DISPLAY_ON);
+	ssd1307fb_write_cmd(par, SSD1307FB_DISPLAY_ON);
 }
 
 static void ssd1307fb_disable(struct fb_info *info)
 {
 	struct ssd1307fb_par *par = info->priv;
-	ssd1307fb_write_cmd(par->client, SSD1307FB_DISPLAY_OFF);
+	ssd1307fb_write_cmd(par, SSD1307FB_DISPLAY_OFF);
 }
 
 static void ssd1307fb_flush(struct fb_info *info)
@@ -215,134 +216,134 @@ static int ssd1307fb_init(struct ssd1307fb_par *par)
 	u32 precharge, dclk, com_invdir, compins;
 
 	/* Set initial contrast */
-	ret = ssd1307fb_write_cmd(par->client, SSD1307FB_CONTRAST);
+	ret = ssd1307fb_write_cmd(par, SSD1307FB_CONTRAST);
 	if (ret < 0)
 		return ret;
 
-	ret = ssd1307fb_write_cmd(par->client, par->contrast);
+	ret = ssd1307fb_write_cmd(par, par->contrast);
 	if (ret < 0)
 		return ret;
 
 	/* Set segment re-map */
 	if (par->seg_remap) {
-		ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SEG_REMAP_ON);
+		ret = ssd1307fb_write_cmd(par, SSD1307FB_SEG_REMAP_ON);
 		if (ret < 0)
 			return ret;
 	};
 
 	/* Set COM direction */
 	com_invdir = 0xc0 | (par->com_invdir & 0x1) << 3;
-	ret = ssd1307fb_write_cmd(par->client,  com_invdir);
+	ret = ssd1307fb_write_cmd(par,  com_invdir);
 	if (ret < 0)
 		return ret;
 
 	/* Set multiplex ratio value */
-	ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_MULTIPLEX_RATIO);
+	ret = ssd1307fb_write_cmd(par, SSD1307FB_SET_MULTIPLEX_RATIO);
 	if (ret < 0)
 		return ret;
 
-	ret = ssd1307fb_write_cmd(par->client, par->height - 1);
+	ret = ssd1307fb_write_cmd(par, par->height - 1);
 	if (ret < 0)
 		return ret;
 
 	/* set display offset value */
-	ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_DISPLAY_OFFSET);
+	ret = ssd1307fb_write_cmd(par, SSD1307FB_SET_DISPLAY_OFFSET);
 	if (ret < 0)
 		return ret;
 
-	ret = ssd1307fb_write_cmd(par->client, par->com_offset);
+	ret = ssd1307fb_write_cmd(par, par->com_offset);
 	if (ret < 0)
 		return ret;
 
 	/* Set clock frequency */
-	ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_CLOCK_FREQ);
+	ret = ssd1307fb_write_cmd(par, SSD1307FB_SET_CLOCK_FREQ);
 	if (ret < 0)
 		return ret;
 
 	dclk = ((par->dclk_div - 1) & 0xf) | (par->dclk_frq & 0xf) << 4;
-	ret = ssd1307fb_write_cmd(par->client, dclk);
+	ret = ssd1307fb_write_cmd(par, dclk);
 	if (ret < 0)
 		return ret;
 
 	/* Set precharge period in number of ticks from the internal clock */
-	ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_PRECHARGE_PERIOD);
+	ret = ssd1307fb_write_cmd(par, SSD1307FB_SET_PRECHARGE_PERIOD);
 	if (ret < 0)
 		return ret;
 
 	precharge = (par->prechargep1 & 0xf) | (par->prechargep2 & 0xf) << 4;
-	ret = ssd1307fb_write_cmd(par->client, precharge);
+	ret = ssd1307fb_write_cmd(par, precharge);
 	if (ret < 0)
 		return ret;
 
 	/* Set COM pins configuration */
-	ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_COM_PINS_CONFIG);
+	ret = ssd1307fb_write_cmd(par, SSD1307FB_SET_COM_PINS_CONFIG);
 	if (ret < 0)
 		return ret;
 
 	compins = 0x02 | !(par->com_seq & 0x1) << 4
 				   | (par->com_lrremap & 0x1) << 5;
-	ret = ssd1307fb_write_cmd(par->client, compins);
+	ret = ssd1307fb_write_cmd(par, compins);
 	if (ret < 0)
 		return ret;
 
 	/* Set VCOMH */
-	ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_VCOMH);
+	ret = ssd1307fb_write_cmd(par, SSD1307FB_SET_VCOMH);
 	if (ret < 0)
 		return ret;
 
-	ret = ssd1307fb_write_cmd(par->client, par->vcomh);
+	ret = ssd1307fb_write_cmd(par, par->vcomh);
 	if (ret < 0)
 		return ret;
 
 	/* Turn on the DC-DC Charge Pump */
-	ret = ssd1307fb_write_cmd(par->client, SSD1307FB_CHARGE_PUMP);
+	ret = ssd1307fb_write_cmd(par, SSD1307FB_CHARGE_PUMP);
 	if (ret < 0)
 		return ret;
 
-	ret = ssd1307fb_write_cmd(par->client,
+	ret = ssd1307fb_write_cmd(par,
 		BIT(4) | (par->device_info->need_chargepump ? BIT(2) : 0));
 	if (ret < 0)
 		return ret;
 
 	/* Switch to horizontal addressing mode */
-	ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_ADDRESS_MODE);
+	ret = ssd1307fb_write_cmd(par, SSD1307FB_SET_ADDRESS_MODE);
 	if (ret < 0)
 		return ret;
 
-	ret = ssd1307fb_write_cmd(par->client,
+	ret = ssd1307fb_write_cmd(par,
 				  SSD1307FB_SET_ADDRESS_MODE_HORIZONTAL);
 	if (ret < 0)
 		return ret;
 
 	/* Set column range */
-	ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_COL_RANGE);
+	ret = ssd1307fb_write_cmd(par, SSD1307FB_SET_COL_RANGE);
 	if (ret < 0)
 		return ret;
 
-	ret = ssd1307fb_write_cmd(par->client, 0x0);
+	ret = ssd1307fb_write_cmd(par, 0x0);
 	if (ret < 0)
 		return ret;
 
-	ret = ssd1307fb_write_cmd(par->client, par->width - 1);
+	ret = ssd1307fb_write_cmd(par, par->width - 1);
 	if (ret < 0)
 		return ret;
 
 	/* Set page range */
-	ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_PAGE_RANGE);
+	ret = ssd1307fb_write_cmd(par, SSD1307FB_SET_PAGE_RANGE);
 	if (ret < 0)
 		return ret;
 
-	ret = ssd1307fb_write_cmd(par->client, 0x0);
+	ret = ssd1307fb_write_cmd(par, 0x0);
 	if (ret < 0)
 		return ret;
 
-	ret = ssd1307fb_write_cmd(par->client,
+	ret = ssd1307fb_write_cmd(par,
 				  par->page_offset + (par->height / 8) - 1);
 	if (ret < 0)
 		return ret;
 
 	/* Turn on the display */
-	ret = ssd1307fb_write_cmd(par->client, SSD1307FB_DISPLAY_ON);
+	ret = ssd1307fb_write_cmd(par, SSD1307FB_DISPLAY_ON);
 	if (ret < 0)
 		return ret;
 
@@ -564,7 +565,7 @@ static int ssd1307fb_probe(struct device_d *dev)
 		}
 	}
 
-	ssd1307fb_write_array(par->client, array, par->width * par->height / 8);
+	ssd1307fb_write_array(par, array, par->width * par->height / 8);
 	kfree(array);
 
 	dev_info(&client->dev,
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/5] video: ssd1307fb: don't use i2c client for logging
  2021-12-17 18:22 [PATCH 0/5] video: ssd1307fb: Add SPI support Michael Tretter
  2021-12-17 18:22 ` [PATCH 1/5] video: ssd1307fb: pass par instead of i2c client to write Michael Tretter
@ 2021-12-17 18:22 ` Michael Tretter
  2021-12-17 18:22 ` [PATCH 3/5] video: ssd1307fb: move i2c setup to single place Michael Tretter
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Michael Tretter @ 2021-12-17 18:22 UTC (permalink / raw)
  To: barebox

We can use the device directly and don't have to use the device that is
attached to the I2C client. This reduces the dependency on i2c.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/video/ssd1307fb.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
index 61d0e083a3f7..88c88e3253cf 100644
--- a/drivers/video/ssd1307fb.c
+++ b/drivers/video/ssd1307fb.c
@@ -401,7 +401,7 @@ static int ssd1307fb_probe(struct device_d *dev)
 	int i, j;
 
 	if (!node) {
-		dev_err(&client->dev, "No device tree data found!\n");
+		dev_err(dev, "No device tree data found!\n");
 		return -EINVAL;
 	}
 
@@ -421,22 +421,22 @@ static int ssd1307fb_probe(struct device_d *dev)
 		goto fb_alloc_error;
 	}
 
-	par->vbat = regulator_get(&client->dev, "vbat");
+	par->vbat = regulator_get(dev, "vbat");
 	if (IS_ERR(par->vbat)) {
-		dev_info(&client->dev, "Will not use VBAT");
+		dev_info(dev, "Will not use VBAT");
 		par->vbat = NULL;
 	}
 
 	ret = of_property_read_u32(node, "solomon,width", &par->width);
 	if (ret) {
-		dev_err(&client->dev,
+		dev_err(dev,
 			"Couldn't find 'solomon,width' in device tree.\n");
 		goto panel_init_error;
 	}
 
 	ret = of_property_read_u32(node, "solomon,height", &par->height);
 	if (ret) {
-		dev_err(&client->dev,
+		dev_err(dev,
 			"Couldn't find 'solomon,height' in device tree.\n");
 		goto panel_init_error;
 	}
@@ -444,7 +444,7 @@ static int ssd1307fb_probe(struct device_d *dev)
 	ret = of_property_read_u32(node, "solomon,page-offset",
 				   &par->page_offset);
 	if (ret) {
-		dev_err(&client->dev,
+		dev_err(dev,
 			"Couldn't find 'solomon,page_offset' in device tree.\n");
 		goto panel_init_error;
 	}
@@ -477,7 +477,7 @@ static int ssd1307fb_probe(struct device_d *dev)
 
 	vmem = malloc(vmem_size);
 	if (!vmem) {
-		dev_err(&client->dev, "Couldn't allocate graphical memory.\n");
+		dev_err(dev, "Couldn't allocate graphical memory.\n");
 		ret = -ENOMEM;
 		goto fb_alloc_error;
 	}
@@ -508,7 +508,7 @@ static int ssd1307fb_probe(struct device_d *dev)
 
 		ret = gpio_request_one(par->reset, flags, "oled-reset");
 		if (ret) {
-			dev_err(&client->dev,
+			dev_err(dev,
 				"failed to request gpio %d: %d\n",
 				par->reset, ret);
 			goto reset_oled_error;
@@ -546,7 +546,7 @@ static int ssd1307fb_probe(struct device_d *dev)
 	info->dev.parent = dev;
 	ret = register_framebuffer(info);
 	if (ret) {
-		dev_err(&client->dev, "Couldn't register the framebuffer\n");
+		dev_err(dev, "Couldn't register the framebuffer\n");
 		goto panel_init_error;
 	}
 
@@ -568,7 +568,7 @@ static int ssd1307fb_probe(struct device_d *dev)
 	ssd1307fb_write_array(par, array, par->width * par->height / 8);
 	kfree(array);
 
-	dev_info(&client->dev,
+	dev_info(dev,
 		 "ssd1307 framebuffer device registered, using %d bytes of video memory\n",
 		 vmem_size);
 
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 3/5] video: ssd1307fb: move i2c setup to single place
  2021-12-17 18:22 [PATCH 0/5] video: ssd1307fb: Add SPI support Michael Tretter
  2021-12-17 18:22 ` [PATCH 1/5] video: ssd1307fb: pass par instead of i2c client to write Michael Tretter
  2021-12-17 18:22 ` [PATCH 2/5] video: ssd1307fb: don't use i2c client for logging Michael Tretter
@ 2021-12-17 18:22 ` Michael Tretter
  2021-12-17 18:22 ` [PATCH 4/5] video: ssd1307fb: use function pointer for write Michael Tretter
  2021-12-17 18:23 ` [PATCH 5/5] video: ssd1307fb: add spi support Michael Tretter
  4 siblings, 0 replies; 9+ messages in thread
From: Michael Tretter @ 2021-12-17 18:22 UTC (permalink / raw)
  To: barebox

By having the entire i2c dependent initialzation in a single place, it
is easier to make it optional later.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/video/ssd1307fb.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
index 88c88e3253cf..1538a1b640a3 100644
--- a/drivers/video/ssd1307fb.c
+++ b/drivers/video/ssd1307fb.c
@@ -387,7 +387,6 @@ static const struct of_device_id ssd1307fb_of_match[] = {
 
 static int ssd1307fb_probe(struct device_d *dev)
 {
-	struct i2c_client *client = to_i2c_client(dev);
 	struct fb_info *info;
 	struct device_node *node = dev->device_node;
 	const struct of_device_id *match =
@@ -410,10 +409,12 @@ static int ssd1307fb_probe(struct device_d *dev)
 
 	info->priv = par;
 	par->info = info;
-	par->client = client;
 
 	par->device_info = (struct ssd1307fb_deviceinfo *)match->data;
 
+	par->client = to_i2c_client(dev);
+	i2c_set_clientdata(par->client, par);
+
 	par->reset = of_get_named_gpio_flags(node,
 					 "reset-gpios", 0, &of_flags);
 	if (!gpio_is_valid(par->reset) && par->reset == -EPROBE_DEFER) {
@@ -519,8 +520,6 @@ static int ssd1307fb_probe(struct device_d *dev)
 	if (ret < 0)
 		goto reset_oled_error;
 
-	i2c_set_clientdata(client, info);
-
 	if (par->reset > 0) {
 		/* Reset the screen */
 		gpio_set_active(par->reset, 1);
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 4/5] video: ssd1307fb: use function pointer for write
  2021-12-17 18:22 [PATCH 0/5] video: ssd1307fb: Add SPI support Michael Tretter
                   ` (2 preceding siblings ...)
  2021-12-17 18:22 ` [PATCH 3/5] video: ssd1307fb: move i2c setup to single place Michael Tretter
@ 2021-12-17 18:22 ` Michael Tretter
  2021-12-17 18:23 ` [PATCH 5/5] video: ssd1307fb: add spi support Michael Tretter
  4 siblings, 0 replies; 9+ messages in thread
From: Michael Tretter @ 2021-12-17 18:22 UTC (permalink / raw)
  To: barebox

The function pointer is an abstraction the I2C accesses to be able to
add other bus protocols underneath the driver.

The functionality kind of reminds of regmap, but the driver does only
write data and does not actually use registers. Therefore, using regmap
with the register abstraction is not appropriate.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/video/ssd1307fb.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
index 1538a1b640a3..d0df073b8ef2 100644
--- a/drivers/video/ssd1307fb.c
+++ b/drivers/video/ssd1307fb.c
@@ -53,6 +53,16 @@ struct ssd1307fb_deviceinfo {
 	int need_chargepump;
 };
 
+struct ssd1307fb_array {
+	u8 type;
+	u8 data[0];
+};
+
+struct ssd1307fb_par;
+
+typedef int (*ssd1307fb_write_array)(struct ssd1307fb_par *par,
+				     struct ssd1307fb_array *array, u32 len);
+
 struct ssd1307fb_par {
 	u32 com_invdir;
 	u32 com_lrremap;
@@ -73,11 +83,8 @@ struct ssd1307fb_par {
 	u32 seg_remap;
 	u32 vcomh;
 	u32 width;
-};
 
-struct ssd1307fb_array {
-	u8 type;
-	u8 data[0];
+	ssd1307fb_write_array write_array;
 };
 
 static struct ssd1307fb_array *ssd1307fb_alloc_array(u32 len, u8 type)
@@ -93,8 +100,8 @@ static struct ssd1307fb_array *ssd1307fb_alloc_array(u32 len, u8 type)
 	return array;
 }
 
-static int ssd1307fb_write_array(struct ssd1307fb_par *par,
-				 struct ssd1307fb_array *array, u32 len)
+static int ssd1307fb_i2c_write_array(struct ssd1307fb_par *par,
+				     struct ssd1307fb_array *array, u32 len)
 {
 	struct i2c_client *client = par->client;
 	int ret;
@@ -121,7 +128,7 @@ static inline int ssd1307fb_write_cmd(struct ssd1307fb_par *par, u8 cmd)
 
 	array->data[0] = cmd;
 
-	ret = ssd1307fb_write_array(par, array, 1);
+	ret = par->write_array(par, array, 1);
 	kfree(array);
 
 	return ret;
@@ -182,7 +189,7 @@ static void ssd1307fb_update_display(struct ssd1307fb_par *par)
 		}
 	}
 
-	ssd1307fb_write_array(par, array, par->width * par->height / 8);
+	par->write_array(par, array, par->width * par->height / 8);
 	kfree(array);
 }
 
@@ -414,6 +421,7 @@ static int ssd1307fb_probe(struct device_d *dev)
 
 	par->client = to_i2c_client(dev);
 	i2c_set_clientdata(par->client, par);
+	par->write_array = ssd1307fb_i2c_write_array;
 
 	par->reset = of_get_named_gpio_flags(node,
 					 "reset-gpios", 0, &of_flags);
@@ -564,7 +572,7 @@ static int ssd1307fb_probe(struct device_d *dev)
 		}
 	}
 
-	ssd1307fb_write_array(par, array, par->width * par->height / 8);
+	par->write_array(par, array, par->width * par->height / 8);
 	kfree(array);
 
 	dev_info(dev,
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 5/5] video: ssd1307fb: add spi support
  2021-12-17 18:22 [PATCH 0/5] video: ssd1307fb: Add SPI support Michael Tretter
                   ` (3 preceding siblings ...)
  2021-12-17 18:22 ` [PATCH 4/5] video: ssd1307fb: use function pointer for write Michael Tretter
@ 2021-12-17 18:23 ` Michael Tretter
  2021-12-17 19:00   ` Ahmad Fatoum
  4 siblings, 1 reply; 9+ messages in thread
From: Michael Tretter @ 2021-12-17 18:23 UTC (permalink / raw)
  To: barebox

The Solomon display drivers also support SPI in addition to the I2C.
Add SPI support to the driver that already supports I2C by implementing
the bus write function for SPI and registering an SPI driver.

While the driver needs I2C or SPI, either subsystem is optional as long
as one is enabled.

WARNING: The device tree bindings for the ssd1306 with SPI are not
documented!

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/video/Kconfig     |  2 +-
 drivers/video/ssd1307fb.c | 72 +++++++++++++++++++++++++++++++++++----
 2 files changed, 67 insertions(+), 7 deletions(-)

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index a87e8c063899..cfbd541a956e 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -15,7 +15,7 @@ config FRAMEBUFFER_CONSOLE
 
 config DRIVER_VIDEO_FB_SSD1307
 	bool "Solomon SSD1307 framebuffer support"
-	depends on I2C && GPIOLIB
+	depends on (I2C || SPI) && GPIOLIB
 
 config VIDEO_VPL
 	depends on OFTREE
diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
index d0df073b8ef2..2939d4348405 100644
--- a/drivers/video/ssd1307fb.c
+++ b/drivers/video/ssd1307fb.c
@@ -23,6 +23,7 @@
 #include <gpio.h>
 #include <of_gpio.h>
 #include <regulator.h>
+#include <spi/spi.h>
 
 #define SSD1307FB_DATA                          0x40
 #define SSD1307FB_COMMAND                       0x80
@@ -73,12 +74,14 @@ struct ssd1307fb_par {
 	u32 dclk_frq;
 	const struct ssd1307fb_deviceinfo *device_info;
 	struct i2c_client *client;
+	struct spi_device *spi;
 	u32 height;
 	struct fb_info *info;
 	u32 page_offset;
 	u32 prechargep1;
 	u32 prechargep2;
 	int reset;
+	int dc;
 	struct regulator *vbat;
 	u32 seg_remap;
 	u32 vcomh;
@@ -100,6 +103,30 @@ static struct ssd1307fb_array *ssd1307fb_alloc_array(u32 len, u8 type)
 	return array;
 }
 
+#if IS_ENABLED(CONFIG_SPI)
+static int ssd1307fb_spi_write_array(struct ssd1307fb_par *par,
+				     struct ssd1307fb_array *array, u32 len)
+{
+	struct spi_device *spi = par->spi;
+	int ret;
+
+	if (array->type == SSD1307FB_COMMAND)
+		gpio_direction_output(par->dc, 0);
+	else
+		gpio_direction_output(par->dc, 1);
+
+	ret = spi_write(spi, array->data, len);
+	if (ret)
+		dev_err(&spi->dev, "Couldn't send SPI command.\n");
+
+	/* Ensure that we remain in data mode. */
+	gpio_direction_output(par->dc, 1);
+
+	return ret;
+}
+#endif
+
+#if IS_ENABLED(CONFIG_I2C)
 static int ssd1307fb_i2c_write_array(struct ssd1307fb_par *par,
 				     struct ssd1307fb_array *array, u32 len)
 {
@@ -116,6 +143,7 @@ static int ssd1307fb_i2c_write_array(struct ssd1307fb_par *par,
 
 	return 0;
 }
+#endif
 
 static inline int ssd1307fb_write_cmd(struct ssd1307fb_par *par, u8 cmd)
 {
@@ -385,6 +413,10 @@ static const struct of_device_id ssd1307fb_of_match[] = {
 		.compatible = "solomon,ssd1306fb-i2c",
 		.data = (void *)&ssd1307fb_ssd1306_deviceinfo,
 	},
+	{
+		.compatible = "solomon,ssd1306",
+		.data = (void *)&ssd1307fb_ssd1306_deviceinfo,
+	},
 	{
 		.compatible = "solomon,ssd1309fb-i2c",
 		.data = (void *)&ssd1307fb_ssd1309_deviceinfo,
@@ -419,9 +451,24 @@ static int ssd1307fb_probe(struct device_d *dev)
 
 	par->device_info = (struct ssd1307fb_deviceinfo *)match->data;
 
-	par->client = to_i2c_client(dev);
-	i2c_set_clientdata(par->client, par);
-	par->write_array = ssd1307fb_i2c_write_array;
+#if IS_ENABLED(CONFIG_I2C)
+	if (dev->bus == &i2c_bus) {
+		par->client = to_i2c_client(dev);
+		i2c_set_clientdata(par->client, par);
+		par->write_array = ssd1307fb_i2c_write_array;
+	}
+#endif
+#if IS_ENABLED(CONFIG_SPI)
+	if (dev->bus == &spi_bus) {
+		par->spi = (struct spi_device *)dev->type_data;
+		par->dc = of_get_named_gpio(node, "dc-gpios", 0);
+		if (!gpio_is_valid(par->dc)) {
+			ret = par->dc;
+			goto fb_alloc_error;
+		}
+		par->write_array = ssd1307fb_spi_write_array;
+	}
+#endif
 
 	par->reset = of_get_named_gpio_flags(node,
 					 "reset-gpios", 0, &of_flags);
@@ -591,9 +638,22 @@ fb_alloc_error:
 	return ret;
 }
 
-static struct driver_d ssd1307fb_driver = {
-	.name = "ssd1307fb",
+static __maybe_unused struct driver_d ssd1307fb_i2c_driver = {
+	.name = "ssd1307fb-i2c",
 	.probe = ssd1307fb_probe,
 	.of_compatible = DRV_OF_COMPAT(ssd1307fb_of_match),
 };
-device_i2c_driver(ssd1307fb_driver);
+
+#if IS_ENABLED(CONFIG_I2C)
+device_i2c_driver(ssd1307fb_i2c_driver);
+#endif
+
+static __maybe_unused struct driver_d ssd1307fb_spi_driver = {
+	.name = "ssd1307fb-spi",
+	.probe = ssd1307fb_probe,
+	.of_compatible = DRV_OF_COMPAT(ssd1307fb_of_match),
+};
+
+#if IS_ENABLED(CONFIG_SPI)
+device_spi_driver(ssd1307fb_spi_driver);
+#endif
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 5/5] video: ssd1307fb: add spi support
  2021-12-17 18:23 ` [PATCH 5/5] video: ssd1307fb: add spi support Michael Tretter
@ 2021-12-17 19:00   ` Ahmad Fatoum
  2021-12-17 21:13     ` Michael Tretter
  0 siblings, 1 reply; 9+ messages in thread
From: Ahmad Fatoum @ 2021-12-17 19:00 UTC (permalink / raw)
  To: Michael Tretter, barebox

On 17.12.21 19:23, Michael Tretter wrote:
> The Solomon display drivers also support SPI in addition to the I2C.
> Add SPI support to the driver that already supports I2C by implementing
> the bus write function for SPI and registering an SPI driver.
> 
> While the driver needs I2C or SPI, either subsystem is optional as long
> as one is enabled.
> 
> WARNING: The device tree bindings for the ssd1306 with SPI are not
> documented!
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/video/Kconfig     |  2 +-
>  drivers/video/ssd1307fb.c | 72 +++++++++++++++++++++++++++++++++++----
>  2 files changed, 67 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index a87e8c063899..cfbd541a956e 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -15,7 +15,7 @@ config FRAMEBUFFER_CONSOLE
>  
>  config DRIVER_VIDEO_FB_SSD1307
>  	bool "Solomon SSD1307 framebuffer support"
> -	depends on I2C && GPIOLIB
> +	depends on (I2C || SPI) && GPIOLIB
>  
>  config VIDEO_VPL
>  	depends on OFTREE
> diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
> index d0df073b8ef2..2939d4348405 100644
> --- a/drivers/video/ssd1307fb.c
> +++ b/drivers/video/ssd1307fb.c
> @@ -23,6 +23,7 @@
>  #include <gpio.h>
>  #include <of_gpio.h>
>  #include <regulator.h>
> +#include <spi/spi.h>
>  
>  #define SSD1307FB_DATA                          0x40
>  #define SSD1307FB_COMMAND                       0x80
> @@ -73,12 +74,14 @@ struct ssd1307fb_par {
>  	u32 dclk_frq;
>  	const struct ssd1307fb_deviceinfo *device_info;
>  	struct i2c_client *client;
> +	struct spi_device *spi;
>  	u32 height;
>  	struct fb_info *info;
>  	u32 page_offset;
>  	u32 prechargep1;
>  	u32 prechargep2;
>  	int reset;
> +	int dc;
>  	struct regulator *vbat;
>  	u32 seg_remap;
>  	u32 vcomh;
> @@ -100,6 +103,30 @@ static struct ssd1307fb_array *ssd1307fb_alloc_array(u32 len, u8 type)
>  	return array;
>  }
>  
> +#if IS_ENABLED(CONFIG_SPI)
> +static int ssd1307fb_spi_write_array(struct ssd1307fb_par *par,
> +				     struct ssd1307fb_array *array, u32 len)

__maybe_unused and drop the #if?

> +{
> +	struct spi_device *spi = par->spi;
> +	int ret;
> +
> +	if (array->type == SSD1307FB_COMMAND)
> +		gpio_direction_output(par->dc, 0);
> +	else
> +		gpio_direction_output(par->dc, 1);
> +
> +	ret = spi_write(spi, array->data, len);
> +	if (ret)
> +		dev_err(&spi->dev, "Couldn't send SPI command.\n");
> +
> +	/* Ensure that we remain in data mode. */
> +	gpio_direction_output(par->dc, 1);
> +
> +	return ret;
> +}
> +#endif
> +
> +#if IS_ENABLED(CONFIG_I2C)

Ditto

>  static int ssd1307fb_i2c_write_array(struct ssd1307fb_par *par,
>  				     struct ssd1307fb_array *array, u32 len)
>  {
> @@ -116,6 +143,7 @@ static int ssd1307fb_i2c_write_array(struct ssd1307fb_par *par,
>  
>  	return 0;
>  }
> +#endif
>  
>  static inline int ssd1307fb_write_cmd(struct ssd1307fb_par *par, u8 cmd)
>  {
> @@ -385,6 +413,10 @@ static const struct of_device_id ssd1307fb_of_match[] = {
>  		.compatible = "solomon,ssd1306fb-i2c",
>  		.data = (void *)&ssd1307fb_ssd1306_deviceinfo,
>  	},
> +	{
> +		.compatible = "solomon,ssd1306",
> +		.data = (void *)&ssd1307fb_ssd1306_deviceinfo,
> +	},
>  	{
>  		.compatible = "solomon,ssd1309fb-i2c",
>  		.data = (void *)&ssd1307fb_ssd1309_deviceinfo,
> @@ -419,9 +451,24 @@ static int ssd1307fb_probe(struct device_d *dev)
>  
>  	par->device_info = (struct ssd1307fb_deviceinfo *)match->data;
>  
> -	par->client = to_i2c_client(dev);
> -	i2c_set_clientdata(par->client, par);
> -	par->write_array = ssd1307fb_i2c_write_array;
> +#if IS_ENABLED(CONFIG_I2C)
> +	if (dev->bus == &i2c_bus) {

if (IS_ENABLED(CONFIG_I2C) && dev->bus == &i2c_bus) {

> +		par->client = to_i2c_client(dev);
> +		i2c_set_clientdata(par->client, par);
> +		par->write_array = ssd1307fb_i2c_write_array;
> +	}
> +#endif
> +#if IS_ENABLED(CONFIG_SPI)
> +	if (dev->bus == &spi_bus) {

Ditto

> +		par->spi = (struct spi_device *)dev->type_data;
> +		par->dc = of_get_named_gpio(node, "dc-gpios", 0);
> +		if (!gpio_is_valid(par->dc)) {
> +			ret = par->dc;
> +			goto fb_alloc_error;
> +		}
> +		par->write_array = ssd1307fb_spi_write_array;
> +	}
> +#endif
>  
>  	par->reset = of_get_named_gpio_flags(node,
>  					 "reset-gpios", 0, &of_flags);
> @@ -591,9 +638,22 @@ fb_alloc_error:
>  	return ret;
>  }
>  
> -static struct driver_d ssd1307fb_driver = {
> -	.name = "ssd1307fb",
> +static __maybe_unused struct driver_d ssd1307fb_i2c_driver = {
> +	.name = "ssd1307fb-i2c",
>  	.probe = ssd1307fb_probe,
>  	.of_compatible = DRV_OF_COMPAT(ssd1307fb_of_match),
>  };
> -device_i2c_driver(ssd1307fb_driver);
> +
> +#if IS_ENABLED(CONFIG_I2C)
> +device_i2c_driver(ssd1307fb_i2c_driver);
> +#endif

I think you should add !CONFIG_I2C stubs to i2c/i2c.h

> +
> +static __maybe_unused struct driver_d ssd1307fb_spi_driver = {
> +	.name = "ssd1307fb-spi",
> +	.probe = ssd1307fb_probe,
> +	.of_compatible = DRV_OF_COMPAT(ssd1307fb_of_match),
> +};
> +
> +#if IS_ENABLED(CONFIG_SPI)
> +device_spi_driver(ssd1307fb_spi_driver);
> +#endif

Ditto.

> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 5/5] video: ssd1307fb: add spi support
  2021-12-17 19:00   ` Ahmad Fatoum
@ 2021-12-17 21:13     ` Michael Tretter
  2021-12-18  6:20       ` Ahmad Fatoum
  0 siblings, 1 reply; 9+ messages in thread
From: Michael Tretter @ 2021-12-17 21:13 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox

On Fri, 17 Dec 2021 20:00:16 +0100, Ahmad Fatoum wrote:
> On 17.12.21 19:23, Michael Tretter wrote:
> > The Solomon display drivers also support SPI in addition to the I2C.
> > Add SPI support to the driver that already supports I2C by implementing
> > the bus write function for SPI and registering an SPI driver.
> > 
> > While the driver needs I2C or SPI, either subsystem is optional as long
> > as one is enabled.
> > 
> > WARNING: The device tree bindings for the ssd1306 with SPI are not
> > documented!
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  drivers/video/Kconfig     |  2 +-
> >  drivers/video/ssd1307fb.c | 72 +++++++++++++++++++++++++++++++++++----
> >  2 files changed, 67 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > index a87e8c063899..cfbd541a956e 100644
> > --- a/drivers/video/Kconfig
> > +++ b/drivers/video/Kconfig
> > @@ -15,7 +15,7 @@ config FRAMEBUFFER_CONSOLE
> >  
> >  config DRIVER_VIDEO_FB_SSD1307
> >  	bool "Solomon SSD1307 framebuffer support"
> > -	depends on I2C && GPIOLIB
> > +	depends on (I2C || SPI) && GPIOLIB
> >  
> >  config VIDEO_VPL
> >  	depends on OFTREE
> > diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
> > index d0df073b8ef2..2939d4348405 100644
> > --- a/drivers/video/ssd1307fb.c
> > +++ b/drivers/video/ssd1307fb.c
> > @@ -23,6 +23,7 @@
> >  #include <gpio.h>
> >  #include <of_gpio.h>
> >  #include <regulator.h>
> > +#include <spi/spi.h>
> >  
> >  #define SSD1307FB_DATA                          0x40
> >  #define SSD1307FB_COMMAND                       0x80
> > @@ -73,12 +74,14 @@ struct ssd1307fb_par {
> >  	u32 dclk_frq;
> >  	const struct ssd1307fb_deviceinfo *device_info;
> >  	struct i2c_client *client;
> > +	struct spi_device *spi;
> >  	u32 height;
> >  	struct fb_info *info;
> >  	u32 page_offset;
> >  	u32 prechargep1;
> >  	u32 prechargep2;
> >  	int reset;
> > +	int dc;
> >  	struct regulator *vbat;
> >  	u32 seg_remap;
> >  	u32 vcomh;
> > @@ -100,6 +103,30 @@ static struct ssd1307fb_array *ssd1307fb_alloc_array(u32 len, u8 type)
> >  	return array;
> >  }
> >  
> > +#if IS_ENABLED(CONFIG_SPI)
> > +static int ssd1307fb_spi_write_array(struct ssd1307fb_par *par,
> > +				     struct ssd1307fb_array *array, u32 len)
> 
> __maybe_unused and drop the #if?

Why doesn't this fail to link? The function uses spi_write, which is static
inline, but spi_write uses spi_sync, which is only defined in spi.c and is not
stubbed if !CONFIG_SPI. I would have expected to not be able to use spi_write
if CONFIG_SPI is disabled. Is there some dead code elimination happening
before linking?

Michael

> 
> > +{
> > +	struct spi_device *spi = par->spi;
> > +	int ret;
> > +
> > +	if (array->type == SSD1307FB_COMMAND)
> > +		gpio_direction_output(par->dc, 0);
> > +	else
> > +		gpio_direction_output(par->dc, 1);
> > +
> > +	ret = spi_write(spi, array->data, len);
> > +	if (ret)
> > +		dev_err(&spi->dev, "Couldn't send SPI command.\n");
> > +
> > +	/* Ensure that we remain in data mode. */
> > +	gpio_direction_output(par->dc, 1);
> > +
> > +	return ret;
> > +}
> > +#endif
> > +
> > +#if IS_ENABLED(CONFIG_I2C)
> 
> Ditto
> 
> >  static int ssd1307fb_i2c_write_array(struct ssd1307fb_par *par,
> >  				     struct ssd1307fb_array *array, u32 len)
> >  {
> > @@ -116,6 +143,7 @@ static int ssd1307fb_i2c_write_array(struct ssd1307fb_par *par,
> >  
> >  	return 0;
> >  }
> > +#endif
> >  
> >  static inline int ssd1307fb_write_cmd(struct ssd1307fb_par *par, u8 cmd)
> >  {
> > @@ -385,6 +413,10 @@ static const struct of_device_id ssd1307fb_of_match[] = {
> >  		.compatible = "solomon,ssd1306fb-i2c",
> >  		.data = (void *)&ssd1307fb_ssd1306_deviceinfo,
> >  	},
> > +	{
> > +		.compatible = "solomon,ssd1306",
> > +		.data = (void *)&ssd1307fb_ssd1306_deviceinfo,
> > +	},
> >  	{
> >  		.compatible = "solomon,ssd1309fb-i2c",
> >  		.data = (void *)&ssd1307fb_ssd1309_deviceinfo,
> > @@ -419,9 +451,24 @@ static int ssd1307fb_probe(struct device_d *dev)
> >  
> >  	par->device_info = (struct ssd1307fb_deviceinfo *)match->data;
> >  
> > -	par->client = to_i2c_client(dev);
> > -	i2c_set_clientdata(par->client, par);
> > -	par->write_array = ssd1307fb_i2c_write_array;
> > +#if IS_ENABLED(CONFIG_I2C)
> > +	if (dev->bus == &i2c_bus) {
> 
> if (IS_ENABLED(CONFIG_I2C) && dev->bus == &i2c_bus) {
> 
> > +		par->client = to_i2c_client(dev);
> > +		i2c_set_clientdata(par->client, par);
> > +		par->write_array = ssd1307fb_i2c_write_array;
> > +	}
> > +#endif
> > +#if IS_ENABLED(CONFIG_SPI)
> > +	if (dev->bus == &spi_bus) {
> 
> Ditto
> 
> > +		par->spi = (struct spi_device *)dev->type_data;
> > +		par->dc = of_get_named_gpio(node, "dc-gpios", 0);
> > +		if (!gpio_is_valid(par->dc)) {
> > +			ret = par->dc;
> > +			goto fb_alloc_error;
> > +		}
> > +		par->write_array = ssd1307fb_spi_write_array;
> > +	}
> > +#endif
> >  
> >  	par->reset = of_get_named_gpio_flags(node,
> >  					 "reset-gpios", 0, &of_flags);
> > @@ -591,9 +638,22 @@ fb_alloc_error:
> >  	return ret;
> >  }
> >  
> > -static struct driver_d ssd1307fb_driver = {
> > -	.name = "ssd1307fb",
> > +static __maybe_unused struct driver_d ssd1307fb_i2c_driver = {
> > +	.name = "ssd1307fb-i2c",
> >  	.probe = ssd1307fb_probe,
> >  	.of_compatible = DRV_OF_COMPAT(ssd1307fb_of_match),
> >  };
> > -device_i2c_driver(ssd1307fb_driver);
> > +
> > +#if IS_ENABLED(CONFIG_I2C)
> > +device_i2c_driver(ssd1307fb_i2c_driver);
> > +#endif
> 
> I think you should add !CONFIG_I2C stubs to i2c/i2c.h
> 
> > +
> > +static __maybe_unused struct driver_d ssd1307fb_spi_driver = {
> > +	.name = "ssd1307fb-spi",
> > +	.probe = ssd1307fb_probe,
> > +	.of_compatible = DRV_OF_COMPAT(ssd1307fb_of_match),
> > +};
> > +
> > +#if IS_ENABLED(CONFIG_SPI)
> > +device_spi_driver(ssd1307fb_spi_driver);
> > +#endif
> 
> Ditto.
> 
> > 
> 
> 

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 5/5] video: ssd1307fb: add spi support
  2021-12-17 21:13     ` Michael Tretter
@ 2021-12-18  6:20       ` Ahmad Fatoum
  0 siblings, 0 replies; 9+ messages in thread
From: Ahmad Fatoum @ 2021-12-18  6:20 UTC (permalink / raw)
  To: Michael Tretter; +Cc: barebox

On 17.12.21 22:13, Michael Tretter wrote:
> On Fri, 17 Dec 2021 20:00:16 +0100, Ahmad Fatoum wrote:
>> On 17.12.21 19:23, Michael Tretter wrote:
>>> The Solomon display drivers also support SPI in addition to the I2C.
>>> Add SPI support to the driver that already supports I2C by implementing
>>> the bus write function for SPI and registering an SPI driver.
>>>
>>> While the driver needs I2C or SPI, either subsystem is optional as long
>>> as one is enabled.
>>>
>>> WARNING: The device tree bindings for the ssd1306 with SPI are not
>>> documented!
>>>
>>> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
>>> ---
>>>  drivers/video/Kconfig     |  2 +-
>>>  drivers/video/ssd1307fb.c | 72 +++++++++++++++++++++++++++++++++++----
>>>  2 files changed, 67 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
>>> index a87e8c063899..cfbd541a956e 100644
>>> --- a/drivers/video/Kconfig
>>> +++ b/drivers/video/Kconfig
>>> @@ -15,7 +15,7 @@ config FRAMEBUFFER_CONSOLE
>>>  
>>>  config DRIVER_VIDEO_FB_SSD1307
>>>  	bool "Solomon SSD1307 framebuffer support"
>>> -	depends on I2C && GPIOLIB
>>> +	depends on (I2C || SPI) && GPIOLIB
>>>  
>>>  config VIDEO_VPL
>>>  	depends on OFTREE
>>> diff --git a/drivers/video/ssd1307fb.c b/drivers/video/ssd1307fb.c
>>> index d0df073b8ef2..2939d4348405 100644
>>> --- a/drivers/video/ssd1307fb.c
>>> +++ b/drivers/video/ssd1307fb.c
>>> @@ -23,6 +23,7 @@
>>>  #include <gpio.h>
>>>  #include <of_gpio.h>
>>>  #include <regulator.h>
>>> +#include <spi/spi.h>
>>>  
>>>  #define SSD1307FB_DATA                          0x40
>>>  #define SSD1307FB_COMMAND                       0x80
>>> @@ -73,12 +74,14 @@ struct ssd1307fb_par {
>>>  	u32 dclk_frq;
>>>  	const struct ssd1307fb_deviceinfo *device_info;
>>>  	struct i2c_client *client;
>>> +	struct spi_device *spi;
>>>  	u32 height;
>>>  	struct fb_info *info;
>>>  	u32 page_offset;
>>>  	u32 prechargep1;
>>>  	u32 prechargep2;
>>>  	int reset;
>>> +	int dc;
>>>  	struct regulator *vbat;
>>>  	u32 seg_remap;
>>>  	u32 vcomh;
>>> @@ -100,6 +103,30 @@ static struct ssd1307fb_array *ssd1307fb_alloc_array(u32 len, u8 type)
>>>  	return array;
>>>  }
>>>  
>>> +#if IS_ENABLED(CONFIG_SPI)
>>> +static int ssd1307fb_spi_write_array(struct ssd1307fb_par *par,
>>> +				     struct ssd1307fb_array *array, u32 len)
>>
>> __maybe_unused and drop the #if?

Ah, you could even drop __maybe_unused altogether as the #ifdef farther down
is replaced with an IS_ENABLED.

> Why doesn't this fail to link? The function uses spi_write, which is static
> inline, but spi_write uses spi_sync, which is only defined in spi.c and is not
> stubbed if !CONFIG_SPI. I would have expected to not be able to use spi_write
> if CONFIG_SPI is disabled. Is there some dead code elimination happening
> before linking?

Dead code elimination would discard this function at compile time, because it's
internal linkage and unreferenced. You could even make it global and it
wouldn't fail to link, but for another reason: barebox places all functions in
separate function sections and linker will garbage-collect unreferenced sections
leading to the same outcome: It never needs to resolve spi_sync.

If you want to break the build, you need to have an actual user
(or enable module support and EXPORT_SYMBOL(ssd1307fb_spi_write_array)).

Cheers,
Ahmad

> 
> Michael
> 
>>
>>> +{
>>> +	struct spi_device *spi = par->spi;
>>> +	int ret;
>>> +
>>> +	if (array->type == SSD1307FB_COMMAND)
>>> +		gpio_direction_output(par->dc, 0);
>>> +	else
>>> +		gpio_direction_output(par->dc, 1);
>>> +
>>> +	ret = spi_write(spi, array->data, len);
>>> +	if (ret)
>>> +		dev_err(&spi->dev, "Couldn't send SPI command.\n");
>>> +
>>> +	/* Ensure that we remain in data mode. */
>>> +	gpio_direction_output(par->dc, 1);
>>> +
>>> +	return ret;
>>> +}
>>> +#endif
>>> +
>>> +#if IS_ENABLED(CONFIG_I2C)
>>
>> Ditto
>>
>>>  static int ssd1307fb_i2c_write_array(struct ssd1307fb_par *par,
>>>  				     struct ssd1307fb_array *array, u32 len)
>>>  {
>>> @@ -116,6 +143,7 @@ static int ssd1307fb_i2c_write_array(struct ssd1307fb_par *par,
>>>  
>>>  	return 0;
>>>  }
>>> +#endif
>>>  
>>>  static inline int ssd1307fb_write_cmd(struct ssd1307fb_par *par, u8 cmd)
>>>  {
>>> @@ -385,6 +413,10 @@ static const struct of_device_id ssd1307fb_of_match[] = {
>>>  		.compatible = "solomon,ssd1306fb-i2c",
>>>  		.data = (void *)&ssd1307fb_ssd1306_deviceinfo,
>>>  	},
>>> +	{
>>> +		.compatible = "solomon,ssd1306",
>>> +		.data = (void *)&ssd1307fb_ssd1306_deviceinfo,
>>> +	},
>>>  	{
>>>  		.compatible = "solomon,ssd1309fb-i2c",
>>>  		.data = (void *)&ssd1307fb_ssd1309_deviceinfo,
>>> @@ -419,9 +451,24 @@ static int ssd1307fb_probe(struct device_d *dev)
>>>  
>>>  	par->device_info = (struct ssd1307fb_deviceinfo *)match->data;
>>>  
>>> -	par->client = to_i2c_client(dev);
>>> -	i2c_set_clientdata(par->client, par);
>>> -	par->write_array = ssd1307fb_i2c_write_array;
>>> +#if IS_ENABLED(CONFIG_I2C)
>>> +	if (dev->bus == &i2c_bus) {
>>
>> if (IS_ENABLED(CONFIG_I2C) && dev->bus == &i2c_bus) {
>>
>>> +		par->client = to_i2c_client(dev);
>>> +		i2c_set_clientdata(par->client, par);
>>> +		par->write_array = ssd1307fb_i2c_write_array;
>>> +	}
>>> +#endif
>>> +#if IS_ENABLED(CONFIG_SPI)
>>> +	if (dev->bus == &spi_bus) {
>>
>> Ditto
>>
>>> +		par->spi = (struct spi_device *)dev->type_data;
>>> +		par->dc = of_get_named_gpio(node, "dc-gpios", 0);
>>> +		if (!gpio_is_valid(par->dc)) {
>>> +			ret = par->dc;
>>> +			goto fb_alloc_error;
>>> +		}
>>> +		par->write_array = ssd1307fb_spi_write_array;
>>> +	}
>>> +#endif
>>>  
>>>  	par->reset = of_get_named_gpio_flags(node,
>>>  					 "reset-gpios", 0, &of_flags);
>>> @@ -591,9 +638,22 @@ fb_alloc_error:
>>>  	return ret;
>>>  }
>>>  
>>> -static struct driver_d ssd1307fb_driver = {
>>> -	.name = "ssd1307fb",
>>> +static __maybe_unused struct driver_d ssd1307fb_i2c_driver = {
>>> +	.name = "ssd1307fb-i2c",
>>>  	.probe = ssd1307fb_probe,
>>>  	.of_compatible = DRV_OF_COMPAT(ssd1307fb_of_match),
>>>  };
>>> -device_i2c_driver(ssd1307fb_driver);
>>> +
>>> +#if IS_ENABLED(CONFIG_I2C)
>>> +device_i2c_driver(ssd1307fb_i2c_driver);
>>> +#endif
>>
>> I think you should add !CONFIG_I2C stubs to i2c/i2c.h
>>
>>> +
>>> +static __maybe_unused struct driver_d ssd1307fb_spi_driver = {
>>> +	.name = "ssd1307fb-spi",
>>> +	.probe = ssd1307fb_probe,
>>> +	.of_compatible = DRV_OF_COMPAT(ssd1307fb_of_match),
>>> +};
>>> +
>>> +#if IS_ENABLED(CONFIG_SPI)
>>> +device_spi_driver(ssd1307fb_spi_driver);
>>> +#endif
>>
>> Ditto.
>>
>>>
>>
>>
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-12-18  6:22 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-17 18:22 [PATCH 0/5] video: ssd1307fb: Add SPI support Michael Tretter
2021-12-17 18:22 ` [PATCH 1/5] video: ssd1307fb: pass par instead of i2c client to write Michael Tretter
2021-12-17 18:22 ` [PATCH 2/5] video: ssd1307fb: don't use i2c client for logging Michael Tretter
2021-12-17 18:22 ` [PATCH 3/5] video: ssd1307fb: move i2c setup to single place Michael Tretter
2021-12-17 18:22 ` [PATCH 4/5] video: ssd1307fb: use function pointer for write Michael Tretter
2021-12-17 18:23 ` [PATCH 5/5] video: ssd1307fb: add spi support Michael Tretter
2021-12-17 19:00   ` Ahmad Fatoum
2021-12-17 21:13     ` Michael Tretter
2021-12-18  6:20       ` Ahmad Fatoum

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox