Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
Hi Jacopo,
On Friday, 22 December 2017 16:40:16 EET jacopo mondi wrote:
> On Fri, Dec 22, 2017 at 02:03:41PM +0200, Laurent Pinchart wrote:
> > On Thursday, 21 December 2017 18:27:02 EET jacopo mondi wrote:
> >> On Mon, Dec 18, 2017 at 05:28:43PM +0200, Laurent Pinchart wrote:
> >>> On Monday, 18 December 2017 14:25:12 EET jacopo mondi wrote:
> On Mon, Dec 11, 2017 at 06:15:23PM +0200, Laurent Pinchart wrote:
> >>
> >> [snip]
> >>
> >> +/**
> >> + * ceu_soft_reset() - Software reset the CEU interface
> >> + */
> >> +static int ceu_soft_reset(struct ceu_device *ceudev)
> >> +{
> >> + unsigned int reset_done;
> >> + unsigned int i;
> >> +
> >> + ceu_write(ceudev, CEU_CAPSR, CEU_CAPSR_CPKIL);
> >> +
> >> + reset_done = 0;
> >> + for (i = 0; i < 1000 && !reset_done; i++) {
> >> + udelay(1);
> >> + if (!(ceu_read(ceudev, CEU_CSTSR) & CEU_CSTRST_CPTON))
> >> + reset_done++;
> >> + }
> >
> > How many iterations does this typically require ? Wouldn't a sleep
> > be better than a delay ? As far as I can tell the ceu_soft_reset()
> > function is only called with interrupts disabled (in interrupt
> > context) from ceu_capture() in an error path, and that code should be
> > reworked to make it possible to sleep if a reset takes too long.
>
> The HW manual does not provide any indication about absolute timings.
> I can empirically try and see, but that would just be a hint.
> >>>
> >>> That's why I asked how many iterations it typically takes :-) A hint
> >>> is enough to start with, preferably on both SH and ARM SoCs.
> >>
> >> I've seen only 0s when printing out how many cycles it takes to clear
> >> both registers. This means 1usec is enough, therefore I would keep using
> >> udelay here. Also, I would reduce the attempts to 100 here (or even
> >> 10), as if a single one is typically enough, 1000 is definitely an
> >> overkill.
> >
> > I'd go for 10. This being said, please make sure you run tests where the
> > reset is performed during capture in the middle of a frame, to see if it
> > changes the number of iterations.
>
> The only way I can think to do this is to stream_on then immediately
> stream_off before we get the frame and thus casue the interface reset.
> Any other idea?
I think we should test reset of the state machine both during vertical
blanking when it's waiting for a frame, and in the middle of the frame. The
former should be easy to test by stopping the stream immediately before the
sensor starts outputting a frame (if you can disable the HSYNC and/or VSYNC
outputs of the sensor it would ensure that the CEU doesn't start receiving
data, that could be useful). The latter shouldn't be difficult to test with an
appropriate delay from the frame end interrupt to the stream stop.
> [snip]
>
> >> +
> >> +/**
> >> + * ceu_capture() - Trigger start of a capture sequence
> >> + *
> >> + * Return value doesn't reflect the success/failure to queue the
> >> new buffer,
> >> + * but rather the status of the previous capture.
> >> + */
> >> +static int ceu_capture(struct ceu_device *ceudev)
> >> +{
> >> + struct v4l2_pix_format_mplane *pix = &ceudev->v4l2_pix;
> >> + dma_addr_t phys_addr_top;
> >> + u32 status;
> >> +
> >> + /* Clean interrupt status and re-enable interrupts */
> >> + status = ceu_read(ceudev, CEU_CETCR);
> >> + ceu_write(ceudev, CEU_CEIER,
> >> +ceu_read(ceudev, CEU_CEIER) & ~CEU_CEIER_MASK);
> >> + ceu_write(ceudev, CEU_CETCR, ~status & CEU_CETCR_MAGIC);
> >> + ceu_write(ceudev, CEU_CEIER, CEU_CEIER_MASK);
> >
> > I wonder why there's a need to disable and reenable interrupts here.
>
> The original driver clearly said "The hardware is -very- picky about
> this sequence" and I got scared and nerver touched this.
> >>>
> >>> How about experimenting to see how the hardware reacts ?
> >>
> >> Turns out this was not needed at all, both on RZ and SH4. I captured
> >> several images without any issues in both platforms just clearing the
> >> interrupt state without disabling interrutps.
> >
> > I wonder whether it could cause an issue when interrupts are raised by the
> > hardware at the same time they are cleared by the driver. That's hard to
> > test though.
> >
> > What happens when an interrupt source is masked by the CEIER register, is
> > it still reported by the status CETCR register (obviously without raising
> > an interrupt to the CPU), or does it not get flagged at all ?
>
> They get flagged, yes, and right now I'm clearing all of them at the
> beginning of IRQ handler writing ~CEU_CETR_ALL_INT to CETCR.
OK. If they didn't get flagged it would mean that disabling interrupts while
clearing the flags could have an influence on wh
Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
Hi Laurent,
On Fri, Dec 22, 2017 at 02:03:41PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Thursday, 21 December 2017 18:27:02 EET jacopo mondi wrote:
> > On Mon, Dec 18, 2017 at 05:28:43PM +0200, Laurent Pinchart wrote:
> > > On Monday, 18 December 2017 14:25:12 EET jacopo mondi wrote:
> > >> On Mon, Dec 11, 2017 at 06:15:23PM +0200, Laurent Pinchart wrote:
> > >
> > [snip]
> >
> > +/**
> > + * ceu_soft_reset() - Software reset the CEU interface
> > + */
> > +static int ceu_soft_reset(struct ceu_device *ceudev)
> > +{
> > + unsigned int reset_done;
> > + unsigned int i;
> > +
> > + ceu_write(ceudev, CEU_CAPSR, CEU_CAPSR_CPKIL);
> > +
> > + reset_done = 0;
> > + for (i = 0; i < 1000 && !reset_done; i++) {
> > + udelay(1);
> > + if (!(ceu_read(ceudev, CEU_CSTSR) & CEU_CSTRST_CPTON))
> > + reset_done++;
> > + }
> > >>>
> > >>> How many iterations does this typically require ? Wouldn't a sleep be
> > >>> better than a delay ? As far as I can tell the ceu_soft_reset()
> > >>> function is only called with interrupts disabled (in interrupt context)
> > >>> from ceu_capture() in an error path, and that code should be reworked
> > >>> to make it possible to sleep if a reset takes too long.
> > >>
> > >> The HW manual does not provide any indication about absolute timings.
> > >> I can empirically try and see, but that would just be a hint.
> > >
> > > That's why I asked how many iterations it typically takes :-) A hint is
> > > enough to start with, preferably on both SH and ARM SoCs.
> >
> > I've seen only 0s when printing out how many cycles it takes to clear
> > both registers. This means 1usec is enough, therefore I would keep using
> > udelay here. Also, I would reduce the attempts to 100 here (or even
> > 10), as if a single one is typically enough, 1000 is definitely an
> > overkill.
>
> I'd go for 10. This being said, please make sure you run tests where the reset
> is performed during capture in the middle of a frame, to see if it changes the
> number of iterations.
>
The only way I can think to do this is to stream_on then immediately
stream_off before we get the frame and thus casue the interface reset.
Any other idea?
[snip]
> > +
> > +/**
> > + * ceu_capture() - Trigger start of a capture sequence
> > + *
> > + * Return value doesn't reflect the success/failure to queue the new
> > buffer,
> > + * but rather the status of the previous capture.
> > + */
> > +static int ceu_capture(struct ceu_device *ceudev)
> > +{
> > + struct v4l2_pix_format_mplane *pix = &ceudev->v4l2_pix;
> > + dma_addr_t phys_addr_top;
> > + u32 status;
> > +
> > + /* Clean interrupt status and re-enable interrupts */
> > + status = ceu_read(ceudev, CEU_CETCR);
> > + ceu_write(ceudev, CEU_CEIER,
> > +ceu_read(ceudev, CEU_CEIER) & ~CEU_CEIER_MASK);
> > + ceu_write(ceudev, CEU_CETCR, ~status & CEU_CETCR_MAGIC);
> > + ceu_write(ceudev, CEU_CEIER, CEU_CEIER_MASK);
> > >>>
> > >>> I wonder why there's a need to disable and reenable interrupts here.
> > >>
> > >> The original driver clearly said "The hardware is -very- picky about
> > >> this sequence" and I got scared and nerver touched this.
> > >
> > > How about experimenting to see how the hardware reacts ?
> >
> > Turns out this was not needed at all, both on RZ and SH4. I captured
> > several images without any issues in both platforms just clearing the
> > interrupt state without disabling interrutps.
>
> I wonder whether it could cause an issue when interrupts are raised by the
> hardware at the same time they are cleared by the driver. That's hard to test
> though.
>
> What happens when an interrupt source is masked by the CEIER register, is it
> still reported by the status CETCR register (obviously without raising an
> interrupt to the CPU), or does it not get flagged at all ?
They get flagged, yes, and right now I'm clearing all of them at the
beginning of IRQ handler writing ~CEU_CETR_ALL_INT to CETCR.
>
> > >> Also, I very much dislike the CEU_CETRC_MAGIC mask, but again the old
> > >> driver said "Acknoledge magical interrupt sources" and I was afraid to
> > >> change it (I can rename it though, to something lioke CEU_CETCR_ALL_INT
> > >> because that's what it is, a mask with all available interrupt source
> > >> enabled).
> > >
> > > I think renaming it is a good idea. Additionally, regardless of whether
> > > there is any hidden interrupt source, the datasheet mentions for all
> > > reserved bits that "The write value should always be 0". They should
> > > read as 0, but masking them would be an additional safeguard.
> >
> > The HW manual is a bit confused (and confusing) on this point.
> > Yes, there is the statement you have cited here, but ther
Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
Hi Jacopo,
On Thursday, 21 December 2017 18:27:02 EET jacopo mondi wrote:
> On Mon, Dec 18, 2017 at 05:28:43PM +0200, Laurent Pinchart wrote:
> > On Monday, 18 December 2017 14:25:12 EET jacopo mondi wrote:
> >> On Mon, Dec 11, 2017 at 06:15:23PM +0200, Laurent Pinchart wrote:
> >
> [snip]
>
> +/**
> + * ceu_soft_reset() - Software reset the CEU interface
> + */
> +static int ceu_soft_reset(struct ceu_device *ceudev)
> +{
> +unsigned int reset_done;
> +unsigned int i;
> +
> +ceu_write(ceudev, CEU_CAPSR, CEU_CAPSR_CPKIL);
> +
> +reset_done = 0;
> +for (i = 0; i < 1000 && !reset_done; i++) {
> +udelay(1);
> +if (!(ceu_read(ceudev, CEU_CSTSR) & CEU_CSTRST_CPTON))
> +reset_done++;
> +}
> >>>
> >>> How many iterations does this typically require ? Wouldn't a sleep be
> >>> better than a delay ? As far as I can tell the ceu_soft_reset()
> >>> function is only called with interrupts disabled (in interrupt context)
> >>> from ceu_capture() in an error path, and that code should be reworked
> >>> to make it possible to sleep if a reset takes too long.
> >>
> >> The HW manual does not provide any indication about absolute timings.
> >> I can empirically try and see, but that would just be a hint.
> >
> > That's why I asked how many iterations it typically takes :-) A hint is
> > enough to start with, preferably on both SH and ARM SoCs.
>
> I've seen only 0s when printing out how many cycles it takes to clear
> both registers. This means 1usec is enough, therefore I would keep using
> udelay here. Also, I would reduce the attempts to 100 here (or even
> 10), as if a single one is typically enough, 1000 is definitely an
> overkill.
I'd go for 10. This being said, please make sure you run tests where the reset
is performed during capture in the middle of a frame, to see if it changes the
number of iterations.
> >> Also, the reset function is called in many places (runtime_pm
> >> suspend/resume) s_stream(0) and in error path of ceu_capture().
> >>
> >> In ceu_capture() we reset the interface if the previous frame was bad,
> >> and we do that before re-enabling the capture interrupt (so interrupts
> >> are not -disabled-, just the one we care about is not enabled yet..)
> >
> > The ceu_capture() function is called from the driver's interrupt handler,
> > so interrupts are disabled in that code path.
>
> I have removed that reset call from capture and re-worked the irq
> handler to manage state before calling capture().
>
> >> But that's not big deal, as if we fail there, we are about to abort
> >> capture anyhow and so if we miss some spurious capture interrupt it's
> >> ok...
> >>
> >> >> + if (!reset_done) {
> >> >> + v4l2_err(&ceudev->v4l2_dev, "soft reset time out\n");
> >> >
> >> > How about dev_err() instead ?
> >>
> >> Is dev_err/dev_dbg preferred over v4l2_err/v4l2_dbg? Is this because
> >> of dynamic debug?
> >
> > Yes, and the fact that the V4L2 macros don't provide us anymore with much
> > compared to the dev_* macros.
> >
> +
> +/**
> + * ceu_capture() - Trigger start of a capture sequence
> + *
> + * Return value doesn't reflect the success/failure to queue the new
> buffer,
> + * but rather the status of the previous capture.
> + */
> +static int ceu_capture(struct ceu_device *ceudev)
> +{
> +struct v4l2_pix_format_mplane *pix = &ceudev->v4l2_pix;
> +dma_addr_t phys_addr_top;
> +u32 status;
> +
> +/* Clean interrupt status and re-enable interrupts */
> +status = ceu_read(ceudev, CEU_CETCR);
> +ceu_write(ceudev, CEU_CEIER,
> + ceu_read(ceudev, CEU_CEIER) & ~CEU_CEIER_MASK);
> +ceu_write(ceudev, CEU_CETCR, ~status & CEU_CETCR_MAGIC);
> +ceu_write(ceudev, CEU_CEIER, CEU_CEIER_MASK);
> >>>
> >>> I wonder why there's a need to disable and reenable interrupts here.
> >>
> >> The original driver clearly said "The hardware is -very- picky about
> >> this sequence" and I got scared and nerver touched this.
> >
> > How about experimenting to see how the hardware reacts ?
>
> Turns out this was not needed at all, both on RZ and SH4. I captured
> several images without any issues in both platforms just clearing the
> interrupt state without disabling interrutps.
I wonder whether it could cause an issue when interrupts are raised by the
hardware at the same time they are cleared by the driver. That's hard to test
though.
What happens when an interrupt source is masked by the CEIER register, is it
still reported by the status CETCR register (obviously without raising an
interrupt to the CPU), or does it not get flagged at all ?
> >> Also, I very much dislike the CEU_CETRC_MAGIC mask, but again the old
> >> drive
Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
Hi Laurent,
On Mon, Dec 18, 2017 at 05:28:43PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Monday, 18 December 2017 14:25:12 EET jacopo mondi wrote:
> > On Mon, Dec 11, 2017 at 06:15:23PM +0200, Laurent Pinchart wrote:
[snip]
> > >> +/**
> > >> + * ceu_soft_reset() - Software reset the CEU interface
> > >> + */
> > >> +static int ceu_soft_reset(struct ceu_device *ceudev)
> > >> +{
> > >> +unsigned int reset_done;
> > >> +unsigned int i;
> > >> +
> > >> +ceu_write(ceudev, CEU_CAPSR, CEU_CAPSR_CPKIL);
> > >> +
> > >> +reset_done = 0;
> > >> +for (i = 0; i < 1000 && !reset_done; i++) {
> > >> +udelay(1);
> > >> +if (!(ceu_read(ceudev, CEU_CSTSR) & CEU_CSTRST_CPTON))
> > >> +reset_done++;
> > >> +}
> > >
> > > How many iterations does this typically require ? Wouldn't a sleep be
> > > better than a delay ? As far as I can tell the ceu_soft_reset() function
> > > is only called with interrupts disabled (in interrupt context) from
> > > ceu_capture() in an error path, and that code should be reworked to make
> > > it possible to sleep if a reset takes too long.
> >
> > The HW manual does not provide any indication about absolute timings.
> > I can empirically try and see, but that would just be a hint.
>
> That's why I asked how many iterations it typically takes :-) A hint is enough
> to start with, preferably on both SH and ARM SoCs.
I've seen only 0s when printing out how many cycles it takes to clear
both registers. This means 1usec is enough, therefore I would keep using
udelay here. Also, I would reduce the attempts to 100 here (or even
10), as if a single one is typically enough, 1000 is definitely an
overkill.
>
> > Also, the reset function is called in many places (runtime_pm
> > suspend/resume) s_stream(0) and in error path of ceu_capture().
> >
> > In ceu_capture() we reset the interface if the previous frame was bad,
> > and we do that before re-enabling the capture interrupt (so interrupts
> > are not -disabled-, just the one we care about is not enabled yet..)
>
> The ceu_capture() function is called from the driver's interrupt handler, so
> interrupts are disabled in that code path.
>
I have removed that reset call from capture and re-worked the irq
handler to manage state before calling capture().
> > But that's not big deal, as if we fail there, we are about to abort
> > capture anyhow and so if we miss some spurious capture interrupt it's
> > ok...
> >
> > >> +if (!reset_done) {
> > >> +v4l2_err(&ceudev->v4l2_dev, "soft reset time out\n");
> > >
> > > How about dev_err() instead ?
> >
> > Is dev_err/dev_dbg preferred over v4l2_err/v4l2_dbg? Is this because
> > of dynamic debug?
>
> Yes, and the fact that the V4L2 macros don't provide us anymore with much
> compared to the dev_* macros.
>
> > >> +
> > >> +/**
> > >> + * ceu_capture() - Trigger start of a capture sequence
> > >> + *
> > >> + * Return value doesn't reflect the success/failure to queue the new
> > >> buffer,
> > >> + * but rather the status of the previous capture.
> > >> + */
> > >> +static int ceu_capture(struct ceu_device *ceudev)
> > >> +{
> > >> +struct v4l2_pix_format_mplane *pix = &ceudev->v4l2_pix;
> > >> +dma_addr_t phys_addr_top;
> > >> +u32 status;
> > >> +
> > >> +/* Clean interrupt status and re-enable interrupts */
> > >> +status = ceu_read(ceudev, CEU_CETCR);
> > >> +ceu_write(ceudev, CEU_CEIER,
> > >> + ceu_read(ceudev, CEU_CEIER) & ~CEU_CEIER_MASK);
> > >> +ceu_write(ceudev, CEU_CETCR, ~status & CEU_CETCR_MAGIC);
> > >> +ceu_write(ceudev, CEU_CEIER, CEU_CEIER_MASK);
> > >
> > > I wonder why there's a need to disable and reenable interrupts here.
> >
> > The original driver clearly said "The hardware is -very- picky about
> > this sequence" and I got scared and nerver touched this.
>
> How about experimenting to see how the hardware reacts ?
Turns out this was not needed at all, both on RZ and SH4. I captured
several images without any issues in both platforms just clearing the
interrupt state without disabling interrutps.
>
> > Also, I very much dislike the CEU_CETRC_MAGIC mask, but again the old driver
> > said "Acknoledge magical interrupt sources" and I was afraid to change it
> > (I can rename it though, to something lioke CEU_CETCR_ALL_INT because that's
> > what it is, a mask with all available interrupt source enabled).
>
> I think renaming it is a good idea. Additionally, regardless of whether there
> is any hidden interrupt source, the datasheet mentions for all reserved bits
> that "The write value should always be 0". They should read as 0, but
> masking them would be an additional safeguard.
The HW manual is a bit confused (and confusing) on this point.
Yes, there is the statement you have cited here, but there's also
"to clear only the CPE bit to 0, write H' FFFE to CETCR" a
Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
Hi Sakari, On Tuesday, 19 December 2017 15:28:55 EET Sakari Ailus wrote: > On Tue, Dec 19, 2017 at 03:07:41PM +0200, Laurent Pinchart wrote: > > On Tuesday, 19 December 2017 13:57:42 EET jacopo mondi wrote: [snip] > >> Ok, actually parse_dt() and parse_platform_data() behaves differently. > >> The former returns error if no subdevices are connected to CEU, the > >> latter returns 0. That's wrong. > >> > >> I wonder what's the correct behavior here. Other mainline drivers I > >> looked into (pxa_camera and atmel-isc) behaves differently from each > >> other, so I guess this is up to each platform to decide. > > > > No, what it means is that we've failed to standardize it, not that it > > shouldn't be standardized :-) > > > >> Also, the CEU can accept one single input (and I made it clear > >> in DT bindings documentation saying it accepts a single endpoint, > >> while I'm parsing all the available ones in driver, I will fix this) > >> but as it happens on Migo-R, there could be HW hacks to share the input > >> lines between multiple subdevices. Should I accept it from dts as well? > >> > >> So: > >> 1) Should we fail to probe if no subdevices are connected? > > > > While the CEU itself would be fully functional without a subdev, in > > practice it would be of no use. I would thus fail probing. > > > >> 2) Should we accept more than 1 subdevice from dts as it happens right > >> now for platform data? > > > > We need to support multiple connected devices, as some of the boards > > require that. What I'm not sure about is whether the multiplexer on the > > Migo-R board should be modeled as a subdevice. We could in theory connect > > multiple sensors to the CEU input signals without any multiplexer as long > > as all but one are in reset with their outputs in a high impedance state. > > As that wouldn' require a multiplexer we would need to support multiple > > endpoints in the CEU port. We could then support Migo-R the same way, > > making the multiplexer transparent. > > > > Sakari, what would you do here ? > > We do have: > > drivers/media/platform/video-mux.c > > What is not addressed right now are the CSI-2 bus parameters, if the mux is > just a passive switch. This could be done using the frame descriptors. We're talking about a parallel bus here so that shouldn't be a problem. Our issue is that the same GPIO controls both the switch and the power down signal of one of the sensors. The hardware has been designed to be as transparent as possible, but that creates issues as Linux doesn't support share GPIOs. -- Regards, Laurent Pinchart
Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
Heippa!
On Tue, Dec 19, 2017 at 03:07:41PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> (CC'ing Sakari)
>
> On Tuesday, 19 December 2017 13:57:42 EET jacopo mondi wrote:
> > On Mon, Dec 11, 2017 at 06:15:23PM +0200, Laurent Pinchart wrote:
> > > Hi Jacopo,
> > >
> > > Thank you for the patch.
> > >
> > > [snip]
> > >
> > >> +static int ceu_sensor_bound(struct v4l2_async_notifier *notifier,
> > >> +struct v4l2_subdev *v4l2_sd,
> > >> +struct v4l2_async_subdev *asd)
> > >> +{
> > >> +struct v4l2_device *v4l2_dev = notifier->v4l2_dev;
> > >> +struct ceu_device *ceudev = v4l2_to_ceu(v4l2_dev);
> > >> +struct ceu_subdev *ceu_sd = to_ceu_subdev(asd);
> > >> +
> > >> +if (video_is_registered(&ceudev->vdev)) {
> > >> +v4l2_err(&ceudev->v4l2_dev,
> > >> + "Video device registered before this
> > >> sub-device.\n");
> > >> +return -EBUSY;
> > >
> > > Can this happen ?
> > >
> > >> +}
> > >> +
> > >> +/* Assign subdevices in the order they appear */
> > >> +ceu_sd->v4l2_sd = v4l2_sd;
> > >> +ceudev->num_sd++;
> > >> +
> > >> +return 0;
> > >> +}
> > >> +
> > > > +static int ceu_sensor_complete(struct v4l2_async_notifier *notifier)
> > > > +{
> > > > + struct v4l2_device *v4l2_dev = notifier->v4l2_dev;
> > > > + struct ceu_device *ceudev = v4l2_to_ceu(v4l2_dev);
> > > > + struct video_device *vdev = &ceudev->vdev;
> > > > + struct vb2_queue *q = &ceudev->vb2_vq;
> > > > + struct v4l2_subdev *v4l2_sd;
> > > > + int ret;
> > > > +
> > > > + /* Initialize vb2 queue */
> > > > + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> > > > + q->io_modes = VB2_MMAP | VB2_USERPTR;
> > >
> > > No dmabuf ?
> > >
> > > > + q->drv_priv = ceudev;
> > > > + q->ops = &ceu_videobuf_ops;
> > > > + q->mem_ops = &vb2_dma_contig_memops;
> > > > + q->buf_struct_size = sizeof(struct ceu_buffer);
> > > > + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > > > + q->lock = &ceudev->mlock;
> > > > + q->dev = ceudev->v4l2_dev.dev;
> > >
> > > [snip]
> > >
> > > > +static int ceu_probe(struct platform_device *pdev)
> > > > +{
> > > > + struct device *dev = &pdev->dev;
> > > > + struct ceu_device *ceudev;
> > > > + struct resource *res;
> > > > + void __iomem *base;
> > > > + unsigned int irq;
> > > > + int num_sd;
> > > > + int ret;
> > > > +
> > > > + ceudev = kzalloc(sizeof(*ceudev), GFP_KERNEL);
> > >
> > > The memory is freed in ceu_vdev_release() as expected, but that will only
> > > work if the video device is registered. If the subdevs are never bound,
> > > the ceudev memory will be leaked if you unbind the CEU device from its
> > > driver. In my opinion this calls for registering the video device at
> > > probe time (although Hans disagrees).
> > >
> > > > + if (!ceudev)
> > > > + return -ENOMEM;
> > > > +
> > > > + platform_set_drvdata(pdev, ceudev);
> > > > + dev_set_drvdata(dev, ceudev);
> > >
> > > You don't need the second line, platform_set_drvdata() is a wrapper around
> > > dev_set_drvdata().
> > >
> > > > + ceudev->dev = dev;
> > > > +
> > > > + INIT_LIST_HEAD(&ceudev->capture);
> > > > + spin_lock_init(&ceudev->lock);
> > > > + mutex_init(&ceudev->mlock);
> > > > +
> > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > + if (IS_ERR(res))
> > > > + return PTR_ERR(res);
> > >
> > > No need for error handling here, devm_ioremap_resource() will check the
> > > res
> > > pointer.
> > >
> > > > + base = devm_ioremap_resource(dev, res);
> > >
> > > You can assign ceudev->base directly and remove the base local variable.
> > >
> > > > + if (IS_ERR(base))
> > > > + return PTR_ERR(base);
> > > > + ceudev->base = base;
> > > > +
> > > > + ret = platform_get_irq(pdev, 0);
> > > > + if (ret < 0) {
> > > > + dev_err(dev, "failed to get irq: %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > + irq = ret;
> > > > +
> > > > + ret = devm_request_irq(dev, irq, ceu_irq,
> > > > + 0, dev_name(dev), ceudev);
> > > > + if (ret) {
> > > > + dev_err(&pdev->dev, "Unable to register CEU
> > > > interrupt.\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + pm_suspend_ignore_children(dev, true);
> > > > + pm_runtime_enable(dev);
> > > > +
> > > > + ret = v4l2_device_register(dev, &ceudev->v4l2_dev);
> > > > + if (ret)
> > > > + goto error_pm_disable;
> > > > +
> > > > + if (IS_ENABLED(C
Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
Hi Jacopo,
(CC'ing Sakari)
On Tuesday, 19 December 2017 13:57:42 EET jacopo mondi wrote:
> On Mon, Dec 11, 2017 at 06:15:23PM +0200, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > [snip]
> >
> >> +static int ceu_sensor_bound(struct v4l2_async_notifier *notifier,
> >> + struct v4l2_subdev *v4l2_sd,
> >> + struct v4l2_async_subdev *asd)
> >> +{
> >> + struct v4l2_device *v4l2_dev = notifier->v4l2_dev;
> >> + struct ceu_device *ceudev = v4l2_to_ceu(v4l2_dev);
> >> + struct ceu_subdev *ceu_sd = to_ceu_subdev(asd);
> >> +
> >> + if (video_is_registered(&ceudev->vdev)) {
> >> + v4l2_err(&ceudev->v4l2_dev,
> >> + "Video device registered before this sub-device.\n");
> >> + return -EBUSY;
> >
> > Can this happen ?
> >
> >> + }
> >> +
> >> + /* Assign subdevices in the order they appear */
> >> + ceu_sd->v4l2_sd = v4l2_sd;
> >> + ceudev->num_sd++;
> >> +
> >> + return 0;
> >> +}
> >> +
> > > +static int ceu_sensor_complete(struct v4l2_async_notifier *notifier)
> > > +{
> > > + struct v4l2_device *v4l2_dev = notifier->v4l2_dev;
> > > + struct ceu_device *ceudev = v4l2_to_ceu(v4l2_dev);
> > > + struct video_device *vdev = &ceudev->vdev;
> > > + struct vb2_queue *q = &ceudev->vb2_vq;
> > > + struct v4l2_subdev *v4l2_sd;
> > > + int ret;
> > > +
> > > + /* Initialize vb2 queue */
> > > + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> > > + q->io_modes = VB2_MMAP | VB2_USERPTR;
> >
> > No dmabuf ?
> >
> > > + q->drv_priv = ceudev;
> > > + q->ops = &ceu_videobuf_ops;
> > > + q->mem_ops = &vb2_dma_contig_memops;
> > > + q->buf_struct_size = sizeof(struct ceu_buffer);
> > > + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > > + q->lock = &ceudev->mlock;
> > > + q->dev = ceudev->v4l2_dev.dev;
> >
> > [snip]
> >
> > > +static int ceu_probe(struct platform_device *pdev)
> > > +{
> > > + struct device *dev = &pdev->dev;
> > > + struct ceu_device *ceudev;
> > > + struct resource *res;
> > > + void __iomem *base;
> > > + unsigned int irq;
> > > + int num_sd;
> > > + int ret;
> > > +
> > > + ceudev = kzalloc(sizeof(*ceudev), GFP_KERNEL);
> >
> > The memory is freed in ceu_vdev_release() as expected, but that will only
> > work if the video device is registered. If the subdevs are never bound,
> > the ceudev memory will be leaked if you unbind the CEU device from its
> > driver. In my opinion this calls for registering the video device at
> > probe time (although Hans disagrees).
> >
> > > + if (!ceudev)
> > > + return -ENOMEM;
> > > +
> > > + platform_set_drvdata(pdev, ceudev);
> > > + dev_set_drvdata(dev, ceudev);
> >
> > You don't need the second line, platform_set_drvdata() is a wrapper around
> > dev_set_drvdata().
> >
> > > + ceudev->dev = dev;
> > > +
> > > + INIT_LIST_HEAD(&ceudev->capture);
> > > + spin_lock_init(&ceudev->lock);
> > > + mutex_init(&ceudev->mlock);
> > > +
> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > + if (IS_ERR(res))
> > > + return PTR_ERR(res);
> >
> > No need for error handling here, devm_ioremap_resource() will check the
> > res
> > pointer.
> >
> > > + base = devm_ioremap_resource(dev, res);
> >
> > You can assign ceudev->base directly and remove the base local variable.
> >
> > > + if (IS_ERR(base))
> > > + return PTR_ERR(base);
> > > + ceudev->base = base;
> > > +
> > > + ret = platform_get_irq(pdev, 0);
> > > + if (ret < 0) {
> > > + dev_err(dev, "failed to get irq: %d\n", ret);
> > > + return ret;
> > > + }
> > > + irq = ret;
> > > +
> > > + ret = devm_request_irq(dev, irq, ceu_irq,
> > > +0, dev_name(dev), ceudev);
> > > + if (ret) {
> > > + dev_err(&pdev->dev, "Unable to register CEU interrupt.\n");
> > > + return ret;
> > > + }
> > > +
> > > + pm_suspend_ignore_children(dev, true);
> > > + pm_runtime_enable(dev);
> > > +
> > > + ret = v4l2_device_register(dev, &ceudev->v4l2_dev);
> > > + if (ret)
> > > + goto error_pm_disable;
> > > +
> > > + if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
> > > + num_sd = ceu_parse_dt(ceudev);
> > > + } else if (dev->platform_data) {
> > > + num_sd = ceu_parse_platform_data(ceudev, dev->platform_data);
> > > + } else {
> > > + dev_err(dev, "CEU platform data not set and no OF support\n");
> > > + ret = -EINVAL;
> > > + goto error_v4l2_unregister;
> > > + }
> > > +
> > > + if (num_sd < 0) {
> > > + ret = num_sd;
> > > + goto error_v4l2_unregister;
> > > + } else if (num_sd == 0)
> > > + return 0;
> >
> > You need braces around the second statement too.
>
> Ok, actually parse_dt() and parse_platform_data() behaves differently.
> The former returns error if no subdevices are connected to CEU, the
> latter returns 0. That's wrong.
>
Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
Hi Laurent,
a few more details on subdevice management
On Mon, Dec 11, 2017 at 06:15:23PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> [snip]
>
> > +static int ceu_sensor_bound(struct v4l2_async_notifier *notifier,
> > + struct v4l2_subdev *v4l2_sd,
> > + struct v4l2_async_subdev *asd)
> > +{
> > + struct v4l2_device *v4l2_dev = notifier->v4l2_dev;
> > + struct ceu_device *ceudev = v4l2_to_ceu(v4l2_dev);
> > + struct ceu_subdev *ceu_sd = to_ceu_subdev(asd);
> > +
> > + if (video_is_registered(&ceudev->vdev)) {
> > + v4l2_err(&ceudev->v4l2_dev,
> > +"Video device registered before this sub-device.\n");
> > + return -EBUSY;
>
> Can this happen ?
>
> > + }
> > +
> > + /* Assign subdevices in the order they appear */
> > + ceu_sd->v4l2_sd = v4l2_sd;
> > + ceudev->num_sd++;
> > +
> > + return 0;
> > +}
> > +
> > +static int ceu_sensor_complete(struct v4l2_async_notifier *notifier)
> > +{
> > + struct v4l2_device *v4l2_dev = notifier->v4l2_dev;
> > + struct ceu_device *ceudev = v4l2_to_ceu(v4l2_dev);
> > + struct video_device *vdev = &ceudev->vdev;
> > + struct vb2_queue *q = &ceudev->vb2_vq;
> > + struct v4l2_subdev *v4l2_sd;
> > + int ret;
> > +
> > + /* Initialize vb2 queue */
> > + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> > + q->io_modes = VB2_MMAP | VB2_USERPTR;
>
> No dmabuf ?
>
> > + q->drv_priv = ceudev;
> > + q->ops = &ceu_videobuf_ops;
> > + q->mem_ops = &vb2_dma_contig_memops;
> > + q->buf_struct_size = sizeof(struct ceu_buffer);
> > + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > + q->lock = &ceudev->mlock;
> > + q->dev = ceudev->v4l2_dev.dev;
>
> [snip]
>
> > +static int ceu_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct ceu_device *ceudev;
> > + struct resource *res;
> > + void __iomem *base;
> > + unsigned int irq;
> > + int num_sd;
> > + int ret;
> > +
> > + ceudev = kzalloc(sizeof(*ceudev), GFP_KERNEL);
>
> The memory is freed in ceu_vdev_release() as expected, but that will only work
> if the video device is registered. If the subdevs are never bound, the ceudev
> memory will be leaked if you unbind the CEU device from its driver. In my
> opinion this calls for registering the video device at probe time (although
> Hans disagrees).
>
> > + if (!ceudev)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, ceudev);
> > + dev_set_drvdata(dev, ceudev);
>
> You don't need the second line, platform_set_drvdata() is a wrapper around
> dev_set_drvdata().
>
> > + ceudev->dev = dev;
> > +
> > + INIT_LIST_HEAD(&ceudev->capture);
> > + spin_lock_init(&ceudev->lock);
> > + mutex_init(&ceudev->mlock);
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (IS_ERR(res))
> > + return PTR_ERR(res);
>
> No need for error handling here, devm_ioremap_resource() will check the res
> pointer.
>
> > + base = devm_ioremap_resource(dev, res);
>
> You can assign ceudev->base directly and remove the base local variable.
>
> > + if (IS_ERR(base))
> > + return PTR_ERR(base);
> > + ceudev->base = base;
> > +
> > + ret = platform_get_irq(pdev, 0);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to get irq: %d\n", ret);
> > + return ret;
> > + }
> > + irq = ret;
> > +
> > + ret = devm_request_irq(dev, irq, ceu_irq,
> > + 0, dev_name(dev), ceudev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Unable to register CEU interrupt.\n");
> > + return ret;
> > + }
> > +
> > + pm_suspend_ignore_children(dev, true);
> > + pm_runtime_enable(dev);
> > +
> > + ret = v4l2_device_register(dev, &ceudev->v4l2_dev);
> > + if (ret)
> > + goto error_pm_disable;
> > +
> > + if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
> > + num_sd = ceu_parse_dt(ceudev);
> > + } else if (dev->platform_data) {
> > + num_sd = ceu_parse_platform_data(ceudev, dev->platform_data);
> > + } else {
> > + dev_err(dev, "CEU platform data not set and no OF support\n");
> > + ret = -EINVAL;
> > + goto error_v4l2_unregister;
> > + }
> > +
> > + if (num_sd < 0) {
> > + ret = num_sd;
> > + goto error_v4l2_unregister;
> > + } else if (num_sd == 0)
> > + return 0;
>
> You need braces around the second statement too.
Ok, actually parse_dt() and parse_platform_data() behaves differently.
The former returns error if no subdevices are connected to CEU, the
latter returns 0. That's wrong.
I wonder what's the correct behavior here. Other mainline drivers I
looked into (pxa_camera and atmel-isc) behaves differently from each
other, so I guess this is up to eac
Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
Hi Jacopo,
On Monday, 18 December 2017 14:25:12 EET jacopo mondi wrote:
> On Mon, Dec 11, 2017 at 06:15:23PM +0200, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
>
> [snip]
>
> >> +
> >> +/**
> >> + * ceu_buffer - Link vb2 buffer to the list of available buffers
> >
> > If you use kerneldoc comments please make them compile. You need to
> > document the structure fields and function arguments.
>
> Ok, no kernel doc for internal structures then and no kernel doc for
> ugly comments you pointed out below
You can use kerneldoc if you want to, but if you do please make sure it
compiles :-)
> [snip]
>
> >> +/**
> >> + * ceu_soft_reset() - Software reset the CEU interface
> >> + */
> >> +static int ceu_soft_reset(struct ceu_device *ceudev)
> >> +{
> >> + unsigned int reset_done;
> >> + unsigned int i;
> >> +
> >> + ceu_write(ceudev, CEU_CAPSR, CEU_CAPSR_CPKIL);
> >> +
> >> + reset_done = 0;
> >> + for (i = 0; i < 1000 && !reset_done; i++) {
> >> + udelay(1);
> >> + if (!(ceu_read(ceudev, CEU_CSTSR) & CEU_CSTRST_CPTON))
> >> + reset_done++;
> >> + }
> >
> > How many iterations does this typically require ? Wouldn't a sleep be
> > better than a delay ? As far as I can tell the ceu_soft_reset() function
> > is only called with interrupts disabled (in interrupt context) from
> > ceu_capture() in an error path, and that code should be reworked to make
> > it possible to sleep if a reset takes too long.
>
> The HW manual does not provide any indication about absolute timings.
> I can empirically try and see, but that would just be a hint.
That's why I asked how many iterations it typically takes :-) A hint is enough
to start with, preferably on both SH and ARM SoCs.
> Also, the reset function is called in many places (runtime_pm
> suspend/resume) s_stream(0) and in error path of ceu_capture().
>
> In ceu_capture() we reset the interface if the previous frame was bad,
> and we do that before re-enabling the capture interrupt (so interrupts
> are not -disabled-, just the one we care about is not enabled yet..)
The ceu_capture() function is called from the driver's interrupt handler, so
interrupts are disabled in that code path.
> But that's not big deal, as if we fail there, we are about to abort
> capture anyhow and so if we miss some spurious capture interrupt it's
> ok...
>
> >> + if (!reset_done) {
> >> + v4l2_err(&ceudev->v4l2_dev, "soft reset time out\n");
> >
> > How about dev_err() instead ?
>
> Is dev_err/dev_dbg preferred over v4l2_err/v4l2_dbg? Is this because
> of dynamic debug?
Yes, and the fact that the V4L2 macros don't provide us anymore with much
compared to the dev_* macros.
> >> +
> >> +/**
> >> + * ceu_capture() - Trigger start of a capture sequence
> >> + *
> >> + * Return value doesn't reflect the success/failure to queue the new
> >> buffer,
> >> + * but rather the status of the previous capture.
> >> + */
> >> +static int ceu_capture(struct ceu_device *ceudev)
> >> +{
> >> + struct v4l2_pix_format_mplane *pix = &ceudev->v4l2_pix;
> >> + dma_addr_t phys_addr_top;
> >> + u32 status;
> >> +
> >> + /* Clean interrupt status and re-enable interrupts */
> >> + status = ceu_read(ceudev, CEU_CETCR);
> >> + ceu_write(ceudev, CEU_CEIER,
> >> +ceu_read(ceudev, CEU_CEIER) & ~CEU_CEIER_MASK);
> >> + ceu_write(ceudev, CEU_CETCR, ~status & CEU_CETCR_MAGIC);
> >> + ceu_write(ceudev, CEU_CEIER, CEU_CEIER_MASK);
> >
> > I wonder why there's a need to disable and reenable interrupts here.
>
> The original driver clearly said "The hardware is -very- picky about
> this sequence" and I got scared and nerver touched this.
How about experimenting to see how the hardware reacts ?
> Also, I very much dislike the CEU_CETRC_MAGIC mask, but again the old driver
> said "Acknoledge magical interrupt sources" and I was afraid to change it
> (I can rename it though, to something lioke CEU_CETCR_ALL_INT because that's
> what it is, a mask with all available interrupt source enabled).
I think renaming it is a good idea. Additionally, regardless of whether there
is any hidden interrupt source, the datasheet mentions for all reserved bits
that "The write value should always be 0". They should read as 0, but
masking them would be an additional safeguard.
Also not that on the RZ/A1 platform bit 22 is documented as reserved, so you
might want to compute the mask based on the CEU model.
If you have time you could add a debug print when an undocumented interrupt is
flagged and see if that happens for real.
> >> +
> >> +static irqreturn_t ceu_irq(int irq, void *data)
> >> +{
> >> + struct ceu_device *ceudev = data;
> >> + struct vb2_v4l2_buffer *vbuf;
> >> + struct ceu_buffer *buf;
> >> + int ret;
> >> +
> >> + spin_lock(&ceudev->lock);
> >> + vbuf = ceudev->active;
> >> + if (!vbuf)
> >> + /* Stale interrupt from a released buffer */
> >> + goto out;
> >
> > Shouldn't you at l
Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
Hi Hans, thanks for review comments On Wed, Dec 13, 2017 at 01:03:03PM +0100, Hans Verkuil wrote: > On 15/11/17 11:55, Jacopo Mondi wrote: > > Add driver for Renesas Capture Engine Unit (CEU). > > + > > + /* Register the video device */ > > + strncpy(vdev->name, DRIVER_NAME, strlen(DRIVER_NAME)); > > + vdev->v4l2_dev = v4l2_dev; > > + vdev->lock = &ceudev->mlock; > > + vdev->queue = &ceudev->vb2_vq; > > + vdev->ctrl_handler = v4l2_sd->ctrl_handler; > > + vdev->fops = &ceu_fops; > > + vdev->ioctl_ops = &ceu_ioctl_ops; > > + vdev->release = ceu_vdev_release; > > + vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE | > > Why MPLANE? It doesn't appear to be needed since there are no multiplane > (really: multibuffer) pixelformats defined. The driver support NV12/21 and NV16/61 as output pixel formats (along with single plane YUYV). NV* formats are semi-planar, as luma is stored in one buffer, while chrominances are stored together in a different one. Am I wrong? > > > > Regards, Thanks j > > Hans
Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
Hi Laurent,
thanks for review comments...
On Mon, Dec 11, 2017 at 06:15:23PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
[snip]
> > +
> > +/**
> > + * ceu_buffer - Link vb2 buffer to the list of available buffers
>
> If you use kerneldoc comments please make them compile. You need to document
> the structure fields and function arguments.
>
Ok, no kernel doc for internal structures then and no kernel doc for
ugly comments you pointed out below
[snip]
> > +/**
> > + * ceu_soft_reset() - Software reset the CEU interface
> > + */
> > +static int ceu_soft_reset(struct ceu_device *ceudev)
> > +{
> > + unsigned int reset_done;
> > + unsigned int i;
> > +
> > + ceu_write(ceudev, CEU_CAPSR, CEU_CAPSR_CPKIL);
> > +
> > + reset_done = 0;
> > + for (i = 0; i < 1000 && !reset_done; i++) {
> > + udelay(1);
> > + if (!(ceu_read(ceudev, CEU_CSTSR) & CEU_CSTRST_CPTON))
> > + reset_done++;
> > + }
>
> How many iterations does this typically require ? Wouldn't a sleep be better
> than a delay ? As far as I can tell the ceu_soft_reset() function is only
> called with interrupts disabled (in interrupt context) from ceu_capture() in
> an error path, and that code should be reworked to make it possible to sleep
> if a reset takes too long.
>
The HW manual does not provide any indication about absolute timings.
I can empirically try and see, but that would just be a hint.
Also, the reset function is called in many places (runtime_pm
suspend/resume) s_stream(0) and in error path of ceu_capture().
In ceu_capture() we reset the interface if the previous frame was bad,
and we do that before re-enabling the capture interrupt (so interrupts
are not -disabled-, just the one we care about is not enabled yet..)
But that's not big deal, as if we fail there, we are about to abort
capture anyhow and so if we miss some spurious capture interrupt it's
ok...
> > + if (!reset_done) {
> > + v4l2_err(&ceudev->v4l2_dev, "soft reset time out\n");
>
> How about dev_err() instead ?
Is dev_err/dev_dbg preferred over v4l2_err/v4l2_dbg? Is this because
of dynamic debug?
> > +
> > +/**
> > + * ceu_capture() - Trigger start of a capture sequence
> > + *
> > + * Return value doesn't reflect the success/failure to queue the new
> > buffer,
> > + * but rather the status of the previous capture.
> > + */
> > +static int ceu_capture(struct ceu_device *ceudev)
> > +{
> > + struct v4l2_pix_format_mplane *pix = &ceudev->v4l2_pix;
> > + dma_addr_t phys_addr_top;
> > + u32 status;
> > +
> > + /* Clean interrupt status and re-enable interrupts */
> > + status = ceu_read(ceudev, CEU_CETCR);
> > + ceu_write(ceudev, CEU_CEIER,
> > + ceu_read(ceudev, CEU_CEIER) & ~CEU_CEIER_MASK);
> > + ceu_write(ceudev, CEU_CETCR, ~status & CEU_CETCR_MAGIC);
> > + ceu_write(ceudev, CEU_CEIER, CEU_CEIER_MASK);
>
> I wonder why there's a need to disable and reenable interrupts here.
The original driver clearly said "The hardware is -very- picky about
this sequence" and I got scared and nerver touched this. Also, I very
much dislike the CEU_CETRC_MAGIC mask, but again the old driver said
"Acknoledge magical interrupt sources" and I was afraid to change it
(I can rename it though, to something lioke CEU_CETCR_ALL_INT because
that's what it is, a mask with all available interrupt source
enabled).
> > +
> > +static irqreturn_t ceu_irq(int irq, void *data)
> > +{
> > + struct ceu_device *ceudev = data;
> > + struct vb2_v4l2_buffer *vbuf;
> > + struct ceu_buffer *buf;
> > + int ret;
> > +
> > + spin_lock(&ceudev->lock);
> > + vbuf = ceudev->active;
> > + if (!vbuf)
> > + /* Stale interrupt from a released buffer */
> > + goto out;
>
> Shouldn't you at least clear the interrupt source (done at the beginning of
> the ceu_capture() function) in this case ? I'd move the handling of the
> interrupt status from ceu_capture() to here and pass the status to the capture
> function. Or even handle the status here completely, as status handling isn't
> needed when ceu_capture() is called from ceu_start_streaming().
I'll try to move interrupt management here, and use flags to tell to
ceu_capture() what happened
>
> > + /* Prepare a new 'active' buffer and trigger a new capture */
> > + buf = vb2_to_ceu(vbuf);
> > + vbuf->vb2_buf.timestamp = ktime_get_ns();
> > +
> > + if (!list_empty(&ceudev->capture)) {
> > + buf = list_first_entry(&ceudev->capture, struct ceu_buffer,
> > + queue);
> > + list_del(&buf->queue);
> > + ceudev->active = &buf->vb;
> > + } else {
> > + ceudev->active = NULL;
> > + }
> > +
> > + /*
> > +* If the new capture started successfully, mark the previous buffer
> > +* as "DONE".
> > +*/
> > + ret = ceu_capture(ceudev);
> > + if (!ret) {
> > + vbuf->field = ceudev->field;
> > + vbuf->sequence
Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
On 15/11/17 11:55, Jacopo Mondi wrote: > Add driver for Renesas Capture Engine Unit (CEU). > > The CEU interface supports capturing 'data' (YUV422) and 'images' > (NV[12|21|16|61]). > > This driver aims to replace the soc_camera based sh_mobile_ceu one. > > Tested with ov7670 camera sensor, providing YUYV_2X8 data on Renesas RZ > platform GR-Peach. > > Tested with ov7725 camera sensor on SH4 platform Migo-R. > > Signed-off-by: Jacopo Mondi > --- > drivers/media/platform/Kconfig |9 + > drivers/media/platform/Makefile |2 + > drivers/media/platform/renesas-ceu.c | 1768 > ++ > 3 files changed, 1779 insertions(+) > create mode 100644 drivers/media/platform/renesas-ceu.c > > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig > index 3c4f7fa..401caea 100644 > --- a/drivers/media/platform/Kconfig > +++ b/drivers/media/platform/Kconfig > @@ -144,6 +144,15 @@ config VIDEO_STM32_DCMI > To compile this driver as a module, choose M here: the module > will be called stm32-dcmi. > > +config VIDEO_RENESAS_CEU > + tristate "Renesas Capture Engine Unit (CEU) driver" > + depends on VIDEO_DEV && VIDEO_V4L2 && HAS_DMA > + depends on ARCH_SHMOBILE || ARCH_R7S72100 || COMPILE_TEST > + select VIDEOBUF2_DMA_CONTIG > + select V4L2_FWNODE > + ---help--- > + This is a v4l2 driver for the Renesas CEU Interface > + > source "drivers/media/platform/soc_camera/Kconfig" > source "drivers/media/platform/exynos4-is/Kconfig" > source "drivers/media/platform/am437x/Kconfig" > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile > index 327f80a..0d1f02b 100644 > --- a/drivers/media/platform/Makefile > +++ b/drivers/media/platform/Makefile > @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_CODA)+= coda/ > > obj-$(CONFIG_VIDEO_SH_VEU) += sh_veu.o > > +obj-$(CONFIG_VIDEO_RENESAS_CEU) += renesas-ceu.o > + > obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE) += m2m-deinterlace.o > > obj-$(CONFIG_VIDEO_MUX) += video-mux.o > diff --git a/drivers/media/platform/renesas-ceu.c > b/drivers/media/platform/renesas-ceu.c > new file mode 100644 > index 000..aaba3cd > --- /dev/null > +++ b/drivers/media/platform/renesas-ceu.c > @@ -0,0 +1,1768 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * V4L2 Driver for Renesas Capture Engine Unit (CEU) interface > + * > + * Copyright (C) 2017 Jacopo Mondi > + * > + * Based on soc-camera driver "soc_camera/sh_mobile_ceu_camera.c" > + * Copyright (C) 2008 Magnus Damm > + * > + * Based on V4L2 Driver for PXA camera host - "pxa_camera.c", > + * Copyright (C) 2006, Sascha Hauer, Pengutronix > + * Copyright (C) 2008, Guennadi Liakhovetski > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#define DRIVER_NAME "renesas-ceu" > + > +/* > > + * CEU registers offsets and masks > + */ > +#define CEU_CAPSR0x00 /* Capture start register */ > +#define CEU_CAPCR0x04 /* Capture control register*/ > +#define CEU_CAMCR0x08 /* Capture interface control register */ > +#define CEU_CAMOR0x10 /* Capture interface offset register */ > +#define CEU_CAPWR0x14 /* Capture interface width register*/ > +#define CEU_CAIFR0x18 /* Capture interface input format register */ > +#define CEU_CRCNTR 0x28 /* CEU register control register */ > +#define CEU_CRCMPR 0x2c /* CEU register forcible control register */ > +#define CEU_CFLCR0x30 /* Capture filter control register */ > +#define CEU_CFSZR0x34 /* Capture filter size clip register */ > +#define CEU_CDWDR0x38 /* Capture destination width register */ > +#define CEU_CDAYR0x3c /* Capture data address Y register */ > +#define CEU_CDACR0x40 /* Capture data address C register */ > +#define CEU_CFWCR0x5c /* Firewall operation control register */ > +#define CEU_CDOCR0x64 /* Capture data output control register*/ > +#define CEU_CEIER0x70 /* Capture event interrupt enable register */ > +#define CEU_CETCR0x74 /* Capture event flag clear register */ > +#define CEU_CSTSR0x7c /* Capture status register */ > +#define CEU_CSRT
Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
Hi Jacopo,
Thank you for the patch.
On Wednesday, 15 November 2017 12:55:56 EET Jacopo Mondi wrote:
> Add driver for Renesas Capture Engine Unit (CEU).
>
> The CEU interface supports capturing 'data' (YUV422) and 'images'
> (NV[12|21|16|61]).
>
> This driver aims to replace the soc_camera based sh_mobile_ceu one.
s/soc_camera based/soc_camera-based/
> Tested with ov7670 camera sensor, providing YUYV_2X8 data on Renesas RZ
> platform GR-Peach.
>
> Tested with ov7725 camera sensor on SH4 platform Migo-R.
>
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/platform/Kconfig |9 +
> drivers/media/platform/Makefile |2 +
> drivers/media/platform/renesas-ceu.c | 1768 +++
> 3 files changed, 1779 insertions(+)
> create mode 100644 drivers/media/platform/renesas-ceu.c
[snip]
> diff --git a/drivers/media/platform/renesas-ceu.c
> b/drivers/media/platform/renesas-ceu.c new file mode 100644
> index 000..aaba3cd
> --- /dev/null
> +++ b/drivers/media/platform/renesas-ceu.c
> @@ -0,0 +1,1768 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * V4L2 Driver for Renesas Capture Engine Unit (CEU) interface
> + *
> + * Copyright (C) 2017 Jacopo Mondi
> + *
> + * Based on soc-camera driver "soc_camera/sh_mobile_ceu_camera.c"
> + * Copyright (C) 2008 Magnus Damm
> + *
> + * Based on V4L2 Driver for PXA camera host - "pxa_camera.c",
> + * Copyright (C) 2006, Sascha Hauer, Pengutronix
> + * Copyright (C) 2008, Guennadi Liakhovetski
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
You can use an SPDX header instead of a copyright text. Start the file with
// SPDX-License-Identifier: GPL-2.0+
and remove this paragraph.
> + */
[snip]
> +/* mbus configuration flags */
> +#define CEU_BUS_FLAGS (V4L2_MBUS_MASTER | \
> + V4L2_MBUS_PCLK_SAMPLE_RISING | \
> + V4L2_MBUS_HSYNC_ACTIVE_HIGH | \
> + V4L2_MBUS_HSYNC_ACTIVE_LOW | \
> + V4L2_MBUS_VSYNC_ACTIVE_HIGH | \
> + V4L2_MBUS_VSYNC_ACTIVE_LOW | \
> + V4L2_MBUS_DATA_ACTIVE_HIGH)
> +
> +#define CEU_MAX_WIDTH2560
> +#define CEU_MAX_HEIGHT 1920
> +#define CEU_W_MAX(w) ((w) < CEU_MAX_WIDTH ? (w) : CEU_MAX_WIDTH)
> +#define CEU_H_MAX(h) ((h) < CEU_MAX_HEIGHT ? (h) : CEU_MAX_HEIGHT)
> +
> +/*
> + * CEU formats
> + */
> +
> +/**
> + * ceu_bus_fmt - describe a 8-bits yuyv format the sensor can produce
> + *
> + * @mbus_code: bus format code
> + * @fmt_order: CEU_CAMCR.DTARY ordering of input components (Y, Cb, Cr)
> + * @fmt_order_swap: swapped CEU_CAMCR.DTARY ordering of input components
> + * (Y, Cr, Cb)
> + * @swapped: does Cr appear before Cb?
> + * @bps: number of bits sent over bus for each sample
> + * @bpp: number of bits per pixels unit
> + */
> +struct ceu_mbus_fmt {
> + u32 mbus_code;
> + u32 fmt_order;
> + u32 fmt_order_swap;
> + boolswapped;
> + u8 bps;
> + u8 bpp;
> +};
> +
> +/**
> + * ceu_buffer - Link vb2 buffer to the list of available buffers
If you use kerneldoc comments please make them compile. You need to document
the structure fields and function arguments.
> + */
> +struct ceu_buffer {
> + struct vb2_v4l2_buffer vb;
> + struct list_head queue;
> +};
> +
> +static inline struct ceu_buffer *vb2_to_ceu(struct vb2_v4l2_buffer *vbuf)
> +{
> + return container_of(vbuf, struct ceu_buffer, vb);
> +}
> +
> +/**
> + * ceu_subdev - Wraps v4l2 sub-device and provides async subdevice.
> + */
> +struct ceu_subdev {
> + struct v4l2_subdev *v4l2_sd;
> + struct v4l2_async_subdev asd;
> +
> + /* per-subdevice mbus configuration options */
> + unsigned int mbus_flags;
> + struct ceu_mbus_fmt mbus_fmt;
> +};
> +
> +static struct ceu_subdev *to_ceu_subdev(struct v4l2_async_subdev *asd)
> +{
> + return container_of(asd, struct ceu_subdev, asd);
> +}
> +
> +/**
> + * ceu_device - CEU device instance
> + */
> +struct ceu_device {
> + struct device *dev;
> + struct video_device vdev;
> + struct v4l2_device v4l2_dev;
> +
> + /* subdevices descriptors */
> + struct ceu_subdev *subdevs;
> + /* the subdevice currently in use */
> + struct ceu_subdev *sd;
> + unsigned intsd_index;
> + unsigned intnum_sd;
> +
> + /* currently configured field and pixel format */
> + enum v4l2_field field;
> + struct v4l2_pix_format_mplane v4l2_pix;
> +
> + /* async subdev notification helpers */
> + struct v4l2_async_notifier notifier;
> + /* pointers to "struct ceu_subdevice -> asd" *
Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
Hi Sakari,
On Friday, 17 November 2017 02:36:51 EET Sakari Ailus wrote:
> On Wed, Nov 15, 2017 at 03:25:11PM +0100, jacopo mondi wrote:
> > On Wed, Nov 15, 2017 at 02:45:51PM +0200, Sakari Ailus wrote:
> >> Hi Jacopo,
> >>
> >> Could you remove the original driver and send the patch using git
> >> send-email -C ? That way a single patch would address converting it to a
> >> proper V4L2 driver as well as move it to the correct location. The
> >> changes would be easier to review that way since then, well, it'd be
> >> easier to see the changes. :-)
> >
> > Actually I prefer not to remove the existing driver at the moment. See
> > the cover letter for reasons why not to do so right now...
>
> So it's about testing mostly? Does someone (possibly you) have those boards
> to test? I'd like to see this patchset to remove that last remaining SoC
> camera bridge driver. :-)
Unfortunately there's also drivers/media/platform/pxa-camera.c :-(
> > Also, there's not that much code from the old driver in here, surely
> > less than the default 50% -C and -M options of 'git format-patch' use
> > as a threshold for detecting copies iirc..
>
> Oh, if that's so, then makes sense to review it as a new driver.
Yes, unfortunately the drivers are too different. Jacopo started developing an
incremental patch series to move the driver away from soc-camera, but in the
end we decided to stop following that path as it was too painful. It's easier
to review a new driver in this case.
> > I would prefer this to be reviewed as new driver, I know it's a bit
> > more painful, but irq handler and a couple of other routines apart,
> > there's not that much code shared between the two...
> >
> >> The same goes for the two V4L2 SoC camera sensor / video decoder drivers
> >> at the end of the set.
> >
> > Also in this case I prefer not to remove existing code, as long as
> > there are platforms using it..
>
> Couldn't they use this driver instead?
>
> >> On Wed, Nov 15, 2017 at 11:55:56AM +0100, Jacopo Mondi wrote:
> >>> Add driver for Renesas Capture Engine Unit (CEU).
> >>>
> >>> The CEU interface supports capturing 'data' (YUV422) and 'images'
> >>> (NV[12|21|16|61]).
> >>>
> >>> This driver aims to replace the soc_camera based sh_mobile_ceu one.
> >>>
> >>> Tested with ov7670 camera sensor, providing YUYV_2X8 data on Renesas
> >>> RZ platform GR-Peach.
> >>>
> >>> Tested with ov7725 camera sensor on SH4 platform Migo-R.
> >>
> >> Nice!
> >>
> >>> Signed-off-by: Jacopo Mondi
> >>> ---
> >>> +#include
> >>
> >> Do you need this header? There would seem some that I wouldn't expect to
> >> be needed below, such as linux/init.h.
> >
> > It's probably a leftover, I'll remove it...
> >
> > [snip]
> >
> >>> +#if IS_ENABLED(CONFIG_OF)
> >>> +static const struct of_device_id ceu_of_match[] = {
> >>> + { .compatible = "renesas,renesas-ceu" },
> >>
> >> Even if you add support for new hardware, shouldn't you maintain support
> >> for renesas,sh-mobile-ceu?
> >
> > As you noticed already, the old driver did not support OF, so there
> > are no compatibility issues here
>
> Yeah, I realised that only after reviewing this patch.
>
> It'd be Super-cool if someone did the DT conversion. Perhaps Laurent? ;-)
Or you ? :-)
--
Regards,
Laurent Pinchart
Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
Hi Sakari! On Sat, Nov 25, 2017 at 05:56:14PM +0200, Sakari Ailus wrote: > On Fri, Nov 17, 2017 at 10:33:55AM +0100, jacopo mondi wrote: > > Hi Sakari! > > [snip] > > I would like to make sure we're all on the same page with this. My > > preference would be: > > > > 1) Have renesas-ceu.c driver merged with Migo-R ported to use this new > > driver as an 'example'. > > 2) Do not remove any of the existing soc_camera code at this point > > 3) Port all other 4 SH users of sh_mobile_ceu_camera to use the now > > merged renesas-ceu driver > > 4) Remove sh_mobile_ceu_camera and soc_camera sensor drivers whose > > only users were those 4 SH boards > > 5) Remove soc_camera completely. For my understanding there are some > > PXA platforms still using soc_camera provided utilities somewhere. > > Hans knows better, but we can discuss this once we'll get there. > > The first point here is basically done by this patchset and your intent > would be to proceed with the rest, right? Yep, you're right! > > The above seems good; what I wanted to say was that I'd like to avoid > ending up in a permanent situation where some hardware works with the new > driver and some will continue to use the old one. I hope that being the last users of soc_camera, there will be enough motivations to complete all the above points :) Thanks j > > -- > Kind regards, > > Sakari Ailus > e-mail: sakari.ai...@iki.fi
Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
On Fri, Nov 17, 2017 at 10:33:55AM +0100, jacopo mondi wrote: > Hi Sakari! > > On Fri, Nov 17, 2017 at 02:36:51AM +0200, Sakari Ailus wrote: > > Hi Jacopo, > > > > On Wed, Nov 15, 2017 at 03:25:11PM +0100, jacopo mondi wrote: > > > Hi Sakari, > > >thanks for review! > > > > You're welcome! > > > > > On Wed, Nov 15, 2017 at 02:45:51PM +0200, Sakari Ailus wrote: > > > > Hi Jacopo, > > > > > > > > Could you remove the original driver and send the patch using git > > > > send-email -C ? That way a single patch would address converting it to a > > > > proper V4L2 driver as well as move it to the correct location. The > > > > changes > > > > would be easier to review that way since then, well, it'd be easier to > > > > see > > > > the changes. :-) > > > > > > Actually I prefer not to remove the existing driver at the moment. See > > > the cover letter for reasons why not to do so right now... > > > > So it's about testing mostly? Does someone (possibly you) have those boards > > to test? I'd like to see this patchset to remove that last remaining SoC > > camera bridge driver. :-) > > Well, we agreed that for most of those platforms, compile testing it > would be enough (let's believe in "if it compiles, it works"). I > personally don't have access to those boards, and frankly I'm not even > sure there are many of them around these days (I guess most of them > are not even produced anymore). > > > > > > > > > Also, there's not that much code from the old driver in here, surely > > > less than the default 50% -C and -M options of 'git format-patch' use > > > as a threshold for detecting copies iirc.. > > > > Oh, if that's so, then makes sense to review it as a new driver. > > thanks :) > > > > > > > > > > The same goes for the two V4L2 SoC camera sensor / video decoder > > > > drivers at > > > > the end of the set. > > > > > > > > > > Also in this case I prefer not to remove existing code, as long as > > > there are platforms using it.. > > > > Couldn't they use this driver instead? > > Oh, they will eventually, I hope :) > > I would like to make sure we're all on the same page with this. My > preference would be: > > 1) Have renesas-ceu.c driver merged with Migo-R ported to use this new > driver as an 'example'. > 2) Do not remove any of the existing soc_camera code at this point > 3) Port all other 4 SH users of sh_mobile_ceu_camera to use the now > merged renesas-ceu driver > 4) Remove sh_mobile_ceu_camera and soc_camera sensor drivers whose > only users were those 4 SH boards > 5) Remove soc_camera completely. For my understanding there are some > PXA platforms still using soc_camera provided utilities somewhere. > Hans knows better, but we can discuss this once we'll get there. The first point here is basically done by this patchset and your intent would be to proceed with the rest, right? The above seems good; what I wanted to say was that I'd like to avoid ending up in a permanent situation where some hardware works with the new driver and some will continue to use the old one. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
Hi Sakari! On Fri, Nov 17, 2017 at 02:36:51AM +0200, Sakari Ailus wrote: > Hi Jacopo, > > On Wed, Nov 15, 2017 at 03:25:11PM +0100, jacopo mondi wrote: > > Hi Sakari, > >thanks for review! > > You're welcome! > > > On Wed, Nov 15, 2017 at 02:45:51PM +0200, Sakari Ailus wrote: > > > Hi Jacopo, > > > > > > Could you remove the original driver and send the patch using git > > > send-email -C ? That way a single patch would address converting it to a > > > proper V4L2 driver as well as move it to the correct location. The changes > > > would be easier to review that way since then, well, it'd be easier to see > > > the changes. :-) > > > > Actually I prefer not to remove the existing driver at the moment. See > > the cover letter for reasons why not to do so right now... > > So it's about testing mostly? Does someone (possibly you) have those boards > to test? I'd like to see this patchset to remove that last remaining SoC > camera bridge driver. :-) Well, we agreed that for most of those platforms, compile testing it would be enough (let's believe in "if it compiles, it works"). I personally don't have access to those boards, and frankly I'm not even sure there are many of them around these days (I guess most of them are not even produced anymore). > > > > > Also, there's not that much code from the old driver in here, surely > > less than the default 50% -C and -M options of 'git format-patch' use > > as a threshold for detecting copies iirc.. > > Oh, if that's so, then makes sense to review it as a new driver. thanks :) > > > > > > The same goes for the two V4L2 SoC camera sensor / video decoder drivers > > > at > > > the end of the set. > > > > > > > Also in this case I prefer not to remove existing code, as long as > > there are platforms using it.. > > Couldn't they use this driver instead? Oh, they will eventually, I hope :) I would like to make sure we're all on the same page with this. My preference would be: 1) Have renesas-ceu.c driver merged with Migo-R ported to use this new driver as an 'example'. 2) Do not remove any of the existing soc_camera code at this point 3) Port all other 4 SH users of sh_mobile_ceu_camera to use the now merged renesas-ceu driver 4) Remove sh_mobile_ceu_camera and soc_camera sensor drivers whose only users were those 4 SH boards 5) Remove soc_camera completely. For my understanding there are some PXA platforms still using soc_camera provided utilities somewhere. Hans knows better, but we can discuss this once we'll get there. Let me know if this is ok for everyone. Thanks j
Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
Hi Jacopo,
On Wed, Nov 15, 2017 at 03:25:11PM +0100, jacopo mondi wrote:
> Hi Sakari,
>thanks for review!
You're welcome!
> On Wed, Nov 15, 2017 at 02:45:51PM +0200, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > Could you remove the original driver and send the patch using git
> > send-email -C ? That way a single patch would address converting it to a
> > proper V4L2 driver as well as move it to the correct location. The changes
> > would be easier to review that way since then, well, it'd be easier to see
> > the changes. :-)
>
> Actually I prefer not to remove the existing driver at the moment. See
> the cover letter for reasons why not to do so right now...
So it's about testing mostly? Does someone (possibly you) have those boards
to test? I'd like to see this patchset to remove that last remaining SoC
camera bridge driver. :-)
>
> Also, there's not that much code from the old driver in here, surely
> less than the default 50% -C and -M options of 'git format-patch' use
> as a threshold for detecting copies iirc..
Oh, if that's so, then makes sense to review it as a new driver.
>
> I would prefer this to be reviewed as new driver, I know it's a bit
> more painful, but irq handler and a couple of other routines apart,
> there's not that much code shared between the two...
>
> >
> > The same goes for the two V4L2 SoC camera sensor / video decoder drivers at
> > the end of the set.
> >
>
> Also in this case I prefer not to remove existing code, as long as
> there are platforms using it..
Couldn't they use this driver instead?
>
> > On Wed, Nov 15, 2017 at 11:55:56AM +0100, Jacopo Mondi wrote:
> > > Add driver for Renesas Capture Engine Unit (CEU).
> > >
> > > The CEU interface supports capturing 'data' (YUV422) and 'images'
> > > (NV[12|21|16|61]).
> > >
> > > This driver aims to replace the soc_camera based sh_mobile_ceu one.
> > >
> > > Tested with ov7670 camera sensor, providing YUYV_2X8 data on Renesas RZ
> > > platform GR-Peach.
> > >
> > > Tested with ov7725 camera sensor on SH4 platform Migo-R.
> >
> > Nice!
> >
> > >
> > > Signed-off-by: Jacopo Mondi
> > > ---
> > > +#include
> >
> > Do you need this header? There would seem some that I wouldn't expect to be
> > needed below, such as linux/init.h.
>
> It's probably a leftover, I'll remove it...
>
> [snip]
> >
> > > +#if IS_ENABLED(CONFIG_OF)
> > > +static const struct of_device_id ceu_of_match[] = {
> > > + { .compatible = "renesas,renesas-ceu" },
> >
> > Even if you add support for new hardware, shouldn't you maintain support
> > for renesas,sh-mobile-ceu?
> >
>
> As you noticed already, the old driver did not support OF, so there
> are no compatibility issues here
Yeah, I realised that only after reviewing this patch.
It'd be Super-cool if someone did the DT conversion. Perhaps Laurent? ;-)
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
Hi Sakari,
thanks for review!
On Wed, Nov 15, 2017 at 02:45:51PM +0200, Sakari Ailus wrote:
> Hi Jacopo,
>
> Could you remove the original driver and send the patch using git
> send-email -C ? That way a single patch would address converting it to a
> proper V4L2 driver as well as move it to the correct location. The changes
> would be easier to review that way since then, well, it'd be easier to see
> the changes. :-)
Actually I prefer not to remove the existing driver at the moment. See
the cover letter for reasons why not to do so right now...
Also, there's not that much code from the old driver in here, surely
less than the default 50% -C and -M options of 'git format-patch' use
as a threshold for detecting copies iirc..
I would prefer this to be reviewed as new driver, I know it's a bit
more painful, but irq handler and a couple of other routines apart,
there's not that much code shared between the two...
>
> The same goes for the two V4L2 SoC camera sensor / video decoder drivers at
> the end of the set.
>
Also in this case I prefer not to remove existing code, as long as
there are platforms using it..
> On Wed, Nov 15, 2017 at 11:55:56AM +0100, Jacopo Mondi wrote:
> > Add driver for Renesas Capture Engine Unit (CEU).
> >
> > The CEU interface supports capturing 'data' (YUV422) and 'images'
> > (NV[12|21|16|61]).
> >
> > This driver aims to replace the soc_camera based sh_mobile_ceu one.
> >
> > Tested with ov7670 camera sensor, providing YUYV_2X8 data on Renesas RZ
> > platform GR-Peach.
> >
> > Tested with ov7725 camera sensor on SH4 platform Migo-R.
>
> Nice!
>
> >
> > Signed-off-by: Jacopo Mondi
> > ---
> > +#include
>
> Do you need this header? There would seem some that I wouldn't expect to be
> needed below, such as linux/init.h.
It's probably a leftover, I'll remove it...
[snip]
>
> > +#if IS_ENABLED(CONFIG_OF)
> > +static const struct of_device_id ceu_of_match[] = {
> > + { .compatible = "renesas,renesas-ceu" },
>
> Even if you add support for new hardware, shouldn't you maintain support
> for renesas,sh-mobile-ceu?
>
As you noticed already, the old driver did not support OF, so there
are no compatibility issues here
Thanks
j
Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver
Hi Jacopo, Could you remove the original driver and send the patch using git send-email -C ? That way a single patch would address converting it to a proper V4L2 driver as well as move it to the correct location. The changes would be easier to review that way since then, well, it'd be easier to see the changes. :-) The same goes for the two V4L2 SoC camera sensor / video decoder drivers at the end of the set. On Wed, Nov 15, 2017 at 11:55:56AM +0100, Jacopo Mondi wrote: > Add driver for Renesas Capture Engine Unit (CEU). > > The CEU interface supports capturing 'data' (YUV422) and 'images' > (NV[12|21|16|61]). > > This driver aims to replace the soc_camera based sh_mobile_ceu one. > > Tested with ov7670 camera sensor, providing YUYV_2X8 data on Renesas RZ > platform GR-Peach. > > Tested with ov7725 camera sensor on SH4 platform Migo-R. Nice! > > Signed-off-by: Jacopo Mondi > --- > drivers/media/platform/Kconfig |9 + > drivers/media/platform/Makefile |2 + > drivers/media/platform/renesas-ceu.c | 1768 > ++ > 3 files changed, 1779 insertions(+) > create mode 100644 drivers/media/platform/renesas-ceu.c > > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig > index 3c4f7fa..401caea 100644 > --- a/drivers/media/platform/Kconfig > +++ b/drivers/media/platform/Kconfig > @@ -144,6 +144,15 @@ config VIDEO_STM32_DCMI > To compile this driver as a module, choose M here: the module > will be called stm32-dcmi. > > +config VIDEO_RENESAS_CEU > + tristate "Renesas Capture Engine Unit (CEU) driver" > + depends on VIDEO_DEV && VIDEO_V4L2 && HAS_DMA > + depends on ARCH_SHMOBILE || ARCH_R7S72100 || COMPILE_TEST > + select VIDEOBUF2_DMA_CONTIG > + select V4L2_FWNODE > + ---help--- > + This is a v4l2 driver for the Renesas CEU Interface > + > source "drivers/media/platform/soc_camera/Kconfig" > source "drivers/media/platform/exynos4-is/Kconfig" > source "drivers/media/platform/am437x/Kconfig" > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile > index 327f80a..0d1f02b 100644 > --- a/drivers/media/platform/Makefile > +++ b/drivers/media/platform/Makefile > @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_CODA)+= coda/ > > obj-$(CONFIG_VIDEO_SH_VEU) += sh_veu.o > > +obj-$(CONFIG_VIDEO_RENESAS_CEU) += renesas-ceu.o > + > obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE) += m2m-deinterlace.o > > obj-$(CONFIG_VIDEO_MUX) += video-mux.o > diff --git a/drivers/media/platform/renesas-ceu.c > b/drivers/media/platform/renesas-ceu.c > new file mode 100644 > index 000..aaba3cd > --- /dev/null > +++ b/drivers/media/platform/renesas-ceu.c > @@ -0,0 +1,1768 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * V4L2 Driver for Renesas Capture Engine Unit (CEU) interface > + * > + * Copyright (C) 2017 Jacopo Mondi > + * > + * Based on soc-camera driver "soc_camera/sh_mobile_ceu_camera.c" > + * Copyright (C) 2008 Magnus Damm > + * > + * Based on V4L2 Driver for PXA camera host - "pxa_camera.c", > + * Copyright (C) 2006, Sascha Hauer, Pengutronix > + * Copyright (C) 2008, Guennadi Liakhovetski > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#include Do you need this header? There would seem some that I wouldn't expect to be needed below, such as linux/init.h. > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#define DRIVER_NAME "renesas-ceu" > + > +/* > > + * CEU registers offsets and masks > + */ > +#define CEU_CAPSR0x00 /* Capture start register */ > +#define CEU_CAPCR0x04 /* Capture control register*/ > +#define CEU_CAMCR0x08 /* Capture interface control register */ > +#define CEU_CAMOR0x10 /* Capture interface offset register */ > +#define CEU_CAPWR0x14 /* Capture interface width register*/ > +#define CEU_CAIFR0x18 /* Capture interface input format register */ > +#define CEU_CRCNTR 0x28 /* CEU register control register */ > +#define CEU_CRCMPR 0x2c /* CEU register forcible control register */ > +#define CEU_CFLCR0x30 /* Capture filter control register */ > +#define CEU_CFSZR0x34 /* Capture filter size clip register */ > +#define CEU_CDWDR0x38 /* Capture destination width
