mbox series

[v2,0/3] Use INTEGER64 type for MEI CSI LINK_FREQ control

Message ID 20240426145538.654212-1-sakari.ailus@linux.intel.com (mailing list archive)
Headers show
Series Use INTEGER64 type for MEI CSI LINK_FREQ control | expand

Message

Sakari Ailus April 26, 2024, 2:55 p.m. UTC
Hi folks,

An integer menu isn't a useful control type for conveying the frequency
depending on an external device to the receiver. Instead, in the MEI CSI
driver, just obtain the link frequency from the upsteam sub-device and
pass it on to the receiver.

The v4l2_get_link_freq() is changed to add support for this and it's
documented as well.

since v1:

- Add a new 64-bit integer control V4L2_CID_CUR_LINK_FREQ instead of
  re-using V4L2_CID_LINK_FREQ.

Sakari Ailus (3):
  media: Documentation: v4l: Add CUR_LINK_FREQ control
  media: v4l: Support obtaining link frequency from CUR_LINK_FREQ
    control
  media: ivsc: csi: Fix link frequency control behaviour

 .../media/v4l/ext-ctrls-image-process.rst     |  4 ++++
 drivers/media/pci/intel/ivsc/mei_csi.c        | 24 ++++++-------------
 drivers/media/v4l2-core/v4l2-common.c         |  6 ++++-
 drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  5 ++++
 include/uapi/linux/v4l2-controls.h            |  1 +
 5 files changed, 22 insertions(+), 18 deletions(-)


base-commit: 6612d1ae37b52d37d1d515d7c5110b9f3afe94d2

Comments

Laurent Pinchart April 28, 2024, 11:20 p.m. UTC | #1
Hi Sakari,

Thank you for the patches.

On Fri, Apr 26, 2024 at 05:55:35PM +0300, Sakari Ailus wrote:
> Hi folks,
> 
> An integer menu isn't a useful control type for conveying the frequency
> depending on an external device to the receiver. Instead, in the MEI CSI
> driver, just obtain the link frequency from the upsteam sub-device and
> pass it on to the receiver.
> 
> The v4l2_get_link_freq() is changed to add support for this and it's
> documented as well.

Using a control for this seems to be a bit of a hack :-S Wouldn't it fit
more nicely in the .get_mbus_config() operation ? That would avoid
exposing this to userspace, and would also have the nice advantage that
the value could be queried per pad.

> since v1:
> 
> - Add a new 64-bit integer control V4L2_CID_CUR_LINK_FREQ instead of
>   re-using V4L2_CID_LINK_FREQ.
> 
> Sakari Ailus (3):
>   media: Documentation: v4l: Add CUR_LINK_FREQ control
>   media: v4l: Support obtaining link frequency from CUR_LINK_FREQ
>     control
>   media: ivsc: csi: Fix link frequency control behaviour
> 
>  .../media/v4l/ext-ctrls-image-process.rst     |  4 ++++
>  drivers/media/pci/intel/ivsc/mei_csi.c        | 24 ++++++-------------
>  drivers/media/v4l2-core/v4l2-common.c         |  6 ++++-
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  5 ++++
>  include/uapi/linux/v4l2-controls.h            |  1 +
>  5 files changed, 22 insertions(+), 18 deletions(-)
> 
> 
> base-commit: 6612d1ae37b52d37d1d515d7c5110b9f3afe94d2
Sakari Ailus April 29, 2024, 7:28 a.m. UTC | #2
Hi Laurent,

On Mon, Apr 29, 2024 at 02:20:28AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patches.
> 
> On Fri, Apr 26, 2024 at 05:55:35PM +0300, Sakari Ailus wrote:
> > Hi folks,
> > 
> > An integer menu isn't a useful control type for conveying the frequency
> > depending on an external device to the receiver. Instead, in the MEI CSI
> > driver, just obtain the link frequency from the upsteam sub-device and
> > pass it on to the receiver.
> > 
> > The v4l2_get_link_freq() is changed to add support for this and it's
> > documented as well.
> 
> Using a control for this seems to be a bit of a hack :-S Wouldn't it fit
> more nicely in the .get_mbus_config() operation ? That would avoid
> exposing this to userspace, and would also have the nice advantage that
> the value could be queried per pad.

Works for me. I also think it's better than using a new control for this
actually.

> 
> > since v1:
> > 
> > - Add a new 64-bit integer control V4L2_CID_CUR_LINK_FREQ instead of
> >   re-using V4L2_CID_LINK_FREQ.
> > 
> > Sakari Ailus (3):
> >   media: Documentation: v4l: Add CUR_LINK_FREQ control
> >   media: v4l: Support obtaining link frequency from CUR_LINK_FREQ
> >     control
> >   media: ivsc: csi: Fix link frequency control behaviour
> > 
> >  .../media/v4l/ext-ctrls-image-process.rst     |  4 ++++
> >  drivers/media/pci/intel/ivsc/mei_csi.c        | 24 ++++++-------------
> >  drivers/media/v4l2-core/v4l2-common.c         |  6 ++++-
> >  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  5 ++++
> >  include/uapi/linux/v4l2-controls.h            |  1 +
> >  5 files changed, 22 insertions(+), 18 deletions(-)
> > 
> > 
> > base-commit: 6612d1ae37b52d37d1d515d7c5110b9f3afe94d2
>
Hans Verkuil April 29, 2024, 7:30 a.m. UTC | #3
On 29/04/2024 09:28, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Mon, Apr 29, 2024 at 02:20:28AM +0300, Laurent Pinchart wrote:
>> Hi Sakari,
>>
>> Thank you for the patches.
>>
>> On Fri, Apr 26, 2024 at 05:55:35PM +0300, Sakari Ailus wrote:
>>> Hi folks,
>>>
>>> An integer menu isn't a useful control type for conveying the frequency
>>> depending on an external device to the receiver. Instead, in the MEI CSI
>>> driver, just obtain the link frequency from the upsteam sub-device and
>>> pass it on to the receiver.
>>>
>>> The v4l2_get_link_freq() is changed to add support for this and it's
>>> documented as well.
>>
>> Using a control for this seems to be a bit of a hack :-S Wouldn't it fit
>> more nicely in the .get_mbus_config() operation ? That would avoid
>> exposing this to userspace, and would also have the nice advantage that
>> the value could be queried per pad.
> 
> Works for me. I also think it's better than using a new control for this
> actually.

Same for me. As you gathered, I was not very enthusiastic about using a
control for this either.

Regards,

	Hans

> 
>>
>>> since v1:
>>>
>>> - Add a new 64-bit integer control V4L2_CID_CUR_LINK_FREQ instead of
>>>   re-using V4L2_CID_LINK_FREQ.
>>>
>>> Sakari Ailus (3):
>>>   media: Documentation: v4l: Add CUR_LINK_FREQ control
>>>   media: v4l: Support obtaining link frequency from CUR_LINK_FREQ
>>>     control
>>>   media: ivsc: csi: Fix link frequency control behaviour
>>>
>>>  .../media/v4l/ext-ctrls-image-process.rst     |  4 ++++
>>>  drivers/media/pci/intel/ivsc/mei_csi.c        | 24 ++++++-------------
>>>  drivers/media/v4l2-core/v4l2-common.c         |  6 ++++-
>>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  5 ++++
>>>  include/uapi/linux/v4l2-controls.h            |  1 +
>>>  5 files changed, 22 insertions(+), 18 deletions(-)
>>>
>>>
>>> base-commit: 6612d1ae37b52d37d1d515d7c5110b9f3afe94d2
>>
>
Sakari Ailus April 29, 2024, 7:50 a.m. UTC | #4
Hi Hans,

On Mon, Apr 29, 2024 at 09:30:37AM +0200, Hans Verkuil wrote:
> On 29/04/2024 09:28, Sakari Ailus wrote:
> > Hi Laurent,
> > 
> > On Mon, Apr 29, 2024 at 02:20:28AM +0300, Laurent Pinchart wrote:
> >> Hi Sakari,
> >>
> >> Thank you for the patches.
> >>
> >> On Fri, Apr 26, 2024 at 05:55:35PM +0300, Sakari Ailus wrote:
> >>> Hi folks,
> >>>
> >>> An integer menu isn't a useful control type for conveying the frequency
> >>> depending on an external device to the receiver. Instead, in the MEI CSI
> >>> driver, just obtain the link frequency from the upsteam sub-device and
> >>> pass it on to the receiver.
> >>>
> >>> The v4l2_get_link_freq() is changed to add support for this and it's
> >>> documented as well.
> >>
> >> Using a control for this seems to be a bit of a hack :-S Wouldn't it fit
> >> more nicely in the .get_mbus_config() operation ? That would avoid
> >> exposing this to userspace, and would also have the nice advantage that
> >> the value could be queried per pad.
> > 
> > Works for me. I also think it's better than using a new control for this
> > actually.
> 
> Same for me. As you gathered, I was not very enthusiastic about using a
> control for this either.

Ack.

I guess these are fine to be merged for 6.11 but still backporting to 6.10
should be done. Nothing breaks now as there's a single frequency used right
now and that's 400 MHz.