Re: [PATCH v2 1/2] media: entity: Add pad_from_fwnode entity operation

[Sakari Ailus]

Hejssan,

On Wed, May 24, 2017 at 04:07:36PM +0200, Niklas Söderlund wrote:
> Hi Sakari,
> 
> Thanks for your feedback.
> 
> On 2017-05-24 16:21:37 +0300, Sakari Ailus wrote:
> > Hi Niklas,
> > 
> > On Wed, May 24, 2017 at 02:09:06AM +0200, Niklas Söderlund wrote:
> > > From: Niklas Söderlund 
> > > 
> > > The optional operation can be used by entities to report how it maps its
> > > fwnode endpoints to media pad numbers. This is useful for devices which
> > > require advanced mappings of pads.
> > > 
> > > Signed-off-by: Niklas Söderlund 
> > > ---
> > >  include/media/media-entity.h | 6 ++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > > index c7c254c5bca1761b..2aea22b0409d1070 100644
> > > --- a/include/media/media-entity.h
> > > +++ b/include/media/media-entity.h
> > > @@ -21,6 +21,7 @@
> > >  
> > >  #include 
> > >  #include 
> > > +#include 
> > >  #include 
> > >  #include 
> > >  #include 
> > > @@ -171,6 +172,9 @@ struct media_pad {
> > >  
> > >  /**
> > >   * struct media_entity_operations - Media entity operations
> > > + * @pad_from_fwnode: Return the pad number based on a fwnode 
> > > endpoint.
> > > + *   This operation can be used to map a fwnode to a
> > > + *   media pad number. Optional.
> > >   * @link_setup:  Notify the entity of link changes. The 
> > > operation can
> > >   *   return an error, in which case link setup will 
> > > be
> > >   *   cancelled. Optional.
> > > @@ -184,6 +188,8 @@ struct media_pad {
> > >   *mutex held.
> > >   */
> > >  struct media_entity_operations {
> > > + int (*pad_from_fwnode)(struct fwnode_endpoint *endpoint,
> > > +unsigned int *pad);
> > 
> > Hmm. How about calling this get_fwnode_pad for instance? I wonder what
> > others think.
> 
> I'm OK with this name change, will update for next version.
> 
> > 
> > You could just return the pad number still, and a negative value on error. I
> > think we won't have more than INT_MAX pads. :-)
> 
> I did that at first but then I remembered all the review comments I have 
> gotten earlier about using int as the type for pads :-) If you and 
> others agree in this case returning the pad as int or a negative value 
> as error I have no problem chaining this for the next version.

unsigned int was proposed for there was no need for negative values. In this
case there is.

I don't really have a strong opinion either way. I wonder what Hans or
Laurent thinks.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH v2 1/2] media: entity: Add pad_from_fwnode entity operation

[Niklas Söderlund]

Hi Sakari,

Thanks for your feedback.

On 2017-05-24 16:21:37 +0300, Sakari Ailus wrote:
> Hi Niklas,
> 
> On Wed, May 24, 2017 at 02:09:06AM +0200, Niklas Söderlund wrote:
> > From: Niklas Söderlund 
> > 
> > The optional operation can be used by entities to report how it maps its
> > fwnode endpoints to media pad numbers. This is useful for devices which
> > require advanced mappings of pads.
> > 
> > Signed-off-by: Niklas Söderlund 
> > ---
> >  include/media/media-entity.h | 6 ++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index c7c254c5bca1761b..2aea22b0409d1070 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -21,6 +21,7 @@
> >  
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -171,6 +172,9 @@ struct media_pad {
> >  
> >  /**
> >   * struct media_entity_operations - Media entity operations
> > + * @pad_from_fwnode:   Return the pad number based on a fwnode 
> > endpoint.
> > + * This operation can be used to map a fwnode to a
> > + * media pad number. Optional.
> >   * @link_setup:Notify the entity of link changes. The 
> > operation can
> >   * return an error, in which case link setup will be
> >   * cancelled. Optional.
> > @@ -184,6 +188,8 @@ struct media_pad {
> >   *mutex held.
> >   */
> >  struct media_entity_operations {
> > +   int (*pad_from_fwnode)(struct fwnode_endpoint *endpoint,
> > +  unsigned int *pad);
> 
> Hmm. How about calling this get_fwnode_pad for instance? I wonder what
> others think.

I'm OK with this name change, will update for next version.

> 
> You could just return the pad number still, and a negative value on error. I
> think we won't have more than INT_MAX pads. :-)

I did that at first but then I remembered all the review comments I have 
gotten earlier about using int as the type for pads :-) If you and 
others agree in this case returning the pad as int or a negative value 
as error I have no problem chaining this for the next version.

> 
> > int (*link_setup)(struct media_entity *entity,
> >   const struct media_pad *local,
> >   const struct media_pad *remote, u32 flags);
> 
> -- 
> Regards,
> 
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi   XMPP: sai...@retiisi.org.uk

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 1/2] media: entity: Add pad_from_fwnode entity operation

[Sakari Ailus]

Hi Niklas,

On Wed, May 24, 2017 at 02:09:06AM +0200, Niklas Söderlund wrote:
> From: Niklas Söderlund 
> 
> The optional operation can be used by entities to report how it maps its
> fwnode endpoints to media pad numbers. This is useful for devices which
> require advanced mappings of pads.
> 
> Signed-off-by: Niklas Söderlund 
> ---
>  include/media/media-entity.h | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index c7c254c5bca1761b..2aea22b0409d1070 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -21,6 +21,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -171,6 +172,9 @@ struct media_pad {
>  
>  /**
>   * struct media_entity_operations - Media entity operations
> + * @pad_from_fwnode: Return the pad number based on a fwnode endpoint.
> + *   This operation can be used to map a fwnode to a
> + *   media pad number. Optional.
>   * @link_setup:  Notify the entity of link changes. The 
> operation can
>   *   return an error, in which case link setup will be
>   *   cancelled. Optional.
> @@ -184,6 +188,8 @@ struct media_pad {
>   *mutex held.
>   */
>  struct media_entity_operations {
> + int (*pad_from_fwnode)(struct fwnode_endpoint *endpoint,
> +unsigned int *pad);

Hmm. How about calling this get_fwnode_pad for instance? I wonder what
others think.

You could just return the pad number still, and a negative value on error. I
think we won't have more than INT_MAX pads. :-)

>   int (*link_setup)(struct media_entity *entity,
> const struct media_pad *local,
> const struct media_pad *remote, u32 flags);

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

[PATCH v2 1/2] media: entity: Add pad_from_fwnode entity operation

[Niklas Söderlund]

From: Niklas Söderlund 

The optional operation can be used by entities to report how it maps its
fwnode endpoints to media pad numbers. This is useful for devices which
require advanced mappings of pads.

Signed-off-by: Niklas Söderlund 
---
 include/media/media-entity.h | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index c7c254c5bca1761b..2aea22b0409d1070 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -21,6 +21,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -171,6 +172,9 @@ struct media_pad {
 
 /**
  * struct media_entity_operations - Media entity operations
+ * @pad_from_fwnode:   Return the pad number based on a fwnode endpoint.
+ * This operation can be used to map a fwnode to a
+ * media pad number. Optional.
  * @link_setup:Notify the entity of link changes. The 
operation can
  * return an error, in which case link setup will be
  * cancelled. Optional.
@@ -184,6 +188,8 @@ struct media_pad {
  *mutex held.
  */
 struct media_entity_operations {
+   int (*pad_from_fwnode)(struct fwnode_endpoint *endpoint,
+  unsigned int *pad);
int (*link_setup)(struct media_entity *entity,
  const struct media_pad *local,
  const struct media_pad *remote, u32 flags);
-- 
2.13.0