Skip to content

Conversation

@mmahadevan108
Copy link
Contributor

@mmahadevan108 mmahadevan108 commented Apr 11, 2024

This PR enables FlexIO support and adds support for the LCD-PAR-S035 NXP display over the FlexIO.

Depends on #70583 for some updates to the MIPI DBI implementation

@zephyrbot
Copy link

zephyrbot commented Apr 11, 2024

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@zephyrbot zephyrbot added manifest manifest-hal_nxp DNM This PR should not be merged (Do Not Merge) labels Apr 11, 2024
@mmahadevan108 mmahadevan108 force-pushed the Add_MCX_LCD branch 2 times, most recently from d7e3b36 to f70a6df Compare April 11, 2024 20:27
@mmahadevan108
Copy link
Contributor Author

cc @jasonNXP

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there plan to support 8-bit data width? if yes, then need to define a macro FLEXIO_MCULCD_DATA_BUS_WIDTH:
https://github.com/zephyrproject-rtos/hal_nxp/blob/abc66979c77421fb3a140ce2e4e6ea7165cdbe8f/mcux/mcux-sdk/drivers/flexio/mculcd/fsl_flexio_mculcd.h#L44C1-L49C3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is passed as a compile time MACRO via the CMake file. I have added a change request to the SDK driver to avoid using this macro as it has limitations. Please see my change request for details

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think rather than adding the data bus property to the display, we should define multiple MIPI modes for each data bus width. Something like the following:

#define MIPI_DBI_MODE_8080_BUS_16_BIT 0x4
#define MIPI_DBI_MODE_8080_BUS_9_BIT 0x5
#define MIPI_DBI_MODE_8080_BUS_8_BIT 0x6

I see two benefits to this:

  • avoid duplication of bus width encoding in each display driver (I suspect this problem won't be unique to the ST7796S)
  • allow MIPI DBI drivers to reconfigure their bus width at runtime (I'm aware that due to the SDK driver structure this won't work for FlexIO, but I suspect most hardware will support runtime reconfiguration like this)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danieldegrasse, I have made this change.

Add support to get FlexIO clock

Signed-off-by: Mahesh Mahadevan <[email protected]>
Add defines for MIPI DBI Type A based on Motorola 6800
and Type B baedon Intel 8080 bus

Signed-off-by: Mahesh Mahadevan <[email protected]>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need support for the rgb-is-inverted property given the discussion here? Or can it be dropped?

If we have a use case I think adding support for this makes sense, but otherwise I would lean towards removing it to minimize the size of the driver

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't data_bus_width being 8 indicate 16 bit mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking at the values in the mipi_dbi.h file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but in both cases (data_bus_width==8 or data_bus_width==5) we are setting the FLEXIO bus width to 8. Is this correct, or should we set it to 16 when data_bus_width==8?

Copy link
Contributor Author

@mmahadevan108 mmahadevan108 Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is accurate. One for 8080 and one for 6800

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some offline discussion, I understand this better now- the SDK driver will default to 16 bits if no value is set for this variable. The enum values 5 and 8 correspond to MIPI DBI 8 bit mode for 6800 and 8080. Since the SDK defaults to 16 bit mode, no else statement is needed here to set the definition in that case

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably add an additional check here to see if the requested MIPI mode is 8 bit when the driver is configured for 16 bit (or vice versa for 16 bit mode). Not sure if we need this though, since the CMake logic should identify the correct MIPI DBI setting based on the chosen display

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats right, the SDK driver configuration is picked from device tree.

MIPI mode is read from the device tree.

Signed-off-by: Mahesh Mahadevan <[email protected]>
1. Add a property for panels where the RGB is displayed as BGR.
2. Add a check for 8080 8-bit mode and invert RGB and BGR for
   this case.

Signed-off-by: Mahesh Mahadevan <[email protected]>
Add a driver to support the NXP FlexIO LCD controller that uses
8080/6800 bus protocol.

Signed-off-by: Mahesh Mahadevan <[email protected]>
NXP LCD-PAR-S035 LCD module will be used by FRDM-X evaluation
kits with a parallel LCD connector.

Signed-off-by: Mahesh Mahadevan <[email protected]>
Add support for the LCD-PAR-S035 display over the
FlexIO interface.

Signed-off-by: Mahesh Mahadevan <[email protected]>
@mmahadevan108 mmahadevan108 added this to the v3.7.0 milestone Jun 12, 2024
type: phandle-array
required: true
description: |
RS Pin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does RS stand for

required: true
description: |
CS Pin
GPIO to drive the CS pin.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does CS stand for (probably chip select?) should put this in description

@nashif nashif merged commit b2d1e45 into zephyrproject-rtos:main Jun 13, 2024
@mmahadevan108 mmahadevan108 deleted the Add_MCX_LCD branch June 17, 2024 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants