edatec Open Source HMI for 7 inches panel driver#7361
Conversation
pelwell
left a comment
There was a problem hiding this comment.
This looks fairly close to being ready, although I know that @6by9 has some feedback about the driver.
As an overall comment for both PRs, we like our submissions to be split into three parts:
- driver code, Kconfig and Makefile
- Device Tree, overlays, README and Makefile
- _defconfig changes
| }; | ||
|
|
||
| fragment@4 { | ||
| target = <&i2c_arm>; |
There was a problem hiding this comment.
There are probably plenty of examples of using &i2c_arm and &i2c1 in the same overlay, but let's be consistent - make this &i2c1 to match the parameter name.
There was a problem hiding this comment.
you mean change like this ?
fragment@4 {
target = <&i2c1>;
| CONFIG_SCHED_TRACER=y | ||
| CONFIG_STRICT_DEVMEM=y | ||
| CONFIG_TEST_KSTRTOX=y | ||
| CONFIG_DRM_PANEL_EDATEC_7INCH=m No newline at end of file |
There was a problem hiding this comment.
bcm2835_defconfig is the upstream defconfig for Pi 1s and Pi Zeros. As of rpi-6.18.y we use:
- arch/arm/configs/bcmrpi_defconfig // Pi 1, Pi Zero
- arch/arm/configs/bcm2709_defconfig // Pi 2, Pi 3, Pi Zero 2
- arch/arm64/configs/bcm2711_defconfig // Pi 3, Pi 4, Pi 5, Pi Zero 2
- arch/arm64/configs/bcm2711_rt_defconfig // RealTime kernel for Pi 3, Pi 4, Pi 5, Pi Zero 2
- arch/arm64/configs/bcm2712_defconfig // Pi 5 only (16kB pages)
To generate _defconfig files:
- Run e.g.
make ARCH=arm ... bcmrpi_defconfig. - Make the changes to
.configin any of the usual ways. - Run
make ARCH=arm ... savedefconfig. - Copy the output file,
defconfig, back to e.g.arch/arm/configs/bcmrpi_defconfig.
There was a problem hiding this comment.
Ok, i will use myself defconfig and don't modify bcm2835_defconfig
| return 0; | ||
| } | ||
|
|
||
| static int ed_panel_unprepare(struct drm_panel *panel) |
There was a problem hiding this comment.
unprepare is optional. Don't define it if it does nothing.
| } | ||
|
|
||
| static int ed_panel_prepare(struct drm_panel *panel) | ||
| { |
There was a problem hiding this comment.
prepare is optional. Don't define it if it does nothing.
| static int ed_panel_disable(struct drm_panel *panel) | ||
| { | ||
| struct ed_panel *ts = panel_to_ts(panel); | ||
| ed_panel_i2c_write(ts, CMD_BACKLIGHT_EN, 0x00); |
There was a problem hiding this comment.
You have a backlight device defined. Associate the display with the backlight and the backlight driver will get called by the framework driver as the panel is enabled/disabled.
You are doing this with the 10" panel branch, so reuse that regulator driver here, and use panel-simple to configure the panel.
There was a problem hiding this comment.
in our project backlight is control by mcu not soc . soc will tell mcu backlight level by i2c . mcu will read i2c and save date in queue.mcu thread will access queue and adjust light by pwm
There was a problem hiding this comment.
The backlight PWM signal is generated by the MCU, but the control of that is from the Linux kernel via some commands. Those commands should be issued from the correct places.
| msgs[0].len = 16; | ||
| msgs[0].buf = data_buf; | ||
|
|
||
| ret = i2c_transfer(i2c->adapter, msgs, ARRAY_SIZE(msgs)); |
There was a problem hiding this comment.
Do you need to do separate write and read transactions for this? The Pi panel had to as the MCU misbehaved with clock stretching and could lock the bus.
There was a problem hiding this comment.
we write some data by i2c tell ic mcu i want do something. ic mcu will run by itself, the i read some reg by i2c get some message , write and read op is sequential operation . between write and read will delay some time. this is no some diffirent with separate write and read . we can delay more time if we need
There was a problem hiding this comment.
between write and read will delay some time.
Is that delay actually needed?
There was a problem hiding this comment.
yes,it's needed. between write and read we must give full time to Transfer and ic muc access, i2c is very slow we always do things whick like this , read result is relay on write reg
| /* This appears last, as it's what will unblock the DSI host | ||
| * driver's component bind function. | ||
| */ | ||
| drm_panel_add(&ts->base); |
There was a problem hiding this comment.
drm_panel_add only adds the panel to a list. If unblocks nothing.
You're probably referring to devm_mipi_dsi_attach
There was a problem hiding this comment.
drm_panel_add will add this panel to list. so other module can get drm_panel by list. eg TPIC get panel on/off by notify chain with drm_panel. devm_mipi_dsi_attach is bind dsi and panel. as usally this two api all be used ,eg panel-ilitek-ili79600a.c , panel-elida-kd35t133.c
| goto error; | ||
| } | ||
|
|
||
| if(ts->mode->clock > 42000) |
There was a problem hiding this comment.
Please provide details of what this is doing.
I'd hazard a guess that it's switching something between rzw,t70p383rk-v2 and rzw,t70p383rk-lite mode, in which case do it by having struct of_device_id data point to a struct that includes this value and a pointer to the drm_display_mode, not inferring it from the clock.
There was a problem hiding this comment.
because cm0 and cm4 dsi frq is diffirent , so this logic is diffirent. i will modify as you mean
| #define CMD_FW_VERSION 0xE1 | ||
|
|
||
| #define PIN_LCD_BL_EN BIT(0) | ||
| #define PIN_LCD_BL_PWM BIT(1) |
There was a problem hiding this comment.
Is this pin actually used as a GPIO, or only as PWM? If only PWM then it shouldn't be exposed as a GPIO.
There was a problem hiding this comment.
this is GPIO control mcu pwm, in our project DDIC and backlight power is control by mcu not soc
There was a problem hiding this comment.
But I see no reference to <®_display 1 0>; in device tree (nor 0 or 2 for that matter). So what are these GPIOs doing?
| } | ||
|
|
||
| static int ed_gpio_get(struct gpio_chip *gc, unsigned int off) | ||
| { |
There was a problem hiding this comment.
Ditto to the query over ed_direction_input, is it really supported and useful to have these GPIOs as inputs that can be read?
There was a problem hiding this comment.
we define a few gpios ,only support output communicate with mcu . in fact mcu some gpio not open ,so we have to do this at here
There was a problem hiding this comment.
If the MCU doesn't allow the GPIOs to be used as inputs, don't add code to support it.
I don't understand what you mean by "in fact mcu some gpio not open".
There was a problem hiding this comment.
i mean this gpios is control by mcu ,soc can only op output.we mcu code is not open. this gpios is used in internal mcu , It is not fully open for soc
| <>911>,"interrupts:0", | ||
| <>911>,"irq-gpios:4"; | ||
|
|
||
| pi4 = <&i2c_frag>, "target:0=",<&i2c_csi_dsi>; |
There was a problem hiding this comment.
2c_frag target defaults to i2c_csi_dsi, so why do we need the pi4, cm4 and pi5 overrides? Just document that the defaults work for those platforms?
There was a problem hiding this comment.
i will ask former colleague . if this is any other use, i will delete them
There was a problem hiding this comment.
i asked former colleague, he tell me that : our product support multiple platforms PI4/CM4 PI5/CM5 CM0 ,we put them in one dts config
| .vtotal = 600 + 12 + 3 + 20, | ||
| }; | ||
|
|
||
| static const struct drm_display_mode ed_panel_7_0_cm0_mode = { |
There was a problem hiding this comment.
I can make guesses as to why the different mode, but please confirm.
The guess is that on CM0 (and CM1 and 3) the parent PLL to DSI is 2GHz and can only integer divide. CM4 is 3GHz.
We need a DSI clock of (pixel clock * 24 / 2), so on CM0 41000 will become 2GHz /4 to give 41666kHz with a small fixup on the HFP.
For CM4 with a 3GHz PLL, /6 would also give a 41666kHz pixel clock. (The 50000kHz clock above is achievable directly with a /5)
The first mode also appears to end up with a refresh rate of 58.58Hz, whereas this cm0 mode is 60.33Hz. What's the desired refresh rate?
There was a problem hiding this comment.
yes it is .
cm0 ratefres = 41000000 /(1114 * 610)about 60.33HZ
cm4 ratefres = 50000000 / (1344 × 635)about 58.58HZ
There was a problem hiding this comment.
yes it is . cm0 ratefres = 41000000 /(1114 * 610)about 60.33HZ cm4 ratefres = 50000000 / (1344 × 635)about 58.58HZ
Why the difference? I believe the CM0 timings should work perfectly on CM4 as well.
There was a problem hiding this comment.
about this question,The information I have learned is as follows:
1、cm4 integer divide has defects . 50MHz is recommended for the Raspberry Pi . this question have email ,i will Ask my leader for the email and send to you
2、in ous project we bring up in CM4 first, use cm4 config cm0 can't work well
3、in this case we try many configs in cm0 by ourself, and current cm0 config is work well
Signed-off-by: lzunspp <bli@edatec.cn>
this is 7 inches panel driver please check them,thanks