Re: [PATCH v3 3/4] leds: Add LED support for MT6323 PMIC

2017-02-17 Thread Jacek Anaszewski
Hi Sean,

Thanks for the updated patch. I have one more comment below.

On 02/17/2017 07:05 AM, sean.w...@mediatek.com wrote:
> From: Sean Wang 
> 
> MT6323 PMIC is a multi-function device that includes
> LED function. It allows attaching up to 4 LEDs which
> can either be on, off or dimmed and/or blinked with
> the controller.
> 
> Signed-off-by: Sean Wang 
> ---
>  drivers/leds/Kconfig   |   8 +
>  drivers/leds/Makefile  |   1 +
>  drivers/leds/leds-mt6323.c | 470 
> +
>  3 files changed, 479 insertions(+)
>  create mode 100644 drivers/leds/leds-mt6323.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index c621cbb..30095fc 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -117,6 +117,14 @@ config LEDS_MIKROTIK_RB532
> This option enables support for the so called "User LED" of
> Mikrotik's Routerboard 532.
>  
> +config LEDS_MT6323
> + tristate "LED Support for Mediatek MT6323 PMIC"
> + depends on LEDS_CLASS
> + depends on MFD_MT6397
> + help
> +   This option enables support for on-chip LED drivers found on
> +   Mediatek MT6323 PMIC.
> +
>  config LEDS_S3C24XX
>   tristate "LED Support for Samsung S3C24XX GPIO LEDs"
>   depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 6b82737..4feb332 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -72,6 +72,7 @@ obj-$(CONFIG_LEDS_IS31FL32XX)   += 
> leds-is31fl32xx.o
>  obj-$(CONFIG_LEDS_PM8058)+= leds-pm8058.o
>  obj-$(CONFIG_LEDS_MLXCPLD)   += leds-mlxcpld.o
>  obj-$(CONFIG_LEDS_NIC78BX)   += leds-nic78bx.o
> +obj-$(CONFIG_LEDS_MT6323)+= leds-mt6323.o
>  
>  # LED SPI Drivers
>  obj-$(CONFIG_LEDS_DAC124S085)+= leds-dac124s085.o
> diff --git a/drivers/leds/leds-mt6323.c b/drivers/leds/leds-mt6323.c
> new file mode 100644
> index 000..f431b1d
> --- /dev/null
> +++ b/drivers/leds/leds-mt6323.c
> @@ -0,0 +1,470 @@
> +/*
> + * LED driver for Mediatek MT6323 PMIC
> + *
> + * Copyright (C) 2017 Sean Wang 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * 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 
> +
> +/*
> + * Register field for MT6323_TOP_CKPDN0 to enable
> + * 32K clock common for LED device.
> + */
> +#define MT6323_RG_DRV_32K_CK_PDN BIT(11)
> +#define MT6323_RG_DRV_32K_CK_PDN_MASKBIT(11)
> +
> +/*
> + * Register field for MT6323_TOP_CKPDN2 to enable
> + * individual clock for LED device.
> + */
> +#define MT6323_RG_ISINK_CK_PDN(i)BIT(i)
> +#define MT6323_RG_ISINK_CK_PDN_MASK(i)   BIT(i)
> +
> +/*
> + * Register field for MT6323_TOP_CKCON1 to select
> + * clock source.
> + */
> +#define MT6323_RG_ISINK_CK_SEL_MASK(i)   (BIT(10) << (i))
> +
> +/*
> + * Register for MT6323_ISINK_CON0 to setup the
> + * duty cycle of the blink.
> + */
> +#define MT6323_ISINK_CON0(i) (MT6323_ISINK0_CON0 + 0x8 * (i))
> +#define MT6323_ISINK_DIM_DUTY_MASK   (0x1f << 8)
> +#define MT6323_ISINK_DIM_DUTY(i) (((i) << 8) & \
> + MT6323_ISINK_DIM_DUTY_MASK)
> +
> +/* Register to setup the period of the blink. */
> +#define MT6323_ISINK_CON1(i) (MT6323_ISINK0_CON1 + 0x8 * (i))
> +#define MT6323_ISINK_DIM_FSEL_MASK   (0x)
> +#define MT6323_ISINK_DIM_FSEL(i) ((i) & MT6323_ISINK_DIM_FSEL_MASK)
> +
> +/* Register to control the brightness. */
> +#define MT6323_ISINK_CON2(i) (MT6323_ISINK0_CON2 + 0x8 * (i))
> +#define MT6323_ISINK_CH_STEP_SHIFT   12
> +#define MT6323_ISINK_CH_STEP_MASK(0x7 << 12)
> +#define MT6323_ISINK_CH_STEP(i)  (((i) << 12) & \
> + MT6323_ISINK_CH_STEP_MASK)
> +#define MT6323_ISINK_SFSTR0_TC_MASK  (0x3 << 1)
> +#define MT6323_ISINK_SFSTR0_TC(i)(((i) << 1) & \
> + MT6323_ISINK_SFSTR0_TC_MASK)
> +#define MT6323_ISINK_SFSTR0_EN_MASK  BIT(0)
> +#define MT6323_ISINK_SFSTR0_EN   BIT(0)
> +
> +/* Register to LED channel enablement. */
> +#define MT6323_ISINK_CH_EN_MASK(i)   BIT(i)
> +#define MT6323_ISINK_CH_EN(i)BIT(i)
> +
> +#define MT6323_MAX_PERIOD  1
> +#define MT6323_MAX_LEDS4
> +#define MT6323_MAX_BRIGHTNESS  6
> +#define 

Re: [PATCH v3 3/4] leds: Add LED support for MT6323 PMIC

2017-02-17 Thread Jacek Anaszewski
Hi Sean,

Thanks for the updated patch. I have one more comment below.

On 02/17/2017 07:05 AM, sean.w...@mediatek.com wrote:
> From: Sean Wang 
> 
> MT6323 PMIC is a multi-function device that includes
> LED function. It allows attaching up to 4 LEDs which
> can either be on, off or dimmed and/or blinked with
> the controller.
> 
> Signed-off-by: Sean Wang 
> ---
>  drivers/leds/Kconfig   |   8 +
>  drivers/leds/Makefile  |   1 +
>  drivers/leds/leds-mt6323.c | 470 
> +
>  3 files changed, 479 insertions(+)
>  create mode 100644 drivers/leds/leds-mt6323.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index c621cbb..30095fc 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -117,6 +117,14 @@ config LEDS_MIKROTIK_RB532
> This option enables support for the so called "User LED" of
> Mikrotik's Routerboard 532.
>  
> +config LEDS_MT6323
> + tristate "LED Support for Mediatek MT6323 PMIC"
> + depends on LEDS_CLASS
> + depends on MFD_MT6397
> + help
> +   This option enables support for on-chip LED drivers found on
> +   Mediatek MT6323 PMIC.
> +
>  config LEDS_S3C24XX
>   tristate "LED Support for Samsung S3C24XX GPIO LEDs"
>   depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 6b82737..4feb332 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -72,6 +72,7 @@ obj-$(CONFIG_LEDS_IS31FL32XX)   += 
> leds-is31fl32xx.o
>  obj-$(CONFIG_LEDS_PM8058)+= leds-pm8058.o
>  obj-$(CONFIG_LEDS_MLXCPLD)   += leds-mlxcpld.o
>  obj-$(CONFIG_LEDS_NIC78BX)   += leds-nic78bx.o
> +obj-$(CONFIG_LEDS_MT6323)+= leds-mt6323.o
>  
>  # LED SPI Drivers
>  obj-$(CONFIG_LEDS_DAC124S085)+= leds-dac124s085.o
> diff --git a/drivers/leds/leds-mt6323.c b/drivers/leds/leds-mt6323.c
> new file mode 100644
> index 000..f431b1d
> --- /dev/null
> +++ b/drivers/leds/leds-mt6323.c
> @@ -0,0 +1,470 @@
> +/*
> + * LED driver for Mediatek MT6323 PMIC
> + *
> + * Copyright (C) 2017 Sean Wang 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * 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 
> +
> +/*
> + * Register field for MT6323_TOP_CKPDN0 to enable
> + * 32K clock common for LED device.
> + */
> +#define MT6323_RG_DRV_32K_CK_PDN BIT(11)
> +#define MT6323_RG_DRV_32K_CK_PDN_MASKBIT(11)
> +
> +/*
> + * Register field for MT6323_TOP_CKPDN2 to enable
> + * individual clock for LED device.
> + */
> +#define MT6323_RG_ISINK_CK_PDN(i)BIT(i)
> +#define MT6323_RG_ISINK_CK_PDN_MASK(i)   BIT(i)
> +
> +/*
> + * Register field for MT6323_TOP_CKCON1 to select
> + * clock source.
> + */
> +#define MT6323_RG_ISINK_CK_SEL_MASK(i)   (BIT(10) << (i))
> +
> +/*
> + * Register for MT6323_ISINK_CON0 to setup the
> + * duty cycle of the blink.
> + */
> +#define MT6323_ISINK_CON0(i) (MT6323_ISINK0_CON0 + 0x8 * (i))
> +#define MT6323_ISINK_DIM_DUTY_MASK   (0x1f << 8)
> +#define MT6323_ISINK_DIM_DUTY(i) (((i) << 8) & \
> + MT6323_ISINK_DIM_DUTY_MASK)
> +
> +/* Register to setup the period of the blink. */
> +#define MT6323_ISINK_CON1(i) (MT6323_ISINK0_CON1 + 0x8 * (i))
> +#define MT6323_ISINK_DIM_FSEL_MASK   (0x)
> +#define MT6323_ISINK_DIM_FSEL(i) ((i) & MT6323_ISINK_DIM_FSEL_MASK)
> +
> +/* Register to control the brightness. */
> +#define MT6323_ISINK_CON2(i) (MT6323_ISINK0_CON2 + 0x8 * (i))
> +#define MT6323_ISINK_CH_STEP_SHIFT   12
> +#define MT6323_ISINK_CH_STEP_MASK(0x7 << 12)
> +#define MT6323_ISINK_CH_STEP(i)  (((i) << 12) & \
> + MT6323_ISINK_CH_STEP_MASK)
> +#define MT6323_ISINK_SFSTR0_TC_MASK  (0x3 << 1)
> +#define MT6323_ISINK_SFSTR0_TC(i)(((i) << 1) & \
> + MT6323_ISINK_SFSTR0_TC_MASK)
> +#define MT6323_ISINK_SFSTR0_EN_MASK  BIT(0)
> +#define MT6323_ISINK_SFSTR0_EN   BIT(0)
> +
> +/* Register to LED channel enablement. */
> +#define MT6323_ISINK_CH_EN_MASK(i)   BIT(i)
> +#define MT6323_ISINK_CH_EN(i)BIT(i)
> +
> +#define MT6323_MAX_PERIOD  1
> +#define MT6323_MAX_LEDS4
> +#define MT6323_MAX_BRIGHTNESS  6
> +#define MT6323_UNIT_DUTY3125
> +
> +struct mt6323_leds;
> +
> +/**
>