Hi Jacek,

Thanks for the patchset.

Jacek Anaszewski wrote:
> This patch adds led-flash support to Maxim max77693 chipset.
> A device can be exposed to user space through LED subsystem
> sysfs interface or through V4L2 subdevice when the support
> for V4L2 Flash sub-devices is enabled. Device supports up to
> two leds which can work in flash and torch mode. Leds can
> be triggered externally or by software.
> 
> Signed-off-by: Jacek Anaszewski <j.anaszew...@samsung.com>
> Signed-off-by: Andrzej Hajda <a.ha...@samsung.com>
> Acked-by: Lee Jones <lee.jo...@linaro.org>
> Acked-by: Kyungmin Park <kyungmin.p...@samsung.com>
> Cc: Bryan Wu <coolo...@gmail.com>
> Cc: Richard Purdie <rpur...@rpsys.net>
> Cc: SangYoung Son <hello....@smasung.com>
> Cc: Samuel Ortiz <sa...@linux.intel.com>
> ---
>  drivers/leds/Kconfig         |    9 +
>  drivers/leds/Makefile        |    1 +
>  drivers/leds/leds-max77693.c | 1070 
> ++++++++++++++++++++++++++++++++++++++++++
>  drivers/mfd/max77693.c       |    5 +-
>  include/linux/mfd/max77693.h |   40 ++
>  5 files changed, 1124 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/leds/leds-max77693.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 5032c6f..794055e 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -457,6 +457,15 @@ config LEDS_TCA6507
>         LED driver chips accessed via the I2C bus.
>         Driver support brightness control and hardware-assisted blinking.
>  
> +config LEDS_MAX77693
> +     tristate "LED support for MAX77693 Flash"
> +     depends on LEDS_CLASS_FLASH
> +     depends on MFD_MAX77693
> +     help
> +       This option enables support for the flash part of the MAX77693
> +       multifunction device. It has build in control for two leds in flash
> +       and torch mode.
> +
>  config LEDS_MAX8997
>       tristate "LED support for MAX8997 PMIC"
>       depends on LEDS_CLASS && MFD_MAX8997
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 237c5ba..da1a4ba 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -55,6 +55,7 @@ obj-$(CONFIG_LEDS_MC13783)          += leds-mc13783.o
>  obj-$(CONFIG_LEDS_NS2)                       += leds-ns2.o
>  obj-$(CONFIG_LEDS_NETXBIG)           += leds-netxbig.o
>  obj-$(CONFIG_LEDS_ASIC3)             += leds-asic3.o
> +obj-$(CONFIG_LEDS_MAX77693)          += leds-max77693.o
>  obj-$(CONFIG_LEDS_MAX8997)           += leds-max8997.o
>  obj-$(CONFIG_LEDS_LM355x)            += leds-lm355x.o
>  obj-$(CONFIG_LEDS_BLINKM)            += leds-blinkm.o
> diff --git a/drivers/leds/leds-max77693.c b/drivers/leds/leds-max77693.c
> new file mode 100644
> index 0000000..38a2398
> --- /dev/null
> +++ b/drivers/leds/leds-max77693.c
> @@ -0,0 +1,1070 @@
> +/*
> + * LED Flash Class driver for the flash cell of max77693 mfd.
> + *
> + *   Copyright (C) 2014, Samsung Electronics Co., Ltd.
> + *
> + *   Authors: Jacek Anaszewski <j.anaszew...@samsung.com>
> + *            Andrzej Hajda <a.ha...@samsung.com>
> + *
> + * 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.
> + */
> +
> +#include <asm/div64.h>
> +#include <linux/led-class-flash.h>
> +#include <linux/led-flash-manager.h>
> +#include <linux/mfd/max77693.h>
> +#include <linux/mfd/max77693-private.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +#include <media/v4l2-flash.h>
> +
> +#define MAX77693_LED_NAME_1          "max77693-flash_1"
> +#define MAX77693_LED_NAME_2          "max77693-flash_2"
> +
> +#define MAX77693_TORCH_IOUT_BITS     4
> +
> +#define MAX77693_TORCH_NO_TIMER              0x40
> +#define MAX77693_FLASH_TIMER_LEVEL   0x80
> +
> +#define MAX77693_FLASH_EN_OFF                0
> +#define MAX77693_FLASH_EN_FLASH              1
> +#define MAX77693_FLASH_EN_TORCH              2
> +#define MAX77693_FLASH_EN_ON         3
> +
> +#define MAX77693_FLASH_EN1_SHIFT     6
> +#define MAX77693_FLASH_EN2_SHIFT     4
> +#define MAX77693_TORCH_EN1_SHIFT     2
> +#define MAX77693_TORCH_EN2_SHIFT     0

Which registers are these definitions related to? Could they be defined
next to the registers instead?

You could parameterise these macros, e.g.

#define MAX77693_FLASH_EN_SHIFT(a)      (6 - ((a) - 1) * 2)

> +#define MAX77693_FLASH_LOW_BATTERY_EN        0x80
> +
> +#define MAX77693_FLASH_BOOST_FIXED   0x04
> +#define MAX77693_FLASH_BOOST_LEDNUM_2        0x80
> +
> +#define MAX77693_FLASH_TIMEOUT_MIN   62500
> +#define MAX77693_FLASH_TIMEOUT_MAX   1000000
> +#define MAX77693_FLASH_TIMEOUT_STEP  62500
> +
> +#define MAX77693_TORCH_TIMEOUT_MIN   262000
> +#define MAX77693_TORCH_TIMEOUT_MAX   15728000
> +
> +#define MAX77693_FLASH_IOUT_MIN              15625
> +#define MAX77693_FLASH_IOUT_MAX_1LED 1000000
> +#define MAX77693_FLASH_IOUT_MAX_2LEDS        625000
> +#define MAX77693_FLASH_IOUT_STEP     15625
> +
> +#define MAX77693_TORCH_IOUT_MIN              15625
> +#define MAX77693_TORCH_IOUT_MAX              250000
> +#define MAX77693_TORCH_IOUT_STEP     15625
> +
> +#define MAX77693_FLASH_VSYS_MIN              2400
> +#define MAX77693_FLASH_VSYS_MAX              3400
> +#define MAX77693_FLASH_VSYS_STEP     33
> +
> +#define MAX77693_FLASH_VOUT_MIN              3300
> +#define MAX77693_FLASH_VOUT_MAX              5500
> +#define MAX77693_FLASH_VOUT_STEP     25
> +#define MAX77693_FLASH_VOUT_RMIN     0x0c
> +
> +#define MAX77693_LED_STATUS_FLASH_ON (1 << 3)
> +#define MAX77693_LED_STATUS_TORCH_ON (1 << 2)
> +
> +#define MAX77693_LED_FLASH_INT_FLED2_OPEN    (1 << 0)
> +#define MAX77693_LED_FLASH_INT_FLED2_SHORT   (1 << 1)
> +#define MAX77693_LED_FLASH_INT_FLED1_OPEN    (1 << 2)
> +#define MAX77693_LED_FLASH_INT_FLED1_SHORT   (1 << 3)
> +#define MAX77693_LED_FLASH_INT_OVER_CURRENT  (1 << 4)
> +
> +#define MAX77693_MODE_OFF                    0
> +#define MAX77693_MODE_FLASH1                 (1 << 0)
> +#define MAX77693_MODE_FLASH2                 (1 << 1)
> +#define MAX77693_MODE_TORCH1                 (1 << 2)
> +#define MAX77693_MODE_TORCH2                 (1 << 3)
> +#define MAX77693_MODE_FLASH_EXTERNAL1                (1 << 4)
> +#define MAX77693_MODE_FLASH_EXTERNAL2                (1 << 5)
> +
> +enum {
> +     FLED1,
> +     FLED2
> +};
> +
> +enum {
> +     FLASH,
> +     TORCH
> +};
> +
> +struct max77693_led {
> +     struct regmap *regmap;
> +     struct platform_device *pdev;
> +     struct max77693_led_platform_data *pdata;
> +     struct mutex lock;
> +
> +     struct led_classdev_flash ldev1;
> +     struct work_struct work1_brightness_set;
> +     struct v4l2_flash *v4l2_flash1;
> +
> +     struct led_classdev_flash ldev2;
> +     struct work_struct work2_brightness_set;
> +     struct v4l2_flash *v4l2_flash2;
> +
> +     unsigned int torch1_brightness;
> +     unsigned int torch2_brightness;
> +     unsigned int flash1_timeout;
> +     unsigned int flash2_timeout;

Could you use e.g. an array of structs instead? You could remove a lot
of redundant code operating on the other led.

> +     unsigned int current_flash_timeout;
> +     unsigned int mode_flags;
> +     u8 torch_iout_reg;
> +     bool iout_joint;
> +};

...

> +static int max77693_led_probe(struct platform_device *pdev)
> +{
> +     struct device *dev = &pdev->dev;
> +     struct max77693_dev *iodev = dev_get_drvdata(dev->parent);
> +     struct max77693_led *led;
> +     struct max77693_led_platform_data *p;
> +     int ret;
> +
> +     led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
> +     if (!led)
> +             return -ENOMEM;
> +
> +     led->pdev = pdev;
> +     led->regmap = iodev->regmap;
> +     platform_set_drvdata(pdev, led);
> +     ret = max77693_led_get_platform_data(led);
> +     if (ret < 0)
> +             return -EINVAL;
> +
> +     p = led->pdata;
> +     mutex_init(&led->lock);
> +
> +     if (p->num_leds == 1 && p->fleds[FLED1] && p->fleds[FLED2])
> +             led->iout_joint = true;
> +
> +     ret = max77693_setup(led);
> +     if (ret < 0)
> +             return ret;

goto err_register_led1;

> +     if (led->iout_joint || p->fleds[FLED1]) {
> +             ret = max77693_register_led1(led);
> +             if (ret < 0)
> +                     goto err_register_led1;
> +     }
> +
> +     if (!led->iout_joint && p->fleds[FLED2]) {
> +             ret = max77693_register_led2(led);
> +             if (ret < 0)
> +                     goto err_register_led2;
> +     }
> +
> +     return 0;
> +
> +err_register_led2:
> +     if (!p->fleds[FLED1])
> +             goto err_register_led1;
> +     v4l2_flash_release(led->v4l2_flash1);
> +     led_classdev_flash_unregister(&led->ldev1);
> +err_register_led1:
> +     mutex_destroy(&led->lock);
> +
> +     return ret;
> +}

...

-- 
Kind regards,

Sakari Ailus
sakari.ai...@iki.fi
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to