* Re: [PATCH v6 09/15] media: bcm2835-unicam: Add support for CCP2/CSI2 camera interface
From: Laurent Pinchart @ 2024-03-26 1:37 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Dave Stevenson, David Plowman, Jean-Michel Hautbois,
Hans Verkuil, Naushir Patuck, kernel-list, linux-rpi-kernel,
Florian Fainelli, Ray Jui, Scott Branden,
bcm-kernel-feedback-list, Conor Dooley, Krzysztof Kozlowski,
Rob Herring, devicetree
In-Reply-To: <ZgHEQTHZlRr_Rz6K@kekkonen.localdomain>
Hi Sakari,
On Mon, Mar 25, 2024 at 06:36:49PM +0000, Sakari Ailus wrote:
> On Fri, Mar 01, 2024 at 11:32:24PM +0200, Laurent Pinchart wrote:
> > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >
> > Add a driver for the Unicam camera receiver block on BCM283x processors.
> > It is represented as two video device nodes: unicam-image and
> > unicam-embedded which are connected to an internal subdev (named
> > unicam-subdev) in order to manage streams routing.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Co-developed-by: Naushir Patuck <naush@raspberrypi.com>
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Co-developed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Thanks for submitting this, it's the cleanest and neatest Unicom driver
Unicam, or if you insist Unicorn, but not Unicom :-)
> I've ever seen!
>
> Some mostly unimportant comments below, however the bus-type issue needs to
> be addressed.
>
> > ---
> > Changes since v5:
> >
> > - Move to drivers/media/platform/broadcom/
> > - Port to the upstream V4L2 streams API
> > - Rebase on latest metadata API proposal
> > - Add missing error message
> > - Drop unneeded documentation block for unicam_isr()
> > - Drop unneeded dev_dbg() and dev_err() messages
> > - Drop unneeded streams_mask and fmt checks
> > - Drop unused unicam_sd_pad_is_sink()
> > - Drop unneeded includes
> > - Drop v4l2_ctrl_subscribe_event() call
> > - Use pm_runtime_resume_and_get()
> > - Indentation and line wrap fixes
> > - Let the framework set bus_info
> > - Use v4l2_fwnode_endpoint_parse()
> > - Fix media device cleanup
> > - Drop lane reordering checks
> > - Fix subdev state locking
> > - Drop extra debug messages
> > - Move clock handling to runtime PM handlers
> > - Reorder functions
> > - Rename init functions for more clarity
> > - Initialize runtime PM earlier
> > - Clarify error messages
> > - Simplify subdev init with local variable
> > - Fix subdev cleanup
> > - Fix typos and indentation
> > - Don't initialize local variables needlessly
> > - Simplify num lanes check
> > - Fix metadata handling in subdev set_fmt
> > - Drop manual fallback to .s_stream()
> > - Pass v4l2_pix_format to unicam_calc_format_size_bpl()
> > - Simplify unicam_set_default_format()
> > - Fix default format settings
> > - Add busy check in unicam_s_fmt_meta()
> > - Add missing \n at end of format strings
> > - Fix metadata handling in subdev set_fmt
> > - Fix locking when starting streaming
> > - Return buffers from start streaming fails
> > - Fix format validation for metadata node
> > - Use video_device_pipeline_{start,stop}() helpers
> > - Simplify format enumeration
> > - Drop unset variable
> > - Update MAINTAINERS entry
> > - Update to the upstream v4l2_async_nf API
> > - Update to the latest subdev routing API
> > - Update to the latest subdev state API
> > - Move from subdev .init_cfg() to .init_state()
> > - Update to the latest videobuf2 API
> > - Fix v4l2_subdev_enable_streams() error check
> > - Use correct pad for the connected subdev
> > - Return buffers to vb2 when start streaming fails
> > - Improve debugging in start streaming handler
> > - Simplify DMA address management
> > - Drop comment about bcm2835-camera driver
> > - Clarify comments that explain min/max sizes
> > - Pass v4l2_pix_format to unicam_try_fmt()
> > - Drop unneeded local variables
> > - Rename image-related constants and functions
> > - Turn unicam_fmt.metadata_fmt into bool
> > - Rename unicam_fmt to unicam_format_info
> > - Rename unicam_format_info variables to fmtinfo
> > - Rename unicam_node.v_fmt to fmt
> > - Add metadata formats for RAW10, RAW12 and RAW14
> > - Make metadata formats line-based
> > - Validate format on metadata video device
> > - Add Co-devlopped-by tags
> >
> > Changes since v3:
> >
> > - Add the vendor prefix for DT name
> > - Use the reg-names in DT parsing
> > - Remove MAINTAINERS entry
> >
> > Changes since v2:
> >
> > - Change code organization
> > - Remove unused variables
> > - Correct the fmt_meta functions
> > - Rewrite the start/stop streaming
> > - You can now start the image node alone, but not the metadata one
> > - The buffers are allocated per-node
> > - only the required stream is started, if the route exists and is
> > enabled
> > - Prefix the macros with UNICAM_ to not have too generic names
> > - Drop colorspace support
> >
> > Changes since v1:
> >
> > - Replace the unicam_{info,debug,error} macros with dev_*()
> > ---
> > MAINTAINERS | 1 +
> > drivers/media/platform/Kconfig | 1 +
> > drivers/media/platform/Makefile | 1 +
> > drivers/media/platform/broadcom/Kconfig | 23 +
> > drivers/media/platform/broadcom/Makefile | 3 +
> > .../platform/broadcom/bcm2835-unicam-regs.h | 255 ++
> > .../media/platform/broadcom/bcm2835-unicam.c | 2607 +++++++++++++++++
> > 7 files changed, 2891 insertions(+)
> > create mode 100644 drivers/media/platform/broadcom/Kconfig
> > create mode 100644 drivers/media/platform/broadcom/Makefile
> > create mode 100644 drivers/media/platform/broadcom/bcm2835-unicam-regs.h
> > create mode 100644 drivers/media/platform/broadcom/bcm2835-unicam.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e50a59654e6e..cc350729f467 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4002,6 +4002,7 @@ M: Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com>
> > L: linux-media@vger.kernel.org
> > S: Maintained
> > F: Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> > +F: drivers/media/platform/bcm2835/
> >
> > BROADCOM BCM47XX MIPS ARCHITECTURE
> > M: Hauke Mehrtens <hauke@hauke-m.de>
> > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > index 91e54215de3a..2d79bfc68c15 100644
> > --- a/drivers/media/platform/Kconfig
> > +++ b/drivers/media/platform/Kconfig
> > @@ -67,6 +67,7 @@ source "drivers/media/platform/amlogic/Kconfig"
> > source "drivers/media/platform/amphion/Kconfig"
> > source "drivers/media/platform/aspeed/Kconfig"
> > source "drivers/media/platform/atmel/Kconfig"
> > +source "drivers/media/platform/broadcom/Kconfig"
> > source "drivers/media/platform/cadence/Kconfig"
> > source "drivers/media/platform/chips-media/Kconfig"
> > source "drivers/media/platform/intel/Kconfig"
> > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> > index 3296ec1ebe16..da17301f7439 100644
> > --- a/drivers/media/platform/Makefile
> > +++ b/drivers/media/platform/Makefile
> > @@ -10,6 +10,7 @@ obj-y += amlogic/
> > obj-y += amphion/
> > obj-y += aspeed/
> > obj-y += atmel/
> > +obj-y += broadcom/
> > obj-y += cadence/
> > obj-y += chips-media/
> > obj-y += intel/
> > diff --git a/drivers/media/platform/broadcom/Kconfig b/drivers/media/platform/broadcom/Kconfig
> > new file mode 100644
> > index 000000000000..cc2c9afcc948
> > --- /dev/null
> > +++ b/drivers/media/platform/broadcom/Kconfig
> > @@ -0,0 +1,23 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +config VIDEO_BCM2835_UNICAM
> > + tristate "Broadcom BCM283x/BCM271x Unicam video capture driver"
> > + depends on ARCH_BCM2835 || COMPILE_TEST
> > + depends on PM
> > + depends on VIDEO_DEV
> > + select MEDIA_CONTROLLER
> > + select V4L2_FWNODE
> > + select VIDEO_V4L2_SUBDEV_API
> > + select VIDEOBUF2_DMA_CONTIG
> > + help
> > + Say Y here to enable support for the BCM283x/BCM271x CSI-2 receiver.
> > + This is a V4L2 driver that controls the CSI-2 receiver directly,
> > + independently from the VC4 firmware.
> > +
> > + This driver is mutually exclusive with the use of bcm2835-camera. The
> > + firmware will disable all access to the peripheral from within the
> > + firmware if it finds a DT node using it, and bcm2835-camera will
> > + therefore fail to probe.
> > +
> > + To compile this driver as a module, choose M here. The module will be
> > + called bcm2835-unicam.
> > diff --git a/drivers/media/platform/broadcom/Makefile b/drivers/media/platform/broadcom/Makefile
> > new file mode 100644
> > index 000000000000..03d2045aba2e
> > --- /dev/null
> > +++ b/drivers/media/platform/broadcom/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +obj-$(CONFIG_VIDEO_BCM2835_UNICAM) += bcm2835-unicam.o
> > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam-regs.h b/drivers/media/platform/broadcom/bcm2835-unicam-regs.h
> > new file mode 100644
> > index 000000000000..84775fd2fac5
> > --- /dev/null
> > +++ b/drivers/media/platform/broadcom/bcm2835-unicam-regs.h
> > @@ -0,0 +1,255 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +/*
> > + * Copyright (C) 2017-2020 Raspberry Pi Trading.
>
> Anything up to 2024?
Not really. The registers haven't really changed :-) I'll update the
copyright in the .c file though.
> > + * Dave Stevenson <dave.stevenson@raspberrypi.com>
> > + */
[snip]
> > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > new file mode 100644
> > index 000000000000..716c89b8a217
> > --- /dev/null
> > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > @@ -0,0 +1,2607 @@
[snip]
> > +static irqreturn_t unicam_isr(int irq, void *dev)
> > +{
> > + struct unicam_device *unicam = dev;
> > + unsigned int lines_done = unicam_get_lines_done(dev);
> > + unsigned int sequence = unicam->sequence;
> > + unsigned int i;
> > + u32 ista, sta;
> > + bool fe;
> > + u64 ts;
> > +
> > + sta = unicam_reg_read(unicam, UNICAM_STA);
> > + /* Write value back to clear the interrupts */
> > + unicam_reg_write(unicam, UNICAM_STA, sta);
> > +
> > + ista = unicam_reg_read(unicam, UNICAM_ISTA);
> > + /* Write value back to clear the interrupts */
> > + unicam_reg_write(unicam, UNICAM_ISTA, ista);
> > +
> > + dev_dbg(unicam->dev, "ISR: ISTA: 0x%X, STA: 0x%X, sequence %d, lines done %d\n",
> > + ista, sta, sequence, lines_done);
> > +
> > + if (!(sta & (UNICAM_IS | UNICAM_PI0)))
> > + return IRQ_HANDLED;
> > +
> > + /*
> > + * Look for either the Frame End interrupt or the Packet Capture status
> > + * to signal a frame end.
> > + */
> > + fe = ista & UNICAM_FEI || sta & UNICAM_PI0;
> > +
> > + /*
> > + * We must run the frame end handler first. If we have a valid next_frm
> > + * and we get a simultaneout FE + FS interrupt, running the FS handler
> > + * first would null out the next_frm ptr and we would have lost the
> > + * buffer forever.
> > + */
> > + if (fe) {
> > + /*
> > + * Ensure we have swapped buffers already as we can't
> > + * stop the peripheral. If no buffer is available, use a
> > + * dummy buffer to dump out frames until we get a new buffer
> > + * to use.
> > + */
> > + for (i = 0; i < ARRAY_SIZE(unicam->node); i++) {
> > + if (!unicam->node[i].streaming)
> > + continue;
> > +
> > + /*
> > + * If cur_frm == next_frm, it means we have not had
> > + * a chance to swap buffers, likely due to having
> > + * multiple interrupts occurring simultaneously (like FE
> > + * + FS + LS). In this case, we cannot signal the buffer
> > + * as complete, as the HW will reuse that buffer.
> > + */
> > + if (unicam->node[i].cur_frm &&
> > + unicam->node[i].cur_frm != unicam->node[i].next_frm)
> > + unicam_process_buffer_complete(&unicam->node[i],
> > + sequence);
> > + unicam->node[i].cur_frm = unicam->node[i].next_frm;
> > + }
> > + unicam->sequence++;
>
> Does access to this data need to be serialised somehow.
Given that it's only accessed from the interrupt handler (beside
start_streaming time, before starting the hardware), I don't think so.
> > + }
> > +
> > + if (ista & UNICAM_FSI) {
> > + /*
> > + * Timestamp is to be when the first data byte was captured,
> > + * aka frame start.
> > + */
> > + ts = ktime_get_ns();
> > + for (i = 0; i < ARRAY_SIZE(unicam->node); i++) {
> > + if (!unicam->node[i].streaming)
> > + continue;
> > +
> > + if (unicam->node[i].cur_frm)
> > + unicam->node[i].cur_frm->vb.vb2_buf.timestamp =
> > + ts;
> > + else
> > + dev_dbg(unicam->v4l2_dev.dev,
> > + "ISR: [%d] Dropping frame, buffer not available at FS\n",
> > + i);
> > + /*
> > + * Set the next frame output to go to a dummy frame
> > + * if we have not managed to obtain another frame
> > + * from the queue.
> > + */
> > + unicam_schedule_dummy_buffer(&unicam->node[i]);
> > + }
> > +
> > + unicam_queue_event_sof(unicam);
> > + }
> > +
> > + /*
> > + * Cannot swap buffer at frame end, there may be a race condition
> > + * where the HW does not actually swap it if the new frame has
> > + * already started.
> > + */
> > + if (ista & (UNICAM_FSI | UNICAM_LCI) && !fe) {
> > + for (i = 0; i < ARRAY_SIZE(unicam->node); i++) {
> > + if (!unicam->node[i].streaming)
> > + continue;
> > +
> > + spin_lock(&unicam->node[i].dma_queue_lock);
> > + if (!list_empty(&unicam->node[i].dma_queue) &&
> > + !unicam->node[i].next_frm)
> > + unicam_schedule_next_buffer(&unicam->node[i]);
> > + spin_unlock(&unicam->node[i].dma_queue_lock);
> > + }
> > + }
> > +
> > + if (unicam_reg_read(unicam, UNICAM_ICTL) & UNICAM_FCM) {
> > + /* Switch out of trigger mode if selected */
> > + unicam_reg_write_field(unicam, UNICAM_ICTL, 1, UNICAM_TFC);
> > + unicam_reg_write_field(unicam, UNICAM_ICTL, 0, UNICAM_FCM);
> > + }
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static void unicam_set_packing_config(struct unicam_device *unicam)
> > +{
> > + struct unicam_node *node = &unicam->node[UNICAM_IMAGE_NODE];
> > + u32 pack, unpack;
> > + u32 val;
> > +
> > + if (node->fmt.fmt.pix.pixelformat == node->fmtinfo->fourcc) {
> > + unpack = UNICAM_PUM_NONE;
> > + pack = UNICAM_PPM_NONE;
> > + } else {
> > + switch (node->fmtinfo->depth) {
> > + case 8:
> > + unpack = UNICAM_PUM_UNPACK8;
> > + break;
> > + case 10:
> > + unpack = UNICAM_PUM_UNPACK10;
> > + break;
> > + case 12:
> > + unpack = UNICAM_PUM_UNPACK12;
> > + break;
> > + case 14:
> > + unpack = UNICAM_PUM_UNPACK14;
> > + break;
> > + case 16:
> > + unpack = UNICAM_PUM_UNPACK16;
> > + break;
> > + default:
> > + unpack = UNICAM_PUM_NONE;
> > + break;
> > + }
> > +
> > + /* Repacking is always to 16bpp */
> > + pack = UNICAM_PPM_PACK16;
>
> Also 8-bit data?
Not that I know of. The 8-bit entries in unicam_image_formats have no
.unpacked_fourcc field, so the condition in the if above will always be
true for those as they can only be selected by setting the pixel format
to fmtinfo->fourcc.
> > + }
> > +
> > + val = 0;
>
> You could do initialisation in declaration.
Yes, but I think it's more readable to keep all the code that affects
the 'val' variable together.
> > + unicam_set_field(&val, unpack, UNICAM_PUM_MASK);
> > + unicam_set_field(&val, pack, UNICAM_PPM_MASK);
> > + unicam_reg_write(unicam, UNICAM_IPIPE, val);
> > +}
> > +
> > +static void unicam_cfg_image_id(struct unicam_device *unicam)
> > +{
> > + struct unicam_node *node = &unicam->node[UNICAM_IMAGE_NODE];
> > +
> > + if (unicam->bus_type == V4L2_MBUS_CSI2_DPHY) {
> > + /* CSI2 mode, hardcode VC 0 for now. */
> > + unicam_reg_write(unicam, UNICAM_IDI0,
> > + (0 << 6) | node->fmtinfo->csi_dt);
> > + } else {
> > + /* CCP2 mode */
> > + unicam_reg_write(unicam, UNICAM_IDI0,
> > + 0x80 | node->fmtinfo->csi_dt);
> > + }
> > +}
> > +
> > +static void unicam_enable_ed(struct unicam_device *unicam)
> > +{
> > + u32 val = unicam_reg_read(unicam, UNICAM_DCS);
> > +
> > + unicam_set_field(&val, 2, UNICAM_EDL_MASK);
> > + /* Do not wrap at the end of the embedded data buffer */
> > + unicam_set_field(&val, 0, UNICAM_DBOB);
> > +
> > + unicam_reg_write(unicam, UNICAM_DCS, val);
> > +}
> > +
> > +static void unicam_start_rx(struct unicam_device *unicam,
> > + struct unicam_buffer *buf)
> > +{
> > + struct unicam_node *node = &unicam->node[UNICAM_IMAGE_NODE];
> > + int line_int_freq = node->fmt.fmt.pix.height >> 2;
> > + unsigned int i;
> > + u32 val;
> > +
> > + if (line_int_freq < 128)
> > + line_int_freq = 128;
>
> line_int_freq = max(line_int_freq, 128);
Ack.
> > +
> > + /* Enable lane clocks */
> > + val = 1;
>
> Initialise in the loop initialisation below, I'd say.
How about
val = 0x55 & GENMASK(unicam->pipe.num_data_lanes * 2 - 1, 0);
?
> > + for (i = 0; i < unicam->active_data_lanes; i++)
> > + val = val << 2 | 1;
> > + unicam_clk_write(unicam, val);
> > +
> > + /* Basic init */
> > + unicam_reg_write(unicam, UNICAM_CTRL, UNICAM_MEM);
> > +
> > + /* Enable analogue control, and leave in reset. */
> > + val = UNICAM_AR;
> > + unicam_set_field(&val, 7, UNICAM_CTATADJ_MASK);
> > + unicam_set_field(&val, 7, UNICAM_PTATADJ_MASK);
> > + unicam_reg_write(unicam, UNICAM_ANA, val);
> > + usleep_range(1000, 2000);
> > +
> > + /* Come out of reset */
> > + unicam_reg_write_field(unicam, UNICAM_ANA, 0, UNICAM_AR);
> > +
> > + /* Peripheral reset */
> > + unicam_reg_write_field(unicam, UNICAM_CTRL, 1, UNICAM_CPR);
> > + unicam_reg_write_field(unicam, UNICAM_CTRL, 0, UNICAM_CPR);
> > +
> > + unicam_reg_write_field(unicam, UNICAM_CTRL, 0, UNICAM_CPE);
> > +
> > + /* Enable Rx control. */
> > + val = unicam_reg_read(unicam, UNICAM_CTRL);
> > + if (unicam->bus_type == V4L2_MBUS_CSI2_DPHY) {
> > + unicam_set_field(&val, UNICAM_CPM_CSI2, UNICAM_CPM_MASK);
> > + unicam_set_field(&val, UNICAM_DCM_STROBE, UNICAM_DCM_MASK);
> > + } else {
> > + unicam_set_field(&val, UNICAM_CPM_CCP2, UNICAM_CPM_MASK);
> > + unicam_set_field(&val, unicam->bus_flags, UNICAM_DCM_MASK);
> > + }
> > + /* Packet framer timeout */
> > + unicam_set_field(&val, 0xf, UNICAM_PFT_MASK);
> > + unicam_set_field(&val, 128, UNICAM_OET_MASK);
> > + unicam_reg_write(unicam, UNICAM_CTRL, val);
> > +
> > + unicam_reg_write(unicam, UNICAM_IHWIN, 0);
> > + unicam_reg_write(unicam, UNICAM_IVWIN, 0);
> > +
> > + /* AXI bus access QoS setup */
> > + val = unicam_reg_read(unicam, UNICAM_PRI);
> > + unicam_set_field(&val, 0, UNICAM_BL_MASK);
> > + unicam_set_field(&val, 0, UNICAM_BS_MASK);
> > + unicam_set_field(&val, 0xe, UNICAM_PP_MASK);
> > + unicam_set_field(&val, 8, UNICAM_NP_MASK);
> > + unicam_set_field(&val, 2, UNICAM_PT_MASK);
> > + unicam_set_field(&val, 1, UNICAM_PE);
> > + unicam_reg_write(unicam, UNICAM_PRI, val);
> > +
> > + unicam_reg_write_field(unicam, UNICAM_ANA, 0, UNICAM_DDL);
> > +
> > + /* Always start in trigger frame capture mode (UNICAM_FCM set) */
> > + val = UNICAM_FSIE | UNICAM_FEIE | UNICAM_FCM | UNICAM_IBOB;
> > + unicam_set_field(&val, line_int_freq, UNICAM_LCIE_MASK);
> > + unicam_reg_write(unicam, UNICAM_ICTL, val);
> > + unicam_reg_write(unicam, UNICAM_STA, UNICAM_STA_MASK_ALL);
> > + unicam_reg_write(unicam, UNICAM_ISTA, UNICAM_ISTA_MASK_ALL);
> > +
> > + /* tclk_term_en */
> > + unicam_reg_write_field(unicam, UNICAM_CLT, 2, UNICAM_CLT1_MASK);
> > + /* tclk_settle */
> > + unicam_reg_write_field(unicam, UNICAM_CLT, 6, UNICAM_CLT2_MASK);
> > + /* td_term_en */
> > + unicam_reg_write_field(unicam, UNICAM_DLT, 2, UNICAM_DLT1_MASK);
> > + /* ths_settle */
> > + unicam_reg_write_field(unicam, UNICAM_DLT, 6, UNICAM_DLT2_MASK);
> > + /* trx_enable */
> > + unicam_reg_write_field(unicam, UNICAM_DLT, 0, UNICAM_DLT3_MASK);
> > +
> > + unicam_reg_write_field(unicam, UNICAM_CTRL, 0, UNICAM_SOE);
> > +
> > + /* Packet compare setup - required to avoid missing frame ends */
> > + val = 0;
> > + unicam_set_field(&val, 1, UNICAM_PCE);
> > + unicam_set_field(&val, 1, UNICAM_GI);
> > + unicam_set_field(&val, 1, UNICAM_CPH);
> > + unicam_set_field(&val, 0, UNICAM_PCVC_MASK);
> > + unicam_set_field(&val, 1, UNICAM_PCDT_MASK);
> > + unicam_reg_write(unicam, UNICAM_CMP0, val);
> > +
> > + /* Enable clock lane and set up terminations */
> > + val = 0;
> > + if (unicam->bus_type == V4L2_MBUS_CSI2_DPHY) {
> > + /* CSI2 */
> > + unicam_set_field(&val, 1, UNICAM_CLE);
> > + unicam_set_field(&val, 1, UNICAM_CLLPE);
> > + if (!(unicam->bus_flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK)) {
> > + unicam_set_field(&val, 1, UNICAM_CLTRE);
> > + unicam_set_field(&val, 1, UNICAM_CLHSE);
> > + }
> > + } else {
> > + /* CCP2 */
> > + unicam_set_field(&val, 1, UNICAM_CLE);
> > + unicam_set_field(&val, 1, UNICAM_CLHSE);
> > + unicam_set_field(&val, 1, UNICAM_CLTRE);
> > + }
> > + unicam_reg_write(unicam, UNICAM_CLK, val);
> > +
> > + /*
> > + * Enable required data lanes with appropriate terminations.
> > + * The same value needs to be written to UNICAM_DATn registers for
> > + * the active lanes, and 0 for inactive ones.
> > + */
> > + val = 0;
> > + if (unicam->bus_type == V4L2_MBUS_CSI2_DPHY) {
> > + /* CSI2 */
> > + unicam_set_field(&val, 1, UNICAM_DLE);
> > + unicam_set_field(&val, 1, UNICAM_DLLPE);
> > + if (!(unicam->bus_flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK)) {
> > + unicam_set_field(&val, 1, UNICAM_DLTRE);
> > + unicam_set_field(&val, 1, UNICAM_DLHSE);
> > + }
> > + } else {
> > + /* CCP2 */
> > + unicam_set_field(&val, 1, UNICAM_DLE);
> > + unicam_set_field(&val, 1, UNICAM_DLHSE);
> > + unicam_set_field(&val, 1, UNICAM_DLTRE);
> > + }
> > + unicam_reg_write(unicam, UNICAM_DAT0, val);
> > +
> > + if (unicam->active_data_lanes == 1)
> > + val = 0;
> > + unicam_reg_write(unicam, UNICAM_DAT1, val);
> > +
> > + if (unicam->max_data_lanes > 2) {
> > + /*
> > + * Registers UNICAM_DAT2 and UNICAM_DAT3 only valid if the
> > + * instance supports more than 2 data lanes.
> > + */
> > + if (unicam->active_data_lanes == 2)
> > + val = 0;
> > + unicam_reg_write(unicam, UNICAM_DAT2, val);
> > +
> > + if (unicam->active_data_lanes == 3)
> > + val = 0;
> > + unicam_reg_write(unicam, UNICAM_DAT3, val);
> > + }
> > +
> > + unicam_reg_write(unicam, UNICAM_IBLS,
> > + node->fmt.fmt.pix.bytesperline);
> > + unicam_wr_dma_addr(&unicam->node[UNICAM_IMAGE_NODE], buf);
> > + unicam_set_packing_config(unicam);
> > + unicam_cfg_image_id(unicam);
> > +
> > + val = unicam_reg_read(unicam, UNICAM_MISC);
> > + unicam_set_field(&val, 1, UNICAM_FL0);
> > + unicam_set_field(&val, 1, UNICAM_FL1);
> > + unicam_reg_write(unicam, UNICAM_MISC, val);
> > +
> > + /* Enable peripheral */
> > + unicam_reg_write_field(unicam, UNICAM_CTRL, 1, UNICAM_CPE);
> > +
> > + /* Load image pointers */
> > + unicam_reg_write_field(unicam, UNICAM_ICTL, 1, UNICAM_LIP_MASK);
> > +
> > + /*
> > + * Enable trigger only for the first frame to
> > + * sync correctly to the FS from the source.
> > + */
> > + unicam_reg_write_field(unicam, UNICAM_ICTL, 1, UNICAM_TFC);
> > +}
[snip]
> > +static int unicam_async_nf_init(struct unicam_device *unicam)
> > +{
> > + struct v4l2_fwnode_endpoint ep = { };
>
> If the bus-type property is mandatory and you have no stated defaults
> anywhere, this is fine. I.e. all the relevant properties would need to be
> mandatory.
They are, as far as I can tell (well, the clock-noncontinuous property
is not mandatory, but that's expected as it's a flag).
> > + struct fwnode_handle *ep_handle;
> > + struct v4l2_async_connection *asc;
> > + int ret;
> > +
> > + ret = of_property_read_u32(unicam->dev->of_node, "brcm,num-data-lanes",
> > + &unicam->max_data_lanes);
> > + if (ret < 0) {
> > + dev_err(unicam->dev, "Missing %s DT property\n",
> > + "brcm,num-data-lanes");
> > + return -EINVAL;
> > + }
> > +
> > + /* Get and parse the local endpoint. */
> > + ep_handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(unicam->dev), 0, 0,
> > + FWNODE_GRAPH_ENDPOINT_NEXT);
> > + if (!ep_handle) {
> > + dev_err(unicam->dev, "No endpoint found\n");
> > + return -ENODEV;
> > + }
> > +
> > + ret = v4l2_fwnode_endpoint_parse(ep_handle, &ep);
> > + if (ret) {
> > + dev_err(unicam->dev, "Failed to parse endpoint: %d\n", ret);
> > + goto error;
> > + }
> > +
> > + unicam->bus_type = ep.bus_type;
> > +
> > + switch (ep.bus_type) {
> > + case V4L2_MBUS_CSI2_DPHY: {
> > + unsigned int num_data_lanes = ep.bus.mipi_csi2.num_data_lanes;
> > +
> > + if (num_data_lanes != 1 && num_data_lanes != 2 &&
> > + num_data_lanes != 4) {
> > + dev_err(unicam->dev, "%u data lanes not supported\n",
> > + num_data_lanes);
> > + goto error;
> > + }
> > +
> > + if (num_data_lanes > unicam->max_data_lanes) {
> > + dev_err(unicam->dev,
> > + "Endpoint uses %u data lanes when %u are supported\n",
> > + num_data_lanes, unicam->max_data_lanes);
> > + goto error;
> > + }
> > +
> > + unicam->active_data_lanes = num_data_lanes;
> > + unicam->bus_flags = ep.bus.mipi_csi2.flags;
> > + break;
> > + }
> > +
> > + case V4L2_MBUS_CCP2:
> > + unicam->max_data_lanes = 1;
> > + unicam->active_data_lanes = 1;
> > + unicam->bus_flags = ep.bus.mipi_csi1.strobe;
> > + break;
> > +
> > + default:
> > + /* Unsupported bus type */
> > + dev_err(unicam->dev, "Unsupported bus type %u\n", ep.bus_type);
> > + goto error;
> > + }
> > +
> > + /* Initialize and register the async notifier. */
> > + v4l2_async_nf_init(&unicam->notifier, &unicam->v4l2_dev);
> > +
> > + asc = v4l2_async_nf_add_fwnode_remote(&unicam->notifier, ep_handle,
> > + struct v4l2_async_connection);
> > + fwnode_handle_put(ep_handle);
> > + ep_handle = NULL;
> > +
> > + if (IS_ERR(asc)) {
> > + ret = PTR_ERR(asc);
> > + dev_err(unicam->dev, "Failed to add entry to notifier: %d\n",
> > + ret);
> > + goto error;
> > + }
> > +
> > + unicam->notifier.ops = &unicam_async_ops;
> > +
> > + ret = v4l2_async_nf_register(&unicam->notifier);
> > + if (ret) {
> > + dev_err(unicam->dev, "Error registering device notifier: %d\n",
> > + ret);
> > + goto error;
> > + }
> > +
> > + return 0;
> > +
> > +error:
> > + fwnode_handle_put(ep_handle);
> > + return ret;
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Probe & remove
> > + */
> > +
> > +static int unicam_media_init(struct unicam_device *unicam)
> > +{
> > + int ret;
> > +
> > + unicam->mdev.dev = unicam->dev;
> > + strscpy(unicam->mdev.model, UNICAM_MODULE_NAME,
> > + sizeof(unicam->mdev.model));
> > + strscpy(unicam->mdev.serial, "", sizeof(unicam->mdev.serial));
>
> Isn't the field already zeroed?
Indeed. I'll drop this.
>
> > + unicam->mdev.hw_revision = 0;
> > +
> > + media_device_init(&unicam->mdev);
> > +
> > + unicam->v4l2_dev.mdev = &unicam->mdev;
> > +
> > + ret = v4l2_device_register(unicam->dev, &unicam->v4l2_dev);
> > + if (ret < 0) {
> > + dev_err(unicam->dev, "Unable to register v4l2 device\n");
> > + goto err_media_cleanup;
> > + }
> > +
> > + ret = media_device_register(&unicam->mdev);
> > + if (ret < 0) {
> > + dev_err(unicam->dev,
> > + "Unable to register media-controller device\n");
> > + goto err_v4l2_unregister;
> > + }
> > +
> > + return 0;
> > +
> > +err_v4l2_unregister:
> > + v4l2_device_unregister(&unicam->v4l2_dev);
> > +err_media_cleanup:
> > + media_device_cleanup(&unicam->mdev);
> > + return ret;
> > +}
[snip]
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH] of: property: fw_devlink: Fix stupid bug in remote-endpoint parsing
From: Saravana Kannan @ 2024-03-26 1:35 UTC (permalink / raw)
To: John Watts
Cc: Rob Herring, Frank Rowand, Hervé Codina, Luca Ceresoli,
kernel-team, Rob Herring, devicetree, linux-kernel
In-Reply-To: <ZgIZ4LmFOqdiDJBH@titan>
On Mon, Mar 25, 2024 at 5:42 PM John Watts <contact@jookia.org> wrote:
>
> Hello there,
>
> On Mon, Mar 25, 2024 at 03:49:44PM -0700, Saravana Kannan wrote:
> > Ok, I think I understand now what's going on. fw_devlink does not know
> > that "sound" device will not populate "multi" as a child device.
> > Typically in such situations, "sound" would probe as a device and add
> > its child DT nodes devices. At that point, the cycle is only between
> > "multi" and "test_codec" and fw_devlink will detect that and not
> > enforce any ordering. However, in this case, "sound" doesn't have any
> > child devices and just depends on the remote endpoints directly.
> >
> > We already have "ports", "in-ports" and "out-ports". Is there a reason
> > none of them will work for your use case and it has to be "multi"?
> > When you use one of those 3 recognized node names, things are handled
> > correctly.
>
> audio-graph-card2 uses 'multi' to define DAI links that have multiple
> endpoints. It also suports codec2codec and dpcm.
>
> > I think the right fix is the use of post-init-providers. Because even
> > if you do the above, all it does is let fw_devlink see that there's a
> > cyclic dependency in DT. And it'll stop enforcing the probe and
> > suspend/resume ordering. Ideally we want to enforce a specific order
> > here. test_codec first and then sound.
>
> Is there a way to do this automatically so all the existing audio-graph-card2
> device trees aren't broken? As it stands it seems like this driver is now
> broken due to this change.
Ok, I have a solution. Have the audio-graph-card2 find the fwnode of
"multi" and mark it as "not a device" by doing something like this in
the driver. That should help fw_devlink handle this correctly.
fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
>
> > Maybe. But the logs would be more helpful.
>
> If you have a way for me to get more logs please tell me.
>
> > > > post-init-provider = <&multi>;
> >
> > Did you try this? Did it help?
> >
> > -Saravana
>
> No I haven't tried this yet. I shall try it soon. But I wouldn't consider
> this a useful fix as it requires upgrading existing device trees.
Definitely do this though as a forward looking improvement. It'll help
make the suspend/resume more deterministic and will eventually let
things happen in an async manner.
-Saravana
^ permalink raw reply
* [PATCH 2/2] dt-bindings: arm: bcm: raspberrypi,bcm2835-firmware: Add gpio child node
From: Laurent Pinchart @ 2024-03-26 0:49 UTC (permalink / raw)
To: devicetree, linux-rpi-kernel, linux-arm-kernel, linux-gpio
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Florian Fainelli,
Broadcom internal kernel review list, Ray Jui, Scott Branden,
Linus Walleij, Bartosz Golaszewski, Eric Anholt, Stefan Wahren
In-Reply-To: <20240326004902.17054-1-laurent.pinchart@ideasonboard.com>
Unlike the other child nodes of the raspberrypi,bcm2835-firmware device,
the gpio child is documented in a legacy text-based binding in
gpio/raspberrypi,firmware-gpio.txt. This causes DT validation failures:
arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b.dtb: 'gpio' does not match any of the regexes: 'pinctrl-[0-9]+'
from schema $id: http://devicetree.org/schemas/arm/bcm/raspberrypi,bcm2835-firmware.yaml#
Convert the binding to YAML and move it to
raspberrypi,bcm2835-firmware.yaml.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
.../arm/bcm/raspberrypi,bcm2835-firmware.yaml | 29 ++++++++++++++++++
.../gpio/raspberrypi,firmware-gpio.txt | 30 -------------------
2 files changed, 29 insertions(+), 30 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/gpio/raspberrypi,firmware-gpio.txt
diff --git a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
index dc38f2be7ad6..999e1bc49539 100644
--- a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
+++ b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
@@ -54,6 +54,29 @@ properties:
- compatible
- "#clock-cells"
+ gpio:
+ type: object
+ additionalProperties: false
+
+ properties:
+ compatible:
+ const: raspberrypi,firmware-gpio
+
+ gpio-controller: true
+
+ "#gpio-cells":
+ const: 2
+ description:
+ The first cell is the pin number, and the second cell is used to
+ specify the gpio polarity (GPIO_ACTIVE_HIGH or GPIO_ACTIVE_LOW).
+
+ gpio-line-names: true
+
+ required:
+ - compatible
+ - gpio-controller
+ - "#gpio-cells"
+
reset:
type: object
additionalProperties: false
@@ -112,6 +135,12 @@ examples:
#clock-cells = <1>;
};
+ expgpio: gpio {
+ compatible = "raspberrypi,firmware-gpio";
+ gpio-controller;
+ #gpio-cells = <2>;
+ };
+
reset: reset {
compatible = "raspberrypi,firmware-reset";
#reset-cells = <1>;
diff --git a/Documentation/devicetree/bindings/gpio/raspberrypi,firmware-gpio.txt b/Documentation/devicetree/bindings/gpio/raspberrypi,firmware-gpio.txt
deleted file mode 100644
index ce97265e23ba..000000000000
--- a/Documentation/devicetree/bindings/gpio/raspberrypi,firmware-gpio.txt
+++ /dev/null
@@ -1,30 +0,0 @@
-Raspberry Pi GPIO expander
-
-The Raspberry Pi 3 GPIO expander is controlled by the VC4 firmware. The
-firmware exposes a mailbox interface that allows the ARM core to control the
-GPIO lines on the expander.
-
-The Raspberry Pi GPIO expander node must be a child node of the Raspberry Pi
-firmware node.
-
-Required properties:
-
-- compatible : Should be "raspberrypi,firmware-gpio"
-- gpio-controller : Marks the device node as a gpio controller
-- #gpio-cells : Should be two. The first cell is the pin number, and
- the second cell is used to specify the gpio polarity:
- 0 = active high
- 1 = active low
-
-Example:
-
-firmware: firmware-rpi {
- compatible = "raspberrypi,bcm2835-firmware";
- mboxes = <&mailbox>;
-
- expgpio: gpio {
- compatible = "raspberrypi,firmware-gpio";
- gpio-controller;
- #gpio-cells = <2>;
- };
-};
--
Regards,
Laurent Pinchart
^ permalink raw reply related
* [PATCH 1/2] dt-bindings: arm: bcm: raspberrypi,bcm2835-firmware: Add missing properties
From: Laurent Pinchart @ 2024-03-26 0:49 UTC (permalink / raw)
To: devicetree, linux-rpi-kernel, linux-arm-kernel, linux-gpio
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Florian Fainelli,
Broadcom internal kernel review list, Ray Jui, Scott Branden,
Linus Walleij, Bartosz Golaszewski, Eric Anholt, Stefan Wahren
In-Reply-To: <20240326004902.17054-1-laurent.pinchart@ideasonboard.com>
The raspberrypi,bcm2835-firmware devices requires a dma-ranges property,
and, as a result, also needs to specify #address-cells and #size-cells.
Those properties have been added to thebcm2835-rpi.dtsi in commits
be08d278eb09 ("ARM: dts: bcm283x: Add cells encoding format to firmware
bus") and 55c7c0621078 ("ARM: dts: bcm283x: Fix vc4's firmware bus DMA
limitations"), but the DT bindings haven't been updated, resulting in
validation errors:
arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b.dtb: firmware: '#address-cells', '#size-cells', 'dma-ranges', 'gpio' do not match any of the regexes: 'pinctrl-[0-9]+'
from schema $id: http://devicetree.org/schemas/arm/bcm/raspberrypi,bcm2835-firmware.yaml#
Fix this by adding the properties to the bindings.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
.../arm/bcm/raspberrypi,bcm2835-firmware.yaml | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
index 39e3c248f5b7..dc38f2be7ad6 100644
--- a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
+++ b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
@@ -25,6 +25,14 @@ properties:
- const: raspberrypi,bcm2835-firmware
- const: simple-mfd
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 1
+
+ dma-ranges: true
+
mboxes:
maxItems: 1
@@ -81,6 +89,9 @@ properties:
required:
- compatible
+ - "#address-cells"
+ - "#size-cells"
+ - dma-ranges
- mboxes
additionalProperties: false
@@ -89,6 +100,11 @@ examples:
- |
firmware {
compatible = "raspberrypi,bcm2835-firmware", "simple-mfd";
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+ dma-ranges;
+
mboxes = <&mailbox>;
firmware_clocks: clocks {
--
Regards,
Laurent Pinchart
^ permalink raw reply related
* [PATCH 0/2] dt-bindings: arm: bcm: raspberrypi,bcm2835-firmware: Drive-by fixes
From: Laurent Pinchart @ 2024-03-26 0:49 UTC (permalink / raw)
To: devicetree, linux-rpi-kernel, linux-arm-kernel, linux-gpio
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Florian Fainelli,
Broadcom internal kernel review list, Ray Jui, Scott Branden,
Linus Walleij, Bartosz Golaszewski, Eric Anholt, Stefan Wahren
Hello,
This small series includes two drive-by fixes to the
raspberrypi,bcm2835-firmware DT bindings that fix validation errors with
the Raspberry Pi 4 device tree sources. I noticed those issues when
working on the Raspberry Pi Unicam driver, but two patches are
independent of that work, they can thus be applied separately.
Laurent Pinchart (2):
dt-bindings: arm: bcm: raspberrypi,bcm2835-firmware: Add missing
properties
dt-bindings: arm: bcm: raspberrypi,bcm2835-firmware: Add gpio child
node
.../arm/bcm/raspberrypi,bcm2835-firmware.yaml | 45 +++++++++++++++++++
.../gpio/raspberrypi,firmware-gpio.txt | 30 -------------
2 files changed, 45 insertions(+), 30 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/gpio/raspberrypi,firmware-gpio.txt
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH] of: property: fw_devlink: Fix stupid bug in remote-endpoint parsing
From: John Watts @ 2024-03-26 0:42 UTC (permalink / raw)
To: Saravana Kannan
Cc: Rob Herring, Frank Rowand, Hervé Codina, Luca Ceresoli,
kernel-team, Rob Herring, devicetree, linux-kernel
In-Reply-To: <CAGETcx_=MmfgDajM16iJ4Of9Yr2Sy6ZpU=MyhYgnmOJFUTD_oA@mail.gmail.com>
Hello there,
On Mon, Mar 25, 2024 at 03:49:44PM -0700, Saravana Kannan wrote:
> Ok, I think I understand now what's going on. fw_devlink does not know
> that "sound" device will not populate "multi" as a child device.
> Typically in such situations, "sound" would probe as a device and add
> its child DT nodes devices. At that point, the cycle is only between
> "multi" and "test_codec" and fw_devlink will detect that and not
> enforce any ordering. However, in this case, "sound" doesn't have any
> child devices and just depends on the remote endpoints directly.
>
> We already have "ports", "in-ports" and "out-ports". Is there a reason
> none of them will work for your use case and it has to be "multi"?
> When you use one of those 3 recognized node names, things are handled
> correctly.
audio-graph-card2 uses 'multi' to define DAI links that have multiple
endpoints. It also suports codec2codec and dpcm.
> I think the right fix is the use of post-init-providers. Because even
> if you do the above, all it does is let fw_devlink see that there's a
> cyclic dependency in DT. And it'll stop enforcing the probe and
> suspend/resume ordering. Ideally we want to enforce a specific order
> here. test_codec first and then sound.
Is there a way to do this automatically so all the existing audio-graph-card2
device trees aren't broken? As it stands it seems like this driver is now
broken due to this change.
> Maybe. But the logs would be more helpful.
If you have a way for me to get more logs please tell me.
> > > post-init-provider = <&multi>;
>
> Did you try this? Did it help?
>
> -Saravana
No I haven't tried this yet. I shall try it soon. But I wouldn't consider
this a useful fix as it requires upgrading existing device trees.
John.
^ permalink raw reply
* Re: [PATCH v6 08/15] dt-bindings: media: Add bindings for bcm2835-unicam
From: Laurent Pinchart @ 2024-03-25 23:02 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Dave Stevenson, David Plowman, Jean-Michel Hautbois,
Hans Verkuil, Naushir Patuck, kernel-list, linux-rpi-kernel,
Florian Fainelli, Ray Jui, Scott Branden,
bcm-kernel-feedback-list, Conor Dooley, Krzysztof Kozlowski,
Rob Herring, devicetree
In-Reply-To: <ZgHCOySoPhOS0u2M@kekkonen.localdomain>
Hi Sakari,
On Mon, Mar 25, 2024 at 06:28:11PM +0000, Sakari Ailus wrote:
> On Fri, Mar 01, 2024 at 11:32:23PM +0200, Laurent Pinchart wrote:
> > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >
> > Introduce the dt-bindings documentation for bcm2835 CCP2/CSI2 Unicam
> > camera interface.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Co-developed-by: Naushir Patuck <naush@raspberrypi.com>
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Co-developed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v5:
> >
> > - Squash MAINTAINERS changes in
> >
> > Changes since v3:
> >
> > - Make MAINTAINERS its own patch
> > - Describe the reg and clocks correctly
> > - Use a vendor entry for the number of data lanes
> > ---
> > .../bindings/media/brcm,bcm2835-unicam.yaml | 117 ++++++++++++++++++
> > MAINTAINERS | 6 +
> > 2 files changed, 123 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> > new file mode 100644
> > index 000000000000..1938ace23b3d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> > @@ -0,0 +1,117 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/brcm,bcm2835-unicam.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Broadcom BCM283x Camera Interface (Unicam)
> > +
> > +maintainers:
> > + - Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com>
> > +
> > +description: |-
> > + The Unicam block on BCM283x SoCs is the receiver for either
> > + CSI-2 or CCP2 data from image sensors or similar devices.
> > +
> > + The main platform using this SoC is the Raspberry Pi family of boards. On
> > + the Pi the VideoCore firmware can also control this hardware block, and
> > + driving it from two different processors will cause issues. To avoid this,
> > + the firmware checks the device tree configuration during boot. If it finds
> > + device tree nodes whose name starts with 'csi' then it will stop the firmware
> > + accessing the block, and it can then safely be used via the device tree
> > + binding.
> > +
> > +properties:
> > + compatible:
> > + const: brcm,bcm2835-unicam
> > +
> > + reg:
> > + items:
> > + - description: Unicam block.
> > + - description: Clock Manager Image (CMI) block.
> > +
> > + reg-names:
> > + items:
> > + - const: unicam
> > + - const: cmi
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + clocks:
> > + items:
> > + - description: Clock to drive the LP state machine of Unicam.
> > + - description: Clock for the VPU (core clock).
> > +
> > + clock-names:
> > + items:
> > + - const: lp
> > + - const: vpu
> > +
> > + power-domains:
> > + items:
> > + - description: Unicam power domain
> > +
> > + brcm,num-data-lanes:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [ 2, 4 ]
> > + description: |
> > + Number of CSI-2 data lanes supported by this Unicam instance. The number
> > + of data lanes actively used is specified with the data-lanes endpoint
> > + property.
> > +
> > + port:
> > + $ref: /schemas/graph.yaml#/$defs/port-base
> > + unevaluatedProperties: false
> > +
> > + properties:
> > + endpoint:
> > + $ref: /schemas/media/video-interfaces.yaml#
> > + unevaluatedProperties: false
> > +
> > + properties:
> > + data-lanes: true
> > +
> > + required:
> > + - data-lanes
>
> As the device supports multiple data interfaces (at least so it seems when
> looking at the driver code), you need to list the bus-type property here,
> too.
Good point, I'll add
properties:
bus-type:
enum: [ 3, 4 ]
required:
- bus-type
Should I also change unevaluatedProperties to additionalProperties for
the endpoint node, to reject any other property (and the explicitly list
remote-endpoint as an allowed property) ? The result would be
endpoint:
$ref: /schemas/media/video-interfaces.yaml#
additionalProperties: false
properties:
bus-type:
enum: [ 3, 4 ]
data-lanes: true
remote-endpoint: true
required:
- bus-type
- data-lanes
- remote-endpoint
> > +
> > + required:
> > + - endpoint
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - reg-names
> > + - interrupts
> > + - clocks
> > + - clock-names
> > + - power-domains
> > + - brcm,num-data-lanes
> > + - port
> > +
> > +additionalProperties: False
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/clock/bcm2835.h>
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > + #include <dt-bindings/power/raspberrypi-power.h>
> > + csi1: csi@7e801000 {
> > + compatible = "brcm,bcm2835-unicam";
> > + reg = <0x7e801000 0x800>,
> > + <0x7e802004 0x4>;
> > + reg-names = "unicam", "cmi";
> > + interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&clocks BCM2835_CLOCK_CAM1>,
> > + <&firmware_clocks 4>;
> > + clock-names = "lp", "vpu";
> > + power-domains = <&power RPI_POWER_DOMAIN_UNICAM1>;
> > + brcm,num-data-lanes = <2>;
> > + port {
> > + csi1_ep: endpoint {
> > + remote-endpoint = <&imx219_0>;
> > + data-lanes = <1 2>;
> > + };
> > + };
> > + };
> > +...
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index fada59148cb5..e50a59654e6e 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3997,6 +3997,12 @@ N: bcm113*
> > N: bcm216*
> > N: kona
> >
> > +BROADCOM BCM2835 CAMERA DRIVERS
> > +M: Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com>
> > +L: linux-media@vger.kernel.org
> > +S: Maintained
> > +F: Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> > +
> > BROADCOM BCM47XX MIPS ARCHITECTURE
> > M: Hauke Mehrtens <hauke@hauke-m.de>
> > M: Rafał Miłecki <zajec5@gmail.com>
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v2 0/3] QCM2290 LMH
From: Nícolas F. R. A. Prado @ 2024-03-25 23:01 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Konrad Dybcio, Bjorn Andersson, Rafael J. Wysocki, Daniel Lezcano,
Zhang Rui, Lukasz Luba, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Thara Gopinath, Amit Kucheria, Marijn Suijten,
linux-arm-msm, linux-pm, devicetree, linux-kernel,
Dmitry Baryshkov, stable, Loic Poulain
In-Reply-To: <dbe90a1c-bac2-4176-8eba-7ad96a182313@linaro.org>
On Mon, Mar 25, 2024 at 08:59:55PM +0100, Krzysztof Kozlowski wrote:
> On 20/03/2024 20:08, Nícolas F. R. A. Prado wrote:
> >> Loic Poulain (1):
> >> arm64: dts: qcom: qcm2290: Add LMH node
> >>
> >> Documentation/devicetree/bindings/thermal/qcom-lmh.yaml | 12 ++++++++----
> >> arch/arm64/boot/dts/qcom/qcm2290.dtsi | 14 +++++++++++++-
> >> drivers/thermal/qcom/lmh.c | 3 +++
> >> 3 files changed, 24 insertions(+), 5 deletions(-)
> >
> > Hi,
> >
> > I've started tracking the results of 'make dtbs_check' on linux-next, and I've
> > noticed that on today's next, next-20240320, there's a new warning coming from
> > this. The reason is that the DT change has landed, but the binding has not,
> > since it goes through a separate tree. I thought the binding was supposed to
> > always land before the driver and DT that make use of it, but looking through
>
> There is no such rule. Of course new binding should be documented in
> earlier or the same kernel release cycle as users get in, but it's not a
> requirement.
So, after giving the documentation a second look, I found this:
"For new platforms, or additions to existing ones, make dtbs_check should not
add any new warnings."
Source: https://www.kernel.org/doc/html/latest/process/maintainer-soc.html#validating-devicetree-files
What is not clear there is what the reference point is: is it on linux-next?
Mainline release?
As Konrad pointed out it's tricky (and maybe not worth it) to guarantee this for
linux-next. But for mainline release it seems feasible (and IMO the target, as
after that stability guarantees should apply).
>
>
> > the dt-binding documentation pages I couldn't find anything confirming or
> > denying that.
> >
> > I expect this to happen again in the future, which is why I'm reaching out to
> > understand better how to deal with this kind of situation.
>
> Deal as what to do? Are you asking in terms of maintenance of some
> subsystem or sending some patches? In this particular case here, I don't
> think there is anything on your side to deal with.
I'm asking what's the most helpful way to you the maintainers for me to report
these failures in the future.
Rob has already automated running dtbs_check for patches coming into the mailing
list. And I have set up KernelCI to run dtbs_check on linux-next in order to
catch any issues that might slip through, or happen during integration of the
trees, etc.
Now, if we agree that dtbs_check regressions on linux-next are acceptable, at
least ones like this, where the issue is just synchronization between
maintainers, then I can simply not report them in the future. But we should
have some point where dtbs_check should not regress, and mainline release seems
the reasonable choice, because if we don't then dtbs_check warnings would just
keep growing forever.
Thanks,
Nícolas
^ permalink raw reply
* [PATCH] arm64: dts: debix-a: Remove i2c2 from base .dts
From: Laurent Pinchart @ 2024-03-25 22:50 UTC (permalink / raw)
To: devicetree, imx, linux-arm-kernel
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Jacopo Mondi, Jacopo Mondi
From: Jacopo Mondi <jacopo@jmondi.org>
The I2C2 bus is used for the CSI and DSI connectors only, no devices are
connected to it on neither the Debix Model A nor its IO board. Remove
the bus from the board's .dts and rely on display panel or camera sensor
overlsy to enable it when necessary.
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
.../boot/dts/freescale/imx8mp-debix-model-a.dts | 14 --------------
1 file changed, 14 deletions(-)
diff --git a/arch/arm64/boot/dts/freescale/imx8mp-debix-model-a.dts b/arch/arm64/boot/dts/freescale/imx8mp-debix-model-a.dts
index 5ac77eaf23d5..26c303b7c7fa 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-debix-model-a.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mp-debix-model-a.dts
@@ -210,13 +210,6 @@ ldo5: LDO5 {
};
};
-&i2c2 {
- clock-frequency = <100000>;
- pinctrl-names = "default";
- pinctrl-0 = <&pinctrl_i2c2>;
- status = "okay";
-};
-
&i2c3 {
clock-frequency = <400000>;
pinctrl-names = "default";
@@ -392,13 +385,6 @@ MX8MP_IOMUXC_I2C1_SDA__I2C1_SDA 0x400001c2
>;
};
- pinctrl_i2c2: i2c2grp {
- fsl,pins = <
- MX8MP_IOMUXC_I2C2_SCL__I2C2_SCL 0x400001c2
- MX8MP_IOMUXC_I2C2_SDA__I2C2_SDA 0x400001c2
- >;
- };
-
pinctrl_i2c3: i2c3grp {
fsl,pins = <
MX8MP_IOMUXC_I2C3_SCL__I2C3_SCL 0x400001c2
--
Regards,
Laurent Pinchart
^ permalink raw reply related
* Re: [PATCH] of: property: fw_devlink: Fix stupid bug in remote-endpoint parsing
From: Saravana Kannan @ 2024-03-25 22:49 UTC (permalink / raw)
To: John Watts
Cc: Rob Herring, Frank Rowand, Hervé Codina, Luca Ceresoli,
kernel-team, Rob Herring, devicetree, linux-kernel
In-Reply-To: <Zf7I65PiOR2wX1Uo@titan>
On Sat, Mar 23, 2024 at 5:20 AM John Watts <contact@jookia.org> wrote:
>
> Hello again,
>
> On Fri, Mar 22, 2024 at 06:53:57PM -0700, Saravana Kannan wrote:
> > Hmmm.... cycle detection should work here and not enforce probe
> > ordering. I'd appreciate help with debugging that. Let me look at it
> > on Monday. Can you enabled all the debug logs in drivers/base/core.c
> > and tell me what cycle detection is telling about these nodes?
>
> Hmm. It's not saying anything more than what I've already sent.
Sorry, I was asking for the logs. But now I'm looking at this again, I
think I understand what's going on.
> I think this is because /sound/multi isn't a device, it's just a
> subnode used in audio-graph-card2.
Ok, I think I understand now what's going on. fw_devlink does not know
that "sound" device will not populate "multi" as a child device.
Typically in such situations, "sound" would probe as a device and add
its child DT nodes devices. At that point, the cycle is only between
"multi" and "test_codec" and fw_devlink will detect that and not
enforce any ordering. However, in this case, "sound" doesn't have any
child devices and just depends on the remote endpoints directly.
We already have "ports", "in-ports" and "out-ports". Is there a reason
none of them will work for your use case and it has to be "multi"?
When you use one of those 3 recognized node names, things are handled
correctly.
Btw, between "test_codec" and "sound", which one is supposed to probe
first? I'm guessing "test_codec" needs to probe first for "sound" to
probe?
> Removing the multi { } section and using direct graph connections
> 'fixes' this.
I think the right fix is the use of post-init-providers. Because even
if you do the above, all it does is let fw_devlink see that there's a
cyclic dependency in DT. And it'll stop enforcing the probe and
suspend/resume ordering. Ideally we want to enforce a specific order
here. test_codec first and then sound.
> I think this might be because usually in a graph each node containing
> ports is a device, such as a display panel, a bridge, an LCD
> controller. These kind of form a dependency chain.
>
> In this case all the ports in multi act as a way to glue multiple
> ports together for the audio-graph-card2.
>
> Does that help?
Maybe. But the logs would be more helpful.
>
> > But the better fix would be to use the new "post-init-providers"
> > property. See below.
> >
> > >
> > > / {
> > > ...
> > >
> > >
> > > test_codec {
> > > compatible = "test-codec";
> > > prefix = "Test codec";
> > > #sound-dai-cells = <0>;
> >
> > post-init-provider = <&multi>;
Did you try this? Did it help?
-Saravana
> >
> > Right now there's a cyclic dependency between test_codec and multi and
> > this tells the kernel that test codec needs to probe first.
> >
> > Similar additions to the other nodes blocked on multi.
> >
> > Thanks,
> > Saravana
>
> John.
^ permalink raw reply
* Re: [PATCH 1/5] dt-bindings: iio: dac: ti,dac5571: Add DAC081C081 support
From: Laurent Pinchart @ 2024-03-25 22:23 UTC (permalink / raw)
To: Jonathan Cameron
Cc: devicetree, imx, linux-arm-kernel, Trevor Zaharichuk, Greg Lytle,
Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Sean Nyekjaer, linux-iio
In-Reply-To: <20240325205641.GD23988@pendragon.ideasonboard.com>
On Mon, Mar 25, 2024 at 10:56:44PM +0200, Laurent Pinchart wrote:
> On Mon, Mar 25, 2024 at 08:48:57PM +0000, Jonathan Cameron wrote:
> > On Mon, 25 Mar 2024 22:32:41 +0200 Laurent Pinchart wrote:
> >
> > > The DAC081C081 is a TI DAC whose software interface is compatible with
> > > the DAC5571. It is the 8-bit version of the DAC121C081, already
> > > supported by the DAC5571 bindings. Extends the bindings to support this
> > > chip.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > Hi Laurent,
> >
> > Given it's a part number where no one is going to guess it is compatible
> > with the DAC5571 and that we don't have a history of fallback compatibles
> > I'm fine with this change, but just wanted to ask is a fallback compatible
> > useful to you to run with older kernels?
> >
> > I should have noticed when Peter added the dac121c081. If we add a fallback
> > should do that one as well.
>
> I've indeed noticed that there should have been a fallback for
> dac121c081, but didn't stop to ponder why that wasn't the case, and just
> went along with the flow :-) I agree a fallback could be useful, which
> would then allow dropping patch 2/5 from this series (*). I can do so if
> you prefer.
And in that case, should I first introduce support in the bindings for
"ti,dac121c081", "ti,dac7571" and deprecate usage of "ti,dac121c081"
alone ?
> * This is not entirely true. While the DAC1081C081 is largely compatible
> with the DAC5573, they have different values for one of the power-down
> resistors (2.5kΩ instead of 1kΩ if I recall correctly). To be completely
> accurate, the driver should report that. We could still use the fallback
> compatible, reporting the wrong power-down resistor value.
>
> > > ---
> > > Documentation/devicetree/bindings/iio/dac/ti,dac5571.yaml | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/dac/ti,dac5571.yaml b/Documentation/devicetree/bindings/iio/dac/ti,dac5571.yaml
> > > index 79da0323c327..e59db861e2eb 100644
> > > --- a/Documentation/devicetree/bindings/iio/dac/ti,dac5571.yaml
> > > +++ b/Documentation/devicetree/bindings/iio/dac/ti,dac5571.yaml
> > > @@ -21,6 +21,7 @@ properties:
> > > - ti,dac5573
> > > - ti,dac6573
> > > - ti,dac7573
> > > + - ti,dac081c081
> > > - ti,dac121c081
> > >
> > > reg:
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 0/4] User Requested Fixes for Powkiddy RK3566 Devices
From: Chris Morgan @ 2024-03-25 22:10 UTC (permalink / raw)
To: Heiko Stübner
Cc: Chris Morgan, Dragan Simic, linux-rockchip, devicetree, conor+dt,
krzysztof.kozlowski+dt, robh
In-Reply-To: <2854897.88bMQJbFj6@diego>
On Mon, Mar 25, 2024 at 10:53:02PM +0100, Heiko Stübner wrote:
> Am Montag, 25. März 2024, 18:57:06 CET schrieb Dragan Simic:
> > Hello Chris,
> >
> > On 2024-03-25 18:51, Chris Morgan wrote:
> > > From: Chris Morgan <macromorgan@hotmail.com>
> > >
> > > Users have requested fixes for Powkiddy devices to help with some
> > > intermittent WiFi issues by adding additional properties to the
> > > SDMMC2 node. They have also requested that the model name be
> > > represented consistently with both the manufacturer name and model
> > > name. Lastly, there exists a second configuration of the RGB30 with
> > > a slightly different regulator layout we need to describe.
> > >
> > > I have added the new/additional property of "chasis-type" as well.
> >
> > Shouldn't this series be labeled as v2?
>
> I think this is for Powkiddy handhelds, the other series was
> for Anberic ones.
>
> Somehow they design and build seemingly dozens of somewhat
> similar handhelds around the rk3566 ;-)
Yes they do, and unfortunately in the case of some of the devices they
love to make changes and not tell anyone (like the regulator change for
the RGB30) In regards to that specific change, let me know if it's not
appropriate to put that in the devicetree the way I did. Ideally I'll
do a fixup of sorts with U-Boot, but it might be easier if I have a
disabled node to work with already...
Otherwise these fixes try to resolve some wifi issues for all the
devices along with fixing the inconsistent names. I have some additional
fixes (coming when I can get them finished and tested) for the Powkiddy
devices as some of the screens have slightly shifted pixels.
Thank you.
>
>
> > By the way, regarding renaming the mmcX aliases, maybe that would
> > actually be doable, but only if you have full control over software
> > that runs on these devices.
> >
> > > Chris Morgan (4):
> > > dts: rockchip: Add chasis-type for Powkiddy rk3566 devices
> > > arm64: dts: rockchip: Update sdmmc node for wifi on powkiddy rk3566
> > > arm64: dts: rockchip: Correct model name for Powkiddy RK3566 Devices
> > > arm64: dts: rockchip: Describe Alternate Regulator Config on RGB30
> > >
> > > .../dts/rockchip/rk3566-powkiddy-rgb30.dts | 30 ++++++++++++++++++-
> > > .../dts/rockchip/rk3566-powkiddy-rk2023.dts | 6 +++-
> > > .../dts/rockchip/rk3566-powkiddy-rk2023.dtsi | 4 +++
> > > .../boot/dts/rockchip/rk3566-powkiddy-x55.dts | 4 +++
> > > 4 files changed, 42 insertions(+), 2 deletions(-)
> >
>
>
>
>
^ permalink raw reply
* Re: [PATCH v4 4/7] dt-bindings: iio: accel: adxl345: Add spi-3wire
From: Lothar Rubusch @ 2024-03-25 22:09 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt, linux-iio, devicetree, linux-kernel, eraretuya
In-Reply-To: <334970e7-2edd-43c8-9f18-b7b3ec5f4d17@linaro.org>
On Mon, Mar 25, 2024 at 10:40 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 25/03/2024 22:05, Lothar Rubusch wrote:
> > On Mon, Mar 25, 2024 at 7:32 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 25/03/2024 16:33, Lothar Rubusch wrote:
> >>> Add spi-3wire because the driver optionally supports spi-3wire.
> >>
> >> This is a friendly reminder during the review process.
> >>
> >> It seems my or other reviewer's previous comments were not fully
> >> addressed. Maybe the feedback got lost between the quotes, maybe you
> >> just forgot to apply it. Please go back to the previous discussion and
> >> either implement all requested changes or keep discussing them.
> >>
> >> Thank you.
> >>
> >
> > You refer yourself to the above mentioned wording. Would replacing
> > "driver" by "device" in the dt-bindings patch comment be sufficient?
> > Did I miss something else?
>
> Yes, the wording, but isn't the device require 3-wire mode? Don't just
> replace one word with another, but write the proper rationale for your
> hardware.
>
It does not require 3-wire SPI. By default the device communicates
regular SPI. It can be configured, though, to communicate 3-wire. The
given patch offers this as option in the DT.
> >
> >>>
> >>> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> >>> ---
> >>
> >> This is a friendly reminder during the review process.
> >>
> >> It looks like you received a tag and forgot to add it.
> >>
> >> If you do not know the process, here is a short explanation:
> >> Please add Acked-by/Reviewed-by/Tested-by tags when posting new
> >> versions, under or above your Signed-off-by tag. Tag is "received", when
> >> provided in a message replied to you on the mailing list. Tools like b4
> >> can help here. However, there's no need to repost patches *only* to add
> >
> > Just for confirmation: when I receive a feedback, requesting a change.
> > And, I accept the change request. This means, I received a tag
> > "Reviewed-by" which I have to mention in the upcoming patch version
> > where this change is implemented and in that particular patch?
>
> Please go through the docs. Yes, you received a tag which should be
> included with the change.
>
> Reviewer's feedback should not be ignored.
>
>
> >
> >> the tags. The upstream maintainer will do that for tags received on the
> >> version they apply.
> >>
> >
> > I'm pretty sure we will still see further iterations. So, I apply the
> > tags in the next version, already scheduled. Ok?
> >
> >> https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
> >>
> >
> > Going over the books I feel it does not make sense to still mention
> > feedback ("Reveiewed-by") for the v1 or v2 of the patch here in a v5,
> > does it? Your link mentiones "However if the patch has changed
>
> I don't understand. When did you receive the tag? v3, right? So what do
> you mean by v1 and v2?
>
V1: The first version of the 3wire patch. I have split the single
patch upon some feedback (yours?!) - V2... So, my current
interpretation is, that every feedback I need to mention as
Reviewed-by tag, no?
>
> Best regards,
> Krzysztof
>
^ permalink raw reply
* Re: [PATCH V8 00/12] soc: imx8mp: Add support for HDMI
From: Laurent Pinchart @ 2024-03-25 22:03 UTC (permalink / raw)
To: Tommaso Merciai
Cc: Adam Ford, linux-arm-kernel, marex, alexander.stein,
frieder.schrempf, Andrzej Hajda, Neil Armstrong, Robert Foss,
Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
Philipp Zabel, Vinod Koul, Kishon Vijay Abraham I,
Catalin Marinas, Will Deacon, Liu Ying, Ulf Hansson, Lucas Stach,
dri-devel, devicetree, linux-kernel, linux-phy, linux-pm
In-Reply-To: <ZgHxSHDAt7ytqDC1@tom-HP-ZBook-Fury-15-G7-Mobile-Workstation>
Hi Tommaso,
On Mon, Mar 25, 2024 at 10:48:56PM +0100, Tommaso Merciai wrote:
> Hi Adam, Lucas,
> Thanks for this series.
>
> This series make HDMI work on evk.
> All is working properly on my side.
>
> Tested on: Linux imx8mp-lpddr4-evk 6.9.0-rc1.
> Hope this help.
>
> Tested-by: Tommaso Merciai <tomm.merciai@gmail.com>
The DRM side has been merged already. The only missing patches are for
the PHY, and the latest version can be found in
https://lore.kernel.org/linux-phy/20240227220444.77566-1-aford173@gmail.com/.
You can test that series and send a Tested-by tag. I'm crossing my
fingers and hoping it will be merged in v6.10.
> On Sat, Feb 03, 2024 at 10:52:40AM -0600, Adam Ford wrote:
> > The i.MX8M Plus has an HDMI controller, but it depends on two
> > other systems, the Parallel Video Interface (PVI) and the
> > HDMI PHY from Samsung. The LCDIF controller generates the display
> > and routes it to the PVI which converts passes the parallel video
> > to the HDMI bridge. The HDMI system has a corresponding power
> > domain controller whose driver was partially written, but the
> > device tree for it was never applied, so some changes to the
> > power domain should be harmless because they've not really been
> > used yet.
> >
> > This series is adapted from multiple series from Lucas Stach with
> > edits and suggestions from feedback from various series, but it
> > since it's difficult to use and test them independently,
> > I merged them into on unified series. The version history is a
> > bit ambiguous since different components were submitted at different
> > times and had different amount of retries. In an effort to merge them
> > I used the highest version attempt.
> >
> > Adam Ford (3):
> > dt-bindings: soc: imx: add missing clock and power-domains to
> > imx8mp-hdmi-blk-ctrl
> > pmdomain: imx8mp-blk-ctrl: imx8mp_blk: Add fdcc clock to hdmimix
> > domain
> > arm64: defconfig: Enable DRM_IMX8MP_DW_HDMI_BRIDGE as module
> >
> > Lucas Stach (9):
> > dt-bindings: phy: add binding for the i.MX8MP HDMI PHY
> > phy: freescale: add Samsung HDMI PHY
> > arm64: dts: imx8mp: add HDMI power-domains
> > arm64: dts: imx8mp: add HDMI irqsteer
> > dt-bindings: display: imx: add binding for i.MX8MP HDMI PVI
> > drm/bridge: imx: add driver for HDMI TX Parallel Video Interface
> > dt-bindings: display: imx: add binding for i.MX8MP HDMI TX
> > drm/bridge: imx: add bridge wrapper driver for i.MX8MP DWC HDMI
> > arm64: dts: imx8mp: add HDMI display pipeline
> >
> > .../display/bridge/fsl,imx8mp-hdmi-tx.yaml | 102 ++
> > .../display/imx/fsl,imx8mp-hdmi-pvi.yaml | 84 ++
> > .../bindings/phy/fsl,imx8mp-hdmi-phy.yaml | 62 +
> > .../soc/imx/fsl,imx8mp-hdmi-blk-ctrl.yaml | 22 +-
> > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 145 +++
> > arch/arm64/configs/defconfig | 1 +
> > drivers/gpu/drm/bridge/imx/Kconfig | 18 +
> > drivers/gpu/drm/bridge/imx/Makefile | 2 +
> > drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c | 207 ++++
> > drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c | 154 +++
> > drivers/phy/freescale/Kconfig | 6 +
> > drivers/phy/freescale/Makefile | 1 +
> > drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 1075 +++++++++++++++++
> > drivers/pmdomain/imx/imx8mp-blk-ctrl.c | 10 +-
> > 14 files changed, 1876 insertions(+), 13 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/display/bridge/fsl,imx8mp-hdmi-tx.yaml
> > create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi-pvi.yaml
> > create mode 100644 Documentation/devicetree/bindings/phy/fsl,imx8mp-hdmi-phy.yaml
> > create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c
> > create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> > create mode 100644 drivers/phy/freescale/phy-fsl-samsung-hdmi.c
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v3 3/4] dt-bindings: rng: Add vmgenid support
From: Krzysztof Kozlowski @ 2024-03-25 21:59 UTC (permalink / raw)
To: Sudan Landge, tytso, Jason, robh+dt, krzysztof.kozlowski+dt,
conor+dt, sathyanarayanan.kuppuswamy, thomas.lendacky,
dan.j.williams, devicetree, linux-kernel
Cc: graf, dwmw, bchalios, xmarcalx
In-Reply-To: <20240325195306.13133-4-sudanl@amazon.com>
On 25/03/2024 20:53, Sudan Landge wrote:
> Virtual Machine Generation ID driver was introduced in commit af6b54e2b5ba
> ("virt: vmgenid: notify RNG of VM fork and supply generation ID"), as an
> ACPI only device.
...
> diff --git a/Documentation/devicetree/bindings/rng/vmgenid.yaml b/Documentation/devicetree/bindings/rng/vmgenid.yaml
> new file mode 100644
> index 000000000000..24643080d6b0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rng/vmgenid.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rng/vmgenid.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Virtual Machine Generation Counter ID device
> +
> +maintainers:
> + - Jason A. Donenfeld <Jason@zx2c4.com>
> +
> +description:
> + Firmwares or hypervisors can use this devicetree to describe
> + interrupts and the shared resources to inject a Virtual Machine Generation
> + counter.
> +
> +properties:
> + compatible:
> + const: virtual,vmgenctr
> +
> + "#interrupt-cells":
> + const: 3
> + description:
> + The 1st cell is the interrupt type.
> + The 2nd cell contains the interrupt number for the interrupt type.
> + The 3rd cell is for trigger type and level flags.
How is this interrupt-controller now? You already got such comment on v2.
> +
> + interrupt-map: true
> +
> + reg:
> + description:
> + The 1st cell specifies the base physical address of the 8-byte aligned
> + buffer in guest memory space which is guaranteed not to be used by the
> + operating system.
> + The 2nd cell specifies the size of the buffer which holds the VMGenID.
> + maxItems: 1
> +
> + interrupts:
> + description:
> + interrupt used to notify that a new VMGenID counter is available.
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + rng@80000000 {
> + compatible = "virtual,vmgenctr";
> + reg = <0x80000000 0x1000>;
> + interrupts = <0x00 0x23 0x01>;
Use standard defines and drop padding from hex numbers. Or just use
decimal numbers because hex is anyway unusual, unless this is what your
device datasheet says.
And what is 0x00 in the interrupt?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 0/4] User Requested Fixes for Powkiddy RK3566 Devices
From: Dragan Simic @ 2024-03-25 21:58 UTC (permalink / raw)
To: Heiko Stübner
Cc: Chris Morgan, linux-rockchip, devicetree, conor+dt,
krzysztof.kozlowski+dt, robh, Chris Morgan
In-Reply-To: <2854897.88bMQJbFj6@diego>
Hello Heiko,
On 2024-03-25 22:53, Heiko Stübner wrote:
> Am Montag, 25. März 2024, 18:57:06 CET schrieb Dragan Simic:
>> Hello Chris,
>>
>> On 2024-03-25 18:51, Chris Morgan wrote:
>> > From: Chris Morgan <macromorgan@hotmail.com>
>> >
>> > Users have requested fixes for Powkiddy devices to help with some
>> > intermittent WiFi issues by adding additional properties to the
>> > SDMMC2 node. They have also requested that the model name be
>> > represented consistently with both the manufacturer name and model
>> > name. Lastly, there exists a second configuration of the RGB30 with
>> > a slightly different regulator layout we need to describe.
>> >
>> > I have added the new/additional property of "chasis-type" as well.
>>
>> Shouldn't this series be labeled as v2?
>
> I think this is for Powkiddy handhelds, the other series was
> for Anberic ones.
>
> Somehow they design and build seemingly dozens of somewhat
> similar handhelds around the rk3566 ;-)
Ah, you're right, I was wrong. It's quite easy to become confused
a bit with such similar devices and similar patches. :)
>> By the way, regarding renaming the mmcX aliases, maybe that would
>> actually be doable, but only if you have full control over software
>> that runs on these devices.
>>
>> > Chris Morgan (4):
>> > dts: rockchip: Add chasis-type for Powkiddy rk3566 devices
>> > arm64: dts: rockchip: Update sdmmc node for wifi on powkiddy rk3566
>> > arm64: dts: rockchip: Correct model name for Powkiddy RK3566 Devices
>> > arm64: dts: rockchip: Describe Alternate Regulator Config on RGB30
>> >
>> > .../dts/rockchip/rk3566-powkiddy-rgb30.dts | 30 ++++++++++++++++++-
>> > .../dts/rockchip/rk3566-powkiddy-rk2023.dts | 6 +++-
>> > .../dts/rockchip/rk3566-powkiddy-rk2023.dtsi | 4 +++
>> > .../boot/dts/rockchip/rk3566-powkiddy-x55.dts | 4 +++
>> > 4 files changed, 42 insertions(+), 2 deletions(-)
>>
>
>
>
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply
* Re: [PATCH v3 2/4] virt: vmgenid: change implementation to use a platform driver
From: Krzysztof Kozlowski @ 2024-03-25 21:54 UTC (permalink / raw)
To: Sudan Landge, tytso, Jason, robh+dt, krzysztof.kozlowski+dt,
conor+dt, sathyanarayanan.kuppuswamy, thomas.lendacky,
dan.j.williams, devicetree, linux-kernel
Cc: graf, dwmw, bchalios, xmarcalx
In-Reply-To: <20240325195306.13133-3-sudanl@amazon.com>
On 25/03/2024 20:53, Sudan Landge wrote:
> Re-implement vmgenid as a platform driver in preparation
> for adding devicetree bindings support in next commits.
>
> Signed-off-by: Sudan Landge <sudanl@amazon.com>
> ---
> drivers/virt/vmgenid.c | 101 ++++++++++++++++++++++++++++++-----------
> 1 file changed, 74 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c
> index ea956df02874..d5394c706bd9 100644
> --- a/drivers/virt/vmgenid.c
> +++ b/drivers/virt/vmgenid.c
> @@ -11,6 +11,8 @@
> #include <linux/module.h>
> #include <linux/acpi.h>
> #include <linux/random.h>
> +#include <acpi/actypes.h>
> +#include <linux/platform_device.h>
Do not add headers to the end of lists.
>
> ACPI_MODULE_NAME("vmgenid");
>
...
>
> -static struct acpi_driver vmgenid_driver = {
> - .name = "vmgenid",
> - .ids = vmgenid_ids,
> - .owner = THIS_MODULE,
> - .ops = {
> - .add = vmgenid_add,
> - .notify = vmgenid_notify
> - }
> +static struct platform_driver vmgenid_plaform_driver = {
> + .probe = vmgenid_add,
> + .driver = {
> + .name = "vmgenid",
> + .acpi_match_table = ACPI_PTR(vmgenid_ids),
ACPI_PTR does not make sense. If this can be compile tested !ACPI, then
this is wrong. If this cannot be compile tested, then this is redundant
and confusing so... also wrong.
> + .owner = THIS_MODULE,
> + },
> };
>
> -module_acpi_driver(vmgenid_driver);
> +static int vmgenid_platform_device_init(void)
> +{
> + return platform_driver_register(&vmgenid_plaform_driver);
> +}
> +
> +static void vmgenid_platform_device_exit(void)
> +{
> + platform_driver_unregister(&vmgenid_plaform_driver);
> +}
> +
> +module_init(vmgenid_platform_device_init)
> +module_exit(vmgenid_platform_device_exit)
Why this cannot be module_platform_driver?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 0/4] User Requested Fixes for Powkiddy RK3566 Devices
From: Heiko Stübner @ 2024-03-25 21:53 UTC (permalink / raw)
To: Chris Morgan, Dragan Simic
Cc: linux-rockchip, devicetree, conor+dt, krzysztof.kozlowski+dt,
robh, Chris Morgan
In-Reply-To: <5e2f0da3fc066cf1faddda71bb6ea2e7@manjaro.org>
Am Montag, 25. März 2024, 18:57:06 CET schrieb Dragan Simic:
> Hello Chris,
>
> On 2024-03-25 18:51, Chris Morgan wrote:
> > From: Chris Morgan <macromorgan@hotmail.com>
> >
> > Users have requested fixes for Powkiddy devices to help with some
> > intermittent WiFi issues by adding additional properties to the
> > SDMMC2 node. They have also requested that the model name be
> > represented consistently with both the manufacturer name and model
> > name. Lastly, there exists a second configuration of the RGB30 with
> > a slightly different regulator layout we need to describe.
> >
> > I have added the new/additional property of "chasis-type" as well.
>
> Shouldn't this series be labeled as v2?
I think this is for Powkiddy handhelds, the other series was
for Anberic ones.
Somehow they design and build seemingly dozens of somewhat
similar handhelds around the rk3566 ;-)
> By the way, regarding renaming the mmcX aliases, maybe that would
> actually be doable, but only if you have full control over software
> that runs on these devices.
>
> > Chris Morgan (4):
> > dts: rockchip: Add chasis-type for Powkiddy rk3566 devices
> > arm64: dts: rockchip: Update sdmmc node for wifi on powkiddy rk3566
> > arm64: dts: rockchip: Correct model name for Powkiddy RK3566 Devices
> > arm64: dts: rockchip: Describe Alternate Regulator Config on RGB30
> >
> > .../dts/rockchip/rk3566-powkiddy-rgb30.dts | 30 ++++++++++++++++++-
> > .../dts/rockchip/rk3566-powkiddy-rk2023.dts | 6 +++-
> > .../dts/rockchip/rk3566-powkiddy-rk2023.dtsi | 4 +++
> > .../boot/dts/rockchip/rk3566-powkiddy-x55.dts | 4 +++
> > 4 files changed, 42 insertions(+), 2 deletions(-)
>
^ permalink raw reply
* Re: [PATCH v3 4/4] virt: vmgenid: add support for devicetree bindings
From: Krzysztof Kozlowski @ 2024-03-25 21:51 UTC (permalink / raw)
To: Sudan Landge, tytso, Jason, robh+dt, krzysztof.kozlowski+dt,
conor+dt, sathyanarayanan.kuppuswamy, thomas.lendacky,
dan.j.williams, devicetree, linux-kernel
Cc: graf, dwmw, bchalios, xmarcalx
In-Reply-To: <20240325195306.13133-5-sudanl@amazon.com>
On 25/03/2024 20:53, Sudan Landge wrote:
> - Extend the vmgenid platform driver to support devicetree bindings.
> With this support, hypervisors can send vmgenid notifications to
> the virtual machine without the need to enable ACPI. The bindings
> are located at: Documentation/devicetree/bindings/rng/vmgenid.yaml
>
> - Use proper FLAGS to compile with and without ACPI and/or devicetree.
I do not see any flags. You use if/ifdefs which is a NAK.
Do not mix independent changes within one commit. Please follow
guidelines in submitting-patches for this.
>
> Signed-off-by: Sudan Landge <sudanl@amazon.com>
> ---
> drivers/virt/Kconfig | 1 -
> drivers/virt/vmgenid.c | 85 +++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 80 insertions(+), 6 deletions(-)
>
...
> +
> +#if IS_ENABLED(CONFIG_OF)
> +static irqreturn_t vmgenid_of_irq_handler(int irq, void *dev)
> +{
> + (void)irq;
> + vmgenid_notify(dev);
> +
> + return IRQ_HANDLED;
> +}
> +#endif
>
> static int setup_vmgenid_state(struct vmgenid_state *state, u8 *next_id)
> {
> @@ -55,6 +70,7 @@ static int setup_vmgenid_state(struct vmgenid_state *state, u8 *next_id)
>
> static int vmgenid_add_acpi(struct device *dev, struct vmgenid_state *state)
> {
> +#if IS_ENABLED(CONFIG_ACPI)
Why do you create all this ifdeffery in the code? You already got
comments to get rid of all this #if.
> struct acpi_device *device = ACPI_COMPANION(dev);
> struct acpi_buffer parsed = { ACPI_ALLOCATE_BUFFER };
> union acpi_object *obj;
> @@ -96,6 +112,49 @@ static int vmgenid_add_acpi(struct device *dev, struct vmgenid_state *state)
> out:
> ACPI_FREE(parsed.pointer);
> return ret;
> +#else
> + (void)dev;
> + (void)state;
> + return -EINVAL;
> +#endif
> +}
> +
> +static int vmgenid_add_of(struct platform_device *pdev, struct vmgenid_state *state)
> +{
> +#if IS_ENABLED(CONFIG_OF)
> + void __iomem *remapped_ptr;
> + int ret = 0;
> +
> + remapped_ptr = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
> + if (IS_ERR(remapped_ptr)) {
> + ret = PTR_ERR(remapped_ptr);
> + goto out;
> + }
> +
> + ret = setup_vmgenid_state(state, remapped_ptr);
> + if (ret)
> + goto out;
> +
> + state->irq = platform_get_irq(pdev, 0);
> + if (state->irq < 0) {
> + ret = state->irq;
> + goto out;
> + }
> + pdev->dev.driver_data = state;
> +
> + ret = devm_request_irq(&pdev->dev, state->irq,
> + vmgenid_of_irq_handler,
> + IRQF_SHARED, "vmgenid", &pdev->dev);
> + if (ret)
> + pdev->dev.driver_data = NULL;
> +
> +out:
> + return ret;
> +#else
That's neither readable code nor matching Linux coding style. Driver
don't do such magic. Please open some recent drivers for ACPI and OF and
look there. Old drivers had stubs for !CONFIG_XXX, but new drivers don't
have even that.
> + (void)dev;
> + (void)state;
> + return -EINVAL;
> +#endif
> }
>
> static int vmgenid_add(struct platform_device *pdev)
> @@ -108,7 +167,10 @@ static int vmgenid_add(struct platform_device *pdev)
> if (!state)
> return -ENOMEM;
>
> - ret = vmgenid_add_acpi(dev, state);
> + if (dev->of_node)
> + ret = vmgenid_add_of(pdev, state);
> + else
> + ret = vmgenid_add_acpi(dev, state);
>
> if (ret)
> devm_kfree(dev, state);
> @@ -116,18 +178,31 @@ static int vmgenid_add(struct platform_device *pdev)
> return ret;
> }
>
> -static const struct acpi_device_id vmgenid_ids[] = {
> +#if IS_ENABLED(CONFIG_OF)
No, drop.
> +static const struct of_device_id vmgenid_of_ids[] = {
> + { .compatible = "linux,vmgenctr", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, vmgenid_of_ids);
> +#endif
> +
> +#if IS_ENABLED(CONFIG_ACPI)
> +static const struct acpi_device_id vmgenid_acpi_ids[] = {
> { "VMGENCTR", 0 },
> { "VM_GEN_COUNTER", 0 },
> { }
> };
> -MODULE_DEVICE_TABLE(acpi, vmgenid_ids);
> +MODULE_DEVICE_TABLE(acpi, vmgenid_acpi_ids);
> +#endif
>
> static struct platform_driver vmgenid_plaform_driver = {
> .probe = vmgenid_add,
> .driver = {
> .name = "vmgenid",
> - .acpi_match_table = ACPI_PTR(vmgenid_ids),
> + .acpi_match_table = ACPI_PTR(vmgenid_acpi_ids),
> +#if IS_ENABLED(CONFIG_OF)
No, really, this is some ancient code.
Please point me to single driver doing such ifdef.
> + .of_match_table = vmgenid_of_ids,
> +#endif
> .owner = THIS_MODULE,
This is clearly some abandoned driver... sigh... I thought we get rid of
all this owner crap. Many years ago. How could it appear back if
automated tools report it?
Considering how many failures LKP reported for your patchsets, I have
real doubts that anyone actually tests this code.
Please confirm:
Did you build W=1, run smatch, sparse and coccinelle on the driver after
your changes?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH V8 00/12] soc: imx8mp: Add support for HDMI
From: Tommaso Merciai @ 2024-03-25 21:48 UTC (permalink / raw)
To: Adam Ford
Cc: linux-arm-kernel, marex, alexander.stein, frieder.schrempf,
Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
Philipp Zabel, Vinod Koul, Kishon Vijay Abraham I,
Catalin Marinas, Will Deacon, Liu Ying, Ulf Hansson, Lucas Stach,
dri-devel, devicetree, linux-kernel, linux-phy, linux-pm
In-Reply-To: <20240203165307.7806-1-aford173@gmail.com>
Hi Adam, Lucas,
Thanks for this series.
This series make HDMI work on evk.
All is working properly on my side.
Tested on: Linux imx8mp-lpddr4-evk 6.9.0-rc1.
Hope this help.
Tested-by: Tommaso Merciai <tomm.merciai@gmail.com>
Thanks & Regards,
Tommaso
On Sat, Feb 03, 2024 at 10:52:40AM -0600, Adam Ford wrote:
> The i.MX8M Plus has an HDMI controller, but it depends on two
> other systems, the Parallel Video Interface (PVI) and the
> HDMI PHY from Samsung. The LCDIF controller generates the display
> and routes it to the PVI which converts passes the parallel video
> to the HDMI bridge. The HDMI system has a corresponding power
> domain controller whose driver was partially written, but the
> device tree for it was never applied, so some changes to the
> power domain should be harmless because they've not really been
> used yet.
>
> This series is adapted from multiple series from Lucas Stach with
> edits and suggestions from feedback from various series, but it
> since it's difficult to use and test them independently,
> I merged them into on unified series. The version history is a
> bit ambiguous since different components were submitted at different
> times and had different amount of retries. In an effort to merge them
> I used the highest version attempt.
>
> Adam Ford (3):
> dt-bindings: soc: imx: add missing clock and power-domains to
> imx8mp-hdmi-blk-ctrl
> pmdomain: imx8mp-blk-ctrl: imx8mp_blk: Add fdcc clock to hdmimix
> domain
> arm64: defconfig: Enable DRM_IMX8MP_DW_HDMI_BRIDGE as module
>
> Lucas Stach (9):
> dt-bindings: phy: add binding for the i.MX8MP HDMI PHY
> phy: freescale: add Samsung HDMI PHY
> arm64: dts: imx8mp: add HDMI power-domains
> arm64: dts: imx8mp: add HDMI irqsteer
> dt-bindings: display: imx: add binding for i.MX8MP HDMI PVI
> drm/bridge: imx: add driver for HDMI TX Parallel Video Interface
> dt-bindings: display: imx: add binding for i.MX8MP HDMI TX
> drm/bridge: imx: add bridge wrapper driver for i.MX8MP DWC HDMI
> arm64: dts: imx8mp: add HDMI display pipeline
>
> .../display/bridge/fsl,imx8mp-hdmi-tx.yaml | 102 ++
> .../display/imx/fsl,imx8mp-hdmi-pvi.yaml | 84 ++
> .../bindings/phy/fsl,imx8mp-hdmi-phy.yaml | 62 +
> .../soc/imx/fsl,imx8mp-hdmi-blk-ctrl.yaml | 22 +-
> arch/arm64/boot/dts/freescale/imx8mp.dtsi | 145 +++
> arch/arm64/configs/defconfig | 1 +
> drivers/gpu/drm/bridge/imx/Kconfig | 18 +
> drivers/gpu/drm/bridge/imx/Makefile | 2 +
> drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c | 207 ++++
> drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c | 154 +++
> drivers/phy/freescale/Kconfig | 6 +
> drivers/phy/freescale/Makefile | 1 +
> drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 1075 +++++++++++++++++
> drivers/pmdomain/imx/imx8mp-blk-ctrl.c | 10 +-
> 14 files changed, 1876 insertions(+), 13 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/display/bridge/fsl,imx8mp-hdmi-tx.yaml
> create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi-pvi.yaml
> create mode 100644 Documentation/devicetree/bindings/phy/fsl,imx8mp-hdmi-phy.yaml
> create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c
> create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> create mode 100644 drivers/phy/freescale/phy-fsl-samsung-hdmi.c
>
> --
> 2.43.0
>
>
^ permalink raw reply
* [PATCH 1/3] dt-bindings: hsi: hsi-client: convert to YAML
From: Sebastian Reichel @ 2024-03-25 21:45 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sebastian Reichel
Cc: Tony Lindgren, devicetree, linux-omap, linux-kernel,
Sebastian Reichel
In-Reply-To: <20240325-hsi-dt-binding-v1-0-88e8e97c3aae@collabora.com>
Convert the legacy txt binding to modern YAML and rename from
client-devices to hsi-client. No semantic change.
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
.../devicetree/bindings/hsi/client-devices.txt | 44 ------------
.../devicetree/bindings/hsi/hsi-client.yaml | 84 ++++++++++++++++++++++
2 files changed, 84 insertions(+), 44 deletions(-)
diff --git a/Documentation/devicetree/bindings/hsi/client-devices.txt b/Documentation/devicetree/bindings/hsi/client-devices.txt
deleted file mode 100644
index 104c9a3e57a4..000000000000
--- a/Documentation/devicetree/bindings/hsi/client-devices.txt
+++ /dev/null
@@ -1,44 +0,0 @@
-Each HSI port is supposed to have one child node, which
-symbols the remote device connected to the HSI port. The
-following properties are standardized for HSI clients:
-
-Required HSI configuration properties:
-
-- hsi-channel-ids: A list of channel ids
-
-- hsi-rx-mode: Receiver Bit transmission mode ("stream" or "frame")
-- hsi-tx-mode: Transmitter Bit transmission mode ("stream" or "frame")
-- hsi-mode: May be used instead hsi-rx-mode and hsi-tx-mode if
- the transmission mode is the same for receiver and
- transmitter
-- hsi-speed-kbps: Max bit transmission speed in kbit/s
-- hsi-flow: RX flow type ("synchronized" or "pipeline")
-- hsi-arb-mode: Arbitration mode for TX frame ("round-robin", "priority")
-
-Optional HSI configuration properties:
-
-- hsi-channel-names: A list with one name per channel specified in the
- hsi-channel-ids property
-
-
-Device Tree node example for an HSI client:
-
-hsi-controller {
- hsi-port {
- modem: hsi-client {
- compatible = "nokia,n900-modem";
-
- hsi-channel-ids = <0>, <1>, <2>, <3>;
- hsi-channel-names = "mcsaab-control",
- "speech-control",
- "speech-data",
- "mcsaab-data";
- hsi-speed-kbps = <55000>;
- hsi-mode = "frame";
- hsi-flow = "synchronized";
- hsi-arb-mode = "round-robin";
-
- /* more client specific properties */
- };
- };
-};
diff --git a/Documentation/devicetree/bindings/hsi/hsi-client.yaml b/Documentation/devicetree/bindings/hsi/hsi-client.yaml
new file mode 100644
index 000000000000..df6e1fdd2702
--- /dev/null
+++ b/Documentation/devicetree/bindings/hsi/hsi-client.yaml
@@ -0,0 +1,84 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hsi/hsi-client.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HSI bus peripheral
+
+description:
+ Each HSI port is supposed to have one child node, which
+ symbols the remote device connected to the HSI port.
+
+maintainers:
+ - Sebastian Reichel <sre@kernel.org>
+
+properties:
+ $nodename:
+ const: hsi-client
+
+ hsi-channel-ids:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ minItems: 1
+ maxItems: 8
+
+ hsi-channel-names:
+ minItems: 1
+ maxItems: 8
+
+ hsi-rx-mode:
+ enum: [stream, frame]
+ description: Receiver Bit transmission mode
+
+ hsi-tx-mode:
+ enum: [stream, frame]
+ description: Transmitter Bit transmission mode
+
+ hsi-mode:
+ enum: [stream, frame]
+ description:
+ May be used instead hsi-rx-mode and hsi-tx-mode if the
+ transmission mode is the same for receiver and transmitter.
+
+ hsi-speed-kbps:
+ description: Max bit transmission speed in kbit/s
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ hsi-flow:
+ enum: [synchronized, pipeline]
+ description: RX flow type
+
+ hsi-arb-mode:
+ enum: [round-robin, priority]
+ description: Arbitration mode for TX frame
+
+additionalProperties: true
+
+required:
+ - compatible
+ - hsi-channel-ids
+ - hsi-speed-kbps
+ - hsi-flow
+ - hsi-arb-mode
+
+anyOf:
+ - required:
+ - hsi-mode
+ - required:
+ - hsi-rx-mode
+ - hsi-tx-mode
+
+allOf:
+ - if:
+ required:
+ - hsi-mode
+ then:
+ properties:
+ hsi-rx-mode: false
+ hsi-tx-mode: false
+ - if:
+ required:
+ - hsi-rx-mode
+ then:
+ properties:
+ hsi-mode: false
--
2.43.0
^ permalink raw reply related
* [PATCH 2/3] dt-bindings: hsi: nokia-modem: convert to YAML
From: Sebastian Reichel @ 2024-03-25 21:45 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sebastian Reichel
Cc: Tony Lindgren, devicetree, linux-omap, linux-kernel,
Sebastian Reichel
In-Reply-To: <20240325-hsi-dt-binding-v1-0-88e8e97c3aae@collabora.com>
Convert the legacy txt binding to modern YAML.
No semantic change.
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
.../devicetree/bindings/hsi/nokia-modem.txt | 59 ------------
.../devicetree/bindings/hsi/nokia-modem.yaml | 101 +++++++++++++++++++++
2 files changed, 101 insertions(+), 59 deletions(-)
diff --git a/Documentation/devicetree/bindings/hsi/nokia-modem.txt b/Documentation/devicetree/bindings/hsi/nokia-modem.txt
deleted file mode 100644
index 53de1d9d0b95..000000000000
--- a/Documentation/devicetree/bindings/hsi/nokia-modem.txt
+++ /dev/null
@@ -1,59 +0,0 @@
-Nokia modem client bindings
-
-The Nokia modem HSI client follows the common HSI client binding
-and inherits all required properties. The following additional
-properties are needed by the Nokia modem HSI client:
-
-Required properties:
-- compatible: Should be one of
- "nokia,n900-modem"
- "nokia,n950-modem"
- "nokia,n9-modem"
-- hsi-channel-names: Should contain the following strings
- "mcsaab-control"
- "speech-control"
- "speech-data"
- "mcsaab-data"
-- gpios: Should provide a GPIO handler for each GPIO listed in
- gpio-names
-- gpio-names: Should contain the following strings
- "cmt_apeslpx" (for n900, n950, n9)
- "cmt_rst_rq" (for n900, n950, n9)
- "cmt_en" (for n900, n950, n9)
- "cmt_rst" (for n900)
- "cmt_bsi" (for n900)
-- interrupts: Should be IRQ handle for modem's reset indication
-
-Example:
-
-&ssi_port {
- modem: hsi-client {
- compatible = "nokia,n900-modem";
-
- pinctrl-names = "default";
- pinctrl-0 = <&modem_pins>;
-
- hsi-channel-ids = <0>, <1>, <2>, <3>;
- hsi-channel-names = "mcsaab-control",
- "speech-control",
- "speech-data",
- "mcsaab-data";
- hsi-speed-kbps = <55000>;
- hsi-mode = "frame";
- hsi-flow = "synchronized";
- hsi-arb-mode = "round-robin";
-
- interrupts-extended = <&gpio3 8 IRQ_TYPE_EDGE_FALLING>; /* 72 */
-
- gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>, /* 70 */
- <&gpio3 9 GPIO_ACTIVE_HIGH>, /* 73 */
- <&gpio3 10 GPIO_ACTIVE_HIGH>, /* 74 */
- <&gpio3 11 GPIO_ACTIVE_HIGH>, /* 75 */
- <&gpio5 29 GPIO_ACTIVE_HIGH>; /* 157 */
- gpio-names = "cmt_apeslpx",
- "cmt_rst_rq",
- "cmt_en",
- "cmt_rst",
- "cmt_bsi";
- };
-};
diff --git a/Documentation/devicetree/bindings/hsi/nokia-modem.yaml b/Documentation/devicetree/bindings/hsi/nokia-modem.yaml
new file mode 100644
index 000000000000..c57cbcc7b722
--- /dev/null
+++ b/Documentation/devicetree/bindings/hsi/nokia-modem.yaml
@@ -0,0 +1,101 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hsi/nokia-modem.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nokia modem
+
+maintainers:
+ - Sebastian Reichel <sre@kernel.org>
+
+properties:
+ compatible:
+ enum:
+ - nokia,n900-modem
+ - nokia,n950-modem
+ - nokia,n9-modem
+
+ hsi-channel-names:
+ items:
+ - const: mcsaab-control
+ - const: speech-control
+ - const: speech-data
+ - const: mcsaab-data
+
+ interrupts:
+ items:
+ - description: modem reset indication
+
+ gpios:
+ minItems: 3
+ maxItems: 5
+
+ gpio-names:
+ items:
+ - const: cmt_apeslpx
+ - const: cmt_rst_rq
+ - const: cmt_en
+ - const: cmt_rst
+ - const: cmt_bsi
+ minItems: 3
+
+required:
+ - gpios
+ - gpio-names
+ - interrupts
+
+allOf:
+ - $ref: hsi-client.yaml#
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - nokia,n950-modem
+ - nokia,n9-modem
+ then:
+ properties:
+ gpios:
+ maxItems: 3
+ gpio-names:
+ maxItems: 3
+ else:
+ properties:
+ gpios:
+ minItems: 5
+ gpio-names:
+ minItems: 5
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+ hsi-client {
+ compatible = "nokia,n900-modem";
+
+ hsi-channel-ids = <0>, <1>, <2>, <3>;
+ hsi-channel-names = "mcsaab-control",
+ "speech-control",
+ "speech-data",
+ "mcsaab-data";
+ hsi-speed-kbps = <55000>;
+ hsi-mode = "frame";
+ hsi-flow = "synchronized";
+ hsi-arb-mode = "round-robin";
+
+ interrupts-extended = <&gpio3 8 IRQ_TYPE_EDGE_FALLING>;
+
+ gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>,
+ <&gpio3 9 GPIO_ACTIVE_HIGH>,
+ <&gpio3 10 GPIO_ACTIVE_HIGH>,
+ <&gpio3 11 GPIO_ACTIVE_HIGH>,
+ <&gpio5 29 GPIO_ACTIVE_HIGH>;
+ gpio-names = "cmt_apeslpx",
+ "cmt_rst_rq",
+ "cmt_en",
+ "cmt_rst",
+ "cmt_bsi";
+ };
--
2.43.0
^ permalink raw reply related
* [PATCH 0/3] Convert MIPI HSI DT bindings to YAML
From: Sebastian Reichel @ 2024-03-25 21:45 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sebastian Reichel
Cc: Tony Lindgren, devicetree, linux-omap, linux-kernel,
Sebastian Reichel
Hi,
This converts all MIPI HSI subystem DT bindings to YAML.
I ran the following tests:
1. Check binding files
make -j$(nproc) dt_binding_check DT_SCHEMA_FILES=/hsi/
2. Check OMAP3 and nokia-modem DT
make -j$(nproc) CHECK_DTBS=y ti/omap/omap3-n900.dtb ti/omap/omap3-n950.dtb ti/omap/omap3-n9.dtb
3. Check OMAP4 DT (OMAP4 HSI is not used upstream, so one is enough)
make -j$(nproc) CHECK_DTBS=y ti/omap/omap4-droid4-xt894.dtb
FWIW I noticed a lots of warnings for OMAP3 & OMAP4, but
none related to HSI/SSI.
Greetings,
-- Sebastian
---
Sebastian Reichel (3):
dt-bindings: hsi: hsi-client: convert to YAML
dt-bindings: hsi: nokia-modem: convert to YAML
dt-bindings: hsi: omap-ssi: convert to YAML
.../devicetree/bindings/hsi/client-devices.txt | 44 -----
.../devicetree/bindings/hsi/hsi-client.yaml | 84 +++++++++
.../devicetree/bindings/hsi/nokia-modem.txt | 59 -------
.../devicetree/bindings/hsi/nokia-modem.yaml | 101 +++++++++++
Documentation/devicetree/bindings/hsi/omap-ssi.txt | 102 -----------
.../devicetree/bindings/hsi/ti,omap-ssi.yaml | 196 +++++++++++++++++++++
6 files changed, 381 insertions(+), 205 deletions(-)
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240325-hsi-dt-binding-a5d662e3d601
Best regards,
--
Sebastian Reichel <sebastian.reichel@collabora.com>
^ permalink raw reply
* [PATCH 3/3] dt-bindings: hsi: omap-ssi: convert to YAML
From: Sebastian Reichel @ 2024-03-25 21:45 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sebastian Reichel
Cc: Tony Lindgren, devicetree, linux-omap, linux-kernel,
Sebastian Reichel
In-Reply-To: <20240325-hsi-dt-binding-v1-0-88e8e97c3aae@collabora.com>
Convert the legacy txt binding to modern YAML.
No semantic change.
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
Documentation/devicetree/bindings/hsi/omap-ssi.txt | 102 -----------
.../devicetree/bindings/hsi/ti,omap-ssi.yaml | 196 +++++++++++++++++++++
2 files changed, 196 insertions(+), 102 deletions(-)
diff --git a/Documentation/devicetree/bindings/hsi/omap-ssi.txt b/Documentation/devicetree/bindings/hsi/omap-ssi.txt
deleted file mode 100644
index 77a0c3c3036e..000000000000
--- a/Documentation/devicetree/bindings/hsi/omap-ssi.txt
+++ /dev/null
@@ -1,102 +0,0 @@
-OMAP SSI controller bindings
-
-OMAP3's Synchronous Serial Interface (SSI) controller implements a
-legacy variant of MIPI's High Speed Synchronous Serial Interface (HSI),
-while the controller found inside OMAP4 is supposed to be fully compliant
-with the HSI standard.
-
-Required properties:
-- compatible: Should include "ti,omap3-ssi" or "ti,omap4-hsi"
-- reg-names: Contains the values "sys" and "gdd" (in this order).
-- reg: Contains a matching register specifier for each entry
- in reg-names.
-- interrupt-names: Contains the value "gdd_mpu".
-- interrupts: Contains matching interrupt information for each entry
- in interrupt-names.
-- ranges: Represents the bus address mapping between the main
- controller node and the child nodes below.
-- clock-names: Must include the following entries:
- "ssi_ssr_fck": The OMAP clock of that name
- "ssi_sst_fck": The OMAP clock of that name
- "ssi_ick": The OMAP clock of that name
-- clocks: Contains a matching clock specifier for each entry in
- clock-names.
-- #address-cells: Should be set to <1>
-- #size-cells: Should be set to <1>
-
-Each port is represented as a sub-node of the ti,omap3-ssi device.
-
-Required Port sub-node properties:
-- compatible: Should be set to the following value
- ti,omap3-ssi-port (applicable to OMAP34xx devices)
- ti,omap4-hsi-port (applicable to OMAP44xx devices)
-- reg-names: Contains the values "tx" and "rx" (in this order).
-- reg: Contains a matching register specifier for each entry
- in reg-names.
-- interrupts: Should contain interrupt specifiers for mpu interrupts
- 0 and 1 (in this order).
-- ti,ssi-cawake-gpio: Defines which GPIO pin is used to signify CAWAKE
- events for the port. This is an optional board-specific
- property. If it's missing the port will not be
- enabled.
-
-Optional properties:
-- ti,hwmods: Shall contain TI interconnect module name if needed
- by the SoC
-
-Example for Nokia N900:
-
-ssi-controller@48058000 {
- compatible = "ti,omap3-ssi";
-
- /* needed until hwmod is updated to use the compatible string */
- ti,hwmods = "ssi";
-
- reg = <0x48058000 0x1000>,
- <0x48059000 0x1000>;
- reg-names = "sys",
- "gdd";
-
- interrupts = <55>;
- interrupt-names = "gdd_mpu";
-
- clocks = <&ssi_ssr_fck>,
- <&ssi_sst_fck>,
- <&ssi_ick>;
- clock-names = "ssi_ssr_fck",
- "ssi_sst_fck",
- "ssi_ick";
-
- #address-cells = <1>;
- #size-cells = <1>;
- ranges;
-
- ssi-port@4805a000 {
- compatible = "ti,omap3-ssi-port";
-
- reg = <0x4805a000 0x800>,
- <0x4805a800 0x800>;
- reg-names = "tx",
- "rx";
-
- interrupt-parent = <&intc>;
- interrupts = <67>,
- <68>;
-
- ti,ssi-cawake-gpio = <&gpio5 23 GPIO_ACTIVE_HIGH>; /* 151 */
- }
-
- ssi-port@4805a000 {
- compatible = "ti,omap3-ssi-port";
-
- reg = <0x4805b000 0x800>,
- <0x4805b800 0x800>;
- reg-names = "tx",
- "rx";
-
- interrupt-parent = <&intc>;
- interrupts = <69>,
- <70>;
-
- }
-}
diff --git a/Documentation/devicetree/bindings/hsi/ti,omap-ssi.yaml b/Documentation/devicetree/bindings/hsi/ti,omap-ssi.yaml
new file mode 100644
index 000000000000..eb82f85c25b6
--- /dev/null
+++ b/Documentation/devicetree/bindings/hsi/ti,omap-ssi.yaml
@@ -0,0 +1,196 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hsi/ti,omap-ssi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SSI Controller on OMAP SoCs
+
+description:
+ OMAP3's Synchronous Serial Interface (SSI) controller implements a
+ legacy variant of MIPI's High Speed Synchronous Serial Interface (HSI),
+ while the controller found inside OMAP4 is supposed to be fully compliant
+ with the HSI standard.
+
+maintainers:
+ - Sebastian Reichel <sre@kernel.org>
+
+properties:
+ compatible:
+ enum:
+ - ti,omap3-ssi
+ - ti,omap4-hsi
+
+ reg:
+ items:
+ - description: registers for sys
+ - description: registers for gdd
+
+ reg-names:
+ items:
+ - const: sys
+ - const: gdd
+
+ ranges: true
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 1
+
+ clocks:
+ minItems: 1
+ maxItems: 3
+
+ clock-names:
+ minItems: 1
+ maxItems: 3
+
+ interrupts:
+ maxItems: 1
+
+ interrupt-names:
+ const: gdd_mpu
+
+ ti,hwmods:
+ const: ssi
+ deprecated: true
+
+patternProperties:
+ "[hs]si-port@[0-9a-f]+":
+ type: object
+
+ additionalProperties: false
+
+ properties:
+ compatible:
+ enum:
+ - ti,omap3-ssi-port
+ - ti,omap4-hsi-port
+
+ reg:
+ items:
+ - description: TX registers
+ - description: RX registers
+
+ reg-names:
+ items:
+ - const: tx
+ - const: rx
+
+ interrupts:
+ items:
+ - description: MPU interrupt 0
+ - description: MPU interrupt 1
+ minItems: 1
+
+ ti,ssi-cawake-gpio:
+ description: GPIO signifying CAWAKE events
+ maxItems: 1
+
+ hsi-client:
+ type: object
+ $ref: /schemas/hsi/hsi-client.yaml#
+
+ required:
+ - compatible
+ - reg
+ - reg-names
+ - interrupts
+
+ allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: ti,omap3-ssi-port
+ then:
+ properties:
+ $nodename:
+ pattern: "^ssi-port@(.*)?$"
+ interrupts:
+ minItems: 2
+ else:
+ properties:
+ $nodename:
+ pattern: "^hsi-port@(.*)?$"
+ interrupts:
+ maxItems: 1
+
+additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - reg-names
+ - ranges
+ - "#address-cells"
+ - "#size-cells"
+ - clocks
+ - clock-names
+ - interrupts
+ - interrupt-names
+
+allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: ti,omap3-ssi
+ then:
+ properties:
+ clocks:
+ minItems: 3
+ clock-names:
+ items:
+ - const: ssi_ssr_fck
+ - const: ssi_sst_fck
+ - const: ssi_ick
+ else:
+ properties:
+ clocks:
+ maxItems: 1
+ clock-names:
+ items:
+ - const: hsi_fck
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ ssi-controller@48058000 {
+ compatible = "ti,omap3-ssi";
+ reg = <0x48058000 0x1000>,
+ <0x48059000 0x1000>;
+ reg-names = "sys", "gdd";
+ ranges;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ clocks = <&ssi_ssr_fck>,
+ <&ssi_sst_fck>,
+ <&ssi_ick>;
+ clock-names = "ssi_ssr_fck",
+ "ssi_sst_fck",
+ "ssi_ick";
+ interrupts = <55>;
+ interrupt-names = "gdd_mpu";
+
+ ssi-port@4805a000 {
+ compatible = "ti,omap3-ssi-port";
+ reg = <0x4805a000 0x800>,
+ <0x4805a800 0x800>;
+ reg-names = "tx", "rx";
+ interrupt-parent = <&intc>;
+ interrupts = <67>, <68>;
+ ti,ssi-cawake-gpio = <&gpio5 23 GPIO_ACTIVE_HIGH>;
+ };
+
+ ssi-port@4805b000 {
+ compatible = "ti,omap3-ssi-port";
+ reg = <0x4805b000 0x800>,
+ <0x4805b800 0x800>;
+ reg-names = "tx", "rx";
+ interrupt-parent = <&intc>;
+ interrupts = <69>, <70>;
+ };
+ };
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v4 4/7] dt-bindings: iio: accel: adxl345: Add spi-3wire
From: Krzysztof Kozlowski @ 2024-03-25 21:40 UTC (permalink / raw)
To: Lothar Rubusch
Cc: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt, linux-iio, devicetree, linux-kernel, eraretuya
In-Reply-To: <CAFXKEHbJ_5unY24aZeutvM-xrjevQ=z7ngDcgwJR=NXzXONx5A@mail.gmail.com>
On 25/03/2024 22:05, Lothar Rubusch wrote:
> On Mon, Mar 25, 2024 at 7:32 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 25/03/2024 16:33, Lothar Rubusch wrote:
>>> Add spi-3wire because the driver optionally supports spi-3wire.
>>
>> This is a friendly reminder during the review process.
>>
>> It seems my or other reviewer's previous comments were not fully
>> addressed. Maybe the feedback got lost between the quotes, maybe you
>> just forgot to apply it. Please go back to the previous discussion and
>> either implement all requested changes or keep discussing them.
>>
>> Thank you.
>>
>
> You refer yourself to the above mentioned wording. Would replacing
> "driver" by "device" in the dt-bindings patch comment be sufficient?
> Did I miss something else?
Yes, the wording, but isn't the device require 3-wire mode? Don't just
replace one word with another, but write the proper rationale for your
hardware.
>
>>>
>>> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
>>> ---
>>
>> This is a friendly reminder during the review process.
>>
>> It looks like you received a tag and forgot to add it.
>>
>> If you do not know the process, here is a short explanation:
>> Please add Acked-by/Reviewed-by/Tested-by tags when posting new
>> versions, under or above your Signed-off-by tag. Tag is "received", when
>> provided in a message replied to you on the mailing list. Tools like b4
>> can help here. However, there's no need to repost patches *only* to add
>
> Just for confirmation: when I receive a feedback, requesting a change.
> And, I accept the change request. This means, I received a tag
> "Reviewed-by" which I have to mention in the upcoming patch version
> where this change is implemented and in that particular patch?
Please go through the docs. Yes, you received a tag which should be
included with the change.
Reviewer's feedback should not be ignored.
>
>> the tags. The upstream maintainer will do that for tags received on the
>> version they apply.
>>
>
> I'm pretty sure we will still see further iterations. So, I apply the
> tags in the next version, already scheduled. Ok?
>
>> https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
>>
>
> Going over the books I feel it does not make sense to still mention
> feedback ("Reveiewed-by") for the v1 or v2 of the patch here in a v5,
> does it? Your link mentiones "However if the patch has changed
I don't understand. When did you receive the tag? v3, right? So what do
you mean by v1 and v2?
Best regards,
Krzysztof
^ 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