diff mbox

[v2,2/9] include: media: Add Renesas CEU driver interface

Message ID 1514469681-15602-3-git-send-email-jacopo+renesas@jmondi.org (mailing list archive)
State New, archived
Headers show

Commit Message

Jacopo Mondi Dec. 28, 2017, 2:01 p.m. UTC
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.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 include/media/drv-intf/renesas-ceu.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 include/media/drv-intf/renesas-ceu.h

Comments

Laurent Pinchart Jan. 2, 2018, 11:50 a.m. UTC | #1
Hi Jacopo,

Thank you for the patch.

On Thursday, 28 December 2017 16:01:14 EET 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.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  include/media/drv-intf/renesas-ceu.h | 20 ++++++++++++++++++++
>  1 file changed, 20 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..7470c3f
> --- /dev/null
> +++ b/include/media/drv-intf/renesas-ceu.h
> @@ -0,0 +1,20 @@
> +// SPDX-License-Identifier: GPL-2.0+

Just out of curiosity, any reason you have picked GPL-2.0+ and not GPL-2.0 ?

You might want to add a copyright header to state copyright ownership

/**
 * renesas-ceu.h - Renesas CEU driver interface
 *
 * Copyright 2017-2018 Jacopo Mondi <jacopo@jmondi.org>
 */

That's up to you.

> +#ifndef __ASM_RENESAS_CEU_H__

Maybe __MEDIA_DRV_INTF_RENESAS_CEU_H__ ?

> +#define __ASM_RENESAS_CEU_H__
> +
> +#define CEU_MAX_SUBDEVS		2
> +
> +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 {

This is really platform data, how about calling it ceu_platform_data ?

> +	unsigned int num_subdevs;
> +	struct ceu_async_subdev subdevs[CEU_MAX_SUBDEVS];
> +};
> +
> +#endif /* __ASM_RENESAS_CEU_H__ */

Don't forget to update the comment here too.

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Jacopo Mondi Jan. 3, 2018, 9 a.m. UTC | #2
Hi Laurent,

On Tue, Jan 02, 2018 at 01:50:07PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thursday, 28 December 2017 16:01:14 EET 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.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  include/media/drv-intf/renesas-ceu.h | 20 ++++++++++++++++++++
> >  1 file changed, 20 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..7470c3f
> > --- /dev/null
> > +++ b/include/media/drv-intf/renesas-ceu.h
> > @@ -0,0 +1,20 @@
> > +// SPDX-License-Identifier: GPL-2.0+
>
> Just out of curiosity, any reason you have picked GPL-2.0+ and not GPL-2.0 ?

Good question. I would mention lazyness in copying the SPDX identifier
from somewhere else and overlooking the license version reported
there. I don't have any personal preference on GPLv3 I was trying to
squeeze in :)

I'll switch it back to GPL-2.0 only

>
> You might want to add a copyright header to state copyright ownership
>
> /**
>  * renesas-ceu.h - Renesas CEU driver interface
>  *
>  * Copyright 2017-2018 Jacopo Mondi <jacopo@jmondi.org>
>  */
>
> That's up to you.
>
> > +#ifndef __ASM_RENESAS_CEU_H__
>
> Maybe __MEDIA_DRV_INTF_RENESAS_CEU_H__ ?
>
> > +#define __ASM_RENESAS_CEU_H__
> > +
> > +#define CEU_MAX_SUBDEVS		2
> > +
> > +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 {
>
> This is really platform data, how about calling it ceu_platform_data ?
>
> > +	unsigned int num_subdevs;
> > +	struct ceu_async_subdev subdevs[CEU_MAX_SUBDEVS];
> > +};
> > +
> > +#endif /* __ASM_RENESAS_CEU_H__ */
>
> Don't forget to update the comment here too.
>
> Apart from that,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks
   j
>
> --
> Regards,
>
> Laurent Pinchart
>
diff mbox

Patch

diff --git a/include/media/drv-intf/renesas-ceu.h b/include/media/drv-intf/renesas-ceu.h
new file mode 100644
index 0000000..7470c3f
--- /dev/null
+++ b/include/media/drv-intf/renesas-ceu.h
@@ -0,0 +1,20 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+#ifndef __ASM_RENESAS_CEU_H__
+#define __ASM_RENESAS_CEU_H__
+
+#define CEU_MAX_SUBDEVS		2
+
+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_SUBDEVS];
+};
+
+#endif /* __ASM_RENESAS_CEU_H__ */