Message-ID: <82f447d1-89ae-450f-813e-02c3d8228895@linaro.org>
Date: Thu, 28 Aug 2025 16:08:48 +0200
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Michael Riesch <michael.riesch@...labora.com>,
 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

On 28/08/2025 12:03, Michael Riesch wrote:
> 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.
The developer is you though :)

Anyway seems a bit weird to me to return invalid registers, you trust 
yourself right ?

>>> +    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.

Why not have separate ISRs then with shared code calling into a function ?

An ISR in my mind should fire for hardware we have a clear idea about 
and only do so in exceptional (pun intended) circumstances.

So I'd suggest two ISRs calling into whatever shared code you deem 
necessary as opposed to NULL checking in your 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.

I'll leave that up to you, I'll still review your v11 but, a slightly 
smaller single patch to digest would be appreciated.

---
bod

Powered by blists - more mailing lists