Re: [PATCH v10 20/24] dt: bindings: smiapp: Document lens-focus and flash properties

[Sakari Ailus]

Hi Rob,

On Tue, Sep 19, 2017 at 03:00:11PM -0500, Rob Herring wrote:
> On Mon, Sep 18, 2017 at 4:56 PM, Sakari Ailus
>  wrote:
> > Hi Rob,
> >
> >
> > Rob Herring wrote:
> >>
> >> On Mon, Sep 11, 2017 at 11:00:04AM +0300, Sakari Ailus wrote:
> >>>
> >>> Document optional lens-focus and flash properties for the smiapp driver.
> >>>
> >>> Signed-off-by: Sakari Ailus 
> >>> ---
> >>>  Documentation/devicetree/bindings/media/i2c/nokia,smia.txt | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>
> >>
> >> Acked-by: Rob Herring 
> >
> >
> > Thanks for the ack. There have been since a few iterations of the set, and
> > the corresponding patch in v13 has minor changes to this:
> 
> My review script can't deal with subject changes...
> 
> > 
> >
> > Essentially "flash" was renamed to "flash-leds" as the current flash devices
> > we have are all LEDs and the referencing assumes LED framework's ways to
> > describe LEDs. The same change is present in the patch adding the property
> 
> So we're kind of creating a binding that mirrors the gpio bindings
> (*-gpios) which is a bit of an oddball as all other bindings have gone
> with a fixed property name and then a *-names property to name them.

It could be that "flash-leds" will remain the only one. Depending on
whether anyone would ever want to support a Xenon flash in which case we
could add a property for "flash-xenon" or such. Quite possibly not; LEDs
have improved in luminosity a lot over the recent years and aren't that far
from tiny Xenon flash devices. I don't remember seeing a mobile phone less
than five years or so with a Xenon flash.

> The main downside to this form is a prefixed property name is harder
> to parse and validate. So perhaps we should follow the more common
> pattern, but we're not really describing a h/w connection just an
> association. And now we also have the trigger source binding to
> associate LEDs with device nodes, so perhaps that should be used here.
> We shouldn't really have 2 ways to associate things in DT even if how
> that gets handled in the kernel is different.

trigger-sources is not really the same thing: it's present in the LED node,
not in the sensor to start with. The LED driver has no knowledge of the
Media device --- the sensor driver gains this information through the
endpoints. The sensor driver would need to find the node which contains a
phandle back to the sensor node.

Having this property in the sensor gives the association information
between the sensor and the LED. It could be present elsewhere, e.g. the
master device with DMA engines if there's no association with a sensor.

Some sensors do support strobing the flash, too. The flash type (Xenon or
LED) needs to be known to the strobe source (sensor).

The strobe wiring may not be there: the module vendors often omit that. Or
it could be omitted on the board. That's the case with LED flashes; they
can mostly be triggered using I²C as well albeit less precisely.

-- 
Kind regards,

Sakari Ailus
sakari.ai...@linux.intel.com

Re: [PATCH v10 20/24] dt: bindings: smiapp: Document lens-focus and flash properties

[Rob Herring]

On Mon, Sep 18, 2017 at 4:56 PM, Sakari Ailus
 wrote:
> Hi Rob,
>
>
> Rob Herring wrote:
>>
>> On Mon, Sep 11, 2017 at 11:00:04AM +0300, Sakari Ailus wrote:
>>>
>>> Document optional lens-focus and flash properties for the smiapp driver.
>>>
>>> Signed-off-by: Sakari Ailus 
>>> ---
>>>  Documentation/devicetree/bindings/media/i2c/nokia,smia.txt | 2 ++
>>>  1 file changed, 2 insertions(+)
>>
>>
>> Acked-by: Rob Herring 
>
>
> Thanks for the ack. There have been since a few iterations of the set, and
> the corresponding patch in v13 has minor changes to this:

My review script can't deal with subject changes...

> 
>
> Essentially "flash" was renamed to "flash-leds" as the current flash devices
> we have are all LEDs and the referencing assumes LED framework's ways to
> describe LEDs. The same change is present in the patch adding the property

So we're kind of creating a binding that mirrors the gpio bindings
(*-gpios) which is a bit of an oddball as all other bindings have gone
with a fixed property name and then a *-names property to name them.
The main downside to this form is a prefixed property name is harder
to parse and validate. So perhaps we should follow the more common
pattern, but we're not really describing a h/w connection just an
association. And now we also have the trigger source binding to
associate LEDs with device nodes, so perhaps that should be used here.
We shouldn't really have 2 ways to associate things in DT even if how
that gets handled in the kernel is different.

Rob

Re: [PATCH v10 20/24] dt: bindings: smiapp: Document lens-focus and flash properties

[Sakari Ailus]

Hi Rob,

Rob Herring wrote:

On Mon, Sep 11, 2017 at 11:00:04AM +0300, Sakari Ailus wrote:

Document optional lens-focus and flash properties for the smiapp driver.

Signed-off-by: Sakari Ailus 
---
 Documentation/devicetree/bindings/media/i2c/nokia,smia.txt | 2 ++
 1 file changed, 2 insertions(+)


Acked-by: Rob Herring 


Thanks for the ack. There have been since a few iterations of the set, 
and the corresponding patch in v13 has minor changes to this:




Essentially "flash" was renamed to "flash-leds" as the current flash 
devices we have are all LEDs and the referencing assumes LED framework's 
ways to describe LEDs. The same change is present in the patch adding 
the property to video-interfaces.txt:




--
Regards,

Sakari Ailus
sakari.ai...@linux.intel.com

Re: [PATCH v10 20/24] dt: bindings: smiapp: Document lens-focus and flash properties

[Rob Herring]

On Mon, Sep 11, 2017 at 11:00:04AM +0300, Sakari Ailus wrote:
> Document optional lens-focus and flash properties for the smiapp driver.
> 
> Signed-off-by: Sakari Ailus 
> ---
>  Documentation/devicetree/bindings/media/i2c/nokia,smia.txt | 2 ++
>  1 file changed, 2 insertions(+)

Acked-by: Rob Herring 

Re: [PATCH v10 20/24] dt: bindings: smiapp: Document lens-focus and flash properties

[Pavel Machek]

On Mon 2017-09-11 11:00:04, Sakari Ailus wrote:
> Document optional lens-focus and flash properties for the smiapp
driver.

Acked-by: Pavel Machek 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH v10 20/24] dt: bindings: smiapp: Document lens-focus and flash properties

[Hans Verkuil]

On 09/11/2017 10:00 AM, Sakari Ailus wrote:
> Document optional lens-focus and flash properties for the smiapp driver.
> 
> Signed-off-by: Sakari Ailus 

Acked-by: Hans Verkuil 

Regards,

Hans

> ---
>  Documentation/devicetree/bindings/media/i2c/nokia,smia.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt 
> b/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt
> index 855e1faf73e2..33f10a94c381 100644
> --- a/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/nokia,smia.txt
> @@ -27,6 +27,8 @@ Optional properties
>  - nokia,nvm-size: The size of the NVM, in bytes. If the size is not given,
>the NVM contents will not be read.
>  - reset-gpios: XSHUTDOWN GPIO
> +- flash-leds: See ../video-interfaces.txt
> +- lens-focus: See ../video-interfaces.txt
>  
>  
>  Endpoint node mandatory properties
>