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

2017-08-19 Thread Sakari Ailus
Hi Jacek,

On Wed, Aug 16, 2017 at 10:27:31PM +0200, Jacek Anaszewski wrote:
> Hi Sakari,
> 
> Thanks for the patch.

Thanks for the review!

> 
> I have few more remarks regarding LED class device naming and
> pm handling below.
> 
> On 08/16/2017 02:55 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 
> > ---
> >  MAINTAINERS |   6 +
> >  drivers/leds/Kconfig|   8 +
> >  drivers/leds/Makefile   |   1 +
> >  drivers/leds/leds-as3645a.c | 785 
> > 
> >  4 files changed, 800 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 ..2335510a08e1
> > --- /dev/null
> > +++ b/drivers/leds/leds-as3645a.c
> > @@ -0,0 +1,785 @@
> > +/*
> > + * 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 
> > +
> > +#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  

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

2017-08-16 Thread Jacek Anaszewski
Hi Sakari,

Thanks for the patch.

I have few more remarks regarding LED class device naming and
pm handling below.

On 08/16/2017 02:55 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 
> ---
>  MAINTAINERS |   6 +
>  drivers/leds/Kconfig|   8 +
>  drivers/leds/Makefile   |   1 +
>  drivers/leds/leds-as3645a.c | 785 
> 
>  4 files changed, 800 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 ..2335510a08e1
> --- /dev/null
> +++ b/drivers/leds/leds-as3645a.c
> @@ -0,0 +1,785 @@
> +/*
> + * 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 
> +
> +#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_SHIFT4
> +#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_SHIFT0
> +#define 

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

2017-08-16 Thread Javier Martinez Canillas
Hello Sakari,

I haven't looked at the driver, but just have a comment about the I2C subsystem.

On Wed, Aug 16, 2017 at 2:55 PM, Sakari Ailus
 wrote:

[snip]

> +
> +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   = _pm_ops,
> +   },
> +   .probe_new  = as3645a_probe,
> +   .remove = as3645a_remove,
> +};
> +
> +module_i2c_driver(as3645a_i2c_driver);
> +

The I2C core is still broken w.r.t reporting a proper MODALIAS for OF
registered devices, it will report a MODALIAS=i2c:as3645a in this
case. So if you build this as a module, autoload won't work.

In theory this will be fixed soon, but for now you should add a
i2c_device_id table and export it with MODULE_DEVICE_TABLE(i2c,...) if
you care about module autoloading.

Best regards,
Javier


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

2017-08-16 Thread 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 
---
 MAINTAINERS |   6 +
 drivers/leds/Kconfig|   8 +
 drivers/leds/Makefile   |   1 +
 drivers/leds/leds-as3645a.c | 785 
 4 files changed, 800 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 ..2335510a08e1
--- /dev/null
+++ b/drivers/leds/leds-as3645a.c
@@ -0,0 +1,785 @@
+/*
+ * 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 
+
+#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 AS_CONTROL_STROBE_ON   (1 << 2)
+#define AS_CONTROL_OUT_ON  (1 << 3)
+#define