From: Javier Martinez Canillas <javierm@redhat.com> Date: 2022-02-11 09:19:47
This patch series adds a DRM driver for the Solomon OLED SSD1305, SSD1306,
SSD1307 and SSD1309 displays. It is a port of the ssd1307fb fbdev driver.
Using the DRM fbdev emulation, all the tests from Geert Uytterhoeven repo
(https://git.kernel.org/pub/scm/linux/kernel/git/geert/fbtest.git) passes.
I've also tested it using the display as a VT output and even though fbcon
seems to work, it is mostly unusable on a 128x64 SSD1306 display.
This is a v4 that addresses the issues pointed in v3. Thanks a lot to all
reviewers that gave me feedback and comments.
I didn't include the patch that adds the SPI support this time, because it
will require changes in the existing Device Tree binding. And I wanted to
avoid that bikesheeding for now, to focus on the core and I2C parts.
Once this series land, I'll post patches for the SPI support. But the WIP
patch posted in v3 should still apply cleanly on top of this v4:
https://patchwork.kernel.org/project/dri-devel/patch/20220209091204.2513437-1-javierm@redhat.com/
Patch #1 splits per-line conversion logic in drm_fb_xrgb8888_to_gray8() to
a separate drm_fb_xrgb8888_to_gray8_line() helper function.
Patch #2 adds a new drm_fb_xrgb8888_to_mono_reversed() helper function to
convert from XR24 to reversed monochrome. The latter internally converts
each line first to 8-bit grayscale and then to 1-bit reversed monochrome.
Patch #3 adds the driver. This only has the core support and doesn't have
any bus specific code, separate drivers are needed for the transport used.
Patch #4 adds a driver to use the I2C bus to communicate with the device.
Patch #5 adds a MAINTAINERS entry for the DRM driver and patch #6 adds
myself as co-maintainer of the existing DT binding for the ssd1307fb,
since the same DT binding is used for both the fbdev and DRM drivers.
Best regards,
Javier
Changes in v4:
- Rename end_offset to end_len (Thomas Zimmermann)
- Warn once if dst_pitch is not a multiple of 8 (Thomas Zimmermann)
- Drop drm_fb_gray8_to_mono_reversed() that's not used (Thomas Zimmermann)
- Allocate single buffer for both copy cma memory and gray8 (Thomas Zimmermann)
- Add Thomas Zimmermann Reviewed-by tag to patch adding XR24 -> mono helper.
- Rename vbat supply to vcc since is how's labeled in the device (Mark Brown)
- Don't make the regulator option since is always needed (Mark Brown)
- Add solomon Kconfig source and directory inclusion sorted (Andy Shevchenko)
- Use SSD130x instead of SSD130X to denote is not a model name (Andy Shevchenko)
- Check if there's a reset pin in the callee and not the caller (Andy Shevchenko)
- Define missing commands instead of using magic numbers (Andy Shevchenko)
- Use GENMASK() and FIELD_PREP() macros when possible (Andy Shevchenko)
- Avoid using ternary operators to ease code readablity (Andy Shevchenko)
- Use i++ instead of --i on some for loops (Andy Shevchenko)
- Remove redundant blank lines (Andy Shevchenko)
- Rename power_off label to out_power_off (Andy Shevchenko)
- Use dev_err_probe() even if no -EPROBE_DEFER (Andy Shevchenko)
- Don't use plural Authors if there's only one (Andy Shevchenko)
- Remove unnecessary casting (Geert Uytterhoeven)
- Remove redundant blank lines (Andy Shevchenko)
- Remove comma after of_device_id table terminator (Andy Shevchenko)
- Add Rob Herring Acked-by tag to patch adding as DT binding co-maintainer.
Changes in v3:
- Add a drm_fb_xrgb8888_to_gray8_line() helper function (Thomas Zimmermann)
- Also add a drm_fb_xrgb8888_to_mono_reversed() helper (Thomas Zimmermann)
- Split lines copy to drm_fb_gray8_to_mono_reversed_line() (Thomas Zimmermann)
- Handle case where the source buffer is not aligned to 8 (Thomas Zimmermann)
- Move driver from tiny sub-dir to drivers/gpu/drm/solomon (Sam Ravnborg)
- Split driver in a bus agnostic core and bus specific (Andy Shevchenko)
- Use regmap to access the chip registers (Andy Shevchenko)
- Remove unnecessary blank lines (Andy Shevchenko)
- Remove unneeded inline specifier in functions (Andy Shevchenko)
- Add a comment about always returning a single mode (Andy Shevchenko)
- Change write command logic to use do while loop (Andy Shevchenko)
- Use "firmware description" instead of "device tree" (Andy Shevchenko)
- Use return foo() instead of returning the return value (Andy Shevchenko)
- Don't split lines longer than 80 chars if makes less readable (Andy Shevchenko)
- Remove redundant else statements in .mode_valid callback (Andy Shevchenko)
- Rename powero{n,ff}() functions to power_o{n,ff)() (Andy Shevchenko)
- Use dev_err_probe() to prevent spam logs on probe deferral (Andy Shevchenko)
- Remove ',' after sentinel terminator in array (Andy Shevchenko)
- Fix a bug when doing partial updates (Geert Uytterhoeven)
- Add a separate driver for SSD130X chips I2C support (Andy Shevchenko)
- Adapt MAINTAINERS entry to point to the new drivers/gpu/drm/solomon directory.
Changes in v2:
- Drop patch that was adding a DRM_MODE_CONNECTOR_I2C type.
- Invert order of backlight {en,dis}able and display {on,off} (Sam Ravnborg)
- Don't clear the screen and turn on display on probe (Sam Ravnborg)
- Use backlight_get_brightness() macro to get BL brightness (Sam Ravnborg)
- Use dev managed version of devm_backlight_device_register() (Sam Ravnborg)
- Use dev_name(dev) for backlight name instead of an array (Sam Ravnborg)
- Drop the .get_brightness callback since isn't needed (Sam Ravnborg)
- Rename driver to ssd130x since supports a display family (Thomas Zimmermann)
- Drop the TINY prefix from the Kconfig symbol (Thomas Zimmermann)
- Sort the Kconfig symbol dependencies alphabetically (Thomas Zimmermann)
- Rename struct ssd130x_array to struct ssd130x_i2c_msg (Thomas Zimmermann)
- Rename struct ssd130x_i2c_msg .type member to .cmd (Thomas Zimmermann)
- Use sizeof(*foo) instead of sizeof(struct foo) (Thomas Zimmermann)
- Use struct_size() macro to calculate sizeof(*foo) + len (Thomas Zimmermann)
- Use kcalloc() instead of kmalloc_array() + memset() (Thomas Zimmermann)
- Use shadow plane helpers virtual screen support (Thomas Zimmermann)
- Remove unused goto label in ssd1307_fb_blit_rect() (Thomas Zimmermann)
- Use drm_set_preferred_mode() inset of manually set (Thomas Zimmermann)
- Use shadow plane helpers virtual screen support (Thomas Zimmermann)
- Remove unused goto label in ssd1307_fb_blit_rect() (Thomas Zimmermann)
- Use drm_set_preferred_mode() inset of manually set (Thomas Zimmermann)
- Reorganize code in probe to make it more legible (Thomas Zimmermann)
- ssd130x_write_cmd() uses varargs to simplify I2C code (Thomas Zimmermann)
- Move regulator/pwm init logic to display pipe enable callback.
- Add Sam Ravnborg's acked-by to patch adding a MAINTAINERS entry (Sam Ravnborg)
- Add myself as co-maintainer of the ssd1370fb DT binding (Sam Ravnborg).
Javier Martinez Canillas (6):
drm/format-helper: Add drm_fb_xrgb8888_to_gray8_line()
drm/format-helper: Add drm_fb_xrgb8888_to_mono_reversed()
drm: Add driver for Solomon SSD130x OLED displays
drm/solomon: Add SSD130x OLED displays I2C support
MAINTAINERS: Add entry for Solomon SSD130x OLED displays DRM driver
dt-bindings: display: ssd1307fb: Add myself as binding co-maintainer
.../bindings/display/solomon,ssd1307fb.yaml | 1 +
MAINTAINERS | 7 +
drivers/gpu/drm/Kconfig | 2 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/drm_format_helper.c | 138 ++-
drivers/gpu/drm/solomon/Kconfig | 21 +
drivers/gpu/drm/solomon/Makefile | 2 +
drivers/gpu/drm/solomon/ssd130x-i2c.c | 116 +++
drivers/gpu/drm/solomon/ssd130x.c | 852 ++++++++++++++++++
drivers/gpu/drm/solomon/ssd130x.h | 76 ++
include/drm/drm_format_helper.h | 4 +
11 files changed, 1208 insertions(+), 12 deletions(-)
create mode 100644 drivers/gpu/drm/solomon/Kconfig
create mode 100644 drivers/gpu/drm/solomon/Makefile
create mode 100644 drivers/gpu/drm/solomon/ssd130x-i2c.c
create mode 100644 drivers/gpu/drm/solomon/ssd130x.c
create mode 100644 drivers/gpu/drm/solomon/ssd130x.h
--
2.34.1
From: Javier Martinez Canillas <javierm@redhat.com> Date: 2022-02-11 09:19:49
Pull the per-line conversion logic into a separate helper function.
This will allow to do line-by-line conversion in other helpers that
convert to a gray8 format.
Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
(no changes since v3)
Changes in v3:
- Add a drm_fb_xrgb8888_to_gray8_line() helper function (Thomas Zimmermann)
drivers/gpu/drm/drm_format_helper.c | 31 ++++++++++++++++++-----------
1 file changed, 19 insertions(+), 12 deletions(-)
@@ -464,6 +464,21 @@ void drm_fb_xrgb8888_to_xrgb2101010_toio(void __iomem *dst,}EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010_toio);+staticvoiddrm_fb_xrgb8888_to_gray8_line(u8*dst,constu32*src,unsignedintpixels)+{+unsignedintx;++for(x=0;x<pixels;x++){+u8r=(*src&0x00ff0000)>>16;+u8g=(*src&0x0000ff00)>>8;+u8b=*src&0x000000ff;++/* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */+*dst++=(3*r+6*g+b)/10;+src++;+}+}+/***drm_fb_xrgb8888_to_gray8-ConvertXRGB8888tograyscale*@dst:8-bitgrayscaledestinationbuffer
@@ -508,16 +524,7 @@ void drm_fb_xrgb8888_to_gray8(void *dst, unsigned int dst_pitch, const void *vadfor(y=clip->y1;y<clip->y2;y++){dst8=dst;src32=memcpy(buf,vaddr,len);-for(x=clip->x1;x<clip->x2;x++){-u8r=(*src32&0x00ff0000)>>16;-u8g=(*src32&0x0000ff00)>>8;-u8b=*src32&0x000000ff;--/* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */-*dst8++=(3*r+6*g+b)/10;-src32++;-}-+drm_fb_xrgb8888_to_gray8_line(dst8,src32,linepixels);vaddr+=fb->pitches[0];dst+=dst_pitch;}
From: Javier Martinez Canillas <javierm@redhat.com> Date: 2022-02-11 09:19:52
Add support to convert from XR24 to reversed monochrome for drivers that
control monochromatic display panels, that only have 1 bit per pixel.
The function does a line-by-line conversion doing an intermediate step
first from XR24 to 8-bit grayscale and then to reversed monochrome.
The drm_fb_gray8_to_mono_reversed_line() helper was based on code from
drivers/gpu/drm/tiny/repaper.c driver.
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
---
Changes in v4:
- Rename end_offset to end_len (Thomas Zimmermann)
- Warn once if dst_pitch is not a multiple of 8 (Thomas Zimmermann)
- Drop drm_fb_gray8_to_mono_reversed() that's not used (Thomas Zimmermann)
- Allocate single buffer for both copy cma memory and gray8 (Thomas Zimmermann)
- Add Thomas Zimmermann Reviewed-by tag to patch adding XR24 -> mono helper.
Changes in v3:
- Also add a drm_fb_xrgb8888_to_mono_reversed() helper (Thomas Zimmermann)
- Split lines copy to drm_fb_gray8_to_mono_reversed_line() (Thomas Zimmermann)
- Handle case where the source buffer is not aligned to 8 (Thomas Zimmermann)
drivers/gpu/drm/drm_format_helper.c | 107 ++++++++++++++++++++++++++++
include/drm/drm_format_helper.h | 4 ++
2 files changed, 111 insertions(+)
@@ -591,3 +591,110 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_forreturn-EINVAL;}EXPORT_SYMBOL(drm_fb_blit_toio);++staticvoiddrm_fb_gray8_to_mono_reversed_line(u8*dst,constu8*src,unsignedintpixels,+unsignedintstart_offset,unsignedintend_len)+{+unsignedintxb,i;++for(xb=0;xb<pixels;xb++){+unsignedintstart=0,end=8;+u8byte=0x00;++if(xb==0&&start_offset)+start=start_offset;++if(xb==pixels-1&&end_len)+end=end_len;++for(i=start;i<end;i++){+unsignedintx=xb*8+i;++byte>>=1;+if(src[x]>>7)+byte|=BIT(7);+}+*dst++=byte;+}+}++/**+*drm_fb_xrgb8888_to_mono_reversed-ConvertXRGB8888toreversedmonochrome+*@dst:reversedmonochromedestinationbuffer+*@dst_pitch:Numberofbytesbetweentwoconsecutivescanlineswithindst+*@src:XRGB8888sourcebuffer+*@fb:DRMframebuffer+*@clip:Cliprectangleareatocopy+*+*DRMdoesn'thavenativemonochromesupport.+*SuchdriverscanannouncethecommonlysupportedXR24formattouserspace+*andusethisfunctiontoconverttothenativeformat.+*+*Thisfunctionusesdrm_fb_xrgb8888_to_gray8()toconverttograyscaleand+*thentheresultisconvertedfromgrayscaletoreversedmonohrome.+*/+voiddrm_fb_xrgb8888_to_mono_reversed(void*dst,unsignedintdst_pitch,constvoid*vaddr,+conststructdrm_framebuffer*fb,conststructdrm_rect*clip)+{+unsignedintlinepixels=drm_rect_width(clip);+unsignedintlines=clip->y2-clip->y1;+unsignedintcpp=fb->format->cpp[0];+unsignedintlen_src32=linepixels*cpp;+unsignedintstart_offset,end_len;+unsignedinty;+u8*mono=dst,*gray8;+u32*src32;++if(WARN_ON(fb->format->format!=DRM_FORMAT_XRGB8888))+return;++/*+*Thereversedmonodestinationbuffercontains1bitperpixel+*anddestinationscanlineshavetobeinmultipleof8pixels.+*/+if(!dst_pitch)+dst_pitch=DIV_ROUND_UP(linepixels,8);++WARN_ONCE(dst_pitch%8!=0,"dst_pitch is not a multiple of 8\n");++/*+*Thecmamemoryiswrite-combinedsoreadsareuncached.+*Speedupbyfetchingonelineatatime.+*+*Also,formatconversionfromXR24toreversedmonochrome+*aredoneline-by-linebutareconvertedto8-bitgrayscale+*asanintermediatestep.+*+*Allocateabuffertobeusedforbothcopyingfromthecma+*memoryandtostoretheintermediategrayscalelinepixels.+*/+src32=kmalloc(len_src32+linepixels,GFP_KERNEL);+if(!src32)+return;++gray8=(u8*)src32+len_src32;++/*+*Fordamagehandling,itispossiblethatonlypartsofthesource+*bufferiscopiedandthiscouldleadtostartandendpixelsthat+*arenotalignedtomultipleof8.+*+*Calculateifthestartandendpixelsarenotalignedandsetthe+*offsetsforthereversedmonolineconversionfunctiontoadjust.+*/+start_offset=clip->x1%8;+end_len=clip->x2%8;++vaddr+=clip_offset(clip,fb->pitches[0],cpp);+for(y=0;y<lines;y++){+src32=memcpy(src32,vaddr,len_src32);+drm_fb_xrgb8888_to_gray8_line(gray8,src32,linepixels);+drm_fb_gray8_to_mono_reversed_line(mono,gray8,dst_pitch,+start_offset,end_len);+vaddr+=fb->pitches[0];+mono+=dst_pitch;+}++kfree(src32);+}+EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono_reversed);
From: Javier Martinez Canillas <javierm@redhat.com> Date: 2022-02-11 09:19:56
The ssd130x driver only provides the core support for these devices but it
does not have any bus transport logic. Add a driver to interface over I2C.
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
Changes in v4:
- Remove unnecessary casting (Geert Uytterhoeven)
- Remove redundant blank lines (Andy Shevchenko)
- Remove comma after of_device_id table terminator (Andy Shevchenko)
Changes in v3:
- Add a separate driver for SSD130X chips I2C support (Andy Shevchenko)
drivers/gpu/drm/solomon/Kconfig | 9 ++
drivers/gpu/drm/solomon/Makefile | 1 +
drivers/gpu/drm/solomon/ssd130x-i2c.c | 116 ++++++++++++++++++++++++++
3 files changed, 126 insertions(+)
create mode 100644 drivers/gpu/drm/solomon/ssd130x-i2c.c
From: Javier Martinez Canillas <javierm@redhat.com> Date: 2022-02-11 09:19:58
This adds a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon
OLED display controllers.
It's only the core part of the driver and a bus specific driver is needed
for each transport interface supported by the display controllers.
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
Changes in v4:
- Rename vbat supply to vcc since is how's labeled in the device (Mark Brown)
- Don't make the regulator option since is always needed (Mark Brown)
- Add solomon Kconfig source and directory inclusion sorted (Andy Shevchenko)
- Use SSD130x instead of SSD130X to denote is not a model name (Andy Shevchenko)
- Check if there's a reset pin in the callee and not the caller (Andy Shevchenko)
- Define missing commands instead of using magic numbers (Andy Shevchenko)
- Use GENMASK() and FIELD_PREP() macros when possible (Andy Shevchenko)
- Avoid using ternary operators to ease code readablity (Andy Shevchenko)
- Use i++ instead of --i on some for loops (Andy Shevchenko)
- Remove redundant blank lines (Andy Shevchenko)
- Rename power_off label to out_power_off (Andy Shevchenko)
- Use dev_err_probe() even if no -EPROBE_DEFER (Andy Shevchenko)
- Don't use plural Authors if there's only one (Andy Shevchenko)
Changes in v3:
- Move driver from tiny sub-dir to drivers/gpu/drm/solomon (Sam Ravnborg)
- Split driver in a bus agnostic core and bus specific (Andy Shevchenko)
- Use regmap to access the chip registers (Andy Shevchenko)
- Remove unnecessary blank lines (Andy Shevchenko)
- Remove unneeded inline specifier in functions (Andy Shevchenko)
- Add a comment about always returning a single mode (Andy Shevchenko)
- Change write command logic to use do while loop (Andy Shevchenko)
- Use "firmware description" instead of "device tree" (Andy Shevchenko)
- Use return foo() instead of returning the return value (Andy Shevchenko)
- Don't split lines longer than 80 chars if makes less readable (Andy Shevchenko)
- Remove redundant else statements in .mode_valid callback (Andy Shevchenko)
- Rename powero{n,ff}() functions to power_o{n,ff)() (Andy Shevchenko)
- Use dev_err_probe() to prevent spam logs on probe deferral (Andy Shevchenko)
- Remove ',' after sentinel terminator in array (Andy Shevchenko)
- Fix a bug when doing partial updates (Geert Uytterhoeven)
Changes in v2:
- Drop patch that was adding a DRM_MODE_CONNECTOR_I2C type.
- Invert order of backlight {en,dis}able and display {on,off} (Sam Ravnborg)
- Don't clear the screen and turn on display on probe (Sam Ravnborg)
- Use backlight_get_brightness() macro to get BL brightness (Sam Ravnborg)
- Use dev managed version of devm_backlight_device_register() (Sam Ravnborg)
- Use dev_name(dev) for backlight name instead of an array (Sam Ravnborg)
- Drop the .get_brightness callback since isn't needed (Sam Ravnborg)
- Rename driver to ssd130x since supports a display family (Thomas Zimmermann)
- Drop the TINY prefix from the Kconfig symbol (Thomas Zimmermann)
- Sort the Kconfig symbol dependencies alphabetically (Thomas Zimmermann)
- Rename struct ssd130x_array to struct ssd130x_i2c_msg (Thomas Zimmermann)
- Rename struct ssd130x_i2c_msg .type member to .cmd (Thomas Zimmermann)
- Use sizeof(*foo) instead of sizeof(struct foo) (Thomas Zimmermann)
- Use struct_size() macro to calculate sizeof(*foo) + len (Thomas Zimmermann)
- Use kcalloc() instead of kmalloc_array() + memset() (Thomas Zimmermann)
- Use shadow plane helpers virtual screen support (Thomas Zimmermann)
- Remove unused goto label in ssd1307_fb_blit_rect() (Thomas Zimmermann)
- Use drm_set_preferred_mode() inset of manually set (Thomas Zimmermann)
- Use shadow plane helpers virtual screen support (Thomas Zimmermann)
- Remove unused goto label in ssd1307_fb_blit_rect() (Thomas Zimmermann)
- Use drm_set_preferred_mode() inset of manually set (Thomas Zimmermann)
- Reorganize code in probe to make it more legible (Thomas Zimmermann)
- ssd130x_write_cmd() uses varargs to simplify I2C code (Thomas Zimmermann)
- Move regulator/pwm init logic to display pipe enable callback.
drivers/gpu/drm/Kconfig | 2 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/solomon/Kconfig | 12 +
drivers/gpu/drm/solomon/Makefile | 1 +
drivers/gpu/drm/solomon/ssd130x.c | 852 ++++++++++++++++++++++++++++++
drivers/gpu/drm/solomon/ssd130x.h | 76 +++
6 files changed, 944 insertions(+)
create mode 100644 drivers/gpu/drm/solomon/Kconfig
create mode 100644 drivers/gpu/drm/solomon/Makefile
create mode 100644 drivers/gpu/drm/solomon/ssd130x.c
create mode 100644 drivers/gpu/drm/solomon/ssd130x.h
@@ -0,0 +1,852 @@+// SPDX-License-Identifier: GPL-2.0-only+/*+*DRMdriverforSolomonSSD130xOLEDdisplays+*+*Copyright2022RedHatInc.+*Author:JavierMartinezCanillas<javierm@redhat.com>+*+*Basedondrivers/video/fbdev/ssd1307fb.c+*Copyright2012FreeElectrons+*/++#include<linux/backlight.h>+#include<linux/bitfield.h>+#include<linux/delay.h>+#include<linux/gpio/consumer.h>+#include<linux/property.h>+#include<linux/pwm.h>+#include<linux/regulator/consumer.h>++#include<drm/drm_atomic_helper.h>+#include<drm/drm_damage_helper.h>+#include<drm/drm_fb_cma_helper.h>+#include<drm/drm_fb_helper.h>+#include<drm/drm_format_helper.h>+#include<drm/drm_gem_atomic_helper.h>+#include<drm/drm_gem_framebuffer_helper.h>+#include<drm/drm_gem_shmem_helper.h>+#include<drm/drm_managed.h>+#include<drm/drm_modes.h>+#include<drm/drm_rect.h>+#include<drm/drm_probe_helper.h>++#include"ssd130x.h"++#define DRIVER_NAME "ssd130x"+#define DRIVER_DESC "DRM driver for Solomon SSD130x OLED displays"+#define DRIVER_DATE "20220131"+#define DRIVER_MAJOR 1+#define DRIVER_MINOR 0++#define SSD130X_DATA 0x40+#define SSD130X_COMMAND 0x80++#define SSD130X_SET_ADDRESS_MODE 0x20+#define SSD130X_SET_COL_RANGE 0x21+#define SSD130X_SET_PAGE_RANGE 0x22+#define SSD130X_CONTRAST 0x81+#define SSD130X_SET_LOOKUP_TABLE 0x91+#define SSD130X_CHARGE_PUMP 0x8d+#define SSD130X_SEG_REMAP_ON 0xa1+#define SSD130X_DISPLAY_OFF 0xae+#define SSD130X_SET_MULTIPLEX_RATIO 0xa8+#define SSD130X_DISPLAY_ON 0xaf+#define SSD130X_START_PAGE_ADDRESS 0xb0+#define SSD130X_SET_COM_SCAN_DIR 0xc0+#define SSD130X_SET_DISPLAY_OFFSET 0xd3+#define SSD130X_SET_CLOCK_FREQ 0xd5+#define SSD130X_SET_AREA_COLOR_MODE 0xd8+#define SSD130X_SET_PRECHARGE_PERIOD 0xd9+#define SSD130X_SET_COM_PINS_CONFIG 0xda+#define SSD130X_SET_VCOMH 0xdb++#define SSD130X_SET_COM_SCAN_DIR_MASK GENMASK(3, 2)+#define SSD130X_SET_COM_SCAN_DIR_SET(val) FIELD_PREP(SSD130X_SET_COM_SCAN_DIR_MASK, (val))+#define SSD130X_SET_CLOCK_DIV_MASK GENMASK(3, 0)+#define SSD130X_SET_CLOCK_DIV_SET(val) FIELD_PREP(SSD130X_SET_CLOCK_DIV_MASK, (val))+#define SSD130X_SET_CLOCK_FREQ_MASK GENMASK(7, 4)+#define SSD130X_SET_CLOCK_FREQ_SET(val) FIELD_PREP(SSD130X_SET_CLOCK_FREQ_MASK, (val))+#define SSD130X_SET_PRECHARGE_PERIOD1_MASK GENMASK(3, 0)+#define SSD130X_SET_PRECHARGE_PERIOD1_SET(val) FIELD_PREP(SSD130X_SET_PRECHARGE_PERIOD1_MASK, (val))+#define SSD130X_SET_PRECHARGE_PERIOD2_MASK GENMASK(7, 4)+#define SSD130X_SET_PRECHARGE_PERIOD2_SET(val) FIELD_PREP(SSD130X_SET_PRECHARGE_PERIOD2_MASK, (val))+#define SSD130X_SET_COM_PINS_CONFIG1_MASK GENMASK(4, 4)+#define SSD130X_SET_COM_PINS_CONFIG1_SET(val) FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG1_MASK, (!val))+#define SSD130X_SET_COM_PINS_CONFIG2_MASK GENMASK(5, 5)+#define SSD130X_SET_COM_PINS_CONFIG2_SET(val) FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG2_MASK, (val))++#define SSD130X_SET_ADDRESS_MODE_HORIZONTAL (0x00)+#define SSD130X_SET_ADDRESS_MODE_VERTICAL (0x01)+#define SSD130X_SET_ADDRESS_MODE_PAGE (0x02)++#define SSD130X_SET_AREA_COLOR_MODE_ENABLE (0x1e)+#define SSD130X_SET_AREA_COLOR_MODE_LOW_POWER (0x05)++#define MAX_CONTRAST 255++staticinlinestructssd130x_device*drm_to_ssd130x(structdrm_device*drm)+{+returncontainer_of(drm,structssd130x_device,drm);+}++/*+*Helpertowritedata(SSD130X_DATA)tothedevice.+*/+staticintssd130x_write_data(structssd130x_device*ssd130x,u8*values,intcount)+{+intret;++ret=regmap_bulk_write(ssd130x->regmap,SSD130X_DATA,values,count);+if(ret)+returnret;++return0;+}++/*+*Helpertowritecommand(SSD130X_COMMAND).Thefistvariadicargument+*isthecommandtowriteandthefollowingarethecommandoptions.+*/+staticintssd130x_write_cmd(structssd130x_device*ssd130x,intcount,+/* u8 cmd, u8 option, ... */...)+{+va_listap;+u8value;+intret;++va_start(ap,count);++do{+value=va_arg(ap,int);+ret=regmap_write(ssd130x->regmap,SSD130X_COMMAND,(u8)value);+if(ret)+gotoout_end;+}while(--count);++out_end:+va_end(ap);++returnret;+}++staticintssd130x_set_col_range(structssd130x_device*ssd130x,+u8col_start,u8cols)+{+u8col_end=col_start+cols-1;+intret;++if(col_start==ssd130x->col_start&&col_end==ssd130x->col_end)+return0;++ret=ssd130x_write_cmd(ssd130x,3,SSD130X_SET_COL_RANGE,col_start,col_end);+if(ret<0)+returnret;++ssd130x->col_start=col_start;+ssd130x->col_end=col_end;+return0;+}++staticintssd130x_set_page_range(structssd130x_device*ssd130x,+u8page_start,u8pages)+{+u8page_end=page_start+pages-1;+intret;++if(page_start==ssd130x->page_start&&page_end==ssd130x->page_end)+return0;++ret=ssd130x_write_cmd(ssd130x,3,SSD130X_SET_PAGE_RANGE,page_start,page_end);+if(ret<0)+returnret;++ssd130x->page_start=page_start;+ssd130x->page_end=page_end;+return0;+}++staticintssd130x_pwm_enable(structssd130x_device*ssd130x)+{+structdevice*dev=ssd130x->dev;+structpwm_statepwmstate;++ssd130x->pwm=pwm_get(dev,NULL);+if(IS_ERR(ssd130x->pwm)){+dev_err(dev,"Could not get PWM from firmware description!\n");+returnPTR_ERR(ssd130x->pwm);+}++pwm_init_state(ssd130x->pwm,&pwmstate);+pwm_set_relative_duty_cycle(&pwmstate,50,100);+pwm_apply_state(ssd130x->pwm,&pwmstate);++/* Enable the PWM */+pwm_enable(ssd130x->pwm);++dev_dbg(dev,"Using PWM%d with a %lluns period.\n",+ssd130x->pwm->pwm,pwm_get_period(ssd130x->pwm));++return0;+}++staticvoidssd130x_reset(structssd130x_device*ssd130x)+{+if(!ssd130x->reset)+return;++/* Reset the screen */+gpiod_set_value_cansleep(ssd130x->reset,1);+udelay(4);+gpiod_set_value_cansleep(ssd130x->reset,0);+udelay(4);+}++staticintssd130x_power_on(structssd130x_device*ssd130x)+{+structdevice*dev=ssd130x->dev;+intret;++ssd130x_reset(ssd130x);++ret=regulator_enable(ssd130x->vcc_reg);+if(ret){+dev_err(dev,"Failed to enable VCC: %d\n",ret);+returnret;+}++if(ssd130x->device_info->need_pwm){+ret=ssd130x_pwm_enable(ssd130x);+if(ret){+dev_err(dev,"Failed to enable PWM: %d\n",ret);+regulator_disable(ssd130x->vcc_reg);+returnret;+}+}++return0;+}++staticvoidssd130x_power_off(structssd130x_device*ssd130x)+{+if(ssd130x->device_info->need_pwm){+pwm_disable(ssd130x->pwm);+pwm_put(ssd130x->pwm);+}++regulator_disable(ssd130x->vcc_reg);+}++staticintssd130x_init(structssd130x_device*ssd130x)+{+u32precharge,dclk,com_invdir,compins,chargepump;+intret;++/* Set initial contrast */+ret=ssd130x_write_cmd(ssd130x,2,SSD130X_CONTRAST,ssd130x->contrast);+if(ret<0)+returnret;++/* Set segment re-map */+if(ssd130x->seg_remap){+ret=ssd130x_write_cmd(ssd130x,1,SSD130X_SEG_REMAP_ON);+if(ret<0)+returnret;+}++/* Set COM direction */+com_invdir=(SSD130X_SET_COM_SCAN_DIR|+SSD130X_SET_COM_SCAN_DIR_SET(ssd130x->com_invdir));+ret=ssd130x_write_cmd(ssd130x,1,com_invdir);+if(ret<0)+returnret;++/* Set multiplex ratio value */+ret=ssd130x_write_cmd(ssd130x,2,SSD130X_SET_MULTIPLEX_RATIO,ssd130x->height-1);+if(ret<0)+returnret;++/* set display offset value */+ret=ssd130x_write_cmd(ssd130x,2,SSD130X_SET_DISPLAY_OFFSET,ssd130x->com_offset);+if(ret<0)+returnret;++/* Set clock frequency */+dclk=(SSD130X_SET_CLOCK_DIV_SET(ssd130x->dclk_div-1)|+SSD130X_SET_CLOCK_FREQ_SET(ssd130x->dclk_frq));+ret=ssd130x_write_cmd(ssd130x,2,SSD130X_SET_CLOCK_FREQ,dclk);+if(ret<0)+returnret;++/* Set Area Color Mode ON/OFF & Low Power Display Mode */+if(ssd130x->area_color_enable||ssd130x->low_power){+u32mode=0;++if(ssd130x->area_color_enable)+mode|=SSD130X_SET_AREA_COLOR_MODE_ENABLE;++if(ssd130x->low_power)+mode|=SSD130X_SET_AREA_COLOR_MODE_LOW_POWER;++ret=ssd130x_write_cmd(ssd130x,2,SSD130X_SET_AREA_COLOR_MODE,mode);+if(ret<0)+returnret;+}++/* Set precharge period in number of ticks from the internal clock */+precharge=(SSD130X_SET_PRECHARGE_PERIOD1_SET(ssd130x->prechargep1)|+SSD130X_SET_PRECHARGE_PERIOD1_SET(ssd130x->prechargep2));+ret=ssd130x_write_cmd(ssd130x,2,SSD130X_SET_PRECHARGE_PERIOD,precharge);+if(ret<0)+returnret;++/* Set COM pins configuration */+compins=BIT(1);+compins|=(SSD130X_SET_COM_PINS_CONFIG1_SET(ssd130x->com_seq)|+SSD130X_SET_COM_PINS_CONFIG2_SET(ssd130x->com_lrremap));+ret=ssd130x_write_cmd(ssd130x,2,SSD130X_SET_COM_PINS_CONFIG,compins);+if(ret<0)+returnret;+++/* Set VCOMH */+ret=ssd130x_write_cmd(ssd130x,2,SSD130X_SET_VCOMH,ssd130x->vcomh);+if(ret<0)+returnret;++/* Turn on the DC-DC Charge Pump */+chargepump=BIT(4);++if(ssd130x->device_info->need_chargepump)+chargepump|=BIT(2);++ret=ssd130x_write_cmd(ssd130x,2,SSD130X_CHARGE_PUMP,chargepump);+if(ret<0)+returnret;++/* Set lookup table */+if(ssd130x->lookup_table_set){+inti;++ret=ssd130x_write_cmd(ssd130x,1,SSD130X_SET_LOOKUP_TABLE);+if(ret<0)+returnret;++for(i=0;i<ARRAY_SIZE(ssd130x->lookup_table);i++){+u8val=ssd130x->lookup_table[i];++if(val<31||val>63)+dev_warn(ssd130x->dev,+"lookup table index %d value out of range 31 <= %d <= 63\n",+i,val);+ret=ssd130x_write_cmd(ssd130x,1,val);+if(ret<0)+returnret;+}+}++/* Switch to horizontal addressing mode */+returnssd130x_write_cmd(ssd130x,2,SSD130X_SET_ADDRESS_MODE,+SSD130X_SET_ADDRESS_MODE_HORIZONTAL);+}++staticintssd130x_update_rect(structssd130x_device*ssd130x,u8*buf,+structdrm_rect*rect)+{+unsignedintx=rect->x1;+unsignedinty=rect->y1;+unsignedintwidth=drm_rect_width(rect);+unsignedintheight=drm_rect_height(rect);+unsignedintline_length=DIV_ROUND_UP(width,8);+unsignedintpages=DIV_ROUND_UP(y%8+height,8);+u32array_idx=0;+intret,i,j,k;+u8*data_array=NULL;++data_array=kcalloc(width,pages,GFP_KERNEL);+if(!data_array)+return-ENOMEM;++/*+*Thescreenisdividedinpages,eachhavingaheightof8+*pixels,andthewidthofthescreen.Whensendingabyteof+*datatothecontroller,itgivesthe8bitsforthecurrent+*column.I.e,thefirstbytearethe8bitsofthefirst+*column,thenthe8bitsforthesecondcolumn,etc.+*+*+*Representationofthescreen,assumingitis5bits+*wide.Eachletter-numbercombinationisabitthatcontrols+*onepixel.+*+*A0A1A2A3A4+*B0B1B2B3B4+*C0C1C2C3C4+*D0D1D2D3D4+*E0E1E2E3E4+*F0F1F2F3F4+*G0G1G2G3G4+*H0H1H2H3H4+*+*Ifyouwanttoupdatethisscreen,youneedtosend5bytes:+*(1)A0B0C0D0E0F0G0H0+*(2)A1B1C1D1E1F1G1H1+*(3)A2B2C2D2E2F2G2H2+*(4)A3B3C3D3E3F3G3H3+*(5)A4B4C4D4E4F4G4H4+*/++ret=ssd130x_set_col_range(ssd130x,ssd130x->col_offset+x,width);+if(ret<0)+gotoout_free;++ret=ssd130x_set_page_range(ssd130x,ssd130x->page_offset+y/8,pages);+if(ret<0)+gotoout_free;++for(i=y/8;i<y/8+pages;i++){+intm=8;++/* Last page may be partial */+if(8*(i+1)>ssd130x->height)+m=ssd130x->height%8;+for(j=x;j<x+width;j++){+u8data=0;++for(k=0;k<m;k++){+u8byte=buf[(8*i+k)*line_length+j/8];+u8bit=(byte>>(j%8))&1;++data|=bit<<k;+}+data_array[array_idx++]=data;+}+}++ret=ssd130x_write_data(ssd130x,data_array,width*pages);++out_free:+kfree(data_array);+returnret;+}++staticvoidssd130x_clear_screen(structssd130x_device*ssd130x)+{+u8*buf=NULL;+structdrm_rectfullscreen={+.x1=0,+.x2=ssd130x->width,+.y1=0,+.y2=ssd130x->height,+};++buf=kcalloc(ssd130x->width,ssd130x->height,GFP_KERNEL);+if(!buf)+return;++ssd130x_update_rect(ssd130x,buf,&fullscreen);++kfree(buf);+}++staticintssd130x_fb_blit_rect(structdrm_framebuffer*fb,conststructdma_buf_map*map,+structdrm_rect*rect)+{+structssd130x_device*ssd130x=drm_to_ssd130x(fb->dev);+void*vmap=map->vaddr;/* TODO: Use mapping abstraction properly */+intret=0;+u8*buf=NULL;++buf=kcalloc(fb->width,fb->height,GFP_KERNEL);+if(!buf)+return-ENOMEM;++drm_fb_xrgb8888_to_mono_reversed(buf,0,vmap,fb,rect);++ssd130x_update_rect(ssd130x,buf,rect);++kfree(buf);++returnret;+}++staticintssd130x_display_pipe_mode_valid(structdrm_simple_display_pipe*pipe,+conststructdrm_display_mode*mode)+{+structssd130x_device*ssd130x=drm_to_ssd130x(pipe->crtc.dev);++if(mode->hdisplay!=ssd130x->mode.hdisplay&&+mode->vdisplay!=ssd130x->mode.vdisplay)+returnMODE_ONE_SIZE;++if(mode->hdisplay!=ssd130x->mode.hdisplay)+returnMODE_ONE_WIDTH;++if(mode->vdisplay!=ssd130x->mode.vdisplay)+returnMODE_ONE_HEIGHT;++returnMODE_OK;+}++staticvoidssd130x_display_pipe_enable(structdrm_simple_display_pipe*pipe,+structdrm_crtc_state*crtc_state,+structdrm_plane_state*plane_state)+{+structssd130x_device*ssd130x=drm_to_ssd130x(pipe->crtc.dev);+structdrm_device*drm=&ssd130x->drm;+intidx,ret;++ret=ssd130x_power_on(ssd130x);+if(ret)+return;++ret=ssd130x_init(ssd130x);+if(ret)+gotoout_power_off;++if(!drm_dev_enter(drm,&idx))+gotoout_power_off;++ssd130x_clear_screen(ssd130x);++ssd130x_write_cmd(ssd130x,1,SSD130X_DISPLAY_ON);++backlight_enable(ssd130x->bl_dev);++drm_dev_exit(idx);++return;+out_power_off:+ssd130x_power_off(ssd130x);+}++staticvoidssd130x_display_pipe_disable(structdrm_simple_display_pipe*pipe)+{+structssd130x_device*ssd130x=drm_to_ssd130x(pipe->crtc.dev);+structdrm_device*drm=&ssd130x->drm;+intidx;++if(!drm_dev_enter(drm,&idx))+return;++ssd130x_clear_screen(ssd130x);++backlight_disable(ssd130x->bl_dev);++ssd130x_write_cmd(ssd130x,1,SSD130X_DISPLAY_OFF);++ssd130x_power_off(ssd130x);++drm_dev_exit(idx);+}++staticvoidssd130x_display_pipe_update(structdrm_simple_display_pipe*pipe,+structdrm_plane_state*old_plane_state)+{+structssd130x_device*ssd130x=drm_to_ssd130x(pipe->crtc.dev);+structdrm_plane_state*plane_state=pipe->plane.state;+structdrm_shadow_plane_state*shadow_plane_state=to_drm_shadow_plane_state(plane_state);+structdrm_framebuffer*fb=plane_state->fb;+structdrm_device*drm=&ssd130x->drm;+structdrm_rectsrc_clip,dst_clip;+intidx;++if(!fb)+return;++if(!pipe->crtc.state->active)+return;++if(!drm_atomic_helper_damage_merged(old_plane_state,plane_state,&src_clip))+return;++dst_clip=plane_state->dst;+if(!drm_rect_intersect(&dst_clip,&src_clip))+return;++if(!drm_dev_enter(drm,&idx))+return;++ssd130x_fb_blit_rect(plane_state->fb,&shadow_plane_state->data[0],&dst_clip);++drm_dev_exit(idx);+}++staticconststructdrm_simple_display_pipe_funcsssd130x_pipe_funcs={+.mode_valid=ssd130x_display_pipe_mode_valid,+.enable=ssd130x_display_pipe_enable,+.disable=ssd130x_display_pipe_disable,+.update=ssd130x_display_pipe_update,+DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,+};++staticintssd130x_connector_get_modes(structdrm_connector*connector)+{+structssd130x_device*ssd130x=drm_to_ssd130x(connector->dev);+structdrm_display_mode*mode=&ssd130x->mode;+structdevice*dev=ssd130x->dev;++mode=drm_mode_duplicate(connector->dev,&ssd130x->mode);+if(!mode){+dev_err(dev,"Failed to duplicated mode\n");+return0;+}++drm_mode_probed_add(connector,mode);+drm_set_preferred_mode(connector,mode->hdisplay,mode->vdisplay);++/* There is only a single mode */+return1;+}++staticconststructdrm_connector_helper_funcsssd130x_connector_helper_funcs={+.get_modes=ssd130x_connector_get_modes,+};++staticconststructdrm_connector_funcsssd130x_connector_funcs={+.reset=drm_atomic_helper_connector_reset,+.fill_modes=drm_helper_probe_single_connector_modes,+.destroy=drm_connector_cleanup,+.atomic_duplicate_state=drm_atomic_helper_connector_duplicate_state,+.atomic_destroy_state=drm_atomic_helper_connector_destroy_state,+};++staticconststructdrm_mode_config_funcsssd130x_mode_config_funcs={+.fb_create=drm_gem_fb_create_with_dirty,+.atomic_check=drm_atomic_helper_check,+.atomic_commit=drm_atomic_helper_commit,+};++staticconstuint32_tssd130x_formats[]={+DRM_FORMAT_XRGB8888,+};++DEFINE_DRM_GEM_FOPS(ssd130x_fops);++staticconststructdrm_driverssd130x_drm_driver={+DRM_GEM_SHMEM_DRIVER_OPS,+.name=DRIVER_NAME,+.desc=DRIVER_DESC,+.date=DRIVER_DATE,+.major=DRIVER_MAJOR,+.minor=DRIVER_MINOR,+.driver_features=DRIVER_ATOMIC|DRIVER_GEM|DRIVER_MODESET,+.fops=&ssd130x_fops,+};++staticintssd130x_update_bl(structbacklight_device*bdev)+{+structssd130x_device*ssd130x=bl_get_data(bdev);+intbrightness=backlight_get_brightness(bdev);+intret;++ssd130x->contrast=brightness;++ret=ssd130x_write_cmd(ssd130x,1,SSD130X_CONTRAST);+if(ret<0)+returnret;++ret=ssd130x_write_cmd(ssd130x,1,ssd130x->contrast);+if(ret<0)+returnret;++return0;+}++staticconststructbacklight_opsssd130xfb_bl_ops={+.update_status=ssd130x_update_bl,+};++staticvoidssd130x_parse_properties(structssd130x_device*ssd130x)+{+structdevice*dev=ssd130x->dev;++if(device_property_read_u32(dev,"solomon,width",&ssd130x->width))+ssd130x->width=96;++if(device_property_read_u32(dev,"solomon,height",&ssd130x->height))+ssd130x->height=16;++if(device_property_read_u32(dev,"solomon,page-offset",&ssd130x->page_offset))+ssd130x->page_offset=1;++if(device_property_read_u32(dev,"solomon,col-offset",&ssd130x->col_offset))+ssd130x->col_offset=0;++if(device_property_read_u32(dev,"solomon,com-offset",&ssd130x->com_offset))+ssd130x->com_offset=0;++if(device_property_read_u32(dev,"solomon,prechargep1",&ssd130x->prechargep1))+ssd130x->prechargep1=2;++if(device_property_read_u32(dev,"solomon,prechargep2",&ssd130x->prechargep2))+ssd130x->prechargep2=2;++if(!device_property_read_u8_array(dev,"solomon,lookup-table",+ssd130x->lookup_table,+ARRAY_SIZE(ssd130x->lookup_table)))+ssd130x->lookup_table_set=1;++ssd130x->seg_remap=!device_property_read_bool(dev,"solomon,segment-no-remap");+ssd130x->com_seq=device_property_read_bool(dev,"solomon,com-seq");+ssd130x->com_lrremap=device_property_read_bool(dev,"solomon,com-lrremap");+ssd130x->com_invdir=device_property_read_bool(dev,"solomon,com-invdir");+ssd130x->area_color_enable=+device_property_read_bool(dev,"solomon,area-color-enable");+ssd130x->low_power=device_property_read_bool(dev,"solomon,low-power");++ssd130x->contrast=127;+ssd130x->vcomh=ssd130x->device_info->default_vcomh;++/* Setup display timing */+if(device_property_read_u32(dev,"solomon,dclk-div",&ssd130x->dclk_div))+ssd130x->dclk_div=ssd130x->device_info->default_dclk_div;+if(device_property_read_u32(dev,"solomon,dclk-frq",&ssd130x->dclk_frq))+ssd130x->dclk_frq=ssd130x->device_info->default_dclk_frq;+}++staticintssd130x_init_modeset(structssd130x_device*ssd130x)+{+structdrm_display_mode*mode=&ssd130x->mode;+structdevice*dev=ssd130x->dev;+structdrm_device*drm=&ssd130x->drm;+unsignedlongmax_width,max_height;+intret;++ret=drmm_mode_config_init(drm);+if(ret){+dev_err(dev,"DRM mode config init failed: %d\n",ret);+returnret;+}++mode->type=DRM_MODE_TYPE_DRIVER;+mode->clock=1;+mode->hdisplay=mode->htotal=ssd130x->width;+mode->hsync_start=mode->hsync_end=ssd130x->width;+mode->vdisplay=mode->vtotal=ssd130x->height;+mode->vsync_start=mode->vsync_end=ssd130x->height;+mode->width_mm=27;+mode->height_mm=27;++max_width=max_t(unsignedlong,mode->hdisplay,DRM_SHADOW_PLANE_MAX_WIDTH);+max_height=max_t(unsignedlong,mode->vdisplay,DRM_SHADOW_PLANE_MAX_HEIGHT);++drm->mode_config.min_width=mode->hdisplay;+drm->mode_config.max_width=max_width;+drm->mode_config.min_height=mode->vdisplay;+drm->mode_config.max_height=max_height;+drm->mode_config.preferred_depth=32;+drm->mode_config.funcs=&ssd130x_mode_config_funcs;++ret=drm_connector_init(drm,&ssd130x->connector,&ssd130x_connector_funcs,+DRM_MODE_CONNECTOR_Unknown);+if(ret){+dev_err(dev,"DRM connector init failed: %d\n",ret);+returnret;+}++drm_connector_helper_add(&ssd130x->connector,&ssd130x_connector_helper_funcs);++ret=drm_simple_display_pipe_init(drm,&ssd130x->pipe,&ssd130x_pipe_funcs,+ssd130x_formats,ARRAY_SIZE(ssd130x_formats),+NULL,&ssd130x->connector);+if(ret){+dev_err(dev,"DRM simple display pipeline init failed: %d\n",ret);+returnret;+}++drm_plane_enable_fb_damage_clips(&ssd130x->pipe.plane);++drm_mode_config_reset(drm);++return0;+}++staticintssd130x_get_resources(structssd130x_device*ssd130x)+{+structdevice*dev=ssd130x->dev;++ssd130x->reset=devm_gpiod_get_optional(dev,"reset",GPIOD_OUT_LOW);+if(IS_ERR(ssd130x->reset))+returndev_err_probe(dev,PTR_ERR(ssd130x->reset),+"Failed to get reset gpio\n");++ssd130x->vcc_reg=devm_regulator_get(dev,"vcc");+if(IS_ERR(ssd130x->vcc_reg))+returndev_err_probe(dev,PTR_ERR(ssd130x->vcc_reg),+"Failed to get VCC regulator\n");++return0;+}++structssd130x_device*ssd130x_probe(structdevice*dev,structregmap*regmap)+{+structssd130x_device*ssd130x;+structbacklight_device*bl;+structdrm_device*drm;+intret;++ssd130x=devm_drm_dev_alloc(dev,&ssd130x_drm_driver,+structssd130x_device,drm);+if(IS_ERR(ssd130x)){+dev_err_probe(dev,PTR_ERR(ssd130x),+"Failed to allocate DRM device\n");+returnssd130x;+}++drm=&ssd130x->drm;++ssd130x->dev=dev;+ssd130x->regmap=regmap;+ssd130x->device_info=device_get_match_data(dev);++ssd130x_parse_properties(ssd130x);++ret=ssd130x_get_resources(ssd130x);+if(ret)+returnERR_PTR(ret);++bl=devm_backlight_device_register(dev,dev_name(dev),dev,ssd130x,+&ssd130xfb_bl_ops,NULL);+if(IS_ERR(bl)){+ret=PTR_ERR(bl);+dev_err_probe(dev,ret,"Unable to register backlight device\n");+returnERR_PTR(ret);+}++bl->props.brightness=ssd130x->contrast;+bl->props.max_brightness=MAX_CONTRAST;+ssd130x->bl_dev=bl;++ret=ssd130x_init_modeset(ssd130x);+if(ret)+returnERR_PTR(ret);++ret=drm_dev_register(drm,0);+if(ret){+dev_err_probe(dev,ret,"DRM device register failed\n");+returnERR_PTR(ret);+}++drm_fbdev_generic_setup(drm,0);++returnssd130x;+}+EXPORT_SYMBOL_GPL(ssd130x_probe);++intssd130x_remove(structssd130x_device*ssd130x)+{+drm_dev_unplug(&ssd130x->drm);++return0;+}+EXPORT_SYMBOL_GPL(ssd130x_remove);++voidssd130x_shutdown(structssd130x_device*ssd130x)+{+drm_atomic_helper_shutdown(&ssd130x->drm);+}+EXPORT_SYMBOL_GPL(ssd130x_shutdown);++MODULE_DESCRIPTION(DRIVER_DESC);+MODULE_AUTHOR("Javier Martinez Canillas <javierm@redhat.com>");+MODULE_LICENSE("GPL v2");
From: Javier Martinez Canillas <javierm@redhat.com> Date: 2022-02-11 09:22:27
To make sure that tools like the get_maintainer.pl script will suggest
to Cc me if patches are posted for this driver.
Also include the Device Tree binding for the old ssd1307fb fbdev driver
since the new DRM driver was made compatible with the existing binding.
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Sam Ravnborg <redacted>
---
(no changes since v3)
Changes in v3:
- Adapt MAINTAINERS entry to point to the new drivers/gpu/drm/solomon directory.
Changes in v2:
- Add Sam Ravnborg's acked-by to patch adding a MAINTAINERS entry (Sam Ravnborg)
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)
From: Javier Martinez Canillas <javierm@redhat.com> Date: 2022-02-11 09:23:06
The ssd130x DRM driver also makes use of this Device Tree binding to allow
existing users of the fbdev driver to migrate without the need to change
their Device Trees.
Add myself as another maintainer of the binding, to make sure that I will
be on Cc when patches are proposed for it.
Suggested-by: Sam Ravnborg <redacted>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Rob Herring <robh@kernel.org>
---
Changes in v4:
- Add Rob Herring Acked-by tag to patch adding as DT binding co-maintainer.
Changes in v2:
- Add myself as co-maintainer of the ssd1370fb DT binding (Sam Ravnborg).
Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml | 1 +
1 file changed, 1 insertion(+)
From: Thomas Zimmermann <tzimmermann@suse.de> Date: 2022-02-11 09:29:23
Am 11.02.22 um 10:19 schrieb Javier Martinez Canillas:
Pull the per-line conversion logic into a separate helper function.
This will allow to do line-by-line conversion in other helpers that
convert to a gray8 format.
Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
quoted hunk
---
(no changes since v3)
Changes in v3:
- Add a drm_fb_xrgb8888_to_gray8_line() helper function (Thomas Zimmermann)
drivers/gpu/drm/drm_format_helper.c | 31 ++++++++++++++++++-----------
1 file changed, 19 insertions(+), 12 deletions(-)
@@ -464,6 +464,21 @@ void drm_fb_xrgb8888_to_xrgb2101010_toio(void __iomem *dst,}EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010_toio);+staticvoiddrm_fb_xrgb8888_to_gray8_line(u8*dst,constu32*src,unsignedintpixels)+{+unsignedintx;++for(x=0;x<pixels;x++){+u8r=(*src&0x00ff0000)>>16;+u8g=(*src&0x0000ff00)>>8;+u8b=*src&0x000000ff;++/* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */+*dst++=(3*r+6*g+b)/10;+src++;+}+}+/***drm_fb_xrgb8888_to_gray8-ConvertXRGB8888tograyscale*@dst:8-bitgrayscaledestinationbuffer
@@ -508,16 +524,7 @@ void drm_fb_xrgb8888_to_gray8(void *dst, unsigned int dst_pitch, const void *vadfor(y=clip->y1;y<clip->y2;y++){dst8=dst;src32=memcpy(buf,vaddr,len);-for(x=clip->x1;x<clip->x2;x++){-u8r=(*src32&0x00ff0000)>>16;-u8g=(*src32&0x0000ff00)>>8;-u8b=*src32&0x000000ff;--/* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */-*dst8++=(3*r+6*g+b)/10;-src32++;-}-+drm_fb_xrgb8888_to_gray8_line(dst8,src32,linepixels);vaddr+=fb->pitches[0];dst+=dst_pitch;}
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2022-02-11 10:29:14
On Fri, Feb 11, 2022 at 10:19:22AM +0100, Javier Martinez Canillas wrote:
Pull the per-line conversion logic into a separate helper function.
This will allow to do line-by-line conversion in other helpers that
convert to a gray8 format.
...
+static void drm_fb_xrgb8888_to_gray8_line(u8 *dst, const u32 *src, unsigned int pixels)
+{
+ unsigned int x;
+
+ for (x = 0; x < pixels; x++) {
+ u8 r = (*src & 0x00ff0000) >> 16;
+ u8 g = (*src & 0x0000ff00) >> 8;
+ u8 b = *src & 0x000000ff;
+
+ /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
+ *dst++ = (3 * r + 6 * g + b) / 10;
+ src++;
+ }
Can be done as
while (pixels--) {
...
}
or
do {
...
} while (--pixels);
From: Javier Martinez Canillas <javierm@redhat.com> Date: 2022-02-11 10:41:05
Hello Andy,
On 2/11/22 11:28, Andy Shevchenko wrote:
On Fri, Feb 11, 2022 at 10:19:22AM +0100, Javier Martinez Canillas wrote:
quoted
Pull the per-line conversion logic into a separate helper function.
This will allow to do line-by-line conversion in other helpers that
convert to a gray8 format.
...
quoted
+static void drm_fb_xrgb8888_to_gray8_line(u8 *dst, const u32 *src, unsigned int pixels)
+{
+ unsigned int x;
+
+ for (x = 0; x < pixels; x++) {
+ u8 r = (*src & 0x00ff0000) >> 16;
+ u8 g = (*src & 0x0000ff00) >> 8;
+ u8 b = *src & 0x000000ff;
+
+ /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
+ *dst++ = (3 * r + 6 * g + b) / 10;
+ src++;
+ }
Can be done as
while (pixels--) {
...
}
or
do {
...
} while (--pixels);
I don't see why a while loop would be an improvement here TBH.
In any case, I just pulled the line conversion logic as a separate
function with minimal code changes since doing that should be in a
separate patch.
Feel free to post a patch if you want to change that while loop.
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2022-02-11 11:12:00
On Fri, Feb 11, 2022 at 10:19:23AM +0100, Javier Martinez Canillas wrote:
Add support to convert from XR24 to reversed monochrome for drivers that
control monochromatic display panels, that only have 1 bit per pixel.
The function does a line-by-line conversion doing an intermediate step
first from XR24 to 8-bit grayscale and then to reversed monochrome.
The drm_fb_gray8_to_mono_reversed_line() helper was based on code from
drivers/gpu/drm/tiny/repaper.c driver.
...
+static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, unsigned int pixels,
+ unsigned int start_offset, unsigned int end_len)
+{
+ unsigned int xb, i;
+
+ for (xb = 0; xb < pixels; xb++) {
+ unsigned int start = 0, end = 8;
+ u8 byte = 0x00;
+ if (xb == pixels - 1 && end_len)
+ end = end_len;
Ditto. However it may require to factor out the following loop to a helper.
+ for (i = start; i < end; i++) {
+ unsigned int x = xb * 8 + i;
+
+ byte >>= 1;
+ if (src[x] >> 7)
+ byte |= BIT(7);
+ }
+ *dst++ = byte;
+ }
+}
...
+ /*
+ * The reversed mono destination buffer contains 1 bit per pixel
+ * and destination scanlines have to be in multiple of 8 pixels.
+ */
+ if (!dst_pitch)
+ dst_pitch = DIV_ROUND_UP(linepixels, 8);
round_up() ?
+ WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n");
I would move this to the if conditional, i.e.
if (dst_pitch)
WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n");
else
dst_pitch = round_up(linepixels, 8);
+ /*
+ * The cma memory is write-combined so reads are uncached.
CMA
+ * Speed up by fetching one line at a time.
+ *
+ * Also, format conversion from XR24 to reversed monochrome
+ * are done line-by-line but are converted to 8-bit grayscale
+ * as an intermediate step.
+ *
+ * Allocate a buffer to be used for both copying from the cma
+ * memory and to store the intermediate grayscale line pixels.
+ */
+ src32 = kmalloc(len_src32 + linepixels, GFP_KERNEL);
size_add() ?
+ if (!src32)
+ return;
...
+ /*
+ * For damage handling, it is possible that only parts of the source
+ * buffer is copied and this could lead to start and end pixels that
+ * are not aligned to multiple of 8.
+ *
+ * Calculate if the start and end pixels are not aligned and set the
+ * offsets for the reversed mono line conversion function to adjust.
+ */
+ start_offset = clip->x1 % 8;
+ end_len = clip->x2 % 8;
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2022-02-11 11:35:03
On Fri, Feb 11, 2022 at 10:19:24AM +0100, Javier Martinez Canillas wrote:
This adds a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon
OLED display controllers.
It's only the core part of the driver and a bus specific driver is needed
for each transport interface supported by the display controllers.
bits.h
(FYI, specifically sent a patch few days ago to add explicitly the inclusions
that needed for bitfield operations in the example inside bitfield.h).
+ ret = ssd130x_write_cmd(ssd130x, 2, SSD130X_SET_COM_PINS_CONFIG, compins);
+ if (ret < 0)
+ return ret;
+
+
One blank line is enough.
...
+ for (i = y / 8; i < y / 8 + pages; i++) {
+ int m = 8;
+
+ /* Last page may be partial */
+ if (8 * (i + 1) > ssd130x->height)
+ m = ssd130x->height % 8;
Perhaps it can be moved out of the loop with refactored piece below.
+ for (j = x; j < x + width; j++) {
+ u8 data = 0;
+
+ for (k = 0; k < m; k++) {
+ u8 byte = buf[(8 * i + k) * line_length + j / 8];
+ u8 bit = (byte >> (j % 8)) & 1;
+
+ data |= bit << k;
+ }
+ data_array[array_idx++] = data;
+ }
+ }
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2022-02-11 11:35:54
On Fri, Feb 11, 2022 at 10:21:57AM +0100, Javier Martinez Canillas wrote:
To make sure that tools like the get_maintainer.pl script will suggest
to Cc me if patches are posted for this driver.
Also include the Device Tree binding for the old ssd1307fb fbdev driver
since the new DRM driver was made compatible with the existing binding.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
quoted hunk
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Sam Ravnborg <redacted>
---
(no changes since v3)
Changes in v3:
- Adapt MAINTAINERS entry to point to the new drivers/gpu/drm/solomon directory.
Changes in v2:
- Add Sam Ravnborg's acked-by to patch adding a MAINTAINERS entry (Sam Ravnborg)
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2022-02-11 11:36:05
On Fri, Feb 11, 2022 at 10:22:53AM +0100, Javier Martinez Canillas wrote:
The ssd130x DRM driver also makes use of this Device Tree binding to allow
existing users of the fbdev driver to migrate without the need to change
their Device Trees.
Add myself as another maintainer of the binding, to make sure that I will
be on Cc when patches are proposed for it.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
quoted hunk
Suggested-by: Sam Ravnborg <redacted>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Rob Herring <robh@kernel.org>
---
Changes in v4:
- Add Rob Herring Acked-by tag to patch adding as DT binding co-maintainer.
Changes in v2:
- Add myself as co-maintainer of the ssd1370fb DT binding (Sam Ravnborg).
Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml | 1 +
1 file changed, 1 insertion(+)
+ if (xb == pixels - 1 && end_len)
+ end = end_len;
Ditto. However it may require to factor out the following loop to a helper.
Not sure I'm following, it's not invariant since it depends on the
loop iterator value. It only applies to the first and last pixels.
[snip]
quoted
+ /*
+ * The reversed mono destination buffer contains 1 bit per pixel
+ * and destination scanlines have to be in multiple of 8 pixels.
+ */
+ if (!dst_pitch)
+ dst_pitch = DIV_ROUND_UP(linepixels, 8);
round_up() ?
But it's not a round up operation but a div and round up.
quoted
+ WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n");
I would move this to the if conditional, i.e.
if (dst_pitch)
WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n");
else
dst_pitch = round_up(linepixels, 8);
No, because we always need to div and round up. The warning is just printed to
let know that the dst pitch is not a multiple of 8 as it should be. So callers
could be fixed.
quoted
+ /*
+ * The cma memory is write-combined so reads are uncached.
CMA
Yes, this bug me too. But other format helpers (e.g: drm_fb_xrgb8888_to_rgb565
and drm_fb_xrgb8888_to_gray8) had this comment with CMA in lower case. So did
the same for consistency.
quoted
+ * Speed up by fetching one line at a time.
+ *
+ * Also, format conversion from XR24 to reversed monochrome
+ * are done line-by-line but are converted to 8-bit grayscale
+ * as an intermediate step.
+ *
+ * Allocate a buffer to be used for both copying from the cma
+ * memory and to store the intermediate grayscale line pixels.
+ */
+ src32 = kmalloc(len_src32 + linepixels, GFP_KERNEL);
size_add() ?
I wasn't familiar with this macro and git grep returned nothing. Then I noticed
that it is fairly new, introduced in commit a66866cff71c ("overflow: Implement
size_t saturating arithmetic helpers").
git tag --contains a66866cff71c | head -1
next-20220207
So I can't really use it since isn't yet in the latest drm-misc-next base tag :)
quoted
+ if (!src32)
+ return;
...
quoted
+ /*
+ * For damage handling, it is possible that only parts of the source
+ * buffer is copied and this could lead to start and end pixels that
+ * are not aligned to multiple of 8.
+ *
+ * Calculate if the start and end pixels are not aligned and set the
+ * offsets for the reversed mono line conversion function to adjust.
+ */
+ start_offset = clip->x1 % 8;
+ end_len = clip->x2 % 8;
ALIGN() ?
But we don't want to align here but to know what's the start and end if is
not aligned since that would mean converting to mono in the middle of a byte.
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
From: Thomas Zimmermann <tzimmermann@suse.de> Date: 2022-02-11 12:03:19
Hi
Am 11.02.22 um 12:10 schrieb Andy Shevchenko:
On Fri, Feb 11, 2022 at 10:19:23AM +0100, Javier Martinez Canillas wrote:
quoted
Add support to convert from XR24 to reversed monochrome for drivers that
control monochromatic display panels, that only have 1 bit per pixel.
The function does a line-by-line conversion doing an intermediate step
first from XR24 to 8-bit grayscale and then to reversed monochrome.
The drm_fb_gray8_to_mono_reversed_line() helper was based on code from
drivers/gpu/drm/tiny/repaper.c driver.
...
quoted
+static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, unsigned int pixels,
+ unsigned int start_offset, unsigned int end_len)
+{
+ unsigned int xb, i;
+
+ for (xb = 0; xb < pixels; xb++) {
+ unsigned int start = 0, end = 8;
+ u8 byte = 0x00;
+ if (xb == pixels - 1 && end_len)
+ end = end_len;
Ditto. However it may require to factor out the following loop to a helper.
Splitting the loop is much nicer, but leaves corner cases where start
and end is in the same byte. Doing this sounds like a premature
optimization to me.
Best regards
Thomas
quoted
+ for (i = start; i < end; i++) {
+ unsigned int x = xb * 8 + i;
+
+ byte >>= 1;
+ if (src[x] >> 7)
+ byte |= BIT(7);
+ }
+ *dst++ = byte;
+ }
+}
...
quoted
+ /*
+ * The reversed mono destination buffer contains 1 bit per pixel
+ * and destination scanlines have to be in multiple of 8 pixels.
+ */
+ if (!dst_pitch)
+ dst_pitch = DIV_ROUND_UP(linepixels, 8);
round_up() ?
quoted
+ WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n");
I would move this to the if conditional, i.e.
if (dst_pitch)
WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n");
else
dst_pitch = round_up(linepixels, 8);
quoted
+ /*
+ * The cma memory is write-combined so reads are uncached.
CMA
quoted
+ * Speed up by fetching one line at a time.
+ *
+ * Also, format conversion from XR24 to reversed monochrome
+ * are done line-by-line but are converted to 8-bit grayscale
+ * as an intermediate step.
+ *
+ * Allocate a buffer to be used for both copying from the cma
+ * memory and to store the intermediate grayscale line pixels.
+ */
+ src32 = kmalloc(len_src32 + linepixels, GFP_KERNEL);
size_add() ?
quoted
+ if (!src32)
+ return;
...
quoted
+ /*
+ * For damage handling, it is possible that only parts of the source
+ * buffer is copied and this could lead to start and end pixels that
+ * are not aligned to multiple of 8.
+ *
+ * Calculate if the start and end pixels are not aligned and set the
+ * offsets for the reversed mono line conversion function to adjust.
+ */
+ start_offset = clip->x1 % 8;
+ end_len = clip->x2 % 8;
ALIGN() ?
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
From: Javier Martinez Canillas <javierm@redhat.com> Date: 2022-02-11 12:06:09
On 2/11/22 12:33, Andy Shevchenko wrote:
On Fri, Feb 11, 2022 at 10:19:24AM +0100, Javier Martinez Canillas wrote:
quoted
This adds a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon
OLED display controllers.
It's only the core part of the driver and a bus specific driver is needed
for each transport interface supported by the display controllers.
Not really, the fbdev driver used it and I understood that was
a convention to denote that these are command options and not a
command or register. I'll drop them.
...
quoted
+/*
+ * Helper to write command (SSD130X_COMMAND). The fist variadic argument
+ * is the command to write and the following are the command options.
This is not correct explanation. Please, rephrase to show that _each_ of the
options is sent with a preceding command.
It's a correct explanation IMO from the caller point of view. The first argument
is the command sent (i.e: SSD130X_SET_ADDRESS_MODE) and the next ones are the
the command options (i.e: SSD130X_SET_ADDRESS_MODE_HORIZONTAL).
The fact that each command and options are preceding with a SSD130X_COMMAND
value is part of the protocol of the device and a detail that's abstracted
away by this helper function to the callers.
quoted
+ */
+static int ssd130x_write_cmd(struct ssd130x_device *ssd130x, int count,
+ /* u8 cmd, u8 option, ... */...)
+{
+ va_list ap;
+ u8 value;
+ int ret;
+
+ va_start(ap, count);
+
+ do {
+ value = va_arg(ap, int);
+ ret = regmap_write(ssd130x->regmap, SSD130X_COMMAND, (u8)value);
+ if (ret)
+ goto out_end;
+ } while (--count);
+
+out_end:
+ va_end(ap);
+
+ return ret;
+}
...
quoted
+ if (ssd130x->device_info->need_pwm) {
Yeah, unfortunately we still don't have pwm_get_optional()...
quoted
+ ret = ssd130x_pwm_enable(ssd130x);
+ if (ret) {
+ dev_err(dev, "Failed to enable PWM: %d\n", ret);
+ regulator_disable(ssd130x->vcc_reg);
+ return ret;
+ }
+ }
+ ret = ssd130x_write_cmd(ssd130x, 2, SSD130X_SET_COM_PINS_CONFIG, compins);
+ if (ret < 0)
+ return ret;
quoted
+
+
One blank line is enough.
Indeed, that was a left over when changing this to use the macros.
...
quoted
+ for (i = y / 8; i < y / 8 + pages; i++) {
+ int m = 8;
+
+ /* Last page may be partial */
+ if (8 * (i + 1) > ssd130x->height)
+ m = ssd130x->height % 8;
Perhaps it can be moved out of the loop with refactored piece below.
Not sure I'm following since it depends on the for loop iterator value.
[snip]
+ ret = PTR_ERR(bl);
+ dev_err_probe(dev, ret, "Unable to register backlight device\n");
+ return ERR_PTR(ret);
dev_err_probe(dev, PTR_ERR(bl), "Unable to register backlight device\n");
return bl;
?
No, because this function's return value is a struct ssd130x_device pointer,
not a struct backlight_device pointer.
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
From: Jani Nikula <jani.nikula@linux.intel.com> Date: 2022-02-11 12:06:19
On Fri, 11 Feb 2022, Thomas Zimmermann [off-list ref] wrote:
Hi
Am 11.02.22 um 12:12 schrieb Andy Shevchenko:
quoted
On Fri, Feb 11, 2022 at 11:40:13AM +0100, Javier Martinez Canillas wrote:
quoted
On 2/11/22 11:28, Andy Shevchenko wrote:
quoted
On Fri, Feb 11, 2022 at 10:19:22AM +0100, Javier Martinez Canillas wrote:
...
quoted
quoted
quoted
+static void drm_fb_xrgb8888_to_gray8_line(u8 *dst, const u32 *src, unsigned int pixels)
+{
+ unsigned int x;
+
+ for (x = 0; x < pixels; x++) {
+ u8 r = (*src & 0x00ff0000) >> 16;
+ u8 g = (*src & 0x0000ff00) >> 8;
+ u8 b = *src & 0x000000ff;
+
+ /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
+ *dst++ = (3 * r + 6 * g + b) / 10;
+ src++;
+ }
Can be done as
while (pixels--) {
...
}
or
do {
...
} while (--pixels);
I don't see why a while loop would be an improvement here TBH.
Less letters to parse when reading the code.
It's a simple refactoring of code that has worked well so far. Let's
leave it as-is for now.
IMO *always* prefer a for loop over while or do-while.
The for (i = 0; i < N; i++) is such a strong paradigm in C. You
instantly know how many times you're going to loop, at a glance. Not so
with with the alternatives, which should be used sparingly.
And yes, the do-while suggested above is buggy, and you actually need to
stop and think to see why.
BR,
Jani.
Best regards
Thomas
quoted
quoted
In any case, I just pulled the line conversion logic as a separate
function with minimal code changes since doing that should be in a
separate patch.
quoted
Feel free to post a patch if you want to change that while loop.
From: Javier Martinez Canillas <javierm@redhat.com> Date: 2022-02-11 12:11:35
Hello Jani,
On 2/11/22 13:05, Jani Nikula wrote:
[snip]
quoted
quoted
quoted
I don't see why a while loop would be an improvement here TBH.
Less letters to parse when reading the code.
It's a simple refactoring of code that has worked well so far. Let's
leave it as-is for now.
IMO *always* prefer a for loop over while or do-while.
The for (i = 0; i < N; i++) is such a strong paradigm in C. You
instantly know how many times you're going to loop, at a glance. Not so
with with the alternatives, which should be used sparingly.
And yes, the do-while suggested above is buggy, and you actually need to
stop and think to see why.
Absolutely agree.
These format conversion helpers are not trivial to read and understand (at
least for me). In my opinion the code should be written in a way that ease
readability and make as robust and less error prone as possible.
BR,
Jani.
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Hi Javier,
On Fri, Feb 11, 2022 at 1:06 PM Javier Martinez Canillas
[off-list ref] wrote:
On 2/11/22 12:33, Andy Shevchenko wrote:
quoted
On Fri, Feb 11, 2022 at 10:19:24AM +0100, Javier Martinez Canillas wrote:
quoted
This adds a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon
OLED display controllers.
It's only the core part of the driver and a bus specific driver is needed
for each transport interface supported by the display controllers.
+ ret = PTR_ERR(bl);
+ dev_err_probe(dev, ret, "Unable to register backlight device\n");
+ return ERR_PTR(ret);
dev_err_probe(dev, PTR_ERR(bl), "Unable to register backlight device\n");
return bl;
?
No, because this function's return value is a struct ssd130x_device pointer,
not a struct backlight_device pointer.
Hence
return ERR_PTR(dev_err_probe(dev, PTR_ERR(bl),
"Unable to register backlight device\n"));
?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Jani,
On Fri, Feb 11, 2022 at 1:06 PM Jani Nikula [off-list ref] wrote:
On Fri, 11 Feb 2022, Thomas Zimmermann [off-list ref] wrote:
quoted
Am 11.02.22 um 12:12 schrieb Andy Shevchenko:
quoted
On Fri, Feb 11, 2022 at 11:40:13AM +0100, Javier Martinez Canillas wrote:
quoted
On 2/11/22 11:28, Andy Shevchenko wrote:
quoted
On Fri, Feb 11, 2022 at 10:19:22AM +0100, Javier Martinez Canillas wrote:
...
quoted
quoted
quoted
+static void drm_fb_xrgb8888_to_gray8_line(u8 *dst, const u32 *src, unsigned int pixels)
+{
+ unsigned int x;
+
+ for (x = 0; x < pixels; x++) {
+ u8 r = (*src & 0x00ff0000) >> 16;
+ u8 g = (*src & 0x0000ff00) >> 8;
+ u8 b = *src & 0x000000ff;
+
+ /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
+ *dst++ = (3 * r + 6 * g + b) / 10;
+ src++;
+ }
Can be done as
while (pixels--) {
...
}
or
do {
...
} while (--pixels);
I don't see why a while loop would be an improvement here TBH.
Less letters to parse when reading the code.
It's a simple refactoring of code that has worked well so far. Let's
leave it as-is for now.
IMO *always* prefer a for loop over while or do-while.
(guess what ;-) I tend to disagree.
The for (i = 0; i < N; i++) is such a strong paradigm in C. You
instantly know how many times you're going to loop, at a glance. Not so
with with the alternatives, which should be used sparingly.
In this case it's fairly obvious, and you get rid of the extra variable x.
Less code, less variables, what can go wrong? ;-)
And yes, the do-while suggested above is buggy, and you actually need to
stop and think to see why.
Yes, that one is wrong.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
From: Thomas Zimmermann <tzimmermann@suse.de> Date: 2022-02-11 12:46:57
Hi
Am 11.02.22 um 10:19 schrieb Javier Martinez Canillas:
quoted hunk
Add support to convert from XR24 to reversed monochrome for drivers that
control monochromatic display panels, that only have 1 bit per pixel.
The function does a line-by-line conversion doing an intermediate step
first from XR24 to 8-bit grayscale and then to reversed monochrome.
The drm_fb_gray8_to_mono_reversed_line() helper was based on code from
drivers/gpu/drm/tiny/repaper.c driver.
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
---
Changes in v4:
- Rename end_offset to end_len (Thomas Zimmermann)
- Warn once if dst_pitch is not a multiple of 8 (Thomas Zimmermann)
- Drop drm_fb_gray8_to_mono_reversed() that's not used (Thomas Zimmermann)
- Allocate single buffer for both copy cma memory and gray8 (Thomas Zimmermann)
- Add Thomas Zimmermann Reviewed-by tag to patch adding XR24 -> mono helper.
Changes in v3:
- Also add a drm_fb_xrgb8888_to_mono_reversed() helper (Thomas Zimmermann)
- Split lines copy to drm_fb_gray8_to_mono_reversed_line() (Thomas Zimmermann)
- Handle case where the source buffer is not aligned to 8 (Thomas Zimmermann)
drivers/gpu/drm/drm_format_helper.c | 107 ++++++++++++++++++++++++++++
include/drm/drm_format_helper.h | 4 ++
2 files changed, 111 insertions(+)
@@ -591,3 +591,110 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_forreturn-EINVAL;}EXPORT_SYMBOL(drm_fb_blit_toio);++staticvoiddrm_fb_gray8_to_mono_reversed_line(u8*dst,constu8*src,unsignedintpixels,+unsignedintstart_offset,unsignedintend_len)+{+unsignedintxb,i;++for(xb=0;xb<pixels;xb++){+unsignedintstart=0,end=8;+u8byte=0x00;++if(xb==0&&start_offset)+start=start_offset;++if(xb==pixels-1&&end_len)+end=end_len;++for(i=start;i<end;i++){+unsignedintx=xb*8+i;++byte>>=1;+if(src[x]>>7)+byte|=BIT(7);+}+*dst++=byte;+}+}++/**+*drm_fb_xrgb8888_to_mono_reversed-ConvertXRGB8888toreversedmonochrome+*@dst:reversedmonochromedestinationbuffer+*@dst_pitch:Numberofbytesbetweentwoconsecutivescanlineswithindst+*@src:XRGB8888sourcebuffer+*@fb:DRMframebuffer+*@clip:Cliprectangleareatocopy+*+*DRMdoesn'thavenativemonochromesupport.+*SuchdriverscanannouncethecommonlysupportedXR24formattouserspace+*andusethisfunctiontoconverttothenativeformat.+*+*Thisfunctionusesdrm_fb_xrgb8888_to_gray8()toconverttograyscaleand+*thentheresultisconvertedfromgrayscaletoreversedmonohrome.+*/+voiddrm_fb_xrgb8888_to_mono_reversed(void*dst,unsignedintdst_pitch,constvoid*vaddr,+conststructdrm_framebuffer*fb,conststructdrm_rect*clip)+{+unsignedintlinepixels=drm_rect_width(clip);+unsignedintlines=clip->y2-clip->y1;+unsignedintcpp=fb->format->cpp[0];+unsignedintlen_src32=linepixels*cpp;+unsignedintstart_offset,end_len;+unsignedinty;+u8*mono=dst,*gray8;+u32*src32;++if(WARN_ON(fb->format->format!=DRM_FORMAT_XRGB8888))+return;
These WARN macros are deprecated. Use drm_warn, drm_WARN_ONCE, etc instead.
Best regards
Thomas
quoted hunk
+
+ /*
+ * The reversed mono destination buffer contains 1 bit per pixel
+ * and destination scanlines have to be in multiple of 8 pixels.
+ */
+ if (!dst_pitch)
+ dst_pitch = DIV_ROUND_UP(linepixels, 8);
+
+ WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n");
+
+ /*
+ * The cma memory is write-combined so reads are uncached.
+ * Speed up by fetching one line at a time.
+ *
+ * Also, format conversion from XR24 to reversed monochrome
+ * are done line-by-line but are converted to 8-bit grayscale
+ * as an intermediate step.
+ *
+ * Allocate a buffer to be used for both copying from the cma
+ * memory and to store the intermediate grayscale line pixels.
+ */
+ src32 = kmalloc(len_src32 + linepixels, GFP_KERNEL);
+ if (!src32)
+ return;
+
+ gray8 = (u8 *)src32 + len_src32;
+
+ /*
+ * For damage handling, it is possible that only parts of the source
+ * buffer is copied and this could lead to start and end pixels that
+ * are not aligned to multiple of 8.
+ *
+ * Calculate if the start and end pixels are not aligned and set the
+ * offsets for the reversed mono line conversion function to adjust.
+ */
+ start_offset = clip->x1 % 8;
+ end_len = clip->x2 % 8;
+
+ vaddr += clip_offset(clip, fb->pitches[0], cpp);
+ for (y = 0; y < lines; y++) {
+ src32 = memcpy(src32, vaddr, len_src32);
+ drm_fb_xrgb8888_to_gray8_line(gray8, src32, linepixels);
+ drm_fb_gray8_to_mono_reversed_line(mono, gray8, dst_pitch,
+ start_offset, end_len);
+ vaddr += fb->pitches[0];
+ mono += dst_pitch;
+ }
+
+ kfree(src32);
+}
+EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono_reversed);
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2022-02-11 15:42:08
On Fri, Feb 11, 2022 at 02:05:56PM +0200, Jani Nikula wrote:
On Fri, 11 Feb 2022, Thomas Zimmermann [off-list ref] wrote:
quoted
Am 11.02.22 um 12:12 schrieb Andy Shevchenko:
quoted
On Fri, Feb 11, 2022 at 11:40:13AM +0100, Javier Martinez Canillas wrote:
quoted
On 2/11/22 11:28, Andy Shevchenko wrote:
quoted
On Fri, Feb 11, 2022 at 10:19:22AM +0100, Javier Martinez Canillas wrote:
...
quoted
quoted
quoted
quoted
quoted
+static void drm_fb_xrgb8888_to_gray8_line(u8 *dst, const u32 *src, unsigned int pixels)
+{
+ unsigned int x;
+
+ for (x = 0; x < pixels; x++) {
+ u8 r = (*src & 0x00ff0000) >> 16;
+ u8 g = (*src & 0x0000ff00) >> 8;
+ u8 b = *src & 0x000000ff;
+
+ /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
+ *dst++ = (3 * r + 6 * g + b) / 10;
+ src++;
+ }
Can be done as
while (pixels--) {
...
}
or
do {
...
} while (--pixels);
I don't see why a while loop would be an improvement here TBH.
Less letters to parse when reading the code.
It's a simple refactoring of code that has worked well so far. Let's
leave it as-is for now.
IMO *always* prefer a for loop over while or do-while.
The for (i = 0; i < N; i++) is such a strong paradigm in C. You
instantly know how many times you're going to loop, at a glance. Not so
with with the alternatives, which should be used sparingly.
while () {} _is_ a paradigm, for-loop is syntax sugar on top of it.
And yes, the do-while suggested above is buggy, and you actually need to
stop and think to see why.
It depends if pixels can be 0 or not and if it's not, then does it contain last
or number.
The do {} while (--pixels); might be buggy iff pixels may be 0.
--
With Best Regards,
Andy Shevchenko
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2022-02-11 15:50:14
On Fri, Feb 11, 2022 at 01:05:57PM +0100, Javier Martinez Canillas wrote:
On 2/11/22 12:33, Andy Shevchenko wrote:
quoted
On Fri, Feb 11, 2022 at 10:19:24AM +0100, Javier Martinez Canillas wrote:
...
quoted
quoted
+ * Helper to write command (SSD130X_COMMAND). The fist variadic argument
+ * is the command to write and the following are the command options.
This is not correct explanation. Please, rephrase to show that _each_ of the
options is sent with a preceding command.
It's a correct explanation IMO from the caller point of view. The first argument
is the command sent (i.e: SSD130X_SET_ADDRESS_MODE) and the next ones are the
the command options (i.e: SSD130X_SET_ADDRESS_MODE_HORIZONTAL).
The fact that each command and options are preceding with a SSD130X_COMMAND
value is part of the protocol of the device and a detail that's abstracted
away by this helper function to the callers.
My previous suggestion about bulk transaction was purely based on this
(misinterpreted) description. Can we make sure somehow that another reader
don't trap into the same?
...
+ if (xb == pixels - 1 && end_len)
+ end = end_len;
Ditto. However it may require to factor out the following loop to a helper.
Not sure I'm following, it's not invariant since it depends on the
loop iterator value. It only applies to the first and last pixels.
It's. You simply does it at the last iteration which may be perfectly done
outside of the main (aligned) loop.
...
quoted
quoted
+ dst_pitch = DIV_ROUND_UP(linepixels, 8);
round_up() ?
But it's not a round up operation but a div and round up.
Indeed.
...
quoted
quoted
+ WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n");
I would move this to the if conditional, i.e.
if (dst_pitch)
WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n");
else
dst_pitch = round_up(linepixels, 8);
No, because we always need to div and round up. The warning is just printed to
let know that the dst pitch is not a multiple of 8 as it should be. So callers
could be fixed.
Okay, you expect that linepixels to be multiple of 64? Otherwise I didn't get
what's going on with this warning.
...
But we don't want to align here but to know what's the start and end if is
not aligned since that would mean converting to mono in the middle of a byte.
Indeed. Somehow I missed that it's a complimentary to ALIGN().
--
With Best Regards,
Andy Shevchenko
From: Jani Nikula <jani.nikula@linux.intel.com> Date: 2022-02-11 16:25:27
On Fri, 11 Feb 2022, Andy Shevchenko [off-list ref] wrote:
On Fri, Feb 11, 2022 at 02:05:56PM +0200, Jani Nikula wrote:
quoted
On Fri, 11 Feb 2022, Thomas Zimmermann [off-list ref] wrote:
quoted
Am 11.02.22 um 12:12 schrieb Andy Shevchenko:
quoted
On Fri, Feb 11, 2022 at 11:40:13AM +0100, Javier Martinez Canillas wrote:
quoted
On 2/11/22 11:28, Andy Shevchenko wrote:
quoted
On Fri, Feb 11, 2022 at 10:19:22AM +0100, Javier Martinez Canillas wrote:
...
quoted
quoted
quoted
quoted
quoted
quoted
+static void drm_fb_xrgb8888_to_gray8_line(u8 *dst, const u32 *src, unsigned int pixels)
+{
+ unsigned int x;
+
+ for (x = 0; x < pixels; x++) {
+ u8 r = (*src & 0x00ff0000) >> 16;
+ u8 g = (*src & 0x0000ff00) >> 8;
+ u8 b = *src & 0x000000ff;
+
+ /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
+ *dst++ = (3 * r + 6 * g + b) / 10;
+ src++;
+ }
Can be done as
while (pixels--) {
...
}
or
do {
...
} while (--pixels);
I don't see why a while loop would be an improvement here TBH.
Less letters to parse when reading the code.
It's a simple refactoring of code that has worked well so far. Let's
leave it as-is for now.
IMO *always* prefer a for loop over while or do-while.
The for (i = 0; i < N; i++) is such a strong paradigm in C. You
instantly know how many times you're going to loop, at a glance. Not so
with with the alternatives, which should be used sparingly.
while () {} _is_ a paradigm, for-loop is syntax sugar on top of it.
And while() is just syntax sugar for goto. :p
The for loop written as for (i = 0; i < N; i++) is hands down the most
obvious counting loop pattern there is in C.
quoted
And yes, the do-while suggested above is buggy, and you actually need to
stop and think to see why.
It depends if pixels can be 0 or not and if it's not, then does it contain last
or number.
The do {} while (--pixels); might be buggy iff pixels may be 0.
Yeah. And how long does it take to figure that out?
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2022-02-11 17:28:19
On Fri, Feb 11, 2022 at 06:25:17PM +0200, Jani Nikula wrote:
On Fri, 11 Feb 2022, Andy Shevchenko [off-list ref] wrote:
quoted
On Fri, Feb 11, 2022 at 02:05:56PM +0200, Jani Nikula wrote:
quoted
On Fri, 11 Feb 2022, Thomas Zimmermann [off-list ref] wrote:
quoted
Am 11.02.22 um 12:12 schrieb Andy Shevchenko:
quoted
On Fri, Feb 11, 2022 at 11:40:13AM +0100, Javier Martinez Canillas wrote:
quoted
On 2/11/22 11:28, Andy Shevchenko wrote:
quoted
On Fri, Feb 11, 2022 at 10:19:22AM +0100, Javier Martinez Canillas wrote:
...
quoted
quoted
quoted
quoted
quoted
quoted
+static void drm_fb_xrgb8888_to_gray8_line(u8 *dst, const u32 *src, unsigned int pixels)
+{
+ unsigned int x;
+
+ for (x = 0; x < pixels; x++) {
+ u8 r = (*src & 0x00ff0000) >> 16;
+ u8 g = (*src & 0x0000ff00) >> 8;
+ u8 b = *src & 0x000000ff;
+
+ /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
+ *dst++ = (3 * r + 6 * g + b) / 10;
+ src++;
+ }
Can be done as
while (pixels--) {
...
}
or
do {
...
} while (--pixels);
I don't see why a while loop would be an improvement here TBH.
Less letters to parse when reading the code.
It's a simple refactoring of code that has worked well so far. Let's
leave it as-is for now.
IMO *always* prefer a for loop over while or do-while.
The for (i = 0; i < N; i++) is such a strong paradigm in C. You
instantly know how many times you're going to loop, at a glance. Not so
with with the alternatives, which should be used sparingly.
while () {} _is_ a paradigm, for-loop is syntax sugar on top of it.
And while() is just syntax sugar for goto. :p
The for loop written as for (i = 0; i < N; i++) is hands down the most
obvious counting loop pattern there is in C.
quoted
quoted
And yes, the do-while suggested above is buggy, and you actually need to
stop and think to see why.
It depends if pixels can be 0 or not and if it's not, then does it contain last
or number.
The do {} while (--pixels); might be buggy iff pixels may be 0.
Yeah. And how long does it take to figure that out?
Okay, I made a mistake to drop the explanation. So, I (mistakenly) assumed
that people know this difference between post-decrement and pre-decrement
(note, while-loop here is not what is problematic).
--
With Best Regards,
Andy Shevchenko
From: Thomas Zimmermann <tzimmermann@suse.de> Date: 2022-02-14 09:04:00
Hi
Am 11.02.22 um 16:41 schrieb Andy Shevchenko:
[...]
quoted
IMO *always* prefer a for loop over while or do-while.
The for (i = 0; i < N; i++) is such a strong paradigm in C. You
instantly know how many times you're going to loop, at a glance. Not so
with with the alternatives, which should be used sparingly.
while () {} _is_ a paradigm, for-loop is syntax sugar on top of it.
Naw, that's not true. An idiomatic for loop, such as for (i = ...; i <
N; ++i), is such a strong pattern that it's way better than the
corresponding while loop.
Best regards
Thomas
quoted
And yes, the do-while suggested above is buggy, and you actually need to
stop and think to see why.
It depends if pixels can be 0 or not and if it's not, then does it contain last
or number.
The do {} while (--pixels); might be buggy iff pixels may be 0.
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
From: Pekka Paalanen <ppaalanen@gmail.com> Date: 2022-02-14 09:17:23
On Fri, 11 Feb 2022 19:27:12 +0200
Andy Shevchenko [off-list ref] wrote:
On Fri, Feb 11, 2022 at 06:25:17PM +0200, Jani Nikula wrote:
quoted
On Fri, 11 Feb 2022, Andy Shevchenko [off-list ref] wrote:
quoted
On Fri, Feb 11, 2022 at 02:05:56PM +0200, Jani Nikula wrote:
quoted
On Fri, 11 Feb 2022, Thomas Zimmermann [off-list ref] wrote:
quoted
Am 11.02.22 um 12:12 schrieb Andy Shevchenko:
quoted
On Fri, Feb 11, 2022 at 11:40:13AM +0100, Javier Martinez Canillas wrote:
quoted
On 2/11/22 11:28, Andy Shevchenko wrote:
quoted
On Fri, Feb 11, 2022 at 10:19:22AM +0100, Javier Martinez Canillas wrote:
...
quoted
quoted
quoted
quoted
quoted
quoted
+static void drm_fb_xrgb8888_to_gray8_line(u8 *dst, const u32 *src, unsigned int pixels)
+{
+ unsigned int x;
+
+ for (x = 0; x < pixels; x++) {
+ u8 r = (*src & 0x00ff0000) >> 16;
+ u8 g = (*src & 0x0000ff00) >> 8;
+ u8 b = *src & 0x000000ff;
+
+ /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
+ *dst++ = (3 * r + 6 * g + b) / 10;
+ src++;
+ }
Can be done as
while (pixels--) {
...
}
or
do {
...
} while (--pixels);
I don't see why a while loop would be an improvement here TBH.
Less letters to parse when reading the code.
It's a simple refactoring of code that has worked well so far. Let's
leave it as-is for now.
IMO *always* prefer a for loop over while or do-while.
The for (i = 0; i < N; i++) is such a strong paradigm in C. You
instantly know how many times you're going to loop, at a glance. Not so
with with the alternatives, which should be used sparingly.
while () {} _is_ a paradigm, for-loop is syntax sugar on top of it.
And while() is just syntax sugar for goto. :p
The for loop written as for (i = 0; i < N; i++) is hands down the most
obvious counting loop pattern there is in C.
quoted
quoted
And yes, the do-while suggested above is buggy, and you actually need to
stop and think to see why.
It depends if pixels can be 0 or not and if it's not, then does it contain last
or number.
The do {} while (--pixels); might be buggy iff pixels may be 0.
Yeah. And how long does it take to figure that out?
Okay, I made a mistake to drop the explanation. So, I (mistakenly) assumed
that people know this difference between post-decrement and pre-decrement
(note, while-loop here is not what is problematic).
That was not the question.
The question was, how long does it take to figure out if pixels can or
cannot be zero?
Code is styled for humans other than the author, not for compilers.
Having to stop to think about the difference between post- and
pre-decrement to figure out when the while-loop runs does take me a few
more brain cycles to understand, even though I know the rules very well.
I would call that brain cycle optimization, and leave the CPU cycle
optimization for the compiler in these cases.
Thanks,
pq
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2022-02-14 11:00:23
On Mon, Feb 14, 2022 at 11:17:11AM +0200, Pekka Paalanen wrote:
On Fri, 11 Feb 2022 19:27:12 +0200
Andy Shevchenko [off-list ref] wrote:
quoted
On Fri, Feb 11, 2022 at 06:25:17PM +0200, Jani Nikula wrote:
quoted
On Fri, 11 Feb 2022, Andy Shevchenko [off-list ref] wrote:
quoted
On Fri, Feb 11, 2022 at 02:05:56PM +0200, Jani Nikula wrote:
quoted
On Fri, 11 Feb 2022, Thomas Zimmermann [off-list ref] wrote:
quoted
Am 11.02.22 um 12:12 schrieb Andy Shevchenko:
quoted
On Fri, Feb 11, 2022 at 11:40:13AM +0100, Javier Martinez Canillas wrote:
quoted
On 2/11/22 11:28, Andy Shevchenko wrote:
quoted
On Fri, Feb 11, 2022 at 10:19:22AM +0100, Javier Martinez Canillas wrote:
...
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
+static void drm_fb_xrgb8888_to_gray8_line(u8 *dst, const u32 *src, unsigned int pixels)
+{
+ unsigned int x;
+
+ for (x = 0; x < pixels; x++) {
+ u8 r = (*src & 0x00ff0000) >> 16;
+ u8 g = (*src & 0x0000ff00) >> 8;
+ u8 b = *src & 0x000000ff;
+
+ /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
+ *dst++ = (3 * r + 6 * g + b) / 10;
+ src++;
+ }
Can be done as
while (pixels--) {
...
}
or
do {
...
} while (--pixels);
I don't see why a while loop would be an improvement here TBH.
Less letters to parse when reading the code.
It's a simple refactoring of code that has worked well so far. Let's
leave it as-is for now.
IMO *always* prefer a for loop over while or do-while.
The for (i = 0; i < N; i++) is such a strong paradigm in C. You
instantly know how many times you're going to loop, at a glance. Not so
with with the alternatives, which should be used sparingly.
while () {} _is_ a paradigm, for-loop is syntax sugar on top of it.
And while() is just syntax sugar for goto. :p
The for loop written as for (i = 0; i < N; i++) is hands down the most
obvious counting loop pattern there is in C.
quoted
quoted
And yes, the do-while suggested above is buggy, and you actually need to
stop and think to see why.
It depends if pixels can be 0 or not and if it's not, then does it contain last
or number.
The do {} while (--pixels); might be buggy iff pixels may be 0.
Yeah. And how long does it take to figure that out?
Okay, I made a mistake to drop the explanation. So, I (mistakenly) assumed
that people know this difference between post-decrement and pre-decrement
(note, while-loop here is not what is problematic).
That was not the question.
The question was, how long does it take to figure out if pixels can or
cannot be zero?
To me these patterns, while() {} and do {} while(), while being shorter,
also give a hint. So if one is familiar with C, the do {} while (--foo)
_gives a hint_ while being shorter. It requires _less_ brain power to get
this.
But I assume my brain is unique and not working as million of others.
Code is styled for humans other than the author, not for compilers.
Having to stop to think about the difference between post- and
pre-decrement to figure out when the while-loop runs does take me a few
more brain cycles to understand, even though I know the rules very well.
I would call that brain cycle optimization, and leave the CPU cycle
optimization for the compiler in these cases.
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2022-02-14 11:12:42
On Mon, Feb 14, 2022 at 10:03:53AM +0100, Thomas Zimmermann wrote:
Am 11.02.22 um 16:41 schrieb Andy Shevchenko:
...
quoted
quoted
IMO *always* prefer a for loop over while or do-while.
The for (i = 0; i < N; i++) is such a strong paradigm in C. You
instantly know how many times you're going to loop, at a glance. Not so
with with the alternatives, which should be used sparingly.
while () {} _is_ a paradigm, for-loop is syntax sugar on top of it.
Naw, that's not true.
In the section 3.5 "Loops - While and For" in "The C Programming
Language" 2nd by K&R, the authors said:
The for statement ... is equivalent to ... while..."
They said that for is equivalent to while, and not otherwise.
Also, syntax sugar by definition declares something that can be written as
a single line of code, which usually is done using more (not always).
An idiomatic for loop, such as for (i = ...; i < N;
++i), is such a strong pattern that it's way better than the corresponding
while loop.
quoted
quoted
And yes, the do-while suggested above is buggy, and you actually need to
stop and think to see why.
It depends if pixels can be 0 or not and if it's not, then does it contain last
or number.
The do {} while (--pixels); might be buggy iff pixels may be 0.
Hi Andy,
On Mon, Feb 14, 2022 at 11:39 AM Andy Shevchenko
[off-list ref] wrote:
On Mon, Feb 14, 2022 at 10:03:53AM +0100, Thomas Zimmermann wrote:
quoted
Am 11.02.22 um 16:41 schrieb Andy Shevchenko:
quoted
quoted
IMO *always* prefer a for loop over while or do-while.
The for (i = 0; i < N; i++) is such a strong paradigm in C. You
instantly know how many times you're going to loop, at a glance. Not so
with with the alternatives, which should be used sparingly.
while () {} _is_ a paradigm, for-loop is syntax sugar on top of it.
Naw, that's not true.
In the section 3.5 "Loops - While and For" in "The C Programming
Language" 2nd by K&R, the authors said:
The for statement ... is equivalent to ... while..."
They said that for is equivalent to while, and not otherwise.
When I learned C, people told me to prefer while() over for() when
possible, as several compilers are better at optimizing while()-loops
than for()-loops.
During the last 3 decades, optimizers got better, and all the bad
old compilers went the way of the dodo (see also [1])...
But even for a human, it's still less symbols to decode (and verify
all the details about =/</>/<=/>=/++/--/...) for
while (n--) { ... }
than for
for (i = 0; i < n; i++) { ... }
[1] https://lwn.net/Articles/871283/
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
From: Simon Ser <hidden> Date: 2022-02-14 11:25:24
On Monday, February 14th, 2022 at 11:38, Andy Shevchenko [off-list ref] wrote:
quoted
quoted
quoted
IMO *always* prefer a for loop over while or do-while.
The for (i = 0; i < N; i++) is such a strong paradigm in C. You
instantly know how many times you're going to loop, at a glance. Not so
with with the alternatives, which should be used sparingly.
while () {} _is_ a paradigm, for-loop is syntax sugar on top of it.
Naw, that's not true.
In the section 3.5 "Loops - While and For" in "The C Programming
Language" 2nd by K&R, the authors said:
The for statement ... is equivalent to ... while..."
They said that for is equivalent to while, and not otherwise.
Also, syntax sugar by definition declares something that can be written as
a single line of code, which usually is done using more (not always).
arr[i] is syntaxic sugar for *(arr + i), yet we keep writing the former,
because it's way more readable. The same goes for the for vs. while loops.
It may be obvious for you because you're a C guru, but to me it just obfuscates
the code. Too many C projects end up becoming completely unreadable because of
patterns like these.
Idiomatic C code isn't written by doing pointless micro-optimizations.
From: Thomas Zimmermann <tzimmermann@suse.de> Date: 2022-02-14 12:12:54
Hi
Am 14.02.22 um 11:38 schrieb Andy Shevchenko:
On Mon, Feb 14, 2022 at 10:03:53AM +0100, Thomas Zimmermann wrote:
quoted
Am 11.02.22 um 16:41 schrieb Andy Shevchenko:
...
quoted
quoted
quoted
IMO *always* prefer a for loop over while or do-while.
The for (i = 0; i < N; i++) is such a strong paradigm in C. You
instantly know how many times you're going to loop, at a glance. Not so
with with the alternatives, which should be used sparingly.
while () {} _is_ a paradigm, for-loop is syntax sugar on top of it.
Naw, that's not true.
In the section 3.5 "Loops - While and For" in "The C Programming
Language" 2nd by K&R, the authors said:
Year of publication: 1988 . It's not the most up-to-date reference for C
programming.
The for statement ... is equivalent to ... while..."
They said that for is equivalent to while, and not otherwise.
Even leaving readability aside, it's not equivalent. You can declare
variables as part of the for statement. (I know it's not the kernel's
style.) Also, 'continue' statements are not well-suited in for loops,
because it's non-obvious if the loop's update statement is being
executed. (It isn't.)
Also, syntax sugar by definition declares something that can be written as
a single line of code, which usually is done using more (not always).
The discussion has entered the phase of hair splitting. Good.
Best regards
Thomas
quoted
An idiomatic for loop, such as for (i = ...; i < N;
++i), is such a strong pattern that it's way better than the corresponding
while loop.
quoted
quoted
quoted
And yes, the do-while suggested above is buggy, and you actually need to
stop and think to see why.
It depends if pixels can be 0 or not and if it's not, then does it contain last
or number.
The do {} while (--pixels); might be buggy iff pixels may be 0.
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
From: Ville Syrjälä <hidden> Date: 2022-02-14 12:47:15
On Mon, Feb 14, 2022 at 01:12:48PM +0100, Thomas Zimmermann wrote:
Hi
Am 14.02.22 um 11:38 schrieb Andy Shevchenko:
quoted
On Mon, Feb 14, 2022 at 10:03:53AM +0100, Thomas Zimmermann wrote:
quoted
Am 11.02.22 um 16:41 schrieb Andy Shevchenko:
...
quoted
quoted
quoted
IMO *always* prefer a for loop over while or do-while.
The for (i = 0; i < N; i++) is such a strong paradigm in C. You
instantly know how many times you're going to loop, at a glance. Not so
with with the alternatives, which should be used sparingly.
while () {} _is_ a paradigm, for-loop is syntax sugar on top of it.
Naw, that's not true.
In the section 3.5 "Loops - While and For" in "The C Programming
Language" 2nd by K&R, the authors said:
Year of publication: 1988 . It's not the most up-to-date reference for C
programming.
quoted
The for statement ... is equivalent to ... while..."
They said that for is equivalent to while, and not otherwise.
Even leaving readability aside, it's not equivalent. You can declare
variables as part of the for statement. (I know it's not the kernel's
style.) Also, 'continue' statements are not well-suited in for loops,
because it's non-obvious if the loop's update statement is being
executed. (It isn't.)
It is.
'continue' is just shorthand for 'goto end_of_loop_body'.
--
Ville Syrjälä
Intel
From: Thomas Zimmermann <tzimmermann@suse.de> Date: 2022-02-14 12:55:12
Hi
Am 14.02.22 um 13:47 schrieb Ville Syrjälä:
On Mon, Feb 14, 2022 at 01:12:48PM +0100, Thomas Zimmermann wrote:
quoted
Hi
Am 14.02.22 um 11:38 schrieb Andy Shevchenko:
quoted
On Mon, Feb 14, 2022 at 10:03:53AM +0100, Thomas Zimmermann wrote:
quoted
Am 11.02.22 um 16:41 schrieb Andy Shevchenko:
...
quoted
quoted
quoted
IMO *always* prefer a for loop over while or do-while.
The for (i = 0; i < N; i++) is such a strong paradigm in C. You
instantly know how many times you're going to loop, at a glance. Not so
with with the alternatives, which should be used sparingly.
while () {} _is_ a paradigm, for-loop is syntax sugar on top of it.
Naw, that's not true.
In the section 3.5 "Loops - While and For" in "The C Programming
Language" 2nd by K&R, the authors said:
Year of publication: 1988 . It's not the most up-to-date reference for C
programming.
quoted
The for statement ... is equivalent to ... while..."
They said that for is equivalent to while, and not otherwise.
Even leaving readability aside, it's not equivalent. You can declare
variables as part of the for statement. (I know it's not the kernel's
style.) Also, 'continue' statements are not well-suited in for loops,
because it's non-obvious if the loop's update statement is being
executed. (It isn't.)
It is.
'continue' is just shorthand for 'goto end_of_loop_body'.
Well, indeed. lol
Fun fact: I actually had to look this up and still got it wrong. Let me
just count it under proving-my-point: continue in a for statement is a
bad idea and for isn't equivalent to while.
Best regards
Thomas
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
From: Ville Syrjälä <hidden> Date: 2022-02-14 13:07:18
On Mon, Feb 14, 2022 at 01:54:59PM +0100, Thomas Zimmermann wrote:
Hi
Am 14.02.22 um 13:47 schrieb Ville Syrjälä:
quoted
On Mon, Feb 14, 2022 at 01:12:48PM +0100, Thomas Zimmermann wrote:
quoted
Hi
Am 14.02.22 um 11:38 schrieb Andy Shevchenko:
quoted
On Mon, Feb 14, 2022 at 10:03:53AM +0100, Thomas Zimmermann wrote:
quoted
Am 11.02.22 um 16:41 schrieb Andy Shevchenko:
...
quoted
quoted
quoted
IMO *always* prefer a for loop over while or do-while.
The for (i = 0; i < N; i++) is such a strong paradigm in C. You
instantly know how many times you're going to loop, at a glance. Not so
with with the alternatives, which should be used sparingly.
while () {} _is_ a paradigm, for-loop is syntax sugar on top of it.
Naw, that's not true.
In the section 3.5 "Loops - While and For" in "The C Programming
Language" 2nd by K&R, the authors said:
Year of publication: 1988 . It's not the most up-to-date reference for C
programming.
quoted
The for statement ... is equivalent to ... while..."
They said that for is equivalent to while, and not otherwise.
Even leaving readability aside, it's not equivalent. You can declare
variables as part of the for statement. (I know it's not the kernel's
style.) Also, 'continue' statements are not well-suited in for loops,
because it's non-obvious if the loop's update statement is being
executed. (It isn't.)
It is.
'continue' is just shorthand for 'goto end_of_loop_body'.
Well, indeed. lol
Fun fact: I actually had to look this up and still got it wrong. Let me
just count it under proving-my-point: continue in a for statement is a
bad idea and for isn't equivalent to while.
Nah. We use 'continue' a *lot* in for loops in kms/atomic code.
I'd be surprised if you can find many loops without a 'continue'.
Looking at the loc stats I was a bit surprised to see more 'break'
but then I realized switch() is bloating up those numbers quite
a bit.
--
Ville Syrjälä
Intel
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2022-02-14 14:00:28
On Mon, Feb 14, 2022 at 01:12:48PM +0100, Thomas Zimmermann wrote:
Am 14.02.22 um 11:38 schrieb Andy Shevchenko:
quoted
On Mon, Feb 14, 2022 at 10:03:53AM +0100, Thomas Zimmermann wrote:
quoted
Am 11.02.22 um 16:41 schrieb Andy Shevchenko:
...
quoted
quoted
quoted
quoted
IMO *always* prefer a for loop over while or do-while.
The for (i = 0; i < N; i++) is such a strong paradigm in C. You
instantly know how many times you're going to loop, at a glance. Not so
with with the alternatives, which should be used sparingly.
while () {} _is_ a paradigm, for-loop is syntax sugar on top of it.
Naw, that's not true.
In the section 3.5 "Loops - While and For" in "The C Programming
Language" 2nd by K&R, the authors said:
Year of publication: 1988 . It's not the most up-to-date reference for C
programming.
Yet this makes your above remark invalid, i.e. `for` _is_ syntax sugar despite
what you think it's idiomatic _nowadays_.
quoted
The for statement ... is equivalent to ... while..."
They said that for is equivalent to while, and not otherwise.
Even leaving readability aside, it's not equivalent. You can declare
variables as part of the for statement. (I know it's not the kernel's
style.) Also, 'continue' statements are not well-suited in for loops,
because it's non-obvious if the loop's update statement is being executed.
(It isn't.)
It's also written in the book :-)
quoted
Also, syntax sugar by definition declares something that can be written as
a single line of code, which usually is done using more (not always).
The discussion has entered the phase of hair splitting. Good.
I don't know why we are adding an oil into the flames...
--
With Best Regards,
Andy Shevchenko