| Message ID | 20200714135812.55158-2-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | v4l2-subdev: Introduce [g|s]et_mbus_format pad op | expand |
On 14/07/2020 15:58, Jacopo Mondi wrote: > Introduce two new pad operations to allow retrieving and configuring the > media bus parameters on a subdevice pad. > > The newly introduced operations aims to replace the s/g_mbus_config video > operations, which have been on their way for deprecation since a long > time. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > include/media/v4l2-subdev.h | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index f7fe78a6f65a..d8b9d5735307 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -670,6 +670,29 @@ struct v4l2_subdev_pad_config { > * > * @set_frame_desc: set the low level media bus frame parameters, @fd array > * may be adjusted by the subdev driver to device capabilities. > + * > + * @get_mbus_config: get the media bus configuration of a remote sub-device. > + * The media bus configuration is usually retrieved from the > + * firmware interface at sub-device probe time, immediately > + * applied to the hardware and eventually adjusted by the > + * driver. Remote sub-devices (usually video receivers) shall > + * use this operation to query the transmitting end bus > + * configuration in order to adjust their own one accordingly. > + * Callers should make sure they get the most up-to-date as > + * possible configuration from the remote end, likely calling > + * this operation as close as possible to stream on time. The > + * operation shall fail if the pad index it has been called on > + * is not valid. > + * > + * @set_mbus_config: set the media bus configuration of a remote sub-device. > + * This operations is intended to allow, in combination with > + * the get_mbus_config operation, the negotiation of media bus > + * configuration parameters between media sub-devices. The > + * operation shall not fail if the requested configuration is > + * not supported, but the driver shall update the content of > + * the %config argument to reflect what has been actually > + * applied to the hardware. The operation shall fail if the > + * pad index it has been called on is not valid. > */ Hm, I'd hoped I could merge this, but I think include/media/v4l2-mediabus.h also needs to be updated. So the old g_mbus_config returned all supported configurations, while the new get_mbus_config returns the *current* configuration. That's fine, but that means that the meaning of the struct v4l2_mbus_config flags field changes as well and several comments in v4l2-mediabus.h need to be updated to reflect this. E.g. instead of '/* How many lanes the client can use */' it becomes '/* How many lanes the client uses */'. Frankly, these flags can be redesigned now since you only need a single e.g. V4L2_MBUS_HSYNC_ACTIVE_HIGH flag since if it is not set, then that means ACTIVE_LOW. And since it is now a single bit, it is also easy to make each flag a bit field. The CSI2 lanes/channels can just be a bit field for the number of lanes/channels, which is much easier to read. I strongly recommend making this change at the minimum. Now all this can be done in a follow-up series, but the v4l2-mediabus.h definitely needs to be updated to reflect the new code. This can be done as a v6 5.5/9 patch (since it should come right after the g/s_mbus_config removal). Regards, Hans > struct v4l2_subdev_pad_ops { > int (*init_cfg)(struct v4l2_subdev *sd, > @@ -710,6 +733,10 @@ struct v4l2_subdev_pad_ops { > struct v4l2_mbus_frame_desc *fd); > int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, > struct v4l2_mbus_frame_desc *fd); > + int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad, > + struct v4l2_mbus_config *config); > + int (*set_mbus_config)(struct v4l2_subdev *sd, unsigned int pad, > + struct v4l2_mbus_config *config); > }; > > /** >
Hi Hans, On Wednesday, July 15, 2020 5:08:05 P.M. CEST Hans Verkuil wrote: > On 14/07/2020 15:58, Jacopo Mondi wrote: > > Introduce two new pad operations to allow retrieving and configuring the > > media bus parameters on a subdevice pad. > > > > The newly introduced operations aims to replace the s/g_mbus_config video > > operations, which have been on their way for deprecation since a long > > time. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > include/media/v4l2-subdev.h | 27 +++++++++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > index f7fe78a6f65a..d8b9d5735307 100644 > > --- a/include/media/v4l2-subdev.h > > +++ b/include/media/v4l2-subdev.h > > @@ -670,6 +670,29 @@ struct v4l2_subdev_pad_config { > > * > > * @set_frame_desc: set the low level media bus frame parameters, @fd array > > * may be adjusted by the subdev driver to device capabilities. > > + * > > + * @get_mbus_config: get the media bus configuration of a remote sub-device. > > + * The media bus configuration is usually retrieved from the > > + * firmware interface at sub-device probe time, immediately > > + * applied to the hardware and eventually adjusted by the > > + * driver. Remote sub-devices (usually video receivers) shall > > + * use this operation to query the transmitting end bus > > + * configuration in order to adjust their own one accordingly. > > + * Callers should make sure they get the most up-to-date as > > + * possible configuration from the remote end, likely calling > > + * this operation as close as possible to stream on time. The > > + * operation shall fail if the pad index it has been called on > > + * is not valid. > > + * > > + * @set_mbus_config: set the media bus configuration of a remote sub-device. > > + * This operations is intended to allow, in combination with > > + * the get_mbus_config operation, the negotiation of media bus > > + * configuration parameters between media sub-devices. The > > + * operation shall not fail if the requested configuration is > > + * not supported, but the driver shall update the content of > > + * the %config argument to reflect what has been actually > > + * applied to the hardware. The operation shall fail if the > > + * pad index it has been called on is not valid. > > */ > > Hm, I'd hoped I could merge this, but I think include/media/v4l2-mediabus.h > also needs to be updated. > > So the old g_mbus_config returned all supported configurations, while the > new get_mbus_config returns the *current* configuration. > > That's fine, but that means that the meaning of the struct v4l2_mbus_config > flags field changes as well and several comments in v4l2-mediabus.h need to > be updated to reflect this. > > E.g. instead of '/* How many lanes the client can use */' it becomes > '/* How many lanes the client uses */'. > > Frankly, these flags can be redesigned now since you only need a single > e.g. V4L2_MBUS_HSYNC_ACTIVE_HIGH flag since if it is not set, then that > means ACTIVE_LOW. And since it is now a single bit, it is also easy to > make each flag a bit field. Even if this makes sense for .get_mbus_config() which returns current configuration, how about keeping the old semantics of the flags and let .set_mbus_config() accept a potentially incomplete or redundant set of flags specified by the caller to select a supported configuration from? That approach was actually proposed before by Jacopo when he argued against my suggestion to add a wrapper with a check for mutually exclusive flags[1], and I found it a very good alternative to my other rejected suggestion of adding TRY support. [1] https://www.spinics.net/lists/linux-media/msg171878.html Thanks, Janusz > > The CSI2 lanes/channels can just be a bit field for the number of lanes/channels, > which is much easier to read. I strongly recommend making this change at the minimum. > > Now all this can be done in a follow-up series, but the v4l2-mediabus.h > definitely needs to be updated to reflect the new code. > > This can be done as a v6 5.5/9 patch (since it should come right after the > g/s_mbus_config removal). > > Regards, > > Hans > > > struct v4l2_subdev_pad_ops { > > int (*init_cfg)(struct v4l2_subdev *sd, > > @@ -710,6 +733,10 @@ struct v4l2_subdev_pad_ops { > > struct v4l2_mbus_frame_desc *fd); > > int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, > > struct v4l2_mbus_frame_desc *fd); > > + int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad, > > + struct v4l2_mbus_config *config); > > + int (*set_mbus_config)(struct v4l2_subdev *sd, unsigned int pad, > > + struct v4l2_mbus_config *config); > > }; > > > > /** > > > >
On 19/07/2020 13:18, Janusz Krzysztofik wrote: > Hi Hans, > > On Wednesday, July 15, 2020 5:08:05 P.M. CEST Hans Verkuil wrote: >> On 14/07/2020 15:58, Jacopo Mondi wrote: >>> Introduce two new pad operations to allow retrieving and configuring the >>> media bus parameters on a subdevice pad. >>> >>> The newly introduced operations aims to replace the s/g_mbus_config video >>> operations, which have been on their way for deprecation since a long >>> time. >>> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>> --- >>> include/media/v4l2-subdev.h | 27 +++++++++++++++++++++++++++ >>> 1 file changed, 27 insertions(+) >>> >>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h >>> index f7fe78a6f65a..d8b9d5735307 100644 >>> --- a/include/media/v4l2-subdev.h >>> +++ b/include/media/v4l2-subdev.h >>> @@ -670,6 +670,29 @@ struct v4l2_subdev_pad_config { >>> * >>> * @set_frame_desc: set the low level media bus frame parameters, @fd array >>> * may be adjusted by the subdev driver to device capabilities. >>> + * >>> + * @get_mbus_config: get the media bus configuration of a remote sub-device. >>> + * The media bus configuration is usually retrieved from the >>> + * firmware interface at sub-device probe time, immediately >>> + * applied to the hardware and eventually adjusted by the >>> + * driver. Remote sub-devices (usually video receivers) shall >>> + * use this operation to query the transmitting end bus >>> + * configuration in order to adjust their own one accordingly. >>> + * Callers should make sure they get the most up-to-date as >>> + * possible configuration from the remote end, likely calling >>> + * this operation as close as possible to stream on time. The >>> + * operation shall fail if the pad index it has been called on >>> + * is not valid. >>> + * >>> + * @set_mbus_config: set the media bus configuration of a remote sub-device. >>> + * This operations is intended to allow, in combination with >>> + * the get_mbus_config operation, the negotiation of media bus >>> + * configuration parameters between media sub-devices. The >>> + * operation shall not fail if the requested configuration is >>> + * not supported, but the driver shall update the content of >>> + * the %config argument to reflect what has been actually >>> + * applied to the hardware. The operation shall fail if the >>> + * pad index it has been called on is not valid. >>> */ >> >> Hm, I'd hoped I could merge this, but I think include/media/v4l2-mediabus.h >> also needs to be updated. >> >> So the old g_mbus_config returned all supported configurations, while the >> new get_mbus_config returns the *current* configuration. >> >> That's fine, but that means that the meaning of the struct v4l2_mbus_config >> flags field changes as well and several comments in v4l2-mediabus.h need to >> be updated to reflect this. >> >> E.g. instead of '/* How many lanes the client can use */' it becomes >> '/* How many lanes the client uses */'. >> >> Frankly, these flags can be redesigned now since you only need a single >> e.g. V4L2_MBUS_HSYNC_ACTIVE_HIGH flag since if it is not set, then that >> means ACTIVE_LOW. And since it is now a single bit, it is also easy to >> make each flag a bit field. > > Even if this makes sense for .get_mbus_config() which returns current > configuration, how about keeping the old semantics of the flags and let > .set_mbus_config() accept a potentially incomplete or redundant set of flags > specified by the caller to select a supported configuration from? That approach > was actually proposed before by Jacopo when he argued against my suggestion to > add a wrapper with a check for mutually exclusive flags[1], and I found it a > very good alternative to my other rejected suggestion of adding TRY support. The information on how a sensor (or similar device) is wired up is not something that should be negotiated. Even if a combination is theoretically possible, it may not have been tested by the board designer and in fact it might not work. (Yes, that happens) It is just a bad design trying to negotiate this. In fact, the only values that can be set as far as I am concerned are lanes and channels. I wouldn't mind if the other settings are purely read-only. The only driver that actively sets this is the pxa_camera driver and I wish it didn't. But there are still two pxa boards that use this mechanism, so I guess we still have to allow this. Anyway, do you have a specific use-case in mind? Note that this is an internal API, so it can always be changed later. Regards, Hans > > [1] https://www.spinics.net/lists/linux-media/msg171878.html > > Thanks, > Janusz > >> >> The CSI2 lanes/channels can just be a bit field for the number of lanes/channels, >> which is much easier to read. I strongly recommend making this change at the minimum. >> >> Now all this can be done in a follow-up series, but the v4l2-mediabus.h >> definitely needs to be updated to reflect the new code. >> >> This can be done as a v6 5.5/9 patch (since it should come right after the >> g/s_mbus_config removal). >> >> Regards, >> >> Hans >> >>> struct v4l2_subdev_pad_ops { >>> int (*init_cfg)(struct v4l2_subdev *sd, >>> @@ -710,6 +733,10 @@ struct v4l2_subdev_pad_ops { >>> struct v4l2_mbus_frame_desc *fd); >>> int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, >>> struct v4l2_mbus_frame_desc *fd); >>> + int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad, >>> + struct v4l2_mbus_config *config); >>> + int (*set_mbus_config)(struct v4l2_subdev *sd, unsigned int pad, >>> + struct v4l2_mbus_config *config); >>> }; >>> >>> /** >>> >> >> > > > >
On Monday, July 20, 2020 10:48:36 A.M. CEST Hans Verkuil wrote: > On 19/07/2020 13:18, Janusz Krzysztofik wrote: > > Hi Hans, > > > > On Wednesday, July 15, 2020 5:08:05 P.M. CEST Hans Verkuil wrote: > >> On 14/07/2020 15:58, Jacopo Mondi wrote: > >>> Introduce two new pad operations to allow retrieving and configuring the > >>> media bus parameters on a subdevice pad. > >>> > >>> The newly introduced operations aims to replace the s/g_mbus_config video > >>> operations, which have been on their way for deprecation since a long > >>> time. > >>> > >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > >>> --- > >>> include/media/v4l2-subdev.h | 27 +++++++++++++++++++++++++++ > >>> 1 file changed, 27 insertions(+) > >>> > >>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > >>> index f7fe78a6f65a..d8b9d5735307 100644 > >>> --- a/include/media/v4l2-subdev.h > >>> +++ b/include/media/v4l2-subdev.h > >>> @@ -670,6 +670,29 @@ struct v4l2_subdev_pad_config { > >>> * > >>> * @set_frame_desc: set the low level media bus frame parameters, @fd array > >>> * may be adjusted by the subdev driver to device capabilities. > >>> + * > >>> + * @get_mbus_config: get the media bus configuration of a remote sub- device. > >>> + * The media bus configuration is usually retrieved from the > >>> + * firmware interface at sub-device probe time, immediately > >>> + * applied to the hardware and eventually adjusted by the > >>> + * driver. Remote sub-devices (usually video receivers) shall > >>> + * use this operation to query the transmitting end bus > >>> + * configuration in order to adjust their own one accordingly. > >>> + * Callers should make sure they get the most up-to- date as > >>> + * possible configuration from the remote end, likely calling > >>> + * this operation as close as possible to stream on time. The > >>> + * operation shall fail if the pad index it has been called on > >>> + * is not valid. > >>> + * > >>> + * @set_mbus_config: set the media bus configuration of a remote sub- device. > >>> + * This operations is intended to allow, in combination with > >>> + * the get_mbus_config operation, the negotiation of media bus > >>> + * configuration parameters between media sub- devices. The > >>> + * operation shall not fail if the requested configuration is > >>> + * not supported, but the driver shall update the content of > >>> + * the %config argument to reflect what has been actually > >>> + * applied to the hardware. The operation shall fail if the > >>> + * pad index it has been called on is not valid. > >>> */ > >> > >> Hm, I'd hoped I could merge this, but I think include/media/v4l2- mediabus.h > >> also needs to be updated. > >> > >> So the old g_mbus_config returned all supported configurations, while the > >> new get_mbus_config returns the *current* configuration. > >> > >> That's fine, but that means that the meaning of the struct v4l2_mbus_config > >> flags field changes as well and several comments in v4l2-mediabus.h need to > >> be updated to reflect this. > >> > >> E.g. instead of '/* How many lanes the client can use */' it becomes > >> '/* How many lanes the client uses */'. > >> > >> Frankly, these flags can be redesigned now since you only need a single > >> e.g. V4L2_MBUS_HSYNC_ACTIVE_HIGH flag since if it is not set, then that > >> means ACTIVE_LOW. And since it is now a single bit, it is also easy to > >> make each flag a bit field. > > > > Even if this makes sense for .get_mbus_config() which returns current > > configuration, how about keeping the old semantics of the flags and let > > .set_mbus_config() accept a potentially incomplete or redundant set of flags > > specified by the caller to select a supported configuration from? That approach > > was actually proposed before by Jacopo when he argued against my suggestion to > > add a wrapper with a check for mutually exclusive flags[1], and I found it a > > very good alternative to my other rejected suggestion of adding TRY support. > > The information on how a sensor (or similar device) is wired up is not something > that should be negotiated. Even if a combination is theoretically possible, it > may not have been tested by the board designer and in fact it might not work. > (Yes, that happens) > > It is just a bad design trying to negotiate this. > > In fact, the only values that can be set as far as I am concerned are lanes and > channels. I wouldn't mind if the other settings are purely read-only. The only > driver that actively sets this is the pxa_camera driver and I wish it didn't. > > But there are still two pxa boards that use this mechanism, so I guess we still > have to allow this. > > Anyway, do you have a specific use-case in mind? Non-DT platforms in general. You repeat quite often that configuration should come from DT. While that's obvious, does that mean media is soon going to drop non-DT support completely? If not then I think that it may be better to allow negotiation where possible than require each platform driver that still wishes to support non-DT platforms to define its own platform data structure. At least parallel link settings seem to be a good candidate to me. Anyway, as long as .set_mbus_config() is going to be supported, it is still possible for a platform driver to iterate through its supported configurations if passing incomplete or redundant flags and let the sensor handle that is not allowed. More complicated, more time consuming, more error prone, I believe, but still possible. Thanks, Janusz > Note that this is an internal > API, so it can always be changed later. > > Regards, > > Hans > > > > > [1] https://www.spinics.net/lists/linux-media/msg171878.html > > > > Thanks, > > Janusz > > > >> > >> The CSI2 lanes/channels can just be a bit field for the number of lanes/ channels, > >> which is much easier to read. I strongly recommend making this change at the minimum. > >> > >> Now all this can be done in a follow-up series, but the v4l2-mediabus.h > >> definitely needs to be updated to reflect the new code. > >> > >> This can be done as a v6 5.5/9 patch (since it should come right after the > >> g/s_mbus_config removal). > >> > >> Regards, > >> > >> Hans > >> > >>> struct v4l2_subdev_pad_ops { > >>> int (*init_cfg)(struct v4l2_subdev *sd, > >>> @@ -710,6 +733,10 @@ struct v4l2_subdev_pad_ops { > >>> struct v4l2_mbus_frame_desc *fd); > >>> int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, > >>> struct v4l2_mbus_frame_desc *fd); > >>> + int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad, > >>> + struct v4l2_mbus_config *config); > >>> + int (*set_mbus_config)(struct v4l2_subdev *sd, unsigned int pad, > >>> + struct v4l2_mbus_config *config); > >>> }; > >>> > >>> /** > >>> > >> > >> > > > > > > > > > >
On 20/07/2020 20:44, Janusz Krzysztofik wrote: > On Monday, July 20, 2020 10:48:36 A.M. CEST Hans Verkuil wrote: >> On 19/07/2020 13:18, Janusz Krzysztofik wrote: >>> Hi Hans, >>> >>> On Wednesday, July 15, 2020 5:08:05 P.M. CEST Hans Verkuil wrote: >>>> On 14/07/2020 15:58, Jacopo Mondi wrote: >>>>> Introduce two new pad operations to allow retrieving and configuring the >>>>> media bus parameters on a subdevice pad. >>>>> >>>>> The newly introduced operations aims to replace the s/g_mbus_config video >>>>> operations, which have been on their way for deprecation since a long >>>>> time. >>>>> >>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>>>> --- >>>>> include/media/v4l2-subdev.h | 27 +++++++++++++++++++++++++++ >>>>> 1 file changed, 27 insertions(+) >>>>> >>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h >>>>> index f7fe78a6f65a..d8b9d5735307 100644 >>>>> --- a/include/media/v4l2-subdev.h >>>>> +++ b/include/media/v4l2-subdev.h >>>>> @@ -670,6 +670,29 @@ struct v4l2_subdev_pad_config { >>>>> * >>>>> * @set_frame_desc: set the low level media bus frame parameters, @fd > array >>>>> * may be adjusted by the subdev driver to device > capabilities. >>>>> + * >>>>> + * @get_mbus_config: get the media bus configuration of a remote sub- > device. >>>>> + * The media bus configuration is usually retrieved > from the >>>>> + * firmware interface at sub-device probe time, > immediately >>>>> + * applied to the hardware and eventually adjusted > by the >>>>> + * driver. Remote sub-devices (usually video > receivers) shall >>>>> + * use this operation to query the transmitting end > bus >>>>> + * configuration in order to adjust their own one > accordingly. >>>>> + * Callers should make sure they get the most up-to- > date as >>>>> + * possible configuration from the remote end, likely > calling >>>>> + * this operation as close as possible to stream on > time. The >>>>> + * operation shall fail if the pad index it has been > called on >>>>> + * is not valid. >>>>> + * >>>>> + * @set_mbus_config: set the media bus configuration of a remote sub- > device. >>>>> + * This operations is intended to allow, in > combination with >>>>> + * the get_mbus_config operation, the negotiation of > media bus >>>>> + * configuration parameters between media sub- > devices. The >>>>> + * operation shall not fail if the requested > configuration is >>>>> + * not supported, but the driver shall update the > content of >>>>> + * the %config argument to reflect what has been > actually >>>>> + * applied to the hardware. The operation shall fail > if the >>>>> + * pad index it has been called on is not valid. >>>>> */ >>>> >>>> Hm, I'd hoped I could merge this, but I think include/media/v4l2- > mediabus.h >>>> also needs to be updated. >>>> >>>> So the old g_mbus_config returned all supported configurations, while the >>>> new get_mbus_config returns the *current* configuration. >>>> >>>> That's fine, but that means that the meaning of the struct v4l2_mbus_config >>>> flags field changes as well and several comments in v4l2-mediabus.h need to >>>> be updated to reflect this. >>>> >>>> E.g. instead of '/* How many lanes the client can use */' it becomes >>>> '/* How many lanes the client uses */'. >>>> >>>> Frankly, these flags can be redesigned now since you only need a single >>>> e.g. V4L2_MBUS_HSYNC_ACTIVE_HIGH flag since if it is not set, then that >>>> means ACTIVE_LOW. And since it is now a single bit, it is also easy to >>>> make each flag a bit field. >>> >>> Even if this makes sense for .get_mbus_config() which returns current >>> configuration, how about keeping the old semantics of the flags and let >>> .set_mbus_config() accept a potentially incomplete or redundant set of flags >>> specified by the caller to select a supported configuration from? That > approach >>> was actually proposed before by Jacopo when he argued against my > suggestion to >>> add a wrapper with a check for mutually exclusive flags[1], and I found it > a >>> very good alternative to my other rejected suggestion of adding TRY > support. >> >> The information on how a sensor (or similar device) is wired up is not > something >> that should be negotiated. Even if a combination is theoretically possible, > it >> may not have been tested by the board designer and in fact it might not > work. >> (Yes, that happens) >> >> It is just a bad design trying to negotiate this. >> >> In fact, the only values that can be set as far as I am concerned are lanes > and >> channels. I wouldn't mind if the other settings are purely read-only. The > only >> driver that actively sets this is the pxa_camera driver and I wish it > didn't. >> >> But there are still two pxa boards that use this mechanism, so I guess we > still >> have to allow this. >> >> Anyway, do you have a specific use-case in mind? > > Non-DT platforms in general. You repeat quite often that configuration > should come from DT. While that's obvious, does that mean media is soon going > to drop non-DT support completely? If not then I think that it may be better It's close to dropping non-DT already. The ov6650 is the only sensor driver that supports s_mbus_config today, so trying to negotiate these settings is only possible with that sensor. In any case, this is really not something that should be negotiated, regardless of DT vs platform. Negotiating this was and is a bad idea and instead you should have a well defined and tested configuration defined in either the DT or in platform data. > to allow negotiation where possible than require each platform driver that > still wishes to support non-DT platforms to define its own platform data > structure. At least parallel link settings seem to be a good candidate to me. > > Anyway, as long as .set_mbus_config() is going to be supported, it is still > possible for a platform driver to iterate through its supported configurations > if passing incomplete or redundant flags and let the sensor handle that is not > allowed. More complicated, more time consuming, more error prone, I believe, > but still possible. It's possible, but it is just not a good idea. As I mentioned before, typically only one combination is actually tested, and negotiating this might end up choosing a combination of settings that, while valid, is actually not working due to HW bugs that were never noticed because the designer never tested that combination. The other reason is that this makes things so much more complicated. It is much easier and safer to just be explicit in the dts or platform data. Regards, Hans > > Thanks, > Janusz
Hans Verkuil <hverkuil-cisco@xs4all.nl> writes: > The information on how a sensor (or similar device) is wired up is not something > that should be negotiated. Even if a combination is theoretically possible, it > may not have been tested by the board designer and in fact it might not work. > (Yes, that happens) > > It is just a bad design trying to negotiate this. I quite agree on that one (on the wiring defined by configuration). > In fact, the only values that can be set as far as I am concerned are lanes and > channels. I wouldn't mind if the other settings are purely read-only. The only > driver that actively sets this is the pxa_camera driver and I wish it didn't. > > But there are still two pxa boards that use this mechanism, so I guess we still > have to allow this. There are 4 : rj@belgarion:~/mio_linux/kernel/arch/arm/mach-pxa$ grep -rs pxa_set_camera * | grep -v devices.c ezx.c: pxa_set_camera_info(&a780_pxacamera_platform_data); ezx.c: pxa_set_camera_info(&a910_pxacamera_platform_data); mioa701.c: pxa_set_camera_info(&mioa701_pxacamera_platform_data); palmz72.c: pxa_set_camera_info(&palmz72_pxacamera_platform_data); pcm990-baseboard.c: pxa_set_camera_info(&pcm990_pxacamera_platform_data); I wouldn't mind that the bus parameters are made "static" by forcing them in the platform data field (struct pxacamera_platform_data), and not doing the bus compatibility matching. I thing Jacopo's work is going this way, as I read that his patch takes first the platform data flags, and only if none are defined goes to the matchup. That looks very sensible to me. Cheers. -- Robert
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index f7fe78a6f65a..d8b9d5735307 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -670,6 +670,29 @@ struct v4l2_subdev_pad_config { * * @set_frame_desc: set the low level media bus frame parameters, @fd array * may be adjusted by the subdev driver to device capabilities. + * + * @get_mbus_config: get the media bus configuration of a remote sub-device. + * The media bus configuration is usually retrieved from the + * firmware interface at sub-device probe time, immediately + * applied to the hardware and eventually adjusted by the + * driver. Remote sub-devices (usually video receivers) shall + * use this operation to query the transmitting end bus + * configuration in order to adjust their own one accordingly. + * Callers should make sure they get the most up-to-date as + * possible configuration from the remote end, likely calling + * this operation as close as possible to stream on time. The + * operation shall fail if the pad index it has been called on + * is not valid. + * + * @set_mbus_config: set the media bus configuration of a remote sub-device. + * This operations is intended to allow, in combination with + * the get_mbus_config operation, the negotiation of media bus + * configuration parameters between media sub-devices. The + * operation shall not fail if the requested configuration is + * not supported, but the driver shall update the content of + * the %config argument to reflect what has been actually + * applied to the hardware. The operation shall fail if the + * pad index it has been called on is not valid. */ struct v4l2_subdev_pad_ops { int (*init_cfg)(struct v4l2_subdev *sd, @@ -710,6 +733,10 @@ struct v4l2_subdev_pad_ops { struct v4l2_mbus_frame_desc *fd); int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, struct v4l2_mbus_frame_desc *fd); + int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad, + struct v4l2_mbus_config *config); + int (*set_mbus_config)(struct v4l2_subdev *sd, unsigned int pad, + struct v4l2_mbus_config *config); }; /**
Introduce two new pad operations to allow retrieving and configuring the media bus parameters on a subdevice pad. The newly introduced operations aims to replace the s/g_mbus_config video operations, which have been on their way for deprecation since a long time. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- include/media/v4l2-subdev.h | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)