Hi Sakari,
Thank you for your comments. I made corrections on top of the
v2 I already pushed and will push this into a v3 shortly.
On Wed, Aug 30, 2023 at 08:20:37AM +0000, Sakari Ailus wrote:
...
> > > > +#define STOP_TIMEOUT_US 1000
> > > > +#define POLL_INTERVAL_US 50
> > > > +static int dcmipp_byteproc_s_stream(struct v4l2_subdev *sd, int enable)
> > > > +{
> > > > + struct dcmipp_byteproc_device *byteproc = v4l2_get_subdevdata(sd);
> > > > + int ret = 0;
> > > > +
> > > > + mutex_lock(&byteproc->lock);
> > > > + if (enable) {
> > > > + dcmipp_byteproc_configure_framerate(byteproc);
> > > > +
> > > > + ret = dcmipp_byteproc_configure_scale_crop(byteproc);
> > > > + if (ret)
> > > > + goto err;
> > >
> > > This does nothing.
> >
> > Not sure to understand your point here. The s_stream callback of this
> > subdev is used to configure the registers (here the ones controlling
> > decimation and cropping) of the byteproc subdev.
>
> I was referring to the last two lines --- you're jumping to essentially the
> same location here.
Ok. Fixed with the s_stream calls rework.
...
> > > > diff --git a/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-core.c b/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-core.c
> > > > new file mode 100644
> > > > index 000000000000..aa7ae9a5b1a8
> > > > --- /dev/null
> > > > +++ b/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-core.c
> > > > @@ -0,0 +1,682 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Driver for STM32 Digital Camera Memory Interface Pixel Processor
> > > > + *
> > > > + * Copyright (C) STMicroelectronics SA 2022
> > > > + * Authors: Hugues Fruchet <hugues.fruchet@xxxxxxxxxxx>
> > > > + * Alain Volmat <alain.volmat@xxxxxxxxxxx>
> > > > + * for STMicroelectronics.
> > > > + */
> > > > +
> > > > +#include <linux/clk.h>
> > > > +#include <linux/component.h>
> > > > +#include <linux/delay.h>
> > > > +#include <linux/init.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/of_device.h>
> > > > +#include <linux/of_graph.h>
> > >
> > > #include <linux/property.h> instead of these three.
> >
> > Added linux/property.h however kept of_graph.h which is still necessary.
> >
> You should switch to fwnode graph API as you're already using fwnodes in
> the driver --- due to V4L2 fwnode.
Done as well.
> ...
>
> > > > +static int dcmipp_graph_notify_bound(struct v4l2_async_notifier *notifier,
> > > > + struct v4l2_subdev *subdev,
> > > > + struct v4l2_async_subdev *asd)
> > > > +{
> > > > + struct dcmipp_device *dcmipp = notifier_to_dcmipp(notifier);
> > > > + unsigned int ret;
> > > > + int src_pad;
> > > > + struct dcmipp_ent_device *sink;
> > > > + struct device_node *np = dcmipp->dev->of_node;
> > > > + struct v4l2_fwnode_endpoint ep = { .bus_type = 0 };
> > >
> > > Please set bus_type explicitly (DPHY)?
> >
> > My understanding is that I cannot set the bus_type here to have the
> > framework check for me since we support both V4L2_MBUS_PARALLEL and
> > V4L2_MBUS_BT656.
>
> Ah, I missed this was using a parallel bus.
>
> As you have a default in bindings, then you'll need to parse this assuming
> that bus-type first. I.e. set the bus type to the default and if parsing
> fails, try the other one.
Ok
Regards,
Alain
