| Message ID | 20220728130237.3396663-4-alexander.stein@ew.tq-group.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | OV9281 support | expand |
On 28/07/2022 15:02, Alexander Stein wrote: > According to product brief they are identical from software point of view. > Differences are a different chief ray angle (CRA) and the package. > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com> > --- > drivers/media/i2c/ov9282.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c > index 8a252bf3b59f..c8d83a29f9bb 100644 > --- a/drivers/media/i2c/ov9282.c > +++ b/drivers/media/i2c/ov9282.c > @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = { > }; > > static const struct of_device_id ov9282_of_match[] = { > + { .compatible = "ovti,ov9281" }, The devices seem entirely compatible, so why you add a new compatible and not re-use existing? The difference in lens does not explain this. Best regards, Krzysztof
Hi Krzysztof, On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote: > On 28/07/2022 15:02, Alexander Stein wrote: > > According to product brief they are identical from software point of view. > > Differences are a different chief ray angle (CRA) and the package. > > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com> > > --- > > drivers/media/i2c/ov9282.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c > > index 8a252bf3b59f..c8d83a29f9bb 100644 > > --- a/drivers/media/i2c/ov9282.c > > +++ b/drivers/media/i2c/ov9282.c > > @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = { > > }; > > > > static const struct of_device_id ov9282_of_match[] = { > > + { .compatible = "ovti,ov9281" }, > > The devices seem entirely compatible, so why you add a new compatible > and not re-use existing? > > The difference in lens does not explain this. It is typically necessary to know what kind of related hardware can be found in the system, beyond just the device's register interface. Apart from USB cameras, less integrated cameras require low-level software control in which specific device properties are important. In this case it could be the lens shading table, among other things. https://www.ovt.com/sensor/ov9282/ Therefore I think adding a specific compatible string for this one is justified. Also cc Laurent.
Hi Sakari, (Adding Dave and Naush to the CC list) On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote: > On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote: > > On 28/07/2022 15:02, Alexander Stein wrote: > > > According to product brief they are identical from software point of view. > > > Differences are a different chief ray angle (CRA) and the package. > > > > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > > Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com> > > > --- > > > drivers/media/i2c/ov9282.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c > > > index 8a252bf3b59f..c8d83a29f9bb 100644 > > > --- a/drivers/media/i2c/ov9282.c > > > +++ b/drivers/media/i2c/ov9282.c > > > @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = { > > > }; > > > > > > static const struct of_device_id ov9282_of_match[] = { > > > + { .compatible = "ovti,ov9281" }, > > > > The devices seem entirely compatible, so why you add a new compatible > > and not re-use existing? > > > > The difference in lens does not explain this. > > It is typically necessary to know what kind of related hardware can be > found in the system, beyond just the device's register interface. Apart > from USB cameras, less integrated cameras require low-level software > control in which specific device properties are important. In this case it > could be the lens shading table, among other things. > > https://www.ovt.com/sensor/ov9282/ > > Therefore I think adding a specific compatible string for this one is > justified. > > Also cc Laurent. Interesting coincidence, we've talked about this topic (as part of a broader discussion) no later than yesterday. I agree with Sakari in that userspace needs to know the exact model of the camera sensor. I don't see a good alternative to providing that information through the platform firmware, so the device tree in this case. The question is how it should be provided (the question of how it should then be exposed to userspace is also important, but out of scope in this discussion). The compatible string is meant to indicate a device's compatibility with "something", and that something is often considered from the point of view of software support, and in particular to pick an appropriate kernel driver and tune its behaviour for the device. Here, one could argue that the exact model is also needed to ensure proper software support, but in userspace this time, not in the kernel. I think using a dedicated compatible string would be reasonable. An alternative would be to use another DT property, which should then be standardized. I'm not sure it's worth it. Broadening the discussion, we also need to know detailed information about the camera lens (I'm talking about the lens itself here, not the lens controller IC that controls the motor that moves the focus lens). The lens isn't described in the device tree with a dedicated device tree node today, and I don't think it should (I'd have a hard time coming up with a naming scheme for lenses that we could use in compatible strings, and the lens-related data that a system requires can possibly vary based not only on the lens itself but on the ISP that the camera sensor is used with). Typical useful data are the lens movement range, the hyperfocal distance, but also the lens shading tables. (Part of) that information is sometimes stored in non-volatile memory in the camera module (OTP in the camera sensor itself, or a separate EEPROM), but that's not always the case. We have considered the possibility of storing the information in the device tree, but I doubt that would be accepted. We can store the information in userspace in configuration files, but we will still need to device tree to provide lens identification information to select the correct configuration file. I don't know how that should be done.
On 29/07/2022 10:18, Laurent Pinchart wrote: > Hi Sakari, > > (Adding Dave and Naush to the CC list) > > On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote: >> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote: >>> On 28/07/2022 15:02, Alexander Stein wrote: >>>> According to product brief they are identical from software point of view. >>>> Differences are a different chief ray angle (CRA) and the package. >>>> >>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> >>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com> >>>> --- >>>> drivers/media/i2c/ov9282.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c >>>> index 8a252bf3b59f..c8d83a29f9bb 100644 >>>> --- a/drivers/media/i2c/ov9282.c >>>> +++ b/drivers/media/i2c/ov9282.c >>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = { >>>> }; >>>> >>>> static const struct of_device_id ov9282_of_match[] = { >>>> + { .compatible = "ovti,ov9281" }, >>> >>> The devices seem entirely compatible, so why you add a new compatible >>> and not re-use existing? >>> >>> The difference in lens does not explain this. >> >> It is typically necessary to know what kind of related hardware can be >> found in the system, beyond just the device's register interface. Apart >> from USB cameras, less integrated cameras require low-level software >> control in which specific device properties are important. In this case it >> could be the lens shading table, among other things. >> >> https://www.ovt.com/sensor/ov9282/ >> >> Therefore I think adding a specific compatible string for this one is >> justified. Specific compatible in binding is a requirement. No one discussed this. However not in the driver. None of the arguments above justify adding such binding, unless user-space depends on matching compatible, but not real compatible? >> >> Also cc Laurent. > > Interesting coincidence, we've talked about this topic (as part of a > broader discussion) no later than yesterday. > > I agree with Sakari in that userspace needs to know the exact model of > the camera sensor. I don't see a good alternative to providing that > information through the platform firmware, so the device tree in this > case. The question is how it should be provided (the question of how it > should then be exposed to userspace is also important, but out of scope > in this discussion). > > The compatible string is meant to indicate a device's compatibility with > "something", and that something is often considered from the point of > view of software support, and in particular to pick an appropriate > kernel driver and tune its behaviour for the device. Here, one could > argue that the exact model is also needed to ensure proper software > support, but in userspace this time, not in the kernel. I think using a > dedicated compatible string would be reasonable. An alternative would be > to use another DT property, which should then be standardized. I'm not > sure it's worth it. > > Broadening the discussion, we also need to know detailed information > about the camera lens (I'm talking about the lens itself here, not the > lens controller IC that controls the motor that moves the focus lens). > The lens isn't described in the device tree with a dedicated device tree > node today, and I don't think it should (I'd have a hard time coming up > with a naming scheme for lenses that we could use in compatible strings, > and the lens-related data that a system requires can possibly vary based > not only on the lens itself but on the ISP that the camera sensor is > used with). Typical useful data are the lens movement range, the > hyperfocal distance, but also the lens shading tables. (Part of) that > information is sometimes stored in non-volatile memory in the camera > module (OTP in the camera sensor itself, or a separate EEPROM), but > that's not always the case. We have considered the possibility of > storing the information in the device tree, but I doubt that would be > accepted. We can store the information in userspace in configuration > files, but we will still need to device tree to provide lens > identification information to select the correct configuration file. I > don't know how that should be done. It seems both you and Sakari suggested not to have specific compatible. Such idea (not to have specific compatible) was not proposed by me. Quite contrary - specific compatible is a requirement. However device driver does no need it. Just use fallback for the driver. Best regards, Krzysztof
On 01/08/2022 20:07, Krzysztof Kozlowski wrote: > On 29/07/2022 10:18, Laurent Pinchart wrote: >> Hi Sakari, >> >> (Adding Dave and Naush to the CC list) >> >> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote: >>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote: >>>> On 28/07/2022 15:02, Alexander Stein wrote: >>>>> According to product brief they are identical from software point of view. >>>>> Differences are a different chief ray angle (CRA) and the package. >>>>> >>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> >>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com> >>>>> --- >>>>> drivers/media/i2c/ov9282.c | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c >>>>> index 8a252bf3b59f..c8d83a29f9bb 100644 >>>>> --- a/drivers/media/i2c/ov9282.c >>>>> +++ b/drivers/media/i2c/ov9282.c >>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = { >>>>> }; >>>>> >>>>> static const struct of_device_id ov9282_of_match[] = { >>>>> + { .compatible = "ovti,ov9281" }, >>>> >>>> The devices seem entirely compatible, so why you add a new compatible >>>> and not re-use existing? >>>> >>>> The difference in lens does not explain this. >>> >>> It is typically necessary to know what kind of related hardware can be >>> found in the system, beyond just the device's register interface. Apart >>> from USB cameras, less integrated cameras require low-level software >>> control in which specific device properties are important. In this case it >>> could be the lens shading table, among other things. >>> >>> https://www.ovt.com/sensor/ov9282/ >>> >>> Therefore I think adding a specific compatible string for this one is >>> justified. > > Specific compatible in binding is a requirement. No one discussed this. > However not in the driver. None of the arguments above justify adding > such binding, unless user-space depends on matching compatible, but not > real compatible? Eh, now I used vague words. This should be instead: "However not in the driver. None of the arguments above justify adding such compatible to driver, unless user-space depends on matching compatible, but not real compatible?" > >>> >>> Also cc Laurent. >> >> Interesting coincidence, we've talked about this topic (as part of a >> broader discussion) no later than yesterday. >> >> I agree with Sakari in that userspace needs to know the exact model of >> the camera sensor. I don't see a good alternative to providing that >> information through the platform firmware, so the device tree in this >> case. The question is how it should be provided (the question of how it >> should then be exposed to userspace is also important, but out of scope >> in this discussion). >> >> The compatible string is meant to indicate a device's compatibility with >> "something", and that something is often considered from the point of >> view of software support, and in particular to pick an appropriate >> kernel driver and tune its behaviour for the device. Here, one could >> argue that the exact model is also needed to ensure proper software >> support, but in userspace this time, not in the kernel. I think using a >> dedicated compatible string would be reasonable. An alternative would be >> to use another DT property, which should then be standardized. I'm not >> sure it's worth it. >> >> Broadening the discussion, we also need to know detailed information >> about the camera lens (I'm talking about the lens itself here, not the >> lens controller IC that controls the motor that moves the focus lens). >> The lens isn't described in the device tree with a dedicated device tree >> node today, and I don't think it should (I'd have a hard time coming up >> with a naming scheme for lenses that we could use in compatible strings, >> and the lens-related data that a system requires can possibly vary based >> not only on the lens itself but on the ISP that the camera sensor is >> used with). Typical useful data are the lens movement range, the >> hyperfocal distance, but also the lens shading tables. (Part of) that >> information is sometimes stored in non-volatile memory in the camera >> module (OTP in the camera sensor itself, or a separate EEPROM), but >> that's not always the case. We have considered the possibility of >> storing the information in the device tree, but I doubt that would be >> accepted. We can store the information in userspace in configuration >> files, but we will still need to device tree to provide lens >> identification information to select the correct configuration file. I >> don't know how that should be done. > > It seems both you and Sakari suggested not to have specific compatible. > Such idea (not to have specific compatible) was not proposed by me. > Quite contrary - specific compatible is a requirement. However device > driver does no need it. Just use fallback for the driver. Best regards, Krzysztof
On Mon, Aug 01, 2022 at 08:08:58PM +0200, Krzysztof Kozlowski wrote: > On 01/08/2022 20:07, Krzysztof Kozlowski wrote: > > On 29/07/2022 10:18, Laurent Pinchart wrote: > >> Hi Sakari, > >> > >> (Adding Dave and Naush to the CC list) > >> > >> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote: > >>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote: > >>>> On 28/07/2022 15:02, Alexander Stein wrote: > >>>>> According to product brief they are identical from software point of view. > >>>>> Differences are a different chief ray angle (CRA) and the package. > >>>>> > >>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > >>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com> > >>>>> --- > >>>>> drivers/media/i2c/ov9282.c | 1 + > >>>>> 1 file changed, 1 insertion(+) > >>>>> > >>>>> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c > >>>>> index 8a252bf3b59f..c8d83a29f9bb 100644 > >>>>> --- a/drivers/media/i2c/ov9282.c > >>>>> +++ b/drivers/media/i2c/ov9282.c > >>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = { > >>>>> }; > >>>>> > >>>>> static const struct of_device_id ov9282_of_match[] = { > >>>>> + { .compatible = "ovti,ov9281" }, > >>>> > >>>> The devices seem entirely compatible, so why you add a new compatible > >>>> and not re-use existing? > >>>> > >>>> The difference in lens does not explain this. > >>> > >>> It is typically necessary to know what kind of related hardware can be > >>> found in the system, beyond just the device's register interface. Apart > >>> from USB cameras, less integrated cameras require low-level software > >>> control in which specific device properties are important. In this case it > >>> could be the lens shading table, among other things. > >>> > >>> https://www.ovt.com/sensor/ov9282/ > >>> > >>> Therefore I think adding a specific compatible string for this one is > >>> justified. > > > > Specific compatible in binding is a requirement. No one discussed this. > > However not in the driver. None of the arguments above justify adding > > such binding, unless user-space depends on matching compatible, but not > > real compatible? > > Eh, now I used vague words. This should be instead: > > "However not in the driver. None of the arguments above justify adding > such compatible to driver, unless user-space depends on matching > compatible, but not real compatible?" If I understand you right, you'd put the more specific model name as well as the more generic one to the compatible property and let the driver match against the more generic one? But in this case neither of these models is more generic than the other.
On 02/08/2022 10:23, Sakari Ailus wrote: > On Mon, Aug 01, 2022 at 08:08:58PM +0200, Krzysztof Kozlowski wrote: >> On 01/08/2022 20:07, Krzysztof Kozlowski wrote: >>> On 29/07/2022 10:18, Laurent Pinchart wrote: >>>> Hi Sakari, >>>> >>>> (Adding Dave and Naush to the CC list) >>>> >>>> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote: >>>>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote: >>>>>> On 28/07/2022 15:02, Alexander Stein wrote: >>>>>>> According to product brief they are identical from software point of view. >>>>>>> Differences are a different chief ray angle (CRA) and the package. >>>>>>> >>>>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> >>>>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com> >>>>>>> --- >>>>>>> drivers/media/i2c/ov9282.c | 1 + >>>>>>> 1 file changed, 1 insertion(+) >>>>>>> >>>>>>> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c >>>>>>> index 8a252bf3b59f..c8d83a29f9bb 100644 >>>>>>> --- a/drivers/media/i2c/ov9282.c >>>>>>> +++ b/drivers/media/i2c/ov9282.c >>>>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = { >>>>>>> }; >>>>>>> >>>>>>> static const struct of_device_id ov9282_of_match[] = { >>>>>>> + { .compatible = "ovti,ov9281" }, >>>>>> >>>>>> The devices seem entirely compatible, so why you add a new compatible >>>>>> and not re-use existing? >>>>>> >>>>>> The difference in lens does not explain this. >>>>> >>>>> It is typically necessary to know what kind of related hardware can be >>>>> found in the system, beyond just the device's register interface. Apart >>>>> from USB cameras, less integrated cameras require low-level software >>>>> control in which specific device properties are important. In this case it >>>>> could be the lens shading table, among other things. >>>>> >>>>> https://www.ovt.com/sensor/ov9282/ >>>>> >>>>> Therefore I think adding a specific compatible string for this one is >>>>> justified. >>> >>> Specific compatible in binding is a requirement. No one discussed this. >>> However not in the driver. None of the arguments above justify adding >>> such binding, unless user-space depends on matching compatible, but not >>> real compatible? >> >> Eh, now I used vague words. This should be instead: >> >> "However not in the driver. None of the arguments above justify adding >> such compatible to driver, unless user-space depends on matching >> compatible, but not real compatible?" > > If I understand you right, you'd put the more specific model name as well > as the more generic one to the compatible property and let the driver match > against the more generic one? Yes. > > But in this case neither of these models is more generic than the other. It's not a problem. Also the spec explains it similar way: "They allow a device to express its compatibility with a family of similar devices, potentially allowing a single device driver to match against several devices." Of course the numbers would suggest that ov9281 should be the family (as lower number usually means designed earlier), but it is a matter of convention which here can be skipped. The point is that ov9281 and ov9282 are compatible between each other, therefore they belong to single family. Best regards, Krzysztof
Hello, Am Dienstag, 2. August 2022, 10:30:40 CEST schrieb Krzysztof Kozlowski: > On 02/08/2022 10:23, Sakari Ailus wrote: > > On Mon, Aug 01, 2022 at 08:08:58PM +0200, Krzysztof Kozlowski wrote: > >> On 01/08/2022 20:07, Krzysztof Kozlowski wrote: > >>> On 29/07/2022 10:18, Laurent Pinchart wrote: > >>>> Hi Sakari, > >>>> > >>>> (Adding Dave and Naush to the CC list) > >>>> > >>>> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote: > >>>>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote: > >>>>>> On 28/07/2022 15:02, Alexander Stein wrote: > >>>>>>> According to product brief they are identical from software point of > >>>>>>> view. > >>>>>>> Differences are a different chief ray angle (CRA) and the package. > >>>>>>> > >>>>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > >>>>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com> > >>>>>>> --- > >>>>>>> > >>>>>>> drivers/media/i2c/ov9282.c | 1 + > >>>>>>> 1 file changed, 1 insertion(+) > >>>>>>> > >>>>>>> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c > >>>>>>> index 8a252bf3b59f..c8d83a29f9bb 100644 > >>>>>>> --- a/drivers/media/i2c/ov9282.c > >>>>>>> +++ b/drivers/media/i2c/ov9282.c > >>>>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = > >>>>>>> { > >>>>>>> > >>>>>>> }; > >>>>>>> > >>>>>>> static const struct of_device_id ov9282_of_match[] = { > >>>>>>> > >>>>>>> + { .compatible = "ovti,ov9281" }, > >>>>>> > >>>>>> The devices seem entirely compatible, so why you add a new compatible > >>>>>> and not re-use existing? > >>>>>> > >>>>>> The difference in lens does not explain this. > >>>>> > >>>>> It is typically necessary to know what kind of related hardware can be > >>>>> found in the system, beyond just the device's register interface. > >>>>> Apart > >>>>> from USB cameras, less integrated cameras require low-level software > >>>>> control in which specific device properties are important. In this > >>>>> case it > >>>>> could be the lens shading table, among other things. > >>>>> > >>>>> https://www.ovt.com/sensor/ov9282/ > >>>>> > >>>>> Therefore I think adding a specific compatible string for this one is > >>>>> justified. > >>> > >>> Specific compatible in binding is a requirement. No one discussed this. > >>> However not in the driver. None of the arguments above justify adding > >>> such binding, unless user-space depends on matching compatible, but not > >>> real compatible? > >> > >> Eh, now I used vague words. This should be instead: > >> > >> "However not in the driver. None of the arguments above justify adding > >> such compatible to driver, unless user-space depends on matching > >> compatible, but not real compatible?" > > > > If I understand you right, you'd put the more specific model name as well > > as the more generic one to the compatible property and let the driver > > match > > against the more generic one? > > Yes. > > > But in this case neither of these models is more generic than the other. > > It's not a problem. Also the spec explains it similar way: > "They > allow a device to express its compatibility with a family of similar > devices, potentially allowing a single > device driver to match against several devices." > > Of course the numbers would suggest that ov9281 should be the family (as > lower number usually means designed earlier), but it is a matter of > convention which here can be skipped. The point is that ov9281 and > ov9282 are compatible between each other, therefore they belong to > single family. > > Best regards, > Krzysztof So what is the conclusion of this? If using the "family" name there is no way for userspace to see the actual device name rather than the driver name. This might be confusing, especially of both ov9281 and ov9282 are attached to the same platform. The only difference would be the i2c-bus-address. You can also go for ov928x but this is not a real improvement. Best regards, Alexander
On 15/08/2022 14:19, Alexander Stein wrote: > Hello, > > Am Dienstag, 2. August 2022, 10:30:40 CEST schrieb Krzysztof Kozlowski: >> On 02/08/2022 10:23, Sakari Ailus wrote: >>> On Mon, Aug 01, 2022 at 08:08:58PM +0200, Krzysztof Kozlowski wrote: >>>> On 01/08/2022 20:07, Krzysztof Kozlowski wrote: >>>>> On 29/07/2022 10:18, Laurent Pinchart wrote: >>>>>> Hi Sakari, >>>>>> >>>>>> (Adding Dave and Naush to the CC list) >>>>>> >>>>>> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote: >>>>>>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote: >>>>>>>> On 28/07/2022 15:02, Alexander Stein wrote: >>>>>>>>> According to product brief they are identical from software point of >>>>>>>>> view. >>>>>>>>> Differences are a different chief ray angle (CRA) and the package. >>>>>>>>> >>>>>>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> >>>>>>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com> >>>>>>>>> --- >>>>>>>>> >>>>>>>>> drivers/media/i2c/ov9282.c | 1 + >>>>>>>>> 1 file changed, 1 insertion(+) >>>>>>>>> >>>>>>>>> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c >>>>>>>>> index 8a252bf3b59f..c8d83a29f9bb 100644 >>>>>>>>> --- a/drivers/media/i2c/ov9282.c >>>>>>>>> +++ b/drivers/media/i2c/ov9282.c >>>>>>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = >>>>>>>>> { >>>>>>>>> >>>>>>>>> }; >>>>>>>>> >>>>>>>>> static const struct of_device_id ov9282_of_match[] = { >>>>>>>>> >>>>>>>>> + { .compatible = "ovti,ov9281" }, >>>>>>>> >>>>>>>> The devices seem entirely compatible, so why you add a new compatible >>>>>>>> and not re-use existing? >>>>>>>> >>>>>>>> The difference in lens does not explain this. >>>>>>> >>>>>>> It is typically necessary to know what kind of related hardware can be >>>>>>> found in the system, beyond just the device's register interface. >>>>>>> Apart >>>>>>> from USB cameras, less integrated cameras require low-level software >>>>>>> control in which specific device properties are important. In this >>>>>>> case it >>>>>>> could be the lens shading table, among other things. >>>>>>> >>>>>>> https://www.ovt.com/sensor/ov9282/ >>>>>>> >>>>>>> Therefore I think adding a specific compatible string for this one is >>>>>>> justified. >>>>> >>>>> Specific compatible in binding is a requirement. No one discussed this. >>>>> However not in the driver. None of the arguments above justify adding >>>>> such binding, unless user-space depends on matching compatible, but not >>>>> real compatible? >>>> >>>> Eh, now I used vague words. This should be instead: >>>> >>>> "However not in the driver. None of the arguments above justify adding >>>> such compatible to driver, unless user-space depends on matching >>>> compatible, but not real compatible?" >>> >>> If I understand you right, you'd put the more specific model name as well >>> as the more generic one to the compatible property and let the driver >>> match >>> against the more generic one? >> >> Yes. >> >>> But in this case neither of these models is more generic than the other. >> >> It's not a problem. Also the spec explains it similar way: >> "They >> allow a device to express its compatibility with a family of similar >> devices, potentially allowing a single >> device driver to match against several devices." >> >> Of course the numbers would suggest that ov9281 should be the family (as >> lower number usually means designed earlier), but it is a matter of >> convention which here can be skipped. The point is that ov9281 and >> ov9282 are compatible between each other, therefore they belong to >> single family. >> >> Best regards, >> Krzysztof > > So what is the conclusion of this? > If using the "family" name there is no way for userspace to see the actual > device name rather than the driver name. This might be confusing, especially > of both ov9281 and ov9282 are attached to the same platform. The only > difference would be the i2c-bus-address. > You can also go for ov928x but this is not a real improvement. I still don't understand. Why user-space cannot see this? I really cannot find any trouble... Your 3/7 patch does nothing special here for user-space... Best regards, Krzysztof
Am Dienstag, 16. August 2022, 09:16:44 CEST schrieb Krzysztof Kozlowski: > On 15/08/2022 14:19, Alexander Stein wrote: > > Hello, > > > > Am Dienstag, 2. August 2022, 10:30:40 CEST schrieb Krzysztof Kozlowski: > >> On 02/08/2022 10:23, Sakari Ailus wrote: > >>> On Mon, Aug 01, 2022 at 08:08:58PM +0200, Krzysztof Kozlowski wrote: > >>>> On 01/08/2022 20:07, Krzysztof Kozlowski wrote: > >>>>> On 29/07/2022 10:18, Laurent Pinchart wrote: > >>>>>> Hi Sakari, > >>>>>> > >>>>>> (Adding Dave and Naush to the CC list) > >>>>>> > >>>>>> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote: > >>>>>>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote: > >>>>>>>> On 28/07/2022 15:02, Alexander Stein wrote: > >>>>>>>>> According to product brief they are identical from software point > >>>>>>>>> of > >>>>>>>>> view. > >>>>>>>>> Differences are a different chief ray angle (CRA) and the package. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > >>>>>>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com> > >>>>>>>>> --- > >>>>>>>>> > >>>>>>>>> drivers/media/i2c/ov9282.c | 1 + > >>>>>>>>> 1 file changed, 1 insertion(+) > >>>>>>>>> > >>>>>>>>> diff --git a/drivers/media/i2c/ov9282.c > >>>>>>>>> b/drivers/media/i2c/ov9282.c > >>>>>>>>> index 8a252bf3b59f..c8d83a29f9bb 100644 > >>>>>>>>> --- a/drivers/media/i2c/ov9282.c > >>>>>>>>> +++ b/drivers/media/i2c/ov9282.c > >>>>>>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops > >>>>>>>>> = > >>>>>>>>> { > >>>>>>>>> > >>>>>>>>> }; > >>>>>>>>> > >>>>>>>>> static const struct of_device_id ov9282_of_match[] = { > >>>>>>>>> > >>>>>>>>> + { .compatible = "ovti,ov9281" }, > >>>>>>>> > >>>>>>>> The devices seem entirely compatible, so why you add a new > >>>>>>>> compatible > >>>>>>>> and not re-use existing? > >>>>>>>> > >>>>>>>> The difference in lens does not explain this. > >>>>>>> > >>>>>>> It is typically necessary to know what kind of related hardware can > >>>>>>> be > >>>>>>> found in the system, beyond just the device's register interface. > >>>>>>> Apart > >>>>>>> from USB cameras, less integrated cameras require low-level software > >>>>>>> control in which specific device properties are important. In this > >>>>>>> case it > >>>>>>> could be the lens shading table, among other things. > >>>>>>> > >>>>>>> https://www.ovt.com/sensor/ov9282/ > >>>>>>> > >>>>>>> Therefore I think adding a specific compatible string for this one > >>>>>>> is > >>>>>>> justified. > >>>>> > >>>>> Specific compatible in binding is a requirement. No one discussed > >>>>> this. > >>>>> However not in the driver. None of the arguments above justify adding > >>>>> such binding, unless user-space depends on matching compatible, but > >>>>> not > >>>>> real compatible? > >>>> > >>>> Eh, now I used vague words. This should be instead: > >>>> > >>>> "However not in the driver. None of the arguments above justify adding > >>>> such compatible to driver, unless user-space depends on matching > >>>> compatible, but not real compatible?" > >>> > >>> If I understand you right, you'd put the more specific model name as > >>> well > >>> as the more generic one to the compatible property and let the driver > >>> match > >>> against the more generic one? > >> > >> Yes. > >> > >>> But in this case neither of these models is more generic than the other. > >> > >> It's not a problem. Also the spec explains it similar way: > >> "They > >> > >> allow a device to express its compatibility with a family of similar > >> > >> devices, potentially allowing a single > >> > >> device driver to match against several devices." > >> > >> Of course the numbers would suggest that ov9281 should be the family (as > >> lower number usually means designed earlier), but it is a matter of > >> convention which here can be skipped. The point is that ov9281 and > >> ov9282 are compatible between each other, therefore they belong to > >> single family. > >> > >> Best regards, > >> Krzysztof > > > > So what is the conclusion of this? > > If using the "family" name there is no way for userspace to see the actual > > device name rather than the driver name. This might be confusing, > > especially of both ov9281 and ov9282 are attached to the same platform. > > The only difference would be the i2c-bus-address. > > You can also go for ov928x but this is not a real improvement. > > I still don't understand. Why user-space cannot see this? I really > cannot find any trouble... Your 3/7 patch does nothing special here for > user-space... 3/7 itself does nothing for userspace, but 6/7 does, which relies on separate compatibles in the driver. Best regards, Alexander
On 16/08/2022 10:21, Alexander Stein wrote: >>> >>> So what is the conclusion of this? >>> If using the "family" name there is no way for userspace to see the actual >>> device name rather than the driver name. This might be confusing, >>> especially of both ov9281 and ov9282 are attached to the same platform. >>> The only difference would be the i2c-bus-address. >>> You can also go for ov928x but this is not a real improvement. >> >> I still don't understand. Why user-space cannot see this? I really >> cannot find any trouble... Your 3/7 patch does nothing special here for >> user-space... > > 3/7 itself does nothing for userspace, but 6/7 does, which relies on separate > compatibles in the driver. Ah, that's so confusing... First adding new incomplete device support in patch 3 and then in patch 6 fixing it. 6 and 3 should be squashed. They really do not make any sense being separate and this just brings this unnecessary confusion. I would argue that the binding still should have devices compatible (in one family), but now it is a bit less important. Best regards, Krzysztof
Hello Kieran, Am Samstag, 12. November 2022, 00:48:24 CET schrieb Kieran Bingham: > Hi All, > > Quoting Alexander Stein (2022-07-28 14:02:33) > > > According to product brief they are identical from software point of view. > > Differences are a different chief ray angle (CRA) and the package. > > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com> > > Throwing my hat in the ring on this thread as I see it has been hanging > around for a while and my attention was sent here from [0] > > [0] > https://lists.libcamera.org/pipermail/libcamera-devel/2022-November/035495. > html I postponed working on this change for a while, because there are (at least) two series from Dave pending which conflict a bit with this series. > > --- > > > > drivers/media/i2c/ov9282.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c > > index 8a252bf3b59f..c8d83a29f9bb 100644 > > --- a/drivers/media/i2c/ov9282.c > > +++ b/drivers/media/i2c/ov9282.c > > @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = { > > > > }; > > > > static const struct of_device_id ov9282_of_match[] = { > > > > + { .compatible = "ovti,ov9281" }, > > I believe from my existing understanding of how we would support > existing sensors even with very similar parts is that a direct > compatible lets the DT express this. > > If there were a common name that we could apply, we could have a generic > name here too, but I don't see anything specifically generic, and I > haven't yet seen a clear pattern in the namings schemes from omnivision > so ov928x wouldn't be appropriate as I couldn't be sure that an > unrelated ov9289 wouldn't exist with very different properties ... so .. > > > { .compatible = "ovti,ov9282" }, > > Either squashed with the later 6/7 that adds the name or not: (I think > it's fine either separated or squashed) > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > It does in turn bring questions into how we handle both the ov9281 and > ov9282 together in libcamera, but I don't think that's an issue to solve > here. Expressing the two separately to userspace also allows libcamera > to make a distiction between the CRA should it need to. Krzysztof is in favor of squashing so I'll respin this accordingly, removing the other changes from the ov9281 support series. Best regards, Alexander
diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c index 8a252bf3b59f..c8d83a29f9bb 100644 --- a/drivers/media/i2c/ov9282.c +++ b/drivers/media/i2c/ov9282.c @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = { }; static const struct of_device_id ov9282_of_match[] = { + { .compatible = "ovti,ov9281" }, { .compatible = "ovti,ov9282" }, { } };