Re: [PATCH v2.1 2/3] leds: as3645a: Add LED flash class driver

[Sakari Ailus]

Hi Jacek,

Jacek Anaszewski wrote:
> Hi Sakari,
> 
> On 08/21/2017 03:53 PM, Sakari Ailus wrote:
>> Hi Jacek,
>>
>> Jacek Anaszewski wrote:
>>> Hi Sakari,
>>>
>>> Thanks for the update.
>>> I've noticed that you added node labels to the child device nodes
>>> in [0]:
>>>
>>> "as3645a_flash : flash" and "as3645a_indicator : indicator"
>>
>> The phandle references (as3645a_flash and as3645a_indicator) should
>> actually be moved to the patch adding the flash property to the sensor
>> device node. It doesn't do anything here, yet.
>>
>>>
>>> I am still seeing problems with this approach:
>>>
>>> 1) AFAIK these labels are only used for referencing nodes inside dts
>>>files and they don't affect the name property of struct device_node
>>
>> That's right.
>>
>>> 2) Even if you changed the node name from flash to as3645a_flash, you
>>>would get weird LED class device name "as3645a_flash:flash" in case
>>>label property is absent. Do you have any objections against the
>>>approach I proposed in the previous review?:
>>>
>>>
>>> snprintf(names->flash, sizeof(names->flash),
>>>  AS_NAME":%s", node->name);
>>
>> In the current patch, the device node of the flash controller is used,
>> postfixed with colon and the name of the LED ("flash" or "indicator") if
>> no label is defined. In other words, with that DT source you'll have
>> "as3645a:flash" and "as3645a:indicator". So if you change the name of
>> the device node of the I²C device, that will be reflected in the label.
>>
>> If a label exists, then the label is used as such.
>>
>> I don't really have objections to what you're proposing as such but my
>> question is: is it useful? With that, the flash and indicator labels
>> will not come from DT if label properties are undefined. They'll always
>> be "as3645a:flash" and "as3645a:indicator", independently of the names
>> of the device nodes.
>>
> 
> Ah, indeed, the node->name is put in place of devicename segment and
> the node points to the LED controller node. Neat approach, likely to
> be adopted as a pattern from now on for all new LED class drivers.
> 
> 
> For the patch going through media tree:
> 
> Acked-by: Jacek Anaszewski 

Thanks!

I sent a new version of the DT source patch for N9 / N950; I'll proceed
to send a pull request tomorrow / the day after unless there are further
comments.

-- 
Regards,

Sakari Ailus
sakari.ai...@iki.fi

Re: [PATCH v2.1 2/3] leds: as3645a: Add LED flash class driver

[Jacek Anaszewski]

Hi Sakari,

On 08/21/2017 03:53 PM, Sakari Ailus wrote:
> Hi Jacek,
> 
> Jacek Anaszewski wrote:
>> Hi Sakari,
>>
>> Thanks for the update.
>> I've noticed that you added node labels to the child device nodes
>> in [0]:
>>
>> "as3645a_flash : flash" and "as3645a_indicator : indicator"
> 
> The phandle references (as3645a_flash and as3645a_indicator) should
> actually be moved to the patch adding the flash property to the sensor
> device node. It doesn't do anything here, yet.
> 
>>
>> I am still seeing problems with this approach:
>>
>> 1) AFAIK these labels are only used for referencing nodes inside dts
>>files and they don't affect the name property of struct device_node
> 
> That's right.
> 
>> 2) Even if you changed the node name from flash to as3645a_flash, you
>>would get weird LED class device name "as3645a_flash:flash" in case
>>label property is absent. Do you have any objections against the
>>approach I proposed in the previous review?:
>>
>>
>> snprintf(names->flash, sizeof(names->flash),
>>   AS_NAME":%s", node->name);
> 
> In the current patch, the device node of the flash controller is used,
> postfixed with colon and the name of the LED ("flash" or "indicator") if
> no label is defined. In other words, with that DT source you'll have
> "as3645a:flash" and "as3645a:indicator". So if you change the name of
> the device node of the I²C device, that will be reflected in the label.
> 
> If a label exists, then the label is used as such.
> 
> I don't really have objections to what you're proposing as such but my
> question is: is it useful? With that, the flash and indicator labels
> will not come from DT if label properties are undefined. They'll always
> be "as3645a:flash" and "as3645a:indicator", independently of the names
> of the device nodes.
> 

Ah, indeed, the node->name is put in place of devicename segment and
the node points to the LED controller node. Neat approach, likely to
be adopted as a pattern from now on for all new LED class drivers.


For the patch going through media tree:

Acked-by: Jacek Anaszewski 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2.1 2/3] leds: as3645a: Add LED flash class driver

[Sakari Ailus]

Hi Jacek,

Jacek Anaszewski wrote:
> Hi Sakari,
> 
> Thanks for the update.
> I've noticed that you added node labels to the child device nodes
> in [0]:
> 
> "as3645a_flash : flash" and "as3645a_indicator : indicator"

The phandle references (as3645a_flash and as3645a_indicator) should
actually be moved to the patch adding the flash property to the sensor
device node. It doesn't do anything here, yet.

> 
> I am still seeing problems with this approach:
> 
> 1) AFAIK these labels are only used for referencing nodes inside dts
>files and they don't affect the name property of struct device_node

That's right.

> 2) Even if you changed the node name from flash to as3645a_flash, you
>would get weird LED class device name "as3645a_flash:flash" in case
>label property is absent. Do you have any objections against the
>approach I proposed in the previous review?:
> 
> 
> snprintf(names->flash, sizeof(names->flash),
>AS_NAME":%s", node->name);

In the current patch, the device node of the flash controller is used,
postfixed with colon and the name of the LED ("flash" or "indicator") if
no label is defined. In other words, with that DT source you'll have
"as3645a:flash" and "as3645a:indicator". So if you change the name of
the device node of the I²C device, that will be reflected in the label.

If a label exists, then the label is used as such.

I don't really have objections to what you're proposing as such but my
question is: is it useful? With that, the flash and indicator labels
will not come from DT if label properties are undefined. They'll always
be "as3645a:flash" and "as3645a:indicator", independently of the names
of the device nodes.

-- 
Kind regards,

Sakari Ailus
sakari.ai...@iki.fi

Re: [PATCH v2.1 2/3] leds: as3645a: Add LED flash class driver

[Jacek Anaszewski]

Forgot to add a link to the referenced patch:

[0] http://www.spinics.net/lists/devicetree/msg191273.html

On 08/20/2017 12:09 PM, Jacek Anaszewski wrote:
> Hi Sakari,
> 
> Thanks for the update.
> I've noticed that you added node labels to the child device nodes
> in [0]:
> 
> "as3645a_flash : flash" and "as3645a_indicator : indicator"
> 
> I am still seeing problems with this approach:
> 
> 1) AFAIK these labels are only used for referencing nodes inside dts
>files and they don't affect the name property of struct device_node
> 2) Even if you changed the node name from flash to as3645a_flash, you
>would get weird LED class device name "as3645a_flash:flash" in case
>label property is absent. Do you have any objections against the
>approach I proposed in the previous review?:
> 
> 
> snprintf(names->flash, sizeof(names->flash),
>AS_NAME":%s", node->name);
> 
> Best regards,
> Jacek Anaszewski
> 
> On 08/19/2017 11:42 PM, Sakari Ailus wrote:
>> From: Sakari Ailus 
>>
>> 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 
>> ---
>> Well. The driver does not control explicit power resources nor it used
>> runtime PM. Remove references to it.
>>
>>  MAINTAINERS |   6 +
>>  drivers/leds/Kconfig|   8 +
>>  drivers/leds/Makefile   |   1 +
>>  drivers/leds/leds-as3645a.c | 763 
>> 
>>  4 files changed, 778 insertions(+)
>>  create mode 100644 drivers/leds/leds-as3645a.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 931abca006b7..8f40ba2e5303 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2124,6 +2124,12 @@ F:arch/arm64/
>>  F:  Documentation/arm64/
>>  
>>  AS3645A LED FLASH CONTROLLER DRIVER
>> +M:  Sakari Ailus 
>> +L:  linux-l...@vger.kernel.org
>> +S:  Maintained
>> +F:  drivers/leds/leds-as3645a.c
>> +
>> +AS3645A LED FLASH CONTROLLER DRIVER
>>  M:  Laurent Pinchart 
>>  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 594b24d410c3..bad3a4098104 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 909dae62ba05..7d7b26552923 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 ..e0898233
>> --- /dev/null
>> +++ b/drivers/leds/leds-as3645a.c
>> @@ -0,0 +1,763 @@
>> +/*
>> + * 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 
>> + *
>> + * 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 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +
>> +#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 

Re: [PATCH v2.1 2/3] leds: as3645a: Add LED flash class driver

[Jacek Anaszewski]

Hi Sakari,

Thanks for the update.
I've noticed that you added node labels to the child device nodes
in [0]:

"as3645a_flash : flash" and "as3645a_indicator : indicator"

I am still seeing problems with this approach:

1) AFAIK these labels are only used for referencing nodes inside dts
   files and they don't affect the name property of struct device_node
2) Even if you changed the node name from flash to as3645a_flash, you
   would get weird LED class device name "as3645a_flash:flash" in case
   label property is absent. Do you have any objections against the
   approach I proposed in the previous review?:


snprintf(names->flash, sizeof(names->flash),
 AS_NAME":%s", node->name);

Best regards,
Jacek Anaszewski

On 08/19/2017 11:42 PM, Sakari Ailus wrote:
> From: Sakari Ailus 
> 
> 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 
> ---
> Well. The driver does not control explicit power resources nor it used
> runtime PM. Remove references to it.
> 
>  MAINTAINERS |   6 +
>  drivers/leds/Kconfig|   8 +
>  drivers/leds/Makefile   |   1 +
>  drivers/leds/leds-as3645a.c | 763 
> 
>  4 files changed, 778 insertions(+)
>  create mode 100644 drivers/leds/leds-as3645a.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 931abca006b7..8f40ba2e5303 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2124,6 +2124,12 @@ F: arch/arm64/
>  F:   Documentation/arm64/
>  
>  AS3645A LED FLASH CONTROLLER DRIVER
> +M:   Sakari Ailus 
> +L:   linux-l...@vger.kernel.org
> +S:   Maintained
> +F:   drivers/leds/leds-as3645a.c
> +
> +AS3645A LED FLASH CONTROLLER DRIVER
>  M:   Laurent Pinchart 
>  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 594b24d410c3..bad3a4098104 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 909dae62ba05..7d7b26552923 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 ..e0898233
> --- /dev/null
> +++ b/drivers/leds/leds-as3645a.c
> @@ -0,0 +1,763 @@
> +/*
> + * 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 
> + *
> + * 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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#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:  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:  
> + * for 

[PATCH v2.1 2/3] leds: as3645a: Add LED flash class driver

[Sakari Ailus]

From: Sakari Ailus 

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 
---
Well. The driver does not control explicit power resources nor it used
runtime PM. Remove references to it.

 MAINTAINERS |   6 +
 drivers/leds/Kconfig|   8 +
 drivers/leds/Makefile   |   1 +
 drivers/leds/leds-as3645a.c | 763 
 4 files changed, 778 insertions(+)
 create mode 100644 drivers/leds/leds-as3645a.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 931abca006b7..8f40ba2e5303 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2124,6 +2124,12 @@ F:   arch/arm64/
 F: Documentation/arm64/
 
 AS3645A LED FLASH CONTROLLER DRIVER
+M: Sakari Ailus 
+L: linux-l...@vger.kernel.org
+S: Maintained
+F: drivers/leds/leds-as3645a.c
+
+AS3645A LED FLASH CONTROLLER DRIVER
 M: Laurent Pinchart 
 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 594b24d410c3..bad3a4098104 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 909dae62ba05..7d7b26552923 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 ..e0898233
--- /dev/null
+++ b/drivers/leds/leds-as3645a.c
@@ -0,0 +1,763 @@
+/*
+ * 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 
+ *
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#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:  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:  
+ * 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:   
*/
+#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