Re: [PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers

[Sylwester Nawrocki]

Hi Laurent,

On 05/05/2011 02:25 PM, Laurent Pinchart wrote:
> On Thursday 05 May 2011 11:33:27 Sylwester Nawrocki wrote:
>> On 05/04/2011 02:00 PM, Laurent Pinchart wrote:
>>> On Tuesday 03 May 2011 20:07:43 Sylwester Nawrocki wrote:
 On 05/03/2011 11:16 AM, Laurent Pinchart wrote:
> On Thursday 21 April 2011 17:21:04 Sylwester Nawrocki wrote:
>>> [snip]
>>>
>> +struct media_pad pads[CSIS_PADS_NUM];
>> +struct v4l2_subdev sd;
>> +struct platform_device *pdev;
>> +struct resource *regs_res;
>> +void __iomem *regs;
>> +struct clk *clock[NUM_CSIS_CLOCKS];
>> +int irq;
>> +struct regulator *supply;
>> +u32 flags;
>> +/* Common format for the source and sink pad. */
>> +const struct csis_pix_format *csis_fmt;
>> +struct v4l2_mbus_framefmt mf[CSIS_NUM_FMTS];
>
> As try formats are stored in the file handle, and as the formats on the
> sink and source pads are identical, a single v4l2_mbus_framefmt will do
> here.

 Ok. How about a situation when the caller never provides a file handle?
 Is it not supposed to happen?
>>>
>>> Good question :-) The subdev pad-level operations have been designed with
>>> a userspace interface in mind, so they require a file handle to store
>>> try the formats (and crop rectangles).
>>>
 For V4L2_SUBDEV_FORMAT_TRY, should set_fmt just abandon storing the
 format and should get_fmt just return -EINVAL when passed fh == NULL ?
>>>
>>> For such a simple subdev, that should work as a workaround, yes. You can
>>> use it as a temporary solution at least.
>>>
 Or should the host driver allocate the file handle just for the sake of
 set_fmt/get_fmt calls (assuming that cropping ops are not supported
 by the subdev) ?
>>>
>>> That's another solution. We could also pass an internal structure that
>>> contains formats and crop rectangles to the pad operations handlers,
>>> instead of passing the whole file handle. Do you think that would be
>>> better ?
>>
>> So it would then be an additional argument for the pad-level operations ?
> 
> It would replace the file handle argument.
> 
>> Perhaps that would make sense when both format and crop information is
>> needed at the same time in the subdev driver. However this would only be
>> required for TRY formats/crop rectangles which wouldn't be supported anyway
>> because of missing file handle.. or I missed something?
> 
> The reason why we pass the file handle to the pad operations is to let 
> drivers 
> access formats/crop try settings that are stored in the file handle. If we 
> moved those settings to a separate structure, and embedded that structure 
> into 
> the file handle structure, we could pass &fh->settings instead of fh to the 
> pad operations. Drivers that want to call pad operations would then need to 
> allocate a settings structure, instead of a complete fake fh.

I see, that sounds like a cleanest solution of the problem.

> 
>> Furthermore I assume more complex pipelines will be handled in user space
> 
> The pad-level API has been designed to get/set formats/crop settings directly 
> from userspace, not from inside the kernel, so that would certainly work.
> 
>> and the host drivers could store format and crop information locally
>> directly from v4l2_subdev_format and v4l2_subdev_crop data structures.
> 
> I'm not sure to understand what you mean there.

I meant that the adjusted format/crop rectangle is still returned in the
pad operations, through the last argument; in struct v4l2_subdev_format::format
or struct v4l2_subdev_crop::rect and these can be queried and interpreted by
a host driver. 
But as you explain purpose of the fh is to aid subdev, not the host drivers.

> 
>>> Quoting one of your comments below,
>>>
>>> x--- FIMC_0 (/dev/video1)
>>>  
>>>  SENSOR -> MIPI_CSIS  --|
>>>  
>>> x--- FIMC_1 (/dev/video3)
>>>
>>> How do you expect to configure the MIPI_CSIS block from the FIMC_0 and
>>> FIMC_1 blocks, without any help from userspace ? Conflicts will need to
>>> be handled, and the best way to handle them is to have userspace
>>> configuring the MIPI_CSIS explicitly.
>>
>> My priority is to make work the configurations without device nodes at
>> sensor and MIPI CSIS subdevs.
>>
>> I agree it would be best to leave the negotiation logic to user space,
>> however I need to assure the regular V4L2 application also can use the
>> driver.
>>
>> My idea was to only try format at CSI slave and sensor subdevs when S_FMT
>> is called on a video node. So the sensor and CSIS constraints are taken
>> into account.
>>
>> Then from VIDIOC_STREAMON, formats at pipeline elements would be set on
>> subdevs without device node or validated on subdevs providing a device
>> node.
> 
> For subdevs without device nodes, why don't you set the active format 
> directly 
> when S_FMT is called,

Re: [PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers

[Laurent Pinchart]

Hi Sylwester,

On Thursday 05 May 2011 11:33:27 Sylwester Nawrocki wrote:
> On 05/04/2011 02:00 PM, Laurent Pinchart wrote:
> > On Tuesday 03 May 2011 20:07:43 Sylwester Nawrocki wrote:
> >> On 05/03/2011 11:16 AM, Laurent Pinchart wrote:
> >>> On Thursday 21 April 2011 17:21:04 Sylwester Nawrocki wrote:
> > [snip]
> > 
>  +struct media_pad pads[CSIS_PADS_NUM];
>  +struct v4l2_subdev sd;
>  +struct platform_device *pdev;
>  +struct resource *regs_res;
>  +void __iomem *regs;
>  +struct clk *clock[NUM_CSIS_CLOCKS];
>  +int irq;
>  +struct regulator *supply;
>  +u32 flags;
>  +/* Common format for the source and sink pad. */
>  +const struct csis_pix_format *csis_fmt;
>  +struct v4l2_mbus_framefmt mf[CSIS_NUM_FMTS];
> >>> 
> >>> As try formats are stored in the file handle, and as the formats on the
> >>> sink and source pads are identical, a single v4l2_mbus_framefmt will do
> >>> here.
> >> 
> >> Ok. How about a situation when the caller never provides a file handle?
> >> Is it not supposed to happen?
> > 
> > Good question :-) The subdev pad-level operations have been designed with
> > a userspace interface in mind, so they require a file handle to store
> > try the formats (and crop rectangles).
> > 
> >> For V4L2_SUBDEV_FORMAT_TRY, should set_fmt just abandon storing the
> >> format and should get_fmt just return -EINVAL when passed fh == NULL ?
> > 
> > For such a simple subdev, that should work as a workaround, yes. You can
> > use it as a temporary solution at least.
> > 
> >> Or should the host driver allocate the file handle just for the sake of
> >> set_fmt/get_fmt calls (assuming that cropping ops are not supported
> >> by the subdev) ?
> > 
> > That's another solution. We could also pass an internal structure that
> > contains formats and crop rectangles to the pad operations handlers,
> > instead of passing the whole file handle. Do you think that would be
> > better ?
> 
> So it would then be an additional argument for the pad-level operations ?

It would replace the file handle argument.

> Perhaps that would make sense when both format and crop information is
> needed at the same time in the subdev driver. However this would only be
> required for TRY formats/crop rectangles which wouldn't be supported anyway
> because of missing file handle.. or I missed something?

The reason why we pass the file handle to the pad operations is to let drivers 
access formats/crop try settings that are stored in the file handle. If we 
moved those settings to a separate structure, and embedded that structure into 
the file handle structure, we could pass &fh->settings instead of fh to the 
pad operations. Drivers that want to call pad operations would then need to 
allocate a settings structure, instead of a complete fake fh.

> Furthermore I assume more complex pipelines will be handled in user space

The pad-level API has been designed to get/set formats/crop settings directly 
from userspace, not from inside the kernel, so that would certainly work.

> and the host drivers could store format and crop information locally
> directly from v4l2_subdev_format and v4l2_subdev_crop data structures.

I'm not sure to understand what you mean there.

> > Quoting one of your comments below,
> > 
> > x--- FIMC_0 (/dev/video1)
> >  
> >  SENSOR -> MIPI_CSIS  --|
> >  
> > x--- FIMC_1 (/dev/video3)
> > 
> > How do you expect to configure the MIPI_CSIS block from the FIMC_0 and
> > FIMC_1 blocks, without any help from userspace ? Conflicts will need to
> > be handled, and the best way to handle them is to have userspace
> > configuring the MIPI_CSIS explicitly.
> 
> My priority is to make work the configurations without device nodes at
> sensor and MIPI CSIS subdevs.
> 
> I agree it would be best to leave the negotiation logic to user space,
> however I need to assure the regular V4L2 application also can use the
> driver.
> 
> My idea was to only try format at CSI slave and sensor subdevs when S_FMT
> is called on a video node. So the sensor and CSIS constraints are taken
> into account.
>
> Then from VIDIOC_STREAMON, formats at pipeline elements would be set on
> subdevs without device node or validated on subdevs providing a device
> node.

For subdevs without device nodes, why don't you set the active format directly 
when S_FMT is called, instead of postponing the operation until 
VIDIOC_STREAMON time ? You wouldn't need to use TRY formats then.

> Another issue is v4l2 controls. The subdevs are now in my case registered
> to a v4l2_device instance embedded in the media device driver. The video
> node drivers (FIMC0...FIMC3) have their own v4l2_device instances. So the
> control inheritance between video node and a subdevs is gone :/, i.e. I
> couldn't find a standard way to remove back from a parent control handler
> the con

Re: [PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers

[Sylwester Nawrocki]

Hi Laurent,

On 05/04/2011 02:00 PM, Laurent Pinchart wrote:
> Hi Sylwester,
> 
> On Tuesday 03 May 2011 20:07:43 Sylwester Nawrocki wrote:
>> On 05/03/2011 11:16 AM, Laurent Pinchart wrote:
>>> On Thursday 21 April 2011 17:21:04 Sylwester Nawrocki wrote:
> 
> [snip]
> 
 +  struct media_pad pads[CSIS_PADS_NUM];
 +  struct v4l2_subdev sd;
 +  struct platform_device *pdev;
 +  struct resource *regs_res;
 +  void __iomem *regs;
 +  struct clk *clock[NUM_CSIS_CLOCKS];
 +  int irq;
 +  struct regulator *supply;
 +  u32 flags;
 +  /* Common format for the source and sink pad. */
 +  const struct csis_pix_format *csis_fmt;
 +  struct v4l2_mbus_framefmt mf[CSIS_NUM_FMTS];
>>>
>>> As try formats are stored in the file handle, and as the formats on the
>>> sink and source pads are identical, a single v4l2_mbus_framefmt will do
>>> here.
>>
>> Ok. How about a situation when the caller never provides a file handle?
>> Is it not supposed to happen?
> 
> Good question :-) The subdev pad-level operations have been designed with a 
> userspace interface in mind, so they require a file handle to store try the 
> formats (and crop rectangles).
> 
>> For V4L2_SUBDEV_FORMAT_TRY, should set_fmt just abandon storing the format
>> and should get_fmt just return -EINVAL when passed fh == NULL ?
> 
> For such a simple subdev, that should work as a workaround, yes. You can use 
> it as a temporary solution at least.
> 
>> Or should the host driver allocate the file handle just for the sake of
>> set_fmt/get_fmt calls (assuming that cropping ops are not supported
>> by the subdev) ?
> 
> That's another solution. We could also pass an internal structure that 
> contains formats and crop rectangles to the pad operations handlers, instead 
> of passing the whole file handle. Do you think that would be better ?

So it would then be an additional argument for the pad-level operations ?
Perhaps that would make sense when both format and crop information is
needed at the same time in the subdev driver. However this would only be
required for TRY formats/crop rectangles which wouldn't be supported anyway
because of missing file handle.. or I missed something?

Furthermore I assume more complex pipelines will be handled in user space
and the host drivers could store format and crop information locally
directly from v4l2_subdev_format and v4l2_subdev_crop data structures.

[snip]
> Quoting one of your comments below,
> 
> x--- FIMC_0 (/dev/video1)
>  SENSOR -> MIPI_CSIS  --|
> x--- FIMC_1 (/dev/video3)
> 
> How do you expect to configure the MIPI_CSIS block from the FIMC_0 and FIMC_1 
> blocks, without any help from userspace ? Conflicts will need to be handled, 
> and the best way to handle them is to have userspace configuring the 
> MIPI_CSIS 
> explicitly.

My priority is to make work the configurations without device nodes at sensor
and MIPI CSIS subdevs.

I agree it would be best to leave the negotiation logic to user space,
however I need to assure the regular V4L2 application also can use the driver.

My idea was to only try format at CSI slave and sensor subdevs when S_FMT is 
called on a video node. So the sensor and CSIS constraints are taken into 
account.

Then from VIDIOC_STREAMON, formats at pipeline elements would be set on subdevs
without device node or validated on subdevs providing a device node.

Another issue is v4l2 controls. The subdevs are now in my case registered 
to a v4l2_device instance embedded in the media device driver. The video node
drivers (FIMC0...FIMC3) have their own v4l2_device instances. So the control
inheritance between video node and a subdevs is gone :/, i.e. I couldn't find
a standard way to remove back from a parent control handler the controls which 
have been added to it with v4l2_ctrl_handler_add().

I've had similar issue with subdev -> v4l2_device notify callback. Before, when
the subdev was directly registered to a v4l2_instance associated with a video
node, v4l2_subdev_notify had been propagated straight to FIMC{N} device the 
subdev
was attached to.
Now I just redirect notifications ending up in the media device driver to
relevant FIMC{N} device instance depending on link configuration.  


[snip]
 +#define csis_pad_valid(pad) (pad == CSIS_PAD_SOURCE || pad ==
 CSIS_PAD_SINK) +
 +static struct csis_state *sd_to_csis_state(struct v4l2_subdev *sdev)
 +{
 +  return container_of(sdev, struct csis_state, sd);
 +}
 +
 +static const struct csis_pix_format *find_csis_format(
 +  struct v4l2_mbus_framefmt *mf)
 +{
 +  int i = ARRAY_SIZE(s5pcsis_formats);
 +
 +  while (--i>= 0)
>>>
>>> I'm curious, why do you search backward instead of doing the usual
>>>
>>> for (i = 0; i<  ARRAY_SIZE(s5pcsis_formats); ++i)
>>>
>>> (in that case 'i' could be unsigned) ?
>>
>> Perhaps doing it either way does not make any difference with the
>> toolchains we use, but the

Re: [PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers

[Laurent Pinchart]

Hi Sylwester,

On Tuesday 03 May 2011 20:07:43 Sylwester Nawrocki wrote:
> On 05/03/2011 11:16 AM, Laurent Pinchart wrote:
> > On Thursday 21 April 2011 17:21:04 Sylwester Nawrocki wrote:

[snip]

> >> +  struct media_pad pads[CSIS_PADS_NUM];
> >> +  struct v4l2_subdev sd;
> >> +  struct platform_device *pdev;
> >> +  struct resource *regs_res;
> >> +  void __iomem *regs;
> >> +  struct clk *clock[NUM_CSIS_CLOCKS];
> >> +  int irq;
> >> +  struct regulator *supply;
> >> +  u32 flags;
> >> +  /* Common format for the source and sink pad. */
> >> +  const struct csis_pix_format *csis_fmt;
> >> +  struct v4l2_mbus_framefmt mf[CSIS_NUM_FMTS];
> > 
> > As try formats are stored in the file handle, and as the formats on the
> > sink and source pads are identical, a single v4l2_mbus_framefmt will do
> > here.
> 
> Ok. How about a situation when the caller never provides a file handle?
> Is it not supposed to happen?

Good question :-) The subdev pad-level operations have been designed with a 
userspace interface in mind, so they require a file handle to store try the 
formats (and crop rectangles).

> For V4L2_SUBDEV_FORMAT_TRY, should set_fmt just abandon storing the format
> and should get_fmt just return -EINVAL when passed fh == NULL ?

For such a simple subdev, that should work as a workaround, yes. You can use 
it as a temporary solution at least.

> Or should the host driver allocate the file handle just for the sake of
> set_fmt/get_fmt calls (assuming that cropping ops are not supported
> by the subdev) ?

That's another solution. We could also pass an internal structure that 
contains formats and crop rectangles to the pad operations handlers, instead 
of passing the whole file handle. Do you think that would be better ?

> It's not my intention to create a broken implementation but it would
> be nice to be able to drop functionality which would never be used.
> 
> As a note, I wanted to avoid bothering user space with setting up the MIPI
> CSI receiver sub-device. There wouldn't be any gain from it, just more
> things to care about for the applications.

Quoting one of your comments below,

x--- FIMC_0 (/dev/video1)
 SENSOR -> MIPI_CSIS  --|
x--- FIMC_1 (/dev/video3)

How do you expect to configure the MIPI_CSIS block from the FIMC_0 and FIMC_1 
blocks, without any help from userspace ? Conflicts will need to be handled, 
and the best way to handle them is to have userspace configuring the MIPI_CSIS 
explicitly.

> Moreover I don't see a good usage for the stored TRY format (yet). So I
> originally thought this subdev could be configurable by the host driver
> which wouldn't provide a file handle.

[snip]

> >> +#define csis_pad_valid(pad) (pad == CSIS_PAD_SOURCE || pad ==
> >> CSIS_PAD_SINK) +
> >> +static struct csis_state *sd_to_csis_state(struct v4l2_subdev *sdev)
> >> +{
> >> +  return container_of(sdev, struct csis_state, sd);
> >> +}
> >> +
> >> +static const struct csis_pix_format *find_csis_format(
> >> +  struct v4l2_mbus_framefmt *mf)
> >> +{
> >> +  int i = ARRAY_SIZE(s5pcsis_formats);
> >> +
> >> +  while (--i>= 0)
> > 
> > I'm curious, why do you search backward instead of doing the usual
> > 
> > for (i = 0; i<  ARRAY_SIZE(s5pcsis_formats); ++i)
> > 
> > (in that case 'i' could be unsigned) ?
> 
> Perhaps doing it either way does not make any difference with the
> toolchains we use, but the loops with test for 0 are supposed to be faster
> on ARM.

I didn't know that. I wonder if it makes a real difference with gcc.

[snip]

> >> +static void s5pcsis_try_format(struct v4l2_mbus_framefmt *mf)
> >> +{
> >> +  struct csis_pix_format const *csis_fmt;
> >> +
> >> +  csis_fmt = find_csis_format(mf);
> >> +  if (csis_fmt == NULL)
> >> +  csis_fmt =&s5pcsis_formats[0];
> >> +
> >> +  mf->code = csis_fmt->code;
> >> +  v4l_bound_align_image(&mf->width, 1, CSIS_MAX_PIX_WIDTH,
> >> +csis_fmt->pix_hor_align,
> >> +  &mf->height, 1, CSIS_MAX_PIX_HEIGHT, 1,
> >> +0);
> >> +}
> >> +
> >> +static int s5pcsis_set_fmt(struct v4l2_subdev *sd, struct
> >> v4l2_subdev_fh *fh, +  struct v4l2_subdev_format 
> >> *fmt)
> >> +{
> >> +  struct csis_state *state = sd_to_csis_state(sd);
> >> +  struct v4l2_mbus_framefmt *mf =&fmt->format;
> >> +  struct csis_pix_format const *csis_fmt = find_csis_format(mf);
> >> +
> >> +  v4l2_dbg(1, debug, sd, "%s: %dx%d, code: %x, csis_fmt: %p\n",
> >> +   __func__, mf->width, mf->height, mf->code, csis_fmt);
> >> +
> >> +  if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> >> +  s5pcsis_try_format(mf);
> > 
> > You need to take the pad into account here. As you mention below, source
> > and sink formats are identical. When the user tries to set the source
> > format, the driver should just return the sink format without performing
> > any modification.
> > 
> >> +  state->mf[CSIS_FMT_TRY] = *mf;
> >> +  return

Re: [PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers

[Sylwester Nawrocki]

Hi Laurent,

thank you for the review. 

On 05/03/2011 11:16 AM, Laurent Pinchart wrote:
> Hi Sylwester,
> 
> Thanks for the patch.
> 
> On Thursday 21 April 2011 17:21:04 Sylwester Nawrocki wrote:
>> Add the subdev driver for the MIPI CSIS units available in
>> S5P and Exynos4 SoC series. This driver supports both CSIS0
>> and CSIS1 MIPI-CSI2 receivers.
>> The driver requires Runtime PM to be enabled for proper operation.
> 
> Then maybe it should depend on runtime PM ?

Yes, it seem the right thing to do. I hesitated to do this, not sure now why.

> 
>> Signed-off-by: Sylwester Nawrocki
>> Signed-off-by: Kyungmin Park
> 
> [snip]
> 
>> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
>> index 4f0ac2d..1da961a 100644
>> --- a/drivers/media/video/Kconfig
>> +++ b/drivers/media/video/Kconfig
>> @@ -939,6 +939,15 @@ config  VIDEO_SAMSUNG_S5P_FIMC
>>To compile this driver as a module, choose M here: the
>>module will be called s5p-fimc.
>>
>> +config VIDEO_S5P_MIPI_CSIS
>> +tristate "S5P and EXYNOS4 MIPI CSI Receiver driver"
> 
> Maybe "Samsung S5P and EXYNOS4" as well ?

Agreed. I'll change it in the patch 2/3 as well.

> 
>> +depends on VIDEO_V4L2&&  VIDEO_SAMSUNG_S5P_FIMC&&  MEDIA_CONTROLLER
>> +---help---
>> +  This is a v4l2 driver for the S5P/EXYNOS4 MIPI-CSI Receiver.
>> +
>> +  To compile this driver as a module, choose M here: the
>> +  module will be called s5p-csis.
>> +
> 
> [snip]
> 
>> diff --git a/drivers/media/video/s5p-fimc/mipi-csis.c
>> b/drivers/media/video/s5p-fimc/mipi-csis.c new file mode 100644
>> index 000..6219754
>> --- /dev/null
>> +++ b/drivers/media/video/s5p-fimc/mipi-csis.c
> 
> [snip]
> 
>> +enum {
>> +CSIS_FMT_TRY,
>> +CSIS_FMT_ACTIVE,
>> +CSIS_NUM_FMTS
>> +}
> 
> There's no need to define new TRY/ACTIVE constants, use the existing
> V4L2_SUBDEV_FORMAT_TRY and V4L2_SUBDEV_FORMAT_ACTIVE values.
> 
>> +struct csis_state {
>> +struct mutex lock;
> 
> checkpatch.pl requires mutexes to be documented. You should add a comment
> (here, or even better, before the structure, with the documentation of all
> members) that explains what the mutex protects.

Didn't know about that.. I'll add a full description of all the structure's
members.

> 
>> +struct media_pad pads[CSIS_PADS_NUM];
>> +struct v4l2_subdev sd;
>> +struct platform_device *pdev;
>> +struct resource *regs_res;
>> +void __iomem *regs;
>> +struct clk *clock[NUM_CSIS_CLOCKS];
>> +int irq;
>> +struct regulator *supply;
>> +u32 flags;
>> +/* Common format for the source and sink pad. */
>> +const struct csis_pix_format *csis_fmt;
>> +struct v4l2_mbus_framefmt mf[CSIS_NUM_FMTS];
> 
> As try formats are stored in the file handle, and as the formats on the sink
> and source pads are identical, a single v4l2_mbus_framefmt will do here.

Ok. How about a situation when the caller never provides a file handle?
Is it not supposed to happen?

For V4L2_SUBDEV_FORMAT_TRY, should set_fmt just abandon storing the format
and should get_fmt just return -EINVAL when passed fh == NULL ?

Or should the host driver allocate the file handle just for the sake of
set_fmt/get_fmt calls (assuming that cropping ops are not supported
by the subdev) ?

It's not my intention to create a broken implementation but it would
be nice to be able to drop functionality which would never be used.

As a note, I wanted to avoid bothering user space with setting up the MIPI CSI
receiver sub-device. There wouldn't be any gain from it, just more things to
care about for the applications.
Moreover I don't see a good usage for the stored TRY format (yet).
So I originally thought this subdev could be configurable by the host
driver which wouldn't provide a file handle.

> 
>> +};
> 
> [snip]
> 
>> +struct csis_pix_format {
>> +enum v4l2_mbus_pixelcode code;
>> +u32 fmt_reg;
>> +u16 pix_hor_align;
> 
> You won't save memory by using a 16-bit integer here, and the code might be
> slower. I would go for a regular unsigned int.

Agreed.
> 
>> +};
>> +
>> +static const struct csis_pix_format s5pcsis_formats[] = {
>> +{
>> +.code   = V4L2_MBUS_FMT_VYUY8_2X8,
>> +.fmt_reg= S5PCSIS_CFG_FMT_YCBCR422_8BIT,
>> +.pix_hor_align  = 1,
>> +},
>> +{
>> +.code   = V4L2_MBUS_FMT_JPEG_1X8,
>> +.fmt_reg= S5PCSIS_CFG_FMT_USER(1),
>> +.pix_hor_align  = 1,
>> +},
>> +};
> 
> Do you plan to add formats with pix_hor_align != 1 ? If not you could remove
> the field completely.

Yes, there is more formats that need different alignment, like RAW10, RAW12.

The documentation for s5pc110, for instance, can be downloaded after 
registration
from http://www.aesop.or.kr/?mid=PageMain_Documents_Tips

Actually it is supposed to be 2^pix_hor_align pixel width alignment.
I need to amend the usage of this alignment property in s5pcsis_set_fmt().

> 
>

Re: [PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers

[Laurent Pinchart]

Hi Sylwester,

Thanks for the patch.

On Thursday 21 April 2011 17:21:04 Sylwester Nawrocki wrote:
> Add the subdev driver for the MIPI CSIS units available in
> S5P and Exynos4 SoC series. This driver supports both CSIS0
> and CSIS1 MIPI-CSI2 receivers.
> The driver requires Runtime PM to be enabled for proper operation.

Then maybe it should depend on runtime PM ?

> Signed-off-by: Sylwester Nawrocki 
> Signed-off-by: Kyungmin Park 

[snip]

> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index 4f0ac2d..1da961a 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -939,6 +939,15 @@ config  VIDEO_SAMSUNG_S5P_FIMC
> To compile this driver as a module, choose M here: the
> module will be called s5p-fimc.
> 
> +config VIDEO_S5P_MIPI_CSIS
> + tristate "S5P and EXYNOS4 MIPI CSI Receiver driver"

Maybe "Samsung S5P and EXYNOS4" as well ?

> + depends on VIDEO_V4L2 && VIDEO_SAMSUNG_S5P_FIMC && MEDIA_CONTROLLER
> + ---help---
> +   This is a v4l2 driver for the S5P/EXYNOS4 MIPI-CSI Receiver.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called s5p-csis.
> +

[snip]

> diff --git a/drivers/media/video/s5p-fimc/mipi-csis.c
> b/drivers/media/video/s5p-fimc/mipi-csis.c new file mode 100644
> index 000..6219754
> --- /dev/null
> +++ b/drivers/media/video/s5p-fimc/mipi-csis.c

[snip]

> +enum {
> + CSIS_FMT_TRY,
> + CSIS_FMT_ACTIVE,
> + CSIS_NUM_FMTS
> +}

There's no need to define new TRY/ACTIVE constants, use the existing 
V4L2_SUBDEV_FORMAT_TRY and V4L2_SUBDEV_FORMAT_ACTIVE values.

> +struct csis_state {
> + struct mutex lock;

checkpatch.pl requires mutexes to be documented. You should add a comment 
(here, or even better, before the structure, with the documentation of all 
memebers) that explains what the mutex protects.

> + struct media_pad pads[CSIS_PADS_NUM];
> + struct v4l2_subdev sd;
> + struct platform_device *pdev;
> + struct resource *regs_res;
> + void __iomem *regs;
> + struct clk *clock[NUM_CSIS_CLOCKS];
> + int irq;
> + struct regulator *supply;
> + u32 flags;
> + /* Common format for the source and sink pad. */
> + const struct csis_pix_format *csis_fmt;
> + struct v4l2_mbus_framefmt mf[CSIS_NUM_FMTS];

As try formats are stored in the file handle, and as the formats on the sink 
and source pads are identical, a single v4l2_mbus_framefmt will do here.

> +};

[snip]

> +struct csis_pix_format {
> + enum v4l2_mbus_pixelcode code;
> + u32 fmt_reg;
> + u16 pix_hor_align;

You won't save memory by using a 16-bit integer here, and the code might be 
slower. I would go for a regular unsigned int.

> +};
> +
> +static const struct csis_pix_format s5pcsis_formats[] = {
> + {
> + .code   = V4L2_MBUS_FMT_VYUY8_2X8,
> + .fmt_reg= S5PCSIS_CFG_FMT_YCBCR422_8BIT,
> + .pix_hor_align  = 1,
> + },
> + {
> + .code   = V4L2_MBUS_FMT_JPEG_1X8,
> + .fmt_reg= S5PCSIS_CFG_FMT_USER(1),
> + .pix_hor_align  = 1,
> + },
> +};

Do you plan to add formats with pix_hor_align != 1 ? If not you could remove 
the field completely.

> +#define csis_pad_valid(pad) (pad == CSIS_PAD_SOURCE || pad ==
> CSIS_PAD_SINK) +
> +static struct csis_state *sd_to_csis_state(struct v4l2_subdev *sdev)
> +{
> + return container_of(sdev, struct csis_state, sd);
> +}
> +
> +static const struct csis_pix_format *find_csis_format(
> + struct v4l2_mbus_framefmt *mf)
> +{
> + int i = ARRAY_SIZE(s5pcsis_formats);
> +
> + while (--i >= 0)

I'm curious, why do you search backward instead of doing the usual

for (i = 0; i < ARRAY_SIZE(s5pcsis_formats); ++i)

(in that case 'i' could be unsigned) ?

> + if (mf->code == s5pcsis_formats[i].code)
> + return &s5pcsis_formats[i];
> +
> + return NULL;
> +}
> +
> +static void s5pcsis_enable_interrupts(struct csis_state *state, bool on)
> +{
> + u32 reg = readl(state->regs + S5PCSIS_CTRL);

I haven't read the hardware spec, but shouldn't the register be S5PCSIS_INTMSK 
here ?

> +
> + if (on)
> + reg |= S5PCSIS_INTMSK_EN_ALL;
> + else
> + reg &= ~S5PCSIS_INTMSK_EN_ALL;
> + writel(reg, state->regs + S5PCSIS_INTMSK);
> +}
> +
> +static void s5pcsis_reset(struct csis_state *state)
> +{
> + u32 reg = readl(state->regs + S5PCSIS_CTRL);
> +
> + writel(reg | S5PCSIS_CTRL_RESET, state->regs + S5PCSIS_CTRL);

Would the code be more readable if you defined macros or inline functions for 
the read, write and read-modify-write register operations ? Something like

s5pcsis_read(struct csis_state *state, u32 reg);
s5pcsis_write(struct csis_state *state, u32 reg, u32 value);
s5pcsis_clr(struct csis_state *state, u32 reg, u32 clr);
s5pcsis_set(struct csis_state *state, u32 reg, u32 set);
s5pcsis_clr_set(st

[PATCH v4 3/3] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers

[Sylwester Nawrocki]

Add the subdev driver for the MIPI CSIS units available in
S5P and Exynos4 SoC series. This driver supports both CSIS0
and CSIS1 MIPI-CSI2 receivers.
The driver requires Runtime PM to be enabled for proper operation.

Signed-off-by: Sylwester Nawrocki 
Signed-off-by: Kyungmin Park 
---
 drivers/media/video/Kconfig  |9 +
 drivers/media/video/s5p-fimc/Makefile|6 +-
 drivers/media/video/s5p-fimc/mipi-csis.c |  745 ++
 drivers/media/video/s5p-fimc/mipi-csis.h |   22 +
 4 files changed, 780 insertions(+), 2 deletions(-)
 create mode 100644 drivers/media/video/s5p-fimc/mipi-csis.c
 create mode 100644 drivers/media/video/s5p-fimc/mipi-csis.h

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 4f0ac2d..1da961a 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -939,6 +939,15 @@ config  VIDEO_SAMSUNG_S5P_FIMC
  To compile this driver as a module, choose M here: the
  module will be called s5p-fimc.
 
+config VIDEO_S5P_MIPI_CSIS
+   tristate "S5P and EXYNOS4 MIPI CSI Receiver driver"
+   depends on VIDEO_V4L2 && VIDEO_SAMSUNG_S5P_FIMC && MEDIA_CONTROLLER
+   ---help---
+ This is a v4l2 driver for the S5P/EXYNOS4 MIPI-CSI Receiver.
+
+ To compile this driver as a module, choose M here: the
+ module will be called s5p-csis.
+
 #
 # USB Multimedia device configuration
 #
diff --git a/drivers/media/video/s5p-fimc/Makefile 
b/drivers/media/video/s5p-fimc/Makefile
index 7ea1b14..df6954a 100644
--- a/drivers/media/video/s5p-fimc/Makefile
+++ b/drivers/media/video/s5p-fimc/Makefile
@@ -1,3 +1,5 @@
+s5p-fimc-objs := fimc-core.o fimc-reg.o fimc-capture.o
+s5p-csis-objs := mipi-csis.o
 
-obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC) := s5p-fimc.o
-s5p-fimc-y := fimc-core.o fimc-reg.o fimc-capture.o
+obj-$(CONFIG_VIDEO_S5P_MIPI_CSIS)  += s5p-csis.o
+obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC)   += s5p-fimc.o
diff --git a/drivers/media/video/s5p-fimc/mipi-csis.c 
b/drivers/media/video/s5p-fimc/mipi-csis.c
new file mode 100644
index 000..6219754
--- /dev/null
+++ b/drivers/media/video/s5p-fimc/mipi-csis.c
@@ -0,0 +1,745 @@
+/*
+ * Samsung S5P SoC series MIPI-CSI receiver driver
+ *
+ * Copyright (C) 2011 Samsung Electronics Co., Ltd.
+ * Contact: Sylwester Nawrocki, 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "mipi-csis.h"
+
+static int debug;
+module_param(debug, int, 0644);
+MODULE_PARM_DESC(debug, "Debug level (0-1)");
+
+/* Register map definition */
+
+/* CSIS global control */
+#define S5PCSIS_CTRL   0x00
+#define S5PCSIS_CTRL_DPDN_DEFAULT  (0 << 31)
+#define S5PCSIS_CTRL_DPDN_SWAP (1 << 31)
+#define S5PCSIS_CTRL_ALIGN_32BIT   (1 << 20)
+#define S5PCSIS_CTRL_UPDATE_SHADOW (1 << 16)
+#define S5PCSIS_CTRL_WCLK_EXTCLK   (1 << 8)
+#define S5PCSIS_CTRL_RESET (1 << 4)
+#define S5PCSIS_CTRL_ENABLE(1 << 0)
+
+/* D-PHY control */
+#define S5PCSIS_DPHYCTRL   0x04
+#define S5PCSIS_DPHYCTRL_HSS_MASK  (0x1f << 27)
+#define S5PCSIS_DPHYCTRL_ENABLE(0x1f << 0)
+
+#define S5PCSIS_CONFIG 0x08
+#define S5PCSIS_CFG_FMT_YCBCR422_8BIT  (0x1e << 2)
+#define S5PCSIS_CFG_FMT_RAW8   (0x2a << 2)
+#define S5PCSIS_CFG_FMT_RAW10  (0x2b << 2)
+#define S5PCSIS_CFG_FMT_RAW12  (0x2c << 2)
+/* User defined formats, x = 1...4 */
+#define S5PCSIS_CFG_FMT_USER(x)((0x30 + x - 1) << 2)
+#define S5PCSIS_CFG_FMT_MASK   (0x3f << 2)
+#define S5PCSIS_CFG_NR_LANE_MASK   3
+
+/* Interrupt mask. */
+#define S5PCSIS_INTMSK 0x10
+#define S5PCSIS_INTMSK_EN_ALL  0xf03f
+#define S5PCSIS_INTSRC 0x14
+
+/* Pixel resolution */
+#define S5PCSIS_RESOL  0x2c
+#define CSIS_MAX_PIX_WIDTH 0x
+#define CSIS_MAX_PIX_HEIGHT0x
+
+enum {
+   CSIS_CLK_MUX,
+   CSIS_CLK_GATE,
+};
+
+static char *csi_clock_name[] = {
+   [CSIS_CLK_MUX]  = "sclk_csis",
+   [CSIS_CLK_GATE] = "csis",
+};
+#define NUM_CSIS_CLOCKSARRAY_SIZE(csi_clock_name)
+
+enum {
+   ST_POWERED  = 1,
+   ST_STREAMING= 2,
+   ST_SUSPENDED= 4,
+};
+
+enum {
+   CSIS_FMT_TRY,
+   CSIS_FMT_ACTIVE,
+   CSIS_NUM_FMTS
+};
+
+struct csis_state {
+   struct mutex lock;
+   struct media_pad pads[CSIS_PADS_NUM];
+   struct v4l2_subdev sd;
+   struct platform_device *pdev;
+   struct resource *regs_res;
+   void __iomem *regs;
+   struct clk *clock[NUM_CSIS_CLOCKS];
+   int irq;
+   struct