* [PATCH 0/3] phy: rockchip: inno-csidphy: fix 2500 Mbps support and add clock lane phase tuning
From: Gerald Loacker @ 2026-06-17 12:23 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Heiko Stuebner, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-phy, linux-arm-kernel, linux-rockchip, linux-kernel,
devicetree, Gerald Loacker
This series fixes and extends the Rockchip Innosilicon CSI D-PHY driver
to support data rates up to 2500 Mbps and adds optional board-specific
clock lane phase tuning for signal integrity.
Patch 1 fixes an off-by-one error in the rk1808 hsfreq range table:
the final entry was capped at 2499 Mbps, causing a rejection of the
maximum supported rate of 2500 Mbps.
Patches 2 and 3 add an optional rockchip,clk-lane-phase device tree
property that allows tuning the clock lane sampling phase in ~40 ps
steps to compensate for board-level signal integrity variations.
---
Gerald Loacker (3):
phy: rockchip: phy-rockchip-inno-csidphy: fix rk1808 hsfreq table
dt-bindings: phy: rockchip-inno-csi-dphy: add rockchip,clk-lane-phase property
phy: rockchip: phy-rockchip-inno-csidphy: add clock lane phase tuning
.../bindings/phy/rockchip-inno-csi-dphy.yaml | 7 ++++++
drivers/phy/rockchip/phy-rockchip-inno-csidphy.c | 27 +++++++++++++++++++++-
2 files changed, 33 insertions(+), 1 deletion(-)
---
base-commit: 8cd9520d35a6c38db6567e97dd93b1f11f185dc6
change-id: 20260617-feature-mipi-csi-dphy-4k60-9879c3d1fe4f
Best regards,
--
Gerald Loacker <gerald.loacker@wolfvision.net>
^ permalink raw reply
* Re: [PATCH v17 01/17] dt-bindings: mmc: renesas,sdhi: Document RZ/G3L (r9a08g046) SoC
From: Wolfram Sang @ 2026-06-17 12:19 UTC (permalink / raw)
To: Biju
Cc: Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm, Biju Das, linux-mmc, devicetree,
linux-kernel, linux-renesas-soc, Prabhakar Mahadev Lad,
Conor Dooley
In-Reply-To: <20260603065731.93243-2-biju.das.jz@bp.renesas.com>
[-- Attachment #1: Type: text/plain, Size: 1205 bytes --]
Hi Biju,
On Wed, Jun 03, 2026 at 07:57:01AM +0100, Biju wrote:
> From: Biju Das <biju.das.jz@bp.renesas.com>
>
> Document the RZ/G3L (r9a08g046) SDHI controller. The RZ/G3L SDHI
> controller is similar to RZ/G2L but has five clocks (core, clkh,
> cd, aclk, aclkm) and three resets (rst, axim, axis), so update the
> clocks/clock-names maximum to 5 and resets/reset-names maximum to 3.
> It has an internal divider for all modes except HS400, and a 2048-bit
> divider compared to 512 on others.
>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
I know you work on v18 already, but some high level remarks already.
> + - description: ACLK/IACLKS, SDHI channel bus clock.
> + - description: IACLKM, SDHI channel bus clock m.
What does 's' and 'm' stand for? Is it mentioned in the docs? Would be
nice to have here as well, if so.
> + resets:
> + items:
> + - description: rst, Core reset.
> + - description: axim, SDHI axi bus reset m.
> + - description: axis, SDHI axi bus reset s.
Ditto.
Happy hacking,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 4/4] arm64: dts: qcom: milos-fairphone-fp6: Enable display
From: Konrad Dybcio @ 2026-06-17 12:12 UTC (permalink / raw)
To: Luca Weiss, Neil Armstrong, Jessica Zhang, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio
Cc: ~postmarketos/upstreaming, phone-devel, dri-devel, devicetree,
linux-kernel, linux-arm-msm
In-Reply-To: <20260501-fp6-panel-v1-4-e09cb05651cc@fairphone.com>
On 5/1/26 3:52 PM, Luca Weiss wrote:
> Configure the MDSS nodes for the phone and add the panel node.
>
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* Re: [PATCH v4 7/7] arm64: dts: qcom: mahua: Switch pcie5_phy ref clock to RPMH_CXO_CLK
From: Konrad Dybcio @ 2026-06-17 12:11 UTC (permalink / raw)
To: Qiang Yu
Cc: Bjorn Andersson, Michael Turquette, Stephen Boyd, Brian Masney,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Taniya Das,
Konrad Dybcio, linux-arm-msm, linux-clk, devicetree, linux-kernel,
krishna.chundru
In-Reply-To: <ai+9CYntPuyEEcLX@hu-qianyu-lv.qualcomm.com>
On 6/15/26 10:51 AM, Qiang Yu wrote:
> On Tue, Jun 09, 2026 at 03:06:02PM +0200, Konrad Dybcio wrote:
>> On 5/28/26 4:29 AM, Qiang Yu wrote:
>>> PCIe5 PHY on Mahua gets refclk from CXO0 pad directly, so no QREF
>>> clkref_en voting is required. Override the clock list to use RPMH_CXO_CLK
>>> directly instead.
>>
>> This is the last piece of the puzzle that this series is missing.
>> There's no QREF clkref_en, but there is a refgen that needs to be
>> powered. For PCIe5 on Mahua this would be L2F_E0 (0p9) and L4H_E0
>> (1p2).
>>
>> I think the easiest (laziest?) solution would be to add dummy clocks
>> in the clkref driver and only toggle the required regulators. Another
>> option is to defer back to individual drivers (such as PCIe QMPPHY).
>>
>> I kinda like the "one central node to drive power" approach, but I'm
>> not sure others agree, since it stretches truth just a tiny bit
>> (although not as much as one would think since there are *some*
>> controls for the transparent-to-the-OS hw pieces in these paths still
>> in TCSR).. Dmitry, Krzysztof, would you object to that?
>>
>
> PCIe5 PHY on Mahua does not use QREF at all, so there is no refgen for
> QREF either. The refgen supplies you mentioned are for the PCIe5 PHY
> itself, not for QREF. For other PHYs that do use QREF, there are two
> refgens: one for QREF (voted here in the TCSR clkref driver) and one for
> the PHY (which should be voted in the PHY driver).
Okay, so in this case we have the refgen regulator hardwired into the
PHY block and we just consume it from the PHY node&driver, am I following
correctly?
Konrad
^ permalink raw reply
* Re: [PATCH v4 2/2] hw_random: timeriomem-rng: add configurable read width and data mask
From: sashiko-bot @ 2026-06-17 12:01 UTC (permalink / raw)
To: Jad Keskes; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260617114642.1911191-1-inasj268@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Incorrect teardown order between manual `hrtimer_cancel()` in `remove()` and devres-managed hwrng unregistration leads to deadlocks or Use-After-Free vulnerabilities.
--
commit f289257a12dc14631ae0271bb9c7ffff6c6a8c94
Author: Jad Keskes <inasj268@gmail.com>
hw_random: timeriomem-rng: add configurable read width and data mask
This commit implements configurable read sizes (1, 2, or 4 bytes) and data
masking for the timeriomem-rng driver. It updates the read loop to dispatch
on the configured width and updates the alignment checks in the probe function.
> diff --git a/drivers/char/hw_random/timeriomem-rng.c b/drivers/char/hw_random/timeriomem-rng.c
> --- a/drivers/char/hw_random/timeriomem-rng.c
> +++ b/drivers/char/hw_random/timeriomem-rng.c
> @@ -71,11 +76,28 @@ static int timeriomem_rng_read(struct hwrng *hwrng, void *data,
> usleep_range(period_us,
> period_us + max(1, period_us / 100));
>
> - *(u32 *)data = readl(priv->io_base);
> - retval += sizeof(u32);
> - data += sizeof(u32);
> - max -= sizeof(u32);
> - } while (wait && max > sizeof(u32));
> + switch (priv->reg_io_width) {
[ ... ]
> + }
> +
> + retval += chunk;
> + data += chunk;
> + max -= chunk;
> + } while (wait && max > chunk);
[Severity: High]
This isn't a bug introduced by this patch, but looking at the driver's
teardown sequence, is there a race condition between devres unregistration
and the hrtimer?
The driver registers the RNG via devm_hwrng_register(), meaning devres handles
unregistration after timeriomem_rng_remove() returns. In
timeriomem_rng_remove(), the driver manually calls hrtimer_cancel().
Since the device remains exposed to userspace during this window, can this
lead to a deadlock? If a reader is waiting in timeriomem_rng_read() on
wait_for_completion(), cancelling the timer ensures it never wakes up. The
reader holds a reference to the hwrng, causing devres unregistration to block
indefinitely in hwrng_unregister().
Additionally, can this lead to a use-after-free? If a read occurs after
hrtimer_cancel() completes but before devres unregisters the RNG,
timeriomem_rng_read() will unconditionally execute hrtimer_restart():
timeriomem_rng_read() {
...
priv->present = 0;
reinit_completion(&priv->completion);
hrtimer_forward_now(&priv->timer, priv->period);
hrtimer_restart(&priv->timer);
return retval;
}
If devres then frees the timeriomem_rng_private structure, wouldn't the timer
fire and cause timeriomem_rng_trigger() to dereference freed memory?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617114436.1909659-1-inasj268@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH 3/6] drm/tiny: Add DRM driver for Solomon SSD16xx e-paper display controllers
From: Devarsh Thakkar @ 2026-06-17 11:47 UTC (permalink / raw)
To: Thomas Zimmermann, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Bjorn Andersson, dri-devel, devicetree,
linux-kernel
Cc: praneeth, vigneshr, s-jain1, r-donadkar, r-sharma3, afd, Sen Wang,
LiangCheng Wang, Aldea, Andrei, Judith Mendez, D, Yashas
In-Reply-To: <eccf407a-c469-4744-a56f-aa7366c58be3@ti.com>
Hi Thomas,
On 08/05/26 21:42, Devarsh Thakkar wrote:
> Hi Thomas,
>
> Thanks for the quick review.
>
> On 05/05/26 12:35, Thomas Zimmermann wrote:
>> Hi,
>>
>> thanks for the driver. See below for a review.
>>
>> There is support for Solomon SSD13xx in solomon/. You should check if
>> the new driver belongs there.
>>
>
Just wanted to follow up on my response [0] to to your v1 review where I
had addressed your comments/questions inline in the thread and had a
couple of questions as I am preparing a V2 patch for the series which I
am planning to send soon.
Would really appreciate your thoughts when you get a chance.
[0]:
https://lore.kernel.org/all/eccf407a-c469-4744-a56f-aa7366c58be3@ti.com/
Thanks,
Devarsh
> Yes I had noticed the solomon directory, but it was only having OLED
> SSD130x drivers which use I2C or SPI to drive an emissive display with a
> completely different programming model and no common factor between this
> ssd16xx driver (which is e-paper display controller) and other drivers
> in that directory.
>
> And since all the e-paper drivers were residing in drivers/gpu/drm/tiny
> I thought to put here along with other e-paper drivers. But if you think
> it is more appropriate to have it in solomon directory along with other
> solomon controller drivers then we can migrate to that folder in V2,
> please let us know your opinion.
>
>> Am 30.04.26 um 20:33 schrieb Devarsh Thakkar:
>>> Add a DRM driver for the Solomon Systech SSD16xx family of e-paper
>>> display
>>> controllers (SSD1680 [4], SSD1673 [3], SSD1681 [5], SSD1683 [1]) based
>>> e-paper display panels, starting with support for the Gooddisplay
>>> GDEY042T81 which is 4.2", 400x300 resolution black/white e-paper display
>>> using SSD1683 controller.
>>>
>>> Hardware features supported:
>>> - SPI communication with optional D/C GPIO (4-wire mode) or 9-bit SPI
>>> with embedded D/C# bit (3-wire mode) command/data signaling
>>> - BUSY GPIO polling with configurable timeout
>>> - Hardware reset via dedicated reset GPIO
>>> - Internal temperature sensor readout
>>> - Border waveform control with 10 configurable modes:
>>> LUT0-3 (GS transition waveforms), fixed voltage levels (VSS/
>>> VSH1/ VSL/
>>> VSH2), VCOM follow, and HiZ (floating, default)
>>> - Deep sleep modes: Mode 1 (RAM retained) and Mode 2 (RAM lost)
>>> - Data entry modes for all four orientations
>>> - Dual display RAM planes: BW RAM (primary image) and RED RAM
>>> (partial refresh baseline / 3-color BWR red pixels)
>>>
>>> Refresh modes:
>>> Three independently selectable refresh waveforms:
>>> - Partial (~300-500 ms): low-latency BW update using RED RAM as
>>> waveform transition baseline
>>> - Full (~1.5-2 s): complete pixel-cycle reset with temperature-
>>> compensated LUT reload
>>> - Fast (~1.0-1.5 s): abbreviated waveform using a pre-loaded LUT
>>>
>>> Pixel format conversions:
>>> The driver accepts all common DRM formats and converts to 1bpp:
>>> - R1 (native 1bpp), color formats (XRGB8888, RGB888, RGB565),
>>> packed YUV (YUYV, UYVY), planar YUV (NV12, NV16), grayscale (R8)
>>> - 3-color BWR: classifies pixels as black, white, or red by dominant
>>> channel; writes separate 1bpp BW and RED buffers
>>>
>>> Orientation / rotation:
>>> - Full 4-way rotation: 0° (landscape), 90° (portrait CW),
>>> 180° (landscape inverted), 270° (portrait CCW)
>>> - Implemented via the Data Entry Mode register — no GPU or memory
>>> rotation overhead
>>> - Configurable via device tree 'rotation' property, module parameter
>>> (overrides DT), or runtime DRM connector property (highest
>>> priority)
>>>
>>> DRM framework integration:
>>> - drm_atomic_helper-based modesetting with damage tracking
>>> - Single primary plane, CRTC, simple encoder, SPI connector
>>> - drm_gem_dma backend for physically contiguous SPI DMA buffers
>>> - fbdev fallback via DRM_FBDEV_DMA_DRIVER_OPS
>>>
>>> E-paper panels are bistable: they retain the last displayed image
>>> indefinitely without power. Ghosting artefacts may accumulate over
>>> time,
>>> and based on user policy the driver can switch to a full refresh mode to
>>> clear them. Similarly, based on user policy, the driver can clear the
>>> screen on every application start or stop to provide a clean slate
>>> for the
>>> next application although some applications may require preserving the
>>> last displayed context when going to suspend or exiting. To provide
>>> such
>>> flexibility, the driver introduces the following DRM connector
>>> properties
>>> along with corresponding module parameters:
>>> DRM connector properties (runtime configurable):
>>> - rotation: Display orientation in degrees (0, 90, 180, 270).
>>>
>>> - refresh_mode: Select update waveform (0=partial, 1=full, 2=fast).
>>> Affects all subsequent frames until changed.
>>>
>>> - refresh_mode_init: Waveform for the first frame after CRTC
>>> enable or a
>>> new DRM master taking control (0=disabled, 1=partial, 2=full,
>>> 3=fast).
>>> After the first frame the driver reverts to refresh_mode. Armed
>>> via
>>> drm_driver.master_set; disarmed by master_drop if the client exits
>>> without submitting a frame.
>>>
>>> - clear_on_init: Clear display on first DRM client open (0=disabled,
>>> 1=partial, 2=full, 3=fast).
>>>
>>> - clear_on_close: Clear display when the displaying client exits
>>> (0=disabled, 1=partial, 2=full, 3=fast). Fires via
>>> drm_driver.master_drop when a client drops DRM master.
>>> It also disarms init_refresh_pending so no stale init refresh
>>> leaks to
>>> the next client.
>>>
>>> - clear_on_disable: Clear display on CRTC disable / DPMS off
>>> (0=disabled, 1=partial, 2=full, 3=fast). Fires via
>>> crtc_atomic_disable. Independent of clear_on_close — a
>>> display_cleared_on_deinit guard prevents a redundant second clear
>>> when both fire for the same session (e.g. compositor does DPMS off
>>> then exits).
>>>
>>> - color_mode: Selects whether a red-plane-capable panel is driven in
>>> 3-color (black/white/red) or black-white-only mode (0=black-white,
>>> 1=3-color). 3-color is only valid for panels with red plane
>>> support;
>>> black-white mode bypasses the RED RAM regardless of panel
>>> capability,
>>> allowing a BWR panel to be driven as a faster BW display.
>>>
>>> - border_waveform_init: Border waveform during clear/init operations
>>> (enum 0-9: LUT0-3, VSS, VSH1, VSL, VSH2, VCOM, HiZ).
>>>
>>> - border_waveform_refresh: Border waveform during display updates
>>> (enum 0-9, same values).
>>>
>>> - border_refresh_on_every_update: When disabled, border waveform
>>> command is sent only when the border state changes (0=disabled,
>>> 1=enabled).
>>
>> Generally speaking, your driver does way too much and leaves too much
>> to DRM clients.
>>
>
> The architecture is such that driver does configure the sane defaults so
> that application need not configure anything mandatorily if the default
> configuration satisfies application requirements before running, but if
> application has specific needs due to which it wants to modify some
> configurations I wanted to provide that flexibility via driver so that
> application can do the re-configuration run-time via drm properties or
> via module params too (which could be helpful for legacy fbdev apps). I
> have given some examples below on where this could be helpful.
>
>
>> - All these properties should go away. There is (mostly) no reason why
>> any of this should be of interest to user space.
>>
>
> We had some reasons actually especially considering the e-paper specific
> properties and use-cases which we have to support and the module params
> and drm properties were mainly added because we did not want to impose
> application specific policy decisions to the kernel and rather provide
> an interface for the applications to customize the configuration as per
> their needs. As e-paper displays are fundamentally different from LCD/
> OLED: they are bistable and retain the last image across sessions and
> power cycles and there is a bit of management w.r.t ghosting and refresh
> mode selection which depends on end use-case and application. This
> create management requirements with no LCD equivalent.
>
> For e.g. w.r.t below properties:
>
>
> 1. Regarding refresh_mode (partial/full/fast): these are not internal
> implementation details but they represent user-visible quality/speed
> tradeoffs. For e.g. Partial refresh is ~300ms but accumulates ghosting
> over time; full refresh is ~1.5-2s but cleans the display.
>
> A drm application displaying clock may want to use partial refresh for
> second or minute updates (as it's only updating a part of screen) but
> switch to full refresh periodically (for e.g. after every few hours) to
> clear ghosting artifacts.
>
> But for another drm application such as an e-book reader when user turns
> the page, the screen is filled with completely new text which has
> updates over most part of the screen and in this case user may want to
> do a full refresh for each page or after every few pages as there are
> more chances of ghosting.
>
> Furthermore, drm application could also leave it to user via a GPIO
> button whenever it want to perform full refresh to clear ghosting based
> on user sees ghosting has started to happen in the screen.
>
> 2. Regarding clear_on_init / clear_on_close / clear_on_disable: because
> bistable displays remember their last content indefinitely, whether to
> clear on session start or end (or not clear at all) is a legitimate
> application policy decision, not a driver decision. A kiosk may always
> want to blank the screen on exit (as it may be displaying personal/
> sensitive information); a document reader or a badge display may want to
> preserve content instead. The driver cannot know which policy is correct.
>
> 3. Regarding color_mode: for BWR panels, switching between 3-color (full
> waveform, ~1.5s) and BW-only (faster, bypasses RED RAM) is a rendering
> policy the application must choose based on whether it has red content
> to display and I think this can't be deduced solely from user-selected
> DRM format. For e.g. if user-space is giving XR24 format then we don't
> know whether they want to render it in color format or BW mode (which
> can give faster refresh rate).
>
> 4. Regarding border_waveform* properties: These control the behaviour of
> e-paper borders which is independent and outside of framebuffer scan-out
> region. So for e.g. if user wants a black border or a gray border or a
> white border, we wanted to provide flexiblity to the user to change
> based on their use-case requirements and since ghosting can accumulate
> in border waveform area too although at a different rate than primary
> screen area, we provided options to trigger a full refresh for border
> areas independently too.
> To summarize, I think these properties reflect genuinte e-paper
> specific application policy decisions and I felt that driver should be
> able to retreive that context from application via these properties and
> make decision accordingly thus supporting the end application use-case
> more properly.
>
>> - User space sets up a state for the display pipeline and the client
>> configures the hardware accordingly.
>>
>> - Don't emulate all these color formats and format conversions. Export
>> what the hardware supports and let user space pick it. Then program
>> the hardware accordingly. You might export XRGB8888 for compatibility
>> and emulate it such that it fits the hardware.
>>
>
> The hardware only supports DRM_FORMAT_R1, DRM_FORMAT_Y1 (which is
> pending upstream merge I think), the other formats we enumerated to
> support pre-configured display servers and apps which generally don't
> support above format. Sure we can support only XRGB8888 as extra format,
> but I just wanted to understand on what's the downside to support other
> formats ? We have tested that there is very less conversion overhead
> given these are all small resolution displays and some of the display
> servers which don't support the native formats (R1, Y1) can use any of
> those.
>
>>
>>>
>>> Module parameters (override DT / panel config defaults):
>>> Each DRM connector property above has a corresponding module
>>> parameter
>>> that sets its initial value at load time, providing the same control via
>>> sysfs and the kernel command line without requiring a DRM-capable
>>> userspace
>>> or handling. All parameters default to -1 (use panel config default) so
>>> that a bare insmod with no arguments works correctly out of the box.
>>
>> That's another thing. Your driver should make good decisions about
>> hardware configuration. Users should not override them.
>>
>
> Yes we have put the most common/sane defaults in the driver w.r.t panel
> (i.e use panel config defaults configured in driver) but as mentioned
> earlier especially for the e-paper panels different applications may
> have conflicting requirements w.r.t configurations, the module params we
> have kept mainly to support legacy fbdev apps.
>>
>>>
>>> Links:
>>> [1] : https://www.crystalfontz.com/controllers/SolomonSystech/SSD1683
>>> [2] : https://files.seeedstudio.com/wiki/Other_Display/42-epaper/
>>> GDEY042T81.pdf
>>> [3] : https://www.crystalfontz.com/controllers/SolomonSystech/SSD1673
>>> [4] : https://www.crystalfontz.com/controllers/SolomonSystech/SSD1680
>>> [5] : https://www.crystalfontz.com/controllers/SolomonSystech/SSD1681
>>>
>>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>>> ---
>>> drivers/gpu/drm/tiny/Kconfig | 13 +
>>> drivers/gpu/drm/tiny/Makefile | 1 +
>>> drivers/gpu/drm/tiny/panel-ssd16xx.c | 2548 ++++++++++++++++++++++++++
>>> 3 files changed, 2562 insertions(+)
>>> create mode 100644 drivers/gpu/drm/tiny/panel-ssd16xx.c
>>>
>>> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
>>> index f0e72d4b6a47..d25363962a1e 100644
>>> --- a/drivers/gpu/drm/tiny/Kconfig
>>> +++ b/drivers/gpu/drm/tiny/Kconfig
>>> @@ -215,3 +215,16 @@ config TINYDRM_SHARP_MEMORY
>>> * 4.40" Sharp Memory LCD (LS044Q7DH01)
>>> If M is selected the module will be called sharp_memory.
>>> +
>>> +config DRM_PANEL_SSD16XX
>>
>> Just call it DRM_SSD16XX without the panel. In DRM, things named
>> 'panel' are usually built around struct drm_panel, which doesn't seem
>> the case here.
>>
>
> Oh ok, I preferred DRM_PANEL_SSD16XX since it also enumerates and uses
> panel specific data/compatible such as this driver supporting
> gooddisplay,gdey042t81 and more can be added too (just like panel-
> ilitek* for e.g.) unlike controller only drivers which need to be linked
> to separate panel drivers.
>
> Do you prefer to change it to DRM_SSD16XX_PANEL to not conflict with
> DRM_PANEL* drivers and for better context or still prefer to keep it as
> DRM_SSD16XX ?
>
>>> + tristate "DRM support for Solomon SSD16xx e-ink display
>>> controllers"
>>> + depends on DRM && SPI
>>> + select DRM_CLIENT_SELECTION
>>> + select DRM_KMS_HELPER
>>> + select DRM_GEM_DMA_HELPER
>>
>> Your driver doesn't seem to do actual DMA from the GEM buffers, but
>> read-and-copy them to the device over SPI. The ideomatic pattern is
>> to use gem-shmem and shadow-plane helpers. Please convert the driver
>> accordingly. See [1] and [2]. Using the initialize macros, you should
>> be able to drop them into the driver without much issues.
>>
>
> Thanks for pointing. Yes i agree with you and will update accordingly in
> V2.
>
>> [1] https://elixir.bootlin.com/linux/v7.0.1/source/include/drm/
>> drm_gem_shmem_helper.h
>> [2] https://elixir.bootlin.com/linux/v7.0.1/source/include/drm/
>> drm_gem_atomic_helper.h#L42
>>
>>> + help
>>> + DRM driver for Solomon SSD16xx family e-ink display controllers
>>> + (SSD1680, SSD1681, SSD1683). Supports the GDEY042T81 4.2" 400x300
>>> + monochrome e-ink panel.
>>> +
>>> + If M is selected the module will be called panel-ssd16xx.
>>> diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/
>>> Makefile
>>> index 48d30bf6152f..3f24e5c6a79a 100644
>>> --- a/drivers/gpu/drm/tiny/Makefile
>>> +++ b/drivers/gpu/drm/tiny/Makefile
>>> @@ -15,3 +15,4 @@ obj-$(CONFIG_TINYDRM_ILI9486) += ili9486.o
>>> obj-$(CONFIG_TINYDRM_MI0283QT) += mi0283qt.o
>>> obj-$(CONFIG_TINYDRM_REPAPER) += repaper.o
>>> obj-$(CONFIG_TINYDRM_SHARP_MEMORY) += sharp-memory.o
>>> +obj-$(CONFIG_DRM_PANEL_SSD16XX) += panel-ssd16xx.o
>>> diff --git a/drivers/gpu/drm/tiny/panel-ssd16xx.c b/drivers/gpu/drm/
>>> tiny/panel-ssd16xx.c
>>> new file mode 100644
>>> index 000000000000..b232837c54ff
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/tiny/panel-ssd16xx.c
>>
>> Again, remove 'panel'.
>
> Yes I can remove the panel, but I am just concerned if it won't mislead
> folks to understand ssd16xx as a controller only driver, requiring a
> separate panel driver to interface with ?
>
> Basically panel-ssd16xx naming was chosen since this driver houses both
> the ssd16xx controller context and also the panel being used along with
> that (similar to panel-ilitek-ili9881c.c) and i did not want to confuse
> it with a controller only driver (similar to tc358775.c), if it is
> overalapping a known pattern reserved for drm_panel drivers do you think
> we should rename it to ssd16xx-panel.c instead or you prefer ssd16xx.c
> as more appropriate one ?
>
>>
>>> @@ -0,0 +1,2548 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * DRM driver for e-paper display panels using Solomon SSD16xx
>>> family controllers
>>> + *
>>> + * Copyright (C) 2026 Texas Instruments Incorporated - https://
>>> www.ti.com/
>>> + *
>>> + * Author: Devarsh Thakkar <devarsht@ti.com>
>>> + *
>>> + * References: https://github.com/Lesords/epaper
>>> + */
>>> +
>>> +#include <linux/delay.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/property.h>
>>> +#include <linux/spi/spi.h>
>>> +
>>> +#include <drm/clients/drm_client_setup.h>
>>> +#include <drm/drm_atomic.h>
>>> +#include <drm/drm_atomic_helper.h>
>>> +#include <drm/drm_damage_helper.h>
>>> +#include <drm/drm_drv.h>
>>> +#include <drm/drm_fb_helper.h>
>>> +#include <drm/drm_fbdev_dma.h>
>>> +#include <drm/drm_fb_dma_helper.h>
>>> +#include <drm/drm_framebuffer.h>
>>> +#include <drm/drm_gem_dma_helper.h>
>>> +#include <drm/drm_gem_framebuffer_helper.h>
>>> +#include <drm/drm_probe_helper.h>
>>
>>> +#include <drm/drm_simple_kms_helper.h>
>>
>> Obsolete. Anything you use from this header should be open-coded in
>> the driver.
>>
>
> Agreed, will remove it in V2.
>
>>> +#include <drm/drm_print.h>
>>
>>
>> Please remove all of the parameters below. They might be nice for your
>> debugging, but they do not belong in the upstream driver.
>>
>
> As mentioned previously, had kept these params mainly for legacy non-drm
> fbdev based applications.
>
>>> +
>>> +static int rotation = -1;
>>> +module_param(rotation, int, 0644);
>>> +MODULE_PARM_DESC(rotation,
>>> + "Display rotation (-1=use DT, 0/180=landscape,
>>> 90/270=portrait)");
>>
>> Please remove this. There is a rotation property in struct
>> drm_connector, which stores the rotation. IIRC it can be overridden on
>> the kernel command line.
>>
>
> As I understand you are referring to below fields from drm_connector
> struct, please correct me if I am wrong here but I think the rotation/
> orientation functionality supported by ssd16xx controller does not match
> much with below model but instead matches what is done in drivers/gpu/
> drm/drm_mipi_dbi.c (although that does not support runtime rotation) as
> explained below :
>
> drm_connector (rotation specific members):
>
>
> 1. panel_orientation (display_info.panel_orientation):
> Readable from DT via of_drm_get_panel_orientation(), overridable from
> cmdline. However it is not writable by userspace at runtime (which we
> require). More importantly, when Weston reads panel_orientation it
> applies an output transform and then attempts to offload rotation to the
> plane via plane.rotation. This model assumes the plane can geometrically
> map a 300x400 source framebuffer to a 400x300 CRTC i.e. hardware scan-
> out rotation. Our driver has no such hardware as explained in detail below.
>
>
> 2. rotation_reflection (cmdline_mode.rotation_reflection):
> Cmdline-only (video=...:rotate=N), no DT path (we require both DT-path
> and runtime suport). Also I think this is strictly for in-kernel
> drm_clients and also It currently returns false for 90/270 unless
> the plane has a hardware rotation property.
>
>
> Both paths therefore ultimately require hardware plane rotation that
> this driver does not have and both seem to be supported just statically
> i.e. cmdline or dt property.
>
> Our use-case needs to support runtime rotation configuration ours is not
> a mounted display but a portable hand-held device (https://
> www.beagleboard.org/boards/beaglebadge) and we have an accelerometer in
> our device which can detect panel orientation and based on accelerometer
> reading the drm app can runtime set the custom drm rotation property to
> switch to new orientation dynamically.
>
> Also our driver is fundamentally different from a GPU display pipeline
> or controllers supporting transpose function. The SSD16xx display
> controller has no transpose or rotation function but instead supports
> different scan-modes, so there is no hardware path that can take a
> 400x300 plane and transpose it to a 300x400 display output. The
> controller is a simple RAM writer: the CPU writes a byte stream over
> SPI, and the controller's internal cursor
> advances sequentially according to the data entry mode register (command
> 0x11), which selects between X++/Y++ and X--/Y-- scan directions with a
> configurable start position.
>
>
> For portrait orientation we therefore change the DRM mode itself to
> 300x400 from the original 400x300, so the application is asked to
> provide a 300x400 framebuffer.
> The driver then writes this buffer column-by-column over SPI to the
> display controller's RAM. Since the controller supports different scan
> start positions (cursor at origin vs cursor at maximum address) combined
> with the appropriate X/Y scan direction, we are able to correctly render
> the 300x400 buffer onto the panel when it is held in portrait
> orientation (90*, 270*).
>
>
> This means the CRTC mode must reflect the logical dimensions directly,
> exactly as drm_mipi_dbi_dev_init() does via mipi_dbi_rotate_mode() for
> MIPI DBI drivers. Accepting a 300x400 framebuffer onto a 400x300 CRTC
> (as the panel_orientation + plane.rotation model requires) is not
> possible: drm_atomic_helper_check_plane_state(DRM_PLANE_NO_SCALING)
> enforces src_w == crtc_w and src_h == crtc_h, and there is no hardware
> to perform the geometric remapping between the two sizes.
>
>
> For runtime rotation changes (which are required as the panel is not
> physically fixed), we therefor wanted to use a custom drm connector
> property. We can look to use the standard DRM_MODE_ROTATE_* bitmask (not
> a custom enum, that was used in v1), we can also look to check if driver
> can triggers a full modeset through the normal DRM path,
> connector_get_modes returns the correctly dimensioned mode for the new
> orientation, and userspace receives a mode-changed event with the new
> dimensions.
>
>
> This is semantically what MIPI DBI tiny drivers do at boot (fixed from
> DT), made runtime-changeable via the custom drm connector property in
> this driver.
>
> Maybe, I can try to use standard bitmask instead of custom enum to re-
> use standard macros :
>
> drm_property_create_bitmask(drm, 0, "rotation",
> rotation_props,
> ARRAY_SIZE(rotation_props),
> DRM_MODE_ROTATE_0 DRM_MODE_ROTATE_90 |
> DRM_MODE_ROTATE_180 |DRM_MODE_ROTATE_270);
>
> but keep it as connector property?
>
>>> +
>>> +static int refresh_mode = -1;
>>> +module_param(refresh_mode, int, 0644);
>>> +MODULE_PARM_DESC(refresh_mode,
>>> + "Refresh mode (-1=panel default, 0=partial ~300-500ms,
>>> 1=full ~1.5-2s, 2=fast ~1.0-1.5s)");
>>> +
>>> +static int border_waveform_init_lut = -1;
>>> +module_param(border_waveform_init_lut, int, 0644);
>>> +MODULE_PARM_DESC(border_waveform_init_lut,
>>> + "Border waveform index during clear/init (-1=panel default,
>>> 0-9=enum index)");
>>> +
>>> +static int border_waveform_lut = -1;
>>> +module_param(border_waveform_lut, int, 0644);
>>> +MODULE_PARM_DESC(border_waveform_lut,
>>> + "Border waveform index during display updates (-1=panel
>>> default, 0-9=enum index)");
>>> +
>>
>> Please remove it. Only the panel default. If you have panels where the
>> default is known to be incorrect, you can add specific workarounds in
>> the driver.
>>
>
> I think the most of these params are kept to sane defaults but they may
> change w.r.t use-cases and each panel can be used in context of multiple
> use-cases.
>
>>
>>> +static bool border_refresh_on_every_update;
>>> +module_param(border_refresh_on_every_update, bool, 0644);
>>> +MODULE_PARM_DESC(border_refresh_on_every_update,
>>> + "Re-send border waveform command before each display update
>>> (default: false)");
>>
>> Pick a sane default.
>>
>
> Yes driver is picking a sane default already for this (refresh border on
> init once with white border and keep it as floating in later updates),
> but just a back-door for the application in case it wants to avoid
> ghosting totally altogether or has specific needs w.r.t border handling.
>
>>> +
>>> +static int clear_on_init = -1;
>>> +module_param(clear_on_init, int, 0644);
>>> +MODULE_PARM_DESC(clear_on_init,
>>> + "Clear display on first app launch (-1=disabled, 0=partial,
>>> 1=full, 2=fast)");
>>> +
>>> +static int clear_on_close = -1;
>>> +module_param(clear_on_close, int, 0644);
>>> +MODULE_PARM_DESC(clear_on_close,
>>> + "Clear display on app close/CRTC disable (-1=disabled,
>>> 0=partial, 1=full, 2=fast)");
>>> +
>>> +static int clear_on_disable = -1;
>>> +module_param(clear_on_disable, int, 0644);
>>> +MODULE_PARM_DESC(clear_on_disable,
>>> + "Clear display on CRTC disable/DPMS off (-1=disabled,
>>> 0=partial, 1=full, 2=fast)");
>>> +
>>> +static int refresh_mode_init = -1;
>>> +module_param(refresh_mode_init, int, 0644);
>>> +MODULE_PARM_DESC(refresh_mode_init,
>>> + "Skip baseline establishment on first enable (-1=disabled,
>>> 0=partial, 1=full, 2=fast)");
>>
>> Use 'disabled' for all of them.
>>
>>> +
>>> +static int color_mode = -1;
>>> +module_param(color_mode, int, 0644);
>>> +MODULE_PARM_DESC(color_mode,
>>> + "Color mode (-1=panel default, 0=black-white, 1=3-color; 3-
>>> color only valid for panels with red plane support)");
>>
>> 'Panel default.' Colors should be controlled by DRM clients via the
>> framebuffer.
>>
>
> As mentioned previously, say user-space is only supporting and giving
> XR24 or XR32 format, from that we can't infer whether user-space want to
> drive display in B/W mode or color-mode.
>
>>> +
>>> +/*
>>> -----------------------------------------------------------------------
>>> + * SSD16xx family common: commands, data values, and bit definitions.
>>> + * These apply equally to SSD1673, SSD1680, and SSD1683.
>>> + *
>>> -----------------------------------------------------------------------
>>> + */
>>> +
>>> +/* SPI command codes (common) */
>>> +#define SSD16XX_CMD_DRIVER_OUTPUT_CONTROL 0x01
>>> +#define SSD16XX_CMD_DATA_ENTRY_MODE 0x11
>>> +#define SSD16XX_CMD_SW_RESET 0x12
>>> +#define SSD16XX_CMD_MASTER_ACTIVATION 0x20
>>> +#define SSD16XX_CMD_DISPLAY_UPDATE_CONTROL1 0x21
>>> +#define SSD16XX_CMD_DISPLAY_UPDATE_CONTROL2 0x22
>>> +#define SSD16XX_CMD_WRITE_RAM_BW 0x24
>>> +#define SSD16XX_CMD_BORDER_WAVEFORM_CONTROL 0x3C
>>> +#define SSD16XX_CMD_SET_RAM_X_ADDRESS_START_END 0x44
>>> +#define SSD16XX_CMD_SET_RAM_Y_ADDRESS_START_END 0x45
>>> +#define SSD16XX_CMD_SET_RAM_X_ADDRESS_COUNTER 0x4E
>>> +#define SSD16XX_CMD_SET_RAM_Y_ADDRESS_COUNTER 0x4F
>>> +
>>> +/*
>>> + * Data Entry Mode (command 0x11) AM/IDY/IDX bit encoding (common).
>>> + *
>>> + * Bit 2 (AM): Address update direction: 0 = X direction, 1 = Y
>>> direction
>>> + * ID[1:0] when AM=0 (X-direction modes, address counter advances in
>>> X):
>>> + * 00 = X decrement, Y decrement 01 = X increment, Y decrement
>>> + * 10 = X decrement, Y increment 11 = X increment, Y increment
>>> (default)
>>> + *
>>> + * Rotation to data entry mode mapping (actual implementation uses
>>> two modes,
>>> + * with scan direction controlled via RAM cursor positioning and
>>> manual tweaking):
>>> + * 0°/270° → 0x03 (X++, Y++) Landscape/Portrait-CW: cursor at
>>> (0, 0)
>>> + * 90°/180° → 0x00 (X--, Y--) Portrait-CCW/Upside-down: cursor at
>>> (max, max)
>>> + *
>>> + * The pixel packing in convert_fb_to_1bpp is grouped by physical
>>> layout:
>>> + * - Portrait (90°/270°): column-major packing, rightmost column
>>> first
>>> + * - Landscape (0°/180°): row-major packing, top to bottom, left
>>> to right
>>> + * Hardware cursor position and scan mode handle the final orientation.
>>> + */
>>> +#define SSD16XX_DATA_ENTRY_XDEC_YDEC 0x00 /* X--, Y-- (X-
>>> mode) */
>>> +#define SSD16XX_DATA_ENTRY_XINC_YINC 0x03 /* X++, Y++ (X-
>>> mode, default) */
>>> +
>>> +/* POR reset value: GD=0 (G0 first), SM=0 (interlaced), TB=0 (G0-
>>> >G299) */
>>> +#define SSD16XX_DRIVER_OUTPUT_CTRL_DEFAULT 0x00
>>> +
>>> +/* Display Update Control 1 (0x21) byte 2 default (common) */
>>> +#define SSD16XX_CTRL1_BYTE2_DEFAULT 0x00
>>> +
>>> +/*
>>> + * Display Update Control 2 (0x22) individual bit definitions (common).
>>> + * NOTE: BIT(3) is NOT common — see SSD1683_CTRL2_MODE2 in the SSD1683
>>> + * section below; it has a completely different meaning in SSD1673.
>>> + */
>>> +#define SSD16XX_CTRL2_ENABLE_CLK BIT(7)
>>> +#define SSD16XX_CTRL2_ENABLE_ANALOG BIT(6)
>>> +#define SSD16XX_CTRL2_LOAD_TEMPERATURE BIT(5)
>>> +#define SSD16XX_CTRL2_LOAD_LUT BIT(4)
>>> +#define SSD16XX_CTRL2_DISPLAY BIT(2)
>>> +#define SSD16XX_CTRL2_DISABLE_ANALOG BIT(1)
>>> +#define SSD16XX_CTRL2_DISABLE_CLK BIT(0)
>>> +
>>> +#define SSD16XX_SPI_BITS_PER_WORD 8
>>> +#define SSD16XX_SPI_SPEED_DEFAULT 1000000
>>> +
>>> +/* Maximum time to wait for the BUSY pin to deassert after a display
>>> update */
>>> +#define SSD16XX_BUSY_WAIT_TIMEOUT_MS 6000
>>> +
>>> +/*
>>> -----------------------------------------------------------------------
>>> + * SSD1683 / SSD1680 specific: commands, data values, and bit
>>> definitions.
>>> + *
>>> -----------------------------------------------------------------------
>>> + */
>>> +
>>> +/*
>>> + * Deep Sleep Mode values (command 0x10).
>>> + */
>>> +#define SSD1683_DEEP_SLEEP_MODE_1 0x01 /* RAM retained */
>>> +#define SSD1683_DEEP_SLEEP_MODE_2 0x03 /* RAM lost (max
>>> power) */
>>> +
>>> +/*
>>> + * Temperature Sensor Selection (command 0x18).
>>> + */
>>> +#define SSD1683_CMD_TEMPERATURE_SENSOR_CONTROL 0x18
>>> +#define SSD1683_TEMP_SENSOR_INTERNAL 0x80 /* Bit 7: use
>>> internal sensor */
>>> +
>>> +/*
>>> + * Write RED RAM (command 0x26).
>>> + */
>>> +#define SSD1683_CMD_WRITE_RAM_RED 0x26
>>> +
>>> +/*
>>> + * Border Waveform Control (command 0x3C) byte values.
>>> + */
>>> +#define SSD1683_BORDER_WAVEFORM_LUT0 0x00 /* GS Transition
>>> LUT0 (black) */
>>> +#define SSD1683_BORDER_WAVEFORM_LUT1 0x01 /* GS Transition
>>> LUT1 (white) */
>>> +#define SSD1683_BORDER_WAVEFORM_LUT2 0x02 /* GS Transition
>>> LUT2 (black) */
>>> +#define SSD1683_BORDER_WAVEFORM_LUT3 0x03 /* GS Transition
>>> LUT3 (gray) */
>>> +#define SSD1683_BORDER_WAVEFORM_FIXLVL_VSS 0x40 /* Fix Level VSS
>>> (0V, black) */
>>> +#define SSD1683_BORDER_WAVEFORM_FIXLVL_VSH1 0x50 /* Fix Level
>>> VSH1 (+15V, black) */
>>> +#define SSD1683_BORDER_WAVEFORM_FIXLVL_VSL 0x60 /* Fix Level VSL
>>> (-15V, white) */
>>> +#define SSD1683_BORDER_WAVEFORM_FIXLVL_VSH2 0x70 /* Fix Level
>>> VSH2 (+15V alt, black) */
>>> +#define SSD1683_BORDER_WAVEFORM_VCOM 0x80 /* Follow VCOM
>>> (-2V~-3V, preserve) */
>>> +#define SSD1683_BORDER_WAVEFORM_HIZ 0xC0 /* HiZ (floating,
>>> default) */
>>> +
>>> +/*
>>> + * Display Update Control 1 (0x21) byte 1 — RED RAM control.
>>> + */
>>> +#define SSD1683_CTRL1_NORMAL 0x00 /* Both BW and RED
>>> RAMs enabled */
>>> +#define SSD1683_CTRL1_BYPASS_RED_RAM 0x40 /* Bypass RED RAM
>>> (force RED=0) */
>>> +
>>> +/*
>>> + * Display Update Control 2 (0x22) BIT(3) — "Display Mode
>>> 2" (partial/BW).
>>> + */
>>> +#define SSD1683_CTRL2_MODE2 BIT(3)
>>> +
>>> +/* Composite CTRL2 sequences for each refresh mode */
>>> +#define SSD1683_CTRL2_FULL_REFRESH (SSD16XX_CTRL2_ENABLE_CLK | \
>>> + SSD16XX_CTRL2_ENABLE_ANALOG | \
>>> + SSD16XX_CTRL2_LOAD_TEMPERATURE | \
>>> + SSD16XX_CTRL2_LOAD_LUT | \
>>> + SSD16XX_CTRL2_DISPLAY | \
>>> + SSD16XX_CTRL2_DISABLE_ANALOG | \
>>> + SSD16XX_CTRL2_DISABLE_CLK) /* 0xF7, ~1.5-2s */
>>> +
>>> +#define SSD1683_CTRL2_FAST_REFRESH (SSD16XX_CTRL2_ENABLE_CLK | \
>>> + SSD16XX_CTRL2_ENABLE_ANALOG | \
>>> + SSD16XX_CTRL2_DISPLAY | \
>>> + SSD16XX_CTRL2_DISABLE_ANALOG | \
>>> + SSD16XX_CTRL2_DISABLE_CLK) /* 0xC7, ~1.0-1.5s */
>>> +
>>> +#define SSD1683_CTRL2_PARTIAL_REFRESH (SSD16XX_CTRL2_ENABLE_CLK | \
>>> + SSD16XX_CTRL2_ENABLE_ANALOG | \
>>> + SSD16XX_CTRL2_LOAD_TEMPERATURE | \
>>> + SSD16XX_CTRL2_LOAD_LUT | \
>>> + SSD1683_CTRL2_MODE2 | \
>>> + SSD16XX_CTRL2_DISPLAY | \
>>> + SSD16XX_CTRL2_DISABLE_ANALOG | \
>>> + SSD16XX_CTRL2_DISABLE_CLK) /* 0xFF,
>>> ~300-500ms */
>>> +
>>> +/*
>>> + * Standalone LUT pre-load sequence (0x91 = ENABLE_CLK | LOAD_LUT |
>>> LOAD_TEMPERATURE |
>>> + * DISABLE_CLK).
>>> + * Pre-loads the OTP LUT without triggering a display update.
>>> Required for
>>> + * FAST refresh mode (0xC7) which omits LOAD_LUT from each update
>>> cycle.
>>> + */
>>> +#define SSD1683_CTRL2_LOAD_TEMP_LUT (SSD16XX_CTRL2_ENABLE_CLK
>>> | \
>>> + SSD16XX_CTRL2_LOAD_LUT | \
>>> + SSD16XX_CTRL2_LOAD_TEMPERATURE | \
>>> + SSD16XX_CTRL2_DISABLE_CLK) /* 0xB1 */
>>> +
>>> +MODULE_IMPORT_NS("DMA_BUF");
>>> +
>>> +enum ssd16xx_controller {
>>> + SSD1683 = 1,
>>> +};
>>> +
>>> +enum ssd16xx_model {
>>> + GDEY042T81 = 1,
>>> +};
>>> +
>>> +enum ssd16xx_refresh_mode {
>>> + SSD16XX_REFRESH_PARTIAL = 0, /* Partial refresh (~300-500ms) */
>>> + SSD16XX_REFRESH_FULL, /* Full refresh (~1.5-2s) */
>>> + SSD16XX_REFRESH_FAST, /* Fast refresh, skip temp load
>>> (~1.0-1.5s) */
>>> +};
>>> +
>>> +enum ssd16xx_color_mode {
>>> + SSD16XX_COLOR_MODE_BW = 0, /* Black/white only; RED RAM
>>> always bypassed */
>>> + SSD16XX_COLOR_MODE_3COLOR = 1, /* 3-colour BWR; RED RAM used for
>>> red pixels */
>>> +};
>>> +
>>> +/* Border waveform enum indices (0-9); mapped to HW bytes via
>>> + * controller_cfg->border_waveform_table[]
>>> + */
>>> +enum ssd16xx_border_waveform {
>>> + SSD16XX_BORDER_LUT0 = 0, /* GS Transition LUT0 (black) */
>>> + SSD16XX_BORDER_LUT1, /* GS Transition LUT1 (white) */
>>> + SSD16XX_BORDER_LUT2, /* GS Transition LUT2 (black) */
>>> + SSD16XX_BORDER_LUT3, /* GS Transition LUT3 (gray) */
>>> + SSD16XX_BORDER_VSS, /* Fix Level VSS (black) */
>>> + SSD16XX_BORDER_VSH1, /* Fix Level VSH1 (black) */
>>> + SSD16XX_BORDER_VSL, /* Fix Level VSL (white) */
>>> + SSD16XX_BORDER_VSH2, /* Fix Level VSH2 (black) */
>>> + SSD16XX_BORDER_VCOM, /* Follow VCOM (preserve) */
>>> + SSD16XX_BORDER_HIZ, /* HiZ (floating, default) */
>>> +};
>>> +
>>> +/* SSD1683/SSD1680 border waveform byte encoding for command 0x3C */
>>> +static const u8 ssd1683_border_waveform_table[] = {
>>> + [SSD16XX_BORDER_LUT0] = SSD1683_BORDER_WAVEFORM_LUT0,
>>> + [SSD16XX_BORDER_LUT1] = SSD1683_BORDER_WAVEFORM_LUT1,
>>> + [SSD16XX_BORDER_LUT2] = SSD1683_BORDER_WAVEFORM_LUT2,
>>> + [SSD16XX_BORDER_LUT3] = SSD1683_BORDER_WAVEFORM_LUT3,
>>> + [SSD16XX_BORDER_VSS] = SSD1683_BORDER_WAVEFORM_FIXLVL_VSS,
>>> + [SSD16XX_BORDER_VSH1] = SSD1683_BORDER_WAVEFORM_FIXLVL_VSH1,
>>> + [SSD16XX_BORDER_VSL] = SSD1683_BORDER_WAVEFORM_FIXLVL_VSL,
>>> + [SSD16XX_BORDER_VSH2] = SSD1683_BORDER_WAVEFORM_FIXLVL_VSH2,
>>> + [SSD16XX_BORDER_VCOM] = SSD1683_BORDER_WAVEFORM_VCOM,
>>> + [SSD16XX_BORDER_HIZ] = SSD1683_BORDER_WAVEFORM_HIZ,
>>> +};
>>> +
>>> +struct ssd16xx_controller_config {
>>> + u16 max_width;
>>> + u16 max_height;
>>> + u8 ram_x_address_bits;
>>> + u8 ram_y_address_bits;
>>> +
>>> + /*
>>> + * has_temp_sensor_ctrl: controller supports command 0x18
>>> (Temperature
>>> + * Sensor Selection). Present in SSD1683/SSD1680; absent in
>>> SSD1673
>>> + * which uses command 0x1A (direct temperature write) instead.
>>> + */
>>> + bool has_temp_sensor_ctrl;
>>> +
>>> + /*
>>> + * Deep sleep mode byte values for command 0x10.
>>> + * deep_sleep_mode_level1: lower-power sleep, RAM content
>>> retained
>>> + * (MODE_1 on SSD1683/SSD1680; used for runtime idle / app-
>>> close).
>>> + * deep_sleep_mode_level2: maximum power savings, RAM may be lost
>>> + * (MODE_2 on SSD1683/SSD1680; used for system suspend).
>>> + * Chips with a single sleep mode set both fields to the same
>>> value.
>>> + */
>>> + u8 deep_sleep_mode_level1;
>>> + u8 deep_sleep_mode_level2;
>>> +
>>> + /*
>>> + * border_waveform_table: chip-specific byte values for the 10
>>> logical
>>> + * border waveform modes (indexed by enum ssd16xx_border_waveform).
>>> + * The encoding of command 0x3C differs between SSD1683/SSD1680 and
>>> + * SSD1673, so each controller provides its own translation table.
>>> + */
>>> + const u8 *border_waveform_table;
>>> +
>>> + /*
>>> + * Display Update Control 1 (cmd 0x21) byte 1 values.
>>> + * ctrl1_normal: both BW and RED RAMs participate in the
>>> waveform.
>>> + * ctrl1_bypass_red_ram: RED RAM bypassed; waveform driven from
>>> BW RAM only.
>>> + * SSD1673 has no RED RAM so both fields carry the same value.
>>> + */
>>> + u8 ctrl1_normal;
>>> + u8 ctrl1_bypass_red_ram;
>>> +
>>> + /*
>>> + * Display Update Control 2 (cmd 0x22) composite sequences for each
>>> + * refresh mode (indexed by enum ssd16xx_refresh_mode) and the
>>> + * standalone LUT pre-load sequence used before fast refresh.
>>> + * Values differ between SSD1683/SSD1680 and SSD1673 (MODE2 bit,
>>> etc.).
>>> + */
>>> + u8 ctrl2_refresh[3]; /* indexed by SSD16XX_REFRESH_PARTIAL/
>>> FULL/FAST */
>>> + u8 ctrl2_load_temp_lut; /* standalone LUT pre-load (no display
>>> update) */
>>> +};
>>> +
>>> +struct ssd16xx_panel_config {
>>> + /* Data Entry Mode - controls X/Y increment direction for
>>> landscape (0°) */
>>> + u8 data_entry_mode;
>>> +
>>> + /* Driver Output Control - third byte (scan direction) */
>>> + u8 driver_output_ctrl_byte3;
>>> +
>>> + /* Default refresh mode for this panel */
>>> + enum ssd16xx_refresh_mode default_refresh_mode;
>>> +
>>> + /* Default border waveform during clear/init (enum index 0-9) */
>>> + enum ssd16xx_border_waveform default_border_waveform_init;
>>> +
>>> + /* Default border waveform during display updates (enum index
>>> 0-9) */
>>> + enum ssd16xx_border_waveform default_border_waveform_update;
>>> +
>>> + /* Whether to re-send border waveform command before each
>>> display update */
>>> + bool default_border_refresh_on_every_update;
>>> +
>>> + /*
>>> + * Default clear-on-init behaviour.
>>> + * -1=disabled, 0=partial, 1=full, 2=fast (matches enum
>>> ssd16xx_refresh_mode)
>>> + */
>>> + int default_clear_on_init;
>>> +
>>> + /* Default clear-on-close behaviour (-1=disabled, 0=partial,
>>> 1=full, 2=fast) */
>>> + int default_clear_on_close;
>>> +
>>> + /* Default clear-on-disable behaviour (-1=disabled, 0=partial,
>>> 1=full, 2=fast) */
>>> + int default_clear_on_disable;
>>> +
>>> + /*
>>> + * Default refresh-mode-init: -1=disabled, else skip baseline
>>> establishment
>>> + * and start directly in this refresh mode.
>>> + */
>>> + int default_refresh_mode_init;
>>> +
>>> + /*
>>> + * Whether this panel has a physical red colour plane (3-colour
>>> BWR).
>>> + * false: 2-colour black/white only; the RED RAM is always
>>> bypassed.
>>> + * true: 3-colour panel; full-refresh writes to the RED RAM so
>>> that
>>> + * red pixels are driven through the red waveform.
>>> + */
>>> + bool red_supported;
>>> +
>>> + /* Panel-specific display mode (resolution and physical
>>> dimensions) */
>>> + const struct drm_display_mode *mode;
>>> +};
>>> +
>>> +struct ssd16xx_panel {
>>
>> Better call this 'struct ssd16xx_device' and the rsp variables
>> 'ssd16xx'. As mentioned, the name 'panel' already has a specific
>> meaning in DRM.
>>
>
> Alright I can do that, I thought folks won't confuse it since this is
> not importing drm_panel struct.
>
>>
>>> + struct drm_device drm;
>>> +
>>> + struct drm_plane primary_plane;
>>> + struct drm_crtc crtc;
>>> + struct drm_encoder encoder;
>>> + struct drm_connector connector;
>>> +
>>> + struct spi_device *spi;
>>> + struct gpio_desc *reset;
>>> + struct gpio_desc *busy;
>>> + struct gpio_desc *dc;
>>> +
>>> + enum ssd16xx_model model;
>>> + enum ssd16xx_controller controller;
>>> + const struct ssd16xx_controller_config *controller_cfg;
>>> + const struct ssd16xx_panel_config *panel_cfg;
>>> + struct drm_display_mode *mode;
>>> + u32 width;
>>> + u32 height;
>>> +
>>> + bool initialized;
>>> + bool reinit_pending; /* HW re-init required after
>>> orientation change */
>>> + bool init_refresh_pending; /* First frame after
>>> refresh_mode_init enable */
>>> + bool first_clear_done; /* clear_on_init has already fired once */
>>> + bool display_cleared_on_deinit; /* Avoid redundant clear in
>>> atomic_disable/master_drop */
>>> +
>>> + int orientation; /* Display orientation in degrees: 0/90/180/270 */
>>> + enum ssd16xx_refresh_mode refresh_mode; /* Active refresh mode */
>>> + enum ssd16xx_color_mode color_mode; /* Active color mode (BW
>>> or 3-color) */
>>> + bool fast_lut_pending; /* LUT pre-load needed before next fast
>>> refresh */
>>> +
>>> + /* Border waveform (as enum indices) */
>>> + int border_waveform_init_idx; /* Border waveform during clear/
>>> init */
>>> + int border_waveform_update_idx; /* Border waveform during
>>> display updates */
>>> + bool border_refresh_on_every_update; /* Re-send border cmd each
>>> display update */
>>> + bool border_waveform_pending; /* One-shot: send border cmd on
>>> next update */
>>> +
>>> + /* Display control */
>>> + int clear_on_init; /* -1=disabled, 0=partial, 1=full, 2=fast */
>>> + int clear_on_close; /* -1=disabled, 0=partial, 1=full, 2=fast */
>>> + int clear_on_disable; /* -1=disabled, 0=partial, 1=full, 2=fast */
>>> + int refresh_mode_init; /* -1=disabled, else use this mode for
>>> the first frame */
>>> +
>>> + u8 *tx_buf; /* 1bpp frame buffer (mono + white) */
>>> + u8 *tx_red_buf; /* 1bpp red-channel buffer (3-color panels
>>> only) */
>>> + u16 *tx_buf9; /* 9-bit SPI expansion buffer (3-wire mode
>>> only) */
>>> +
>>> + struct drm_framebuffer *last_fb; /* Last drawn FB for
>>> reinit redraws */
>>> + struct drm_property *rotation_property;
>>> + struct drm_property *refresh_mode_property;
>>> + struct drm_property *border_waveform_init_property;
>>> + struct drm_property *border_waveform_update_property;
>>> + struct drm_property *border_refresh_on_every_update_property;
>>> + struct drm_property *clear_on_init_property;
>>> + struct drm_property *clear_on_close_property;
>>> + struct drm_property *clear_on_disable_property;
>>> + struct drm_property *refresh_mode_init_property;
>>> + struct drm_property *color_mode_property;
>>> +};
>>> +
>>> +static inline struct ssd16xx_panel *to_ssd16xx_panel(struct
>>> drm_device *drm)
>>> +{
>>> + return container_of(drm, struct ssd16xx_panel, drm);
>>> +}
>>> +
>>> +static inline struct ssd16xx_panel *crtc_to_ssd16xx_panel(struct
>>> drm_crtc *crtc)
>>> +{
>>> + return container_of(crtc, struct ssd16xx_panel, crtc);
>>> +}
>>> +
>>> +static inline struct ssd16xx_panel *plane_to_ssd16xx_panel(struct
>>> drm_plane *plane)
>>> +{
>>> + return container_of(plane, struct ssd16xx_panel, primary_plane);
>>> +}
>>> +
>>> +static const struct ssd16xx_controller_config
>>> ssd16xx_controller_configs[] = {
>>> + [SSD1683] = {
>>> + .max_width = 400,
>>> + .max_height = 300,
>>> + .ram_x_address_bits = 8,
>>> + .ram_y_address_bits = 16,
>>> + .has_temp_sensor_ctrl = true,
>>> + .deep_sleep_mode_level1 = SSD1683_DEEP_SLEEP_MODE_1,
>>> + .deep_sleep_mode_level2 = SSD1683_DEEP_SLEEP_MODE_2,
>>> + .border_waveform_table = ssd1683_border_waveform_table,
>>> + .ctrl1_normal = SSD1683_CTRL1_NORMAL,
>>> + .ctrl1_bypass_red_ram = SSD1683_CTRL1_BYPASS_RED_RAM,
>>> + .ctrl2_refresh = {
>>> + [SSD16XX_REFRESH_PARTIAL] = SSD1683_CTRL2_PARTIAL_REFRESH,
>>> + [SSD16XX_REFRESH_FULL] = SSD1683_CTRL2_FULL_REFRESH,
>>> + [SSD16XX_REFRESH_FAST] = SSD1683_CTRL2_FAST_REFRESH,
>>> + },
>>> + .ctrl2_load_temp_lut = SSD1683_CTRL2_LOAD_TEMP_LUT,
>>> + },
>>> +};
>>> +
>>> +/* GDEY042T81: 4.2" 400x300 panel, 84.8x63.6mm active area */
>>> +static const struct drm_display_mode gdey042t81_mode = {
>>> + DRM_SIMPLE_MODE(400, 300, 85, 64),
>>> +};
>>> +
>>> +static const struct ssd16xx_panel_config ssd16xx_panel_configs[] = {
>>> + [GDEY042T81] = {
>>> + .data_entry_mode = SSD16XX_DATA_ENTRY_XINC_YINC,
>>> + .driver_output_ctrl_byte3 = SSD16XX_DRIVER_OUTPUT_CTRL_DEFAULT,
>>> + .default_refresh_mode = SSD16XX_REFRESH_PARTIAL,
>>> + .default_border_waveform_init = SSD16XX_BORDER_LUT1, /*
>>> white, clean clear */
>>> + .default_border_waveform_update = SSD16XX_BORDER_HIZ, /*
>>> floating, preserve */
>>> + .default_border_refresh_on_every_update = false,
>>> + .default_clear_on_init = -1,
>>> + .default_clear_on_close = -1,
>>> + .default_clear_on_disable = -1,
>>> + .default_refresh_mode_init = SSD16XX_REFRESH_FULL,
>>> + .red_supported = false, /* 2-colour black/white panel */
>>> + .mode = &gdey042t81_mode,
>>> + },
>>> +};
>>> +
>>> +static void ssd16xx_wait_for_panel(struct ssd16xx_panel *panel,
>>> + int *err)
>>> +{
>>> + unsigned long timeout_jiffies = jiffies +
>>> + msecs_to_jiffies(SSD16XX_BUSY_WAIT_TIMEOUT_MS);
>>> + unsigned long start_ms = jiffies_to_msecs(jiffies);
>>> + int busy_val;
>>> +
>>> + if (*err)
>>> + return;
>>
>> This is good. It'll simplify error handling in other places.
>>
>>> +
>>> + busy_val = gpiod_get_value_cansleep(panel->busy);
>>> + drm_dbg(&panel->drm, "BUSY initial value: %d\n", busy_val);
>>> +
>>> + while (gpiod_get_value_cansleep(panel->busy) == 1) {
>>> + if (time_after(jiffies, timeout_jiffies)) {
>>> + drm_err(&panel->drm, "Busy wait timed out after %lums\n",
>>> + jiffies_to_msecs(jiffies) - start_ms);
>>> + *err = -ETIMEDOUT;
>>> + return;
>>> + }
>>> + usleep_range(100, 200);
>>> + }
>>> +
>>> + drm_dbg(&panel->drm, "BUSY became ready after %lums\n",
>>> + jiffies_to_msecs(jiffies) - start_ms);
>>> +}
>>> +
>>> +static void ssd16xx_spi_sync(struct spi_device *spi, struct
>>> spi_message *msg,
>>> + int *err)
>>> +{
>>> + int ret;
>>> +
>>> + if (*err)
>>> + return;
>>> +
>>> + ret = spi_sync(spi, msg);
>>> + if (ret < 0)
>>> + *err = ret;
>>> +}
>>> +
>>> +static void ssd16xx_send_cmd(struct ssd16xx_panel *panel, u8 cmd,
>>> + int *err)
>>> +{
>>> + u16 word;
>>> + struct spi_transfer xfer = {};
>>> + struct spi_message msg;
>>> +
>>> + if (*err)
>>> + return;
>>> +
>>> + spi_message_init(&msg);
>>> + spi_message_add_tail(&xfer, &msg);
>>> +
>>> + if (panel->dc) {
>>> + /* 4-wire SPI: D/C# GPIO low selects command mode */
>>> + xfer.tx_buf = &cmd;
>>> + xfer.len = 1;
>>> + gpiod_set_value_cansleep(panel->dc, 0);
>>> + } else {
>>> + /*
>>> + * 3-wire SPI (9-bit): bit 8 is the D/C# bit.
>>> + * D/C# = 0 means the following 8 bits are a command.
>>> + */
>>> + word = cmd; /* bit 8 = 0 for command */
>>> + xfer.tx_buf = &word;
>>> + xfer.len = sizeof(u16);
>>> + xfer.bits_per_word = 9;
>>> + }
>>> +
>>> + ssd16xx_spi_sync(panel->spi, &msg, err);
>>> +}
>>> +
>>> +static void ssd16xx_send_data(struct ssd16xx_panel *panel, u8 data,
>>> + int *err)
>>> +{
>>> + u16 word;
>>> + struct spi_transfer xfer = {};
>>> + struct spi_message msg;
>>> +
>>> + if (*err)
>>> + return;
>>> +
>>> + spi_message_init(&msg);
>>> + spi_message_add_tail(&xfer, &msg);
>>> +
>>> + if (panel->dc) {
>>> + /* 4-wire SPI: D/C# GPIO high selects data mode */
>>> + xfer.tx_buf = &data;
>>> + xfer.len = 1;
>>> + gpiod_set_value_cansleep(panel->dc, 1);
>>> + } else {
>>> + /*
>>> + * 3-wire SPI (9-bit): bit 8 is the D/C# bit.
>>> + * D/C# = 1 means the following 8 bits are data.
>>> + */
>>> + word = 0x100 | data;
>>> + xfer.tx_buf = &word;
>>> + xfer.len = sizeof(u16);
>>> + xfer.bits_per_word = 9;
>>> + }
>>> +
>>> + ssd16xx_spi_sync(panel->spi, &msg, err);
>>> +}
>>> +
>>> +static void ssd16xx_send_x_param(struct ssd16xx_panel *panel, u16 x,
>>> + int *err)
>>> +{
>>> + if (*err)
>>> + return;
>>> +
>>> + if (panel->controller_cfg->ram_x_address_bits == 8) {
>>> + ssd16xx_send_data(panel, (u8)x, err);
>>> + } else {
>>> + ssd16xx_send_data(panel, x & 0xFF, err);
>>> + ssd16xx_send_data(panel, (x >> 8) & 0xFF, err);
>>> + }
>>> +}
>>> +
>>> +static void ssd16xx_send_y_param(struct ssd16xx_panel *panel, u16 y,
>>> + int *err)
>>> +{
>>> + if (*err)
>>> + return;
>>> +
>>> + if (panel->controller_cfg->ram_y_address_bits == 8) {
>>> + ssd16xx_send_data(panel, (u8)y, err);
>>> + } else {
>>> + ssd16xx_send_data(panel, y & 0xFF, err);
>>> + ssd16xx_send_data(panel, (y >> 8) & 0xFF, err);
>>> + }
>>> +}
>>> +
>>> +static void ssd16xx_send_data_bulk(struct ssd16xx_panel *panel,
>>> + const u8 *data, size_t len,
>>> + int *err)
>>> +{
>>> + struct spi_transfer xfer = {};
>>> + struct spi_message msg;
>>> +
>>> + if (*err)
>>> + return;
>>> +
>>> + if (!data || !len)
>>> + return;
>>> +
>>> + spi_message_init(&msg);
>>> + spi_message_add_tail(&xfer, &msg);
>>> +
>>> + if (panel->dc) {
>>> + /* 4-wire SPI: D/C# GPIO high selects data mode */
>>> + xfer.tx_buf = data;
>>> + xfer.len = len;
>>> + gpiod_set_value_cansleep(panel->dc, 1);
>>> + ssd16xx_spi_sync(panel->spi, &msg, err);
>>> + } else {
>>> + /* 3-wire (9-bit): expand u8 → u16 with D/C#=1 in bit 8. */
>>> + size_t i;
>>> + u16 *buf = panel->tx_buf9;
>>> +
>>> + for (i = 0; i < len; i++)
>>> + buf[i] = 0x100 | data[i];
>>> +
>>> + xfer.tx_buf = buf;
>>> + xfer.len = len * sizeof(u16);
>>> + xfer.bits_per_word = 9;
>>> + ssd16xx_spi_sync(panel->spi, &msg, err);
>>> + }
>>> +}
>>> +
>>> +static void ssd16xx_display_update(struct ssd16xx_panel *panel,
>>> + u8 ctrl1_byte1, u8 ctrl1_byte2, u8 ctrl2_mode,
>>> + int *err)
>>> +{
>>> + if (*err)
>>> + return;
>>> +
>>> + drm_dbg(&panel->drm,
>>> + "display_update: Setting ctrl1=0x%02x,0x%02x mode=0x%02x\n",
>>> + ctrl1_byte1, ctrl1_byte2, ctrl2_mode);
>>> +
>>> + ssd16xx_send_cmd(panel, SSD16XX_CMD_DISPLAY_UPDATE_CONTROL1, err);
>>> + ssd16xx_send_data(panel, ctrl1_byte1, err);
>>> + ssd16xx_send_data(panel, ctrl1_byte2, err);
>>> +
>>> + ssd16xx_send_cmd(panel, SSD16XX_CMD_DISPLAY_UPDATE_CONTROL2, err);
>>> + ssd16xx_send_data(panel, ctrl2_mode, err);
>>> + ssd16xx_send_cmd(panel, SSD16XX_CMD_MASTER_ACTIVATION, err);
>>> +
>>> + drm_dbg(&panel->drm,
>>> + "display_update: Master activation sent, waiting...\n");
>>> +
>>> + ssd16xx_wait_for_panel(panel, err);
>>> +}
>>> +
>>> +static void ssd16xx_hw_reset(struct ssd16xx_panel *panel)
>>> +{
>>> + gpiod_set_value_cansleep(panel->reset, 1);
>>> + usleep_range(10000, 11000);
>>> + gpiod_set_value_cansleep(panel->reset, 0);
>>> + usleep_range(10000, 11000);
>>> +}
>>> +
>>> +/*
>>> + * ssd16xx_preload_fast_lut() - pre-load the OTP LUT for fast
>>> refresh mode.
>>> + *
>>> + * Fast refresh (CTRL2 = 0xC7) omits the LOAD_LUT step on every
>>> update to save
>>> + * time. It relies on the LUT being loaded upfront via this
>>> standalone sequence
>>> + * (CTRL2 = 0xB1: ENABLE_CLK | LOAD_LUT |
>>> SSD16XX_CTRL2_LOAD_TEMPERATURE | DISABLE_CLK,
>>> + * no display update).
>>> + *
>>> + * Must be called when:
>>> + * a) hw_init runs with refresh_mode == FAST, and
>>> + * b) switching to fast refresh from a mode that did not leave a
>>> valid Mode1
>>> + * LUT in the controller (i.e. previous mode was not FULL
>>> refresh, which
>>> + * carries LOAD_LUT in its own CTRL2 sequence).
>>> + */
>>> +static int ssd16xx_preload_fast_lut(struct ssd16xx_panel *panel)
>>> +{
>>> + int err = 0;
>>> +
>>> + ssd16xx_send_cmd(panel, SSD16XX_CMD_DISPLAY_UPDATE_CONTROL1, &err);
>>> + ssd16xx_send_data(panel, panel->controller_cfg-
>>> >ctrl1_bypass_red_ram, &err);
>>> + ssd16xx_send_data(panel, SSD16XX_CTRL1_BYTE2_DEFAULT, &err);
>>> +
>>> + ssd16xx_send_cmd(panel, SSD16XX_CMD_DISPLAY_UPDATE_CONTROL2, &err);
>>> + ssd16xx_send_data(panel, panel->controller_cfg-
>>> >ctrl2_load_temp_lut, &err);
>>> +
>>> + ssd16xx_send_cmd(panel, SSD16XX_CMD_MASTER_ACTIVATION, &err);
>>> + ssd16xx_wait_for_panel(panel, &err);
>>> +
>>> + return err;
>>> +}
>>> +
>>> +static int ssd16xx_hw_init(struct ssd16xx_panel *panel)
>>> +{
>>> + int err = 0;
>>> + u16 ram_height = panel->controller_cfg->max_height;
>>> + u8 data_entry_mode;
>>> +
>>> + ssd16xx_hw_reset(panel);
>>> +
>>> + /* Software reset */
>>> + ssd16xx_send_cmd(panel, SSD16XX_CMD_SW_RESET, &err);
>>> + ssd16xx_wait_for_panel(panel, &err);
>>> +
>>> + /* Driver output control (0x01): MUX ratio and scan direction. */
>>> + ssd16xx_send_cmd(panel, SSD16XX_CMD_DRIVER_OUTPUT_CONTROL, &err);
>>> + ssd16xx_send_y_param(panel, ram_height - 1, &err);
>>> + ssd16xx_send_data(panel, panel->panel_cfg-
>>> >driver_output_ctrl_byte3, &err);
>>> +
>>> + /* Internal temperature sensor (SSD1683/SSD1680 only; not
>>> present in SSD1673) */
>>> + if (panel->controller_cfg->has_temp_sensor_ctrl) {
>>> + ssd16xx_send_cmd(panel,
>>> SSD1683_CMD_TEMPERATURE_SENSOR_CONTROL, &err);
>>> + ssd16xx_send_data(panel, SSD1683_TEMP_SENSOR_INTERNAL, &err);
>>> + }
>>> +
>>> + /*
>>> + * For FAST refresh mode, pre-load the LUT once here during
>>> initialization.
>>> + * FAST mode ctrl2 (0xC7) omits LOAD_LUT on every update for
>>> speed, so the
>>> + * LUT must be loaded upfront. FULL (0xF7) and PARTIAL (0xFF)
>>> load LUT on
>>> + * every update, so no preload is needed for those modes.
>>> + */
>>> + if (panel->refresh_mode == SSD16XX_REFRESH_FAST) {
>>> + ssd16xx_send_cmd(panel, SSD16XX_CMD_DISPLAY_UPDATE_CONTROL1,
>>> &err);
>>> + ssd16xx_send_data(panel, panel->controller_cfg-
>>> >ctrl1_bypass_red_ram, &err);
>>> + ssd16xx_send_data(panel, SSD16XX_CTRL1_BYTE2_DEFAULT, &err);
>>> +
>>> + ssd16xx_send_cmd(panel, SSD16XX_CMD_DISPLAY_UPDATE_CONTROL2,
>>> &err);
>>> + ssd16xx_send_data(panel, panel->controller_cfg-
>>> >ctrl2_load_temp_lut, &err);
>>> +
>>> + ssd16xx_send_cmd(panel, SSD16XX_CMD_MASTER_ACTIVATION, &err);
>>> + ssd16xx_wait_for_panel(panel, &err);
>>> + }
>>> +
>>> + /*
>>> + * Set Data Entry Mode (0x11) based on orientation. This controls
>>> + * how the RAM address counter auto-advances after each byte write.
>>> + *
>>> + * Implementation uses two data entry modes:
>>> + * - 90°/180° use XDEC_YDEC (0x00): X--, Y-- with cursor at
>>> (max, max)
>>> + * - 0°/270° use XINC_YINC (0x03): X++, Y++ with cursor at (0, 0)
>>> + *
>>> + * The convert_fb_to_1bpp packing is grouped by physical layout:
>>> + * - Portrait orientations (90°/270°): column-major packing
>>> + * - Landscape orientations (0°/180°): row-major packing
>>> + *
>>> + * Final scan direction and image orientation are controlled by the
>>> + * combination of data entry mode and RAM cursor position set in
>>> fb_dirty.
>>> + *
>>> + * The RAM address window and cursor are NOT set here; fb_dirty
>>> + * always programmes them (with the correct end-before-start order
>>> + * for decrement modes) immediately before writing frame data.
>>> + */
>>> + switch (panel->orientation) {
>>
>> As mentioned, use the connector property instead.
>>
>>> + case 90:
>>> + case 180:
>>> + data_entry_mode = SSD16XX_DATA_ENTRY_XDEC_YDEC;
>>> + break;
>>> + default: /* 0°/270° */
>>> + data_entry_mode = SSD16XX_DATA_ENTRY_XINC_YINC;
>>> + break;
>>> + }
>>> +
>>> + ssd16xx_send_cmd(panel, SSD16XX_CMD_DATA_ENTRY_MODE, &err);
>>> + ssd16xx_send_data(panel, data_entry_mode, &err);
>>> + drm_dbg(&panel->drm, "hw_init: orientation=%u°
>>> data_entry=0x%02x\n",
>>> + panel->orientation, data_entry_mode);
>>> +
>>> + ssd16xx_wait_for_panel(panel, &err);
>>> +
>>> + if (err)
>>> + drm_err(&panel->drm, "Hardware initialization failed: %d\n",
>>> err);
>>> +
>>> + return err;
>>> +}
>>> +
>>> +/*
>>> + * Clear display by writing all-white to both BW and RED RAM.
>>> + * The ctrl2 argument selects the waveform (full/partial/fast refresh).
>>> + * Border waveform is set to init value before clearing, then restored
>>> + * to the update value to preserve the border during subsequent
>>> updates.
>>> + */
>>> +static int ssd16xx_clear_display(struct ssd16xx_panel *panel, u8 ctrl2)
>>> +{
>>> + const u8 *bw_tbl = panel->controller_cfg->border_waveform_table;
>>> + int err = 0;
>>> + unsigned int data_size = (panel->width * panel->height) / 8;
>>> + u8 *white_buffer = panel->tx_buf;
>>> +
>>> + memset(white_buffer, 0xFF, data_size);
>>> +
>>> + ssd16xx_send_cmd(panel, SSD16XX_CMD_SET_RAM_X_ADDRESS_COUNTER,
>>> &err);
>>> + ssd16xx_send_x_param(panel, 0x00, &err);
>>> +
>>> + ssd16xx_send_cmd(panel, SSD16XX_CMD_SET_RAM_Y_ADDRESS_COUNTER,
>>> &err);
>>> + ssd16xx_send_y_param(panel, 0x00, &err);
>>> +
>>> + ssd16xx_send_cmd(panel, SSD16XX_CMD_WRITE_RAM_BW, &err);
>>> + ssd16xx_send_data_bulk(panel, white_buffer, data_size, &err);
>>> +
>>> + ssd16xx_send_cmd(panel, SSD1683_CMD_WRITE_RAM_RED, &err);
>>> + ssd16xx_send_data_bulk(panel, white_buffer, data_size, &err);
>>> +
>>> + /* Set border waveform for the clear operation */
>>> + drm_dbg(&panel->drm, "clear_display: Set border init waveform:
>>> 0x%02x\n",
>>> + bw_tbl[panel->border_waveform_init_idx]);
>>> + ssd16xx_send_cmd(panel, SSD16XX_CMD_BORDER_WAVEFORM_CONTROL, &err);
>>> + ssd16xx_send_data(panel,
>>> + bw_tbl[panel->border_waveform_init_idx],
>>> + &err);
>>> +
>>> + /* 3-colour mode: CTRL1_NORMAL (read both RAMs); BW mode: bypass
>>> RED. */
>>> + ssd16xx_display_update(panel,
>>> + panel->color_mode == SSD16XX_COLOR_MODE_3COLOR
>>> + ? panel->controller_cfg->ctrl1_normal
>>> + : panel->controller_cfg->ctrl1_bypass_red_ram,
>>> + SSD16XX_CTRL1_BYTE2_DEFAULT, ctrl2, &err);
>>> +
>>> + /* Restore border waveform to update/preservation value */
>>> + drm_dbg(&panel->drm, "clear_display: Restored border update
>>> waveform: 0x%02x\n",
>>> + bw_tbl[panel->border_waveform_update_idx]);
>>> + ssd16xx_send_cmd(panel, SSD16XX_CMD_BORDER_WAVEFORM_CONTROL, &err);
>>> + ssd16xx_send_data(panel,
>>> + bw_tbl[panel->border_waveform_update_idx],
>>> + &err);
>>> +
>>> + return err;
>>> +}
>>> +
>>> +static u8 ssd16xx_refresh_mode_to_ctrl2(struct ssd16xx_panel *panel,
>>> + enum ssd16xx_refresh_mode mode)
>>> +{
>>> + if (mode < ARRAY_SIZE(panel->controller_cfg->ctrl2_refresh))
>>> + return panel->controller_cfg->ctrl2_refresh[mode];
>>> + return panel->controller_cfg->ctrl2_refresh[SSD16XX_REFRESH_FULL];
>>> +}
>>> +
>>> +/*
>>> + * Clear display on new DRM master open (if clear_on_init >= 0).
>>> + * Guarded by panel->first_clear_done; master_drop resets it
>>> unconditionally
>>> + * so each new client session gets a fresh clear.
>>> + */
>>> +static int ssd16xx_clear_display_on_init(struct ssd16xx_panel *panel)
>>> +{
>>> + int ret;
>>> +
>>> + if (panel->clear_on_init < 0 || panel->first_clear_done)
>>> + return 0;
>>> +
>>> + drm_dbg(&panel->drm, "clear_on_init: running, mode=%d\n",
>>> + panel->clear_on_init);
>>> + ret = ssd16xx_clear_display(panel,
>>> + ssd16xx_refresh_mode_to_ctrl2(panel, panel-
>>> >clear_on_init));
>>> + if (ret)
>>> + return ret;
>>> +
>>> + panel->first_clear_done = true;
>>> + return 0;
>>> +}
>>> +
>>> +/*
>>> + * Clear display when the displaying client exits (if clear_on_close
>>> >= 0).
>>> + * Called from ssd16xx_drm_master_drop().
>>> + */
>>> +static int ssd16xx_clear_display_on_exit(struct ssd16xx_panel *panel)
>>> +{
>>> + int ret;
>>> +
>>> + if (panel->clear_on_close < 0)
>>> + return 0;
>>> +
>>> + drm_dbg(&panel->drm, "clear_on_close: running, mode=%d\n",
>>> + panel->clear_on_close);
>>> + ret = ssd16xx_clear_display(panel,
>>> + ssd16xx_refresh_mode_to_ctrl2(panel, panel-
>>> >clear_on_close));
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/*
>>> + * ssd16xx_pixel_luma() - return ITU-R BT.601 luminance (0-255) for
>>> one pixel.
>>> + *
>>> + * For colour formats the result is (299*R + 587*G + 114*B) / 1000;
>>> + * for luma-only formats the luma byte is returned directly.
>>> + *
>>> + * R1 is never passed here — it is already 1bpp and is handled
>>> directly by
>>> + * the callers.
>>> + */
>>> +static u8 ssd16xx_pixel_luma(struct iosys_map *src,
>>> + struct drm_framebuffer *fb,
>>> + unsigned int x, unsigned int y)
>>> +{
>>> + switch (fb->format->format) {
>>> + case DRM_FORMAT_XRGB8888: {
>>> + u32 *line = (u32 *)(src->vaddr + y * fb->pitches[0]);
>>> + u32 px = line[x];
>>> + u8 r = (px >> 16) & 0xFF, g = (px >> 8) & 0xFF, b = px & 0xFF;
>>> +
>>> + return (u8)((299u * r + 587u * g + 114u * b) / 1000u);
>>> + }
>>> + case DRM_FORMAT_RGB888: {
>>> + u8 *line = (u8 *)(src->vaddr + y * fb->pitches[0]);
>>> + u8 r = line[x * 3], g = line[x * 3 + 1], b = line[x * 3 + 2];
>>> +
>>> + return (u8)((299u * r + 587u * g + 114u * b) / 1000u);
>>> + }
>>> + case DRM_FORMAT_RGB565: {
>>> + u16 *line = (u16 *)(src->vaddr + y * fb->pitches[0]);
>>> + u16 px = line[x];
>>> + u8 r = ((px >> 11) & 0x1F) << 3;
>>> + u8 g = ((px >> 5) & 0x3F) << 2;
>>> + u8 b = (px & 0x1F) << 3;
>>> +
>>> + return (u8)((299u * r + 587u * g + 114u * b) / 1000u);
>>> + }
>>> + case DRM_FORMAT_R8: {
>>> + u8 *line = (u8 *)(src->vaddr + y * fb->pitches[0]);
>>> +
>>> + return line[x];
>>> + }
>>> + case DRM_FORMAT_NV12:
>>> + case DRM_FORMAT_NV16:
>>> + return ((u8 *)(src->vaddr))[y * fb->pitches[0] + x];
>>> + case DRM_FORMAT_YUYV: {
>>> + u8 *line = (u8 *)(src->vaddr + y * fb->pitches[0]);
>>> +
>>> + return line[x * 2];
>>> + }
>>> + case DRM_FORMAT_UYVY: {
>>> + u8 *line = (u8 *)(src->vaddr + y * fb->pitches[0]);
>>> +
>>> + return line[x * 2 + 1];
>>> + }
>>> + default:
>>> + return 0;
>>> + }
>>> +}
>>> +
>>> +/*
>>> + * ssd16xx_pixel_is_white() - test whether a pixel maps to white in
>>> 1bpp output.
>>> + *
>>> + * Uses fixed threshold of 127. Pixels with luma strictly greater
>>> than 127
>>> + * are rendered white.
>>> + */
>>> +static bool ssd16xx_pixel_is_white(struct iosys_map *src,
>>> + struct drm_framebuffer *fb,
>>> + unsigned int x, unsigned int y)
>>> +{
>>> + /* R1 is already binarised; avoid the luma computation entirely. */
>>> + if (fb->format->format == DRM_FORMAT_R1) {
>>> + u8 *line = (u8 *)(src->vaddr + y * fb->pitches[0]);
>>> +
>>> + return !!(line[x / 8] & (1 << (7 - (x % 8))));
>>> + }
>>> + return ssd16xx_pixel_luma(src, fb, x, y) > 127;
>>> +}
>>> +
>>> +/*
>>> + * ssd16xx_pixel_is_red() - test whether a pixel is dominated by the
>>> red channel.
>>> + *
>>> + * Only meaningful for formats that carry RGB information (XRGB8888,
>>> RGB888,
>>> + * RGB565). For luma-only and monochrome formats there is no red
>>> channel, so
>>> + * the function always returns false; callers should use
>>> ssd16xx_pixel_is_white()
>>> + * to obtain the BW value for those formats.
>>> + *
>>> + * Returns true when the red component exceeds 50% intensity AND is
>>> strictly
>>> + * greater than both green and blue (dominant red hue).
>>> + */
>>> +static bool ssd16xx_pixel_is_red(struct iosys_map *src,
>>> + struct drm_framebuffer *fb,
>>> + unsigned int x, unsigned int y)
>>> +{
>>> + u32 format = fb->format->format;
>>> +
>>> + switch (format) {
>>> + case DRM_FORMAT_XRGB8888: {
>>> + u32 *line = (u32 *)(src->vaddr + y * fb->pitches[0]);
>>> + u32 px = line[x];
>>> + u8 r = (px >> 16) & 0xFF;
>>> + u8 g = (px >> 8) & 0xFF;
>>> + u8 b = px & 0xFF;
>>> +
>>> + return r > 127 && r > g && r > b;
>>> + }
>>> + case DRM_FORMAT_RGB888: {
>>> + u8 *line = (u8 *)(src->vaddr + y * fb->pitches[0]);
>>> + u8 r = line[x * 3];
>>> + u8 g = line[x * 3 + 1];
>>> + u8 b = line[x * 3 + 2];
>>> +
>>> + return r > 127 && r > g && r > b;
>>> + }
>>> + case DRM_FORMAT_RGB565: {
>>> + u16 *line = (u16 *)(src->vaddr + y * fb->pitches[0]);
>>> + u16 px = line[x];
>>> + u8 r = ((px >> 11) & 0x1F) << 3;
>>> + u8 g = ((px >> 5) & 0x3F) << 2;
>>> + u8 b = (px & 0x1F) << 3;
>>> +
>>> + return r > 127 && r > g && r > b;
>>> + }
>>> + default:
>>> + return false; /* No colour channel information */
>>> + }
>>> +}
>>> +
>>> +/*
>>> + * ssd16xx_convert_fb_to_3color() - split a framebuffer into BW and
>>> RED planes.
>>> + * @bw_dst: output buffer for the black/white RAM plane (1=white,
>>> 0=black)
>>> + * @red_dst: output buffer for the red RAM plane (1=red,
>>> 0=not red)
>>> + * @src: mapped framebuffer memory
>>> + * @fb: DRM framebuffer descriptor
>>> + * @rect: region to convert (must be aligned to 8-pixel boundaries)
>>> + *
>>> + * Each output buffer must be at least rect_width/8 * rect_height
>>> bytes.
>>> + * Pixels are classified as:
>>> + * - red: written to red_dst as 1, bw_dst as 0 (black)
>>> + * - white: written to bw_dst as 1, red_dst as 0
>>> + * - black: written to both as 0
>>> + *
>>> + * For monochrome formats (R1) where no colour information is
>>> available the
>>> + * source data is copied verbatim to bw_dst and red_dst is cleared
>>> to 0xFF
>>> + * (all-white = no red pixels).
>>> + */
>>> +static void ssd16xx_convert_fb_to_3color(u8 *bw_dst, u8 *red_dst,
>>> + struct iosys_map *src,
>>> + struct drm_framebuffer *fb,
>>> + struct drm_rect *rect)
>>> +{
>>> + unsigned int x, y;
>>> + u8 bw_byte = 0, red_byte = 0;
>>> + unsigned int bit_pos = 0;
>>> + unsigned int dst_idx = 0;
>>> +
>>> + drm_dbg(fb->dev,
>>> + "convert_3color: fmt=%p4cc rect=(%d,%d)-(%d,%d) path=%s\n",
>>> + &fb->format->format,
>>> + rect->x1, rect->y1, rect->x2, rect->y2,
>>> + fb->format->format == DRM_FORMAT_R1 ? "R1-direct" : "color-
>>> pixel");
>>> +
>>> + /*
>>> + * R1 is already monochrome — no colour channel exists.
>>> + * Copy BW data directly and leave the red plane all-white
>>> (transparent).
>>> + */
>>> + if (fb->format->format == DRM_FORMAT_R1) {
>>> + unsigned int src_pitch = fb->pitches[0];
>>> + unsigned int width_bytes = drm_rect_width(rect) / 8;
>>> + unsigned int data_size = width_bytes * drm_rect_height(rect);
>>> +
>>> + for (y = rect->y1; y < rect->y2; y++) {
>>> + u8 *line = src->vaddr + y * src_pitch + (rect->x1 / 8);
>>> +
>>> + memcpy(bw_dst + dst_idx, line, width_bytes);
>>> + dst_idx += width_bytes;
>>> + }
>>> + memset(red_dst, 0xFF, data_size); /* 0xFF = all white: no
>>> red pixels */
>>> + return;
>>> + }
>>> +
>>> + /* Use fixed threshold of 127 for grayscale to monochrome
>>> conversion. */
>>> + for (y = rect->y1; y < rect->y2; y++) {
>>> + for (x = rect->x1; x < rect->x2; x++) {
>>> + bool is_red = ssd16xx_pixel_is_red(src, fb, x, y);
>>> +
>>> + if (is_red)
>>> + red_byte |= (1 << (7 - bit_pos));
>>> + else if (ssd16xx_pixel_is_white(src, fb, x, y))
>>> + bw_byte |= (1 << (7 - bit_pos));
>>> + /* else: black pixel — both bits remain 0 */
>>> + if (++bit_pos == 8) {
>>> + bw_dst[dst_idx] = bw_byte;
>>> + red_dst[dst_idx] = red_byte;
>>> + dst_idx++;
>>> + bw_byte = 0;
>>> + red_byte = 0;
>>> + bit_pos = 0;
>>> + }
>>> + }
>>> +
>>> + /* Flush any partial byte at the end of each row */
>>> + if (bit_pos > 0) {
>>> + bw_dst[dst_idx] = bw_byte;
>>> + red_dst[dst_idx] = red_byte;
>>> + dst_idx++;
>>> + bw_byte = 0;
>>> + red_byte = 0;
>>> + bit_pos = 0;
>>> + }
>>> + }
>>> +}
>>> +
>>> +/*
>>> + * ssd16xx_convert_r8_to_red_only() - map an R8 framebuffer to the
>>> RED RAM plane.
>>> + *
>>> + * Used when the panel has a physical red colour plane
>>> (red_supported == true)
>>> + * and the framebuffer format is DRM_FORMAT_R8. Pixels with value
>>> >= 128 are
>>> + * treated as red ink; the BW RAM is set to all-white so that only
>>> red ink
>>> + * appears on the white background.
>>> + *
>>> + * Hardware orientation is handled by the caller via RAM counter
>>> positioning;
>>> + * data is written in normal row-major order here (same as
>>> convert_fb_to_3color).
>>> + */
>>> +static void ssd16xx_convert_r8_to_red_only(u8 *bw_dst, u8 *red_dst,
>>> + struct iosys_map *src,
>>> + struct drm_framebuffer *fb,
>>> + struct drm_rect *rect)
>>> +{
>>> + unsigned int src_pitch = fb->pitches[0];
>>> + unsigned int width = drm_rect_width(rect);
>>> + unsigned int height = drm_rect_height(rect);
>>> + unsigned int data_size = DIV_ROUND_UP(width, 8) * height;
>>> + unsigned int dst_idx = 0;
>>> + unsigned int x, y;
>>> + u8 red_byte = 0;
>>> + unsigned int bit_pos = 0;
>>> +
>>> + /* BW RAM: all-white background - no black ink, only red ink
>>> shows */
>>> + memset(bw_dst, 0xFF, data_size);
>>> +
>>> + /* RED RAM: R8 >= 128 -> red ink (1-bit set) */
>>> + for (y = rect->y1; y < rect->y2; y++) {
>>> + u8 *line = src->vaddr + y * src_pitch;
>>> +
>>> + for (x = rect->x1; x < rect->x2; x++) {
>>> + if (line[x] >= 128)
>>> + red_byte |= (1 << (7 - bit_pos));
>>> + if (++bit_pos == 8) {
>>> + red_dst[dst_idx++] = red_byte;
>>> + red_byte = 0;
>>> + bit_pos = 0;
>>> + }
>>> + }
>>> + if (bit_pos > 0) {
>>> + red_dst[dst_idx++] = red_byte;
>>> + red_byte = 0;
>>> + bit_pos = 0;
>>> + }
>>> + }
>>> +}
>>> +
>>> +/*
>>> + * Convert framebuffer to 1-bit monochrome for e-paper display.
>>> + *
>>> + * Supported formats: XRGB8888, RGB888, RGB565, R8, NV12, NV16,
>>> YUYV, UYVY, R1.
>>> + * For colour and luma formats, Otsu's global binarisation method
>>> computes an
>>> + * optimal per-image threshold from the luminance histogram.
>>> + * R1 is the controller's native format and bypasses conversion
>>> entirely.
>>> + *
>>> + * Output layout:
>>> + * 0°/180° landscape: row-major, left-to-right, top-to-bottom
>>> + * 90°/270° CW portrait: column-major, rightmost column first
>>> + */
>>> +static void ssd16xx_convert_fb_to_1bpp(u8 *dst, struct iosys_map *src,
>>> + struct drm_framebuffer *fb,
>>> + struct drm_rect *rect,
>>> + unsigned int orientation)
>>> +{
>>> + u32 format = fb->format->format;
>>> + int x, y;
>>> + u8 byte = 0;
>>> + unsigned int bit_pos = 0;
>>> + unsigned int dst_idx = 0;
>>> +
>>> + /* Use fixed threshold of 127 for grayscale to monochrome
>>> conversion. */
>>> + drm_dbg(fb->dev,
>>> + "convert_1bpp: fmt=%p4cc rect=(%d,%d)-(%d,%d) orient=%u°
>>> path=%s\n",
>>> + &fb->format->format,
>>> + rect->x1, rect->y1, rect->x2, rect->y2,
>>> + orientation,
>>> + (format == DRM_FORMAT_R1 && orientation == 0 && rect->x1 % 8
>>> == 0) ? "R1-fast" :
>>> + (orientation == 90 || orientation == 270) ? "portrait" :
>>> "landscape");
>>> +
>>> + /*
>>> + * R1 fast path: 0° landscape with byte-aligned rect.
>>> + * R1 is already 1bpp so landscape rows map directly to output
>>> bytes via
>>> + * memcpy — no per-pixel computation needed. rect->x1 must be a
>>> + * multiple of 8 so that (rect->x1 / 8) gives the correct byte
>>> offset;
>>> + * if not, the generic pixel-by-pixel loop below handles non-
>>> aligned
>>> + * rects safely.
>>> + */
>>> + if (format == DRM_FORMAT_R1 && orientation == 0 && rect->x1 % 8
>>> == 0) {
>>> + unsigned int src_pitch = fb->pitches[0];
>>> + unsigned int width_bytes = drm_rect_width(rect) / 8;
>>> +
>>> + for (y = rect->y1; y < rect->y2; y++) {
>>> + u8 *src_line = src->vaddr + y * src_pitch + (rect->x1 / 8);
>>> +
>>> + memcpy(dst + dst_idx, src_line, width_bytes);
>>> + dst_idx += width_bytes;
>>> + }
>>> + return;
>>> + }
>>> +
>>> + switch (orientation) {
>>> + case 90:
>>> + case 270:
>>> + /*
>>> + * Portrait (90° or 270°): column-major packing.
>>> + * Each portrait source column becomes one physical RAM row.
>>> + * The data entry mode and cursor position control scan
>>> direction.
>>> + */
>>> + for (x = rect->x2 - 1; x >= (int)rect->x1; x--) {
>>> + for (y = rect->y1; y < rect->y2; y++) {
>>> + if (ssd16xx_pixel_is_white(src, fb, x, y))
>>> + byte |= (1 << (7 - bit_pos));
>>> + if (++bit_pos == 8) {
>>> + dst[dst_idx++] = byte;
>>> + byte = 0;
>>> + bit_pos = 0;
>>> + }
>>> + }
>>> + if (bit_pos > 0) {
>>> + dst[dst_idx++] = byte;
>>> + byte = 0;
>>> + bit_pos = 0;
>>> + }
>>> + }
>>> + break;
>>> +
>>> + case 0:
>>> + case 180:
>>> + default:
>>> + /*
>>> + * Landscape (0° or 180°): row-major packing.
>>> + * Each landscape source row becomes one physical RAM row.
>>> + * The data entry mode and cursor position control scan
>>> direction.
>>> + */
>>> + for (y = rect->y1; y < rect->y2; y++) {
>>> + for (x = rect->x1; x < rect->x2; x++) {
>>> + if (ssd16xx_pixel_is_white(src, fb, x, y))
>>> + byte |= (1 << (7 - bit_pos));
>>> + if (++bit_pos == 8) {
>>> + dst[dst_idx++] = byte;
>>> + byte = 0;
>>> + bit_pos = 0;
>>> + }
>>> + }
>>> + if (bit_pos > 0) {
>>> + dst[dst_idx++] = byte;
>>> + byte = 0;
>>> + bit_pos = 0;
>>> + }
>>> + }
>>> + break;
>>> + }
>>> +}
>>> +
>>> +static int ssd16xx_fb_dirty(struct drm_framebuffer *fb, struct
>>> drm_rect *rect,
>>> + struct ssd16xx_panel *panel)
>>> +{
>>> + const u8 *ctrl2_tbl = panel->controller_cfg->ctrl2_refresh;
>>> + struct drm_gem_dma_object *dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
>>> + struct iosys_map map;
>>> + int err = 0;
>>> + unsigned int data_size = (panel->width * panel->height) / 8;
>>> + u8 *mono_buffer = NULL;
>>> + u8 *red_buffer = NULL;
>>> + u16 ram_x_start, ram_x_end, ram_y_start, ram_y_end;
>>> +
>>> + /* Process full display area; convert handles orientation
>>> traversal. */
>>> + rect->x1 = 0;
>>> + rect->y1 = 0;
>>> + rect->x2 = panel->width;
>>> + rect->y2 = panel->height;
>>> +
>>> + drm_dbg(&panel->drm,
>>> + "fb_dirty: fb=%dx%d, refresh_mode=%d, orientation=%d\n",
>>> + fb->width, fb->height, panel->refresh_mode, panel-
>>> >orientation);
>>> +
>>> + mono_buffer = panel->tx_buf;
>>> + memset(mono_buffer, 0, data_size);
>>> +
>>> + /* 3-colour FULL/FAST: populate red channel. */
>>> + if (panel->color_mode == SSD16XX_COLOR_MODE_3COLOR &&
>>> + (panel->refresh_mode == SSD16XX_REFRESH_FULL ||
>>> + panel->refresh_mode == SSD16XX_REFRESH_FAST)) {
>>> + red_buffer = panel->tx_red_buf;
>>> + memset(red_buffer, 0, data_size);
>>> + }
>>> +
>>> + iosys_map_set_vaddr(&map, dma_obj->vaddr);
>>> +
>>> + if (red_buffer && fb->format->format == DRM_FORMAT_R8)
>>> + ssd16xx_convert_r8_to_red_only(mono_buffer, red_buffer,
>>> &map, fb, rect);
>>> + else if (red_buffer)
>>> + ssd16xx_convert_fb_to_3color(mono_buffer, red_buffer, &map,
>>> fb, rect);
>>> + else
>>> + ssd16xx_convert_fb_to_1bpp(mono_buffer, &map, fb, rect,
>>> panel->orientation);
>>> +
>>> + drm_dbg(&panel->drm,
>>> + "fb_dirty: mono[0..3]=0x%02x 0x%02x 0x%02x 0x%02x
>>> (data_size=%u)\n",
>>> + mono_buffer[0], mono_buffer[1], mono_buffer[2], mono_buffer[3],
>>> + data_size);
>>> +
>>> + /* Set RAM window and cursor for current orientation. */
>>> + ram_x_start = 0;
>>> + ram_x_end = (panel->controller_cfg->max_width / 8) - 1;
>>> + ram_y_start = 0;
>>> + ram_y_end = panel->controller_cfg->max_height - 1;
>>> +
>>> + switch (panel->orientation) {
>>> + case 90:
>>> + case 180:
>>> + /* 90°/180°: XDEC_YDEC mode, send end-before-start; cursor
>>> at (max, max). */
>>> + ssd16xx_send_cmd(panel,
>>> SSD16XX_CMD_SET_RAM_X_ADDRESS_START_END, &err);
>>> + ssd16xx_send_x_param(panel, ram_x_end, &err);
>>> + ssd16xx_send_x_param(panel, ram_x_start, &err);
>>> +
>>> + ssd16xx_send_cmd(panel,
>>> SSD16XX_CMD_SET_RAM_Y_ADDRESS_START_END, &err);
>>> + ssd16xx_send_y_param(panel, ram_y_end, &err);
>>> + ssd16xx_send_y_param(panel, ram_y_start, &err);
>>> +
>>> + ssd16xx_send_cmd(panel,
>>> SSD16XX_CMD_SET_RAM_X_ADDRESS_COUNTER, &err);
>>> + ssd16xx_send_x_param(panel, ram_x_end, &err);
>>> +
>>> + ssd16xx_send_cmd(panel,
>>> SSD16XX_CMD_SET_RAM_Y_ADDRESS_COUNTER, &err);
>>> + ssd16xx_send_y_param(panel, ram_y_end, &err);
>>> + break;
>>> +
>>> + default: /* 0°/270° */
>>> + /* 0°/270°: XINC_YINC mode, cursor at (0, 0). */
>>> + ssd16xx_send_cmd(panel,
>>> SSD16XX_CMD_SET_RAM_X_ADDRESS_START_END, &err);
>>> + ssd16xx_send_x_param(panel, ram_x_start, &err);
>>> + ssd16xx_send_x_param(panel, ram_x_end, &err);
>>> +
>>> + ssd16xx_send_cmd(panel,
>>> SSD16XX_CMD_SET_RAM_Y_ADDRESS_START_END, &err);
>>> + ssd16xx_send_y_param(panel, ram_y_start, &err);
>>> + ssd16xx_send_y_param(panel, ram_y_end, &err);
>>> +
>>> + ssd16xx_send_cmd(panel,
>>> SSD16XX_CMD_SET_RAM_X_ADDRESS_COUNTER, &err);
>>> + ssd16xx_send_x_param(panel, ram_x_start, &err);
>>> +
>>> + ssd16xx_send_cmd(panel,
>>> SSD16XX_CMD_SET_RAM_Y_ADDRESS_COUNTER, &err);
>>> + ssd16xx_send_y_param(panel, ram_y_start, &err);
>>> + break;
>>> + }
>>> +
>>> + ssd16xx_send_cmd(panel, SSD16XX_CMD_WRITE_RAM_BW, &err);
>>> + ssd16xx_send_data_bulk(panel, mono_buffer, data_size, &err);
>>> +
>>> + /* Re-send border waveform when: every-update mode, init frame, or
>>> + * the border_waveform_update property just changed (one-shot).
>>> + */
>>> + drm_dbg(&panel->drm,
>>> + "fb_dirty: border check: every_update=%d init_pending=%d
>>> border_pending=%d idx=%d hw=0x%02x\n",
>>> + panel->border_refresh_on_every_update, panel-
>>> >init_refresh_pending,
>>> + panel->border_waveform_pending, panel-
>>> >border_waveform_update_idx,
>>> + panel->controller_cfg->border_waveform_table[panel-
>>> >border_waveform_update_idx]);
>>> + if (panel->border_refresh_on_every_update || panel-
>>> >init_refresh_pending ||
>>> + panel->border_waveform_pending) {
>>> + u8 idx = panel->border_waveform_update_idx;
>>> + u8 border = panel->controller_cfg->border_waveform_table[idx];
>>> +
>>> + drm_dbg(&panel->drm, "fb_dirty: Sending border waveform:
>>> 0x%02x\n",
>>> + border);
>>> + ssd16xx_send_cmd(panel, SSD16XX_CMD_BORDER_WAVEFORM_CONTROL,
>>> &err);
>>> + ssd16xx_send_data(panel, border, &err);
>>> + panel->border_waveform_pending = false;
>>> + }
>>> +
>>> + switch (panel->refresh_mode) {
>>> + case SSD16XX_REFRESH_FULL:
>>> + /*
>>> + * BW full refresh: write RED RAM BEFORE display_update
>>> + * to avoid a post-BUSY write timing issue on some
>>> + * controller revisions that silently corrupts RED RAM.
>>> + * RED RAM is then bypassed (CTRL1_BYPASS_RED_RAM) so
>>> + * stale RED RAM content does not affect the output.
>>> + */
>>> + ssd16xx_send_cmd(panel, SSD1683_CMD_WRITE_RAM_RED, &err);
>>> + if (red_buffer) {
>>> + /* 3-colour: write red channel before activating */
>>> + ssd16xx_send_data_bulk(panel, red_buffer, data_size, &err);
>>> + ssd16xx_display_update(panel, panel->controller_cfg-
>>> >ctrl1_normal,
>>> + SSD16XX_CTRL1_BYTE2_DEFAULT,
>>> + ctrl2_tbl[SSD16XX_REFRESH_FULL], &err);
>>> + } else {
>>> + ssd16xx_send_data_bulk(panel, mono_buffer, data_size,
>>> &err);
>>> + ssd16xx_display_update(panel, panel->controller_cfg-
>>> >ctrl1_bypass_red_ram,
>>> + SSD16XX_CTRL1_BYTE2_DEFAULT,
>>> + ctrl2_tbl[SSD16XX_REFRESH_FULL], &err);
>>> + }
>>> + break;
>>> + case SSD16XX_REFRESH_FAST:
>>> + /*
>>> + * Fast refresh: LUT pre-loaded during hw_init; BYPASS_RED_RAM
>>> + * so RED RAM does not affect the current output.
>>> + * Write RED RAM BEFORE display_update (same reasoning as FULL)
>>> + * so it holds the just-displayed frame as a valid reference
>>> for
>>> + * any subsequent PARTIAL refresh.
>>> + */
>>> +
>>> + ssd16xx_send_cmd(panel, SSD1683_CMD_WRITE_RAM_RED, &err);
>>> + if (red_buffer) {
>>> + /* 3-colour: write red channel before activating */
>>> + ssd16xx_send_data_bulk(panel, red_buffer, data_size, &err);
>>> + ssd16xx_display_update(panel, panel->controller_cfg-
>>> >ctrl1_normal,
>>> + SSD16XX_CTRL1_BYTE2_DEFAULT,
>>> + ctrl2_tbl[SSD16XX_REFRESH_FAST], &err);
>>> + } else {
>>> + ssd16xx_send_data_bulk(panel, mono_buffer, data_size,
>>> &err);
>>> + ssd16xx_display_update(panel, panel->controller_cfg-
>>> >ctrl1_bypass_red_ram,
>>> + SSD16XX_CTRL1_BYTE2_DEFAULT,
>>> + ctrl2_tbl[SSD16XX_REFRESH_FAST], &err);
>>> + }
>>> + break;
>>> + case SSD16XX_REFRESH_PARTIAL:
>>> + default:
>>> + /*
>>> + * Partial refresh: both RAMs used for transition waveforms.
>>> + * RED RAM must hold the PREVIOUS frame (= current display
>>> + * content) so the controller can compute pixel transitions.
>>> + * Write RED RAM AFTER display_update so it captures the
>>> + * just-displayed frame as the reference for the next partial.
>>> + */
>>> + drm_dbg(&panel->drm,
>>> + "fb_dirty: partial pre-update: mono[0]=0x%02x (BW=new,
>>> RED=prev)\n",
>>> + mono_buffer[0]);
>>> + ssd16xx_display_update(panel, panel->controller_cfg-
>>> >ctrl1_normal,
>>> + SSD16XX_CTRL1_BYTE2_DEFAULT,
>>> + ctrl2_tbl[SSD16XX_REFRESH_PARTIAL], &err);
>>> + ssd16xx_send_cmd(panel, SSD1683_CMD_WRITE_RAM_RED, &err);
>>> + ssd16xx_send_data_bulk(panel, mono_buffer, data_size, &err);
>>> + drm_dbg(&panel->drm,
>>> + "fb_dirty: partial post-update: wrote RED baseline
>>> mono[0]=0x%02x\n",
>>> + mono_buffer[0]);
>>> + break;
>>> + }
>>> +
>>> + return err;
>>> +}
>>> +
>>> +/*
>>> -----------------------------------------------------------------------------
>>> + * Plane Functions
>>> + */
>>> +
>>> +static void ssd16xx_plane_destroy(struct drm_plane *plane)
>>> +{
>>> + drm_plane_cleanup(plane);
>>> +}
>>> +
>>> +static void ssd16xx_plane_reset(struct drm_plane *plane)
>>> +{
>>> + drm_atomic_helper_plane_reset(plane);
>>> +}
>>
>> Please avoid these wrappers.
>>
>
> Understood, will update in V2.
>
>>> +
>>> +static const struct drm_plane_funcs ssd16xx_plane_funcs = {
>>> + .update_plane = drm_atomic_helper_update_plane,
>>> + .disable_plane = drm_atomic_helper_disable_plane,
>>> + .destroy = ssd16xx_plane_destroy,
>>> + .reset = ssd16xx_plane_reset,
>>> + .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>>> + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
>>> +};
>>> +
>>> +static int ssd16xx_plane_atomic_check(struct drm_plane *plane,
>>> + struct drm_atomic_state *state)
>>> +{
>>> + struct drm_plane_state *new_plane_state =
>>> + drm_atomic_get_new_plane_state(state, plane);
>>> + struct drm_crtc_state *crtc_state;
>>> +
>>> + if (!new_plane_state->crtc)
>>> + return 0;
>>> +
>>> + crtc_state = drm_atomic_get_new_crtc_state(state,
>>> new_plane_state->crtc);
>>> +
>>> + return drm_atomic_helper_check_plane_state(new_plane_state,
>>> crtc_state,
>>> + DRM_PLANE_NO_SCALING,
>>> + DRM_PLANE_NO_SCALING,
>>> + false, false);
>>> +}
>>> +
>>> +static void ssd16xx_plane_atomic_update(struct drm_plane *plane,
>>> + struct drm_atomic_state *state)
>>> +{
>>> + struct drm_plane_state *old_state =
>>> drm_atomic_get_old_plane_state(state, plane);
>>> + struct drm_plane_state *new_state =
>>> drm_atomic_get_new_plane_state(state, plane);
>>> + struct ssd16xx_panel *panel = plane_to_ssd16xx_panel(plane);
>>> + enum ssd16xx_refresh_mode saved_mode;
>>> + u8 saved_border_waveform_idx;
>>> + struct drm_framebuffer *fb = new_state->fb;
>>> + struct drm_rect rect;
>>> + int ret;
>>> +
>>> + drm_dbg(&panel->drm, "plane_atomic_update: fb=%p,
>>> initialized=%d\n",
>>> + fb, panel->initialized);
>>> +
>>> + if (!fb || !panel->initialized)
>>> + return;
>>> +
>>> + /*
>>> + * If a rotation change is pending, skip the update here —
>>> crtc_atomic_flush
>>> + * will re-init the hardware for the new orientation and redraw.
>>> + */
>>> + if (panel->reinit_pending) {
>>> + drm_dbg(&panel->drm, "plane_atomic_update: skipping (reinit
>>> pending)\n");
>>> + return;
>>> + }
>>> +
>>> + if (!drm_atomic_helper_damage_merged(old_state, new_state,
>>> &rect)) {
>>> + rect.x1 = 0;
>>> + rect.y1 = 0;
>>> + rect.x2 = fb->width;
>>> + rect.y2 = fb->height;
>>> + drm_dbg(&panel->drm, "plane_atomic_update: no damage, using
>>> full screen\n");
>>> + }
>>> +
>>> + drm_dbg(&panel->drm, "plane_atomic_update: calling fb_dirty
>>> rect=(%d,%d)-(%d,%d)\n",
>>> + rect.x1, rect.y1, rect.x2, rect.y2);
>>> + /*
>>> + * When refresh_mode_init was set, use the specified mode for
>>> this first
>>> + * frame only, then restore the user-configured refresh_mode so
>>> + * subsequent updates continue with the configured mode.
>>> + */
>>> + saved_mode = panel->refresh_mode;
>>> + saved_border_waveform_idx = panel->border_waveform_update_idx;
>>> + if (panel->init_refresh_pending) {
>>> + panel->refresh_mode = panel->refresh_mode_init;
>>> + panel->border_waveform_update_idx = panel-
>>> >border_waveform_init_idx;
>>> + }
>>> +
>>> + /*
>>> + * Fast refresh (0xC7) omits LOAD_LUT on every update cycle and
>>> relies
>>> + * on the LUT being pre-loaded upfront. The property setter arms
>>> + * fast_lut_pending whenever the user switches into fast mode.
>>> Consume
>>> + * the flag here (once) before the first fast-refresh frame so the
>>> + * controller's LUT is in the correct state.
>>> + */
>>> + if (panel->fast_lut_pending) {
>>> + ret = ssd16xx_preload_fast_lut(panel);
>>> + if (ret) {
>>> + drm_err(&panel->drm,
>>> + "plane_atomic_update: fast LUT preload failed:
>>> %d\n", ret);
>>> + }
>>> +
>>> + panel->fast_lut_pending = false;
>>> + }
>>> +
>>> + ret = ssd16xx_fb_dirty(fb, &rect, panel);
>>> + if (ret)
>>> + drm_err(&panel->drm, "plane_atomic_update: display update
>>> failed: %d\n", ret);
>>> + else
>>> + panel->last_fb = fb;
>>> +
>>> + panel->refresh_mode = saved_mode;
>>> + panel->border_waveform_update_idx = saved_border_waveform_idx;
>>> +
>>> + /*
>>> + * If this was the init frame (which used border_waveform_init_idx
>>> + * inside fb_dirty), arm border_waveform_pending so the normal
>>> + * (non-init) border value is sent at the start of the next update.
>>> + */
>>> + if (panel->init_refresh_pending) {
>>> + panel->init_refresh_pending = false;
>>> + panel->border_waveform_pending = true;
>>> + }
>>> +}
>>> +
>>> +static const struct drm_plane_helper_funcs
>>> ssd16xx_plane_helper_funcs = {
>>> + .atomic_check = ssd16xx_plane_atomic_check,
>>> + .atomic_update = ssd16xx_plane_atomic_update,
>>> +};
>>> +
>>> +/*
>>> -----------------------------------------------------------------------------
>>> + * CRTC Functions
>>> + */
>>> +
>>> +static void ssd16xx_crtc_destroy(struct drm_crtc *crtc)
>>> +{
>>> + drm_crtc_cleanup(crtc);
>>> +}
>>> +
>>> +static const struct drm_crtc_funcs ssd16xx_crtc_funcs = {
>>> + .reset = drm_atomic_helper_crtc_reset,
>>> + .destroy = ssd16xx_crtc_destroy,
>>> + .set_config = drm_atomic_helper_set_config,
>>> + .page_flip = drm_atomic_helper_page_flip,
>>> + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>>> + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>>> +};
>>> +
>>> +static enum drm_mode_status ssd16xx_crtc_mode_valid(struct drm_crtc
>>> *crtc,
>>> + const struct drm_display_mode *mode)
>>> +{
>>> + struct ssd16xx_panel *panel = crtc_to_ssd16xx_panel(crtc);
>>> +
>>> + /* Accept only our panel's native mode (landscape or portrait) */
>>> + if ((mode->hdisplay == panel->mode->hdisplay &&
>>> + mode->vdisplay == panel->mode->vdisplay) ||
>>> + (mode->hdisplay == panel->mode->vdisplay &&
>>> + mode->vdisplay == panel->mode->hdisplay))
>>> + return MODE_OK;
>>> +
>>> + return MODE_BAD;
>>> +}
>>> +
>>> +static int ssd16xx_crtc_atomic_check(struct drm_crtc *crtc,
>>> + struct drm_atomic_state *state)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +static void ssd16xx_crtc_atomic_disable(struct drm_crtc *crtc,
>>> + struct drm_atomic_state *state)
>>> +{
>>> + struct ssd16xx_panel *panel = crtc_to_ssd16xx_panel(crtc);
>>> + int ret, idx;
>>> +
>>> + if (!drm_dev_enter(&panel->drm, &idx))
>>> + return;
>>> +
>>> + if (panel->clear_on_disable < 0 || panel-
>>> >display_cleared_on_deinit)
>>> + goto out;
>>> +
>>> + drm_dbg(&panel->drm, "clear_on_disable: running, mode=%d\n",
>>> + panel->clear_on_disable);
>>> + ret = ssd16xx_clear_display(panel,
>>> + ssd16xx_refresh_mode_to_ctrl2(panel,
>>> + panel->clear_on_disable));
>>> + if (ret) {
>>> + drm_err(&panel->drm, "atomic_disable: clear failed: %d\n",
>>> ret);
>>> + goto out;
>>> + }
>>> +
>>> + panel->display_cleared_on_deinit = true;
>>> +out:
>>> + drm_dev_exit(idx);
>>> +}
>>> +
>>> +static void ssd16xx_crtc_atomic_enable(struct drm_crtc *crtc,
>>> + struct drm_atomic_state *state)
>>> +{
>>> + struct ssd16xx_panel *panel = crtc_to_ssd16xx_panel(crtc);
>>> + struct drm_crtc_state *crtc_state =
>>> drm_atomic_get_new_crtc_state(state, crtc);
>>> + int ret, idx;
>>> +
>>> + if (!drm_dev_enter(&panel->drm, &idx))
>>> + return;
>>> +
>>> + panel->display_cleared_on_deinit = false;
>>> +
>>> + drm_dbg(&panel->drm, "atomic_enable: %dx%d\n",
>>> + crtc_state->mode.hdisplay, crtc_state->mode.vdisplay);
>>> +
>>> + panel->width = crtc_state->mode.hdisplay;
>>> + panel->height = crtc_state->mode.vdisplay;
>>> +
>>> + ret = ssd16xx_hw_init(panel);
>>> + if (ret) {
>>> + drm_err(&panel->drm, "crtc_atomic_enable: HW init failed:
>>> %d\n", ret);
>>> + goto out;
>>> + }
>>> + panel->initialized = true;
>>> +
>>> + /* Clear display on first app launch if configured */
>>> + ret = ssd16xx_clear_display_on_init(panel);
>>> + if (ret)
>>> + drm_err(&panel->drm, "crtc_atomic_enable: clear on init
>>> failed: %d\n", ret);
>>> +
>>> + /*
>>> + * If refresh_mode_init is set, arm init_refresh_pending so
>>> + * plane_atomic_update uses the specified mode for the first frame
>>> + * then restores the user-configured or panel default refresh_mode.
>>> + */
>>> + if (panel->refresh_mode_init >= 0) {
>>> + drm_dbg(&panel->drm,
>>> + "atomic_enable: refresh_mode_init=%d, using for first
>>> frame\n",
>>> + panel->refresh_mode_init);
>>> + panel->init_refresh_pending = true;
>>> + }
>>> +
>>> +out:
>>> + drm_dev_exit(idx);
>>> +}
>>> +
>>> +/*
>>> + * Re-initialize hardware and redraw the current framebuffer when the
>>> + * display orientation changes at runtime via the rotation connector
>>> property.
>>> + * Called by the DRM atomic helper after atomic_enable/disable have
>>> run.
>>> + */
>>> +static void ssd16xx_crtc_atomic_flush(struct drm_crtc *crtc,
>>> + struct drm_atomic_state *state)
>>> +{
>>> + struct ssd16xx_panel *panel = crtc_to_ssd16xx_panel(crtc);
>>> + struct drm_framebuffer *fb;
>>> + struct drm_rect full;
>>> + int ret, idx;
>>> +
>>> + if (!panel->reinit_pending || !panel->initialized)
>>> + return;
>>> +
>>> + if (!drm_dev_enter(&panel->drm, &idx))
>>> + return;
>>> +
>>> + panel->reinit_pending = false;
>>> +
>>> + drm_dbg(&panel->drm, "atomic_flush: reinit, orientation=%u°\n",
>>> + panel->orientation);
>>> +
>>> + ret = ssd16xx_hw_init(panel);
>>> + if (ret) {
>>> + drm_err(&panel->drm, "Orientation re-init failed: %d\n", ret);
>>> + goto out;
>>> + }
>>> +
>>> + fb = panel->primary_plane.state ? panel->primary_plane.state->fb
>>> + : panel->last_fb;
>>> + if (fb) {
>>> + full.x1 = 0;
>>> + full.y1 = 0;
>>> + full.x2 = fb->width;
>>> + full.y2 = fb->height;
>>> + ret = ssd16xx_fb_dirty(fb, &full, panel);
>>> + if (ret)
>>> + drm_err(&panel->drm, "atomic_flush: display update
>>> failed: %d\n", ret);
>>> + else
>>> + panel->last_fb = fb;
>>> + }
>>> +
>>> +out:
>>> + drm_dev_exit(idx);
>>> +}
>>> +
>>> +static const struct drm_crtc_helper_funcs ssd16xx_crtc_helper_funcs = {
>>> + .mode_valid = ssd16xx_crtc_mode_valid,
>>> + .atomic_check = ssd16xx_crtc_atomic_check,
>>> + .atomic_disable = ssd16xx_crtc_atomic_disable,
>>> + .atomic_enable = ssd16xx_crtc_atomic_enable,
>>> + .atomic_flush = ssd16xx_crtc_atomic_flush,
>>> +};
>>> +
>>> +/*
>>> -----------------------------------------------------------------------------
>>> + * Connector Functions
>>> + */
>>> +
>>> +static int ssd16xx_connector_get_modes(struct drm_connector *connector)
>>> +{
>>> + struct ssd16xx_panel *panel = to_ssd16xx_panel(connector->dev);
>>> + bool mode_is_portrait = (panel->mode->hdisplay < panel->mode-
>>> >vdisplay);
>>> + bool orient_is_portrait = (panel->orientation == 90 || panel-
>>> >orientation == 270);
>>> +
>>> + drm_dbg(&panel->drm, "connector_get_modes: orientation=%u°\n",
>>> + panel->orientation);
>>> +
>>> + /* For portrait, swap dimensions so clients see logical size. */
>>> + if (mode_is_portrait != orient_is_portrait) {
>>> + struct drm_display_mode *mode;
>>> +
>>> + mode = drm_mode_duplicate(&panel->drm, panel->mode);
>>> + if (!mode)
>>> + return 0;
>>> + swap(mode->hdisplay, mode->vdisplay);
>>> + swap(mode->hsync_start, mode->vsync_start);
>>> + swap(mode->hsync_end, mode->vsync_end);
>>> + swap(mode->htotal, mode->vtotal);
>>> + swap(mode->width_mm, mode->height_mm);
>>> + mode->type |= DRM_MODE_TYPE_PREFERRED;
>>> + drm_mode_set_name(mode);
>>> + drm_mode_probed_add(connector, mode);
>>> + return 1;
>>> + }
>>> +
>>> + return drm_connector_helper_get_modes_fixed(connector, panel-
>>> >mode);
>>> +}
>>> +
>>> +static const struct drm_connector_helper_funcs
>>> ssd16xx_connector_helper_funcs = {
>>> + .get_modes = ssd16xx_connector_get_modes,
>>> +};
>>> +
>>> +/* Enum values for the rotation connector property (degrees
>>> clockwise) */
>>> +static const struct drm_prop_enum_list ssd16xx_rotation_enum[] = {
>>> + { 0, "0" },
>>> + { 90, "90" },
>>> + { 180, "180" },
>>> + { 270, "270" },
>>> +};
>>> +
>>> +/* Enum values for the refresh_mode connector property */
>>> +static const struct drm_prop_enum_list ssd16xx_refresh_mode_enum[] = {
>>> + { SSD16XX_REFRESH_PARTIAL, "partial" },
>>> + { SSD16XX_REFRESH_FULL, "full" },
>>> + { SSD16XX_REFRESH_FAST, "fast" },
>>> +};
>>> +
>>> +/*
>>> + * Enum for clear_on_init, clear_on_close, refresh_mode_init
>>> properties.
>>> + * Value 0 = disabled; values 1-3 = partial/full/fast (refresh mode
>>> + 1).
>>> + * The +1 offset allows a single enum to represent both "disabled"
>>> and the
>>> + * three refresh modes without sign-extending the DRM property value.
>>> + */
>>> +static const struct drm_prop_enum_list ssd16xx_init_refresh_enum[] = {
>>> + { 0, "disabled" },
>>> + { 1, "partial" },
>>> + { 2, "full" },
>>> + { 3, "fast" },
>>> +};
>>> +
>>> +/* Enum values for the color_mode connector property */
>>> +static const struct drm_prop_enum_list ssd16xx_color_mode_enum[] = {
>>> + { SSD16XX_COLOR_MODE_BW, "black-white" },
>>> + { SSD16XX_COLOR_MODE_3COLOR, "3-color" },
>>> +};
>>> +
>>> +/* Enum values for border_waveform connector properties (one per HW
>>> mode) */
>>> +static const struct drm_prop_enum_list
>>> ssd16xx_border_waveform_enum[] = {
>>> + { SSD16XX_BORDER_LUT0, "lut0_black" },
>>> + { SSD16XX_BORDER_LUT1, "lut1_white" },
>>> + { SSD16XX_BORDER_LUT2, "lut2_black" },
>>> + { SSD16XX_BORDER_LUT3, "lut3_gray" },
>>> + { SSD16XX_BORDER_VSS, "vss_black" },
>>> + { SSD16XX_BORDER_VSH1, "vsh1_black" },
>>> + { SSD16XX_BORDER_VSL, "vsl_white" },
>>> + { SSD16XX_BORDER_VSH2, "vsh2_black" },
>>> + { SSD16XX_BORDER_VCOM, "vcom_preserve" },
>>> + { SSD16XX_BORDER_HIZ, "hiz_float" },
>>> +};
>>> +
>>> +static int ssd16xx_connector_create_properties(struct ssd16xx_panel
>>> *panel)
>>> +{
>>> + struct drm_device *drm = &panel->drm;
>>> + struct drm_connector *connector = &panel->connector;
>>> +
>>> + panel->rotation_property =
>>> + drm_property_create_enum(drm, 0, "rotation",
>>> + ssd16xx_rotation_enum,
>>> + ARRAY_SIZE(ssd16xx_rotation_enum));
>>> + if (!panel->rotation_property)
>>> + return -ENOMEM;
>>> + drm_object_attach_property(&connector->base,
>>> + panel->rotation_property, panel->orientation);
>>> +
>>> + panel->refresh_mode_property =
>>> + drm_property_create_enum(drm, 0, "refresh_mode",
>>> + ssd16xx_refresh_mode_enum,
>>> + ARRAY_SIZE(ssd16xx_refresh_mode_enum));
>>> + if (!panel->refresh_mode_property)
>>> + return -ENOMEM;
>>> + drm_object_attach_property(&connector->base,
>>> + panel->refresh_mode_property, panel->refresh_mode);
>>> +
>>> + panel->border_waveform_init_property =
>>> + drm_property_create_enum(drm, 0, "border_waveform_init",
>>> + ssd16xx_border_waveform_enum,
>>> + ARRAY_SIZE(ssd16xx_border_waveform_enum));
>>> + if (!panel->border_waveform_init_property)
>>> + return -ENOMEM;
>>> + drm_object_attach_property(&connector->base,
>>> + panel->border_waveform_init_property,
>>> + panel->border_waveform_init_idx);
>>> +
>>> + panel->border_waveform_update_property =
>>> + drm_property_create_enum(drm, 0, "border_waveform_update",
>>> + ssd16xx_border_waveform_enum,
>>> + ARRAY_SIZE(ssd16xx_border_waveform_enum));
>>> + if (!panel->border_waveform_update_property)
>>> + return -ENOMEM;
>>> + drm_object_attach_property(&connector->base,
>>> + panel->border_waveform_update_property,
>>> + panel->border_waveform_update_idx);
>>> +
>>> + panel->border_refresh_on_every_update_property =
>>> + drm_property_create_bool(drm, 0,
>>> "border_refresh_on_every_update");
>>> + if (!panel->border_refresh_on_every_update_property)
>>> + return -ENOMEM;
>>> + drm_object_attach_property(&connector->base,
>>> + panel->border_refresh_on_every_update_property,
>>> + panel->border_refresh_on_every_update);
>>> +
>>> + panel->clear_on_init_property =
>>> + drm_property_create_enum(drm, 0, "clear_on_init",
>>> + ssd16xx_init_refresh_enum,
>>> + ARRAY_SIZE(ssd16xx_init_refresh_enum));
>>> + if (!panel->clear_on_init_property)
>>> + return -ENOMEM;
>>> + /* Property value 0=disabled, 1-3=mode; field is -1/0/1/2 → val
>>> = field+1 */
>>> + drm_object_attach_property(&connector->base,
>>> + panel->clear_on_init_property,
>>> + panel->clear_on_init + 1);
>>> +
>>> + panel->clear_on_close_property =
>>> + drm_property_create_enum(drm, 0, "clear_on_close",
>>> + ssd16xx_init_refresh_enum,
>>> + ARRAY_SIZE(ssd16xx_init_refresh_enum));
>>> + if (!panel->clear_on_close_property)
>>> + return -ENOMEM;
>>> + drm_object_attach_property(&connector->base,
>>> + panel->clear_on_close_property,
>>> + panel->clear_on_close + 1);
>>> +
>>> + panel->clear_on_disable_property =
>>> + drm_property_create_enum(drm, 0, "clear_on_disable",
>>> + ssd16xx_init_refresh_enum,
>>> + ARRAY_SIZE(ssd16xx_init_refresh_enum));
>>> + if (!panel->clear_on_disable_property)
>>> + return -ENOMEM;
>>> + drm_object_attach_property(&connector->base,
>>> + panel->clear_on_disable_property,
>>> + panel->clear_on_disable + 1);
>>> +
>>> + panel->refresh_mode_init_property =
>>> + drm_property_create_enum(drm, 0, "refresh_mode_init",
>>> + ssd16xx_init_refresh_enum,
>>> + ARRAY_SIZE(ssd16xx_init_refresh_enum));
>>> + if (!panel->refresh_mode_init_property)
>>> + return -ENOMEM;
>>> + drm_object_attach_property(&connector->base,
>>> + panel->refresh_mode_init_property,
>>> + panel->refresh_mode_init + 1);
>>> +
>>> + /*
>>> + * color_mode: only expose 3-color option on panels that
>>> physically have
>>> + * a red plane; on BW-only panels the property still exists for
>>> + * consistency but userspace can only set "black-white".
>>> + */
>>> + panel->color_mode_property =
>>> + drm_property_create_enum(drm, 0, "color_mode",
>>> + ssd16xx_color_mode_enum,
>>> + panel->panel_cfg->red_supported
>>> + ? ARRAY_SIZE(ssd16xx_color_mode_enum)
>>> + : 1);
>>> + if (!panel->color_mode_property)
>>> + return -ENOMEM;
>>> + drm_object_attach_property(&connector->base,
>>> + panel->color_mode_property,
>>> + panel->color_mode);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int ssd16xx_connector_atomic_get_property(struct
>>> drm_connector *connector,
>>> + const struct drm_connector_state *state,
>>> + struct drm_property *property,
>>> + uint64_t *val)
>>> +{
>>> + struct ssd16xx_panel *panel = to_ssd16xx_panel(connector->dev);
>>> +
>>> + drm_dbg(&panel->drm, "get_property: %s\n", property->name);
>>> +
>>> + if (property == panel->rotation_property) {
>>> + *val = panel->orientation;
>>> + return 0;
>>> + }
>>> + if (property == panel->refresh_mode_property) {
>>> + *val = panel->refresh_mode;
>>> + return 0;
>>> + }
>>> + if (property == panel->border_waveform_init_property) {
>>> + *val = panel->border_waveform_init_idx;
>>> + return 0;
>>> + }
>>> + if (property == panel->border_waveform_update_property) {
>>> + *val = panel->border_waveform_update_idx;
>>> + return 0;
>>> + }
>>> + if (property == panel->border_refresh_on_every_update_property) {
>>> + *val = panel->border_refresh_on_every_update;
>>> + return 0;
>>> + }
>>> + if (property == panel->clear_on_init_property) {
>>> + *val = panel->clear_on_init + 1; /* field -1/0/1/2 → val
>>> 0/1/2/3 */
>>> + return 0;
>>> + }
>>> + if (property == panel->clear_on_close_property) {
>>> + *val = panel->clear_on_close + 1;
>>> + return 0;
>>> + }
>>> + if (property == panel->clear_on_disable_property) {
>>> + *val = panel->clear_on_disable + 1;
>>> + return 0;
>>> + }
>>> + if (property == panel->refresh_mode_init_property) {
>>> + *val = panel->refresh_mode_init + 1;
>>> + return 0;
>>> + }
>>> + if (property == panel->color_mode_property) {
>>> + *val = panel->color_mode;
>>> + return 0;
>>> + }
>>> +
>>> + return -EINVAL;
>>> +}
>>> +
>>> +static int ssd16xx_connector_atomic_set_property(struct
>>> drm_connector *connector,
>>> + struct drm_connector_state *state,
>>> + struct drm_property *property,
>>> + uint64_t val)
>>> +{
>>> + struct ssd16xx_panel *panel = to_ssd16xx_panel(connector->dev);
>>> +
>>> + drm_dbg(&panel->drm, "set_property: %s = %llu\n", property-
>>> >name, val);
>>> +
>>> + if (property == panel->rotation_property) {
>>> + if (val != 0 && val != 90 && val != 180 && val != 270)
>>> + return -EINVAL;
>>> + panel->orientation = val;
>>> + /*
>>> + * Flag hardware re-init needed. crtc_atomic_flush will call
>>> + * ssd16xx_hw_init() with the new orientation and redraw.
>>> + */
>>> + panel->reinit_pending = true;
>>> + return 0;
>>> + }
>>> + if (property == panel->refresh_mode_property) {
>>> + if (val > SSD16XX_REFRESH_FAST)
>>> + return -EINVAL;
>>> + /*
>>> + * Fast refresh (0xC7) omits LOAD_LUT on every update and
>>> relies
>>> + * on the LUT being pre-loaded upfront. Arm the one-shot flag
>>> + * when switching into fast mode so the next
>>> plane_atomic_update
>>> + * loads the LUT before the first fast-refresh cycle. Clear it
>>> + * when switching away so a fresh pre-load happens if the user
>>> + * returns to fast mode later.
>>> + */
>>> + if (val == SSD16XX_REFRESH_FAST &&
>>> + panel->refresh_mode != SSD16XX_REFRESH_FULL)
>>> + panel->fast_lut_pending = true;
>>> + else
>>> + panel->fast_lut_pending = false;
>>> + panel->refresh_mode = val;
>>> + return 0;
>>> + }
>>> + if (property == panel->border_waveform_init_property) {
>>> + if (val >= ARRAY_SIZE(ssd1683_border_waveform_table))
>>> + return -EINVAL;
>>> + panel->border_waveform_init_idx = val;
>>> + return 0;
>>> + }
>>> + if (property == panel->border_waveform_update_property) {
>>> + const u8 *bw_tbl = panel->controller_cfg-
>>> >border_waveform_table;
>>> + bool changed = (int)val != panel->border_waveform_update_idx;
>>> +
>>> + if (val >= ARRAY_SIZE(ssd1683_border_waveform_table))
>>> + return -EINVAL;
>>> + drm_dbg(&panel->drm,
>>> + "set_property: border_waveform_update old=%d new=%llu
>>> hw=0x%02x -> 0x%02x %s\n",
>>> + panel->border_waveform_update_idx, val,
>>> + bw_tbl[panel->border_waveform_update_idx],
>>> + bw_tbl[val],
>>> + changed ? "(arming pending)" : "(no change)");
>>> + /* Arm one-shot flag so the new border value is sent on the
>>> very
>>> + * next display update, even if
>>> border_refresh_on_every_update is
>>> + * not set. Cleared in fb_dirty after the command is sent.
>>> + */
>>> + if ((int)val != panel->border_waveform_update_idx)
>>> + panel->border_waveform_pending = true;
>>> + panel->border_waveform_update_idx = val;
>>> + return 0;
>>> + }
>>> + if (property == panel->border_refresh_on_every_update_property) {
>>> + panel->border_refresh_on_every_update = !!val;
>>> + return 0;
>>> + }
>>> + if (property == panel->clear_on_init_property) {
>>> + if (val > 3)
>>> + return -EINVAL;
>>> + panel->clear_on_init = (int)val - 1; /* val 0/1/2/3 → field
>>> -1/0/1/2 */
>>> + panel->first_clear_done = false; /* allow re-fire on next
>>> enable */
>>> + return 0;
>>> + }
>>> + if (property == panel->clear_on_close_property) {
>>> + if (val > 3)
>>> + return -EINVAL;
>>> + panel->clear_on_close = (int)val - 1;
>>> + return 0;
>>> + }
>>> + if (property == panel->clear_on_disable_property) {
>>> + if (val > 3)
>>> + return -EINVAL;
>>> + panel->clear_on_disable = (int)val - 1;
>>> + return 0;
>>> + }
>>> + if (property == panel->refresh_mode_init_property) {
>>> + if (val > 3)
>>> + return -EINVAL;
>>> + panel->refresh_mode_init = (int)val - 1;
>>> + return 0;
>>> + }
>>> + if (property == panel->color_mode_property) {
>>> + if (val > SSD16XX_COLOR_MODE_3COLOR)
>>> + return -EINVAL;
>>> + if (val == SSD16XX_COLOR_MODE_3COLOR && !panel->panel_cfg-
>>> >red_supported) {
>>> + drm_dbg(&panel->drm,
>>> + "set_property: 3-color mode not supported by this
>>> panel\n");
>>> + return -EINVAL;
>>> + }
>>> + panel->color_mode = val;
>>> + return 0;
>>> + }
>>> +
>>> + return -EINVAL;
>>> +}
>>> +
>>> +static const struct drm_connector_funcs ssd16xx_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,
>>> + .atomic_get_property = ssd16xx_connector_atomic_get_property,
>>> + .atomic_set_property = ssd16xx_connector_atomic_set_property,
>>> +};
>>> +
>>> +static const u32 ssd16xx_formats[] = {
>>> + DRM_FORMAT_XRGB8888, /* 32-bit RGB with padding (preferred) */
>>> + DRM_FORMAT_RGB888, /* 24-bit packed RGB */
>>> + DRM_FORMAT_RGB565, /* 16-bit RGB (5:6:5) */
>>> + DRM_FORMAT_R8, /* 8-bit grayscale */
>>> + DRM_FORMAT_NV12, /* YUV 4:2:0 planar */
>>> + DRM_FORMAT_NV16, /* YUV 4:2:2 planar */
>>> + DRM_FORMAT_YUYV, /* Packed YUV 4:2:2 (Y0 U0 Y1 V0) */
>>> + DRM_FORMAT_UYVY, /* Packed YUV 4:2:2 (U0 Y0 V0 Y1) */
>>> + DRM_FORMAT_R1, /* 1-bit monochrome (native, 8 pixels/
>>> byte) */
>>> +};
>>
>> Why do you have all these formats?
>>
>> Only export the modes your panel can do natively; plus maybe XRGB8888
>> for compatibility.
>>
>
> I wanted to keep YUV formats too since some apps such as camera apps (in
> case we want to click a picture and display over on the e-paper badge
> directly) support only YUV formats but yeah if it's too much I can
> remove them from driver and instead have the conversion in the app itself.
>
>>> +
>>> +DEFINE_DRM_GEM_FOPS(ssd16xx_fops);
>>> +
>>> +/*
>>> + * ssd16xx_drm_master_set - arm init refresh when a new master takes
>>> control.
>>> + */
>>> +static void ssd16xx_drm_master_set(struct drm_device *drm,
>>> + struct drm_file *file, bool from_open)
>>> +{
>>> + struct ssd16xx_panel *panel = to_ssd16xx_panel(drm);
>>> +
>>> + panel->display_cleared_on_deinit = false;
>>> + panel->first_clear_done = false;
>>> +
>>> + if (panel->refresh_mode_init >= 0)
>>> + panel->init_refresh_pending = true;
>>> +}
>>> +
>>> +/*
>>> + * ssd16xx_drm_master_drop - clear display and disarm init refresh
>>> when the
>>> + * master client exits.
>>> + */
>>> +static void ssd16xx_drm_master_drop(struct drm_device *drm,
>>> + struct drm_file *file)
>>> +{
>>> + struct ssd16xx_panel *panel = to_ssd16xx_panel(drm);
>>> + int ret;
>>> +
>>> + panel->init_refresh_pending = false;
>>> + panel->first_clear_done = false;
>>> +
>>> + if (panel->clear_on_close < 0 || panel->display_cleared_on_deinit)
>>> + return;
>>> +
>>> + ret = ssd16xx_clear_display_on_exit(panel);
>>> + if (ret)
>>> + drm_err(drm, "master_drop: clear on close failed: %d\n", ret);
>>> +
>>> + panel->display_cleared_on_deinit = true;
>>> +}
>>
>> No, don't overload these. Just remove all this. Clearing should be
>> left to the DRM client.
>>
>
> Yes, the choice to clear or not to clear is left to drm client depending
> on drm property setting done by drm client, the driver clears the
> display. It would be difficult to update all different apps to pass a
> blank white buffer to clear the screen and what if the app gets closed
> abruptly (as master drop callback will get triggered), then in that case
> the current driver logic ensures that screen gets cleared. In normal LCD
> displays if app gets closed abruptly, the display would have gone-off
> automatically as signals would stop getting transmitted but in e-paper
> panel the last display context would remain and I think it is driver
> responsibility to clear that if that was the policy communicated by
> application to the driver.
>
>>> +
>>> +static struct drm_driver ssd16xx_drm_driver = {
>>> + .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
>>> + .fops = &ssd16xx_fops,
>>> + .name = "ssd16xx",
>>> + .desc = "DRM driver for SSD16xx e-paper controller family",
>>> + .major = 1,
>>> + .minor = 0,
>>> + .master_set = ssd16xx_drm_master_set,
>>> + .master_drop = ssd16xx_drm_master_drop,
>>> + DRM_GEM_DMA_DRIVER_OPS,
>>> + DRM_FBDEV_DMA_DRIVER_OPS,
>>> +};
>>> +
>>> +static const struct drm_mode_config_funcs ssd16xx_mode_config_funcs = {
>>> + .fb_create = drm_gem_fb_create_with_dirty,
>>> + .atomic_check = drm_atomic_helper_check,
>>> + .atomic_commit = drm_atomic_helper_commit,
>>> +};
>>> +
>>> +/*
>>> + * Use the RPM commit-tail variant so that
>>> drm_atomic_helper_commit_modeset_enables
>>> + * (which calls crtc_atomic_enable) runs before
>>> drm_atomic_helper_commit_planes.
>>> + * Without this, the standard commit_tail calls commit_planes before
>>> + * modeset_enables, so plane_atomic_update would see initialized ==
>>> false on the
>>> + * first commit and silently drop the frame.
>>> + */
>>> +static const struct drm_mode_config_helper_funcs
>>> ssd16xx_mode_config_helper_funcs = {
>>> + .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
>>> +};
>>> +
>>> +static int ssd16xx_alloc_tx_bufs(struct ssd16xx_panel *panel)
>>> +{
>>> + struct device *dev = &panel->spi->dev;
>>> + size_t frame_size = (panel->controller_cfg->max_width *
>>> + panel->controller_cfg->max_height) / 8;
>>> +
>>> + panel->tx_buf = devm_kmalloc(dev, frame_size, GFP_KERNEL);
>>
>> drmm_kmalloc() here and for the other buffers.
>>
>
> Understood, thanks for pointing will fix it in V2.
>
> Best Regards
> Devarsh
>
>> Best regards
>> Thomas
>
>>
>>> + if (!panel->tx_buf)
>>> + return -ENOMEM;
>>> +
>>> + if (panel->panel_cfg->red_supported) {
>>> + panel->tx_red_buf = devm_kmalloc(dev, frame_size, GFP_KERNEL);
>>> + if (!panel->tx_red_buf)
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + if (!panel->dc) {
>>> + panel->tx_buf9 = devm_kmalloc_array(dev, frame_size,
>>> + sizeof(u16), GFP_KERNEL);
>>> + if (!panel->tx_buf9)
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int ssd16xx_probe(struct spi_device *spi)
>>> +{
>>> + struct device *dev = &spi->dev;
>>> + struct ssd16xx_panel *panel;
>>> + struct drm_device *drm;
>>> + const struct spi_device_id *spi_id;
>>> + struct drm_display_mode *mode;
>>> + const void *match;
>>> + enum ssd16xx_model model;
>>> + u32 dt_rotation = 0;
>>> + int ret;
>>> +
>>> + match = device_get_match_data(dev);
>>> + if (match) {
>>> + model = (enum ssd16xx_model)(uintptr_t)match;
>>> + } else {
>>> + spi_id = spi_get_device_id(spi);
>>> + model = (enum ssd16xx_model)spi_id->driver_data;
>>> + }
>>> +
>>> + if (!dev->coherent_dma_mask) {
>>> + ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64));
>>> + if (ret) {
>>> + dev_warn(dev, "Failed to set DMA mask: %d\n", ret);
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> + panel = devm_drm_dev_alloc(dev, &ssd16xx_drm_driver,
>>> + struct ssd16xx_panel, drm);
>>> + if (IS_ERR(panel))
>>> + return PTR_ERR(panel);
>>> +
>>> + drm = &panel->drm;
>>> + panel->spi = spi;
>>> + panel->model = model;
>>> + spi_set_drvdata(spi, panel);
>>> +
>>> + spi->mode = SPI_MODE_0;
>>> + spi->bits_per_word = SSD16XX_SPI_BITS_PER_WORD;
>>> +
>>> + if (!spi->max_speed_hz) {
>>> + drm_warn(drm, "spi-max-frequency not specified, using %u Hz\n",
>>> + SSD16XX_SPI_SPEED_DEFAULT);
>>> + spi->max_speed_hz = SSD16XX_SPI_SPEED_DEFAULT;
>>> + }
>>> +
>>> + ret = spi_setup(spi);
>>> + if (ret < 0) {
>>> + drm_err(drm, "SPI setup failed: %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + switch (model) {
>>> + case GDEY042T81:
>>> + panel->controller = SSD1683;
>>> + break;
>>> + default:
>>> + drm_err(drm, "Unknown panel model: %d\n", model);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (panel->controller >= ARRAY_SIZE(ssd16xx_controller_configs) ||
>>> + !ssd16xx_controller_configs[panel->controller].max_width)
>>> + return -EINVAL;
>>> + panel->controller_cfg = &ssd16xx_controller_configs[panel-
>>> >controller];
>>> +
>>> + if (model >= ARRAY_SIZE(ssd16xx_panel_configs))
>>> + return -EINVAL;
>>> + panel->panel_cfg = &ssd16xx_panel_configs[model];
>>> +
>>> + mode = devm_kmemdup(dev, panel->panel_cfg->mode,
>>> + sizeof(*panel->panel_cfg->mode), GFP_KERNEL);
>>> + if (!mode)
>>> + return -ENOMEM;
>>> +
>>> + panel->refresh_mode = panel->panel_cfg->default_refresh_mode;
>>> + /* Default color mode: 3-color for panels with red plane, BW
>>> otherwise */
>>> + panel->color_mode = panel->panel_cfg->red_supported
>>> + ? SSD16XX_COLOR_MODE_3COLOR
>>> + : SSD16XX_COLOR_MODE_BW;
>>> + panel->border_waveform_init_idx = panel->panel_cfg-
>>> >default_border_waveform_init;
>>> + panel->border_waveform_update_idx = panel->panel_cfg-
>>> >default_border_waveform_update;
>>> + panel->border_refresh_on_every_update =
>>> + panel->panel_cfg->default_border_refresh_on_every_update;
>>> + panel->clear_on_init = panel->panel_cfg->default_clear_on_init;
>>> + panel->clear_on_close = panel->panel_cfg->default_clear_on_close;
>>> + panel->clear_on_disable = panel->panel_cfg-
>>> >default_clear_on_disable;
>>> + panel->refresh_mode_init = panel->panel_cfg-
>>> >default_refresh_mode_init;
>>> +
>>> + /* Module parameter overrides for border/display control */
>>> + if (border_waveform_init_lut >= 0 &&
>>> + border_waveform_init_lut <
>>> (int)ARRAY_SIZE(ssd1683_border_waveform_table))
>>> + panel->border_waveform_init_idx = border_waveform_init_lut;
>>> + if (border_waveform_lut >= 0 &&
>>> + border_waveform_lut <
>>> (int)ARRAY_SIZE(ssd1683_border_waveform_table))
>>> + panel->border_waveform_update_idx = border_waveform_lut;
>>> + if (border_refresh_on_every_update)
>>> + panel->border_refresh_on_every_update = true;
>>> + if (clear_on_init >= 0 && clear_on_init <= 2)
>>> + panel->clear_on_init = clear_on_init;
>>> + if (clear_on_close >= 0 && clear_on_close <= 2)
>>> + panel->clear_on_close = clear_on_close;
>>> + if (clear_on_disable >= 0 && clear_on_disable <= 2)
>>> + panel->clear_on_disable = clear_on_disable;
>>> + if (refresh_mode_init >= 0 && refresh_mode_init <= 2)
>>> + panel->refresh_mode_init = refresh_mode_init;
>>> +
>>> + /* Module parameter overrides panel default refresh mode when
>>> set */
>>> + if (refresh_mode >= 0) {
>>> + if (refresh_mode > SSD16XX_REFRESH_FAST)
>>> + drm_warn(drm, "Invalid refresh_mode module param %d,
>>> ignored\n",
>>> + refresh_mode);
>>> + else
>>> + panel->refresh_mode = refresh_mode;
>>> + }
>>> +
>>> + /* Module parameter overrides panel default color mode when set */
>>> + if (color_mode >= 0) {
>>> + if (color_mode > SSD16XX_COLOR_MODE_3COLOR)
>>> + drm_warn(drm, "Invalid color_mode module param %d,
>>> ignored\n",
>>> + color_mode);
>>> + else if (color_mode == SSD16XX_COLOR_MODE_3COLOR &&
>>> + !panel->panel_cfg->red_supported)
>>> + drm_warn(drm,
>>> + "color_mode=3-color requested but panel has no red
>>> plane, ignored\n");
>>> + else
>>> + panel->color_mode = color_mode;
>>> + }
>>> +
>>> + /* Parse "rotation" DT property; swap mode dimensions for
>>> portrait. */
>>> + device_property_read_u32(dev, "rotation", &dt_rotation);
>>> + if (dt_rotation != 0 && dt_rotation != 90 && dt_rotation != 180
>>> && dt_rotation != 270) {
>>> + drm_warn(drm, "Invalid DT rotation %u, defaulting to 0°\n",
>>> dt_rotation);
>>> + dt_rotation = 0;
>>> + }
>>> + panel->orientation = dt_rotation;
>>> +
>>> + /* Module parameter overrides DT rotation when set */
>>> + if (rotation >= 0) {
>>> + if (rotation != 0 && rotation != 90 && rotation != 180 &&
>>> rotation != 270)
>>> + drm_warn(drm, "Invalid rotation module param %d,
>>> ignored\n",
>>> + rotation);
>>> + else
>>> + panel->orientation = rotation;
>>> + }
>>> +
>>> + drm_dbg(drm, "Using %s orientation (%u°, %ux%u logical)\n",
>>> + (panel->orientation == 90 || panel->orientation == 270) ?
>>> "portrait" : "landscape",
>>> + panel->orientation, mode->hdisplay, mode->vdisplay);
>>> +
>>> + /* Swap mode dimensions for portrait so clients see logical
>>> size. */
>>> + if (panel->orientation == 90 || panel->orientation == 270) {
>>> + swap(mode->hdisplay, mode->vdisplay);
>>> + swap(mode->hsync_start, mode->vsync_start);
>>> + swap(mode->hsync_end, mode->vsync_end);
>>> + swap(mode->htotal, mode->vtotal);
>>> + swap(mode->width_mm, mode->height_mm);
>>> + drm_dbg(drm, "Mode dimensions swapped for portrait: %ux%u\n",
>>> + mode->hdisplay, mode->vdisplay);
>>> + } else {
>>> + drm_dbg(drm, "Mode dimensions unchanged: %ux%u\n",
>>> + mode->hdisplay, mode->vdisplay);
>>> + }
>>> + panel->mode = mode;
>>> + panel->width = mode->hdisplay;
>>> + panel->height = mode->vdisplay;
>>> +
>>> + /* Acquire GPIOs. */
>>> + panel->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
>>> + if (IS_ERR(panel->reset))
>>> + return dev_err_probe(dev, PTR_ERR(panel->reset), "Failed to
>>> get RESET GPIO\n");
>>> +
>>> + panel->busy = devm_gpiod_get(dev, "busy", GPIOD_IN);
>>> + if (IS_ERR(panel->busy))
>>> + return dev_err_probe(dev, PTR_ERR(panel->busy), "Failed to
>>> get BUSY GPIO\n");
>>> +
>>> + panel->dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
>>> + if (IS_ERR(panel->dc))
>>> + return dev_err_probe(dev, PTR_ERR(panel->dc), "Failed to get
>>> DC GPIO\n");
>>> + if (!panel->dc) {
>>> + if (!spi_is_bpw_supported(spi, 9))
>>> + return dev_err_probe(dev, -EINVAL,
>>> + "3-wire SPI mode requires 9-bit word
>>> support\n");
>>> + drm_dbg(drm, "dc-gpios not specified, using 3-wire (9-bit)
>>> SPI mode\n");
>>> + }
>>> +
>>> + ret = ssd16xx_alloc_tx_bufs(panel);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ssd16xx_hw_reset(panel);
>>> +
>>> + ret = drmm_mode_config_init(drm);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + drm->mode_config.funcs = &ssd16xx_mode_config_funcs;
>>> + drm->mode_config.helper_private =
>>> &ssd16xx_mode_config_helper_funcs;
>>> + drm->mode_config.min_width = min(panel->width, panel->height);
>>> + drm->mode_config.max_width = max(panel->width, panel->height);
>>> + drm->mode_config.min_height = min(panel->width, panel->height);
>>> + drm->mode_config.max_height = max(panel->width, panel->height);
>>> +
>>> + drm_connector_helper_add(&panel->connector,
>>> &ssd16xx_connector_helper_funcs);
>>> + ret = drm_connector_init(drm, &panel->connector,
>>> &ssd16xx_connector_funcs,
>>> + DRM_MODE_CONNECTOR_SPI);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = drm_universal_plane_init(drm, &panel->primary_plane, 0,
>>> + &ssd16xx_plane_funcs,
>>> + ssd16xx_formats, ARRAY_SIZE(ssd16xx_formats),
>>> + NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
>>> + if (ret)
>>> + return ret;
>>> + drm_plane_helper_add(&panel->primary_plane,
>>> &ssd16xx_plane_helper_funcs);
>>> + drm_plane_enable_fb_damage_clips(&panel->primary_plane);
>>> +
>>> + ret = drm_crtc_init_with_planes(drm, &panel->crtc, &panel-
>>> >primary_plane,
>>> + NULL, &ssd16xx_crtc_funcs, NULL);
>>> + if (ret)
>>> + return ret;
>>> + drm_crtc_helper_add(&panel->crtc, &ssd16xx_crtc_helper_funcs);
>>> +
>>> + ret = drm_simple_encoder_init(drm, &panel->encoder,
>>> DRM_MODE_ENCODER_NONE);
>>> + if (ret)
>>> + return ret;
>>> + panel->encoder.possible_crtcs = drm_crtc_mask(&panel->crtc);
>>> +
>>> + ret = drm_connector_attach_encoder(&panel->connector, &panel-
>>> >encoder);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = ssd16xx_connector_create_properties(panel);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + drm_mode_config_reset(drm);
>>> +
>>> + ret = drm_dev_register(drm, 0);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + drm_dbg(drm, "SSD16xx e-paper display initialized (%dx%d, %d°
>>> rotation)\n",
>>> + panel->width, panel->height, panel->orientation);
>>> +
>>> + drm_client_setup(drm, NULL);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void ssd16xx_remove(struct spi_device *spi)
>>> +{
>>> + struct ssd16xx_panel *panel = spi_get_drvdata(spi);
>>> +
>>> + drm_dev_unplug(&panel->drm);
>>> + drm_atomic_helper_shutdown(&panel->drm);
>>> +}
>>> +
>>> +static void ssd16xx_shutdown(struct spi_device *spi)
>>> +{
>>> + struct ssd16xx_panel *panel = spi_get_drvdata(spi);
>>> +
>>> + drm_atomic_helper_shutdown(&panel->drm);
>>> +}
>>> +
>>> +static const struct of_device_id ssd16xx_of_match[] = {
>>> + { .compatible = "gooddisplay,gdey042t81", .data = (void
>>> *)GDEY042T81 },
>>> + { }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, ssd16xx_of_match);
>>> +
>>> +static const struct spi_device_id ssd16xx_id[] = {
>>> + { "gdey042t81", GDEY042T81 },
>>> + { }
>>> +};
>>> +MODULE_DEVICE_TABLE(spi, ssd16xx_id);
>>> +
>>> +static struct spi_driver ssd16xx_spi_driver = {
>>> + .driver = {
>>> + .name = "ssd16xx",
>>> + .of_match_table = ssd16xx_of_match,
>>> + },
>>> + .probe = ssd16xx_probe,
>>> + .remove = ssd16xx_remove,
>>> + .shutdown = ssd16xx_shutdown,
>>> + .id_table = ssd16xx_id,
>>> +};
>>> +module_spi_driver(ssd16xx_spi_driver);
>>> +
>>> +MODULE_AUTHOR("Devarsh Thakkar <devarsht@ti.com>");
>>> +MODULE_DESCRIPTION("DRM driver for Solomon SSD16xx e-paper display
>>> controller family");
>>> +MODULE_LICENSE("GPL");
>>
>
^ permalink raw reply
* [PATCH v4 2/2] hw_random: timeriomem-rng: add configurable read width and data mask
From: Jad Keskes @ 2026-06-17 11:46 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Olivia Mackall, Herbert Xu, Rob Herring, Conor Dooley,
Alexander Clouter, linux-crypto, devicetree, linux-kernel,
Jad Keskes
In-Reply-To: <20260617114436.1909659-1-inasj268@gmail.com>
The TODO for supporting read sizes other than 32 bits and masking has
been sitting in this driver since 2009. Implement it.
Add reg-io-width (1, 2, or 4 bytes) and mask support. The read loop
dispatches on width using readb/readw/readl so a configured 1-byte
access doesn't trigger a bus error on hardware that rejects 32-bit
reads to that address. The mask is ANDed with the value before storing.
These are platform properties, not runtime policy -- width depends on
SoC integration, mask reflects which output bits carry entropy.
The alignment check in probe is updated to verify the resource is
aligned to the configured width instead of hardcoding 4-byte alignment.
Signed-off-by: Jad Keskes <inasj268@gmail.com>
---
drivers/char/hw_random/timeriomem-rng.c | 77 ++++++++++++++++++++-----
include/linux/timeriomem-rng.h | 12 ++++
2 files changed, 76 insertions(+), 13 deletions(-)
diff --git a/drivers/char/hw_random/timeriomem-rng.c b/drivers/char/hw_random/timeriomem-rng.c
index e61f06393209..42393409f22a 100644
--- a/drivers/char/hw_random/timeriomem-rng.c
+++ b/drivers/char/hw_random/timeriomem-rng.c
@@ -14,7 +14,9 @@
* has to do is provide the address and 'wait time' that new data becomes
* available.
*
- * TODO: add support for reading sizes other than 32bits and masking
+ * The read width (8, 16, or 32 bits) and an optional data mask can be
+ * configured through platform data or device tree properties. Default is
+ * 32-bit reads with no mask.
*/
#include <linux/completion.h>
@@ -34,6 +36,8 @@ struct timeriomem_rng_private {
void __iomem *io_base;
ktime_t period;
unsigned int present:1;
+ unsigned int reg_io_width;
+ u32 mask;
struct hrtimer timer;
struct completion completion;
@@ -48,6 +52,7 @@ static int timeriomem_rng_read(struct hwrng *hwrng, void *data,
container_of(hwrng, struct timeriomem_rng_private, rng_ops);
int retval = 0;
int period_us = ktime_to_us(priv->period);
+ int chunk = priv->reg_io_width;
/*
* There may not have been enough time for new data to be generated
@@ -71,11 +76,28 @@ static int timeriomem_rng_read(struct hwrng *hwrng, void *data,
usleep_range(period_us,
period_us + max(1, period_us / 100));
- *(u32 *)data = readl(priv->io_base);
- retval += sizeof(u32);
- data += sizeof(u32);
- max -= sizeof(u32);
- } while (wait && max > sizeof(u32));
+ switch (priv->reg_io_width) {
+ case 1: {
+ u8 val = readb(priv->io_base) & priv->mask;
+ *(u8 *)data = val;
+ break;
+ }
+ case 2: {
+ u16 val = readw(priv->io_base) & priv->mask;
+ *(u16 *)data = val;
+ break;
+ }
+ case 4: {
+ u32 val = readl(priv->io_base) & priv->mask;
+ *(u32 *)data = val;
+ break;
+ }
+ }
+
+ retval += chunk;
+ data += chunk;
+ max -= chunk;
+ } while (wait && max > chunk);
/*
* Block any new callers until the RNG has had time to generate new
@@ -125,11 +147,8 @@ static int timeriomem_rng_probe(struct platform_device *pdev)
if (IS_ERR(priv->io_base))
return PTR_ERR(priv->io_base);
- if (res->start % 4 != 0 || resource_size(res) < 4) {
- dev_err(&pdev->dev,
- "address must be at least four bytes wide and 32-bit aligned\n");
- return -EINVAL;
- }
+ priv->reg_io_width = 4;
+ priv->mask = 0xFFFFFFFF;
if (pdev->dev.of_node) {
int i;
@@ -145,9 +164,41 @@ static int timeriomem_rng_probe(struct platform_device *pdev)
if (!of_property_read_u32(pdev->dev.of_node,
"quality", &i))
priv->rng_ops.quality = i;
+
+ of_property_read_u32(pdev->dev.of_node,
+ "reg-io-width", &priv->reg_io_width);
+ of_property_read_u32(pdev->dev.of_node,
+ "mask", &priv->mask);
} else {
period = pdata->period;
priv->rng_ops.quality = pdata->quality;
+
+ if (pdata->reg_io_width_set)
+ priv->reg_io_width = pdata->reg_io_width;
+ if (pdata->mask_set)
+ priv->mask = pdata->mask;
+ }
+
+ if (priv->reg_io_width == 0)
+ priv->reg_io_width = 4;
+
+ switch (priv->reg_io_width) {
+ case 1:
+ case 2:
+ case 4:
+ break;
+ default:
+ dev_err(&pdev->dev, "invalid reg-io-width %u, must be 1, 2, or 4\n",
+ priv->reg_io_width);
+ return -EINVAL;
+ }
+
+ if (!IS_ALIGNED(res->start, priv->reg_io_width) ||
+ resource_size(res) < priv->reg_io_width) {
+ dev_err(&pdev->dev,
+ "address must be %u-byte aligned\n",
+ priv->reg_io_width);
+ return -EINVAL;
}
priv->period = us_to_ktime(period);
@@ -167,8 +218,8 @@ static int timeriomem_rng_probe(struct platform_device *pdev)
return err;
}
- dev_info(&pdev->dev, "32bits from 0x%p @ %dus\n",
- priv->io_base, period);
+ dev_info(&pdev->dev, "%u-byte from %p @ %dus\n",
+ priv->reg_io_width, priv->io_base, period);
return 0;
}
diff --git a/include/linux/timeriomem-rng.h b/include/linux/timeriomem-rng.h
index 672df7fbf6c1..5732489a17a1 100644
--- a/include/linux/timeriomem-rng.h
+++ b/include/linux/timeriomem-rng.h
@@ -16,6 +16,18 @@ struct timeriomem_rng_data {
/* bits of entropy per 1024 bits read */
unsigned int quality;
+
+ /* read width (1, 2, or 4 bytes), 0 means 4 */
+ unsigned int reg_io_width;
+
+ /* set to true if reg-io-width is explicitly provided */
+ bool reg_io_width_set;
+
+ /* mask applied to raw read value */
+ u32 mask;
+
+ /* set to true if mask is explicitly provided */
+ bool mask_set;
};
#endif /* _LINUX_TIMERIOMEM_RNG_H */
--
2.54.0
^ permalink raw reply related
* [PATCH v4 1/2] dt-bindings: rng: timeriomem_rng: add reg-io-width and mask properties
From: Jad Keskes @ 2026-06-17 11:44 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Olivia Mackall, Herbert Xu, Rob Herring, Conor Dooley,
Alexander Clouter, linux-crypto, devicetree, linux-kernel,
Jad Keskes
Add optional reg-io-width (1, 2, or 4 bytes) and mask properties to the
binding. reg-io-width selects the bus access size, mask is ANDed with
the raw register value to allow only the entropy-bearing bits through.
Update the example to show a typical 1-byte configuration.
Update SPDX to dual license to match kernel convention.
Drop the misleading '32-bit aligned' constraint from the reg
description since alignment now depends on the configured width.
Signed-off-by: Jad Keskes <inasj268@gmail.com>
---
.../bindings/rng/timeriomem_rng.yaml | 48 +++++++++++++++----
1 file changed, 40 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml b/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml
index 4754174e9849..740bc52bf474 100644
--- a/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml
+++ b/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml
@@ -1,10 +1,16 @@
-# SPDX-License-Identifier: GPL-2.0-only
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
%YAML 1.2
---
$id: http://devicetree.org/schemas/rng/timeriomem_rng.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#
-title: TimerIO Random Number Generator
+title: Timer IOMEM Hardware Random Number Generator
+
+description: |
+ This binding covers platforms that have a single IO memory address which
+ provides periodic random data. The driver reads from the address at a
+ fixed interval, returning a configurable-width value masked to the desired
+ bits.
maintainers:
- Krzysztof Kozlowski <krzk@kernel.org>
@@ -13,9 +19,17 @@ properties:
compatible:
const: timeriomem_rng
+ reg:
+ maxItems: 1
+ description:
+ Base address to sample from. Must be aligned to the configured access
+ width (1, 2, or 4 bytes) and at least that wide.
+
period:
$ref: /schemas/types.yaml#/definitions/uint32
- description: wait time in microseconds to use between samples
+ description:
+ Interval in microseconds between reads. New random data is expected to
+ be available at this rate.
quality:
$ref: /schemas/types.yaml#/definitions/uint32
@@ -26,16 +40,26 @@ properties:
instead. Note that the default quality is usually zero which disables
using this rng to automatically fill the kernel's entropy pool.
- reg:
- maxItems: 1
+ reg-io-width:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ default: 4
+ enum: [1, 2, 4]
description:
- Base address to sample from. Currently 'reg' must be at least four bytes
- wide and 32-bit aligned.
+ Access width in bytes. Determines whether the read is performed as
+ an 8-bit, 16-bit, or 32-bit bus access.
+
+ mask:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ default: 0xFFFFFFFF
+ description:
+ Mask applied to the value read from the register. Bits set to 0 in
+ the mask are cleared in the output data. Default (no mask) passes
+ all bits through.
required:
- compatible
- - period
- reg
+ - period
additionalProperties: false
@@ -46,3 +70,11 @@ examples:
reg = <0x44 0x04>;
period = <1000000>;
};
+
+ rng@64 {
+ compatible = "timeriomem_rng";
+ reg = <0x64 0x01>;
+ period = <50000>;
+ reg-io-width = <1>;
+ mask = <0xFF>;
+ };
--
2.54.0
^ permalink raw reply related
* Re: [PATCH 1/4] dt-bindings: mtd: qcom,nandc: Add MDM9607 QPIC NAND controller
From: Konrad Dybcio @ 2026-06-17 11:42 UTC (permalink / raw)
To: Stephan Gerhold, Miquel Raynal, Manivannan Sadhasivam,
Kathiravan Thirumoorthy
Cc: Krzysztof Kozlowski, Manivannan Sadhasivam, Richard Weinberger,
Vignesh Raghavendra, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-mtd, linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <aifktn_s2qmg7MzI@linaro.org>
On 6/9/26 12:02 PM, Stephan Gerhold wrote:
> On Tue, Jun 09, 2026 at 11:30:54AM +0200, Miquel Raynal wrote:
>> On 09/06/2026 at 11:08:03 +02, Stephan Gerhold <stephan.gerhold@linaro.org> wrote:
>>
>>> On Tue, Jun 09, 2026 at 11:01:18AM +0200, Konrad Dybcio wrote:
>>>> On 6/9/26 10:55 AM, Konrad Dybcio wrote:
>>>>> On 6/9/26 10:10 AM, Stephan Gerhold wrote:
>>>>>> On Tue, Jun 09, 2026 at 09:52:51AM +0200, Miquel Raynal wrote:
>>>>>>>>> On MDM9607, there is only a single controllable clock for the NAND
>>>>>>>>> controller (RPM_SMD_QPIC_CLK). The same situation also applies e.g. for
>>>>>>>>> qcom,sdx55-nand, but the corresponding device tree (qcom-sdx55.dtsi) works
>>>>>>>>> around that by assigning a dummy clock (&nand_clk_dummy) to the second
>>>>>>>>> clock ("aon") that is required by the dt-bindings. This is not really
>>>>>>>>> useful, so avoid doing that for new platforms by excluding the second "aon"
>>>>>>>>> clock entry in the dt-bindings.
>>>>>>>>
>>>>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
>>>>>>>
>>>>>>> What is the problem in giving twice the same clock? If this is what is
>>>>>>> done in the hardware routing, I do not see the reason for more
>>>>>>> complexity in the binding?
>>>>>>>
>>>>>>
>>>>>> I had that in my first draft for this series, but this would be wrong
>>>>>> IMO. I suspect there is no QPIC/NAND related "aon" (always-on) clock on
>>>>>> this platform at all. I'm not sure about MDM9607 in particular (maybe
>>>>>> someone from Qualcomm can confirm), but a similar platform I was looking
>>>>>> into at some point actually had *3* separate clocks for QPIC in the
>>>>>> hardware and none of them were called "aon" ...
>>>>>
>>>>> gcc_qpic_ahb_clk (50/100/133.(3) MHz sourced from PCNoC_bfdcd_clk_src)
>>>>> gcc_qpic_clk (likewise, sourced from qpic_clk_src which is sourced
>>>>> from GPLLs)
>>>>> gcc_qpic_system_clk (32 KHz)
>>>>>
>>>>> No clock containing the substring 'aon' in its name on this platform
>>>>
>>>> Looking at SDX65, perhaps the 32 Khz clock is the "aon" one after all..
>>>> The NAND documentation says
>>>>
>>>> CC_QPIC_SYSTEM_CLK - Always-on timeout clock (32 KHz)
>>>>
>>>
>>> Thanks for looking this up.
>>>
>>> IMO, if we want to describe the actual hardware routing, we should
>>> describe all 3 clocks and assign all of them to RPM_SMD_QPIC_CLK for
>>> MDM9607).
>>
>> Sounds more accurate to me.
>>
>>> The resulting diff would be basically the same as this patch just
>>> inversed (3 clocks for MDM9607+SDX(?) and 2 clocks for the IPQ* SoCs.
>>
>> Diff would not be simpler but more accurate. So if we go for a
>> modification of the bindings, I would prefer that path.
>>
>
> IMO the result wouldn't be much more accurate from the perspective of
> the kernel. If we assign RPM_SMD_QPIC_CLK to all 3 clocks we would be
> effectively saying "there is a single clock with a single rate that is
> sourcing 'core', 'ahb' and 'system'(/'aon')". But in reality, these are
> 3 separate clock domains with separate rates, as shown by Konrad above.
>
> We could try defining dummy clocks like the &nand_clk_dummy in
> qcom-sdx55.dtsi, but this isn't very accurate either. Presumably, all of
> these clocks are toggled by RPM_SMD_QPIC_CLK. So if we define a dummy
> clock for 'ahb', then enabling that clock without also enabling the
> non-dummy 'core' (RPM_SMD_QPIC_CLK) will do nothing.
I can't find a good answer for what RPM_SMD_QPIC_CLK controls, maybe
+Mani or +Kathiravan know where to look
Konrad
>
> At the end, the truth for the OS/kernel running on this hardware is that
> it can only see the 'core' clock (with the option to change its rate).
> All others are invisible, with no way to influence or check the status,
> so pretending that we have separate resources for them doesn't really
> make things more accurate in my opinion.
>
> But yeah, let's leave the decision up to Krzysztof. I'm happy to change
> this patch as needed as long it works at the end. :-)
^ permalink raw reply
* Re: [PATCH v3 2/2] hw_random: timeriomem-rng: add configurable read width and data mask
From: sashiko-bot @ 2026-06-17 11:41 UTC (permalink / raw)
To: Jad Keskes; +Cc: robh, conor+dt, devicetree
In-Reply-To: <20260617112642.1897096-2-inasj268@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Use-After-Free / Deadlock in the device removal path due to incorrect devres ordering.
--
commit 4300c70f186763a1c545b060ff78e036f3434d38
Author: Jad Keskes <inasj268@gmail.com>
hw_random: timeriomem-rng: add configurable read width and data mask
This commit implements configurable read widths (8, 16, or 32 bits) and data
masking for the timeriomem-rng driver. It updates the read loop to dispatch
based on width to avoid bus errors, and uses properties from platform data or
device tree.
> diff --git a/drivers/char/hw_random/timeriomem-rng.c b/drivers/char/hw_random/timeriomem-rng.c
> index e61f063932090..4557326618c90 100644
> --- a/drivers/char/hw_random/timeriomem-rng.c
> +++ b/drivers/char/hw_random/timeriomem-rng.c
[ ... ]
> @@ -167,8 +219,8 @@ static int timeriomem_rng_probe(struct platform_device *pdev)
> return err;
> }
>
> - dev_info(&pdev->dev, "32bits from 0x%p @ %dus\n",
> - priv->io_base, period);
> + dev_info(&pdev->dev, "%ubit from %p @ %dus\n",
> + priv->width, priv->io_base, period);
>
> return 0;
> }
[Severity: High]
This isn't a bug introduced by this patch, but looking at the probe function
above, it uses devm_hwrng_register() to register the RNG device. This means
that hwrng_unregister() is automatically called by devres after
timeriomem_rng_remove() returns.
In timeriomem_rng_remove(), the driver manually calls
hrtimer_cancel(&priv->timer). At this point, the RNG is still registered and
fully exposed to concurrent readers. This ordering creates two distinct issues:
1. Deadlock: If a concurrent reader is inside timeriomem_rng_read() and
blocking on wait_for_completion(&priv->completion), it will wait forever
because the timer was just cancelled. When devres subsequently calls
hwrng_unregister(), it waits indefinitely for the reader's reference count to
drop, deadlocking the unbind process.
2. Use-After-Free: If a concurrent reader finishes its read loop just after
remove() cancelled the timer, it unconditionally executes
hrtimer_restart(&priv->timer) in the read path:
timeriomem_rng_read() {
...
priv->present = 0;
reinit_completion(&priv->completion);
hrtimer_forward_now(&priv->timer, priv->period);
hrtimer_restart(&priv->timer);
...
}
The reader then completes, remove() finishes, and devres frees the priv
structure. The re-armed timer will then fire and dereference the freed priv
pointer.
Could this devres ordering issue be addressed by manually unregistering the
hwrng before cancelling the timer in remove(), or by using a custom devm
action to ensure correct teardown order?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617112642.1897096-1-inasj268@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: rng: timeriomem_rng: add width and mask properties
From: sashiko-bot @ 2026-06-17 11:37 UTC (permalink / raw)
To: Jad Keskes; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260617112642.1897096-1-inasj268@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Use of non-standard `width` property instead of `reg-io-width`.
--
commit 7dac0db93dcf3017652753bebd5c0a2a7a2a06b5
Author: Jad Keskes <inasj268@gmail.com>
dt-bindings: rng: timeriomem_rng: add width and mask properties
Add optional width (8, 16, 32) and mask properties to the binding. The width
selects the bus access size for reads, while the mask allows filtering out
non-entropy-bearing bits.
> diff --git a/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml b/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml
> index 4754174e9849b..636305f211c88 100644
> --- a/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml
> +++ b/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml
[ ... ]
> @@ -26,16 +40,26 @@ properties:
> instead. Note that the default quality is usually zero which disables
> using this rng to automatically fill the kernel's entropy pool.
>
> - reg:
> - maxItems: 1
> + width:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + default: 32
> + enum: [8, 16, 32]
> description:
> - Base address to sample from. Currently 'reg' must be at least four bytes
> - wide and 32-bit aligned.
> + Access width in bits. Determines whether the read is performed as
> + an 8-bit, 16-bit, or 32-bit bus access.
[Severity: Low]
Should this binding use the standard reg-io-width property (representing the
width in bytes) instead of introducing a custom width property in bits?
Using the established reg-io-width standard prevents fragmentation of the
device tree API conventions for specifying memory-mapped I/O access width.
> +
> + mask:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + default: 0xFFFFFFFF
> + description:
> + Mask applied to the value read from the register. Bits set to 0 in
> + the mask are cleared in the output data. Default (no mask) passes
> + all bits through.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617112642.1897096-1-inasj268@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH 1/9] dt-bindings: nvmem: imx-ocotp: Add support for secure-enclave
From: Frieder Schrempf @ 2026-06-17 11:36 UTC (permalink / raw)
To: Krzysztof Kozlowski, Frieder Schrempf
Cc: Srinivas Kandagatla, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Shawn Guo, devicetree, imx, linux-arm-kernel,
linux-kernel
In-Reply-To: <20260617-prodigious-private-inchworm-beae1e@quoll>
On 17.06.26 12:49, Krzysztof Kozlowski wrote:
> On Tue, Jun 16, 2026 at 01:52:16PM +0200, Frieder Schrempf wrote:
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> Some SoCs like the i.MX9 family allow full access to the fuses only
>> through the secure enclave firmware API. Add a property to reference
>> the secure enclave node and let the driver use the API.
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> ---
>> Documentation/devicetree/bindings/nvmem/imx-ocotp.yaml | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/nvmem/imx-ocotp.yaml b/Documentation/devicetree/bindings/nvmem/imx-ocotp.yaml
>> index a8076d0e2737..14a6429f4a4c 100644
>> --- a/Documentation/devicetree/bindings/nvmem/imx-ocotp.yaml
>> +++ b/Documentation/devicetree/bindings/nvmem/imx-ocotp.yaml
>> @@ -53,6 +53,10 @@ properties:
>> reg:
>> maxItems: 1
>>
>> + secure-enclave:
>> + $ref: /schemas/types.yaml#/definitions/phandle
>> + description: A phandle to the secure enclave node
>
> Two things here:
> 1. Here you describe what for is that phandle, how it is used by the
> hardware. Currently the description repeats the property name and type,
> so not much useful.
Ok, agree.
>
> 2. If you access OTP via firmware, then this is completely different
> interface than MMIO, thus:
> A. reg is not appropriate
> B. Device is very different thus it has different compatible and I even
> claim should be in different binding. Devices having completely
> different SW interface should not be in the same binding, at least
> usually.
>
> If any of above is not accurate, then your commit msg should answer why
> and give some background.
Thanks for the feedback!
The driver currently uses the limited MMIO (FSB) interface to access the
OTPs. The intention is to support the firmware interface alongside the
MMIO interface so the driver can pick the interface that is available
(firmware might not be loaded) and fallback to MMIO.
Following your argument would mean a driver deciding by itself which
interface to use at runtime is not something we want to have in general,
right?
In turn this would mean we need two drivers, or at least two
compatibles/bindings for something that is effectively the same hardware.
Actually, my first RFC approach [1] was to create a separate driver. But
in the end it seemed very weird to have two drivers and two DT nodes for
the same hardware block. Also I have no idea what happens if both
interfaces are used at the same time.
The other idea from back then was to replace the MMIO (FSB) interface
with ELE, but this would mean that we rely on the proprietary ELE
firmware to be available for simple things like reading a MAC address,
which is not desirable either, I guess.
In which direction should I move on with this?
[1]
https://patchwork.kernel.org/project/linux-arm-kernel/patch/20250416142715.1042363-1-frieder@fris.de/
^ permalink raw reply
* Re: [PATCH v2 3/4] clk: qcom: Add EVA clock controller driver for Glymur SoC
From: Konrad Dybcio @ 2026-06-17 11:34 UTC (permalink / raw)
To: Taniya Das, Bjorn Andersson, Michael Turquette, Stephen Boyd,
Brian Masney, Dmitry Baryshkov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Konrad Dybcio
Cc: Ajit Pandey, Imran Shaik, Jagadeesh Kona, linux-arm-msm,
linux-clk, linux-kernel, devicetree
In-Reply-To: <20260617-evacc_glymur-v2-3-905108dacaaa@oss.qualcomm.com>
On 6/17/26 1:07 PM, Taniya Das wrote:
> Add the Enhanced Video Analytics (EVA) clock controller driver for
> the Glymur SoC. The EVACC manages the PLL, RCGs, branch clocks, GDSCs
> and resets for the EVA subsystem which handles vision processing
> workloads.
>
> Signed-off-by: Taniya Das <taniya.das@oss.qualcomm.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply
* [PATCH v3 2/2] hw_random: timeriomem-rng: add configurable read width and data mask
From: Jad Keskes @ 2026-06-17 11:26 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Olivia Lu, Herbert Xu, Rob Herring, Alexander Clouter,
linux-crypto, devicetree, linux-kernel, Jad Keskes
In-Reply-To: <20260617112642.1897096-1-inasj268@gmail.com>
The TODO for supporting read sizes other than 32 bits and masking has
been sitting in this driver since 2009. Implement it.
Add width (8, 16, or 32 bits) and mask properties to the platform data
and device tree bindings. The read loop dispatches on width using
readb/readw/readl so a configured 8-bit access doesn't trigger a bus
error on hardware that rejects 32-bit reads to that address. The mask
is ANDed with the value before storing.
These are platform properties, not runtime policy -- width depends on
SoC integration, mask reflects which output bits carry entropy.
The alignment check in probe is updated to verify the resource is
aligned to the configured width instead of hardcoding 4-byte alignment.
Signed-off-by: Jad Keskes <inasj268@gmail.com>
---
drivers/char/hw_random/timeriomem-rng.c | 78 ++++++++++++++++++++-----
include/linux/timeriomem-rng.h | 12 ++++
2 files changed, 77 insertions(+), 13 deletions(-)
diff --git a/drivers/char/hw_random/timeriomem-rng.c b/drivers/char/hw_random/timeriomem-rng.c
index e61f06393209..4557326618c9 100644
--- a/drivers/char/hw_random/timeriomem-rng.c
+++ b/drivers/char/hw_random/timeriomem-rng.c
@@ -14,7 +14,9 @@
* has to do is provide the address and 'wait time' that new data becomes
* available.
*
- * TODO: add support for reading sizes other than 32bits and masking
+ * The read width (8, 16, or 32 bits) and an optional data mask can be
+ * configured through platform data or device tree properties. Default is
+ * 32-bit reads with no mask.
*/
#include <linux/completion.h>
@@ -34,6 +36,8 @@ struct timeriomem_rng_private {
void __iomem *io_base;
ktime_t period;
unsigned int present:1;
+ unsigned int width;
+ u32 mask;
struct hrtimer timer;
struct completion completion;
@@ -48,6 +52,7 @@ static int timeriomem_rng_read(struct hwrng *hwrng, void *data,
container_of(hwrng, struct timeriomem_rng_private, rng_ops);
int retval = 0;
int period_us = ktime_to_us(priv->period);
+ int chunk = priv->width / 8;
/*
* There may not have been enough time for new data to be generated
@@ -71,11 +76,28 @@ static int timeriomem_rng_read(struct hwrng *hwrng, void *data,
usleep_range(period_us,
period_us + max(1, period_us / 100));
- *(u32 *)data = readl(priv->io_base);
- retval += sizeof(u32);
- data += sizeof(u32);
- max -= sizeof(u32);
- } while (wait && max > sizeof(u32));
+ switch (priv->width) {
+ case 8: {
+ u8 val = readb(priv->io_base) & priv->mask;
+ *(u8 *)data = val;
+ break;
+ }
+ case 16: {
+ u16 val = readw(priv->io_base) & priv->mask;
+ *(u16 *)data = val;
+ break;
+ }
+ case 32: {
+ u32 val = readl(priv->io_base) & priv->mask;
+ *(u32 *)data = val;
+ break;
+ }
+ }
+
+ retval += chunk;
+ data += chunk;
+ max -= chunk;
+ } while (wait && max > chunk);
/*
* Block any new callers until the RNG has had time to generate new
@@ -125,11 +147,8 @@ static int timeriomem_rng_probe(struct platform_device *pdev)
if (IS_ERR(priv->io_base))
return PTR_ERR(priv->io_base);
- if (res->start % 4 != 0 || resource_size(res) < 4) {
- dev_err(&pdev->dev,
- "address must be at least four bytes wide and 32-bit aligned\n");
- return -EINVAL;
- }
+ priv->width = 32;
+ priv->mask = 0xFFFFFFFF;
if (pdev->dev.of_node) {
int i;
@@ -145,9 +164,42 @@ static int timeriomem_rng_probe(struct platform_device *pdev)
if (!of_property_read_u32(pdev->dev.of_node,
"quality", &i))
priv->rng_ops.quality = i;
+
+ of_property_read_u32(pdev->dev.of_node,
+ "width", &priv->width);
+ of_property_read_u32(pdev->dev.of_node,
+ "mask", &priv->mask);
} else {
period = pdata->period;
priv->rng_ops.quality = pdata->quality;
+
+ if (pdata->width_set)
+ priv->width = pdata->width;
+ if (pdata->mask_set)
+ priv->mask = pdata->mask;
+ }
+
+ if (priv->width == 0)
+ priv->width = 32;
+
+ switch (priv->width) {
+ case 8:
+ case 16:
+ case 32:
+ break;
+ default:
+ dev_err(&pdev->dev, "invalid width %u, must be 8, 16, or 32\n",
+ priv->width);
+ return -EINVAL;
+ }
+
+ if (!IS_ALIGNED(res->start, priv->width / 8) ||
+ resource_size(res) < priv->width / 8) {
+ dev_err(&pdev->dev,
+ "address must be at least %u-bit aligned (%u byte%s)\n",
+ priv->width, priv->width / 8,
+ priv->width / 8 > 1 ? "s" : "");
+ return -EINVAL;
}
priv->period = us_to_ktime(period);
@@ -167,8 +219,8 @@ static int timeriomem_rng_probe(struct platform_device *pdev)
return err;
}
- dev_info(&pdev->dev, "32bits from 0x%p @ %dus\n",
- priv->io_base, period);
+ dev_info(&pdev->dev, "%ubit from %p @ %dus\n",
+ priv->width, priv->io_base, period);
return 0;
}
diff --git a/include/linux/timeriomem-rng.h b/include/linux/timeriomem-rng.h
index 672df7fbf6c1..b4202ad2f507 100644
--- a/include/linux/timeriomem-rng.h
+++ b/include/linux/timeriomem-rng.h
@@ -16,6 +16,18 @@ struct timeriomem_rng_data {
/* bits of entropy per 1024 bits read */
unsigned int quality;
+
+ /* read width (8, 16, or 32), 0 means 32 */
+ unsigned int width;
+
+ /* set to true if width is explicitly provided */
+ bool width_set;
+
+ /* mask applied to raw read value */
+ u32 mask;
+
+ /* set to true if mask is explicitly provided */
+ bool mask_set;
};
#endif /* _LINUX_TIMERIOMEM_RNG_H */
--
2.54.0
^ permalink raw reply related
* [PATCH v3 1/2] dt-bindings: rng: timeriomem_rng: add width and mask properties
From: Jad Keskes @ 2026-06-17 11:26 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Olivia Lu, Herbert Xu, Rob Herring, Alexander Clouter,
linux-crypto, devicetree, linux-kernel, Jad Keskes
Add optional width (8, 16, 32) and mask properties to the binding.
The width selects the bus access size for reads. The mask is ANDed
with the raw register value to allow only the entropy-bearing bits
through.
Update the example to show a typical 8-bit configuration.
Update SPDX to dual license to match kernel convention.
Drop the misleading '32-bit aligned' constraint from the reg
description since alignment now depends on the configured width.
Signed-off-by: Jad Keskes <inasj268@gmail.com>
---
.../bindings/rng/timeriomem_rng.yaml | 48 +++++++++++++++----
1 file changed, 40 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml b/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml
index 4754174e9849..636305f211c8 100644
--- a/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml
+++ b/Documentation/devicetree/bindings/rng/timeriomem_rng.yaml
@@ -1,10 +1,16 @@
-# SPDX-License-Identifier: GPL-2.0-only
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
%YAML 1.2
---
$id: http://devicetree.org/schemas/rng/timeriomem_rng.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#
-title: TimerIO Random Number Generator
+title: Timer IOMEM Hardware Random Number Generator
+
+description: |
+ This binding covers platforms that have a single IO memory address which
+ provides periodic random data. The driver reads from the address at a
+ fixed interval, returning a configurable-width value masked to the desired
+ bits.
maintainers:
- Krzysztof Kozlowski <krzk@kernel.org>
@@ -13,9 +19,17 @@ properties:
compatible:
const: timeriomem_rng
+ reg:
+ maxItems: 1
+ description:
+ Base address to sample from. Must be aligned to the configured access
+ width (1, 2, or 4 bytes) and at least that wide.
+
period:
$ref: /schemas/types.yaml#/definitions/uint32
- description: wait time in microseconds to use between samples
+ description:
+ Interval in microseconds between reads. New random data is expected to
+ be available at this rate.
quality:
$ref: /schemas/types.yaml#/definitions/uint32
@@ -26,16 +40,26 @@ properties:
instead. Note that the default quality is usually zero which disables
using this rng to automatically fill the kernel's entropy pool.
- reg:
- maxItems: 1
+ width:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ default: 32
+ enum: [8, 16, 32]
description:
- Base address to sample from. Currently 'reg' must be at least four bytes
- wide and 32-bit aligned.
+ Access width in bits. Determines whether the read is performed as
+ an 8-bit, 16-bit, or 32-bit bus access.
+
+ mask:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ default: 0xFFFFFFFF
+ description:
+ Mask applied to the value read from the register. Bits set to 0 in
+ the mask are cleared in the output data. Default (no mask) passes
+ all bits through.
required:
- compatible
- - period
- reg
+ - period
additionalProperties: false
@@ -46,3 +70,11 @@ examples:
reg = <0x44 0x04>;
period = <1000000>;
};
+
+ rng@64 {
+ compatible = "timeriomem_rng";
+ reg = <0x64 0x01>;
+ period = <50000>;
+ width = <8>;
+ mask = <0xFF>;
+ };
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v12 3/7] mfd: p3h2x4x: Add driver for NXP P3H2x4x i3c hub and on-die regulator
From: sashiko-bot @ 2026-06-17 11:24 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: Frank.Li, linux-i3c, conor+dt, devicetree, robh
In-Reply-To: <20260617110355.1591844-4-lakshay.piplani@nxp.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The I3C driver incorrectly binds to any generic I3C Hub instead of specifically to NXP P3H2x4x devices.
--
commit e3f8613aba897f64df36ad159bade0cf0e0c5cf1
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
mfd: p3h2x4x: Add driver for NXP P3H2x4x i3c hub and on-die regulator
This commit adds core MFD support for the NXP P3H2x4x family of multiport
I3C hub devices. It registers the devices via I3C/I2C/SMBus and exposes
multiple downstream target ports and an on-die regulator.
> diff --git a/drivers/mfd/p3h2840.c b/drivers/mfd/p3h2840.c
> new file mode 100644
> index 0000000000000..0a80992d6250a
> --- /dev/null
> +++ b/drivers/mfd/p3h2840.c
[ ... ]
> +static const struct i3c_device_id p3h2x4x_i3c_ids[] = {
> + I3C_CLASS(I3C_DCR_HUB, NULL),
[Severity: High]
Does this match table inappropriately bind the driver to any generic I3C Hub
instead of specifically NXP P3H2x4x devices?
Since p3h2x4x_device_probe_i3c() does not perform manufacturer or part ID
validation, could this claim non-NXP hubs and cause child drivers to send
proprietary NXP register sequences to them?
Should this use the I3C_DEVICE() macro with MIPI_VENDOR_ID_NXP instead?
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(i3c, p3h2x4x_i3c_ids);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617110355.1591844-1-lakshay.piplani@nxp.com?part=3
^ permalink raw reply
* Re: [PATCH v12 7/7] i3c: hub: p3h2x4x: Add SMBus slave mode support
From: sashiko-bot @ 2026-06-17 11:24 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: linux-i3c, Frank.Li, robh, conor+dt, devicetree
In-Reply-To: <20260617110355.1591844-8-lakshay.piplani@nxp.com>
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Unconditional IBI request causes probe failure if the upstream I3C controller lacks IBI support.
- [High] Over-clearing buffer status flags on error paths discards pending data in the receive buffers.
- [High] DMA-from-stack vulnerabilities by allocating buffers on the stack and passing them to regmap bulk operations.
- [Medium] SMBus slave registration configures IBI without verifying if the upstream connection is I3C.
- [Medium] Missing SMBus agent status clear for unregistered ports leads to IBI storms.
- [Medium] Lockless read of `is_registered` in the IBI handler races with lockless write during SMBus adapter teardown.
--
commit 4badb83d7f4fe074334808f3d555ec26e7923604
Author: Lakshay Piplani <lakshay.piplani@nxp.com>
i3c: hub: p3h2x4x: Add SMBus slave mode support
This commit adds SMBus slave mode support for the P3H2x4x hub target ports.
The hub SMBus slave agent can receive downstream payloads into target buffers
and report receive events through IBI, which are then forwarded to the
registered I2C slave client.
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> --- a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
[ ... ]
> @@ -117,5 +135,25 @@ int p3h2x4x_tp_i3c_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_hub)
> p3h2x4x_hub->tp_bus[tp].is_registered = true;
> p3h2x4x_hub->hub_config.tp_config[tp].always_enable = true;
> }
> - return regmap_write(p3h2x4x_hub->regmap, P3H2X4X_TP_NET_CON_CONF, ntwk_mask);
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> + ret = i3c_device_request_ibi(p3h2x4x_hub->i3cdev, &p3h2x4x_ibireq);
> + if (ret)
> + return ret;
[Severity: High]
Could this unconditional IBI request cause the probe to fail entirely if the
upstream I3C master controller does not support In-Band Interrupts?
If i3c_device_request_ibi() returns an error like -ENOTSUPP, it looks like
it aborts the entire initialization sequence for the hub.
Does this mean the core I3C/I2C target port routing functionality would break
on systems without IBI support merely because an optional SMBus slave feature
could not be enabled?
[ ... ]
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> --- a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
[ ... ]
> @@ -15,6 +15,146 @@ enum p3h2x4x_smbus_desc_idx {
> P3H2X4X_DESC_READ_LEN,
> };
>
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static void p3h2x4x_read_smbus_agent_rx_buf(struct i3c_device *i3cdev, enum p3h2x4x_rcv_buf rfbuf,
> + enum p3h2x4x_tp tp, bool is_of)
> +{
> + struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = i3cdev_get_drvdata(i3cdev);
> + u8 slave_rx_buffer[P3H2X4X_SMBUS_TARGET_PAYLOAD_SIZE] = { 0 };
> + u8 target_buffer_page, flag_clear = 0x0f, temp = 0, i, addr;
[Severity: High]
This isn't a bug strictly introduced by this patch, as a similar preexisting
flaw exists in p3h2x4x_tp_i2c_xfer_msg() with the desc array, but allocating
slave_rx_buffer on the stack and passing it to regmap bulk operations can
cause DMA-from-stack vulnerabilities.
Because the underlying regmap is backed by an I3C controller that may use DMA
for transfers, passing stack addresses is unsafe.
If CONFIG_VMAP_STACK is enabled, the stack memory is not guaranteed to be
physically contiguous, which could cause dma_map_single() to fail or corrupt
memory.
Should these buffers be dynamically allocated or pre-allocated in the device
structure instead?
> + u32 packet_len, slave_address, ret;
[ ... ]
> +ibi_err:
> + regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_PAGE_PTR, 0x00);
> + regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_TP0_SMBUS_AGNT_STS + tp, flag_clear);
> +}
[Severity: High]
Could this over-clear the buffer status flags on error paths and discard
pending data?
The local variable flag_clear is initialized to 0x0f, which matches
BUF_RECEIVED_FLAG_TF_MASK.
If an early error occurs before flag_clear is constrained to the specific port
buffer flag, it will write 0x0f to the status register.
Will this unconditionally clear all receive buffer flags, including buffer 0,
buffer 1, and overflow flags, for that port? If the other buffer contained a
valid pending message, would it be silently dropped?
> +
> +/**
> + * p3h2x4x_ibi_handler - IBI handler.
> + * @i3cdev: i3c device.
> + * @payload: two byte IBI payload data.
> + *
> + */
> +void p3h2x4x_ibi_handler(struct i3c_device *i3cdev,
> + const struct i3c_ibi_payload *payload)
> +{
[ ... ]
> + guard(mutex)(&p3h2x4x_i3c_hub->etx_mutex);
> +
> + for (i = 0; i < P3H2X4X_TP_MAX_COUNT; ++i) {
> + if (p3h2x4x_i3c_hub->tp_bus[i].is_registered && (payload_byte_two >> i) & 0x01) {
[Severity: Medium]
What happens if an IBI is triggered by a port that is not registered?
Since the loop skips unregistered ports entirely, the hardware interrupt source
at P3H2X4X_TP0_SMBUS_AGNT_STS is never read or cleared.
Does this mean the underlying hardware condition remains active, causing the
hub to immediately re-assert the IBI after the handler returns?
Could this result in an infinite loop of IBIs locking up the I3C bus, and
should the event status be cleared defensively even for unregistered ports?
[Severity: Medium]
Is there a data race here involving the read of is_registered?
During driver unbind or probe failure, p3h2x4x_unregister_smbus_adapters()
executes hub->tp_bus[tp].is_registered = false; without holding etx_mutex.
Because the devm action to free the IBI executes after the SMBus teardown
action, this new handler could be running concurrently and reading
is_registered under etx_mutex.
Since the writer does not hold the mutex, should this state modification be
properly synchronized?
> + ret = regmap_read(p3h2x4x_i3c_hub->regmap, P3H2X4X_TP0_SMBUS_AGNT_STS + i,
[ ... ]
> @@ -205,11 +345,64 @@ static u32 p3h2x4x_tp_smbus_funcs(struct i2c_adapter *adapter)
> return I2C_FUNC_I2C | I2C_FUNC_SMBUS_BLOCK_DATA;
> }
>
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static int p3h2x4x_tp_i2c_reg_slave(struct i2c_client *slave)
> +{
> + struct tp_bus *bus = i2c_get_adapdata(slave->adapter);
> + struct p3h2x4x_i3c_hub_dev *hub = bus->p3h2x4x_i3c_hub;
> + int ret;
> +
> + guard(mutex)(&hub->etx_mutex);
> +
> + if (bus->tp_smbus_client)
> + return -EBUSY;
> +
> + ret = regmap_set_bits(hub->regmap,
> + P3H2X4X_TP_SMBUS_AGNT_IBI_CONFIG,
> + bus->tp_mask);
[Severity: Medium]
Does this configure IBI without verifying if the upstream connection is I3C?
The registration enables P3H2X4X_TP_SMBUS_AGNT_IBI_CONFIG to route receive
events via IBI. However, it does not check if the hub is connected via an I3C
bus (i.e., whether hub->i3cdev is valid and an IBI handler was successfully
requested).
If the hub is connected via an I2C upstream bus, it cannot generate IBIs.
Enabling IBI generation on an I2C-connected hub would cause slave receive
events to be silently dropped since no handler is registered.
Should this function return -EOPNOTSUPP if !hub->i3cdev?
> + if (ret)
> + return ret;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617110355.1591844-1-lakshay.piplani@nxp.com?part=7
^ permalink raw reply
* Re: [PATCH v9 0/6] Add driver for EC found on Qualcomm reference devices
From: Anthony Ruhier @ 2026-06-17 11:23 UTC (permalink / raw)
To: Anvesh Jain P
Cc: Sibi Sankar, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Hans de Goede, Ilpo Järvinen, Bryan O'Donoghue,
Bjorn Andersson, Konrad Dybcio, Randy Dunlap, linux-arm-msm,
devicetree, linux-kernel, platform-driver-x86, Maya Matuszczyk,
Krzysztof Kozlowski, Dmitry Baryshkov, Konrad Dybcio,
Akhil P Oommen, Abel Vesa, Gaurav Kohli, Pankaj Patil
In-Reply-To: <20260511-add-driver-for-ec-v9-0-e5437c39b7f8@oss.qualcomm.com>
Tested-by: Anthony Ruhier <aruhier@mailbox.org>
Thanks, I tested it on the slim 7x (2024) on which I was previously using the
first patches from Maya, it works great!
I confirm that the following works: keyboard backlight shutdowns when
suspended, led blinks when suspended, keyboard fn shortcuts.
For the Slim 7x, I needed to add the following in the DTS:
---
diff --git a/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts b/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts
index b5247fb91299..423bf7340b83 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts
+++ b/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts
@@ -1070,6 +1070,21 @@ retimer_ss0_con_sbu_out: endpoint {
};
};
+&i2c5 {
+ clock-frequency = <400000>;
+ status = "okay";
+
+ embedded-controller@76 {
+ compatible = "qcom,hamoa-crd-ec";
+ reg = <0x76>;
+
+ interrupts-extended = <&tlmm 66 IRQ_TYPE_EDGE_FALLING>;
+
+ pinctrl-0 = <&ec_int_n_default>;
+ pinctrl-names = "default";
+ };
+};
+
&i2c7 {
clock-frequency = <400000>;
@@ -1487,6 +1502,12 @@ reset-n-pins {
};
};
+ ec_int_n_default: ec-int-n-state {
+ pins = "gpio66";
+ function = "gpio";
+ bias-disable;
+ };
+
edp_reg_en: edp-reg-en-state {
pins = "gpio70";
function = "gpio";
--
Anthony Ruhier
^ permalink raw reply related
* Re: [PATCH v12 1/7] i3c: master: Add APIs for I3C hub support
From: sashiko-bot @ 2026-06-17 11:20 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: Frank.Li, linux-i3c, conor+dt, devicetree, robh
In-Reply-To: <20260617110355.1591844-2-lakshay.piplani@nxp.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Bypassing core state tracking in `direct_attach` / `direct_detach` causes Use-After-Free, NULL pointer dereferences, and Memory Leaks.
- [High] Bypassing address slot reservation in `direct_attach` leads to unrecoverable I3C bus address collisions.
--
commit ce08f4a6bc0d82c966d4e3337e6b4d082a9d09d2
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
i3c: master: Add APIs for I3C hub support
This commit introduces helper functions for attaching and detaching I3C
devices, alongside CCC command helpers. These additions prepare the core
framework for handling I3C hubs.
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1775,6 +1775,70 @@ static int i3c_master_attach_i3c_dev(struct i3c_master_controller *master,
[ ... ]
> +int i3c_master_direct_attach_i3c_dev_locked(struct i3c_master_controller *master,
> + struct i3c_dev_desc *dev)
> +{
> + int ret = 0;
> +
> + /*
> + * We don't attach devices to the controller until they are
> + * addressable on the bus.
> + */
> +
> + if (!dev->info.static_addr && !dev->info.dyn_addr)
> + return 0;
> +
> + /* Do not attach the master device itself. */
> + if (master->this != dev && master->ops->attach_i3c_dev)
> + ret = master->ops->attach_i3c_dev(dev);
[Severity: High]
Does bypassing address slot reservation here lead to I3C bus collisions?
In the standard attachment flow, i3c_master_get_i3c_addrs() is called to
reserve the address slot in the controller's bus.addrslots bitmap. Because
this direct helper skips that reservation, the parent controller's address
map is never updated to reflect the newly occupied address.
Since the parent controller and the virtual hub share the physical I3C bus,
the parent might still believe the downstream device's address is free. If
a new device hot-joins, could the parent controller allocate the exact same
address, resulting in an unrecoverable address collision?
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_direct_attach_i3c_dev_locked);
> +
> +/**
> + * i3c_master_direct_detach_i3c_dev_locked() - Detach an I3C device from its
> + * master controller.
[ ... ]
> +void i3c_master_direct_detach_i3c_dev_locked(struct i3c_dev_desc *dev)
> +{
> + struct i3c_master_controller *master = i3c_dev_get_master(dev);
> +
> + /*
> + * We don't attach devices to the controller until they are
> + * addressable on the bus, so skip detach for unaddressed devices.
> + */
> + if (!dev->info.static_addr && !dev->info.dyn_addr)
> + return;
> +
> + /* Do not detach the master device itself. */
> + if (master->this != dev && master->ops->detach_i3c_dev)
> + master->ops->detach_i3c_dev(dev);
[Severity: Critical]
Can this lead to a use-after-free or a NULL pointer dereference by bypassing
the core state tracking?
When i3c_master_direct_detach_i3c_dev_locked() invokes the controller's
detach callback, master drivers typically free the master_priv data. However,
the dev->common.master_priv pointer is never cleared here.
If i3c_hub_free_ibi() calls this detach helper, and later a transfer is sent
via i3c_hub_master_priv_xfers():
drivers/i3c/hub.c:i3c_hub_master_priv_xfers() {
...
xfer_ret = i3c_master_priv_xfers(dev, xfers, nxfers, mode);
...
}
Will the parent controller end up dereferencing the freed master_priv
pointer?
Additionally, if a device is never explicitly attached via the direct helper
(leaving master_priv NULL), would a standard transfer immediately cause a
NULL pointer dereference when forwarded to the parent driver?
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_direct_detach_i3c_dev_locked);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617110355.1591844-1-lakshay.piplani@nxp.com?part=1
^ permalink raw reply
* Re: [PATCH v3 2/7] gpio: regmap: add gpio_regmap_get_gpiochip() accessor
From: Michael Walle @ 2026-06-17 11:19 UTC (permalink / raw)
To: Yu-Chun Lin [林祐君], Bartosz Golaszewski,
Andy Shevchenko
Cc: linusw@kernel.org, robh@kernel.org, krzk+dt@kernel.org,
conor+dt@kernel.org, afaerber@suse.com, wbg@kernel.org,
mathieu.dubois-briand@bootlin.com, lars@metafoo.de,
Michael.Hennerich@analog.com, jic23@kernel.org,
nuno.sa@analog.com, andy@kernel.org, dlechner@baylibre.com,
TY_Chang[張子逸], linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-realtek-soc@lists.infradead.org, linux-iio@vger.kernel.org,
CY_Huang[黃鉦晏],
Stanley Chang[昌育德],
James Tai [戴志峰]
In-Reply-To: <61c053a5a8e6461f9e6fcd40b6b5064d@realtek.com>
[-- Attachment #1: Type: text/plain, Size: 2246 bytes --]
Hi,
On Wed Jun 17, 2026 at 11:54 AM CEST, Yu-Chun Lin [林祐君] wrote:
> Hi Michael,
>
>> Hi,
>>
>> On Wed Jun 17, 2026 at 10:36 AM CEST, Yu-Chun Lin [林祐君] wrote:
>>>>>>> Without an accessor like gpio_regmap_get_gpiochip(), we cannot
>>>>>>> retrieve the gpio_chip instantiated inside gpio-regmap.c to
>>>>>>> fulfill these requirements in our
>>>>>>> map() function.
>>>>
>>>> Why is gpiochip_irq_reqres() called in the first place? Isn't that
>>>> only called if the irq handling is set up via gc->irq.chip and not
>>>> via
>>>> gpiochip_irqchip_add_domain() like in gpio-regmap?
>>>>
>>>
>>> The panic was caused by my driver including
>>> 'GPIOCHIP_IRQ_RESOURCE_HELPERS', which forced the call to 'gpiochip_irq_reqres()' and crashed.
>>
>> But why did you use it if your irq domain isn't managed by the gpiolib, but rather your own >irq domain? Before going with option #3 I'd double check if that is correct in your driver.
>>
>> -michael
>
> Do you mean that a custom IRQ domain shouldn't be mixed with gpiolib features like
> 'GPIOCHIP_IRQ_RESOURCE_HELPERS'?
Honestly, I'm not sure. I've never done anything with irq domains
except for using the regmap_irq_chip. But from what I can tell is
that GPIOCHIP_IRQ_RESOURCE_HELPERS are tied to the handling with
gc->irq.chip, which isn't used at all if you add the domain via
gpiochip_irqchip_add_domain(). Please correct me if I'm wrong
though.
-michael
> Additional information: our GPIO controller receives 3 separate interrupt lines.
> Because the standard 'regmap_irq_chip' mechanism in 'gpio-regmap' does not support
> this multi-line hardware design, we are forced to create our own IRQ domain and pass
> it via 'config->irq_domain'.
>
> Given this constraint (that we must use our own IRQ domain), are you suggesting
> that we should implement our own 'irq_request_resources' and
> 'irq_release_resources' callbacks instead of relying on
> 'GPIOCHIP_IRQ_RESOURCE_HELPERS'?
>
> But if that is the case, we would much prefer to let the core gpiolib handle
> these resource and state management tasks for us *as proposed in option 3), rather
> than duplicating the effort in our driver.
>
> Best Regards,
> Yu-Chun
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]
^ permalink raw reply
* Re: [PATCH RFC v4 9/9] arm64: dts: qcom: glymur: Wire PCIe3a/3b to shared Gen5x8 PHY
From: Konrad Dybcio @ 2026-06-17 11:19 UTC (permalink / raw)
To: Qiang Yu, Vinod Koul, Neil Armstrong, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Philipp Zabel, Bjorn Andersson,
Konrad Dybcio
Cc: linux-arm-msm, linux-phy, devicetree, linux-kernel
In-Reply-To: <20260518-link_mode_0519-v4-9-269cd73cc5d1@oss.qualcomm.com>
On 5/19/26 7:47 AM, Qiang Yu wrote:
> Glymur PCIe3 uses a single shared Gen5x8 QMP PHY block. Model PCIe3a and
> PCIe3b as consumers of that shared PHY provider instead of separate PHY
> nodes.
>
> Update the DTS wiring to:
> - point GCC PCIe3A/3B pipe parents to the shared PHY clock outputs
> - add PCIe3a controller node and route PCIe3a/PCIe3b port phys to
> &pcie3_phy using two-cell PHY arguments
> - configure the shared PHY node with link-mode and dual pipe outputs
>
> Use QMP_PCIE_GLYMUR_MODE_* dt-binding macros for mode selection.
>
> Signed-off-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
> ---
[...]
> + pcie3a: pci@1c10000 {
> + device_type = "pci";
> + compatible = "qcom,glymur-pcie", "qcom,pcie-x1e80100";
> + reg = <0x0 0x01c10000 0x0 0x3000>,
> + <0x0 0x70000000 0x0 0xf20>,
> + <0x0 0x70000f40 0x0 0xa8>,
> + <0x0 0x70001000 0x0 0x4000>,
> + <0x0 0x70100000 0x0 0x100000>,
> + <0x0 0x01c13000 0x0 0x1000>;
> + reg-names = "parf",
> + "dbi",
> + "elbi",
> + "atu",
> + "config",
> + "mhi";
> + #address-cells = <3>;
> + #size-cells = <2>;
> + ranges = <0x01000000 0x0 0x00000000 0x0 0x70200000 0x0 0x100000>,
> + <0x02000000 0x0 0x70000000 0x0 0x70300000 0x0 0x3d00000>,
> + <0x03000000 0x7 0x00000000 0x7 0x00000000 0x0 0x40000000>,
> + <0x43000000 0x70 0x00000000 0x70 0x00000000 0x10 0x00000000>;
> +
> + bus-range = <0 0xff>;
> +
> + dma-coherent;
> +
> + linux,pci-domain = <3>;
> + num-lanes = <8>;
Is it fine to keep num-lanes 8 here even for configurations with
bifurcated PHY?
I would assume so, given essentially this is a x8 host, whose 4
lanes may simply be effectively NC
Konrad
^ permalink raw reply
* Re: [PATCH v12 6/7] i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality
From: sashiko-bot @ 2026-06-17 11:18 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: devicetree, robh, linux-i3c, Frank.Li, conor+dt
In-Reply-To: <20260617110355.1591844-7-lakshay.piplani@nxp.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The SMBus transaction wait logic uses a fixed sleep instead of polling the hardware completion flag, leading to premature reads of incomplete transactions and data corruption.
- [High] Overwriting the parent device's `of_node` pointer iteratively causes a memory leak of the original parent node, a double-free of the target port nodes, and state corruption.
- [Medium] Potential `of_node` reference leak in `p3h2x4x_get_target_port_dt_conf()` if multiple child nodes specify the same target port index.
- [Medium] The hub's protected registers remain permanently unlocked if the hardware configuration or bus registration fails during probe.
- [Low] Comment describes a read followed by a write, but the code implements a write followed by a read.
--
commit 1c982ecf71c6fb3b765caf255191a821a5e37cb5
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
i3c: hub: p3h2x4x: Add support for NXP P3H2x4x I3C hub functionality
Add I3C hub functionality for the NXP P3H2x4x family of multiport hubs.
These devices support downstream target ports that can be configured
as I3C, I2C, or SMBus.
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_common.c b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
> new file mode 100644
> index 0000000000000..05e19bff20d5c
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_common.c
[ ... ]
> +static void p3h2x4x_get_target_port_dt_conf(struct device *dev,
> + const struct device_node *node)
> +{
> + struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub = dev_get_drvdata(dev);
> + u64 tp_port;
> +
> + for_each_available_child_of_node_scoped(node, dev_node) {
> + if (of_property_read_reg(dev_node, 0, &tp_port, NULL))
> + continue;
> +
> + if (tp_port < P3H2X4X_TP_MAX_COUNT) {
> + p3h2x4x_i3c_hub->tp_bus[tp_port].of_node = of_node_get(dev_node);
[Severity: Medium]
Could this leak the of_node reference if a malformed device tree specifies
multiple child nodes with the same target port index?
If multiple child nodes specify the same reg value, the pointer is
overwritten with a new of_node_get() result, which abandons the previous
reference.
> + p3h2x4x_i3c_hub->tp_bus[tp_port].tp_mask = P3H2X4X_SET_BIT(tp_port);
> + p3h2x4x_i3c_hub->tp_bus[tp_port].p3h2x4x_i3c_hub = p3h2x4x_i3c_hub;
> + p3h2x4x_i3c_hub->tp_bus[tp_port].tp_port = tp_port;
> + }
> + }
> +}
[ ... ]
> +static int p3h2x4x_i3c_hub_probe(struct platform_device *pdev)
> +{
[ ... ]
> + /* Unlock access to protected registers */
> + ret = regmap_write(p3h2x4x_i3c_hub->regmap, P3H2X4X_DEV_REG_PROTECTION_CODE,
> + P3H2X4X_REGISTERS_UNLOCK_CODE);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to unlock HUB's protected registers\n");
> +
> + ret = p3h2x4x_configure_hw(dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to configure the HUB\n");
[Severity: Medium]
Are the hub's protected registers left permanently unlocked if probe fails
during p3h2x4x_configure_hw() or bus registration?
The driver unlocks the hardware registers at the start of probe and locks
them at the very end. However, if any initialization step fails and returns
an error, the function exits immediately without locking the registers again.
Could this be mitigated by using a devm_add_action_or_reset() callback or
explicit error handling labels to ensure the registers are safely relocked?
> +
> + /* Register virtual I3C master controllers for I3C target ports */
> + if (p3h2x4x->i3cdev) {
[ ... ]
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> new file mode 100644
> index 0000000000000..8adb235b992c0
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
[ ... ]
> +int p3h2x4x_tp_i3c_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_hub)
> +{
[ ... ]
> + for (tp = 0; tp < P3H2X4X_TP_MAX_COUNT; tp++) {
> + if (!p3h2x4x_hub->tp_bus[tp].of_node ||
> + p3h2x4x_hub->hub_config.tp_config[tp].mode != P3H2X4X_TP_MODE_I3C)
> + continue;
> +
> + /* Assign DT node for this TP */
> + p3h2x4x_hub->dev->of_node = p3h2x4x_hub->tp_bus[tp].of_node;
[Severity: High]
Does overwriting the parent device's of_node pointer iteratively cause a
memory leak of the original parent node and a double-free of the target port
nodes?
The loop over target ports directly mutates the shared pdev->dev.of_node
pointer which was populated in probe using device_set_of_node_from_dev().
This leaks the reference to the original parent node, and leaves
dev->of_node pointing to the last target port node.
When the driver is unbound, the driver core will drop a reference on
dev->of_node, but p3h2x4x_put_target_port_of_nodes() also drops a reference
on the same node, which can cause a use-after-free or double-free refcount
underflow. Mutating dev->of_node also creates a race with sysfs/uevent.
Should the driver assign the node to controller->dev.of_node directly
instead of mutating the parent's pointer?
> +
> + struct i3c_hub_controller *hub_controller =
> + &p3h2x4x_hub->tp_bus[tp].hub_controller;
[ ... ]
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> new file mode 100644
> index 0000000000000..cf664425af071
> --- /dev/null
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
[ ... ]
> +static int p3h2x4x_read_smbus_transaction_status(struct p3h2x4x_i3c_hub_dev *hub,
> + u8 target_port_status,
> + u8 data_length)
> +{
> + u32 status_read;
> + u8 status;
> + int ret;
> +
> + fsleep(P3H2X4X_SMBUS_400kHz_TRANSFER_TIMEOUT(data_length));
> +
> + ret = regmap_read(hub->regmap, target_port_status, &status_read);
[Severity: High]
Does using a hardcoded sleep based on bytes transferred instead of polling a
hardware completion status bit lead to premature reads of incomplete
transactions?
If the downstream device performs clock stretching or is slow to respond,
the transfer will take longer than the hardcoded timeout. The driver then
reads target_port_status and shifts to check the status bits.
Since an ongoing transaction leaves the status bits at 0
(P3H2X4X_SMBUS_CNTRL_STATUS_TXN_OK), the driver will incorrectly assume the
transaction has finished successfully. It will then proceed to read stale or
uninitialized memory buffers and return garbage data.
Could this be updated to use a polling loop checking the finish flag?
> + if (ret)
> + return ret;
> +
> + status = (u8)status_read;
[ ... ]
> +static s32 p3h2x4x_tp_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
[ ... ]
> + rw = (msgs[msg_count].flags & I2C_M_RD) ? 1 : 0;
> + if (!rw) {
> + /* If a read message is immediately followed by a write message to
> + * the same address, consider combining them into a single transaction.
> + */
[Severity: Low]
This isn't a bug, but the comment describes a read followed by a write while
the code implements a write followed by a read.
The actual code block checks !rw for the first message and flags & I2C_M_RD
for the second message, which accurately handles a write message immediately
followed by a read message.
> + if (msg_count + 1 < num &&
> + msgs[msg_count].addr == msgs[msg_count + 1].addr &&
> + (msgs[msg_count + 1].flags & I2C_M_RD)) {
> + if (msgs[msg_count].len + msgs[msg_count + 1].len >
> + P3H2X4X_SMBUS_PAYLOAD_SIZE)
> + return -EINVAL;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617110355.1591844-1-lakshay.piplani@nxp.com?part=6
^ permalink raw reply
* Re: [PATCH v12 5/7] i3c: hub: Add support for the I3C interface in the I3C hub
From: sashiko-bot @ 2026-06-17 11:18 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: robh, conor+dt, Frank.Li, devicetree, linux-i3c
In-Reply-To: <20260617110355.1591844-6-lakshay.piplani@nxp.com>
Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] Deterministic NULL pointer dereference in `i3c_hub_free_ibi()` due to recursive callback chain.
- [Critical] Memory leak and double-free memory corruption in `i3c_hub_request_ibi()` error paths.
- [Critical] Critical race condition and parent bus state corruption in `i3c_hub_master_priv_xfers()` due to unlocked address mutation.
- [Critical] Unimplemented `i3c_hub_recycle_ibi_slot()` permanently leaks hardware IBI slots.
- [Critical] Architectural bypass in `i3c_hub_do_daa()` attaches dynamically discovered devices to the parent bus instead of the virtual hub bus.
- [High] Calling `i3c_hub_enable_port()` outside of any locks allows concurrent operations to physically enable multiple downstream ports simultaneously, breaking electrical isolation.
- [High] Silently dropping broadcast RSTDAA commands prevents resetting of downstream devices.
- [Medium] Accepting I2C devices while unconditionally rejecting all I2C transfers violates the API contract.
--
commit 2a201a9e0e9a55b31467d874340085240b34afc3
Author: Lakshay Piplani <lakshay.piplani@nxp.com>
i3c: hub: Add support for the I3C interface in the I3C hub
This patch adds virtual I3C bus support for the hub and provides an
interface to enable or disable downstream ports.
> diff --git a/drivers/i3c/hub.c b/drivers/i3c/hub.c
> new file mode 100644
> index 0000000000000..e8744b5172340
> --- /dev/null
> +++ b/drivers/i3c/hub.c
> @@ -0,0 +1,483 @@
[ ... ]
> +static int i3c_hub_do_daa(struct i3c_hub *hub,
> + struct i3c_master_controller *controller)
> +{
> + struct i3c_master_controller *parent;
> + int ret;
> +
> + if (!hub || !hub->hub_dev)
> + return -ENODEV;
> +
> + parent = i3c_dev_get_master(hub->hub_dev->desc);
> + if (!parent)
> + return -ENODEV;
> +
> + i3c_hub_enable_port(controller);
> + ret = i3c_master_do_daa(parent);
[Severity: Critical]
Since `i3c_master_do_daa()` executes the DAA sequence natively on the parent
controller, will the newly discovered devices be allocated and attached
directly to the parent bus instead of this virtual hub's bus?
If they are attached to the parent bus, won't subsequent transfers completely
bypass `i3c_hub_master_priv_xfers()`, causing communication with these
dynamically enumerated devices to fail?
[Severity: High]
Is there a risk of breaking electrical isolation here? `i3c_hub_enable_port()`
is called before acquiring any parent bus locks.
If multiple threads operate on different virtual controllers concurrently,
could they physically enable multiple downstream ports at the same time
before the bus lock is acquired?
> + i3c_hub_disable_port(controller);
> +
> + return ret;
> +}
[ ... ]
> +static int i3c_hub_send_ccc_cmd(struct i3c_hub *hub,
> + struct i3c_master_controller *controller,
> + struct i3c_ccc_cmd *cmd)
> +{
> + struct i3c_master_controller *parent;
> + int ret;
> +
> + if (!hub || !hub->hub_dev)
> + return -ENODEV;
> +
> + parent = i3c_dev_get_master(hub->hub_dev->desc);
> + if (!parent)
> + return -ENODEV;
> +
> + i3c_hub_enable_port(controller);
> + ret = i3c_master_send_ccc_cmd(parent, cmd);
[Severity: High]
Similar to `i3c_hub_do_daa()`, does calling `i3c_hub_enable_port()` without a
lock risk enabling multiple ports concurrently if multiple CCC commands are
issued on different ports?
> + i3c_hub_disable_port(controller);
> +
> + return ret;
> +}
[ ... ]
> +static int i3c_hub_master_priv_xfers(struct i3c_dev_desc *dev,
> + struct i3c_xfer *xfers,
> + int nxfers,
> + enum i3c_xfer_mode mode)
> +{
[ ... ]
> + if (hub_addr != target_addr) {
> + down_write(&parent->bus.lock);
> + hub_dev->info.dyn_addr = target_addr;
> +
> + ret = i3c_master_reattach_i3c_dev_locked(hub_dev, hub_addr);
> + up_write(&parent->bus.lock);
> + if (ret) {
> + hub_dev->info.dyn_addr = hub_addr;
> + goto disable;
> + }
> + }
> +
> + ret = i3c_device_do_xfers(hub->hub_dev, xfers, nxfers, mode);
[Severity: Critical]
Does dropping `parent->bus.lock` before calling `i3c_device_do_xfers()` expose
a race condition?
If multiple downstream ports initiate transfers concurrently, could they
overwrite the shared `hub_dev->info.dyn_addr` before the transfer completes,
causing packets to be routed to the wrong physical device?
> +
> + if (hub_addr != target_addr) {
> + down_write(&parent->bus.lock);
> + hub_dev->info.dyn_addr = hub_addr;
> +
> + xfer_ret = i3c_master_reattach_i3c_dev_locked(hub_dev,
> + target_addr);
[Severity: Critical]
Could passing `target_addr` as the old address corrupt the parent bus state?
`i3c_master_reattach_i3c_dev_locked()` frees the old address slot in the bus
slot map. Will this erroneously mark `target_addr` as `I3C_ADDR_SLOT_FREE`
on the parent bus?
> +
> + up_write(&parent->bus.lock);
> + if (!ret)
> + ret = xfer_ret;
> + }
> +disable:
> + i3c_hub_disable_port(controller);
> + return ret;
> +}
> +
> +static int i3c_hub_attach_i2c_dev(struct i2c_dev_desc *dev)
> +{
> + return 0;
> +}
[ ... ]
> +static int i3c_hub_i2c_xfers(struct i2c_dev_desc *dev,
> + struct i2c_msg *xfers, int nxfers)
> +{
> + return -EOPNOTSUPP;
> +}
[Severity: Medium]
Is it intentional to return `0` from `i3c_hub_attach_i2c_dev()` while rejecting
all I2C transfers with `-EOPNOTSUPP`?
Would it be safer to return an error from the attach callback to cleanly
prevent I2C driver binding if the hub does not support I2C forwarding?
[ ... ]
> +static int i3c_hub_master_send_ccc_cmd(struct i3c_master_controller *controller,
> + struct i3c_ccc_cmd *cmd)
> +{
> + struct i3c_hub_controller *hub_controller;
> + struct i3c_hub *hub;
> +
> + hub_controller = dev_get_drvdata(&controller->dev);
> + if (!hub_controller || !hub_controller->hub)
> + return -ENODEV;
> +
> + hub = hub_controller->hub;
> +
> + if (!hub->hub_dev)
> + return -ENODEV;
> +
> + if (cmd->id == I3C_CCC_RSTDAA(true))
> + return 0;
[Severity: High]
Does silently dropping the broadcast RSTDAA command prevent downstream devices
from being correctly reset by the hub?
If the command is dropped instead of being forwarded to the downstream ports,
will devices retain their old dynamic addresses and fail to re-initialize
during hot-plug recovery?
> +
> + return i3c_hub_send_ccc_cmd(hub, controller, cmd);
> +}
[ ... ]
> +static int i3c_hub_request_ibi(struct i3c_dev_desc *desc,
> + const struct i3c_ibi_setup *req)
> +{
[ ... ]
> + ret = i3c_master_direct_attach_i3c_dev_locked(parent, desc);
> + if (ret) {
> + i3c_hub_update_desc_parent(&desc->common, orig_parent);
> + up_write(&parent->bus.lock);
> + return ret;
> + }
> +
> + kfree(desc->ibi);
[Severity: Critical]
Does freeing `desc->ibi` here leak `desc->ibi->wq`?
When the core function `i3c_dev_request_ibi_locked()` allocates `ibi`, it also
allocates `ibi->wq`.
Also, if the inner call to `i3c_dev_request_ibi_locked()` fails, this function
returns an error to the original core caller. Will the outer caller's error
path execute `kfree(ibi)` on its local variable, resulting in a double-free
since it was already freed here?
> + desc->ibi = NULL;
> + ret = i3c_dev_request_ibi_locked(desc, req);
> + if (ret)
> + i3c_master_direct_detach_i3c_dev_locked(desc);
> +
> + i3c_hub_update_desc_parent(&desc->common, orig_parent);
> +
> + up_write(&parent->bus.lock);
> +
> + return ret;
> +}
> +
> +static void i3c_hub_free_ibi(struct i3c_dev_desc *desc)
> +{
[ ... ]
> + i3c_hub_enable_port(controller);
> +
> + down_write(&parent->bus.lock);
> + orig_parent = i3c_hub_update_desc_parent(&desc->common, parent);
> + i3c_master_direct_detach_i3c_dev_locked(desc);
> + i3c_dev_free_ibi_locked(desc);
[Severity: Critical]
Can this recursive call to `i3c_dev_free_ibi_locked(desc)` cause a null
pointer dereference?
When the core initially calls `i3c_dev_free_ibi_locked(desc)`, it invokes
this hub callback. The inner call here successfully frees `desc->ibi` and sets
it to null.
When control returns to the outer core function, it unconditionally evaluates:
if (dev->ibi->wq) {
Will this immediately dereference the null pointer?
> + i3c_hub_update_desc_parent(&desc->common, orig_parent);
> + up_write(&parent->bus.lock);
> +
> + i3c_hub_disable_port(controller);
> +}
[ ... ]
> +static void i3c_hub_recycle_ibi_slot(struct i3c_dev_desc *desc,
> + struct i3c_ibi_slot *slot)
> +{
> +}
[Severity: Critical]
Does leaving this function empty permanently leak hardware IBI slots?
When the core finishes processing an IBI, it returns the slot to the virtual
controller. Because this doesn't forward the recycle request to the parent
hardware controller, will the parent controller eventually exhaust its hardware
slots and permanently stop processing IBIs?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617110355.1591844-1-lakshay.piplani@nxp.com?part=5
^ permalink raw reply
* Re: [PATCH v12 4/7] regulator: p3h2x4x: Add driver for on-die regulators in NXP P3H2x4x i3c hub
From: sashiko-bot @ 2026-06-17 11:17 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: linux-i3c, robh, conor+dt, Frank.Li, devicetree
In-Reply-To: <20260617110355.1591844-5-lakshay.piplani@nxp.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Concurrent regulator operations race on the shared device-level protection register, leading to silent hardware configuration failures.
- [Medium] The driver module will not auto-load because it lacks a MODULE_ALIAS or platform_device_id table.
- [Medium] If regmap_read or the unlock sequence fails in the guard constructor, the driver proceeds with the regulator operation on locked registers.
- [Low] Typo in regulator config assignment assigns dev->of_node to itself instead of rcfg.of_node.
--
commit e68081b58005a2422d441a5611dac29f8cffe2f4
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
regulator: p3h2x4x: Add driver for on-die regulators in NXP P3H2x4x i3c hub
The NXP P3H2x4x family integrates on-die regulators alongside I3C hub
functionality. This driver registers the regulators using the MFD
framework and exposes them via the regulator subsystem.
> --- /dev/null
> +++ b/drivers/regulator/p3h2840_i3c_hub_regulator.c
[ ... ]
> +static void p3h2x4x_reg_guard_enter(struct regulator_dev *rdev,
> + struct p3h2x4x_reg_state *state)
> +{
> + state->restore = false;
> +
> + if (regmap_read(rdev->regmap,
> + P3H2X4X_DEV_REG_PROTECTION_CODE,
> + &state->orig))
> + return;
[Severity: Medium]
If regmap_read() fails here, the function returns early and swallows the
error since the guard constructor returns void. Will this cause the caller
to proceed with regulator operations on locked hardware, potentially
resulting in silent hardware configuration failures?
If the device ACKs the transaction despite ignoring the write, wouldn't
the driver incorrectly report success to the regulator core?
> +
> + if (state->orig != P3H2X4X_REGISTERS_UNLOCK_CODE) {
> + regmap_write(rdev->regmap,
> + P3H2X4X_DEV_REG_PROTECTION_CODE,
> + P3H2X4X_REGISTERS_UNLOCK_CODE);
> + state->restore = true;
> + }
> +}
> +
> +static void p3h2x4x_reg_guard_exit(struct regulator_dev *rdev,
> + struct p3h2x4x_reg_state *state)
> +{
> + if (state->restore)
> + regmap_write(rdev->regmap,
> + P3H2X4X_DEV_REG_PROTECTION_CODE,
> + state->orig);
> +}
> +
> +DEFINE_LOCK_GUARD_1(p3h2x4x_reg, struct regulator_dev,
> + p3h2x4x_reg_guard_enter(_T->lock, &_T->state),
> + p3h2x4x_reg_guard_exit(_T->lock, &_T->state),
> + struct p3h2x4x_reg_state state);
[Severity: High]
Does this guard provide mutual exclusion across the four different regulators
on the same device?
The regmap lock protects individual I/O operations, but since there is no
device-level lock held across the unlock/modify/lock sequence, can concurrent
operations on different regulators interleave?
For example, could one thread re-lock the device in p3h2x4x_reg_guard_exit()
while another thread is just about to write its configuration, causing the
hardware write to be silently ignored? Is a shared device-level mutex needed
to serialize these sequences?
[ ... ]
> +static int p3h2x4x_regulator_probe(struct platform_device *pdev)
> +{
> + struct p3h2x4x_dev *p3h2x4x = dev_get_drvdata(pdev->dev.parent);
> + struct p3h2x4x_regulator_dev *p3h2x4x_regulator;
> + struct regulator_config rcfg = { };
> + struct device *dev = &pdev->dev;
> + struct regulator_dev *rdev;
> + int i;
> +
> + p3h2x4x_regulator = devm_kzalloc(dev, sizeof(*p3h2x4x_regulator), GFP_KERNEL);
> + if (!p3h2x4x_regulator)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, p3h2x4x_regulator);
> +
> + p3h2x4x_regulator->regmap = p3h2x4x->regmap;
> + device_set_of_node_from_dev(dev, dev->parent);
> +
> + rcfg.dev = dev;
> + rcfg.dev->of_node = dev->of_node;
[Severity: Low]
Should this be rcfg.of_node = dev->of_node? Since rcfg.dev is assigned to dev
on the previous line, this just assigns dev->of_node to itself.
> + rcfg.regmap = p3h2x4x_regulator->regmap;
> + rcfg.driver_data = p3h2x4x_regulator;
[ ... ]
> +static struct platform_driver p3h2x4x_regulator_driver = {
> + .driver = {
> + .name = "p3h2x4x-regulator",
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + },
> + .probe = p3h2x4x_regulator_probe,
> +};
> +module_platform_driver(p3h2x4x_regulator_driver);
> +
> +MODULE_AUTHOR("Aman Kumar Pandey <aman.kumarpandey@nxp.com>");
> +MODULE_AUTHOR("Vikash Bansal <vikash.bansal@nxp.com>");
> +MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@nxp.com>");
> +MODULE_DESCRIPTION("NXP P3H2X4X I3C HUB Regulator driver");
> +MODULE_LICENSE("GPL");
[Severity: Medium]
Since the driver is registered as a platform driver with the name
"p3h2x4x-regulator" but the module is compiled as
"p3h2840_i3c_hub_regulator.ko", will this module fail to auto-load?
Without a MODULE_ALIAS("platform:p3h2x4x-regulator") or an exported
platform_device_id table, udev may not be able to load the module when the
MFD core creates the child platform device.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617110355.1591844-1-lakshay.piplani@nxp.com?part=4
^ permalink raw reply
* Re: [PATCH v12 2/7] dt-bindings: i3c: Add NXP P3H2x4x i3c-hub support
From: sashiko-bot @ 2026-06-17 11:13 UTC (permalink / raw)
To: Lakshay Piplani; +Cc: robh, conor+dt, devicetree, linux-i3c, Frank.Li
In-Reply-To: <20260617110355.1591844-3-lakshay.piplani@nxp.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The binding documents GPIO capability for target ports but omits the standard GPIO provider properties, violating subsystem guidelines.
- [Medium] The binding fails to include compatible strings for the 4-port variants and unconditionally allows up to 8 ports.
- [Low] The unit address in the I3C example does not match its `reg` property.
--
commit af3b77595fece3e15864b37b9b683be835a0e1eb
Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
dt-bindings: i3c: Add NXP P3H2x4x i3c-hub support
This commit adds device tree bindings for the NXP P3H2x4x multi-port I3C hub
family. It supports connecting a host via I3C/I2C/SMBus to multiple downstream
peripherals across up to 8 ports.
> diff --git a/Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml b/Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
> new file mode 100644
> index 0000000000000..c080eeb0eeaa8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
> @@ -0,0 +1,291 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2025 NXP
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i3c/nxp,p3h2840.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP P3H2X4X I3C HUB
> +
> +maintainers:
> + - Aman Kumar Pandey <aman.kumarpandey@nxp.com>
> + - Vikash Bansal <vikash.bansal@nxp.com>
> + - Lakshay Piplani <lakshay.piplani@nxp.com>
> +
> +description: |
> + P3H2x4x (P3H2440/P3H2441/P3H2840/P3H2841) is a family of multiport I3C
> + hub devices that connect to:-
> + 1. A host CPU via I3C/I2C/SMBus bus on upstream side and connect to multiple
> + peripheral devices on the downstream side.
> + 2. Have two Controller Ports which can support either
> + I2C/SMBus or I3C buses and connect to a CPU, BMC or SOC.
> + 3. P3H2840/ P3H2841 are 8 port I3C hub with eight I3C/I2C Target Port.
> + 4. P3H2440/ P3H2441 are 4 port I3C hub with four I3C/I2C Target Port.
> + Target ports can be configured as I2C/SMBus, I3C or GPIO and connect to
> + peripherals.
[Severity: Medium]
Since the description advertises that target ports can be configured as GPIO,
should this binding include the standard GPIO provider properties like
gpio-controller and #gpio-cells?
Because unevaluatedProperties is false and there are no child node patterns
defined for GPIO ports in patternProperties, won't adding a GPIO child node
fail schema validation?
> +
> +properties:
> + compatible:
> + const: nxp,p3h2840
[Severity: Medium]
The commit message and description state the binding supports the P3H2440
and P3H2441 4-port hubs alongside the 8-port P3H2840/P3H2841.
Should compatible strings be included for the 4-port variants here as well?
[ ... ]
> +patternProperties:
> + "^i3c@[0-7]$":
> + type: object
> + $ref: /schemas/i3c/i3c.yaml#
> + unevaluatedProperties: false
> +
> + properties:
> + reg:
> + description:
> + The I3C HUB Target Port number.
> + maximum: 7
[Severity: Medium]
Does unconditionally allowing a maximum port index of 7 cause issues for the
4-port variants mentioned in the description?
It seems this allows invalid configurations that define non-existent ports
(4-7) for the 4-port hardware.
[ ... ]
> +examples:
> + - |
> + i3c {
> + #address-cells = <3>;
> + #size-cells = <0>;
> +
> + hub@70,236153000c2 {
> + reg = <0x70 0x236 0x3000c2>;
[Severity: Low]
Does the unit address in this example match its reg property?
The reg property combines to a provisioned ID of 0x236003000c2, which appears
to differ from 236153000c2 in the unit address. Could this mismatch trigger a
simple_bus_reg warning during make dt_binding_check?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617110355.1591844-1-lakshay.piplani@nxp.com?part=2
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox