linguini1 commented on code in PR #18301: URL: https://github.com/apache/nuttx/pull/18301#discussion_r2756281179
########## include/nuttx/lcd/st7796.h: ########## @@ -0,0 +1,242 @@ +/**************************************************************************** + * include/nuttx/lcd/st7796.h + * + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +#ifndef __INCLUDE_NUTTX_LCD_ST7796_H +#define __INCLUDE_NUTTX_LCD_ST7796_H + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <nuttx/config.h> +#include <stdint.h> +#include <stdbool.h> + +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +/* ST7796 Commands - Standard */ + +#define ST7796_NOP 0x00 /* NOP */ +#define ST7796_SWRESET 0x01 /* Software Reset */ +#define ST7796_RDDID 0x04 /* Read Display ID */ +#define ST7796_RDDST 0x09 /* Read Display Status */ +#define ST7796_SLPIN 0x10 /* Sleep In */ +#define ST7796_SLPOUT 0x11 /* Sleep Out */ +#define ST7796_PTLON 0x12 /* Partial Display Mode On */ +#define ST7796_NORON 0x13 /* Normal Display Mode On */ +#define ST7796_RDIMGFMT 0x0a /* Read Display Image Format */ +#define ST7796_RDSELFDIAG 0x0f /* Read Display Self-Diagnostic Result */ +#define ST7796_INVOFF 0x20 /* Display Inversion Off */ +#define ST7796_INVON 0x21 /* Display Inversion On */ +#define ST7796_GAMMASET 0x26 /* Gamma Set */ +#define ST7796_DISPOFF 0x28 /* Display Off */ +#define ST7796_DISPON 0x29 /* Display On */ +#define ST7796_CASET 0x2a /* Column Address Set */ +#define ST7796_RASET 0x2b /* Row Address Set */ +#define ST7796_RAMWR 0x2c /* Memory Write */ +#define ST7796_RAMRD 0x2e /* Memory Read */ +#define ST7796_PTLAR 0x30 /* Partial Area */ +#define ST7796_VSCRDEF 0x33 /* Vertical Scrolling Definition */ +#define ST7796_TEOFF 0x34 /* Tearing Effect Line Off */ +#define ST7796_TEON 0x35 /* Tearing Effect Line On */ +#define ST7796_MADCTL 0x36 /* Memory Access Control */ +#define ST7796_VSCRSADD 0x37 /* Vertical Scrolling Start Address */ +#define ST7796_PIXFMT 0x3a /* Pixel Format Set */ +#define ST7796_WRDISPBRIGHT 0x51 /* Write Display Brightness */ +#define ST7796_RDDISPBRIGHT 0x52 /* Read Display Brightness */ +#define ST7796_WRCTRLD 0x53 /* Write Control Display */ +#define ST7796_RDCTRLD 0x54 /* Read Control Display */ +#define ST7796_WRCABC 0x55 /* Write Content Adaptive Brightness */ +#define ST7796_RDCABC 0x56 /* Read Content Adaptive Brightness */ +#define ST7796_WRCABCMIN 0x5e /* Write CABC Minimum Brightness */ +#define ST7796_RDCABCMIN 0x5f /* Read CABC Minimum Brightness */ + +/* ST7796 Commands - Extended */ + +#define ST7796_INVCTR 0xb4 /* Display Inversion Control */ +#define ST7796_DFC 0xb6 /* Display Function Control */ +#define ST7796_PWCTRL1 0xc0 /* Power Control 1 */ +#define ST7796_PWCTRL2 0xc1 /* Power Control 2 */ +#define ST7796_PWCTRL3 0xc2 /* Power Control 3 */ +#define ST7796_PWCTRL4 0xc3 /* Power Control 4 */ +#define ST7796_PWCTRL5 0xc4 /* Power Control 5 */ +#define ST7796_VCOM 0xc5 /* VCOM Control */ +#define ST7796_PWCTRL6 0xc6 /* Power Control 6 */ +#define ST7796_GAMMAPOS 0xe0 /* Positive Gamma Correction */ +#define ST7796_GAMMANEG 0xe1 /* Negative Gamma Correction */ +#define ST7796_DOCA 0xe9 /* Set DDB Write Address */ +#define ST7796_CSCON 0xf0 /* Command Set Control */ + +/* ST7796 MADCTL bits */ + +#define ST7796_MADCTL_MY 0x80 /* Row Address Order */ +#define ST7796_MADCTL_MX 0x40 /* Column Address Order */ +#define ST7796_MADCTL_MV 0x20 /* Row/Column Exchange */ +#define ST7796_MADCTL_ML 0x10 /* Vertical Refresh Order */ +#define ST7796_MADCTL_BGR 0x08 /* BGR color filter panel */ +#define ST7796_MADCTL_MH 0x04 /* Horizontal Refresh Order */ + +/* Pre-defined MADCTL values for common orientations */ + +#define ST7796_MADCTL_PORTRAIT (ST7796_MADCTL_MX) +#define ST7796_MADCTL_PORTRAIT_BGR (ST7796_MADCTL_MX | ST7796_MADCTL_BGR) +#define ST7796_MADCTL_RPORTRAIT (ST7796_MADCTL_MY) +#define ST7796_MADCTL_RPORTRAIT_BGR (ST7796_MADCTL_MY | ST7796_MADCTL_BGR) +#define ST7796_MADCTL_LANDSCAPE (ST7796_MADCTL_MV) +#define ST7796_MADCTL_LANDSCAPE_BGR (ST7796_MADCTL_MV | ST7796_MADCTL_BGR) +#define ST7796_MADCTL_RLANDSCAPE (ST7796_MADCTL_MY | ST7796_MADCTL_MX | \ + ST7796_MADCTL_MV) +#define ST7796_MADCTL_RLANDSCAPE_BGR (ST7796_MADCTL_MY | ST7796_MADCTL_MX | \ + ST7796_MADCTL_MV | ST7796_MADCTL_BGR) + +/* Display raw dimensions (before orientation transform) */ + +#define ST7796_XRES_RAW 320 +#define ST7796_YRES_RAW 480 + +/* Default SPI frequency */ + +#define ST7796_SPI_MAXFREQUENCY 40000000 + +/* Rotation ioctl commands (if not defined in fb.h) */ + +#ifndef FBIOSET_ROTATION +# define FBIOSET_ROTATION _FBIOC(0x0100) +#endif + +#ifndef FBIOGET_ROTATION +# define FBIOGET_ROTATION _FBIOC(0x0101) +#endif + +/**************************************************************************** + * Public Types + ****************************************************************************/ + +#ifndef __ASSEMBLY__ + +/* Command sequence entry for initialization */ + +struct st7796_cmd_s Review Comment: > I agree, in parts. > > **To remove:** I have one question. Should I remove from .h all internal registers? Ex: > > ST7796_NOP ST7796_SWRESET ST7796_RDDID ST7796_RDDST ST7796_SLPIN ST7796_SLPOUT ST7796_PTLON ST7796_NORON ST7796_RDIMGFMT etc... Yes, anything that isn't needed by the user including your header file should not be exposed. Registers are internally used only; your LCD driver is interacted with by users through the public functions only. > I'll also move: Individual MADCTL bit definitions (ST7796_MADCTL_MY, MX, MV, etc.) and struct st7796_cmd_s (internal command sequence structure). > > **To keep:** Basically the ST7796_XRES_RAW and ST7796_YRES_RAW are part of the public API because board code needs them to calculate orientation-dependent resolution at compile-time based on Kconfig settings. That seems reasonable if there's no way around it! > So, I have created a port for STM32H753ZI (from stm32_st7796.c): > > ``` > #if defined(CONFIG_NUCLEO_H753ZI_ST7796_LANDSCAPE) || \ > defined(CONFIG_NUCLEO_H753ZI_ST7796_RLANDSCAPE) > # define ST7796_XRES ST7796_YRES_RAW /* 480 */ > # define ST7796_YRES ST7796_XRES_RAW /* 320 */ > #else > # define ST7796_XRES ST7796_XRES_RAW /* 320 */ > # define ST7796_YRES ST7796_YRES_RAW /* 480 */ > #endif > ``` > > From my understanding this have to be at the .h because the board must access it. The board shall pass xres and yres via st7796_config_s. Indeed there is a alternative for it (wrong decision from my view): hardcode it as below: > > .xres = 480, .yres = 320, > > Without these, board files would need to hardcode dimensions, breaking portability across different ST7796 variants (e.g., 320x480 vs 480x320 vs 240x320)." That seems totally fine, go with your macro-define method. > The same is applicable for the other #defines. Actually I came across such situation in my 'pcb'. I realized that I mounted it upside down and actually, it was nice to see it and develop such thing to fix it quickly. > > ``` > > #if defined(CONFIG_NUCLEO_H753ZI_ST7796_LANDSCAPE) > # ifdef CONFIG_NUCLEO_H753ZI_ST7796_BGR > # define ST7796_MADCTL_BASE ST7796_MADCTL_LANDSCAPE_BGR > # else > # define ST7796_MADCTL_BASE ST7796_MADCTL_LANDSCAPE > # endif > #elif > ``` > > In the end, with such parameters at .h, the board code must select the appropriate MADCTL value based on hardware orientation and RGB/BGR panel type configured via Kconfig. These macros abstract the hardware-specific bit manipulation from board code. The alternative would require board files to manually construct MADCTL values using bit operations, which violates encapsulation and makes the code unmaintainable when hardware details change. > > Finally, the same is applicable for the spi frequency. > > ``` > #ifndef CONFIG_NUCLEO_H753ZI_ST7796_FREQUENCY > # define CONFIG_NUCLEO_H753ZI_ST7796_FREQUENCY ST7796_SPI_MAXFREQUENCY > #endif > ``` > > Board code uses this as a default when users don't specify a custom frequency via Kconfig. This ensures boards use safe values by default while allowing as well the users to override it when needed. Without this, every board would need to duplicate the datasheet value, risking errors and maintenance burden when hardware specs change and the same reasoning is applicable for many other macros. You can set the default value in Kconfig instead by using `default 1000` (replace 1000 with your frequency). This is the preferred method. > **conclusion?** Hence, to sumarize it, I agree to move internal implementation details (register commands, bit definitions, internal structures) to the .c file. This is the right approach. But I disagree with moving these macros to the .c file, as they are part of the public configuration API: > > ST7796_XRES_RAW ST7796_YRES_RAW ST7796_SPI_MAXFREQUENCY ST7796_MADCTL_PORTRAIT ST7796_MADCTL_PORTRAIT_BGR ST7796_MADCTL_RPORTRAIT ST7796_MADCTL_RPORTRAIT_BGR ST7796_MADCTL_LANDSCAPE ST7796_MADCTL_LANDSCAPE_BGR ST7796_MADCTL_RLANDSCAPE ST7796_MADCTL_RLANDSCAPE_BGR That's fine, I only mean that you should move anything not needed by the user to the private C file. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
