[PATCH 0/8] Support registering lens, flash and EEPROM devices

[PATCH 0/8] Support registering lens, flash and EEPROM devices

[Sakari Ailus]

Hi folks,

This set adds support for async registering of lens, flash and EEPROM
devices, as well as support for this in the smiapp driver and a LED driver
for the as3645a.

The lens and flash devices are entities in the media graph whereas the
EEPROM is at least currently not. By providing the association information
it is possible to add the flash device to the media graph.

The smiapp driver makes use of the newly added properties.

changes since "Document bindings for camera modules and associated flash 
        devices",
	<URL:https://www.spinics.net/lists/linux-media/msg115124.html>:

- Mention flash is a phandle reference to the flash driver chip only. Do
  not reference to LEDs themselves since this would be somewhat
  problematic for drivers to handle: the V4L2 sub-devices may have a flash
  as well as an indicator LED. Alternatively, allowing to use both LED
  driver and LED references could cause complications in async matching:
  the flash driver (software) doesn't know which one is presend in the
  sensor OF node.

  Instead, I'll propose using numeric IDs for the LEDs, just as we have
  for clocks for instance. The current definition of a flash driver device
  reference remains extensible.

  Due to the changes I've dropped the acks I've received to the flash
  binding patch.

Sakari Ailus (8):
  dt: bindings: Add a binding for flash devices associated to a sensor
  dt: bindings: Add lens-focus binding for image sensors
  dt: bindings: Add a binding for referencing EEPROM from camera sensors
  v4l2-flash: Use led_classdev instead of led_classdev_flash for
    indicator
  v4l2-flash: Flash ops aren't mandatory
  leds: as3645a: Add LED flash class driver
  smiapp: Add support for flash, lens and EEPROM devices
  arm: dts: omap3: N9/N950: Add AS3645A camera flash

 .../devicetree/bindings/media/video-interfaces.txt |  13 +
 MAINTAINERS                                        |   6 +
 arch/arm/boot/dts/omap3-n9.dts                     |   1 +
 arch/arm/boot/dts/omap3-n950-n9.dtsi               |  14 +
 arch/arm/boot/dts/omap3-n950.dts                   |   1 +
 drivers/leds/Kconfig                               |   8 +
 drivers/leds/Makefile                              |   1 +
 drivers/leds/leds-as3645a.c                        | 744 +++++++++++++++++++++
 drivers/media/i2c/smiapp/smiapp-core.c             |  81 ++-
 drivers/media/i2c/smiapp/smiapp.h                  |   5 +
 drivers/media/v4l2-core/v4l2-flash-led-class.c     |  23 +-
 include/media/v4l2-flash-led-class.h               |   6 +-
 12 files changed, 879 insertions(+), 24 deletions(-)
 create mode 100644 drivers/leds/leds-as3645a.c

-- 
2.1.4

[PATCH 1/8] dt: bindings: Add a binding for flash devices associated to a sensor

[Sakari Ailus]

Camera flash drivers (and LEDs) are separate from the sensor devices in
DT. In order to make an association between the two, provide the
association information to the software.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 Documentation/devicetree/bindings/media/video-interfaces.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index 9cd2a36..9723f7e 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -67,6 +67,14 @@ are required in a relevant parent node:
 		    identifier, should be 1.
  - #size-cells    : should be zero.
 
+
+Optional properties
+-------------------
+
+- flash: phandle referring to the flash driver chip. A flash driver may
+  have multiple flashes connected to it.
+
+
 Optional endpoint properties
 ----------------------------
 
-- 
2.1.4

Re: [PATCH 1/8] dt: bindings: Add a binding for flash devices associated to a sensor

[Rob Herring]

On Wed, Jun 14, 2017 at 12:47:12PM +0300, Sakari Ailus wrote:
> Camera flash drivers (and LEDs) are separate from the sensor devices in
> DT. In order to make an association between the two, provide the
> association information to the software.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  Documentation/devicetree/bindings/media/video-interfaces.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 1/8] dt: bindings: Add a binding for flash devices associated to a sensor

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 530 bytes --]

On Wed 2017-06-14 12:47:12, Sakari Ailus wrote:
> Camera flash drivers (and LEDs) are separate from the sensor devices in
> DT. In order to make an association between the two, provide the
> association information to the software.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Acked-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/8] dt: bindings: Add a binding for flash devices associated to a sensor

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]

Hi,

On Wed, Jun 14, 2017 at 12:47:12PM +0300, Sakari Ailus wrote:
> Camera flash drivers (and LEDs) are separate from the sensor devices in
> DT. In order to make an association between the two, provide the
> association information to the software.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

-- Sebastian

> ---
>  Documentation/devicetree/bindings/media/video-interfaces.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index 9cd2a36..9723f7e 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -67,6 +67,14 @@ are required in a relevant parent node:
>  		    identifier, should be 1.
>   - #size-cells    : should be zero.
>  
> +
> +Optional properties
> +-------------------
> +
> +- flash: phandle referring to the flash driver chip. A flash driver may
> +  have multiple flashes connected to it.
> +
> +
>  Optional endpoint properties
>  ----------------------------
>  
> -- 
> 2.1.4
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/8] dt: bindings: Add lens-focus binding for image sensors

[Sakari Ailus]

The lens-focus property contains a phandle to the lens voice coil driver
that is associated to the sensor; typically both are contained in the same
camera module.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 Documentation/devicetree/bindings/media/video-interfaces.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index 9723f7e..a18d9b2 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -74,6 +74,8 @@ Optional properties
 - flash: phandle referring to the flash driver chip. A flash driver may
   have multiple flashes connected to it.
 
+- lens-focus: A phandle to the node of the focus lens controller.
+
 
 Optional endpoint properties
 ----------------------------
-- 
2.1.4

Re: [PATCH 2/8] dt: bindings: Add lens-focus binding for image sensors

[Rob Herring]

On Wed, Jun 14, 2017 at 12:47:13PM +0300, Sakari Ailus wrote:
> The lens-focus property contains a phandle to the lens voice coil driver
> that is associated to the sensor; typically both are contained in the same
> camera module.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> ---
>  Documentation/devicetree/bindings/media/video-interfaces.txt | 2 ++
>  1 file changed, 2 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

[PATCH 3/8] dt: bindings: Add a binding for referencing EEPROM from camera sensors

[Sakari Ailus]

Many camera sensor devices contain EEPROM chips that describe the
properties of a given unit --- the data is specific to a given unit can
thus is not stored e.g. in user space or the driver.

Some sensors embed the EEPROM chip and it can be accessed through the
sensor's I2C interface. This property is to be used for devices where the
EEPROM chip is accessed through a different I2C address than the sensor.

The intent is to later provide this information to the user space.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 Documentation/devicetree/bindings/media/video-interfaces.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index a18d9b2..ae259924 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -76,6 +76,9 @@ Optional properties
 
 - lens-focus: A phandle to the node of the focus lens controller.
 
+- eeprom: A phandle to the node of the EEPROM describing the camera sensor
+  (i.e. device specific calibration data), in case it differs from the
+  sensor node.
 
 Optional endpoint properties
 ----------------------------
-- 
2.1.4

Re: [PATCH 3/8] dt: bindings: Add a binding for referencing EEPROM from camera sensors

[Rob Herring]

On Wed, Jun 14, 2017 at 12:47:14PM +0300, Sakari Ailus wrote:
> Many camera sensor devices contain EEPROM chips that describe the
> properties of a given unit --- the data is specific to a given unit can
> thus is not stored e.g. in user space or the driver.
> 
> Some sensors embed the EEPROM chip and it can be accessed through the
> sensor's I2C interface. This property is to be used for devices where the
> EEPROM chip is accessed through a different I2C address than the sensor.
> 
> The intent is to later provide this information to the user space.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> ---
>  Documentation/devicetree/bindings/media/video-interfaces.txt | 3 +++
>  1 file changed, 3 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 3/8] dt: bindings: Add a binding for referencing EEPROM from camera sensors

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1791 bytes --]

Hi Sakari,

On Wed, Jun 14, 2017 at 12:47:14PM +0300, Sakari Ailus wrote:
> Many camera sensor devices contain EEPROM chips that describe the
> properties of a given unit --- the data is specific to a given unit can
> thus is not stored e.g. in user space or the driver.
> 
> Some sensors embed the EEPROM chip and it can be accessed through the
> sensor's I2C interface. This property is to be used for devices where the
> EEPROM chip is accessed through a different I2C address than the sensor.
> 
> The intent is to later provide this information to the user space.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Acked-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> Reviewed-by: Sebastian Reichel <sebastian.reichel-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/media/video-interfaces.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index a18d9b2..ae259924 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -76,6 +76,9 @@ Optional properties
>  
>  - lens-focus: A phandle to the node of the focus lens controller.
>  
> +- eeprom: A phandle to the node of the EEPROM describing the camera sensor
> +  (i.e. device specific calibration data), in case it differs from the
> +  sensor node.

Wouldn't it makes sense (especially if you want to provide user space
access) to reuse what nvmem provides for this?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 3/8] dt: bindings: Add a binding for referencing EEPROM from camera sensors

[Sakari Ailus]

Hi Maxime,

On Wed, Jul 19, 2017 at 09:52:55AM +0200, Maxime Ripard wrote:
> Hi Sakari,
> 
> On Wed, Jun 14, 2017 at 12:47:14PM +0300, Sakari Ailus wrote:
> > Many camera sensor devices contain EEPROM chips that describe the
> > properties of a given unit --- the data is specific to a given unit can
> > thus is not stored e.g. in user space or the driver.
> > 
> > Some sensors embed the EEPROM chip and it can be accessed through the
> > sensor's I2C interface. This property is to be used for devices where the
> > EEPROM chip is accessed through a different I2C address than the sensor.
> > 
> > The intent is to later provide this information to the user space.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Acked-by: Pavel Machek <pavel@ucw.cz>
> > Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > ---
> >  Documentation/devicetree/bindings/media/video-interfaces.txt | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > index a18d9b2..ae259924 100644
> > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > @@ -76,6 +76,9 @@ Optional properties
> >  
> >  - lens-focus: A phandle to the node of the focus lens controller.
> >  
> > +- eeprom: A phandle to the node of the EEPROM describing the camera sensor
> > +  (i.e. device specific calibration data), in case it differs from the
> > +  sensor node.
> 
> Wouldn't it makes sense (especially if you want to provide user space
> access) to reuse what nvmem provides for this?

I wasn't aware of the nvmem bindings. Thanks for the pointer.

These are EEPROM chips that already have bindings documented under
Documentation/devicetree/bindings/eeprom as well as existing drivers under
drivers/misc/eeprom. Is there a reason why we have separate eeprom and
nvmem devices? Do you see issues in adding nvmem support for the existing
eeprom drivers, other than it misses using the nvmem framework?

There's also a small issue (or a big one, depending on which part of it you
consider) of the EEPROM content being parsed in the user space. The sensor
drivers do not use that information nor the contents are specific to the
sensor alone, it is ultimately up to the system integrator what to put to
the EEPROM. The typical size of an EEPROM is in perhaps one or two
kilobytes so that there's a lot of room for storing different individual
settings there.

nvmem bindings require referring to individual data cells but it's rather
the entire EEPROM contents that would be of interest here. I guess you
could create a single node under the EEPROM chip that covers the entire
chip. Or change the documentation to allow referring to the chip, rather
than a node under it.

Let me know your thoughts.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 3/8] dt: bindings: Add a binding for referencing EEPROM from camera sensors

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4154 bytes --]

Hi Sakari,

On Wed, Jul 19, 2017 at 12:21:06PM +0300, Sakari Ailus wrote:
> On Wed, Jul 19, 2017 at 09:52:55AM +0200, Maxime Ripard wrote:
> > Hi Sakari,
> > 
> > On Wed, Jun 14, 2017 at 12:47:14PM +0300, Sakari Ailus wrote:
> > > Many camera sensor devices contain EEPROM chips that describe the
> > > properties of a given unit --- the data is specific to a given unit can
> > > thus is not stored e.g. in user space or the driver.
> > > 
> > > Some sensors embed the EEPROM chip and it can be accessed through the
> > > sensor's I2C interface. This property is to be used for devices where the
> > > EEPROM chip is accessed through a different I2C address than the sensor.
> > > 
> > > The intent is to later provide this information to the user space.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Acked-by: Pavel Machek <pavel@ucw.cz>
> > > Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > > ---
> > >  Documentation/devicetree/bindings/media/video-interfaces.txt | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > index a18d9b2..ae259924 100644
> > > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > @@ -76,6 +76,9 @@ Optional properties
> > >  
> > >  - lens-focus: A phandle to the node of the focus lens controller.
> > >  
> > > +- eeprom: A phandle to the node of the EEPROM describing the camera sensor
> > > +  (i.e. device specific calibration data), in case it differs from the
> > > +  sensor node.
> > 
> > Wouldn't it makes sense (especially if you want to provide user space
> > access) to reuse what nvmem provides for this?
> 
> I wasn't aware of the nvmem bindings. Thanks for the pointer.
> 
> These are EEPROM chips that already have bindings documented under
> Documentation/devicetree/bindings/eeprom as well as existing drivers under
> drivers/misc/eeprom. Is there a reason why we have separate eeprom and
> nvmem devices? Do you see issues in adding nvmem support for the existing
> eeprom drivers, other than it misses using the nvmem framework?

As far as I know, the nvmem framework has superseeded the
drivers/misc/eeprom one, and both AT24 and AT25's bindings are still
respected by their respective drivers in nvmem.

> There's also a small issue (or a big one, depending on which part of it you
> consider) of the EEPROM content being parsed in the user space. The sensor
> drivers do not use that information nor the contents are specific to the
> sensor alone, it is ultimately up to the system integrator what to put to
> the EEPROM. The typical size of an EEPROM is in perhaps one or two
> kilobytes so that there's a lot of room for storing different individual
> settings there.
> 
> nvmem bindings require referring to individual data cells but it's rather
> the entire EEPROM contents that would be of interest here. I guess you
> could create a single node under the EEPROM chip that covers the entire
> chip. Or change the documentation to allow referring to the chip, rather
> than a node under it.

I'm not sure I really followed your thoughts here, but the fact that
the EEPROM are usually way larger than the data each and every driver
needs is indeed true. And this is exactly why we have cells, in order
to differentiate the camera calibration data, from the touchscreen
ones, and from the MAC address of the device.

I guess if you really need the whole EEPROM, yeah, a single big cell
would be the way to go I guess.

And there's currently a way to lockdown the EEPROM at the provider
level, I guess it would make sense to have the same kind of API for
the consumer too if the data are to be protected.

I'm not that involved in nvmem anymore, so the maintainer might have a
different opinion though :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 3/8] dt: bindings: Add a binding for referencing EEPROM from camera sensors

[Sakari Ailus]

Hi Maxime,

On Wed, Jul 19, 2017 at 01:18:43PM +0200, Maxime Ripard wrote:
> Hi Sakari,
> 
> On Wed, Jul 19, 2017 at 12:21:06PM +0300, Sakari Ailus wrote:
> > On Wed, Jul 19, 2017 at 09:52:55AM +0200, Maxime Ripard wrote:
> > > Hi Sakari,
> > > 
> > > On Wed, Jun 14, 2017 at 12:47:14PM +0300, Sakari Ailus wrote:
> > > > Many camera sensor devices contain EEPROM chips that describe the
> > > > properties of a given unit --- the data is specific to a given unit can
> > > > thus is not stored e.g. in user space or the driver.
> > > > 
> > > > Some sensors embed the EEPROM chip and it can be accessed through the
> > > > sensor's I2C interface. This property is to be used for devices where the
> > > > EEPROM chip is accessed through a different I2C address than the sensor.
> > > > 
> > > > The intent is to later provide this information to the user space.
> > > > 
> > > > Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > > > Acked-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> > > > Reviewed-by: Sebastian Reichel <sebastian.reichel-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/media/video-interfaces.txt | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > > index a18d9b2..ae259924 100644
> > > > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > > @@ -76,6 +76,9 @@ Optional properties
> > > >  
> > > >  - lens-focus: A phandle to the node of the focus lens controller.
> > > >  
> > > > +- eeprom: A phandle to the node of the EEPROM describing the camera sensor
> > > > +  (i.e. device specific calibration data), in case it differs from the
> > > > +  sensor node.
> > > 
> > > Wouldn't it makes sense (especially if you want to provide user space
> > > access) to reuse what nvmem provides for this?
> > 
> > I wasn't aware of the nvmem bindings. Thanks for the pointer.
> > 
> > These are EEPROM chips that already have bindings documented under
> > Documentation/devicetree/bindings/eeprom as well as existing drivers under
> > drivers/misc/eeprom. Is there a reason why we have separate eeprom and
> > nvmem devices? Do you see issues in adding nvmem support for the existing
> > eeprom drivers, other than it misses using the nvmem framework?
> 
> As far as I know, the nvmem framework has superseeded the
> drivers/misc/eeprom one, and both AT24 and AT25's bindings are still
> respected by their respective drivers in nvmem.

Right. This all makes sense now. Nvmem superseding the eeprom API isn't
very well documented I'd say. Especially if you did only looked into eeprom
drivers.

> 
> > There's also a small issue (or a big one, depending on which part of it you
> > consider) of the EEPROM content being parsed in the user space. The sensor
> > drivers do not use that information nor the contents are specific to the
> > sensor alone, it is ultimately up to the system integrator what to put to
> > the EEPROM. The typical size of an EEPROM is in perhaps one or two
> > kilobytes so that there's a lot of room for storing different individual
> > settings there.
> > 
> > nvmem bindings require referring to individual data cells but it's rather
> > the entire EEPROM contents that would be of interest here. I guess you
> > could create a single node under the EEPROM chip that covers the entire
> > chip. Or change the documentation to allow referring to the chip, rather
> > than a node under it.
> 
> I'm not sure I really followed your thoughts here, but the fact that
> the EEPROM are usually way larger than the data each and every driver
> needs is indeed true. And this is exactly why we have cells, in order
> to differentiate the camera calibration data, from the touchscreen
> ones, and from the MAC address of the device.
> 
> I guess if you really need the whole EEPROM, yeah, a single big cell
> would be the way to go I guess.

Ack.

The nvmem framework requires ACPI support then. We'll see how to get there.

> 
> And there's currently a way to lockdown the EEPROM at the provider
> level, I guess it would make sense to have the same kind of API for
> the consumer too if the data are to be protected.
> 
> I'm not that involved in nvmem anymore, so the maintainer might have a
> different opinion though :)

-- 
Regards,

Sakari Ailus
sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/8] v4l2-flash: Use led_classdev instead of led_classdev_flash for indicator

[Sakari Ailus]

The V4L2 flash class initialisation expects struct led_classdev_flash that
describes an indicator but only uses struct led_classdev which is a field
iled_cdev in the struct. Use struct iled_cdev only.

Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/media/v4l2-core/v4l2-flash-led-class.c | 19 +++++++------------
 include/media/v4l2-flash-led-class.h           |  6 +++---
 2 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
index 7b82881..6d69119 100644
--- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
+++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
@@ -110,7 +110,7 @@ static void v4l2_flash_set_led_brightness(struct v4l2_flash *v4l2_flash,
 		led_set_brightness_sync(&v4l2_flash->fled_cdev->led_cdev,
 					brightness);
 	} else {
-		led_set_brightness_sync(&v4l2_flash->iled_cdev->led_cdev,
+		led_set_brightness_sync(v4l2_flash->iled_cdev,
 					brightness);
 	}
 }
@@ -133,7 +133,7 @@ static int v4l2_flash_update_led_brightness(struct v4l2_flash *v4l2_flash,
 			return 0;
 		led_cdev = &v4l2_flash->fled_cdev->led_cdev;
 	} else {
-		led_cdev = &v4l2_flash->iled_cdev->led_cdev;
+		led_cdev = v4l2_flash->iled_cdev;
 	}
 
 	ret = led_update_brightness(led_cdev);
@@ -529,8 +529,7 @@ static int v4l2_flash_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 	struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
 	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
 	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
-	struct led_classdev_flash *iled_cdev = v4l2_flash->iled_cdev;
-	struct led_classdev *led_cdev_ind = NULL;
+	struct led_classdev *led_cdev_ind = v4l2_flash->iled_cdev;
 	int ret = 0;
 
 	if (!v4l2_fh_is_singular(&fh->vfh))
@@ -543,9 +542,7 @@ static int v4l2_flash_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 
 	mutex_unlock(&led_cdev->led_access);
 
-	if (iled_cdev) {
-		led_cdev_ind = &iled_cdev->led_cdev;
-
+	if (led_cdev_ind) {
 		mutex_lock(&led_cdev_ind->led_access);
 
 		led_sysfs_disable(led_cdev_ind);
@@ -578,7 +575,7 @@ static int v4l2_flash_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 	struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
 	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
 	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
-	struct led_classdev_flash *iled_cdev = v4l2_flash->iled_cdev;
+	struct led_classdev *led_cdev_ind = v4l2_flash->iled_cdev;
 	int ret = 0;
 
 	if (!v4l2_fh_is_singular(&fh->vfh))
@@ -593,9 +590,7 @@ static int v4l2_flash_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 
 	mutex_unlock(&led_cdev->led_access);
 
-	if (iled_cdev) {
-		struct led_classdev *led_cdev_ind = &iled_cdev->led_cdev;
-
+	if (led_cdev_ind) {
 		mutex_lock(&led_cdev_ind->led_access);
 		led_sysfs_enable(led_cdev_ind);
 		mutex_unlock(&led_cdev_ind->led_access);
@@ -614,7 +609,7 @@ static const struct v4l2_subdev_ops v4l2_flash_subdev_ops;
 struct v4l2_flash *v4l2_flash_init(
 	struct device *dev, struct fwnode_handle *fwn,
 	struct led_classdev_flash *fled_cdev,
-	struct led_classdev_flash *iled_cdev,
+	struct led_classdev *iled_cdev,
 	const struct v4l2_flash_ops *ops,
 	struct v4l2_flash_config *config)
 {
diff --git a/include/media/v4l2-flash-led-class.h b/include/media/v4l2-flash-led-class.h
index f9dcd54..54e31a8 100644
--- a/include/media/v4l2-flash-led-class.h
+++ b/include/media/v4l2-flash-led-class.h
@@ -85,7 +85,7 @@ struct v4l2_flash_config {
  */
 struct v4l2_flash {
 	struct led_classdev_flash *fled_cdev;
-	struct led_classdev_flash *iled_cdev;
+	struct led_classdev *iled_cdev;
 	const struct v4l2_flash_ops *ops;
 
 	struct v4l2_subdev sd;
@@ -124,7 +124,7 @@ static inline struct v4l2_flash *v4l2_ctrl_to_v4l2_flash(struct v4l2_ctrl *c)
 struct v4l2_flash *v4l2_flash_init(
 	struct device *dev, struct fwnode_handle *fwn,
 	struct led_classdev_flash *fled_cdev,
-	struct led_classdev_flash *iled_cdev,
+	struct led_classdev *iled_cdev,
 	const struct v4l2_flash_ops *ops,
 	struct v4l2_flash_config *config);
 
@@ -140,7 +140,7 @@ void v4l2_flash_release(struct v4l2_flash *v4l2_flash);
 static inline struct v4l2_flash *v4l2_flash_init(
 	struct device *dev, struct fwnode_handle *fwn,
 	struct led_classdev_flash *fled_cdev,
-	struct led_classdev_flash *iled_cdev,
+	struct led_classdev *iled_cdev,
 	const struct v4l2_flash_ops *ops,
 	struct v4l2_flash_config *config)
 {
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/8] v4l2-flash: Use led_classdev instead of led_classdev_flash for indicator

[Jacek Anaszewski]

Hi Sakari,

On 06/14/2017 11:47 AM, Sakari Ailus wrote:
> The V4L2 flash class initialisation expects struct led_classdev_flash that
> describes an indicator but only uses struct led_classdev which is a field
> iled_cdev in the struct. Use struct iled_cdev only.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-flash-led-class.c | 19 +++++++------------
>  include/media/v4l2-flash-led-class.h           |  6 +++---
>  2 files changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> index 7b82881..6d69119 100644
> --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> @@ -110,7 +110,7 @@ static void v4l2_flash_set_led_brightness(struct v4l2_flash *v4l2_flash,
>  		led_set_brightness_sync(&v4l2_flash->fled_cdev->led_cdev,
>  					brightness);
>  	} else {
> -		led_set_brightness_sync(&v4l2_flash->iled_cdev->led_cdev,
> +		led_set_brightness_sync(v4l2_flash->iled_cdev,
>  					brightness);
>  	}
>  }
> @@ -133,7 +133,7 @@ static int v4l2_flash_update_led_brightness(struct v4l2_flash *v4l2_flash,
>  			return 0;
>  		led_cdev = &v4l2_flash->fled_cdev->led_cdev;
>  	} else {
> -		led_cdev = &v4l2_flash->iled_cdev->led_cdev;
> +		led_cdev = v4l2_flash->iled_cdev;
>  	}
>  
>  	ret = led_update_brightness(led_cdev);
> @@ -529,8 +529,7 @@ static int v4l2_flash_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  	struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
>  	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
>  	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> -	struct led_classdev_flash *iled_cdev = v4l2_flash->iled_cdev;
> -	struct led_classdev *led_cdev_ind = NULL;
> +	struct led_classdev *led_cdev_ind = v4l2_flash->iled_cdev;
>  	int ret = 0;
>  
>  	if (!v4l2_fh_is_singular(&fh->vfh))
> @@ -543,9 +542,7 @@ static int v4l2_flash_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  
>  	mutex_unlock(&led_cdev->led_access);
>  
> -	if (iled_cdev) {
> -		led_cdev_ind = &iled_cdev->led_cdev;
> -
> +	if (led_cdev_ind) {
>  		mutex_lock(&led_cdev_ind->led_access);
>  
>  		led_sysfs_disable(led_cdev_ind);
> @@ -578,7 +575,7 @@ static int v4l2_flash_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  	struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
>  	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
>  	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> -	struct led_classdev_flash *iled_cdev = v4l2_flash->iled_cdev;
> +	struct led_classdev *led_cdev_ind = v4l2_flash->iled_cdev;
>  	int ret = 0;
>  
>  	if (!v4l2_fh_is_singular(&fh->vfh))
> @@ -593,9 +590,7 @@ static int v4l2_flash_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  
>  	mutex_unlock(&led_cdev->led_access);
>  
> -	if (iled_cdev) {
> -		struct led_classdev *led_cdev_ind = &iled_cdev->led_cdev;
> -
> +	if (led_cdev_ind) {
>  		mutex_lock(&led_cdev_ind->led_access);
>  		led_sysfs_enable(led_cdev_ind);
>  		mutex_unlock(&led_cdev_ind->led_access);
> @@ -614,7 +609,7 @@ static const struct v4l2_subdev_ops v4l2_flash_subdev_ops;
>  struct v4l2_flash *v4l2_flash_init(
>  	struct device *dev, struct fwnode_handle *fwn,
>  	struct led_classdev_flash *fled_cdev,
> -	struct led_classdev_flash *iled_cdev,
> +	struct led_classdev *iled_cdev,
>  	const struct v4l2_flash_ops *ops,
>  	struct v4l2_flash_config *config)
>  {
> diff --git a/include/media/v4l2-flash-led-class.h b/include/media/v4l2-flash-led-class.h
> index f9dcd54..54e31a8 100644
> --- a/include/media/v4l2-flash-led-class.h
> +++ b/include/media/v4l2-flash-led-class.h
> @@ -85,7 +85,7 @@ struct v4l2_flash_config {
>   */
>  struct v4l2_flash {
>  	struct led_classdev_flash *fled_cdev;
> -	struct led_classdev_flash *iled_cdev;
> +	struct led_classdev *iled_cdev;
>  	const struct v4l2_flash_ops *ops;
>  
>  	struct v4l2_subdev sd;
> @@ -124,7 +124,7 @@ static inline struct v4l2_flash *v4l2_ctrl_to_v4l2_flash(struct v4l2_ctrl *c)
>  struct v4l2_flash *v4l2_flash_init(
>  	struct device *dev, struct fwnode_handle *fwn,
>  	struct led_classdev_flash *fled_cdev,
> -	struct led_classdev_flash *iled_cdev,
> +	struct led_classdev *iled_cdev,
>  	const struct v4l2_flash_ops *ops,
>  	struct v4l2_flash_config *config);
>  
> @@ -140,7 +140,7 @@ void v4l2_flash_release(struct v4l2_flash *v4l2_flash);
>  static inline struct v4l2_flash *v4l2_flash_init(
>  	struct device *dev, struct fwnode_handle *fwn,
>  	struct led_classdev_flash *fled_cdev,
> -	struct led_classdev_flash *iled_cdev,
> +	struct led_classdev *iled_cdev,
>  	const struct v4l2_flash_ops *ops,
>  	struct v4l2_flash_config *config)
>  {
> 

Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 4/8] v4l2-flash: Use led_classdev instead of led_classdev_flash for indicator

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3459 bytes --]

Hi Sakari,

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.12-rc5 next-20170614]
[cannot apply to robh/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sakari-Ailus/Support-registering-lens-flash-and-EEPROM-devices/20170615-084016
base:   git://linuxtv.org/media_tree.git master
config: tile-allmodconfig (attached as .config)
compiler: tilegx-linux-gcc (GCC) 4.6.2
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=tile 

All warnings (new ones prefixed by >>):

   drivers/staging//greybus/light.c: In function 'gb_lights_light_v4l2_register':
>> drivers/staging//greybus/light.c:574:10: warning: passing argument 4 of 'v4l2_flash_init' from incompatible pointer type [enabled by default]
   include/media/v4l2-flash-led-class.h:124:20: note: expected 'struct led_classdev *' but argument is of type 'struct led_classdev_flash *'

vim +/v4l2_flash_init +574 drivers/staging//greybus/light.c

2870b52b Rui Miguel Silva 2015-08-14  558  
2870b52b Rui Miguel Silva 2015-08-14  559  	channel_flash = get_channel_from_mode(light, GB_CHANNEL_MODE_FLASH);
2870b52b Rui Miguel Silva 2015-08-14  560  	WARN_ON(!channel_flash);
2870b52b Rui Miguel Silva 2015-08-14  561  
2870b52b Rui Miguel Silva 2015-08-14  562  	fled = &channel_flash->fled;
2870b52b Rui Miguel Silva 2015-08-14  563  
2870b52b Rui Miguel Silva 2015-08-14  564  	snprintf(sd_cfg->dev_name, sizeof(sd_cfg->dev_name), "%s", light->name);
2870b52b Rui Miguel Silva 2015-08-14  565  
2870b52b Rui Miguel Silva 2015-08-14  566  	/* Set the possible values to faults, in our case all faults */
2870b52b Rui Miguel Silva 2015-08-14  567  	sd_cfg->flash_faults = LED_FAULT_OVER_VOLTAGE | LED_FAULT_TIMEOUT |
2870b52b Rui Miguel Silva 2015-08-14  568  		LED_FAULT_OVER_TEMPERATURE | LED_FAULT_SHORT_CIRCUIT |
2870b52b Rui Miguel Silva 2015-08-14  569  		LED_FAULT_OVER_CURRENT | LED_FAULT_INDICATOR |
2870b52b Rui Miguel Silva 2015-08-14  570  		LED_FAULT_UNDER_VOLTAGE | LED_FAULT_INPUT_VOLTAGE |
2870b52b Rui Miguel Silva 2015-08-14  571  		LED_FAULT_LED_OVER_TEMPERATURE;
2870b52b Rui Miguel Silva 2015-08-14  572  
2870b52b Rui Miguel Silva 2015-08-14  573  	light->v4l2_flash = v4l2_flash_init(dev, NULL, fled, iled,
3f85c787 Rui Miguel Silva 2015-12-03 @574  					    &v4l2_flash_ops, sd_cfg);
2870b52b Rui Miguel Silva 2015-08-14  575  	if (IS_ERR_OR_NULL(light->v4l2_flash)) {
2870b52b Rui Miguel Silva 2015-08-14  576  		ret = PTR_ERR(light->v4l2_flash);
2870b52b Rui Miguel Silva 2015-08-14  577  		goto out_free;
2870b52b Rui Miguel Silva 2015-08-14  578  	}
2870b52b Rui Miguel Silva 2015-08-14  579  
2870b52b Rui Miguel Silva 2015-08-14  580  	return ret;
2870b52b Rui Miguel Silva 2015-08-14  581  
2870b52b Rui Miguel Silva 2015-08-14  582  out_free:

:::::: The code at line 574 was first introduced by commit
:::::: 3f85c787b74c26f3816017e64288af907f291462 greybus: lights: add v4l2 flash operations

:::::: TO: Rui Miguel Silva <rui.silva@linaro.org>
:::::: CC: Greg Kroah-Hartman <gregkh@google.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 49758 bytes --]

Re: [PATCH 4/8] v4l2-flash: Use led_classdev instead of led_classdev_flash for indicator

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 5021 bytes --]

Hi,

On Wed, Jun 14, 2017 at 12:47:15PM +0300, Sakari Ailus wrote:
> The V4L2 flash class initialisation expects struct led_classdev_flash that
> describes an indicator but only uses struct led_classdev which is a field
> iled_cdev in the struct. Use struct iled_cdev only.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

-- Sebastian

> ---
>  drivers/media/v4l2-core/v4l2-flash-led-class.c | 19 +++++++------------
>  include/media/v4l2-flash-led-class.h           |  6 +++---
>  2 files changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> index 7b82881..6d69119 100644
> --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> @@ -110,7 +110,7 @@ static void v4l2_flash_set_led_brightness(struct v4l2_flash *v4l2_flash,
>  		led_set_brightness_sync(&v4l2_flash->fled_cdev->led_cdev,
>  					brightness);
>  	} else {
> -		led_set_brightness_sync(&v4l2_flash->iled_cdev->led_cdev,
> +		led_set_brightness_sync(v4l2_flash->iled_cdev,
>  					brightness);
>  	}
>  }
> @@ -133,7 +133,7 @@ static int v4l2_flash_update_led_brightness(struct v4l2_flash *v4l2_flash,
>  			return 0;
>  		led_cdev = &v4l2_flash->fled_cdev->led_cdev;
>  	} else {
> -		led_cdev = &v4l2_flash->iled_cdev->led_cdev;
> +		led_cdev = v4l2_flash->iled_cdev;
>  	}
>  
>  	ret = led_update_brightness(led_cdev);
> @@ -529,8 +529,7 @@ static int v4l2_flash_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  	struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
>  	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
>  	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> -	struct led_classdev_flash *iled_cdev = v4l2_flash->iled_cdev;
> -	struct led_classdev *led_cdev_ind = NULL;
> +	struct led_classdev *led_cdev_ind = v4l2_flash->iled_cdev;
>  	int ret = 0;
>  
>  	if (!v4l2_fh_is_singular(&fh->vfh))
> @@ -543,9 +542,7 @@ static int v4l2_flash_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  
>  	mutex_unlock(&led_cdev->led_access);
>  
> -	if (iled_cdev) {
> -		led_cdev_ind = &iled_cdev->led_cdev;
> -
> +	if (led_cdev_ind) {
>  		mutex_lock(&led_cdev_ind->led_access);
>  
>  		led_sysfs_disable(led_cdev_ind);
> @@ -578,7 +575,7 @@ static int v4l2_flash_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  	struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
>  	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
>  	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> -	struct led_classdev_flash *iled_cdev = v4l2_flash->iled_cdev;
> +	struct led_classdev *led_cdev_ind = v4l2_flash->iled_cdev;
>  	int ret = 0;
>  
>  	if (!v4l2_fh_is_singular(&fh->vfh))
> @@ -593,9 +590,7 @@ static int v4l2_flash_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  
>  	mutex_unlock(&led_cdev->led_access);
>  
> -	if (iled_cdev) {
> -		struct led_classdev *led_cdev_ind = &iled_cdev->led_cdev;
> -
> +	if (led_cdev_ind) {
>  		mutex_lock(&led_cdev_ind->led_access);
>  		led_sysfs_enable(led_cdev_ind);
>  		mutex_unlock(&led_cdev_ind->led_access);
> @@ -614,7 +609,7 @@ static const struct v4l2_subdev_ops v4l2_flash_subdev_ops;
>  struct v4l2_flash *v4l2_flash_init(
>  	struct device *dev, struct fwnode_handle *fwn,
>  	struct led_classdev_flash *fled_cdev,
> -	struct led_classdev_flash *iled_cdev,
> +	struct led_classdev *iled_cdev,
>  	const struct v4l2_flash_ops *ops,
>  	struct v4l2_flash_config *config)
>  {
> diff --git a/include/media/v4l2-flash-led-class.h b/include/media/v4l2-flash-led-class.h
> index f9dcd54..54e31a8 100644
> --- a/include/media/v4l2-flash-led-class.h
> +++ b/include/media/v4l2-flash-led-class.h
> @@ -85,7 +85,7 @@ struct v4l2_flash_config {
>   */
>  struct v4l2_flash {
>  	struct led_classdev_flash *fled_cdev;
> -	struct led_classdev_flash *iled_cdev;
> +	struct led_classdev *iled_cdev;
>  	const struct v4l2_flash_ops *ops;
>  
>  	struct v4l2_subdev sd;
> @@ -124,7 +124,7 @@ static inline struct v4l2_flash *v4l2_ctrl_to_v4l2_flash(struct v4l2_ctrl *c)
>  struct v4l2_flash *v4l2_flash_init(
>  	struct device *dev, struct fwnode_handle *fwn,
>  	struct led_classdev_flash *fled_cdev,
> -	struct led_classdev_flash *iled_cdev,
> +	struct led_classdev *iled_cdev,
>  	const struct v4l2_flash_ops *ops,
>  	struct v4l2_flash_config *config);
>  
> @@ -140,7 +140,7 @@ void v4l2_flash_release(struct v4l2_flash *v4l2_flash);
>  static inline struct v4l2_flash *v4l2_flash_init(
>  	struct device *dev, struct fwnode_handle *fwn,
>  	struct led_classdev_flash *fled_cdev,
> -	struct led_classdev_flash *iled_cdev,
> +	struct led_classdev *iled_cdev,
>  	const struct v4l2_flash_ops *ops,
>  	struct v4l2_flash_config *config)
>  {
> -- 
> 2.1.4
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 5/8] v4l2-flash: Flash ops aren't mandatory

[Sakari Ailus]

None of the flash operations are not mandatory and therefore there should
be no need for the flash ops structure either. Accept NULL.

Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/media/v4l2-core/v4l2-flash-led-class.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
index 6d69119..fdb79da 100644
--- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
+++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
@@ -18,7 +18,7 @@
 #include <media/v4l2-flash-led-class.h>
 
 #define has_flash_op(v4l2_flash, op)				\
-	(v4l2_flash && v4l2_flash->ops->op)
+	(v4l2_flash && v4l2_flash->ops && v4l2_flash->ops->op)
 
 #define call_flash_op(v4l2_flash, op, arg)			\
 		(has_flash_op(v4l2_flash, op) ?			\
@@ -618,7 +618,7 @@ struct v4l2_flash *v4l2_flash_init(
 	struct v4l2_subdev *sd;
 	int ret;
 
-	if (!fled_cdev || !ops || !config)
+	if (!fled_cdev || !config)
 		return ERR_PTR(-EINVAL);
 
 	led_cdev = &fled_cdev->led_cdev;
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/8] v4l2-flash: Flash ops aren't mandatory

[Jacek Anaszewski]

Hi Sakari,

On 06/14/2017 11:47 AM, Sakari Ailus wrote:
> None of the flash operations are not mandatory and therefore there should
> be no need for the flash ops structure either. Accept NULL.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-flash-led-class.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> index 6d69119..fdb79da 100644
> --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> @@ -18,7 +18,7 @@
>  #include <media/v4l2-flash-led-class.h>
>  
>  #define has_flash_op(v4l2_flash, op)				\
> -	(v4l2_flash && v4l2_flash->ops->op)
> +	(v4l2_flash && v4l2_flash->ops && v4l2_flash->ops->op)

This change doesn't seem to be related to the patch subject.

>  #define call_flash_op(v4l2_flash, op, arg)			\
>  		(has_flash_op(v4l2_flash, op) ?			\
> @@ -618,7 +618,7 @@ struct v4l2_flash *v4l2_flash_init(
>  	struct v4l2_subdev *sd;
>  	int ret;
>  
> -	if (!fled_cdev || !ops || !config)
> +	if (!fled_cdev || !config)
>  		return ERR_PTR(-EINVAL);
>  
>  	led_cdev = &fled_cdev->led_cdev;
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 5/8] v4l2-flash: Flash ops aren't mandatory

[Sakari Ailus]

Hi Jacek,

On Wed, Jun 14, 2017 at 11:14:13PM +0200, Jacek Anaszewski wrote:
> Hi Sakari,
> 
> On 06/14/2017 11:47 AM, Sakari Ailus wrote:
> > None of the flash operations are not mandatory and therefore there should
> > be no need for the flash ops structure either. Accept NULL.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-flash-led-class.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > index 6d69119..fdb79da 100644
> > --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > @@ -18,7 +18,7 @@
> >  #include <media/v4l2-flash-led-class.h>
> >  
> >  #define has_flash_op(v4l2_flash, op)				\
> > -	(v4l2_flash && v4l2_flash->ops->op)
> > +	(v4l2_flash && v4l2_flash->ops && v4l2_flash->ops->op)
> 
> This change doesn't seem to be related to the patch subject.

Yes, it is: if there's a chance that ops is NULL, then you have to test here
you actually have the ops struct around. The test is no longer in
v4l2_flash_init().

> 
> >  #define call_flash_op(v4l2_flash, op, arg)			\
> >  		(has_flash_op(v4l2_flash, op) ?			\
> > @@ -618,7 +618,7 @@ struct v4l2_flash *v4l2_flash_init(
> >  	struct v4l2_subdev *sd;
> >  	int ret;
> >  
> > -	if (!fled_cdev || !ops || !config)
> > +	if (!fled_cdev || !config)
> >  		return ERR_PTR(-EINVAL);
> >  
> >  	led_cdev = &fled_cdev->led_cdev;
> > 

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org	XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/8] v4l2-flash: Flash ops aren't mandatory

[Jacek Anaszewski]

Hi Sakari,

On 06/14/2017 11:19 PM, Sakari Ailus wrote:
> Hi Jacek,
> 
> On Wed, Jun 14, 2017 at 11:14:13PM +0200, Jacek Anaszewski wrote:
>> Hi Sakari,
>>
>> On 06/14/2017 11:47 AM, Sakari Ailus wrote:
>>> None of the flash operations are not mandatory and therefore there should
>>> be no need for the flash ops structure either. Accept NULL.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>> ---
>>>  drivers/media/v4l2-core/v4l2-flash-led-class.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
>>> index 6d69119..fdb79da 100644
>>> --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
>>> +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
>>> @@ -18,7 +18,7 @@
>>>  #include <media/v4l2-flash-led-class.h>
>>>  
>>>  #define has_flash_op(v4l2_flash, op)				\
>>> -	(v4l2_flash && v4l2_flash->ops->op)
>>> +	(v4l2_flash && v4l2_flash->ops && v4l2_flash->ops->op)
>>
>> This change doesn't seem to be related to the patch subject.
> 
> Yes, it is: if there's a chance that ops is NULL, then you have to test here
> you actually have the ops struct around. The test is no longer in
> v4l2_flash_init().

Indeed.

Reviewed-by: Jacek Anaszewski <jacek.anaszewski-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

>>>  #define call_flash_op(v4l2_flash, op, arg)			\
>>>  		(has_flash_op(v4l2_flash, op) ?			\
>>> @@ -618,7 +618,7 @@ struct v4l2_flash *v4l2_flash_init(
>>>  	struct v4l2_subdev *sd;
>>>  	int ret;
>>>  
>>> -	if (!fled_cdev || !ops || !config)
>>> +	if (!fled_cdev || !config)
>>>  		return ERR_PTR(-EINVAL);
>>>  
>>>  	led_cdev = &fled_cdev->led_cdev;
>>>
> 

-- 
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/8] v4l2-flash: Flash ops aren't mandatory

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1388 bytes --]

Hi,

On Wed, Jun 14, 2017 at 12:47:16PM +0300, Sakari Ailus wrote:
> None of the flash operations are not mandatory and therefore there should
> be no need for the flash ops structure either. Accept NULL.

I think you negated one time too much :). Otherwise:

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

-- Sebastian

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-flash-led-class.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> index 6d69119..fdb79da 100644
> --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> @@ -18,7 +18,7 @@
>  #include <media/v4l2-flash-led-class.h>
>  
>  #define has_flash_op(v4l2_flash, op)				\
> -	(v4l2_flash && v4l2_flash->ops->op)
> +	(v4l2_flash && v4l2_flash->ops && v4l2_flash->ops->op)
>  
>  #define call_flash_op(v4l2_flash, op, arg)			\
>  		(has_flash_op(v4l2_flash, op) ?			\
> @@ -618,7 +618,7 @@ struct v4l2_flash *v4l2_flash_init(
>  	struct v4l2_subdev *sd;
>  	int ret;
>  
> -	if (!fled_cdev || !ops || !config)
> +	if (!fled_cdev || !config)
>  		return ERR_PTR(-EINVAL);
>  
>  	led_cdev = &fled_cdev->led_cdev;
> -- 
> 2.1.4
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/8] v4l2-flash: Flash ops aren't mandatory

[Sakari Ailus]

On Thu, Jun 15, 2017 at 11:24:26AM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Jun 14, 2017 at 12:47:16PM +0300, Sakari Ailus wrote:
> > None of the flash operations are not mandatory and therefore there should
> > be no need for the flash ops structure either. Accept NULL.
> 
> I think you negated one time too much :). Otherwise:
> 
> Reviewed-by: Sebastian Reichel <sebastian.reichel-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>

Thanks!

The new one reads:

None of the flash operations are mandatory and therefore there should be no
need for the flash ops structure either. Accept NULL.

-- 
Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org	XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/8] v4l2-flash: Flash ops aren't mandatory

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

Hi,

On Thu, Jun 15, 2017 at 03:32:10PM +0300, Sakari Ailus wrote:
> On Thu, Jun 15, 2017 at 11:24:26AM +0200, Sebastian Reichel wrote:
> > On Wed, Jun 14, 2017 at 12:47:16PM +0300, Sakari Ailus wrote:
> > > None of the flash operations are not mandatory and therefore there should
> > > be no need for the flash ops structure either. Accept NULL.
> > 
> > I think you negated one time too much :). Otherwise:
> > 
> > Reviewed-by: Sebastian Reichel <sebastian.reichel-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
> 
> Thanks!
> 
> The new one reads:
> 
> None of the flash operations are mandatory and therefore there should be no
> need for the flash ops structure either. Accept NULL.

Fine with me.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v1.1 1/1] v4l2-flash: Flash ops aren't mandatory

[Sakari Ailus]

None of the flash operations are mandatory and therefore there should be
no need for the flash ops structure either. Accept NULL.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
---
Hi folks,

I'm planning to get this one plus "v4l2-flash: Use led_classdev instead of
led_classdev_flash for indicator" to linux-media in the near future. The
rest still needs work.

since v1:

- Use has_flash_op() in __fill_ctrl_init_data() to check an op exists.

 drivers/media/v4l2-core/v4l2-flash-led-class.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
index 6d69119ff097..aabc85dbb8b5 100644
--- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
+++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
@@ -18,7 +18,7 @@
 #include <media/v4l2-flash-led-class.h>
 
 #define has_flash_op(v4l2_flash, op)				\
-	(v4l2_flash && v4l2_flash->ops->op)
+	(v4l2_flash && v4l2_flash->ops && v4l2_flash->ops->op)
 
 #define call_flash_op(v4l2_flash, op, arg)			\
 		(has_flash_op(v4l2_flash, op) ?			\
@@ -299,7 +299,6 @@ static void __fill_ctrl_init_data(struct v4l2_flash *v4l2_flash,
 			  struct v4l2_flash_ctrl_data *ctrl_init_data)
 {
 	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
-	const struct led_flash_ops *fled_cdev_ops = fled_cdev->ops;
 	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
 	struct v4l2_ctrl_config *ctrl_cfg;
 	u32 mask;
@@ -376,7 +375,7 @@ static void __fill_ctrl_init_data(struct v4l2_flash *v4l2_flash,
 	}
 
 	/* Init STROBE_STATUS ctrl data */
-	if (fled_cdev_ops->strobe_get) {
+	if (has_flash_op(fled_cdev, strobe_get)) {
 		ctrl_init_data[STROBE_STATUS].cid =
 					V4L2_CID_FLASH_STROBE_STATUS;
 		ctrl_cfg = &ctrl_init_data[STROBE_STATUS].config;
@@ -386,7 +385,7 @@ static void __fill_ctrl_init_data(struct v4l2_flash *v4l2_flash,
 	}
 
 	/* Init FLASH_TIMEOUT ctrl data */
-	if (fled_cdev_ops->timeout_set) {
+	if (has_flash_op(fled_cdev, timeout_set)) {
 		ctrl_init_data[FLASH_TIMEOUT].cid = V4L2_CID_FLASH_TIMEOUT;
 		ctrl_cfg = &ctrl_init_data[FLASH_TIMEOUT].config;
 		__lfs_to_v4l2_ctrl_config(&fled_cdev->timeout, ctrl_cfg);
@@ -394,7 +393,7 @@ static void __fill_ctrl_init_data(struct v4l2_flash *v4l2_flash,
 	}
 
 	/* Init FLASH_INTENSITY ctrl data */
-	if (fled_cdev_ops->flash_brightness_set) {
+	if (has_flash_op(fled_cdev, flash_brightness_set)) {
 		ctrl_init_data[FLASH_INTENSITY].cid = V4L2_CID_FLASH_INTENSITY;
 		ctrl_cfg = &ctrl_init_data[FLASH_INTENSITY].config;
 		__lfs_to_v4l2_ctrl_config(&fled_cdev->brightness, ctrl_cfg);
@@ -618,7 +617,7 @@ struct v4l2_flash *v4l2_flash_init(
 	struct v4l2_subdev *sd;
 	int ret;
 
-	if (!fled_cdev || !ops || !config)
+	if (!fled_cdev || !config)
 		return ERR_PTR(-EINVAL);
 
 	led_cdev = &fled_cdev->led_cdev;
-- 
2.11.0

Re: [PATCH v1.1 1/1] v4l2-flash: Flash ops aren't mandatory

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 431 bytes --]

On Tue 2017-07-18 20:36:23, Sakari Ailus wrote:
> None of the flash operations are mandatory and therefore there should be
> no need for the flash ops structure either. Accept NULL.

Well, ok, but is not the flash without any operations kind of useless?

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH 6/8] leds: as3645a: Add LED flash class driver

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus-X3B1VOXEql0@public.gmane.org>

Add a LED flash class driver for the as3654a flash controller. A V4L2 flash
driver for it already exists (drivers/media/i2c/as3645a.c), and this driver
is based on that.

Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 MAINTAINERS                 |   6 +
 drivers/leds/Kconfig        |   8 +
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-as3645a.c | 744 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 759 insertions(+)
 create mode 100644 drivers/leds/leds-as3645a.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 053c3bd..c7682af 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2067,6 +2067,12 @@ F:	arch/arm64/
 F:	Documentation/arm64/
 
 AS3645A LED FLASH CONTROLLER DRIVER
+M:	Sakari Ailus <sakari.ailus-X3B1VOXEql0@public.gmane.org>
+L:	linux-leds-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+S:	Maintained
+F:	drivers/leds/leds-as3645a.c
+
+AS3645A LED FLASH CONTROLLER DRIVER
 M:	Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
 L:	linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
 T:	git git://linuxtv.org/media_tree.git
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 6c29998..9fb1d86 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -58,6 +58,14 @@ config LEDS_AAT1290
 	help
 	 This option enables support for the LEDs on the AAT1290.
 
+config LEDS_AS3645A
+	tristate "AS3645A LED flash controller support"
+	depends on I2C && LEDS_CLASS_FLASH
+	help
+	  Enable LED flash class support for AS3645A LED flash
+	  controller. V4L2 flash API is provided as well if
+	  CONFIG_V4L2_FLASH_API is enabled.
+
 config LEDS_BCM6328
 	tristate "LED Support for Broadcom BCM6328"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 45f1339..b4def76 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
 # LED Platform Drivers
 obj-$(CONFIG_LEDS_88PM860X)		+= leds-88pm860x.o
 obj-$(CONFIG_LEDS_AAT1290)		+= leds-aat1290.o
+obj-$(CONFIG_LEDS_AS3645A)		+= leds-as3645a.o
 obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
 obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
 obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
diff --git a/drivers/leds/leds-as3645a.c b/drivers/leds/leds-as3645a.c
new file mode 100644
index 0000000..862d1b5
--- /dev/null
+++ b/drivers/leds/leds-as3645a.c
@@ -0,0 +1,744 @@
+/*
+ * drivers/leds/leds-as3645a.c - AS3645A and LM3555 flash controllers driver
+ *
+ * Copyright (C) 2008-2011 Nokia Corporation
+ * Copyright (c) 2011, 2017 Intel Corporation.
+ *
+ * Based on drivers/media/i2c/as3645a.c.
+ *
+ * Contact: Sakari Ailus <sakari.ailus-X3B1VOXEql0@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/led-class-flash.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+
+#include <media/v4l2-flash-led-class.h>
+
+#define AS_TIMER_US_TO_CODE(t)			(((t) / 1000 - 100) / 50)
+#define AS_TIMER_CODE_TO_US(c)			((50 * (c) + 100) * 1000)
+
+/* Register definitions */
+
+/* Read-only Design info register: Reset state: xxxx 0001 */
+#define AS_DESIGN_INFO_REG			0x00
+#define AS_DESIGN_INFO_FACTORY(x)		(((x) >> 4))
+#define AS_DESIGN_INFO_MODEL(x)			((x) & 0x0f)
+
+/* Read-only Version control register: Reset state: 0000 0000
+ * for first engineering samples
+ */
+#define AS_VERSION_CONTROL_REG			0x01
+#define AS_VERSION_CONTROL_RFU(x)		(((x) >> 4))
+#define AS_VERSION_CONTROL_VERSION(x)		((x) & 0x0f)
+
+/* Read / Write	(Indicator and timer register): Reset state: 0000 1111 */
+#define AS_INDICATOR_AND_TIMER_REG		0x02
+#define AS_INDICATOR_AND_TIMER_TIMEOUT_SHIFT	0
+#define AS_INDICATOR_AND_TIMER_VREF_SHIFT	4
+#define AS_INDICATOR_AND_TIMER_INDICATOR_SHIFT	6
+
+/* Read / Write	(Current set register): Reset state: 0110 1001 */
+#define AS_CURRENT_SET_REG			0x03
+#define AS_CURRENT_ASSIST_LIGHT_SHIFT		0
+#define AS_CURRENT_LED_DET_ON			(1 << 3)
+#define AS_CURRENT_FLASH_CURRENT_SHIFT		4
+
+/* Read / Write	(Control register): Reset state: 1011 0100 */
+#define AS_CONTROL_REG				0x04
+#define AS_CONTROL_MODE_SETTING_SHIFT		0
+#define AS_CONTROL_STROBE_ON			(1 << 2)
+#define AS_CONTROL_OUT_ON			(1 << 3)
+#define AS_CONTROL_EXT_TORCH_ON			(1 << 4)
+#define AS_CONTROL_STROBE_TYPE_EDGE		(0 << 5)
+#define AS_CONTROL_STROBE_TYPE_LEVEL		(1 << 5)
+#define AS_CONTROL_COIL_PEAK_SHIFT		6
+
+/* Read only (D3 is read / write) (Fault and info): Reset state: 0000 x000 */
+#define AS_FAULT_INFO_REG			0x05
+#define AS_FAULT_INFO_INDUCTOR_PEAK_LIMIT	(1 << 1)
+#define AS_FAULT_INFO_INDICATOR_LED		(1 << 2)
+#define AS_FAULT_INFO_LED_AMOUNT		(1 << 3)
+#define AS_FAULT_INFO_TIMEOUT			(1 << 4)
+#define AS_FAULT_INFO_OVER_TEMPERATURE		(1 << 5)
+#define AS_FAULT_INFO_SHORT_CIRCUIT		(1 << 6)
+#define AS_FAULT_INFO_OVER_VOLTAGE		(1 << 7)
+
+/* Boost register */
+#define AS_BOOST_REG				0x0d
+#define AS_BOOST_CURRENT_DISABLE		(0 << 0)
+#define AS_BOOST_CURRENT_ENABLE			(1 << 0)
+
+/* Password register is used to unlock boost register writing */
+#define AS_PASSWORD_REG				0x0f
+#define AS_PASSWORD_UNLOCK_VALUE		0x55
+
+#define AS_NAME					"as3645a"
+#define AS_I2C_ADDR				(0x60 >> 1) /* W:0x60, R:0x61 */
+
+#define AS_FLASH_TIMEOUT_MIN			100000	/* us */
+#define AS_FLASH_TIMEOUT_MAX			850000
+#define AS_FLASH_TIMEOUT_STEP			50000
+
+#define AS_FLASH_INTENSITY_MIN			200000	/* uA */
+#define AS_FLASH_INTENSITY_MAX_1LED		500000
+#define AS_FLASH_INTENSITY_MAX_2LEDS		400000
+#define AS_FLASH_INTENSITY_STEP			20000
+
+#define AS_TORCH_INTENSITY_MIN			20000	/* uA */
+#define AS_TORCH_INTENSITY_MAX			160000
+#define AS_TORCH_INTENSITY_STEP			20000
+
+#define AS_INDICATOR_INTENSITY_MIN		0	/* uA */
+#define AS_INDICATOR_INTENSITY_MAX		10000
+#define AS_INDICATOR_INTENSITY_STEP		2500
+
+#define AS_PEAK_mA_MAX				2000
+#define AS_PEAK_mA_TO_REG(a) \
+	((min_t(u32, AS_PEAK_mA_MAX, a) - 1250) / 250)
+
+enum as_mode {
+	AS_MODE_EXT_TORCH = 0 << AS_CONTROL_MODE_SETTING_SHIFT,
+	AS_MODE_INDICATOR = 1 << AS_CONTROL_MODE_SETTING_SHIFT,
+	AS_MODE_ASSIST = 2 << AS_CONTROL_MODE_SETTING_SHIFT,
+	AS_MODE_FLASH = 3 << AS_CONTROL_MODE_SETTING_SHIFT,
+};
+
+struct as3645a_config {
+	u32 flash_timeout_us;
+	u32 flash_max_ua;
+	u32 assist_max_ua;
+	u32 indicator_max_ua;
+	u32 voltage_reference;
+	u32 peak;
+};
+
+struct as3645a {
+	struct i2c_client *client;
+
+	struct mutex mutex;
+
+	struct led_classdev_flash fled;
+	struct led_classdev iled_cdev;
+
+	struct v4l2_flash *vf;
+
+	struct as3645a_config cfg;
+
+	enum as_mode mode;
+	unsigned int timeout;
+	unsigned int flash_current;
+	unsigned int assist_current;
+	unsigned int indicator_current;
+	enum v4l2_flash_strobe_source strobe_source;
+};
+
+#define fled_to_as3645a(__fled) container_of(__fled, struct as3645a, fled)
+#define iled_cdev_to_as3645a(__iled_cdev) \
+	container_of(__iled_cdev, struct as3645a, iled_cdev)
+
+/* Return negative errno else zero on success */
+static int as3645a_write(struct as3645a *flash, u8 addr, u8 val)
+{
+	struct i2c_client *client = flash->client;
+	int rval;
+
+	rval = i2c_smbus_write_byte_data(client, addr, val);
+
+	dev_dbg(&client->dev, "Write Addr:%02X Val:%02X %s\n", addr, val,
+		rval < 0 ? "fail" : "ok");
+
+	return rval;
+}
+
+/* Return negative errno else a data byte received from the device. */
+static int as3645a_read(struct as3645a *flash, u8 addr)
+{
+	struct i2c_client *client = flash->client;
+	int rval;
+
+	rval = i2c_smbus_read_byte_data(client, addr);
+
+	dev_dbg(&client->dev, "Read Addr:%02X Val:%02X %s\n", addr, rval,
+		rval < 0 ? "fail" : "ok");
+
+	return rval;
+}
+
+/* -----------------------------------------------------------------------------
+ * Hardware configuration and trigger
+ */
+
+/*
+ * as3645a_set_config - Set flash configuration registers
+ * @flash: The flash
+ *
+ * Configure the hardware with flash, assist and indicator currents, as well as
+ * flash timeout.
+ *
+ * Return 0 on success, or a negative error code if an I2C communication error
+ * occurred.
+ */
+static int as3645a_set_current(struct as3645a *flash)
+{
+	u8 val;
+
+	val = (flash->flash_current << AS_CURRENT_FLASH_CURRENT_SHIFT)
+	    | (flash->assist_current << AS_CURRENT_ASSIST_LIGHT_SHIFT)
+	    | AS_CURRENT_LED_DET_ON;
+
+	return as3645a_write(flash, AS_CURRENT_SET_REG, val);
+}
+
+static int as3645a_set_timeout(struct as3645a *flash)
+{
+	u8 val;
+
+	val = flash->timeout << AS_INDICATOR_AND_TIMER_TIMEOUT_SHIFT;
+
+	val |= (flash->cfg.voltage_reference
+		<< AS_INDICATOR_AND_TIMER_VREF_SHIFT)
+	    |  ((flash->indicator_current ? flash->indicator_current - 1 : 0)
+		 << AS_INDICATOR_AND_TIMER_INDICATOR_SHIFT);
+
+	return as3645a_write(flash, AS_INDICATOR_AND_TIMER_REG, val);
+}
+
+/*
+ * as3645a_set_control - Set flash control register
+ * @flash: The flash
+ * @mode: Desired output mode
+ * @on: Desired output state
+ *
+ * Configure the hardware with output mode and state.
+ *
+ * Return 0 on success, or a negative error code if an I2C communication error
+ * occurred.
+ */
+static int
+as3645a_set_control(struct as3645a *flash, enum as_mode mode, bool on)
+{
+	u8 reg;
+
+	/* Configure output parameters and operation mode. */
+	reg = (flash->cfg.peak << AS_CONTROL_COIL_PEAK_SHIFT)
+	    | (on ? AS_CONTROL_OUT_ON : 0)
+	    | mode;
+
+	if (mode == AS_MODE_FLASH &&
+	    flash->strobe_source == V4L2_FLASH_STROBE_SOURCE_EXTERNAL)
+		reg |= AS_CONTROL_STROBE_TYPE_LEVEL
+		    |  AS_CONTROL_STROBE_ON;
+
+	return as3645a_write(flash, AS_CONTROL_REG, reg);
+}
+
+static int as3645a_get_fault(struct led_classdev_flash *fled, u32 *fault)
+{
+	struct as3645a *flash = fled_to_as3645a(fled);
+	int rval;
+
+	/* NOTE: reading register clear fault status */
+	rval = as3645a_read(flash, AS_FAULT_INFO_REG);
+	if (rval < 0)
+		return rval;
+
+	if (rval & AS_FAULT_INFO_INDUCTOR_PEAK_LIMIT)
+		*fault |= LED_FAULT_OVER_CURRENT;
+
+	if (rval & AS_FAULT_INFO_INDICATOR_LED)
+		*fault |= LED_FAULT_INDICATOR;
+
+	dev_dbg(&flash->client->dev, "%u connected LEDs\n",
+		rval & AS_FAULT_INFO_LED_AMOUNT ? 2 : 1);
+
+	if (rval & AS_FAULT_INFO_TIMEOUT)
+		*fault |= LED_FAULT_TIMEOUT;
+
+	if (rval & AS_FAULT_INFO_OVER_TEMPERATURE)
+		*fault |= LED_FAULT_OVER_TEMPERATURE;
+
+	if (rval & AS_FAULT_INFO_SHORT_CIRCUIT)
+		*fault |= LED_FAULT_OVER_CURRENT;
+
+	if (rval & AS_FAULT_INFO_OVER_VOLTAGE)
+		*fault |= LED_FAULT_INPUT_VOLTAGE;
+
+	return rval;
+}
+
+static unsigned int as3645a_current_to_reg(struct as3645a *flash, bool is_flash,
+					   unsigned int ua)
+{
+	struct {
+		unsigned int min;
+		unsigned int max;
+		unsigned int step;
+	} __mms[] = {
+		{
+			AS_TORCH_INTENSITY_MIN,
+			flash->cfg.assist_max_ua,
+			AS_TORCH_INTENSITY_STEP
+		},
+		{
+			AS_FLASH_INTENSITY_MIN,
+			flash->cfg.flash_max_ua,
+			AS_FLASH_INTENSITY_STEP
+		},
+	}, *mms = &__mms[is_flash];
+
+	if (ua < mms->min)
+		ua = mms->min;
+
+	if (ua > mms->max)
+		ua = mms->max;
+
+	return (ua - mms->min) / mms->step;
+}
+
+static int as3645a_set_indicator_brightness(struct led_classdev *iled_cdev,
+					    enum led_brightness brightness)
+{
+	struct as3645a *flash = iled_cdev_to_as3645a(iled_cdev);
+	int rval;
+
+	flash->indicator_current = brightness;
+
+	rval = as3645a_set_timeout(flash);
+	if (rval)
+		return rval;
+
+	return as3645a_set_control(flash, AS_MODE_INDICATOR, brightness);
+}
+
+static int as3645a_set_assist_brightness(struct led_classdev *fled_cdev,
+					 enum led_brightness brightness)
+{
+	struct led_classdev_flash *fled = lcdev_to_flcdev(fled_cdev);
+	struct as3645a *flash = fled_to_as3645a(fled);
+	int rval;
+
+	if (brightness) {
+		/* Register value 0 is 20 mA. */
+		flash->assist_current = brightness - 1;
+
+		rval = as3645a_set_current(flash);
+		if (rval)
+			return rval;
+	}
+
+	return as3645a_set_control(flash, AS_MODE_ASSIST, brightness);
+}
+
+static int as3645a_set_flash_brightness(struct led_classdev_flash *fled,
+					u32 brightness_ua)
+{
+	struct as3645a *flash = fled_to_as3645a(fled);
+
+	flash->flash_current = as3645a_current_to_reg(flash, true, brightness_ua);
+
+	return as3645a_set_current(flash);
+}
+
+static int as3645a_set_flash_timeout(struct led_classdev_flash *fled,
+				     u32 timeout_us)
+{
+	struct as3645a *flash = fled_to_as3645a(fled);
+
+	flash->timeout = AS_TIMER_US_TO_CODE(timeout_us);
+
+	return as3645a_set_timeout(flash);
+}
+
+static int as3645a_set_strobe(struct led_classdev_flash *fled, bool state)
+{
+	struct as3645a *flash = fled_to_as3645a(fled);
+
+	return as3645a_set_control(flash, AS_MODE_FLASH, state);
+}
+
+static const struct led_flash_ops as3645a_led_flash_ops = {
+	.flash_brightness_set = as3645a_set_flash_brightness,
+	.timeout_set = as3645a_set_flash_timeout,
+	.strobe_set = as3645a_set_strobe,
+	.fault_get = as3645a_get_fault,
+};
+
+static int as3645a_setup(struct as3645a *flash)
+{
+	struct device *dev = &flash->client->dev;
+	u32 fault = 0;
+	int rval;
+
+	/* clear errors */
+	rval = as3645a_read(flash, AS_FAULT_INFO_REG);
+	if (rval < 0)
+		return rval;
+
+	dev_dbg(dev, "Fault info: %02x\n", rval);
+
+	rval = as3645a_set_current(flash);
+	if (rval < 0)
+		return rval;
+
+	rval = as3645a_set_timeout(flash);
+	if (rval < 0)
+		return rval;
+
+	rval = as3645a_set_control(flash, AS_MODE_INDICATOR, false);
+	if (rval < 0)
+		return rval;
+
+	/* read status */
+	rval = as3645a_get_fault(&flash->fled, &fault);
+	if (rval < 0)
+		return rval;
+
+	dev_dbg(dev, "AS_INDICATOR_AND_TIMER_REG: %02x\n",
+		as3645a_read(flash, AS_INDICATOR_AND_TIMER_REG));
+	dev_dbg(dev, "AS_CURRENT_SET_REG: %02x\n",
+		as3645a_read(flash, AS_CURRENT_SET_REG));
+	dev_dbg(dev, "AS_CONTROL_REG: %02x\n",
+		as3645a_read(flash, AS_CONTROL_REG));
+
+	return rval & ~AS_FAULT_INFO_LED_AMOUNT ? -EIO : 0;
+}
+
+static int as3645a_detect(struct as3645a *flash)
+{
+	struct device *dev = &flash->client->dev;
+	int rval, man, model, rfu, version;
+	const char *vendor;
+
+	rval = as3645a_read(flash, AS_DESIGN_INFO_REG);
+	if (rval < 0) {
+		dev_err(dev, "can't read design info reg\n");
+		return rval;
+	}
+
+	man = AS_DESIGN_INFO_FACTORY(rval);
+	model = AS_DESIGN_INFO_MODEL(rval);
+
+	rval = as3645a_read(flash, AS_VERSION_CONTROL_REG);
+	if (rval < 0) {
+		dev_err(dev, "can't read version control reg\n");
+		return rval;
+	}
+
+	rfu = AS_VERSION_CONTROL_RFU(rval);
+	version = AS_VERSION_CONTROL_VERSION(rval);
+
+	/* Verify the chip model and version. */
+	if (model != 0x01 || rfu != 0x00) {
+		dev_err(dev, "AS3645A not detected "
+			"(model %d rfu %d)\n", model, rfu);
+		return -ENODEV;
+	}
+
+	switch (man) {
+	case 1:
+		vendor = "AMS, Austria Micro Systems";
+		break;
+	case 2:
+		vendor = "ADI, Analog Devices Inc.";
+		break;
+	case 3:
+		vendor = "NSC, National Semiconductor";
+		break;
+	case 4:
+		vendor = "NXP";
+		break;
+	case 5:
+		vendor = "TI, Texas Instrument";
+		break;
+	default:
+		vendor = "Unknown";
+	}
+
+	dev_info(dev, "Chip vendor: %s (%d) Version: %d\n", vendor,
+		 man, version);
+
+	rval = as3645a_write(flash, AS_PASSWORD_REG, AS_PASSWORD_UNLOCK_VALUE);
+	if (rval < 0)
+		return rval;
+
+	return as3645a_write(flash, AS_BOOST_REG, AS_BOOST_CURRENT_DISABLE);
+}
+
+static __maybe_unused int as3645a_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct as3645a *flash = i2c_get_clientdata(client);
+	int rval;
+
+	rval = as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
+	dev_dbg(dev, "Suspend %s\n", rval < 0 ? "failed" : "ok");
+
+	return rval;
+}
+
+static __maybe_unused int as3645a_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct as3645a *flash = i2c_get_clientdata(client);
+	int rval;
+
+	rval = as3645a_setup(flash);
+
+	dev_dbg(dev, "Resume %s\n", rval < 0 ? "fail" : "ok");
+
+	return rval;
+}
+
+static int as3645a_parse_node(struct as3645a *flash,
+			      struct device_node *node)
+{
+	struct as3645a_config *cfg = &flash->cfg;
+	struct device_node *child;
+	int rval;
+
+	child = of_get_child_by_name(node, "flash");
+	if (!child) {
+		dev_err(&flash->client->dev, "can't find flash node\n");
+		return -ENODEV;
+	}
+
+	rval = of_property_read_u32(child, "flash-timeout-us",
+				   &cfg->flash_timeout_us);
+	if (rval < 0) {
+		dev_err(&flash->client->dev,
+			"can't read flash-timeout-us property for flash\n");
+		goto out_err;
+	}
+
+	rval = of_property_read_u32(child, "flash-max-microamp",
+				   &cfg->flash_max_ua);
+	if (rval < 0) {
+		dev_err(&flash->client->dev,
+			"can't read flash-max-microamp property for flash\n");
+		goto out_err;
+	}
+
+	rval = of_property_read_u32(child, "led-max-microamp",
+				   &cfg->assist_max_ua);
+	if (rval < 0) {
+		dev_err(&flash->client->dev,
+			"can't read led-max-microamp property for flash\n");
+		goto out_err;
+	}
+
+	of_property_read_u32(child, "voltage-reference",
+			     &cfg->voltage_reference);
+
+	of_property_read_u32(child, "peak-current-limit", &cfg->peak);
+	cfg->peak = AS_PEAK_mA_TO_REG(cfg->peak);
+
+	of_node_put(child);
+
+	child = of_get_child_by_name(node, "indicator");
+	if (!child) {
+		dev_warn(&flash->client->dev,
+			 "can't find indicator node\n");
+		return 0;
+	}
+
+	rval = of_property_read_u32(child, "led-max-microamp",
+				   &cfg->indicator_max_ua);
+	if (rval < 0) {
+		dev_err(&flash->client->dev,
+			"can't read led-max-microamp property for indicator\n");
+		goto out_err;
+	}
+
+	of_node_put(child);
+
+	return 0;
+
+out_err:
+	of_node_put(child);
+
+	return rval;
+}
+
+static int as3645a_led_class_setup(struct as3645a *flash)
+{
+	struct led_classdev *fled_cdev = &flash->fled.led_cdev;
+	struct led_classdev *iled_cdev = &flash->iled_cdev;
+	struct led_flash_setting *cfg;
+	int rval;
+
+	iled_cdev->name = "as3645a indicator";
+	iled_cdev->brightness_set_blocking = as3645a_set_indicator_brightness;
+	iled_cdev->max_brightness =
+		flash->cfg.indicator_max_ua / AS_INDICATOR_INTENSITY_STEP;
+
+	rval = led_classdev_register(&flash->client->dev, iled_cdev);
+	if (rval < 0)
+		return rval;
+
+	cfg = &flash->fled.brightness;
+	cfg->min = AS_FLASH_INTENSITY_MIN;
+	cfg->max = flash->cfg.flash_max_ua;
+	cfg->step = AS_FLASH_INTENSITY_STEP;
+	cfg->val = flash->cfg.flash_max_ua;
+
+	cfg = &flash->fled.timeout;
+	cfg->min = AS_FLASH_TIMEOUT_MIN;
+	cfg->max = flash->cfg.flash_timeout_us;
+	cfg->step = AS_FLASH_TIMEOUT_STEP;
+	cfg->val = flash->cfg.flash_timeout_us;
+
+	flash->fled.ops = &as3645a_led_flash_ops;
+
+	fled_cdev->name = "as3645a flash";
+	fled_cdev->brightness_set_blocking = as3645a_set_assist_brightness;
+	/* Value 0 is off in LED class. */
+	fled_cdev->max_brightness =
+		as3645a_current_to_reg(flash, false,
+				       flash->cfg.assist_max_ua) + 1;
+	fled_cdev->flags = LED_DEV_CAP_FLASH;
+
+	rval = led_classdev_flash_register(&flash->client->dev, &flash->fled);
+	if (rval) {
+		led_classdev_unregister(iled_cdev);
+		dev_err(&flash->client->dev,
+			"led_classdev_flash_register() failed, error %d\n",
+			rval);
+	}
+
+	return rval;
+}
+
+static int as3645a_v4l2_setup(struct as3645a *flash)
+{
+	struct led_classdev_flash *fled = &flash->fled;
+	struct led_classdev *led = &fled->led_cdev;
+	struct v4l2_flash_config cfg = {
+		.torch_intensity = {
+			.min = AS_TORCH_INTENSITY_MIN,
+			.max = flash->cfg.assist_max_ua,
+			.step = AS_TORCH_INTENSITY_STEP,
+			.val = flash->cfg.assist_max_ua,
+		},
+		.indicator_intensity = {
+			.min = AS_INDICATOR_INTENSITY_MIN,
+			.max = flash->cfg.indicator_max_ua,
+			.step = AS_INDICATOR_INTENSITY_STEP,
+			.val = flash->cfg.indicator_max_ua,
+		},
+	};
+
+	strlcpy(cfg.dev_name, led->name, sizeof(cfg.dev_name));
+
+	flash->vf = v4l2_flash_init(&flash->client->dev, NULL, &flash->fled,
+				    &flash->iled_cdev, NULL, &cfg);
+	if (IS_ERR(flash->vf))
+		return PTR_ERR(flash->vf);
+
+	return 0;
+}
+
+static int as3645a_probe(struct i2c_client *client)
+{
+	struct as3645a *flash;
+	int rval;
+
+	if (client->dev.of_node == NULL)
+		return -ENODEV;
+
+	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
+	if (flash == NULL)
+		return -ENOMEM;
+
+	flash->client = client;
+
+	rval = as3645a_parse_node(flash, client->dev.of_node);
+	if (rval < 0)
+		return rval;
+
+	rval = as3645a_detect(flash);
+	if (rval < 0)
+		return rval;
+
+	mutex_init(&flash->mutex);
+	i2c_set_clientdata(client, flash);
+
+	rval = as3645a_setup(flash);
+	if (rval)
+		goto out_mutex_destroy;
+
+	rval = as3645a_led_class_setup(flash);
+	if (rval)
+		goto out_mutex_destroy;
+
+	rval = as3645a_v4l2_setup(flash);
+	if (rval)
+		goto out_led_classdev_flash_unregister;
+
+	return 0;
+
+out_led_classdev_flash_unregister:
+	led_classdev_flash_unregister(&flash->fled);
+
+out_mutex_destroy:
+	mutex_destroy(&flash->mutex);
+
+	return rval;
+}
+
+static int as3645a_remove(struct i2c_client *client)
+{
+	struct as3645a *flash = i2c_get_clientdata(client);
+
+	as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
+
+	v4l2_flash_release(flash->vf);
+
+	led_classdev_flash_unregister(&flash->fled);
+	led_classdev_unregister(&flash->iled_cdev);
+
+	mutex_destroy(&flash->mutex);
+
+	return 0;
+}
+
+static const struct of_device_id as3645a_of_table[] = {
+	{ .compatible = "ams,as3645a" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, as3645a_of_table);
+
+SIMPLE_DEV_PM_OPS(as3645a_pm_ops, as3645a_resume, as3645a_suspend);
+
+static struct i2c_driver as3645a_i2c_driver = {
+	.driver	= {
+		.of_match_table = as3645a_of_table,
+		.name = AS_NAME,
+		.pm   = &as3645a_pm_ops,
+	},
+	.probe_new	= as3645a_probe,
+	.remove	= as3645a_remove,
+};
+
+module_i2c_driver(as3645a_i2c_driver);
+
+MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>");
+MODULE_AUTHOR("Sakari Ailus <sakari.ailus-X3B1VOXEql0@public.gmane.org>");
+MODULE_DESCRIPTION("LED flash driver for AS3645A, LM3555 and their clones");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] leds: as3645a: Add LED flash class driver

[Jacek Anaszewski]

Hi Sakari,

I have two remarks in the code below.

On 06/14/2017 11:47 AM, Sakari Ailus wrote:
> From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> Add a LED flash class driver for the as3654a flash controller. A V4L2 flash
> driver for it already exists (drivers/media/i2c/as3645a.c), and this driver
> is based on that.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  MAINTAINERS                 |   6 +
>  drivers/leds/Kconfig        |   8 +
>  drivers/leds/Makefile       |   1 +
>  drivers/leds/leds-as3645a.c | 744 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 759 insertions(+)
>  create mode 100644 drivers/leds/leds-as3645a.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 053c3bd..c7682af 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2067,6 +2067,12 @@ F:	arch/arm64/
>  F:	Documentation/arm64/
>  
>  AS3645A LED FLASH CONTROLLER DRIVER
> +M:	Sakari Ailus <sakari.ailus@iki.fi>
> +L:	linux-leds@vger.kernel.org
> +S:	Maintained
> +F:	drivers/leds/leds-as3645a.c
> +
> +AS3645A LED FLASH CONTROLLER DRIVER
>  M:	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>  L:	linux-media@vger.kernel.org
>  T:	git git://linuxtv.org/media_tree.git
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 6c29998..9fb1d86 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -58,6 +58,14 @@ config LEDS_AAT1290
>  	help
>  	 This option enables support for the LEDs on the AAT1290.
>  
> +config LEDS_AS3645A
> +	tristate "AS3645A LED flash controller support"
> +	depends on I2C && LEDS_CLASS_FLASH
> +	help
> +	  Enable LED flash class support for AS3645A LED flash
> +	  controller. V4L2 flash API is provided as well if
> +	  CONFIG_V4L2_FLASH_API is enabled.
> +
>  config LEDS_BCM6328
>  	tristate "LED Support for Broadcom BCM6328"
>  	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 45f1339..b4def76 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
>  # LED Platform Drivers
>  obj-$(CONFIG_LEDS_88PM860X)		+= leds-88pm860x.o
>  obj-$(CONFIG_LEDS_AAT1290)		+= leds-aat1290.o
> +obj-$(CONFIG_LEDS_AS3645A)		+= leds-as3645a.o
>  obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
>  obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
>  obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
> diff --git a/drivers/leds/leds-as3645a.c b/drivers/leds/leds-as3645a.c
> new file mode 100644
> index 0000000..862d1b5
> --- /dev/null
> +++ b/drivers/leds/leds-as3645a.c
> @@ -0,0 +1,744 @@
> +/*
> + * drivers/leds/leds-as3645a.c - AS3645A and LM3555 flash controllers driver
> + *
> + * Copyright (C) 2008-2011 Nokia Corporation
> + * Copyright (c) 2011, 2017 Intel Corporation.
> + *
> + * Based on drivers/media/i2c/as3645a.c.
> + *
> + * Contact: Sakari Ailus <sakari.ailus@iki.fi>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/led-class-flash.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +
> +#include <media/v4l2-flash-led-class.h>
> +
> +#define AS_TIMER_US_TO_CODE(t)			(((t) / 1000 - 100) / 50)
> +#define AS_TIMER_CODE_TO_US(c)			((50 * (c) + 100) * 1000)
> +
> +/* Register definitions */
> +
> +/* Read-only Design info register: Reset state: xxxx 0001 */
> +#define AS_DESIGN_INFO_REG			0x00
> +#define AS_DESIGN_INFO_FACTORY(x)		(((x) >> 4))
> +#define AS_DESIGN_INFO_MODEL(x)			((x) & 0x0f)
> +
> +/* Read-only Version control register: Reset state: 0000 0000
> + * for first engineering samples
> + */
> +#define AS_VERSION_CONTROL_REG			0x01
> +#define AS_VERSION_CONTROL_RFU(x)		(((x) >> 4))
> +#define AS_VERSION_CONTROL_VERSION(x)		((x) & 0x0f)
> +
> +/* Read / Write	(Indicator and timer register): Reset state: 0000 1111 */
> +#define AS_INDICATOR_AND_TIMER_REG		0x02
> +#define AS_INDICATOR_AND_TIMER_TIMEOUT_SHIFT	0
> +#define AS_INDICATOR_AND_TIMER_VREF_SHIFT	4
> +#define AS_INDICATOR_AND_TIMER_INDICATOR_SHIFT	6
> +
> +/* Read / Write	(Current set register): Reset state: 0110 1001 */
> +#define AS_CURRENT_SET_REG			0x03
> +#define AS_CURRENT_ASSIST_LIGHT_SHIFT		0
> +#define AS_CURRENT_LED_DET_ON			(1 << 3)
> +#define AS_CURRENT_FLASH_CURRENT_SHIFT		4
> +
> +/* Read / Write	(Control register): Reset state: 1011 0100 */
> +#define AS_CONTROL_REG				0x04
> +#define AS_CONTROL_MODE_SETTING_SHIFT		0
> +#define AS_CONTROL_STROBE_ON			(1 << 2)
> +#define AS_CONTROL_OUT_ON			(1 << 3)
> +#define AS_CONTROL_EXT_TORCH_ON			(1 << 4)
> +#define AS_CONTROL_STROBE_TYPE_EDGE		(0 << 5)
> +#define AS_CONTROL_STROBE_TYPE_LEVEL		(1 << 5)
> +#define AS_CONTROL_COIL_PEAK_SHIFT		6
> +
> +/* Read only (D3 is read / write) (Fault and info): Reset state: 0000 x000 */
> +#define AS_FAULT_INFO_REG			0x05
> +#define AS_FAULT_INFO_INDUCTOR_PEAK_LIMIT	(1 << 1)
> +#define AS_FAULT_INFO_INDICATOR_LED		(1 << 2)
> +#define AS_FAULT_INFO_LED_AMOUNT		(1 << 3)
> +#define AS_FAULT_INFO_TIMEOUT			(1 << 4)
> +#define AS_FAULT_INFO_OVER_TEMPERATURE		(1 << 5)
> +#define AS_FAULT_INFO_SHORT_CIRCUIT		(1 << 6)
> +#define AS_FAULT_INFO_OVER_VOLTAGE		(1 << 7)
> +
> +/* Boost register */
> +#define AS_BOOST_REG				0x0d
> +#define AS_BOOST_CURRENT_DISABLE		(0 << 0)
> +#define AS_BOOST_CURRENT_ENABLE			(1 << 0)
> +
> +/* Password register is used to unlock boost register writing */
> +#define AS_PASSWORD_REG				0x0f
> +#define AS_PASSWORD_UNLOCK_VALUE		0x55
> +
> +#define AS_NAME					"as3645a"
> +#define AS_I2C_ADDR				(0x60 >> 1) /* W:0x60, R:0x61 */
> +
> +#define AS_FLASH_TIMEOUT_MIN			100000	/* us */
> +#define AS_FLASH_TIMEOUT_MAX			850000
> +#define AS_FLASH_TIMEOUT_STEP			50000
> +
> +#define AS_FLASH_INTENSITY_MIN			200000	/* uA */
> +#define AS_FLASH_INTENSITY_MAX_1LED		500000
> +#define AS_FLASH_INTENSITY_MAX_2LEDS		400000
> +#define AS_FLASH_INTENSITY_STEP			20000
> +
> +#define AS_TORCH_INTENSITY_MIN			20000	/* uA */
> +#define AS_TORCH_INTENSITY_MAX			160000
> +#define AS_TORCH_INTENSITY_STEP			20000
> +
> +#define AS_INDICATOR_INTENSITY_MIN		0	/* uA */
> +#define AS_INDICATOR_INTENSITY_MAX		10000
> +#define AS_INDICATOR_INTENSITY_STEP		2500
> +
> +#define AS_PEAK_mA_MAX				2000
> +#define AS_PEAK_mA_TO_REG(a) \
> +	((min_t(u32, AS_PEAK_mA_MAX, a) - 1250) / 250)
> +
> +enum as_mode {
> +	AS_MODE_EXT_TORCH = 0 << AS_CONTROL_MODE_SETTING_SHIFT,
> +	AS_MODE_INDICATOR = 1 << AS_CONTROL_MODE_SETTING_SHIFT,
> +	AS_MODE_ASSIST = 2 << AS_CONTROL_MODE_SETTING_SHIFT,
> +	AS_MODE_FLASH = 3 << AS_CONTROL_MODE_SETTING_SHIFT,
> +};
> +
> +struct as3645a_config {
> +	u32 flash_timeout_us;
> +	u32 flash_max_ua;
> +	u32 assist_max_ua;
> +	u32 indicator_max_ua;
> +	u32 voltage_reference;
> +	u32 peak;
> +};
> +
> +struct as3645a {
> +	struct i2c_client *client;
> +
> +	struct mutex mutex;
> +
> +	struct led_classdev_flash fled;
> +	struct led_classdev iled_cdev;
> +
> +	struct v4l2_flash *vf;
> +
> +	struct as3645a_config cfg;
> +
> +	enum as_mode mode;
> +	unsigned int timeout;
> +	unsigned int flash_current;
> +	unsigned int assist_current;
> +	unsigned int indicator_current;
> +	enum v4l2_flash_strobe_source strobe_source;
> +};
> +
> +#define fled_to_as3645a(__fled) container_of(__fled, struct as3645a, fled)
> +#define iled_cdev_to_as3645a(__iled_cdev) \
> +	container_of(__iled_cdev, struct as3645a, iled_cdev)
> +
> +/* Return negative errno else zero on success */
> +static int as3645a_write(struct as3645a *flash, u8 addr, u8 val)
> +{
> +	struct i2c_client *client = flash->client;
> +	int rval;
> +
> +	rval = i2c_smbus_write_byte_data(client, addr, val);
> +
> +	dev_dbg(&client->dev, "Write Addr:%02X Val:%02X %s\n", addr, val,
> +		rval < 0 ? "fail" : "ok");
> +
> +	return rval;
> +}
> +
> +/* Return negative errno else a data byte received from the device. */
> +static int as3645a_read(struct as3645a *flash, u8 addr)
> +{
> +	struct i2c_client *client = flash->client;
> +	int rval;
> +
> +	rval = i2c_smbus_read_byte_data(client, addr);
> +
> +	dev_dbg(&client->dev, "Read Addr:%02X Val:%02X %s\n", addr, rval,
> +		rval < 0 ? "fail" : "ok");
> +
> +	return rval;
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * Hardware configuration and trigger
> + */
> +
> +/*
> + * as3645a_set_config - Set flash configuration registers
> + * @flash: The flash
> + *
> + * Configure the hardware with flash, assist and indicator currents, as well as
> + * flash timeout.
> + *
> + * Return 0 on success, or a negative error code if an I2C communication error
> + * occurred.
> + */
> +static int as3645a_set_current(struct as3645a *flash)
> +{
> +	u8 val;
> +
> +	val = (flash->flash_current << AS_CURRENT_FLASH_CURRENT_SHIFT)
> +	    | (flash->assist_current << AS_CURRENT_ASSIST_LIGHT_SHIFT)
> +	    | AS_CURRENT_LED_DET_ON;
> +
> +	return as3645a_write(flash, AS_CURRENT_SET_REG, val);
> +}
> +
> +static int as3645a_set_timeout(struct as3645a *flash)
> +{
> +	u8 val;
> +
> +	val = flash->timeout << AS_INDICATOR_AND_TIMER_TIMEOUT_SHIFT;
> +
> +	val |= (flash->cfg.voltage_reference
> +		<< AS_INDICATOR_AND_TIMER_VREF_SHIFT)
> +	    |  ((flash->indicator_current ? flash->indicator_current - 1 : 0)
> +		 << AS_INDICATOR_AND_TIMER_INDICATOR_SHIFT);
> +
> +	return as3645a_write(flash, AS_INDICATOR_AND_TIMER_REG, val);
> +}
> +
> +/*
> + * as3645a_set_control - Set flash control register
> + * @flash: The flash
> + * @mode: Desired output mode
> + * @on: Desired output state
> + *
> + * Configure the hardware with output mode and state.
> + *
> + * Return 0 on success, or a negative error code if an I2C communication error
> + * occurred.
> + */
> +static int
> +as3645a_set_control(struct as3645a *flash, enum as_mode mode, bool on)
> +{
> +	u8 reg;
> +
> +	/* Configure output parameters and operation mode. */
> +	reg = (flash->cfg.peak << AS_CONTROL_COIL_PEAK_SHIFT)
> +	    | (on ? AS_CONTROL_OUT_ON : 0)
> +	    | mode;
> +
> +	if (mode == AS_MODE_FLASH &&
> +	    flash->strobe_source == V4L2_FLASH_STROBE_SOURCE_EXTERNAL)
> +		reg |= AS_CONTROL_STROBE_TYPE_LEVEL
> +		    |  AS_CONTROL_STROBE_ON;
> +
> +	return as3645a_write(flash, AS_CONTROL_REG, reg);
> +}
> +
> +static int as3645a_get_fault(struct led_classdev_flash *fled, u32 *fault)
> +{
> +	struct as3645a *flash = fled_to_as3645a(fled);
> +	int rval;
> +
> +	/* NOTE: reading register clear fault status */
> +	rval = as3645a_read(flash, AS_FAULT_INFO_REG);
> +	if (rval < 0)
> +		return rval;
> +
> +	if (rval & AS_FAULT_INFO_INDUCTOR_PEAK_LIMIT)
> +		*fault |= LED_FAULT_OVER_CURRENT;
> +
> +	if (rval & AS_FAULT_INFO_INDICATOR_LED)
> +		*fault |= LED_FAULT_INDICATOR;
> +
> +	dev_dbg(&flash->client->dev, "%u connected LEDs\n",
> +		rval & AS_FAULT_INFO_LED_AMOUNT ? 2 : 1);
> +
> +	if (rval & AS_FAULT_INFO_TIMEOUT)
> +		*fault |= LED_FAULT_TIMEOUT;
> +
> +	if (rval & AS_FAULT_INFO_OVER_TEMPERATURE)
> +		*fault |= LED_FAULT_OVER_TEMPERATURE;
> +
> +	if (rval & AS_FAULT_INFO_SHORT_CIRCUIT)
> +		*fault |= LED_FAULT_OVER_CURRENT;
> +
> +	if (rval & AS_FAULT_INFO_OVER_VOLTAGE)
> +		*fault |= LED_FAULT_INPUT_VOLTAGE;
> +
> +	return rval;
> +}
> +
> +static unsigned int as3645a_current_to_reg(struct as3645a *flash, bool is_flash,
> +					   unsigned int ua)
> +{
> +	struct {
> +		unsigned int min;
> +		unsigned int max;
> +		unsigned int step;
> +	} __mms[] = {
> +		{
> +			AS_TORCH_INTENSITY_MIN,
> +			flash->cfg.assist_max_ua,
> +			AS_TORCH_INTENSITY_STEP
> +		},
> +		{
> +			AS_FLASH_INTENSITY_MIN,
> +			flash->cfg.flash_max_ua,
> +			AS_FLASH_INTENSITY_STEP
> +		},
> +	}, *mms = &__mms[is_flash];
> +
> +	if (ua < mms->min)
> +		ua = mms->min;
> +
> +	if (ua > mms->max)
> +		ua = mms->max;
> +
> +	return (ua - mms->min) / mms->step;
> +}
> +
> +static int as3645a_set_indicator_brightness(struct led_classdev *iled_cdev,
> +					    enum led_brightness brightness)
> +{
> +	struct as3645a *flash = iled_cdev_to_as3645a(iled_cdev);
> +	int rval;
> +
> +	flash->indicator_current = brightness;
> +
> +	rval = as3645a_set_timeout(flash);
> +	if (rval)
> +		return rval;
> +
> +	return as3645a_set_control(flash, AS_MODE_INDICATOR, brightness);
> +}
> +
> +static int as3645a_set_assist_brightness(struct led_classdev *fled_cdev,
> +					 enum led_brightness brightness)
> +{
> +	struct led_classdev_flash *fled = lcdev_to_flcdev(fled_cdev);
> +	struct as3645a *flash = fled_to_as3645a(fled);
> +	int rval;
> +
> +	if (brightness) {
> +		/* Register value 0 is 20 mA. */
> +		flash->assist_current = brightness - 1;
> +
> +		rval = as3645a_set_current(flash);
> +		if (rval)
> +			return rval;
> +	}
> +
> +	return as3645a_set_control(flash, AS_MODE_ASSIST, brightness);
> +}
> +
> +static int as3645a_set_flash_brightness(struct led_classdev_flash *fled,
> +					u32 brightness_ua)
> +{
> +	struct as3645a *flash = fled_to_as3645a(fled);
> +
> +	flash->flash_current = as3645a_current_to_reg(flash, true, brightness_ua);
> +
> +	return as3645a_set_current(flash);
> +}
> +
> +static int as3645a_set_flash_timeout(struct led_classdev_flash *fled,
> +				     u32 timeout_us)
> +{
> +	struct as3645a *flash = fled_to_as3645a(fled);
> +
> +	flash->timeout = AS_TIMER_US_TO_CODE(timeout_us);
> +
> +	return as3645a_set_timeout(flash);
> +}
> +
> +static int as3645a_set_strobe(struct led_classdev_flash *fled, bool state)
> +{
> +	struct as3645a *flash = fled_to_as3645a(fled);
> +
> +	return as3645a_set_control(flash, AS_MODE_FLASH, state);
> +}
> +
> +static const struct led_flash_ops as3645a_led_flash_ops = {
> +	.flash_brightness_set = as3645a_set_flash_brightness,
> +	.timeout_set = as3645a_set_flash_timeout,
> +	.strobe_set = as3645a_set_strobe,
> +	.fault_get = as3645a_get_fault,
> +};
> +
> +static int as3645a_setup(struct as3645a *flash)
> +{
> +	struct device *dev = &flash->client->dev;
> +	u32 fault = 0;
> +	int rval;
> +
> +	/* clear errors */
> +	rval = as3645a_read(flash, AS_FAULT_INFO_REG);
> +	if (rval < 0)
> +		return rval;
> +
> +	dev_dbg(dev, "Fault info: %02x\n", rval);
> +
> +	rval = as3645a_set_current(flash);
> +	if (rval < 0)
> +		return rval;
> +
> +	rval = as3645a_set_timeout(flash);
> +	if (rval < 0)
> +		return rval;
> +
> +	rval = as3645a_set_control(flash, AS_MODE_INDICATOR, false);
> +	if (rval < 0)
> +		return rval;
> +
> +	/* read status */
> +	rval = as3645a_get_fault(&flash->fled, &fault);
> +	if (rval < 0)
> +		return rval;
> +
> +	dev_dbg(dev, "AS_INDICATOR_AND_TIMER_REG: %02x\n",
> +		as3645a_read(flash, AS_INDICATOR_AND_TIMER_REG));
> +	dev_dbg(dev, "AS_CURRENT_SET_REG: %02x\n",
> +		as3645a_read(flash, AS_CURRENT_SET_REG));
> +	dev_dbg(dev, "AS_CONTROL_REG: %02x\n",
> +		as3645a_read(flash, AS_CONTROL_REG));
> +
> +	return rval & ~AS_FAULT_INFO_LED_AMOUNT ? -EIO : 0;
> +}
> +
> +static int as3645a_detect(struct as3645a *flash)
> +{
> +	struct device *dev = &flash->client->dev;
> +	int rval, man, model, rfu, version;
> +	const char *vendor;
> +
> +	rval = as3645a_read(flash, AS_DESIGN_INFO_REG);
> +	if (rval < 0) {
> +		dev_err(dev, "can't read design info reg\n");
> +		return rval;
> +	}
> +
> +	man = AS_DESIGN_INFO_FACTORY(rval);
> +	model = AS_DESIGN_INFO_MODEL(rval);
> +
> +	rval = as3645a_read(flash, AS_VERSION_CONTROL_REG);
> +	if (rval < 0) {
> +		dev_err(dev, "can't read version control reg\n");
> +		return rval;
> +	}
> +
> +	rfu = AS_VERSION_CONTROL_RFU(rval);
> +	version = AS_VERSION_CONTROL_VERSION(rval);
> +
> +	/* Verify the chip model and version. */
> +	if (model != 0x01 || rfu != 0x00) {
> +		dev_err(dev, "AS3645A not detected "
> +			"(model %d rfu %d)\n", model, rfu);
> +		return -ENODEV;
> +	}
> +
> +	switch (man) {
> +	case 1:
> +		vendor = "AMS, Austria Micro Systems";
> +		break;
> +	case 2:
> +		vendor = "ADI, Analog Devices Inc.";
> +		break;
> +	case 3:
> +		vendor = "NSC, National Semiconductor";
> +		break;
> +	case 4:
> +		vendor = "NXP";
> +		break;
> +	case 5:
> +		vendor = "TI, Texas Instrument";
> +		break;
> +	default:
> +		vendor = "Unknown";
> +	}
> +
> +	dev_info(dev, "Chip vendor: %s (%d) Version: %d\n", vendor,
> +		 man, version);
> +
> +	rval = as3645a_write(flash, AS_PASSWORD_REG, AS_PASSWORD_UNLOCK_VALUE);
> +	if (rval < 0)
> +		return rval;
> +
> +	return as3645a_write(flash, AS_BOOST_REG, AS_BOOST_CURRENT_DISABLE);
> +}
> +
> +static __maybe_unused int as3645a_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct as3645a *flash = i2c_get_clientdata(client);
> +	int rval;
> +
> +	rval = as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
> +	dev_dbg(dev, "Suspend %s\n", rval < 0 ? "failed" : "ok");
> +
> +	return rval;
> +}
> +
> +static __maybe_unused int as3645a_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct as3645a *flash = i2c_get_clientdata(client);
> +	int rval;
> +
> +	rval = as3645a_setup(flash);
> +

nitpicking: inconsistent coding style - there is no empty line before
dev_dbg() in the as3645a_suspend().

> +	dev_dbg(dev, "Resume %s\n", rval < 0 ? "fail" : "ok");
> +
> +	return rval;
> +}
> +
> +static int as3645a_parse_node(struct as3645a *flash,
> +			      struct device_node *node)
> +{
> +	struct as3645a_config *cfg = &flash->cfg;
> +	struct device_node *child;
> +	int rval;
> +
> +	child = of_get_child_by_name(node, "flash");
> +	if (!child) {
> +		dev_err(&flash->client->dev, "can't find flash node\n");
> +		return -ENODEV;
> +	}
> +
> +	rval = of_property_read_u32(child, "flash-timeout-us",
> +				   &cfg->flash_timeout_us);
> +	if (rval < 0) {
> +		dev_err(&flash->client->dev,
> +			"can't read flash-timeout-us property for flash\n");
> +		goto out_err;
> +	}
> +
> +	rval = of_property_read_u32(child, "flash-max-microamp",
> +				   &cfg->flash_max_ua);
> +	if (rval < 0) {
> +		dev_err(&flash->client->dev,
> +			"can't read flash-max-microamp property for flash\n");
> +		goto out_err;
> +	}
> +
> +	rval = of_property_read_u32(child, "led-max-microamp",
> +				   &cfg->assist_max_ua);
> +	if (rval < 0) {
> +		dev_err(&flash->client->dev,
> +			"can't read led-max-microamp property for flash\n");
> +		goto out_err;
> +	}
> +
> +	of_property_read_u32(child, "voltage-reference",
> +			     &cfg->voltage_reference);
> +
> +	of_property_read_u32(child, "peak-current-limit", &cfg->peak);
> +	cfg->peak = AS_PEAK_mA_TO_REG(cfg->peak);
> +
> +	of_node_put(child);
> +
> +	child = of_get_child_by_name(node, "indicator");
> +	if (!child) {
> +		dev_warn(&flash->client->dev,
> +			 "can't find indicator node\n");
> +		return 0;
> +	}
> +
> +	rval = of_property_read_u32(child, "led-max-microamp",
> +				   &cfg->indicator_max_ua);
> +	if (rval < 0) {
> +		dev_err(&flash->client->dev,
> +			"can't read led-max-microamp property for indicator\n");
> +		goto out_err;
> +	}
> +
> +	of_node_put(child);
> +
> +	return 0;
> +
> +out_err:
> +	of_node_put(child);
> +
> +	return rval;
> +}
> +
> +static int as3645a_led_class_setup(struct as3645a *flash)
> +{
> +	struct led_classdev *fled_cdev = &flash->fled.led_cdev;
> +	struct led_classdev *iled_cdev = &flash->iled_cdev;
> +	struct led_flash_setting *cfg;
> +	int rval;
> +
> +	iled_cdev->name = "as3645a indicator";
> +	iled_cdev->brightness_set_blocking = as3645a_set_indicator_brightness;
> +	iled_cdev->max_brightness =
> +		flash->cfg.indicator_max_ua / AS_INDICATOR_INTENSITY_STEP;
> +
> +	rval = led_classdev_register(&flash->client->dev, iled_cdev);
> +	if (rval < 0)
> +		return rval;
> +
> +	cfg = &flash->fled.brightness;
> +	cfg->min = AS_FLASH_INTENSITY_MIN;
> +	cfg->max = flash->cfg.flash_max_ua;
> +	cfg->step = AS_FLASH_INTENSITY_STEP;
> +	cfg->val = flash->cfg.flash_max_ua;
> +
> +	cfg = &flash->fled.timeout;
> +	cfg->min = AS_FLASH_TIMEOUT_MIN;
> +	cfg->max = flash->cfg.flash_timeout_us;
> +	cfg->step = AS_FLASH_TIMEOUT_STEP;
> +	cfg->val = flash->cfg.flash_timeout_us;
> +
> +	flash->fled.ops = &as3645a_led_flash_ops;
> +
> +	fled_cdev->name = "as3645a flash";

LED class device name should be taken from label DT property,
or DT node name if the former wasn't defined.

Also LED device naming convention defines colon as a separator
between name segments.

> +	fled_cdev->brightness_set_blocking = as3645a_set_assist_brightness;
> +	/* Value 0 is off in LED class. */
> +	fled_cdev->max_brightness =
> +		as3645a_current_to_reg(flash, false,
> +				       flash->cfg.assist_max_ua) + 1;
> +	fled_cdev->flags = LED_DEV_CAP_FLASH;
> +
> +	rval = led_classdev_flash_register(&flash->client->dev, &flash->fled);
> +	if (rval) {
> +		led_classdev_unregister(iled_cdev);
> +		dev_err(&flash->client->dev,
> +			"led_classdev_flash_register() failed, error %d\n",
> +			rval);
> +	}
> +
> +	return rval;
> +}
> +
> +static int as3645a_v4l2_setup(struct as3645a *flash)
> +{
> +	struct led_classdev_flash *fled = &flash->fled;
> +	struct led_classdev *led = &fled->led_cdev;
> +	struct v4l2_flash_config cfg = {
> +		.torch_intensity = {
> +			.min = AS_TORCH_INTENSITY_MIN,
> +			.max = flash->cfg.assist_max_ua,
> +			.step = AS_TORCH_INTENSITY_STEP,
> +			.val = flash->cfg.assist_max_ua,
> +		},
> +		.indicator_intensity = {
> +			.min = AS_INDICATOR_INTENSITY_MIN,
> +			.max = flash->cfg.indicator_max_ua,
> +			.step = AS_INDICATOR_INTENSITY_STEP,
> +			.val = flash->cfg.indicator_max_ua,
> +		},
> +	};
> +
> +	strlcpy(cfg.dev_name, led->name, sizeof(cfg.dev_name));
> +
> +	flash->vf = v4l2_flash_init(&flash->client->dev, NULL, &flash->fled,
> +				    &flash->iled_cdev, NULL, &cfg);
> +	if (IS_ERR(flash->vf))
> +		return PTR_ERR(flash->vf);
> +
> +	return 0;
> +}
> +
> +static int as3645a_probe(struct i2c_client *client)
> +{
> +	struct as3645a *flash;
> +	int rval;
> +
> +	if (client->dev.of_node == NULL)
> +		return -ENODEV;
> +
> +	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
> +	if (flash == NULL)
> +		return -ENOMEM;
> +
> +	flash->client = client;
> +
> +	rval = as3645a_parse_node(flash, client->dev.of_node);
> +	if (rval < 0)
> +		return rval;
> +
> +	rval = as3645a_detect(flash);
> +	if (rval < 0)
> +		return rval;
> +
> +	mutex_init(&flash->mutex);
> +	i2c_set_clientdata(client, flash);
> +
> +	rval = as3645a_setup(flash);
> +	if (rval)
> +		goto out_mutex_destroy;
> +
> +	rval = as3645a_led_class_setup(flash);
> +	if (rval)
> +		goto out_mutex_destroy;
> +
> +	rval = as3645a_v4l2_setup(flash);
> +	if (rval)
> +		goto out_led_classdev_flash_unregister;
> +
> +	return 0;
> +
> +out_led_classdev_flash_unregister:
> +	led_classdev_flash_unregister(&flash->fled);
> +
> +out_mutex_destroy:
> +	mutex_destroy(&flash->mutex);
> +
> +	return rval;
> +}
> +
> +static int as3645a_remove(struct i2c_client *client)
> +{
> +	struct as3645a *flash = i2c_get_clientdata(client);
> +
> +	as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
> +
> +	v4l2_flash_release(flash->vf);
> +
> +	led_classdev_flash_unregister(&flash->fled);
> +	led_classdev_unregister(&flash->iled_cdev);
> +
> +	mutex_destroy(&flash->mutex);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id as3645a_of_table[] = {
> +	{ .compatible = "ams,as3645a" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, as3645a_of_table);
> +
> +SIMPLE_DEV_PM_OPS(as3645a_pm_ops, as3645a_resume, as3645a_suspend);
> +
> +static struct i2c_driver as3645a_i2c_driver = {
> +	.driver	= {
> +		.of_match_table = as3645a_of_table,
> +		.name = AS_NAME,
> +		.pm   = &as3645a_pm_ops,
> +	},
> +	.probe_new	= as3645a_probe,
> +	.remove	= as3645a_remove,
> +};
> +
> +module_i2c_driver(as3645a_i2c_driver);
> +
> +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
> +MODULE_AUTHOR("Sakari Ailus <sakari.ailus@iki.fi>");
> +MODULE_DESCRIPTION("LED flash driver for AS3645A, LM3555 and their clones");
> +MODULE_LICENSE("GPL v2");
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 6/8] leds: as3645a: Add LED flash class driver

[Sakari Ailus]

Hi Jacek,

Thanks for the review!

I have to say I found the v4l2-flash-led-class framework quite useful, now
that I refactored a driver for using it. Now we have a user for the
indicator, too. :-)

On Wed, Jun 14, 2017 at 11:15:24PM +0200, Jacek Anaszewski wrote:
> > +static __maybe_unused int as3645a_suspend(struct device *dev)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct as3645a *flash = i2c_get_clientdata(client);
> > +	int rval;
> > +
> > +	rval = as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
> > +	dev_dbg(dev, "Suspend %s\n", rval < 0 ? "failed" : "ok");
> > +
> > +	return rval;
> > +}
> > +
> > +static __maybe_unused int as3645a_resume(struct device *dev)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct as3645a *flash = i2c_get_clientdata(client);
> > +	int rval;
> > +
> > +	rval = as3645a_setup(flash);
> > +
> 
> nitpicking: inconsistent coding style - there is no empty line before
> dev_dbg() in the as3645a_suspend().

Added one for as3645a_suspend() --- it should have been there.

> 
> > +	dev_dbg(dev, "Resume %s\n", rval < 0 ? "fail" : "ok");
> > +
> > +	return rval;
> > +}

...

> > +static int as3645a_led_class_setup(struct as3645a *flash)
> > +{
> > +	struct led_classdev *fled_cdev = &flash->fled.led_cdev;
> > +	struct led_classdev *iled_cdev = &flash->iled_cdev;
> > +	struct led_flash_setting *cfg;
> > +	int rval;
> > +
> > +	iled_cdev->name = "as3645a indicator";
> > +	iled_cdev->brightness_set_blocking = as3645a_set_indicator_brightness;
> > +	iled_cdev->max_brightness =
> > +		flash->cfg.indicator_max_ua / AS_INDICATOR_INTENSITY_STEP;
> > +
> > +	rval = led_classdev_register(&flash->client->dev, iled_cdev);
> > +	if (rval < 0)
> > +		return rval;
> > +
> > +	cfg = &flash->fled.brightness;
> > +	cfg->min = AS_FLASH_INTENSITY_MIN;
> > +	cfg->max = flash->cfg.flash_max_ua;
> > +	cfg->step = AS_FLASH_INTENSITY_STEP;
> > +	cfg->val = flash->cfg.flash_max_ua;
> > +
> > +	cfg = &flash->fled.timeout;
> > +	cfg->min = AS_FLASH_TIMEOUT_MIN;
> > +	cfg->max = flash->cfg.flash_timeout_us;
> > +	cfg->step = AS_FLASH_TIMEOUT_STEP;
> > +	cfg->val = flash->cfg.flash_timeout_us;
> > +
> > +	flash->fled.ops = &as3645a_led_flash_ops;
> > +
> > +	fled_cdev->name = "as3645a flash";
> 
> LED class device name should be taken from label DT property,
> or DT node name if the former wasn't defined.
> 
> Also LED device naming convention defines colon as a separator
> between name segments.

Right. I'll fix that.

I just realised I'm missing DT binding documentation for this device; I'll
add that, too.

Is the preference to allow freely chosen node names for the LEDs? Now that
there's the label, too, this appears to be somewhat duplicated information.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 6/8] leds: as3645a: Add LED flash class driver

[Jacek Anaszewski]

Hi Sakari,

On 06/15/2017 12:10 AM, Sakari Ailus wrote:
> Hi Jacek,
> 
> Thanks for the review!

You're welcome!

> I have to say I found the v4l2-flash-led-class framework quite useful, now
> that I refactored a driver for using it. Now we have a user for the
> indicator, too. :-)

Nice :-). I'm also surprised that v4l2-flash API is also used in
drivers/staging/greybus/light.c which popped up with kbuild test robot
complaints.

> On Wed, Jun 14, 2017 at 11:15:24PM +0200, Jacek Anaszewski wrote:
>>> +static __maybe_unused int as3645a_suspend(struct device *dev)
>>> +{
>>> +	struct i2c_client *client = to_i2c_client(dev);
>>> +	struct as3645a *flash = i2c_get_clientdata(client);
>>> +	int rval;
>>> +
>>> +	rval = as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
>>> +	dev_dbg(dev, "Suspend %s\n", rval < 0 ? "failed" : "ok");
>>> +
>>> +	return rval;
>>> +}
>>> +
>>> +static __maybe_unused int as3645a_resume(struct device *dev)
>>> +{
>>> +	struct i2c_client *client = to_i2c_client(dev);
>>> +	struct as3645a *flash = i2c_get_clientdata(client);
>>> +	int rval;
>>> +
>>> +	rval = as3645a_setup(flash);
>>> +
>>
>> nitpicking: inconsistent coding style - there is no empty line before
>> dev_dbg() in the as3645a_suspend().
> 
> Added one for as3645a_suspend() --- it should have been there.
> 
>>
>>> +	dev_dbg(dev, "Resume %s\n", rval < 0 ? "fail" : "ok");
>>> +
>>> +	return rval;
>>> +}
> 
> ...
> 
>>> +static int as3645a_led_class_setup(struct as3645a *flash)
>>> +{
>>> +	struct led_classdev *fled_cdev = &flash->fled.led_cdev;
>>> +	struct led_classdev *iled_cdev = &flash->iled_cdev;
>>> +	struct led_flash_setting *cfg;
>>> +	int rval;
>>> +
>>> +	iled_cdev->name = "as3645a indicator";
>>> +	iled_cdev->brightness_set_blocking = as3645a_set_indicator_brightness;
>>> +	iled_cdev->max_brightness =
>>> +		flash->cfg.indicator_max_ua / AS_INDICATOR_INTENSITY_STEP;
>>> +
>>> +	rval = led_classdev_register(&flash->client->dev, iled_cdev);
>>> +	if (rval < 0)
>>> +		return rval;
>>> +
>>> +	cfg = &flash->fled.brightness;
>>> +	cfg->min = AS_FLASH_INTENSITY_MIN;
>>> +	cfg->max = flash->cfg.flash_max_ua;
>>> +	cfg->step = AS_FLASH_INTENSITY_STEP;
>>> +	cfg->val = flash->cfg.flash_max_ua;
>>> +
>>> +	cfg = &flash->fled.timeout;
>>> +	cfg->min = AS_FLASH_TIMEOUT_MIN;
>>> +	cfg->max = flash->cfg.flash_timeout_us;
>>> +	cfg->step = AS_FLASH_TIMEOUT_STEP;
>>> +	cfg->val = flash->cfg.flash_timeout_us;
>>> +
>>> +	flash->fled.ops = &as3645a_led_flash_ops;
>>> +
>>> +	fled_cdev->name = "as3645a flash";
>>
>> LED class device name should be taken from label DT property,
>> or DT node name if the former wasn't defined.
>>
>> Also LED device naming convention defines colon as a separator
>> between name segments.
> 
> Right. I'll fix that.
> 
> I just realised I'm missing DT binding documentation for this device; I'll
> add that, too.
> 
> Is the preference to allow freely chosen node names for the LEDs? Now that
> there's the label, too, this appears to be somewhat duplicated information.

It depends on whether the sub-leds are identified by reg property.
In this case usually common prefix is used followed by reg value,
e.g. led@1, led@2 etc.

Otherwise prevailing scheme is e.g.:

        blue-power {
		...
                label = "netxbig:blue:power";
	}

-- 
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] leds: as3645a: Add LED flash class driver

[Sakari Ailus]

Hi Jacek,

On Thu, Jun 15, 2017 at 03:01:47PM +0200, Jacek Anaszewski wrote:
> Hi Sakari,
> 
> On 06/15/2017 12:10 AM, Sakari Ailus wrote:
> > Hi Jacek,
> > 
> > Thanks for the review!
> 
> You're welcome!
> 
> > I have to say I found the v4l2-flash-led-class framework quite useful, now
> > that I refactored a driver for using it. Now we have a user for the
> > indicator, too. :-)
> 
> Nice :-). I'm also surprised that v4l2-flash API is also used in
> drivers/staging/greybus/light.c which popped up with kbuild test robot
> complaints.

I missed that on the first round of the submission as well. I'll fix that
in v2.

> 
> > On Wed, Jun 14, 2017 at 11:15:24PM +0200, Jacek Anaszewski wrote:
> >>> +static __maybe_unused int as3645a_suspend(struct device *dev)
> >>> +{
> >>> +	struct i2c_client *client = to_i2c_client(dev);
> >>> +	struct as3645a *flash = i2c_get_clientdata(client);
> >>> +	int rval;
> >>> +
> >>> +	rval = as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
> >>> +	dev_dbg(dev, "Suspend %s\n", rval < 0 ? "failed" : "ok");
> >>> +
> >>> +	return rval;
> >>> +}
> >>> +
> >>> +static __maybe_unused int as3645a_resume(struct device *dev)
> >>> +{
> >>> +	struct i2c_client *client = to_i2c_client(dev);
> >>> +	struct as3645a *flash = i2c_get_clientdata(client);
> >>> +	int rval;
> >>> +
> >>> +	rval = as3645a_setup(flash);
> >>> +
> >>
> >> nitpicking: inconsistent coding style - there is no empty line before
> >> dev_dbg() in the as3645a_suspend().
> > 
> > Added one for as3645a_suspend() --- it should have been there.
> > 
> >>
> >>> +	dev_dbg(dev, "Resume %s\n", rval < 0 ? "fail" : "ok");
> >>> +
> >>> +	return rval;
> >>> +}
> > 
> > ...
> > 
> >>> +static int as3645a_led_class_setup(struct as3645a *flash)
> >>> +{
> >>> +	struct led_classdev *fled_cdev = &flash->fled.led_cdev;
> >>> +	struct led_classdev *iled_cdev = &flash->iled_cdev;
> >>> +	struct led_flash_setting *cfg;
> >>> +	int rval;
> >>> +
> >>> +	iled_cdev->name = "as3645a indicator";
> >>> +	iled_cdev->brightness_set_blocking = as3645a_set_indicator_brightness;
> >>> +	iled_cdev->max_brightness =
> >>> +		flash->cfg.indicator_max_ua / AS_INDICATOR_INTENSITY_STEP;
> >>> +
> >>> +	rval = led_classdev_register(&flash->client->dev, iled_cdev);
> >>> +	if (rval < 0)
> >>> +		return rval;
> >>> +
> >>> +	cfg = &flash->fled.brightness;
> >>> +	cfg->min = AS_FLASH_INTENSITY_MIN;
> >>> +	cfg->max = flash->cfg.flash_max_ua;
> >>> +	cfg->step = AS_FLASH_INTENSITY_STEP;
> >>> +	cfg->val = flash->cfg.flash_max_ua;
> >>> +
> >>> +	cfg = &flash->fled.timeout;
> >>> +	cfg->min = AS_FLASH_TIMEOUT_MIN;
> >>> +	cfg->max = flash->cfg.flash_timeout_us;
> >>> +	cfg->step = AS_FLASH_TIMEOUT_STEP;
> >>> +	cfg->val = flash->cfg.flash_timeout_us;
> >>> +
> >>> +	flash->fled.ops = &as3645a_led_flash_ops;
> >>> +
> >>> +	fled_cdev->name = "as3645a flash";
> >>
> >> LED class device name should be taken from label DT property,
> >> or DT node name if the former wasn't defined.
> >>
> >> Also LED device naming convention defines colon as a separator
> >> between name segments.
> > 
> > Right. I'll fix that.
> > 
> > I just realised I'm missing DT binding documentation for this device; I'll
> > add that, too.
> > 
> > Is the preference to allow freely chosen node names for the LEDs? Now that
> > there's the label, too, this appears to be somewhat duplicated information.
> 
> It depends on whether the sub-leds are identified by reg property.
> In this case usually common prefix is used followed by reg value,
> e.g. led@1, led@2 etc.

Is there a device that would use this already? I checked common.txt and
I couldn't find a suggestion of this scheme there.

> 
> Otherwise prevailing scheme is e.g.:
> 
>         blue-power {
> 		...
>                 label = "netxbig:blue:power";
> 	}
> 

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 6/8] leds: as3645a: Add LED flash class driver

[Jacek Anaszewski]

On 06/15/2017 03:34 PM, Sakari Ailus wrote:
> Hi Jacek,
> 
> On Thu, Jun 15, 2017 at 03:01:47PM +0200, Jacek Anaszewski wrote:
>> Hi Sakari,
>>
>> On 06/15/2017 12:10 AM, Sakari Ailus wrote:
>>> Hi Jacek,
>>>
>>> Thanks for the review!
>>
>> You're welcome!
>>
>>> I have to say I found the v4l2-flash-led-class framework quite useful, now
>>> that I refactored a driver for using it. Now we have a user for the
>>> indicator, too. :-)
>>
>> Nice :-). I'm also surprised that v4l2-flash API is also used in
>> drivers/staging/greybus/light.c which popped up with kbuild test robot
>> complaints.
> 
> I missed that on the first round of the submission as well. I'll fix that
> in v2.
> 
>>
>>> On Wed, Jun 14, 2017 at 11:15:24PM +0200, Jacek Anaszewski wrote:
>>>>> +static __maybe_unused int as3645a_suspend(struct device *dev)
>>>>> +{
>>>>> +	struct i2c_client *client = to_i2c_client(dev);
>>>>> +	struct as3645a *flash = i2c_get_clientdata(client);
>>>>> +	int rval;
>>>>> +
>>>>> +	rval = as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
>>>>> +	dev_dbg(dev, "Suspend %s\n", rval < 0 ? "failed" : "ok");
>>>>> +
>>>>> +	return rval;
>>>>> +}
>>>>> +
>>>>> +static __maybe_unused int as3645a_resume(struct device *dev)
>>>>> +{
>>>>> +	struct i2c_client *client = to_i2c_client(dev);
>>>>> +	struct as3645a *flash = i2c_get_clientdata(client);
>>>>> +	int rval;
>>>>> +
>>>>> +	rval = as3645a_setup(flash);
>>>>> +
>>>>
>>>> nitpicking: inconsistent coding style - there is no empty line before
>>>> dev_dbg() in the as3645a_suspend().
>>>
>>> Added one for as3645a_suspend() --- it should have been there.
>>>
>>>>
>>>>> +	dev_dbg(dev, "Resume %s\n", rval < 0 ? "fail" : "ok");
>>>>> +
>>>>> +	return rval;
>>>>> +}
>>>
>>> ...
>>>
>>>>> +static int as3645a_led_class_setup(struct as3645a *flash)
>>>>> +{
>>>>> +	struct led_classdev *fled_cdev = &flash->fled.led_cdev;
>>>>> +	struct led_classdev *iled_cdev = &flash->iled_cdev;
>>>>> +	struct led_flash_setting *cfg;
>>>>> +	int rval;
>>>>> +
>>>>> +	iled_cdev->name = "as3645a indicator";
>>>>> +	iled_cdev->brightness_set_blocking = as3645a_set_indicator_brightness;
>>>>> +	iled_cdev->max_brightness =
>>>>> +		flash->cfg.indicator_max_ua / AS_INDICATOR_INTENSITY_STEP;
>>>>> +
>>>>> +	rval = led_classdev_register(&flash->client->dev, iled_cdev);
>>>>> +	if (rval < 0)
>>>>> +		return rval;
>>>>> +
>>>>> +	cfg = &flash->fled.brightness;
>>>>> +	cfg->min = AS_FLASH_INTENSITY_MIN;
>>>>> +	cfg->max = flash->cfg.flash_max_ua;
>>>>> +	cfg->step = AS_FLASH_INTENSITY_STEP;
>>>>> +	cfg->val = flash->cfg.flash_max_ua;
>>>>> +
>>>>> +	cfg = &flash->fled.timeout;
>>>>> +	cfg->min = AS_FLASH_TIMEOUT_MIN;
>>>>> +	cfg->max = flash->cfg.flash_timeout_us;
>>>>> +	cfg->step = AS_FLASH_TIMEOUT_STEP;
>>>>> +	cfg->val = flash->cfg.flash_timeout_us;
>>>>> +
>>>>> +	flash->fled.ops = &as3645a_led_flash_ops;
>>>>> +
>>>>> +	fled_cdev->name = "as3645a flash";
>>>>
>>>> LED class device name should be taken from label DT property,
>>>> or DT node name if the former wasn't defined.
>>>>
>>>> Also LED device naming convention defines colon as a separator
>>>> between name segments.
>>>
>>> Right. I'll fix that.
>>>
>>> I just realised I'm missing DT binding documentation for this device; I'll
>>> add that, too.
>>>
>>> Is the preference to allow freely chosen node names for the LEDs? Now that
>>> there's the label, too, this appears to be somewhat duplicated information.
>>
>> It depends on whether the sub-leds are identified by reg property.
>> In this case usually common prefix is used followed by reg value,
>> e.g. led@1, led@2 etc.
> 
> Is there a device that would use this already? I checked common.txt and
> I couldn't find a suggestion of this scheme there.

There is no suitable suggestion in common.txt indeed, but it is used
e.g. in:

leds-mt6323.txt, leds-bcm6328.txt, leds-pm8058.txt.

>> Otherwise prevailing scheme is e.g.:
>>
>>         blue-power {
>> 		...
>>                 label = "netxbig:blue:power";
>> 	}
>>
> 

-- 
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] leds: as3645a: Add LED flash class driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2173 bytes --]

Hi!

> From: Sakari Ailus <sakari.ailus@iki.fi>

That address no longer works, right?

> Add a LED flash class driver for the as3654a flash controller. A V4L2 flash
> driver for it already exists (drivers/media/i2c/as3645a.c), and this driver
> is based on that.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> + * Based on drivers/media/i2c/as3645a.c.
> + *
> + * Contact: Sakari Ailus <sakari.ailus@iki.fi>

So I believe it should not be here.

> +/*
> + * as3645a_set_config - Set flash configuration registers
> + * @flash: The flash
> + *

/** for linuxdoc? 

> +	struct as3645a *flash = fled_to_as3645a(fled);
> +	int rval;
> +
> +	/* NOTE: reading register clear fault status */

clears.

> +static unsigned int as3645a_current_to_reg(struct as3645a *flash, bool is_flash,
> +					   unsigned int ua)
> +{
> +	struct {
> +		unsigned int min;
> +		unsigned int max;
> +		unsigned int step;
> +	} __mms[] = {
> +		{
> +			AS_TORCH_INTENSITY_MIN,
> +			flash->cfg.assist_max_ua,
> +			AS_TORCH_INTENSITY_STEP
> +		},
> +		{
> +			AS_FLASH_INTENSITY_MIN,
> +			flash->cfg.flash_max_ua,
> +			AS_FLASH_INTENSITY_STEP
> +		},
> +	}, *mms = &__mms[is_flash];
> +
> +	if (ua < mms->min)
> +		ua = mms->min;

That's some... seriously interesting code. And you are forcing gcc to
create quite interesting structure on stack. Would it be easier to do
normal if()... without this magic?

> +	struct v4l2_flash_config cfg = {
> +		.torch_intensity = {
> +			.min = AS_TORCH_INTENSITY_MIN,
> +			.max = flash->cfg.assist_max_ua,
> +			.step = AS_TORCH_INTENSITY_STEP,
> +			.val = flash->cfg.assist_max_ua,
> +		},
> +		.indicator_intensity = {
> +			.min = AS_INDICATOR_INTENSITY_MIN,
> +			.max = flash->cfg.indicator_max_ua,
> +			.step = AS_INDICATOR_INTENSITY_STEP,
> +			.val = flash->cfg.indicator_max_ua,
> +		},
> +	};

Ugh. And here you have copy of the above struct, + .val. Can it be
somehow de-duplicated?

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 6/8] leds: as3645a: Add LED flash class driver

[Sakari Ailus]

Hi Pavel,

Thanks for the review!

On Wed, Jun 14, 2017 at 11:39:41PM +0200, Pavel Machek wrote:
> Hi!
> 
> > From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> That address no longer works, right?

Why wouldn't it work? Or... do you know something I don't? :-)

> 
> > Add a LED flash class driver for the as3654a flash controller. A V4L2 flash
> > driver for it already exists (drivers/media/i2c/as3645a.c), and this driver
> > is based on that.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> > + * Based on drivers/media/i2c/as3645a.c.
> > + *
> > + * Contact: Sakari Ailus <sakari.ailus@iki.fi>
> 
> So I believe it should not be here.
> 
> > +/*
> > + * as3645a_set_config - Set flash configuration registers
> > + * @flash: The flash
> > + *
> 
> /** for linuxdoc? 

Fixed.

> 
> > +	struct as3645a *flash = fled_to_as3645a(fled);
> > +	int rval;
> > +
> > +	/* NOTE: reading register clear fault status */
> 
> clears.

Fixed.

> 
> > +static unsigned int as3645a_current_to_reg(struct as3645a *flash, bool is_flash,
> > +					   unsigned int ua)
> > +{
> > +	struct {
> > +		unsigned int min;
> > +		unsigned int max;
> > +		unsigned int step;
> > +	} __mms[] = {
> > +		{
> > +			AS_TORCH_INTENSITY_MIN,
> > +			flash->cfg.assist_max_ua,
> > +			AS_TORCH_INTENSITY_STEP
> > +		},
> > +		{
> > +			AS_FLASH_INTENSITY_MIN,
> > +			flash->cfg.flash_max_ua,
> > +			AS_FLASH_INTENSITY_STEP
> > +		},
> > +	}, *mms = &__mms[is_flash];
> > +
> > +	if (ua < mms->min)
> > +		ua = mms->min;
> 
> That's some... seriously interesting code. And you are forcing gcc to
> create quite interesting structure on stack. Would it be easier to do
> normal if()... without this magic?
> 
> > +	struct v4l2_flash_config cfg = {
> > +		.torch_intensity = {
> > +			.min = AS_TORCH_INTENSITY_MIN,
> > +			.max = flash->cfg.assist_max_ua,
> > +			.step = AS_TORCH_INTENSITY_STEP,
> > +			.val = flash->cfg.assist_max_ua,
> > +		},
> > +		.indicator_intensity = {
> > +			.min = AS_INDICATOR_INTENSITY_MIN,
> > +			.max = flash->cfg.indicator_max_ua,
> > +			.step = AS_INDICATOR_INTENSITY_STEP,
> > +			.val = flash->cfg.indicator_max_ua,
> > +		},
> > +	};
> 
> Ugh. And here you have copy of the above struct, + .val. Can it be
> somehow de-duplicated?

The flash_brightness_set callback uses micro-Amps as the unit and the driver
needs to convert that to its own specific units. Yeah, there would be
probably an easier way, too. But that'd likely require changes to the LED
flash class.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 6/8] leds: as3645a: Add LED flash class driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2409 bytes --]

Hi!

> Thanks for the review!

You are welcome :-).

> On Wed, Jun 14, 2017 at 11:39:41PM +0200, Pavel Machek wrote:
> > Hi!
> > 
> > > From: Sakari Ailus <sakari.ailus@iki.fi>
> > 
> > That address no longer works, right?
> 
> Why wouldn't it work? Or... do you know something I don't? :-)

Aha. I thought I was removing it from source files because it was no
longer working, but maybe I'm misremembering? 

> > > +static unsigned int as3645a_current_to_reg(struct as3645a *flash, bool is_flash,
> > > +					   unsigned int ua)
> > > +{
> > > +	struct {
> > > +		unsigned int min;
> > > +		unsigned int max;
> > > +		unsigned int step;
> > > +	} __mms[] = {
> > > +		{
> > > +			AS_TORCH_INTENSITY_MIN,
> > > +			flash->cfg.assist_max_ua,
> > > +			AS_TORCH_INTENSITY_STEP
> > > +		},
> > > +		{
> > > +			AS_FLASH_INTENSITY_MIN,
> > > +			flash->cfg.flash_max_ua,
> > > +			AS_FLASH_INTENSITY_STEP
> > > +		},
> > > +	}, *mms = &__mms[is_flash];
> > > +
> > > +	if (ua < mms->min)
> > > +		ua = mms->min;
> > 
> > That's some... seriously interesting code. And you are forcing gcc to
> > create quite interesting structure on stack. Would it be easier to do
> > normal if()... without this magic?
> > 
> > > +	struct v4l2_flash_config cfg = {
> > > +		.torch_intensity = {
> > > +			.min = AS_TORCH_INTENSITY_MIN,
> > > +			.max = flash->cfg.assist_max_ua,
> > > +			.step = AS_TORCH_INTENSITY_STEP,
> > > +			.val = flash->cfg.assist_max_ua,
> > > +		},
> > > +		.indicator_intensity = {
> > > +			.min = AS_INDICATOR_INTENSITY_MIN,
> > > +			.max = flash->cfg.indicator_max_ua,
> > > +			.step = AS_INDICATOR_INTENSITY_STEP,
> > > +			.val = flash->cfg.indicator_max_ua,
> > > +		},
> > > +	};
> > 
> > Ugh. And here you have copy of the above struct, + .val. Can it be
> > somehow de-duplicated?
> 
> The flash_brightness_set callback uses micro-Amps as the unit and the driver
> needs to convert that to its own specific units. Yeah, there would be
> probably an easier way, too. But that'd likely require changes to the LED
> flash class.

Can as3645a_current_to_reg just access struct v4l2_flash_config so
that it does not have to recreate its look-alike on the fly?

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 6/8] leds: as3645a: Add LED flash class driver

[Sakari Ailus]

Ahoy!

On Thu, Jun 15, 2017 at 12:28:33AM +0200, Pavel Machek wrote:
> Hi!
> 
> > Thanks for the review!
> 
> You are welcome :-).
> 
> > On Wed, Jun 14, 2017 at 11:39:41PM +0200, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > From: Sakari Ailus <sakari.ailus@iki.fi>
> > > 
> > > That address no longer works, right?
> > 
> > Why wouldn't it work? Or... do you know something I don't? :-)
> 
> Aha. I thought I was removing it from source files because it was no
> longer working, but maybe I'm misremembering? 

That was probably my @maxwell.research.nokia.com address. :-) There are no
occurrences of that in the kernel source anymore.

> 
> > > > +static unsigned int as3645a_current_to_reg(struct as3645a *flash, bool is_flash,
> > > > +					   unsigned int ua)
> > > > +{
> > > > +	struct {
> > > > +		unsigned int min;
> > > > +		unsigned int max;
> > > > +		unsigned int step;
> > > > +	} __mms[] = {
> > > > +		{
> > > > +			AS_TORCH_INTENSITY_MIN,
> > > > +			flash->cfg.assist_max_ua,
> > > > +			AS_TORCH_INTENSITY_STEP
> > > > +		},
> > > > +		{
> > > > +			AS_FLASH_INTENSITY_MIN,
> > > > +			flash->cfg.flash_max_ua,
> > > > +			AS_FLASH_INTENSITY_STEP
> > > > +		},
> > > > +	}, *mms = &__mms[is_flash];
> > > > +
> > > > +	if (ua < mms->min)
> > > > +		ua = mms->min;
> > > 
> > > That's some... seriously interesting code. And you are forcing gcc to
> > > create quite interesting structure on stack. Would it be easier to do
> > > normal if()... without this magic?
> > > 
> > > > +	struct v4l2_flash_config cfg = {
> > > > +		.torch_intensity = {
> > > > +			.min = AS_TORCH_INTENSITY_MIN,
> > > > +			.max = flash->cfg.assist_max_ua,
> > > > +			.step = AS_TORCH_INTENSITY_STEP,
> > > > +			.val = flash->cfg.assist_max_ua,
> > > > +		},
> > > > +		.indicator_intensity = {
> > > > +			.min = AS_INDICATOR_INTENSITY_MIN,
> > > > +			.max = flash->cfg.indicator_max_ua,
> > > > +			.step = AS_INDICATOR_INTENSITY_STEP,
> > > > +			.val = flash->cfg.indicator_max_ua,
> > > > +		},
> > > > +	};
> > > 
> > > Ugh. And here you have copy of the above struct, + .val. Can it be
> > > somehow de-duplicated?
> > 
> > The flash_brightness_set callback uses micro-Amps as the unit and the driver
> > needs to convert that to its own specific units. Yeah, there would be
> > probably an easier way, too. But that'd likely require changes to the LED
> > flash class.
> 
> Can as3645a_current_to_reg just access struct v4l2_flash_config so
> that it does not have to recreate its look-alike on the fly?

struct v4l2_flash_config is only needed as an argument for
v4l2_flash_init(). I'll split that into two functions in this occasion,
it'll be nicer.

We now have more or less the same conversion implemented in three or so
times, there have to be ways to make that easier for drivers. I think that
could be done later, as well as adding support for checking the flash
strobe status.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 6/8] leds: as3645a: Add LED flash class driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2387 bytes --]

Krasny den!

> > > Thanks for the review!
> > 
> > You are welcome :-).
> > 
> > > On Wed, Jun 14, 2017 at 11:39:41PM +0200, Pavel Machek wrote:
> > > > Hi!
> > > > 
> > > > > From: Sakari Ailus <sakari.ailus@iki.fi>
> > > > 
> > > > That address no longer works, right?
> > > 
> > > Why wouldn't it work? Or... do you know something I don't? :-)
> > 
> > Aha. I thought I was removing it from source files because it was no
> > longer working, but maybe I'm misremembering? 
> 
> That was probably my @maxwell.research.nokia.com address. :-) There are no
> occurrences of that in the kernel source anymore.

I guess it probably was someone else using @iki.fi address, and I am
confused.

> > > > > +	struct v4l2_flash_config cfg = {
> > > > > +		.torch_intensity = {
> > > > > +			.min = AS_TORCH_INTENSITY_MIN,
> > > > > +			.max = flash->cfg.assist_max_ua,
> > > > > +			.step = AS_TORCH_INTENSITY_STEP,
> > > > > +			.val = flash->cfg.assist_max_ua,
> > > > > +		},
> > > > > +		.indicator_intensity = {
> > > > > +			.min = AS_INDICATOR_INTENSITY_MIN,
> > > > > +			.max = flash->cfg.indicator_max_ua,
> > > > > +			.step = AS_INDICATOR_INTENSITY_STEP,
> > > > > +			.val = flash->cfg.indicator_max_ua,
> > > > > +		},
> > > > > +	};
> > > > 
> > > > Ugh. And here you have copy of the above struct, + .val. Can it be
> > > > somehow de-duplicated?
> > > 
> > > The flash_brightness_set callback uses micro-Amps as the unit and the driver
> > > needs to convert that to its own specific units. Yeah, there would be
> > > probably an easier way, too. But that'd likely require changes to the LED
> > > flash class.
> > 
> > Can as3645a_current_to_reg just access struct v4l2_flash_config so
> > that it does not have to recreate its look-alike on the fly?
> 
> struct v4l2_flash_config is only needed as an argument for
> v4l2_flash_init(). I'll split that into two functions in this occasion,
> it'll be nicer.
> 
> We now have more or less the same conversion implemented in three or so
> times, there have to be ways to make that easier for drivers. I think that
> could be done later, as well as adding support for checking the flash
> strobe status.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH 7/8] smiapp: Add support for flash, lens and EEPROM devices

[Sakari Ailus]

These types devices aren't directly related to the sensor, but are
nevertheless handled by the smiapp driver due to the relationship of these
component to the main part of the camera module --- the sensor.

Additionally, for the async sub-device registration to work, the notifier
containing matching fwnodes will need to be registered. This is natural to
perform in a sensor driver as well.

This does not yet address providing the user space with information on how
to associate the sensor, lens or EEPROM devices but the kernel now has the
necessary information to do that.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c | 81 +++++++++++++++++++++++++++++++---
 drivers/media/i2c/smiapp/smiapp.h      |  5 +++
 2 files changed, 79 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index e0b0c03..26f2873 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2551,6 +2551,22 @@ static int smiapp_register_subdev(struct smiapp_sensor *sensor,
 	return 0;
 }
 
+static int smiapp_subdev_notifier_bound(struct v4l2_async_notifier *notifier,
+					struct v4l2_subdev *sd,
+					struct v4l2_async_subdev *asd)
+{
+	return 0;
+}
+
+static int smiapp_subdev_notifier_complete(
+	struct v4l2_async_notifier *notifier)
+{
+	struct smiapp_sensor *sensor =
+		container_of(notifier, struct smiapp_sensor, notifier);
+
+	return v4l2_device_register_subdev_nodes(sensor->src->sd.v4l2_dev);
+}
+
 static void smiapp_unregistered(struct v4l2_subdev *subdev)
 {
 	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
@@ -2558,6 +2574,8 @@ static void smiapp_unregistered(struct v4l2_subdev *subdev)
 
 	for (i = 1; i < sensor->ssds_used; i++)
 		v4l2_device_unregister_subdev(&sensor->ssds[i].sd);
+
+	v4l2_async_subnotifier_unregister(&sensor->notifier);
 }
 
 static int smiapp_registered(struct v4l2_subdev *subdev)
@@ -2581,6 +2599,15 @@ static int smiapp_registered(struct v4l2_subdev *subdev)
 	if (rval)
 		goto out_err;
 
+	if (!sensor->notifier.num_subdevs)
+		return 0;
+
+	sensor->notifier.bound = smiapp_subdev_notifier_bound;
+	sensor->notifier.complete = smiapp_subdev_notifier_complete;
+	rval = v4l2_async_subnotifier_register(subdev, &sensor->notifier);
+	if (rval)
+		goto out_err;
+
 	return 0;
 
 out_err:
@@ -2782,13 +2809,15 @@ static int __maybe_unused smiapp_resume(struct device *dev)
 	return rval;
 }
 
-static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
+static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev,
+						   struct smiapp_sensor *sensor)
 {
+	static const char *props[] = { "flash", "lens", "eeprom" };
 	struct smiapp_hwconfig *hwcfg;
 	struct v4l2_fwnode_endpoint *bus_cfg;
 	struct fwnode_handle *ep;
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
-	int i;
+	unsigned int i;
 	int rval;
 
 	if (!fwnode)
@@ -2849,6 +2878,45 @@ static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
 
 	v4l2_fwnode_endpoint_free(bus_cfg);
 	fwnode_handle_put(ep);
+
+	sensor->notifier.subdevs =
+		devm_kcalloc(dev, SMIAPP_MAX_ASYNC_SUBDEVS,
+			     sizeof(struct v4l2_async_subdev *), GFP_KERNEL);
+	if (!sensor->notifier.subdevs)
+		goto out_err;
+
+	for (i = 0; i < ARRAY_SIZE(props); i++) {
+		struct device_node *node;
+		unsigned int j = 0;
+
+		while ((node = of_parse_phandle(dev->of_node, props[i], j++))) {
+			struct v4l2_async_subdev **asd =
+				 &sensor->notifier.subdevs[
+					 sensor->notifier.num_subdevs];
+
+			if (WARN_ON(sensor->notifier.num_subdevs >=
+				    SMIAPP_MAX_ASYNC_SUBDEVS)) {
+				of_node_put(node);
+				goto out;
+			}
+
+			*asd = devm_kzalloc(
+				dev, sizeof(struct v4l2_async_subdev),
+				GFP_KERNEL);
+			if (!*asd) {
+				of_node_put(node);
+				goto out_err;
+			}
+
+			(*asd)->match.fwnode.fwnode = of_fwnode_handle(node);
+			(*asd)->match_type = V4L2_ASYNC_MATCH_FWNODE;
+			sensor->notifier.num_subdevs++;
+
+			of_node_put(node);
+		}
+	}
+
+out:
 	return hwcfg;
 
 out_err:
@@ -2861,18 +2929,17 @@ static int smiapp_probe(struct i2c_client *client,
 			const struct i2c_device_id *devid)
 {
 	struct smiapp_sensor *sensor;
-	struct smiapp_hwconfig *hwcfg = smiapp_get_hwconfig(&client->dev);
 	unsigned int i;
 	int rval;
 
-	if (hwcfg == NULL)
-		return -ENODEV;
-
 	sensor = devm_kzalloc(&client->dev, sizeof(*sensor), GFP_KERNEL);
 	if (sensor == NULL)
 		return -ENOMEM;
 
-	sensor->hwcfg = hwcfg;
+	sensor->hwcfg = smiapp_get_hwconfig(&client->dev, sensor);
+	if (sensor->hwcfg == NULL)
+		return -ENODEV;
+
 	mutex_init(&sensor->mutex);
 	sensor->src = &sensor->ssds[sensor->ssds_used];
 
diff --git a/drivers/media/i2c/smiapp/smiapp.h b/drivers/media/i2c/smiapp/smiapp.h
index f74d695..21a55de 100644
--- a/drivers/media/i2c/smiapp/smiapp.h
+++ b/drivers/media/i2c/smiapp/smiapp.h
@@ -20,6 +20,7 @@
 #define __SMIAPP_PRIV_H_
 
 #include <linux/mutex.h>
+#include <media/v4l2-async.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-subdev.h>
 #include <media/i2c/smiapp.h>
@@ -143,6 +144,9 @@ struct smiapp_csi_data_format {
 	u8 pixel_order;
 };
 
+/* Lens, EEPROM and a flash LEDs? */
+#define SMIAPP_MAX_ASYNC_SUBDEVS	3
+
 #define SMIAPP_SUBDEVS			3
 
 #define SMIAPP_PA_PAD_SRC		0
@@ -189,6 +193,7 @@ struct smiapp_sensor {
 	struct regulator *vana;
 	struct clk *ext_clk;
 	struct gpio_desc *xshutdown;
+	struct v4l2_async_notifier notifier;
 	u32 limits[SMIAPP_LIMIT_LAST];
 	u8 nbinning_subtypes;
 	struct smiapp_binning_subtype binning_subtypes[SMIAPP_BINNING_SUBTYPES];
-- 
2.1.4

Re: [PATCH 7/8] smiapp: Add support for flash, lens and EEPROM devices

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2964 bytes --]

Hi Sakari,

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on next-20170614]
[cannot apply to robh/for-next v4.12-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sakari-Ailus/Support-registering-lens-flash-and-EEPROM-devices/20170615-084016
base:   git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-x000-201724 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/media/i2c/smiapp/smiapp-core.c: In function 'smiapp_unregistered':
>> drivers/media/i2c/smiapp/smiapp-core.c:2578:2: error: implicit declaration of function 'v4l2_async_subnotifier_unregister' [-Werror=implicit-function-declaration]
     v4l2_async_subnotifier_unregister(&sensor->notifier);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/media/i2c/smiapp/smiapp-core.c: In function 'smiapp_registered':
>> drivers/media/i2c/smiapp/smiapp-core.c:2607:9: error: implicit declaration of function 'v4l2_async_subnotifier_register' [-Werror=implicit-function-declaration]
     rval = v4l2_async_subnotifier_register(subdev, &sensor->notifier);
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/v4l2_async_subnotifier_unregister +2578 drivers/media/i2c/smiapp/smiapp-core.c

  2572		struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
  2573		unsigned int i;
  2574	
  2575		for (i = 1; i < sensor->ssds_used; i++)
  2576			v4l2_device_unregister_subdev(&sensor->ssds[i].sd);
  2577	
> 2578		v4l2_async_subnotifier_unregister(&sensor->notifier);
  2579	}
  2580	
  2581	static int smiapp_registered(struct v4l2_subdev *subdev)
  2582	{
  2583		struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
  2584		int rval;
  2585	
  2586		if (sensor->scaler) {
  2587			rval = smiapp_register_subdev(
  2588				sensor, sensor->binner, sensor->scaler,
  2589				SMIAPP_PAD_SRC, SMIAPP_PAD_SINK,
  2590				MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
  2591			if (rval < 0)
  2592				return rval;
  2593		}
  2594	
  2595		rval = smiapp_register_subdev(
  2596			sensor, sensor->pixel_array, sensor->binner,
  2597			SMIAPP_PA_PAD_SRC, SMIAPP_PAD_SINK,
  2598			MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
  2599		if (rval)
  2600			goto out_err;
  2601	
  2602		if (!sensor->notifier.num_subdevs)
  2603			return 0;
  2604	
  2605		sensor->notifier.bound = smiapp_subdev_notifier_bound;
  2606		sensor->notifier.complete = smiapp_subdev_notifier_complete;
> 2607		rval = v4l2_async_subnotifier_register(subdev, &sensor->notifier);
  2608		if (rval)
  2609			goto out_err;
  2610	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25120 bytes --]

Re: [PATCH 7/8] smiapp: Add support for flash, lens and EEPROM devices

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 4048 bytes --]

Hi!

> These types devices aren't directly related to the sensor, but are
> nevertheless handled by the smiapp driver due to the relationship of these
> component to the main part of the camera module --- the sensor.
> 
> Additionally, for the async sub-device registration to work, the notifier
> containing matching fwnodes will need to be registered. This is natural to
> perform in a sensor driver as well.
> 
> This does not yet address providing the user space with information on how
> to associate the sensor, lens or EEPROM devices but the kernel now has the
> necessary information to do that.

Let me see... I guess this is going to be quite interesting for me,
too, because I'll be able to remove similar code in omap3 isp driver.

I'm getting same error as the build bot... which is expected as you
did mention it depends on some other series.

(I'll take a look if I can test it easily.)

Acked-by: Pavel Machek <pavel@ucw.cz>


> @@ -2849,6 +2878,45 @@ static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
>  
>  	v4l2_fwnode_endpoint_free(bus_cfg);
>  	fwnode_handle_put(ep);
> +
> +	sensor->notifier.subdevs =
> +		devm_kcalloc(dev, SMIAPP_MAX_ASYNC_SUBDEVS,
> +			     sizeof(struct v4l2_async_subdev *), GFP_KERNEL);
> +	if (!sensor->notifier.subdevs)
> +		goto out_err;
> +
> +	for (i = 0; i < ARRAY_SIZE(props); i++) {
> +		struct device_node *node;
> +		unsigned int j = 0;
> +
> +		while ((node = of_parse_phandle(dev->of_node, props[i], j++))) {
> +			struct v4l2_async_subdev **asd =
> +				 &sensor->notifier.subdevs[
> +					 sensor->notifier.num_subdevs];
> +
> +			if (WARN_ON(sensor->notifier.num_subdevs >=
> +				    SMIAPP_MAX_ASYNC_SUBDEVS)) {
> +				of_node_put(node);
> +				goto out;
> +			}
> +
> +			*asd = devm_kzalloc(
> +				dev, sizeof(struct v4l2_async_subdev),
> +				GFP_KERNEL);
> +			if (!*asd) {
> +				of_node_put(node);
> +				goto out_err;
> +			}
> +
> +			(*asd)->match.fwnode.fwnode = of_fwnode_handle(node);
> +			(*asd)->match_type = V4L2_ASYNC_MATCH_FWNODE;
> +			sensor->notifier.num_subdevs++;
> +
> +			of_node_put(node);
> +		}
> +	}
> +
> +out:
>  	return hwcfg;
>  
>  out_err:
> @@ -2861,18 +2929,17 @@ static int smiapp_probe(struct i2c_client *client,
>  			const struct i2c_device_id *devid)
>  {
>  	struct smiapp_sensor *sensor;
> -	struct smiapp_hwconfig *hwcfg = smiapp_get_hwconfig(&client->dev);
>  	unsigned int i;
>  	int rval;
>  
> -	if (hwcfg == NULL)
> -		return -ENODEV;
> -
>  	sensor = devm_kzalloc(&client->dev, sizeof(*sensor), GFP_KERNEL);
>  	if (sensor == NULL)
>  		return -ENOMEM;
>  
> -	sensor->hwcfg = hwcfg;
> +	sensor->hwcfg = smiapp_get_hwconfig(&client->dev, sensor);
> +	if (sensor->hwcfg == NULL)
> +		return -ENODEV;
> +
>  	mutex_init(&sensor->mutex);
>  	sensor->src = &sensor->ssds[sensor->ssds_used];
>  
> diff --git a/drivers/media/i2c/smiapp/smiapp.h b/drivers/media/i2c/smiapp/smiapp.h
> index f74d695..21a55de 100644
> --- a/drivers/media/i2c/smiapp/smiapp.h
> +++ b/drivers/media/i2c/smiapp/smiapp.h
> @@ -20,6 +20,7 @@
>  #define __SMIAPP_PRIV_H_
>  
>  #include <linux/mutex.h>
> +#include <media/v4l2-async.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-subdev.h>
>  #include <media/i2c/smiapp.h>
> @@ -143,6 +144,9 @@ struct smiapp_csi_data_format {
>  	u8 pixel_order;
>  };
>  
> +/* Lens, EEPROM and a flash LEDs? */
> +#define SMIAPP_MAX_ASYNC_SUBDEVS	3
> +
>  #define SMIAPP_SUBDEVS			3
>  
>  #define SMIAPP_PA_PAD_SRC		0
> @@ -189,6 +193,7 @@ struct smiapp_sensor {
>  	struct regulator *vana;
>  	struct clk *ext_clk;
>  	struct gpio_desc *xshutdown;
> +	struct v4l2_async_notifier notifier;
>  	u32 limits[SMIAPP_LIMIT_LAST];
>  	u8 nbinning_subtypes;
>  	struct smiapp_binning_subtype binning_subtypes[SMIAPP_BINNING_SUBTYPES];

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 7/8] smiapp: Add support for flash, lens and EEPROM devices

[Sakari Ailus]

Hi Pavel,

On Fri, Jun 16, 2017 at 02:07:13PM +0200, Pavel Machek wrote:
> Hi!
> 
> > These types devices aren't directly related to the sensor, but are
> > nevertheless handled by the smiapp driver due to the relationship of these
> > component to the main part of the camera module --- the sensor.
> > 
> > Additionally, for the async sub-device registration to work, the notifier
> > containing matching fwnodes will need to be registered. This is natural to
> > perform in a sensor driver as well.
> > 
> > This does not yet address providing the user space with information on how
> > to associate the sensor, lens or EEPROM devices but the kernel now has the
> > necessary information to do that.
> 
> Let me see... I guess this is going to be quite interesting for me,
> too, because I'll be able to remove similar code in omap3 isp driver.

Yes, indeed. And with this, we have the lens - sensor association
information as a bonus.

I'll drop EEPROM support in v2, I guess you wouldn't have needed it? I guess
we'll need to see examples that can be found in the wild. My current
understanding is that EEPROM could be a separate chip in the module as well
as integrated to the sensor.

SMIA++ supports EEPROM as well (it's accessible through the sensor) but not
all (more or less) compliant sensors implement it (instead it's a separate
I²C device).

> 
> I'm getting same error as the build bot... which is expected as you
> did mention it depends on some other series.

Yes, I missed half of the change. I'm pushing it again now... this will take
time until my server has SSDs.

> 
> (I'll take a look if I can test it easily.)
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>

Thanks!

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 7/8] smiapp: Add support for flash, lens and EEPROM devices

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1422 bytes --]

Hi!

> > > These types devices aren't directly related to the sensor, but are
> > > nevertheless handled by the smiapp driver due to the relationship of these
> > > component to the main part of the camera module --- the sensor.
> > > 
> > > Additionally, for the async sub-device registration to work, the notifier
> > > containing matching fwnodes will need to be registered. This is natural to
> > > perform in a sensor driver as well.
> > > 
> > > This does not yet address providing the user space with information on how
> > > to associate the sensor, lens or EEPROM devices but the kernel now has the
> > > necessary information to do that.
> > 
> > Let me see... I guess this is going to be quite interesting for me,
> > too, because I'll be able to remove similar code in omap3 isp driver.
> 
> Yes, indeed. And with this, we have the lens - sensor association
> information as a bonus.
> 
> I'll drop EEPROM support in v2, I guess you wouldn't have needed it? I guess
> we'll need to see examples that can be found in the wild. My current
> understanding is that EEPROM could be a separate chip in the module as well
> as integrated to the sensor.

I don't think I need EEPROM, no. (But I did not check the datasheets).

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 7/8] smiapp: Add support for flash, lens and EEPROM devices

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2864 bytes --]

Hi!

> These types devices aren't directly related to the sensor, but are
> nevertheless handled by the smiapp driver due to the relationship of these
> component to the main part of the camera module --- the sensor.
> 
> Additionally, for the async sub-device registration to work, the notifier
> containing matching fwnodes will need to be registered. This is natural to
> perform in a sensor driver as well.
> 
> This does not yet address providing the user space with information on how
> to associate the sensor, lens or EEPROM devices but the kernel now has the
> necessary information to do that.

Do I understand it correctly that basically every sensor driver (in my
case et8ek8) needs to get this kind of support? I2c leds are cheap,
and may be asociated with pretty much any sensor, AFAICT.

This is quite a lot of boilerplate for that. Would it make sense to
provide helper function at least for this?

Thanks,
								Pavel

> -static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
> +static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev,
> +						   struct smiapp_sensor *sensor)
>  {
> +	static const char *props[] = { "flash", "lens", "eeprom" };
>  	struct smiapp_hwconfig *hwcfg;
>  	struct v4l2_fwnode_endpoint *bus_cfg;
>  	struct fwnode_handle *ep;
>  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> -	int i;
> +	unsigned int i;
>  	int rval;
>  
>  	if (!fwnode)
> @@ -2849,6 +2878,45 @@ static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
>  
>  	v4l2_fwnode_endpoint_free(bus_cfg);
>  	fwnode_handle_put(ep);
> +
> +	sensor->notifier.subdevs =
> +		devm_kcalloc(dev, SMIAPP_MAX_ASYNC_SUBDEVS,
> +			     sizeof(struct v4l2_async_subdev *), GFP_KERNEL);
> +	if (!sensor->notifier.subdevs)
> +		goto out_err;
> +
> +	for (i = 0; i < ARRAY_SIZE(props); i++) {
> +		struct device_node *node;
> +		unsigned int j = 0;
> +
> +		while ((node = of_parse_phandle(dev->of_node, props[i], j++))) {
> +			struct v4l2_async_subdev **asd =
> +				 &sensor->notifier.subdevs[
> +					 sensor->notifier.num_subdevs];
> +
> +			if (WARN_ON(sensor->notifier.num_subdevs >=
> +				    SMIAPP_MAX_ASYNC_SUBDEVS)) {
> +				of_node_put(node);
> +				goto out;
> +			}
> +
> +			*asd = devm_kzalloc(
> +				dev, sizeof(struct v4l2_async_subdev),
> +				GFP_KERNEL);
> +			if (!*asd) {
> +				of_node_put(node);
> +				goto out_err;
> +			}
> +
> +			(*asd)->match.fwnode.fwnode = of_fwnode_handle(node);
> +			(*asd)->match_type = V4L2_ASYNC_MATCH_FWNODE;
> +			sensor->notifier.num_subdevs++;
> +
> +			of_node_put(node);
> +		}
> +	}
> +
> +out:
>  	return hwcfg;
>  
>  out_err:

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 7/8] smiapp: Add support for flash, lens and EEPROM devices

[Sakari Ailus]

Hi Pavel,

On Fri, Jun 16, 2017 at 02:42:42PM +0200, Pavel Machek wrote:
> Hi!
> 
> > These types devices aren't directly related to the sensor, but are
> > nevertheless handled by the smiapp driver due to the relationship of these
> > component to the main part of the camera module --- the sensor.
> > 
> > Additionally, for the async sub-device registration to work, the notifier
> > containing matching fwnodes will need to be registered. This is natural to
> > perform in a sensor driver as well.
> > 
> > This does not yet address providing the user space with information on how
> > to associate the sensor, lens or EEPROM devices but the kernel now has the
> > necessary information to do that.
> 
> Do I understand it correctly that basically every sensor driver (in my
> case et8ek8) needs to get this kind of support? I2c leds are cheap,
> and may be asociated with pretty much any sensor, AFAICT.

That's right.

> 
> This is quite a lot of boilerplate for that. Would it make sense to
> provide helper function at least for this?

Yes. I've been thinking of having helper functions for notifiers and
sub-notifiers. Most of the receiver drivers are implementing exactly the
same thing but with different twists (read: bugs).

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 7/8] smiapp: Add support for flash, lens and EEPROM devices

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1713 bytes --]

Hi!

> > > These types devices aren't directly related to the sensor, but are
> > > nevertheless handled by the smiapp driver due to the relationship of these
> > > component to the main part of the camera module --- the sensor.
> > > 
> > > Additionally, for the async sub-device registration to work, the notifier
> > > containing matching fwnodes will need to be registered. This is natural to
> > > perform in a sensor driver as well.
> > > 
> > > This does not yet address providing the user space with information on how
> > > to associate the sensor, lens or EEPROM devices but the kernel now has the
> > > necessary information to do that.
> > 
> > Do I understand it correctly that basically every sensor driver (in my
> > case et8ek8) needs to get this kind of support? I2c leds are cheap,
> > and may be asociated with pretty much any sensor, AFAICT.
> 
> That's right.
> 
> > 
> > This is quite a lot of boilerplate for that. Would it make sense to
> > provide helper function at least for this?
> 
> Yes. I've been thinking of having helper functions for notifiers and
> sub-notifiers. Most of the receiver drivers are implementing exactly the
> same thing but with different twists (read: bugs).

Agreed, helpers would be nice. Ping me if you have them, I'll happily
test it with et8ek8. (Or I can try to create them, but...)

If we move lens/flash to the sensor, this one can probably be dropped:

https://git.linuxtv.org/sailus/media_tree.git/commit/?h=ccp2&id=1796bbce05964f86cf546557a96626b2bdebe65b



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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 7/8] smiapp: Add support for flash, lens and EEPROM devices

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 4964 bytes --]

Hi!

> > This is quite a lot of boilerplate for that. Would it make sense to
> > provide helper function at least for this?
> 
> Yes. I've been thinking of having helper functions for notifiers and
> sub-notifiers. Most of the receiver drivers are implementing exactly the
> same thing but with different twists (read: bugs).

Perhaps something like this is a starting point?

									Pavel

commit 22d722b7ee7cb437ae2013fa83a215f91151621c
Author: Pavel <pavel@ucw.cz>
Date:   Sat Jun 17 14:14:08 2017 +0200

    Subdev support for et8ek8.
    
    Subdev code is ready for moving to some other place -- this is really
    common code.
    
    Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c
index 05b9bd9..a6899fc 100644
--- a/drivers/media/i2c/et8ek8/et8ek8_driver.c
+++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c
@@ -32,13 +32,90 @@
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/sort.h>
-#include <linux/v4l2-mediabus.h>
 
+#include <media/v4l2-async.h>
+#include <linux/v4l2-mediabus.h>
 #include <media/media-entity.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-subdev.h>
 
+static int simple_subdev_notifier_bound(struct v4l2_async_notifier *notifier,
+                                       struct v4l2_subdev *sd,
+                                       struct v4l2_async_subdev *asd)
+{
+       return 0;
+}
+
+static int simple_subdev_notifier_complete(
+       struct v4l2_async_notifier *notifier)
+{
+       struct v4l2_async_notifier_simple *notifier_s =
+               container_of(notifier, struct v4l2_async_notifier_simple, notifier);
+
+       return v4l2_device_register_subdev_nodes(notifier_s->v4l2_dev);
+}
+
+static inline int v4l2_simple_subnotifier_register(struct v4l2_subdev *subdev,
+				     struct v4l2_async_notifier_simple *notifier_s)
+{
+	struct v4l2_async_notifier *notifier = &notifier_s->notifier;
+	int rval;
+	if (!notifier->num_subdevs)
+		return 0;
+
+	notifier_s->v4l2_dev = subdev->v4l2_dev;
+	notifier->bound = simple_subdev_notifier_bound;
+	notifier->complete = simple_subdev_notifier_complete;
+	rval = v4l2_async_subnotifier_register(subdev, notifier);
+	return rval;
+}
+
+static inline int simple_subdev_probe(struct device *dev,
+				      struct v4l2_async_notifier *notifier)
+{
+	static const char *props[] = { "flash", "lens", "eeprom" };
+	unsigned int i;
+	const int max_subdevs = 3;
+	
+	notifier->subdevs =
+		devm_kcalloc(dev, max_subdevs,
+			     sizeof(struct v4l2_async_subdev *), GFP_KERNEL);
+	if (!notifier->subdevs)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(props); i++) {
+		struct device_node *node;
+		unsigned int j = 0;
+
+		while ((node = of_parse_phandle(dev->of_node, props[i], j++))) {
+			struct v4l2_async_subdev **asd =
+                                &notifier->subdevs[
+                                        notifier->num_subdevs];
+
+			if (WARN_ON(notifier->num_subdevs >= max_subdevs)) {
+				of_node_put(node);
+				return 0;
+			}
+
+			*asd = devm_kzalloc(
+				dev, sizeof(struct v4l2_async_subdev),
+				GFP_KERNEL);
+			if (!*asd) {
+				of_node_put(node);
+				return 0;
+			}
+
+			(*asd)->match.fwnode.fwnode = of_fwnode_handle(node);
+			(*asd)->match_type = V4L2_ASYNC_MATCH_FWNODE;
+			notifier->num_subdevs++;
+
+			of_node_put(node);
+		}
+	}
+	return 0;
+}
+
 #include "et8ek8_reg.h"
 
 #define ET8EK8_NAME		"et8ek8"
@@ -67,6 +144,8 @@ struct et8ek8_sensor {
 
 	struct mutex power_lock;
 	int power_count;
+
+	struct v4l2_async_notifier_simple notifier_s;
 };
 
 #define to_et8ek8_sensor(sd)	container_of(sd, struct et8ek8_sensor, subdev)
@@ -1510,6 +1589,8 @@ et8ek8_registered(struct v4l2_subdev *subdev)
 		goto err_file;
 	}
 
+	v4l2_simple_subnotifier_register(subdev, &sensor->notifier_s);
+	
 	__et8ek8_get_pad_format(sensor, NULL, 0, V4L2_SUBDEV_FORMAT_ACTIVE);
 
 	return 0;
@@ -1680,6 +1761,8 @@ static int et8ek8_probe(struct i2c_client *client,
 	if (ret < 0)
 		goto err_entity;
 
+	simple_subdev_probe(dev, &sensor->notifier_s.notifier);
+
 	dev_dbg(dev, "initialized!\n");
 
 	return 0;
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index f7e2a1a..63bcf80 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -104,6 +104,11 @@ struct v4l2_async_notifier {
 		       struct v4l2_async_subdev *asd);
 };
 
+struct v4l2_async_notifier_simple {
+	struct v4l2_async_notifier notifier;
+	struct v4l2_device * v4l2_dev;
+};
+
 /**
  * v4l2_async_notifier_register - registers a subdevice asynchronous subnotifier
  *


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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 7/8] smiapp: Add support for flash, lens and EEPROM devices

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 5514 bytes --]

Hi!

> > > This is quite a lot of boilerplate for that. Would it make sense to
> > > provide helper function at least for this?
> > 
> > Yes. I've been thinking of having helper functions for notifiers and
> > sub-notifiers. Most of the receiver drivers are implementing exactly the
> > same thing but with different twists (read: bugs).
> 
> Perhaps something like this is a starting point?

And here's tested version that actually works for me.

Wants moving to common code, perhaps renaming functions.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

Best regards,
									Pavel

diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c
index 05b9bd9..4674939 100644
--- a/drivers/media/i2c/et8ek8/et8ek8_driver.c
+++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c
@@ -32,13 +32,92 @@
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/sort.h>
-#include <linux/v4l2-mediabus.h>
 
+#include <media/v4l2-async.h>
+#include <linux/v4l2-mediabus.h>
 #include <media/media-entity.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-subdev.h>
 
+static int simple_subdev_notifier_bound(struct v4l2_async_notifier *notifier,
+                                       struct v4l2_subdev *sd,
+                                       struct v4l2_async_subdev *asd)
+{
+       return 0;
+}
+
+static int simple_subdev_notifier_complete(
+       struct v4l2_async_notifier *notifier)
+{
+       struct v4l2_async_notifier_simple *notifier_s =
+               container_of(notifier, struct v4l2_async_notifier_simple, notifier);
+
+       return v4l2_device_register_subdev_nodes(notifier_s->v4l2_dev);
+}
+
+static inline int v4l2_simple_subnotifier_register(struct v4l2_subdev *subdev,
+				     struct v4l2_async_notifier_simple *notifier_s)
+{
+	struct v4l2_async_notifier *notifier = &notifier_s->notifier;
+	int rval;
+
+	if (!notifier->num_subdevs) {
+		return 0;
+	}
+
+	notifier_s->v4l2_dev = subdev->v4l2_dev;
+	notifier->bound = simple_subdev_notifier_bound;
+	notifier->complete = simple_subdev_notifier_complete;
+	rval = v4l2_async_subnotifier_register(subdev, notifier);
+	return rval;
+}
+
+static inline int simple_subdev_probe(struct device *dev,
+				      struct v4l2_async_notifier *notifier)
+{
+	static const char *props[] = { "flash", "lens" };
+	unsigned int i;
+	const int max_subdevs = 3;
+	
+	notifier->subdevs =
+		devm_kcalloc(dev, max_subdevs,
+			     sizeof(struct v4l2_async_subdev *), GFP_KERNEL);
+	if (!notifier->subdevs)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(props); i++) {
+		struct device_node *node;
+		unsigned int j = 0;
+
+		while ((node = of_parse_phandle(dev->of_node, props[i], j++))) {
+			struct v4l2_async_subdev **asd =
+                                &notifier->subdevs[
+                                        notifier->num_subdevs];
+
+			if (WARN_ON(notifier->num_subdevs >= max_subdevs)) {
+				of_node_put(node);
+				return 0;
+			}
+
+			*asd = devm_kzalloc(
+				dev, sizeof(struct v4l2_async_subdev),
+				GFP_KERNEL);
+			if (!*asd) {
+				of_node_put(node);
+				return 0;
+			}
+
+			(*asd)->match.fwnode.fwnode = of_fwnode_handle(node);
+			(*asd)->match_type = V4L2_ASYNC_MATCH_FWNODE;
+			notifier->num_subdevs++;
+
+			of_node_put(node);
+		}
+	}
+	return 0;
+}
+
 #include "et8ek8_reg.h"
 
 #define ET8EK8_NAME		"et8ek8"
@@ -67,6 +146,8 @@ struct et8ek8_sensor {
 
 	struct mutex power_lock;
 	int power_count;
+
+	struct v4l2_async_notifier_simple notifier_s;
 };
 
 #define to_et8ek8_sensor(sd)	container_of(sd, struct et8ek8_sensor, subdev)
@@ -1509,8 +1590,9 @@ et8ek8_registered(struct v4l2_subdev *subdev)
 		dev_err(&client->dev, "controls initialization failed\n");
 		goto err_file;
 	}
-
+	
 	__et8ek8_get_pad_format(sensor, NULL, 0, V4L2_SUBDEV_FORMAT_ACTIVE);
+	v4l2_simple_subnotifier_register(subdev, &sensor->notifier_s);
 
 	return 0;
 
@@ -1663,7 +1745,13 @@ static int et8ek8_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	sensor->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+
 	mutex_init(&sensor->power_lock);
+	
+	ret = simple_subdev_probe(dev, &sensor->notifier_s.notifier);
+	if (ret < 0)
+		printk("Simple subdev probe failed\n");
 
 	v4l2_i2c_subdev_init(&sensor->subdev, client, &et8ek8_ops);
 	sensor->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
@@ -1703,6 +1791,7 @@ static int __exit et8ek8_remove(struct i2c_client *client)
 	}
 
 	v4l2_device_unregister_subdev(&sensor->subdev);
+	/* FIXME: v4l2_async_subnotifier_unregister(&sensor->notifier_s.notifier); */
 	device_remove_file(&client->dev, &dev_attr_priv_mem);
 	v4l2_ctrl_handler_free(&sensor->ctrl_handler);
 	v4l2_async_unregister_subdev(&sensor->subdev);
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index f7e2a1a..63bcf80 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -104,6 +104,11 @@ struct v4l2_async_notifier {
 		       struct v4l2_async_subdev *asd);
 };
 
+struct v4l2_async_notifier_simple {
+	struct v4l2_async_notifier notifier;
+	struct v4l2_device * v4l2_dev;
+};
+
 /**
  * v4l2_async_notifier_register - registers a subdevice asynchronous subnotifier
  *

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 7/8] smiapp: Add support for flash, lens and EEPROM devices

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1205 bytes --]

Hi!

> These types devices aren't directly related to the sensor, but are
> nevertheless handled by the smiapp driver due to the relationship of these
> component to the main part of the camera module --- the sensor.
> 
> Additionally, for the async sub-device registration to work, the notifier
> containing matching fwnodes will need to be registered. This is natural to
> perform in a sensor driver as well.
> 
> This does not yet address providing the user space with information on how
> to associate the sensor, lens or EEPROM devices but the kernel now has the
> necessary information to do that.

> -static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
> +static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev,
> +						   struct smiapp_sensor *sensor)
>  {
> +	static const char *props[] = { "flash", "lens", "eeprom" };
>  	struct smiapp_hwconfig *hwcfg;

Binding says "lens-focus" but this uses just "lens". I prefer
lens-focus, because we may also have lens-aperture...

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH 8/8] arm: dts: omap3: N9/N950: Add AS3645A camera flash

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@iki.fi>

Add the as3645a flash controller to the DT source as well as the flash
property with the as3645a device phandle to the sensor DT node.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 arch/arm/boot/dts/omap3-n9.dts       |  1 +
 arch/arm/boot/dts/omap3-n950-n9.dtsi | 14 ++++++++++++++
 arch/arm/boot/dts/omap3-n950.dts     |  1 +
 3 files changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/omap3-n9.dts b/arch/arm/boot/dts/omap3-n9.dts
index b9e58c5..f95e7b1 100644
--- a/arch/arm/boot/dts/omap3-n9.dts
+++ b/arch/arm/boot/dts/omap3-n9.dts
@@ -26,6 +26,7 @@
 		clocks = <&isp 0>;
 		clock-frequency = <9600000>;
 		nokia,nvm-size = <(16 * 64)>;
+		flash = <&as3645a>;
 		port {
 			smia_1_1: endpoint {
 				link-frequencies = /bits/ 64 <199200000 210000000 499200000>;
diff --git a/arch/arm/boot/dts/omap3-n950-n9.dtsi b/arch/arm/boot/dts/omap3-n950-n9.dtsi
index df3366f..8bd6673 100644
--- a/arch/arm/boot/dts/omap3-n950-n9.dtsi
+++ b/arch/arm/boot/dts/omap3-n950-n9.dtsi
@@ -265,6 +265,20 @@
 
 &i2c2 {
 	clock-frequency = <400000>;
+
+	as3645a: flash@30 {
+		reg = <0x30>;
+		compatible = "ams,as3645a";
+		flash {
+			flash-timeout-us = <150000>;
+			flash-max-microamp = <320000>;
+			led-max-microamp = <60000>;
+			peak-current-limit = <1750000>;
+		};
+		indicator {
+			led-max-microamp = <10000>;
+		};
+	};
 };
 
 &i2c3 {
diff --git a/arch/arm/boot/dts/omap3-n950.dts b/arch/arm/boot/dts/omap3-n950.dts
index 646601a..8fca038 100644
--- a/arch/arm/boot/dts/omap3-n950.dts
+++ b/arch/arm/boot/dts/omap3-n950.dts
@@ -60,6 +60,7 @@
 		clocks = <&isp 0>;
 		clock-frequency = <9600000>;
 		nokia,nvm-size = <(16 * 64)>;
+		flash = <&as3645a>;
 		port {
 			smia_1_1: endpoint {
 				link-frequencies = /bits/ 64 <210000000 333600000 398400000>;
-- 
2.1.4

Re: [PATCH 8/8] arm: dts: omap3: N9/N950: Add AS3645A camera flash

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2181 bytes --]

Hi,

On Wed, Jun 14, 2017 at 12:47:19PM +0300, Sakari Ailus wrote:
> From: Sakari Ailus <sakari.ailus@iki.fi>
> 
> Add the as3645a flash controller to the DT source as well as the flash
> property with the as3645a device phandle to the sensor DT node.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

-- Sebastian

> ---
>  arch/arm/boot/dts/omap3-n9.dts       |  1 +
>  arch/arm/boot/dts/omap3-n950-n9.dtsi | 14 ++++++++++++++
>  arch/arm/boot/dts/omap3-n950.dts     |  1 +
>  3 files changed, 16 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/omap3-n9.dts b/arch/arm/boot/dts/omap3-n9.dts
> index b9e58c5..f95e7b1 100644
> --- a/arch/arm/boot/dts/omap3-n9.dts
> +++ b/arch/arm/boot/dts/omap3-n9.dts
> @@ -26,6 +26,7 @@
>  		clocks = <&isp 0>;
>  		clock-frequency = <9600000>;
>  		nokia,nvm-size = <(16 * 64)>;
> +		flash = <&as3645a>;
>  		port {
>  			smia_1_1: endpoint {
>  				link-frequencies = /bits/ 64 <199200000 210000000 499200000>;
> diff --git a/arch/arm/boot/dts/omap3-n950-n9.dtsi b/arch/arm/boot/dts/omap3-n950-n9.dtsi
> index df3366f..8bd6673 100644
> --- a/arch/arm/boot/dts/omap3-n950-n9.dtsi
> +++ b/arch/arm/boot/dts/omap3-n950-n9.dtsi
> @@ -265,6 +265,20 @@
>  
>  &i2c2 {
>  	clock-frequency = <400000>;
> +
> +	as3645a: flash@30 {
> +		reg = <0x30>;
> +		compatible = "ams,as3645a";
> +		flash {
> +			flash-timeout-us = <150000>;
> +			flash-max-microamp = <320000>;
> +			led-max-microamp = <60000>;
> +			peak-current-limit = <1750000>;
> +		};
> +		indicator {
> +			led-max-microamp = <10000>;
> +		};
> +	};
>  };
>  
>  &i2c3 {
> diff --git a/arch/arm/boot/dts/omap3-n950.dts b/arch/arm/boot/dts/omap3-n950.dts
> index 646601a..8fca038 100644
> --- a/arch/arm/boot/dts/omap3-n950.dts
> +++ b/arch/arm/boot/dts/omap3-n950.dts
> @@ -60,6 +60,7 @@
>  		clocks = <&isp 0>;
>  		clock-frequency = <9600000>;
>  		nokia,nvm-size = <(16 * 64)>;
> +		flash = <&as3645a>;
>  		port {
>  			smia_1_1: endpoint {
>  				link-frequencies = /bits/ 64 <210000000 333600000 398400000>;
> -- 
> 2.1.4
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/8] Support registering lens, flash and EEPROM devices

[Sakari Ailus]

On Wed, Jun 14, 2017 at 12:47:11PM +0300, Sakari Ailus wrote:
> Hi folks,
> 
> This set adds support for async registering of lens, flash and EEPROM
> devices, as well as support for this in the smiapp driver and a LED driver
> for the as3645a.
> 
> The lens and flash devices are entities in the media graph whereas the
> EEPROM is at least currently not. By providing the association information
> it is possible to add the flash device to the media graph.

I forgot to add that this set depends on another set Niklas recently posted:

<URL:http://www.spinics.net/lists/linux-media/msg116906.html>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk