From: Ahmad Fatoum <a.fatoum@pengutronix.de>
To: Michael Tretter <m.tretter@pengutronix.de>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH 5/5] video: ssd1307fb: add spi support
Date: Sat, 18 Dec 2021 07:20:50 +0100 [thread overview]
Message-ID: <ab81b5e2-15c6-c632-ed25-87e1af2a6e4e@pengutronix.de> (raw)
In-Reply-To: <20211217211346.GF29518@pengutronix.de>
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
prev parent reply other threads:[~2021-12-18 6:22 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ab81b5e2-15c6-c632-ed25-87e1af2a6e4e@pengutronix.de \
--to=a.fatoum@pengutronix.de \
--cc=barebox@lists.infradead.org \
--cc=m.tretter@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox