by Sakari Ailus

[permalink] [raw]

Subject: Re: [PATCH/RFC v4 16/21] leds: Add support for max77693 mfd flash cell

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 <[email protected]>
> Signed-off-by: Andrzej Hajda <[email protected]>
> Acked-by: Lee Jones <[email protected]>
> Acked-by: Kyungmin Park <[email protected]>
> Cc: Bryan Wu <[email protected]>
> Cc: Richard Purdie <[email protected]>
> Cc: SangYoung Son <[email protected]>
> Cc: Samuel Ortiz <[email protected]>
> ---
> 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 <[email protected]>
> + * Andrzej Hajda <[email protected]>
> + *
> + * 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
[email protected]

2014-07-28 13:41:12

by Grant Likely

[permalink] [raw]

Subject: Re: [PATCH/RFC v4 07/21] of: add of_node_ncmp wrapper

On Fri, 11 Jul 2014 16:04:10 +0200, Jacek Anaszewski <[email protected]> wrote:
> The wrapper for strnicmp is required for checking whether a node has
> expected prefix.
>
> Signed-off-by: Jacek Anaszewski <[email protected]>
> Acked-by: Kyungmin Park <[email protected]>
> Cc: Grant Likely <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Michal Simek <[email protected]>
> ---
> include/linux/of.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 692b56c..9a53eea 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -199,6 +199,7 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size)
> #define of_compat_cmp(s1, s2, l) strcasecmp((s1), (s2))
> #define of_prop_cmp(s1, s2) strcmp((s1), (s2))
> #define of_node_cmp(s1, s2) strcasecmp((s1), (s2))
> +#define of_node_ncmp(s1, s2, n) strnicmp((s1), (s2), (n))
> #endif

Don't forget to add an of_node_ncmp() define to
arch/sparc/include/asm/prom.h. Sparc has its own rules.

g.