On Thu, Mar 28, 2024 at 01:16:22PM -0600, Luigi311 wrote: > On 3/28/24 12:55, Rob Herring wrote: > > On Wed, Mar 27, 2024 at 05:17:04PM -0600, git@xxxxxxxxxxxx wrote: > >> From: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> > >> > >> There are a number of variants of the imx258 modules that can not > >> be differentiated at runtime, so add compatible strings for them. > >> But you are only adding 1 variant. > > I can not speak for Dave but as to why this was added here but looking > at the imx296 yaml that has something similar where there are multiple > variants that may not be detectable at run time but does not include > similar verbiage in the main description. Should I drop this from the > description so it matches the imx296? Just change "add compatible strings for them" to "add compatible string for the PDAF variant" or something. > > > > >> > >> Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> > >> Signed-off-by: Luigi311 <git@xxxxxxxxxxxx> > >> --- > >> .../devicetree/bindings/media/i2c/sony,imx258.yaml | 6 +++++- > >> 1 file changed, 5 insertions(+), 1 deletion(-) > >> > >> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml > >> index bee61a443b23..c7856de15ba3 100644 > >> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml > >> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml > >> @@ -14,10 +14,14 @@ description: |- > >> type stacked image sensor with a square pixel array of size 4208 x 3120. It > >> is programmable through I2C interface. Image data is sent through MIPI > >> CSI-2. > >> + There are a number of variants of the sensor which cannot be detected at > >> + runtime, so multiple compatible strings are required to differentiate these. > > > > That's more reasoning/why for the patch than description of the h/w. > > > >> properties: > >> compatible: > >> - const: sony,imx258 > >> + - enum: > >> + - sony,imx258 > >> + - sony,imx258-pdaf > > > > How do I know which one to use? Please define what PDAF means somewhere > > as well as perhaps what the original/default variant is or isn't. > > Would it make sense to change the properties to include a description like so > > properties: > compatible: > enum: > - sony,imx258 > - sony,imx258-pdaf > description: > The IMX258 sensor exists in two different models, a standard variant > (IMX258) and a variant with phase detection autofocus (IMX258-PDAF). > The camera module does not expose the model through registers, so the > exact model needs to be specified. Looks fine, but I'd move this to the top-level 'description'. Rob
