Re: [PATCH v1 02/10] include: media: Add Renesas CEU driver interface

[Laurent Pinchart]

Hi Jacopo,

On Monday, 11 December 2017 16:26:27 EET Laurent Pinchart wrote:
> On Wednesday, 15 November 2017 14:36:33 EET Sakari Ailus wrote:
> > On Wed, Nov 15, 2017 at 11:55:55AM +0100, Jacopo Mondi wrote:
> >> Add renesas-ceu header file.
> >> 
> >> Do not remove the existing sh_mobile_ceu.h one as long as the original
> >> driver does not go away.
> > 
> > Hmm. This isn't really not about not removing a file but adding a new one.
> > Do you really need it outside the driver's own directory?
> 
> The file defines platform data structures that are needed for arch/sh/.
> 
> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> >> ---
> >> 
> >>  include/media/drv-intf/renesas-ceu.h | 23 +++++++++++++++++++++++
> >>  1 file changed, 23 insertions(+)
> >>  create mode 100644 include/media/drv-intf/renesas-ceu.h
> >> 
> >> diff --git a/include/media/drv-intf/renesas-ceu.h
> >> b/include/media/drv-intf/renesas-ceu.h new file mode 100644
> >> index 0000000..f2da78c
> >> --- /dev/null
> >> +++ b/include/media/drv-intf/renesas-ceu.h
> >> @@ -0,0 +1,23 @@
> >> +// SPDX-License-Identifier: GPL-2.0+
> >> +#ifndef __ASM_RENESAS_CEU_H__
> >> +#define __ASM_RENESAS_CEU_H__
> >> +
> >> +#include <media/v4l2-mediabus.h>
> 
> I don't think you need this.
> 
> > > +#define CEU_FLAG_PRIMARY_SENS	BIT(0)

I forgot to mention that this flag should be renamed as well, but couldn't we 
get rid of it completely by mandating that the first subdev in the subdevs 
array should be the primary one ?

> > > +#define CEU_MAX_SENS		2
> 
> I assume SENS stands for sensor. As other sources than sensors can be
> supported (video decoders for instance), I would name this CEU_MAX_SUBDEVS,
> CEU_MAX_INPUTS or something similar.
> 
> >> +
> >> +struct ceu_async_subdev {
> >> +	unsigned long flags;
> >> +	unsigned char bus_width;
> >> +	unsigned char bus_shift;
> >> +	unsigned int i2c_adapter_id;
> >> +	unsigned int i2c_address;
> >> +};
> >> +
> >> +struct ceu_info {
> >> +	unsigned int num_subdevs;
> >> +	struct ceu_async_subdev subdevs[CEU_MAX_SENS];
> >> +};
> >> +
> >> +#endif /* __ASM_RENESAS_CEU_H__ */

-- 
Regards,

Laurent Pinchart