Re: [Linux-stm32] [PATCH v1 3/5] media: stm32-dcmipp: STM32 DCMIPP camera interface driver

[Alain Volmat <alain.volmat@foss.st.com>]

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@foss.st.com>
> > > > + *          Alain Volmat <alain.volmat@foss.st.com>
> > > > + *          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