Message-ID: <23ccc744-745d-4a31-a79c-2d64bf1ed43d@collabora.com>
Date: Thu, 28 Aug 2025 12:03:13 +0200
From: Michael Riesch <michael.riesch@...labora.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
 Mehdi Djait <mehdi.djait@...ux.intel.com>,
 Maxime Chevallier <maxime.chevallier@...tlin.com>,
 Théo Lebrun <theo.lebrun@...tlin.com>,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 Gerald Loacker <gerald.loacker@...fvision.net>,
 Markus Elfring <Markus.Elfring@....de>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring
 <robh+dt@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 Kever Yang <kever.yang@...k-chips.com>,
 Nicolas Dufresne <nicolas.dufresne@...labora.com>,
 Sebastian Reichel <sebastian.reichel@...labora.com>,
 Collabora Kernel Team <kernel@...labora.com>,
 Paul Kocialkowski <paulk@...-base.io>,
 Alexander Shiyan <eagle.alexander923@...il.com>,
 Val Packett <val@...kett.cool>, Rob Herring <robh@...nel.org>,
 Philipp Zabel <p.zabel@...gutronix.de>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: linux-media@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, Mehdi Djait <mehdi.djait@...tlin.com>
Subject: Re: [PATCH v10 06/13] media: rockchip: add a driver for the rockchip
 camera interface

Hi Bryan,

Thanks for your comments :-)

On 8/26/25 08:21, Bryan O'Donoghue wrote:
> On 19/08/2025 00:25, Michael Riesch via B4 Relay wrote:

[...]

>> +
>> +static void rkcif_dvp_stop_streaming(struct rkcif_stream *stream)
>> +{
>> +    struct rkcif_device *rkcif = stream->rkcif;
>> +    u32 val;
>> +
>> +    val = rkcif_dvp_read(rkcif, RKCIF_DVP_CTRL);
> 
> This dvp_read stuff looks a bit funny to me, you have a lookup which can
> return 0 for unknown registers.
> 
> Probably not the case with a control register like this one but, for
> argument sake if RKCIF_DVP_CTRL was not a valid register i.e.
> rkcif_dvp_read() would return 0 and you'd still act on that data to
> write back to an unkown register.

...which would then hit the same check in rkcif_dvp_write and simply
return without writing anything. Also, the WARN_ON_ONCE in the lookup
would complain and indicate that the driver developer made some mistake.
I hope that the driver developer is thus nudged towards fixing the code
they wrote.

> Would you not be better off having say callbacks to contain cases where
> registers are potentially not present
> 
> ops->update_maybe_not_present_reg();
> 
> followed by writes to registers that would alawys be there ?

I'll think about that in more detail, but right now my thoughts are that
if any of the registers below are not valid, this piece of hardware is
pretty useless and there is something rotten in the driver. Thus, we
complain loudly to the developer.

>> +    rkcif_dvp_write(rkcif, RKCIF_DVP_CTRL,
>> +            val & (~RKCIF_CTRL_ENABLE_CAPTURE));
>> +    rkcif_dvp_write(rkcif, RKCIF_DVP_INTEN, 0x0);
>> +    rkcif_dvp_write(rkcif, RKCIF_DVP_INTSTAT, 0x3ff);
>> +    rkcif_dvp_write(rkcif, RKCIF_DVP_FRAME_STATUS, 0x0);
>> +
>> +    stream->stopping = false;
>> +}
>> +
>> +static void rkcif_dvp_reset_stream(struct rkcif_device *rkcif)
>> +{
>> +    u32 ctl = rkcif_dvp_read(rkcif, RKCIF_DVP_CTRL);
>> +
>> +    rkcif_dvp_write(rkcif, RKCIF_DVP_CTRL,
>> +            ctl & (~RKCIF_CTRL_ENABLE_CAPTURE));
>> +    rkcif_dvp_write(rkcif, RKCIF_DVP_CTRL, ctl |
>> RKCIF_CTRL_ENABLE_CAPTURE);
>> +}
>> +
>> +static void rkcif_dvp_set_crop(struct rkcif_stream *stream, u16 left,
>> u16 top)
>> +{
>> +    struct rkcif_device *rkcif = stream->rkcif;
>> +    u32 val;
>> +
>> +    val = RKCIF_XY_COORD(left, top);
>> +    rkcif_dvp_write(rkcif, RKCIF_DVP_CROP, val);
>> +}
>> +
>> +irqreturn_t rkcif_dvp_isr(int irq, void *ctx)
>> +{
>> +    struct device *dev = ctx;
>> +    struct rkcif_device *rkcif = dev_get_drvdata(dev);
>> +    struct rkcif_stream *stream;
>> +    u32 intstat, lastline, lastpix, cif_frmst;
>> +    irqreturn_t ret = IRQ_NONE;
>> +
>> +    if (!rkcif->match_data->dvp)
>> +        return ret;
> 
> Wouldn't you be better off conditionally registering your ISR if
> match_data->dvp is true instead ?

As you have surely seen, the ISR is shared between all interfaces, i.e.,
DVP and MIPI. Now the currently supported models all have a DVP and your
suggestion would work. However, I think the RK3562 VICAP can be easily
supported by this driver but does not feature a DVP (several MIPI
interfaces, though). In this case match_data->dvp evaluates to false but
still there is the need to register the ISR.

> 
>> +
>> +    intstat = rkcif_dvp_read(rkcif, RKCIF_DVP_INTSTAT);
>> +    cif_frmst = rkcif_dvp_read(rkcif, RKCIF_DVP_FRAME_STATUS);
>> +    lastline = RKCIF_FETCH_Y(rkcif_dvp_read(rkcif,
>> RKCIF_DVP_LAST_LINE));
>> +    lastpix = RKCIF_FETCH_Y(rkcif_dvp_read(rkcif, RKCIF_DVP_LAST_PIX));
>> +
>> +    if (intstat & RKCIF_INTSTAT_FRAME_END) {
>> +        rkcif_dvp_write(rkcif, RKCIF_DVP_INTSTAT,
>> +                RKCIF_INTSTAT_FRAME_END_CLR |
>> +                RKCIF_INTSTAT_LINE_END_CLR);
>> +
>> +        stream = &rkcif->interfaces[RKCIF_DVP].streams[RKCIF_ID0];
>> +
>> +        if (stream->stopping) {
>> +            rkcif_dvp_stop_streaming(stream);
>> +            wake_up(&stream->wq_stopped);
>> +            ret = IRQ_HANDLED;
>> +            goto out;
>> +        }
>> +
>> +        if (lastline != stream->pix.height) {
>> +            v4l2_err(&rkcif->v4l2_dev,
>> +                 "bad frame, irq:%#x frmst:%#x size:%dx%d\n",
>> +                 intstat, cif_frmst, lastpix, lastline);
>> +
>> +            rkcif_dvp_reset_stream(rkcif);
>> +        }
>> +
>> +        rkcif_stream_pingpong(stream);
>> +
>> +        ret = IRQ_HANDLED;
>> +    }
>> +out:
>> +    return ret;
>> +}
>> +
>> +int rkcif_dvp_register(struct rkcif_device *rkcif)
>> +{
>> +    struct rkcif_interface *interface;
>> +    unsigned int streams_num;
>> +    int ret;
>> +
>> +    if (!rkcif->match_data->dvp)
>> +        return 0;
> 
> If you don't register the device when match_data->dvp is false, then I
> think you can relax the carry-on checks elsewhere on match_data->dvp,
> not including dvp_unregister

+1 I'll review all instances of this check.

> The rest of the file as I breifly skim it looks OK to me, its a bit big
> though.
> 
> Would it be possible to break this patch up a little bit ? Might make it
> easier for other reviewers to give an SoB for smaller chunks.

I suppose what I could do is split this up into five patches, as the
commit message already outlines:

1) add a basic driver (no-op skeleton only)
2) abstraction for the ping-pong scheme to allow for future extensions
3) abstraction for the INTERFACE and CROP parts to allow for future
   extensions
4) support for the PX30 VIP
5) support for the RK3568 VICAP DVP

Please note that in this case I would rework the patch for a final
(this-time-really-final) time and drop this elaborate co-developed-by
list, as the patch in question will then have nothing to do it all with
anything what was before v2 of this series.

Unless I hear any objections to that from anyone, I'll try to implement
that next week.

Best regards,
Michael

> 
> ---
> bod

Powered by blists - more mailing lists