by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH v2 1/4] mfd: add LM3533 lighting-power core driver

On Thu, May 03, 2012 at 12:26:36PM +0200, Johan Hovold wrote:
> Add support for National Semiconductor / TI LM3533 lighting power chips.
>
> This is the core driver which provides register access over I2C and
> registers the ambient-light-sensor, LED and backlight sub-drivers.

Reviwed-by: Mark Brown <[email protected]>

though

> + dev_dbg(lm3533->dev, "read [%02x]: %02x\n", reg, *val);

I'd expect you can drop these log messages, if there's stuff like this
missing we should add it to regmap. At the minute the regmap logging is
via trace points rather than debug logs as you can leave them enabled
all the time.

Might also be worth moving some of the sysfs stuff to live with the
relevant drivers.

Attachments:

(No filename) (726.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2012-05-03 10:43:49

On Thu, May 03, 2012 at 01:28:03PM +0200, Johan Hovold wrote:
> On Thu, May 03, 2012 at 11:38:48AM +0100, Mark Brown wrote:

> > I'd expect you can drop these log messages, if there's stuff like this
> > missing we should add it to regmap. At the minute the regmap logging is
> > via trace points rather than debug logs as you can leave them enabled
> > all the time.

> If such debugging is added to regmap we still need a way to enable them
> per driver (or rather regmap) to not clutter the logs.

This is one of the reasons why we currently use tracepoints (they just
don't have this issue as they're trivial to filter), though
adding some sort of infrastructure for it ought not to be too difficult
even if it's just at the regmap level.

> These three dev_dbg statements are extremely useful during debugging /
> development especially in combination with the other dynamic printks in
> these drivers.

> I'd actually prefer just keeping them for now.

OTOH the whole point in having stuff like this is to factor out repeated
code like this so if the infrastructure isn't working we should fix
that.

> > Might also be worth moving some of the sysfs stuff to live with the
> > relevant drivers.

> Which attributes do you have in mind?

Pretty much all of those on the MFD.

> The boost_freq and boost_ovp affect both backlight devices (i.e. cannot
> be set separately) and as such belong in the parent driver IMO.

> Same with the output_hvled{1..2}, output_lvled{1..5} attributes. The
> chip has four logical LEDs ("control banks") but five low-voltage output
> sinks. The five output_lvled attributes determine the mapping and as
> such belong in the parent driver. The two logical backlight devices can
> likewise be used to control either or both high-voltage outputs and
> belong in the parent driver for the same reasons.

Actually, the other question I had but forgot to ask (or I think punted
on for your response) was why these are in sysfs at all - things like
which things are connected to the backlight are going to be a property
of the board design so should be defined by the machine not tweaked from
userspace.

Attachments:

(No filename) (2.08 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2012-05-03 11:40:22

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] iio: add LM3533 ambient light sensor driver

On 5/3/2012 11:26 AM, Johan Hovold wrote:
> Add sub-driver for the ambient light sensor interface on National
> Semiconductor / TI LM3533 lighting power chips.
>
> The sensor interface can be used to control the LEDs and backlights of
> the chip through defining five light zones and three sets of
> corresponding brightness target levels.
>
> The driver provides raw and mean adc readings along with the current
> light zone through sysfs. A threshold event can be generated on zone
> changes.
Code is fine. Pretty much all my comments are to do with the interface.
>
> Signed-off-by: Johan Hovold<[email protected]>
> ---
>
> v2:
> - reimplement using iio
> - add sysfs-ABI documentation
>
>
> .../Documentation/sysfs-bus-iio-light-lm3533-als | 62 ++
> drivers/staging/iio/light/Kconfig | 16 +
> drivers/staging/iio/light/Makefile | 1 +
> drivers/staging/iio/light/lm3533-als.c | 617 ++++++++++++++++++++
> 4 files changed, 696 insertions(+), 0 deletions(-)
> create mode 100644 drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als
> create mode 100644 drivers/staging/iio/light/lm3533-als.c
>
> diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als
> new file mode 100644
> index 0000000..9849d14
> --- /dev/null
> +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als
> @@ -0,0 +1,62 @@
> +What: /sys/bus/iio/devices/iio:deviceX/gain
> +Date: April 2012
> +KernelVersion: 3.5
> +Contact: Johan Hovold<[email protected]>
> +Description:
> + Set the ALS gain-resistor setting (0..127) for analog input
> + mode, where
> +
> + 0000000 - ALS input is high impedance
> + 0000001 - 200kOhm (10uA at 2V full-scale)
> + 0000010 - 100kOhm (20uA at 2V full-scale)
> + ...
> + 1111110 - 1.587kOhm (1.26mA at 2V full-scale)
> + 1111111 - 1.575kOhm (1.27mA at 2V full-scale)
> +
> + R_als = 2V / (10uA * gain) (gain> 0)
Firstly, no magic numbers. These are definitely magic. Secondly see
in_illuminance0_scale for a suitable existing attribute.
> +
> +What: /sys/bus/iio/devices/iio:deviceX/illuminance_zone
> +Date: April 2012
> +KernelVersion: 3.5
> +Contact: Johan Hovold<[email protected]>
> +Description:
> + Get the current light zone (0..4) as defined by the
> + in_illuminance_thresh[n]_{falling,rising} thresholds.
Hmm.. definitely have an in prefix, beyond that I'm not sure what the
cleanest
interface will be for this. Could extend the event codes to deal with the
zone index. Slightly tricky as the channel could already be modified so
chan2 isn't necesarily available.
> +
> +What: /sys/.../events/in_illuminance_thresh_either_en
> +Date: April 2012
> +KernelVersion: 3.5
> +Contact: Johan Hovold<[email protected]>
> +Description:
> + Event generated when channel passes one of the four threshold
> + in either direction (rising|falling) and a zone change occurs.
> + The corresponding light zone can be read from
> + illuminance_zone.
> +
> +What: /sys/.../events/illuminance_thresh0_falling_value
hmm.. every time you think you are making progress a new and exciting
device comes
along requiring the abi to be extended.
in_illuminanceX_threshY_rising_value
in_illuminanceX_threshY_falling_value
should do with appropriate description.
> +What: /sys/.../events/illuminance_thresh0_raising_value
> +What: /sys/.../events/illuminance_thresh1_falling_value
> +What: /sys/.../events/illuminance_thresh1_raising_value
> +What: /sys/.../events/illuminance_thresh2_falling_value
> +What: /sys/.../events/illuminance_thresh2_raising_value
> +What: /sys/.../events/illuminance_thresh3_falling_value
> +What: /sys/.../events/illuminance_thresh3_raising_value
> +Date: April 2012
> +KernelVersion: 3.5
> +Contact: Johan Hovold<[email protected]>
> +Description:
> + Specifies the value of threshold that the device is comparing
> + against for the events enabled by
> + in_illuminance_thresh_either_en, and defines the
> + the five light zones.
> +
> + These thresholds correspond to the eight zone-boundary
> + registers (boundary[n]_{low,high}).
> +
This interface is going to take some thought. We have
in_illuminance0_target at the
moment, so I guess we can add a zoning concept to that...
> +What: /sys/bus/iio/devices/iio:deviceX/target[m]_[n]
> +Date: April 2012
> +KernelVersion: 3.5
> +Contact: Johan Hovold<[email protected]>
> +Description:
> + Set the target brightness for ALS-mapper m in light zone n
> + (0..255), where m in 1..3 and n in 0..4.
Don't suppose you could do a quick summary of what these zones are and
why there
are 3 ALS-mappers? I'm not getting terribly far on a quick look at the
datasheet!
> diff --git a/drivers/staging/iio/light/Kconfig b/drivers/staging/iio/light/Kconfig
> index e7e9159..263e44a 100644
> --- a/drivers/staging/iio/light/Kconfig
> +++ b/drivers/staging/iio/light/Kconfig
> @@ -24,6 +24,22 @@ config SENSORS_TSL2563
> This driver can also be built as a module. If so, the module
> will be called tsl2563.
>
> +config SENSORS_LM3533
> + tristate "LM3533 ambient light sensor"
> + depends on MFD_LM3533
> + help
> + If you say yes here you get support for the ambient light sensor
> + interface on National Semiconductor / TI LM3533 Lighting Power
> + chips.
> +
> + The sensor interface can be used to control the LEDs and backlights
> + of the chip through defining five light zones and three sets of
> + corresponding brightness target levels.
> +
> + The driver provides raw and mean adc readings along with the current
> + light zone through sysfs. A threshold event can be generated on zone
> + changes.
> +
> config TSL2583
> tristate "TAOS TSL2580, TSL2581 and TSL2583 light-to-digital converters"
> depends on I2C
> diff --git a/drivers/staging/iio/light/Makefile b/drivers/staging/iio/light/Makefile
> index 3011fbf..16a60a2 100644
> --- a/drivers/staging/iio/light/Makefile
> +++ b/drivers/staging/iio/light/Makefile
> @@ -4,4 +4,5 @@
>
> obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o
> obj-$(CONFIG_SENSORS_ISL29018) += isl29018.o
> +obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o
> obj-$(CONFIG_TSL2583) += tsl2583.o
> diff --git a/drivers/staging/iio/light/lm3533-als.c b/drivers/staging/iio/light/lm3533-als.c
> new file mode 100644
> index 0000000..e2c9be6
> --- /dev/null
> +++ b/drivers/staging/iio/light/lm3533-als.c
> @@ -0,0 +1,617 @@
> +/*
> + * lm3533-als.c -- LM3533 Ambient Light Sensor driver
> + *
> + * Copyright (C) 2011-2012 Texas Instruments
> + *
> + * Author: Johan Hovold<[email protected]>
> + *
> + * 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.
> + */
> +
> +#include<linux/atomic.h>
> +#include<linux/fs.h>
> +#include<linux/interrupt.h>
> +#include<linux/io.h>
> +#include<linux/module.h>
> +#include<linux/mfd/core.h>
> +#include<linux/platform_device.h>
> +#include<linux/slab.h>
> +#include<linux/uaccess.h>
> +
> +#include<linux/mfd/lm3533.h>
> +
> +#include "../events.h"
> +#include "../iio.h"
This will need to go through the staging-next tree. In there these
headers have moved.
> +
> +
> +#define LM3533_ALS_RESISTOR_MAX 0x7f
> +#define LM3533_ALS_ADC_MAX 0xff
> +#define LM3533_ALS_BOUNDARY_MAX LM3533_ALS_ADC_MAX
> +#define LM3533_ALS_TARGET_MAX LM3533_ALS_ADC_MAX
> +#define LM3533_ALS_ZONE_MAX 4
> +
> +#define LM3533_REG_ALS_RESISTOR_SELECT 0x30
> +#define LM3533_REG_ALS_CONF 0x31
> +#define LM3533_REG_ALS_ZONE_INFO 0x34
> +#define LM3533_REG_ALS_READ_ADC_AVERAGE 0x37
> +#define LM3533_REG_ALS_READ_ADC_RAW 0x38
> +#define LM3533_REG_ALS_BOUNDARY0_HIGH 0x50
> +#define LM3533_REG_ALS_BOUNDARY0_LOW 0x51
> +#define LM3533_REG_ALS_BOUNDARY1_HIGH 0x52
> +#define LM3533_REG_ALS_BOUNDARY1_LOW 0x53
> +#define LM3533_REG_ALS_BOUNDARY2_HIGH 0x54
> +#define LM3533_REG_ALS_BOUNDARY2_LOW 0x55
> +#define LM3533_REG_ALS_BOUNDARY3_HIGH 0x56
> +#define LM3533_REG_ALS_BOUNDARY3_LOW 0x57
> +#define LM3533_REG_ALS_M1_TARGET_0 0x60
> +#define LM3533_REG_ALS_M1_TARGET_1 0x61
> +#define LM3533_REG_ALS_M1_TARGET_2 0x62
> +#define LM3533_REG_ALS_M1_TARGET_3 0x63
> +#define LM3533_REG_ALS_M1_TARGET_4 0x64
> +#define LM3533_REG_ALS_M2_TARGET_0 0x65
> +#define LM3533_REG_ALS_M2_TARGET_1 0x66
> +#define LM3533_REG_ALS_M2_TARGET_2 0x67
> +#define LM3533_REG_ALS_M2_TARGET_3 0x68
> +#define LM3533_REG_ALS_M2_TARGET_4 0x69
> +#define LM3533_REG_ALS_M3_TARGET_0 0x6a
> +#define LM3533_REG_ALS_M3_TARGET_1 0x6b
> +#define LM3533_REG_ALS_M3_TARGET_2 0x6c
> +#define LM3533_REG_ALS_M3_TARGET_3 0x6d
> +#define LM3533_REG_ALS_M3_TARGET_4 0x6e
> +
> +#define LM3533_ALS_ENABLE_MASK 0x01
> +#define LM3533_ALS_INPUT_MODE_MASK 0x02
> +#define LM3533_ALS_INT_ENABLE_MASK 0x01
> +
> +#define LM3533_ALS_ZONE_SHIFT 2
> +#define LM3533_ALS_ZONE_MASK 0x1c
> +
> +#define LM3533_ALS_FLAG_INT_ENABLED 1
> +
> +
> +struct lm3533_als {
> + struct lm3533 *lm3533;
> +
> + unsigned long flags;
> + int irq;
> +
> + atomic_t zone;
> +};
> +
> +
> +static int lm3533_als_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val1, int *val2, long mask)
> +{
> + struct lm3533_als *als = iio_priv(indio_dev);
> + u8 reg;
> + u8 val;
> + int ret;
> +
> + switch (mask) {
> + case 0:
> + reg = LM3533_REG_ALS_READ_ADC_RAW;
> + break;
> + case IIO_CHAN_INFO_AVERAGE_RAW:
> + reg = LM3533_REG_ALS_READ_ADC_AVERAGE;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + ret = lm3533_read(als->lm3533, reg,&val);
> + if (ret) {
> + dev_err(&indio_dev->dev, "failed to read adc\n");
> + return ret;
> + }
> +
> + *val1 = val;
> +
> + return IIO_VAL_INT;
> +}
> +
> +static const struct iio_chan_spec lm3533_als_channels[] = {
> + {
> + .type = IIO_LIGHT,
> + .info_mask = IIO_CHAN_INFO_AVERAGE_RAW_SEPARATE_BIT,
> + .channel = 0,
channel doesn't get used unless you also set indexed = 1.
> + }
> +};
> +
> +static int lm3533_als_get_zone(struct iio_dev *indio_dev, u8 *zone)
> +{
> + struct lm3533_als *als = iio_priv(indio_dev);
> + u8 val;
> + int ret;
> +
> + ret = lm3533_read(als->lm3533, LM3533_REG_ALS_ZONE_INFO,&val);
> + if (ret) {
> + dev_err(&indio_dev->dev, "failed to read zone\n");
> + return ret;
> + }
> +
> + val = (val& LM3533_ALS_ZONE_MASK)>> LM3533_ALS_ZONE_SHIFT;
> + *zone = min_t(u8, val, LM3533_ALS_ZONE_MAX);
> +
> + return 0;
> +}
> +
> +static irqreturn_t lm3533_als_isr(int irq, void *dev_id)
> +{
> +
> + struct iio_dev *indio_dev = dev_id;
> + struct lm3533_als *als = iio_priv(indio_dev);
> + u8 zone;
> + int ret;
> +
> + /* Clear interrupt by reading the ALS zone register. */
> + ret = lm3533_als_get_zone(indio_dev,&zone);
> + if (ret)
> + goto out;
> +
> + atomic_set(&als->zone, zone);
> +
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
> + 0,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_EITHER),
> + iio_get_time_ns());
> +out:
> + return IRQ_HANDLED;
> +}
> +
> +static int lm3533_als_set_int_mode(struct iio_dev *indio_dev, int enable)
> +{
> + struct lm3533_als *als = iio_priv(indio_dev);
> + u8 mask = LM3533_ALS_INT_ENABLE_MASK;
> + u8 val;
> + int ret;
> +
> + if (enable)
> + val = mask;
> + else
> + val = 0;
> +
> + ret = lm3533_update(als->lm3533, LM3533_REG_ALS_ZONE_INFO, val, mask);
> + if (ret) {
> + dev_err(&indio_dev->dev, "failed to set int mode %d\n",
> + enable);
extra brackets.
> + }
> +
> + return ret;
> +}
> +
> +static int lm3533_als_get_int_mode(struct iio_dev *indio_dev, int *enable)
> +{
> + struct lm3533_als *als = iio_priv(indio_dev);
> + u8 mask = LM3533_ALS_INT_ENABLE_MASK;
> + u8 val;
> + int ret;
> +
> + ret = lm3533_read(als->lm3533, LM3533_REG_ALS_ZONE_INFO,&val);
> + if (ret) {
> + dev_err(&indio_dev->dev, "failed to get int mode\n");
> + return ret;
> + }
> +
> + *enable = !!(val& mask);
> +
> + return 0;
> +}
> +
> +static int show_thresh_either_en(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct lm3533_als *als = iio_priv(indio_dev);
> + int enable;
> + int ret;
> +
> + if (als->irq) {
> + ret = lm3533_als_get_int_mode(indio_dev,&enable);
> + if (ret)
> + return ret;
> + } else {
> + enable = 0;
> + }
> +
> + return scnprintf(buf, PAGE_SIZE, "%u\n", enable);
> +}
> +
> +static int store_thresh_either_en(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct lm3533_als *als = iio_priv(indio_dev);
> + unsigned long enable;
> + bool int_enabled;
> + u8 zone;
> + int ret;
> +
> + if (!als->irq)
> + return -EBUSY;
> +
> + if (kstrtoul(buf, 0,&enable))
> + return -EINVAL;
> +
> + int_enabled = test_bit(LM3533_ALS_FLAG_INT_ENABLED,&als->flags);
> +
> + if (enable&& !int_enabled) {
> + ret = lm3533_als_get_zone(indio_dev,&zone);
> + if (ret)
> + return ret;
> +
> + atomic_set(&als->zone, zone);
> +
> + set_bit(LM3533_ALS_FLAG_INT_ENABLED,&als->flags);
> + }
> +
> + ret = lm3533_als_set_int_mode(indio_dev, enable);
> + if (ret) {
> + if (!int_enabled)
> + clear_bit(LM3533_ALS_FLAG_INT_ENABLED,&als->flags);
> +
> + return ret;
> + }
> +
> + if (!enable)
> + clear_bit(LM3533_ALS_FLAG_INT_ENABLED,&als->flags);
> +
> + return len;
> +}
> +
> +static ssize_t show_zone(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct lm3533_als *als = iio_priv(indio_dev);
> + u8 zone;
> + int ret;
> +
> + if (test_bit(LM3533_ALS_FLAG_INT_ENABLED,&als->flags)) {
> + zone = atomic_read(&als->zone);
> + } else {
> + ret = lm3533_als_get_zone(indio_dev,&zone);
> + if (ret)
> + return ret;
> + }
> +
> + return scnprintf(buf, PAGE_SIZE, "%u\n", zone);
> +}
> +
> +struct lm3533_device_attribute {
> + struct device_attribute dev_attr;
> + u8 reg;
> + u8 max;
> +};
> +
> +#define to_lm3533_dev_attr(_dev_attr) \
> + container_of(_dev_attr, struct lm3533_device_attribute, dev_attr)
> +
> +static ssize_t show_lm3533_als_reg(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct lm3533_als *als = iio_priv(indio_dev);
> + struct lm3533_device_attribute *lm3533_attr = to_lm3533_dev_attr(attr);
> + u8 val;
> + int ret;
> +
> + ret = lm3533_read(als->lm3533, lm3533_attr->reg,&val);
> + if (ret)
> + return ret;
> +
> + return scnprintf(buf, PAGE_SIZE, "%u\n", val);
> +}
> +
> +static ssize_t store_lm3533_als_reg(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct lm3533_als *als = iio_priv(indio_dev);
> + struct lm3533_device_attribute *lm3533_attr = to_lm3533_dev_attr(attr);
> + u8 val;
> + int ret;
> +
> + if (kstrtou8(buf, 0,&val) || val> lm3533_attr->max)
> + return -EINVAL;
> +
> + ret = lm3533_write(als->lm3533, lm3533_attr->reg, val);
> + if (ret)
> + return ret;
> +
> + return len;
> +}
> +
> +#define REG_ATTR(_name, _mode, _show, _store, _reg, _max) \
> + { .dev_attr = __ATTR(_name, _mode, _show, _store), \
> + .reg = _reg, \
> + .max = _max }
> +
> +#define LM3533_REG_ATTR(_name, _mode, _show, _store, _reg, _max) \
> + struct lm3533_device_attribute lm3533_dev_attr_##_name \
> + = REG_ATTR(_name, _mode, _show, _store, _reg, _max)
> +
> +#define LM3533_REG_ATTR_RW(_name, _reg, _max) \
> + LM3533_REG_ATTR(_name, S_IRUGO | S_IWUSR, show_lm3533_als_reg, \
> + store_lm3533_als_reg, _reg, _max)
> +
> +#define ALS_THRESH_FALLING_ATTR_RW(_nr) \
> + LM3533_REG_ATTR_RW(in_illuminance_thresh##_nr##_falling_value, \
> + LM3533_REG_ALS_BOUNDARY##_nr##_LOW, LM3533_ALS_BOUNDARY_MAX)
> +
> +#define ALS_THRESH_RAISING_ATTR_RW(_nr) \
> + LM3533_REG_ATTR_RW(in_illuminance_thresh##_nr##_raising_value, \
> + LM3533_REG_ALS_BOUNDARY##_nr##_HIGH, LM3533_ALS_BOUNDARY_MAX)
> +
> +/* ALS Zone thresholds (boundaries)
> + *
> + * in_illuminance_thresh[0-3]_falling_value 0-255
> + * in_illuminance_thresh[0-3]_raising_value 0-255
> + */
> +static ALS_THRESH_FALLING_ATTR_RW(0);
> +static ALS_THRESH_FALLING_ATTR_RW(1);
> +static ALS_THRESH_FALLING_ATTR_RW(2);
> +static ALS_THRESH_FALLING_ATTR_RW(3);
> +
> +static ALS_THRESH_RAISING_ATTR_RW(0);
> +static ALS_THRESH_RAISING_ATTR_RW(1);
> +static ALS_THRESH_RAISING_ATTR_RW(2);
> +static ALS_THRESH_RAISING_ATTR_RW(3);
> +
> +#define LM3533_ALS_ATTR_RO(_name) \
> + DEVICE_ATTR(in_illuminance_##_name, S_IRUGO, show_##_name, NULL)
> +#define LM3533_ALS_ATTR_RW(_name) \
> + DEVICE_ATTR(in_illuminance_##_name, S_IRUGO | S_IWUSR , \
> + show_##_name, store_##_name)
> +
> +/* ALS Zone threshold-event enable
> + *
> + * in_illuminance_thresh_either_en 0,1
> + */
> +static LM3533_ALS_ATTR_RW(thresh_either_en);
> +
> +/* ALS Current Zone
> + *
> + * in_illuminance_zone 0-4
> + */
> +static LM3533_ALS_ATTR_RO(zone);
> +
> +#define ALS_TARGET_ATTR_RW(_mapper, _nr) \
> + LM3533_REG_ATTR_RW(target##_mapper##_##_nr, \
> + LM3533_REG_ALS_M##_mapper##_TARGET_##_nr, LM3533_ALS_TARGET_MAX)
> +
> +/* ALS Mapper targets
> + *
> + * target[1-3]_[0-4] 0-255
> + */
> +static ALS_TARGET_ATTR_RW(1, 0);
> +static ALS_TARGET_ATTR_RW(1, 1);
> +static ALS_TARGET_ATTR_RW(1, 2);
> +static ALS_TARGET_ATTR_RW(1, 3);
> +static ALS_TARGET_ATTR_RW(1, 4);
> +
> +static ALS_TARGET_ATTR_RW(2, 0);
> +static ALS_TARGET_ATTR_RW(2, 1);
> +static ALS_TARGET_ATTR_RW(2, 2);
> +static ALS_TARGET_ATTR_RW(2, 3);
> +static ALS_TARGET_ATTR_RW(2, 4);
> +
> +static ALS_TARGET_ATTR_RW(3, 0);
> +static ALS_TARGET_ATTR_RW(3, 1);
> +static ALS_TARGET_ATTR_RW(3, 2);
> +static ALS_TARGET_ATTR_RW(3, 3);
> +static ALS_TARGET_ATTR_RW(3, 4);
> +
> +/* ALS Gain resistor setting
> + *
> + * gain 0-127
> + */
> +static LM3533_REG_ATTR_RW(gain, LM3533_REG_ALS_RESISTOR_SELECT,
> + LM3533_ALS_RESISTOR_MAX);
> +
> +static struct attribute *lm3533_als_event_attributes[] = {
> + &dev_attr_in_illuminance_thresh_either_en.attr,
> + &lm3533_dev_attr_in_illuminance_thresh0_falling_value.dev_attr.attr,
> + &lm3533_dev_attr_in_illuminance_thresh0_raising_value.dev_attr.attr,
> + &lm3533_dev_attr_in_illuminance_thresh1_falling_value.dev_attr.attr,
> + &lm3533_dev_attr_in_illuminance_thresh1_raising_value.dev_attr.attr,
> + &lm3533_dev_attr_in_illuminance_thresh2_falling_value.dev_attr.attr,
> + &lm3533_dev_attr_in_illuminance_thresh2_raising_value.dev_attr.attr,
> + &lm3533_dev_attr_in_illuminance_thresh3_falling_value.dev_attr.attr,
> + &lm3533_dev_attr_in_illuminance_thresh3_raising_value.dev_attr.attr,
> + NULL
> +};
> +
> +static struct attribute_group lm3533_als_event_attribute_group = {
> + .attrs = lm3533_als_event_attributes
> +};
> +
> +static struct attribute *lm3533_als_attributes[] = {
> + &lm3533_dev_attr_target1_0.dev_attr.attr,
> + &lm3533_dev_attr_target1_1.dev_attr.attr,
> + &lm3533_dev_attr_target1_2.dev_attr.attr,
> + &lm3533_dev_attr_target1_3.dev_attr.attr,
> + &lm3533_dev_attr_target1_4.dev_attr.attr,
> + &lm3533_dev_attr_target2_0.dev_attr.attr,
> + &lm3533_dev_attr_target2_1.dev_attr.attr,
> + &lm3533_dev_attr_target2_2.dev_attr.attr,
> + &lm3533_dev_attr_target2_3.dev_attr.attr,
> + &lm3533_dev_attr_target2_4.dev_attr.attr,
> + &lm3533_dev_attr_target3_0.dev_attr.attr,
> + &lm3533_dev_attr_target3_1.dev_attr.attr,
> + &lm3533_dev_attr_target3_2.dev_attr.attr,
> + &lm3533_dev_attr_target3_3.dev_attr.attr,
> + &lm3533_dev_attr_target3_4.dev_attr.attr,
> + &lm3533_dev_attr_gain.dev_attr.attr,
> + &dev_attr_in_illuminance_zone.attr,
> + NULL
> +};
> +
> +static struct attribute_group lm3533_als_attribute_group = {
> + .attrs = lm3533_als_attributes
> +};
> +
> +static int __devinit lm3533_als_set_input_mode(struct lm3533 *lm3533,
> + int pwm_mode)
> +{
> + u8 mask = LM3533_ALS_INPUT_MODE_MASK;
> + u8 val;
> + int ret;
> +
> + if (pwm_mode)
> + val = mask; /* pwm input */
> + else
> + val = 0; /* analog input */
> +
> + ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF, mask, mask);
> + if (ret) {
> + dev_err(lm3533->dev,
> + "failed to set input mode %d\n", pwm_mode);
> + }
> +
> + return ret;
> +}
> +
> +static int __devinit lm3533_als_enable(struct lm3533 *lm3533)
> +{
> + u8 mask = LM3533_ALS_ENABLE_MASK;
> + int ret;
> +
> + ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF, mask, mask);
> + if (ret)
> + dev_err(lm3533->dev, "failed to enable ALS\n");
> +
> + return ret;
> +}
> +
> +static int lm3533_als_disable(struct lm3533 *lm3533)
> +{
> + u8 mask = LM3533_ALS_ENABLE_MASK;
> + int ret;
> +
> + ret = lm3533_update(lm3533, LM3533_REG_ALS_CONF, 0, mask);
> + if (ret)
> + dev_err(lm3533->dev, "failed to disable ALS\n");
> +
> + return ret;
> +}
> +
> +static const struct iio_info lm3533_als_info = {
> + .attrs =&lm3533_als_attribute_group,
> + .event_attrs =&lm3533_als_event_attribute_group,
> + .driver_module = THIS_MODULE,
> + .read_raw =&lm3533_als_read_raw,
> +};
> +
> +static int __devinit lm3533_als_probe(struct platform_device *pdev)
> +{
> + struct lm3533 *lm3533;
> + struct lm3533_als_platform_data *pdata;
> + struct lm3533_als *als;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + dev_dbg(&pdev->dev, "%s\n", __func__);
> +
> + lm3533 = dev_get_drvdata(pdev->dev.parent);
> + if (!lm3533)
> + return -EINVAL;
> +
> + pdata = pdev->dev.platform_data;
> + if (!pdata) {
> + dev_err(&pdev->dev, "no platform data\n");
> + return -EINVAL;
> + }
> +
> + indio_dev = iio_allocate_device(sizeof(*als));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + indio_dev->info =&lm3533_als_info;
> + indio_dev->channels = lm3533_als_channels;
> + indio_dev->num_channels = ARRAY_SIZE(lm3533_als_channels);
> + indio_dev->name = "lm3533-als";
> + indio_dev->dev.parent = pdev->dev.parent;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + als = iio_priv(indio_dev);
> + als->lm3533 = lm3533;
> + als->irq = lm3533->irq;
> + atomic_set(&als->zone, 0);
> +
> + if (als->irq) {
> + ret = request_threaded_irq(als->irq, NULL, lm3533_als_isr,
> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> + indio_dev->name, indio_dev);
> + if (ret) {
> + dev_err(&indio_dev->dev, "failed to request irq %d\n",
> + lm3533->irq);
> + goto err_free;
> + }
> + }
> +
> + platform_set_drvdata(pdev, indio_dev);
> +
> + ret = lm3533_als_set_input_mode(lm3533, pdata->pwm_mode);
> + if (ret)
> + goto err_free;
> +
> + ret = lm3533_als_enable(lm3533);
> + if (ret)
> + goto err_free;
> +
> + ret = iio_device_register(indio_dev);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register ALS\n");
> + goto err_disable;
> + }
> +
> + return 0;
> +
> +err_disable:
> + lm3533_als_disable(lm3533);
> +err_free:
> + iio_free_device(indio_dev);
> +
> + return ret;
> +}
> +
> +static int __devexit lm3533_als_remove(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct lm3533_als *als = iio_priv(indio_dev);
> +
> + dev_dbg(&pdev->dev, "%s\n", __func__);
> +
> + iio_device_unregister(indio_dev);
> + lm3533_als_disable(als->lm3533);
> + if (als->irq)
> + free_irq(als->irq, indio_dev);
> + iio_free_device(indio_dev);
> +
> + return 0;
> +}
> +
> +static struct platform_driver lm3533_als_driver = {
> + .driver = {
> + .name = "lm3533-als",
> + .owner = THIS_MODULE,
> + },
> + .probe = lm3533_als_probe,
> + .remove = __devexit_p(lm3533_als_remove),
> +};
> +module_platform_driver(lm3533_als_driver);
> +
> +MODULE_AUTHOR("Johan Hovold<[email protected]>");
> +MODULE_DESCRIPTION("LM3533 Ambient Light Sensor driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:lm3533-als");

2012-05-03 11:51:06

by Johan Hovold

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] leds: add LM3533 LED driver

On Thu, May 03, 2012 at 11:43:44AM +0100, Mark Brown wrote:
> On Thu, May 03, 2012 at 12:26:38PM +0200, Johan Hovold wrote:
>
> > +What: /sys/class/leds/<led>/risetime
> > +Date: April 2012
> > +KernelVersion: 3.5
> > +Contact: Johan Hovold <[email protected]>
> > +Description:
> > + Set the pattern generator fall and rise times (0..7), where
> > +
> > + 0 - 2048 us
> > + 1 - 262 ms
> > + 2 - 524 ms
> > + 3 - 1.049 s
> > + 4 - 2.097 s
> > + 5 - 4.194 s
> > + 6 - 8.389 s
> > + 7 - 16.78 s
> > +
>
> Shouldn't these be controlled by led_blink_set() rather than a custom
> ABI?

led_blink_set controls the on/off times, but the LM3533 has the two
additional rise and fall-time settings which determine the transition
time between these states.

> > +What: /sys/class/leds/<led>/id
> > +Date: April 2012
> > +KernelVersion: 3.5
> > +Contact: Johan Hovold <[email protected]>
> > +Description:
> > + Get the id of this led (0..3).
> > +
>
> This should just be a generic LED subsystem thing?

It's related to the output mapping discussed in my previous mail. The
four logical LEDs (0..3) can be used to control either (or all) of the
five low-voltage output. This attribute provides the identity of the
class devices (logical LEDs) which can then be used in the output
mapping (done in the parent device). These id's have been chosen to
correspond to the MFD-id's, but are really device specific.

> > +What: /sys/class/leds/<led>/max_current
> > +Date: April 2012
> > +KernelVersion: 3.5
> > +Contact: Johan Hovold <[email protected]>
> > +Description:
> > + Set the full-scale current I_{LED_FULLSCALE} (0..31), where
> > +
> > + I_{LED_FULLSCALE} = 5mA + max_current * 0.8mA
> > +
>
> Shouldn't this be set by platform data, the maximum current you can push
> through the LEDs seems like a board dependant thing which won't change
> dynamically at runtime. The brightness can already be varied.

I fully agree and it is possible to set via the platform data for that
reason. The end-customer, however, insisted that even this setting be
available through sysfs to facilitate their integration and testing.

I'd be willing drop this attribute if requested, as it would only be used
during integration and could easily be added back by the end-customer if
needed.

> It'd also be nicer if the kernel did the calculation for the user.

If it was something that was going to be changed a lot, then yes,
perhaps. But as you point out above, this is generally a fixed setting
set using platform data once by integrators that will have access to
the datasheet and it's current table.

Thanks,
Johan

2012-05-03 14:51:15

On Thu, May 03, 2012 at 05:00:40PM +0200, Johan Hovold wrote:
> On Thu, May 03, 2012 at 12:38:02PM +0100, Mark Brown wrote:

> > This is one of the reasons why we currently use tracepoints (they just
> > don't have this issue as they're trivial to filter), though
> > adding some sort of infrastructure for it ought not to be too difficult
> > even if it's just at the regmap level.

> So a /sys/kernel/debug/regmap/<device>/io_printk attribute (with a
> better name) to enable debug printks in io paths
> (regmap*{read,write,update} outside of mutex) in regmap.c would be
> acceptable?

Yes, that'd be totally fine for me - it's debugfs so we can always drop
it later if someone comes up with a better idea or something.

> > Actually, the other question I had but forgot to ask (or I think punted
> > on for your response) was why these are in sysfs at all - things like
> > which things are connected to the backlight are going to be a property
> > of the board design so should be defined by the machine not tweaked from
> > userspace.

> I agree with you and the reason is the same as for the max_current
> attribute (discussed in the other thread) -- it was an explicit request
> from the end customer.

> I could replace the boost attributes with a platform_data entry where it
> really belongs.

I really think this is much better for mainline.

> There is a use case (beyond testing/integration) for keeping the (lvled)
> outputs configurable from userspace, in that it provides a way to
> synchronise LED activity such as blinking. So I still want to keep those,
> at least for the lvleds.

I'm not sure exactly which control that is?

Attachments:

(No filename) (1.61 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2012-05-03 16:36:28

by Johan Hovold

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] iio: add LM3533 ambient light sensor driver

On Thu, May 03, 2012 at 12:40:10PM +0100, Jonathan Cameron wrote:
> On 5/3/2012 11:26 AM, Johan Hovold wrote:
> > Add sub-driver for the ambient light sensor interface on National
> > Semiconductor / TI LM3533 lighting power chips.
> >
> > The sensor interface can be used to control the LEDs and backlights of
> > the chip through defining five light zones and three sets of
> > corresponding brightness target levels.
> >
> > The driver provides raw and mean adc readings along with the current
> > light zone through sysfs. A threshold event can be generated on zone
> > changes.
> Code is fine. Pretty much all my comments are to do with the interface.

[...]

> > diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als
> > new file mode 100644
> > index 0000000..9849d14
> > --- /dev/null
> > +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als
> > @@ -0,0 +1,62 @@
> > +What: /sys/bus/iio/devices/iio:deviceX/gain
> > +Date: April 2012
> > +KernelVersion: 3.5
> > +Contact: Johan Hovold<[email protected]>
> > +Description:
> > + Set the ALS gain-resistor setting (0..127) for analog input
> > + mode, where
> > +
> > + 0000000 - ALS input is high impedance
> > + 0000001 - 200kOhm (10uA at 2V full-scale)
> > + 0000010 - 100kOhm (20uA at 2V full-scale)
> > + ...
> > + 1111110 - 1.587kOhm (1.26mA at 2V full-scale)
> > + 1111111 - 1.575kOhm (1.27mA at 2V full-scale)
> > +
> > + R_als = 2V / (10uA * gain) (gain> 0)
> Firstly, no magic numbers. These are definitely magic.

Not that magic as they're clearly documented (in code and public
datasheets), right? What would you prefer instead?

> Secondly see
> in_illuminance0_scale for a suitable existing attribute.

I didn't consider scale to be appropriate given the following
documentation (e.g, for in_voltageY_scale):

"If known for a device, scale to be applied to <type>Y[_name]_raw post
addition of <type>[Y][_name]_offset in order to obtain the measured
value in <type> units as specified in <type>[Y][_name]_raw
documentation."

That is, the gain setting has nothing to do with scaling the raw adc
reading to SI-units or such, it's simply a setup dependent gain setting
(which affects the raw readings as well). [And as such, should probably
go into to the platform data eventually as well.]

> > +
> > +What: /sys/bus/iio/devices/iio:deviceX/illuminance_zone
> > +Date: April 2012
> > +KernelVersion: 3.5
> > +Contact: Johan Hovold<[email protected]>
> > +Description:
> > + Get the current light zone (0..4) as defined by the
> > + in_illuminance_thresh[n]_{falling,rising} thresholds.
> Hmm.. definitely have an in prefix, beyond that I'm not sure what the
> cleanest

Thanks for catching this, it's a typo in the sysfs document -- the in_
prefix is in the code.

> interface will be for this. Could extend the event codes to deal with the
> zone index. Slightly tricky as the channel could already be modified so
> chan2 isn't necesarily available.
>
> > +
> > +What: /sys/.../events/in_illuminance_thresh_either_en
> > +Date: April 2012
> > +KernelVersion: 3.5
> > +Contact: Johan Hovold<[email protected]>
> > +Description:
> > + Event generated when channel passes one of the four threshold
> > + in either direction (rising|falling) and a zone change occurs.
> > + The corresponding light zone can be read from
> > + illuminance_zone.
> > +
> > +What: /sys/.../events/illuminance_thresh0_falling_value
> hmm.. every time you think you are making progress a new and exciting
> device comes
> along requiring the abi to be extended.

Exciting isn't it. :)

> in_illuminanceX_threshY_rising_value
> in_illuminanceX_threshY_falling_value
> should do with appropriate description.

Ok.

> > +What: /sys/.../events/illuminance_thresh0_raising_value
> > +What: /sys/.../events/illuminance_thresh1_falling_value
> > +What: /sys/.../events/illuminance_thresh1_raising_value
> > +What: /sys/.../events/illuminance_thresh2_falling_value
> > +What: /sys/.../events/illuminance_thresh2_raising_value
> > +What: /sys/.../events/illuminance_thresh3_falling_value
> > +What: /sys/.../events/illuminance_thresh3_raising_value
> > +Date: April 2012
> > +KernelVersion: 3.5
> > +Contact: Johan Hovold<[email protected]>
> > +Description:
> > + Specifies the value of threshold that the device is comparing
> > + against for the events enabled by
> > + in_illuminance_thresh_either_en, and defines the
> > + the five light zones.
> > +
> > + These thresholds correspond to the eight zone-boundary
> > + registers (boundary[n]_{low,high}).
> > +
> This interface is going to take some thought. We have
> in_illuminance0_target at the
> moment, so I guess we can add a zoning concept to that...

But target isn't really related, as far as I understand. That's another
calibration setting right? While zone is derived from the average adc
readings. (More below.)

> > +What: /sys/bus/iio/devices/iio:deviceX/target[m]_[n]
> > +Date: April 2012
> > +KernelVersion: 3.5
> > +Contact: Johan Hovold<[email protected]>
> > +Description:
> > + Set the target brightness for ALS-mapper m in light zone n
> > + (0..255), where m in 1..3 and n in 0..4.
> Don't suppose you could do a quick summary of what these zones are and
> why there
> are 3 ALS-mappers? I'm not getting terribly far on a quick look at the
> datasheet!

Of course. The average adc readings are mapped to five light zones using
eight zone boundary registers (4 boundaries with hysteresis) and a set
of rules.

To simplify somewhat (by ignoring some of the rules): If the average
adc input drops below boundary0_low, the zone register reads 0; if it
drops below boundary1_low, it reads 1, and so on. If the input it
increases over boundary3_high, the zone register return 4; if it
increases passed boundary2_high, it returns zone 3, etc.

That is, roughly something like (we get 8-bits of input from the ADC):

zone 0

boundary0_low 51
boundary0_high 53

zone 1

boundary1_low 102
boundary1_high 106

zone 2

boundary2_low 153
boundary2_high 161

zone 3

boundary3_low 204
boundary3_high 220

zone 4

[ Figure 6 on page 20 in the datasheets should make it clear. ]

The ALS interface and it's zone concept can then be used to control the
LEDs and backlights of the chip, by determining the target brightness for
each zone, e.g., set brightness to 52 when in zone 0.

To complicate things further (and it is complicated), there are three
such sets of target brightness values: ALSM1, ALSM2, ALSM3.

So for each LED or backlight you can set ALS-input control mode, by
saying that the device should get it's brightness levels from target set
1, 2, or 3.

[ And it gets even more complicated, as ALSM1 can only control
backlight0, where as ALSM2 and ALSM3 can control any of the remaining
devices, but that's irrelevant here. ]

Initially, I thought this interface to be too esoteric to be worth
generalising, but it sort of fits with event thresholds so I gave it a
try. The biggest conceptual problem, I think, is that the zone
boundaries can be used to control the other devices, even when the event
is not enabled (or even an irq line not configured). That is, I find it
a bit awkward that the event thresholds also defines the zones (a sort of
discrete scaling factor).

Perhaps simply keeping the attributes outside of events (e.g. named
boundary[n]_{low,high}) and having a custom event enabled (e.g.
in_illuminance_zone_change_en) is the best solution?

[...]

> > diff --git a/drivers/staging/iio/light/lm3533-als.c b/drivers/staging/iio/light/lm3533-als.c
> > new file mode 100644
> > index 0000000..e2c9be6
> > --- /dev/null
> > +++ b/drivers/staging/iio/light/lm3533-als.c
> > @@ -0,0 +1,617 @@
> > +/*
> > + * lm3533-als.c -- LM3533 Ambient Light Sensor driver
> > + *
> > + * Copyright (C) 2011-2012 Texas Instruments
> > + *
> > + * Author: Johan Hovold<[email protected]>
> > + *
> > + * 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.
> > + */
> > +
> > +#include<linux/atomic.h>
> > +#include<linux/fs.h>
> > +#include<linux/interrupt.h>
> > +#include<linux/io.h>
> > +#include<linux/module.h>
> > +#include<linux/mfd/core.h>
> > +#include<linux/platform_device.h>
> > +#include<linux/slab.h>
> > +#include<linux/uaccess.h>
> > +
> > +#include<linux/mfd/lm3533.h>
> > +
> > +#include "../events.h"
> > +#include "../iio.h"
> This will need to go through the staging-next tree. In there these
> headers have moved.

I'm aware of the move. Should the different sub-drivers go in through
each tree respectively? Right now the four patches are all against rc5.

[...]

> > +static const struct iio_chan_spec lm3533_als_channels[] = {
> > + {
> > + .type = IIO_LIGHT,
> > + .info_mask = IIO_CHAN_INFO_AVERAGE_RAW_SEPARATE_BIT,
> > + .channel = 0,
> channel doesn't get used unless you also set indexed = 1.

So, you mean I could drop channel as well? Or should I add indexed, as I
use channel 0 when reporting the event?

[...]

> > +static int lm3533_als_set_int_mode(struct iio_dev *indio_dev, int enable)
> > +{
> > + struct lm3533_als *als = iio_priv(indio_dev);
> > + u8 mask = LM3533_ALS_INT_ENABLE_MASK;
> > + u8 val;
> > + int ret;
> > +
> > + if (enable)
> > + val = mask;
> > + else
> > + val = 0;
> > +
> > + ret = lm3533_update(als->lm3533, LM3533_REG_ALS_ZONE_INFO, val, mask);
> > + if (ret) {
> > + dev_err(&indio_dev->dev, "failed to set int mode %d\n",
> > + enable);
> extra brackets.

I prefer the brackets for multi-line (single) statements even though
they are not required. (Especially if the single statement spans
several lines -- but I try to be consistent.) If you have a strong
opinion about this, I'll drop them.

> > + }
> > +
> > + return ret;
> > +}
> > +

Thanks,
Johan

2012-05-03 16:46:48

by Johan Hovold

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] leds: add LM3533 LED driver

On Thu, May 03, 2012 at 03:51:08PM +0100, Mark Brown wrote:
> On Thu, May 03, 2012 at 01:50:59PM +0200, Johan Hovold wrote:
> > On Thu, May 03, 2012 at 11:43:44AM +0100, Mark Brown wrote:
>
> > > > + 5 - 4.194 s
> > > > + 6 - 8.389 s
> > > > + 7 - 16.78 s
>
> > > Shouldn't these be controlled by led_blink_set() rather than a custom
> > > ABI?
>
> > led_blink_set controls the on/off times, but the LM3533 has the two
> > additional rise and fall-time settings which determine the transition
> > time between these states.
>
> Hrm. In that case these rise times are very large - I'd expect them to
> cause issues with led_set_blink() users?

They are. The default settings (as fast a transition as possible) will
probably what most people use, and if they start fiddling with the
transition times they probably know what they're doing.

> Though actually I suspect the
> solution here is to pull these out into the framework later; we can
> probably simulate reasonably in software with a lot of brightness
> variable LEDs.

Ok.

> > > > +What: /sys/class/leds/<led>/max_current
>
> > > Shouldn't this be set by platform data, the maximum current you can push
> > > through the LEDs seems like a board dependant thing which won't change
> > > dynamically at runtime. The brightness can already be varied.
>
> > I fully agree and it is possible to set via the platform data for that
> > reason. The end-customer, however, insisted that even this setting be
> > available through sysfs to facilitate their integration and testing.
>
> > I'd be willing drop this attribute if requested, as it would only be used
> > during integration and could easily be added back by the end-customer if
> > needed.
>
> I'd strongly suggest removing this for mainline. If it's present it
> should at least be limited to the maximum specified in platform data
> (just for safety if nothing else).

Agreed.

Thanks,
Johan

2012-05-03 16:54:43

by Johan Hovold

[permalink] [raw]

Subject: Re: [PATCH v2 1/4] mfd: add LM3533 lighting-power core driver

On Thu, May 03, 2012 at 04:24:07PM +0100, Mark Brown wrote:
> On Thu, May 03, 2012 at 05:00:40PM +0200, Johan Hovold wrote:
> > On Thu, May 03, 2012 at 12:38:02PM +0100, Mark Brown wrote:
>
> > > This is one of the reasons why we currently use tracepoints (they just
> > > don't have this issue as they're trivial to filter), though
> > > adding some sort of infrastructure for it ought not to be too difficult
> > > even if it's just at the regmap level.
>
> > So a /sys/kernel/debug/regmap/<device>/io_printk attribute (with a
> > better name) to enable debug printks in io paths
> > (regmap*{read,write,update} outside of mutex) in regmap.c would be
> > acceptable?
>
> Yes, that'd be totally fine for me - it's debugfs so we can always drop
> it later if someone comes up with a better idea or something.

Ok. I'll have a look at this next week (will be on the road for a few
days), and drop the dev_dbg from the lm3533 io-functions for now.

> > > Actually, the other question I had but forgot to ask (or I think punted
> > > on for your response) was why these are in sysfs at all - things like
> > > which things are connected to the backlight are going to be a property
> > > of the board design so should be defined by the machine not tweaked from
> > > userspace.
>
> > I agree with you and the reason is the same as for the max_current
> > attribute (discussed in the other thread) -- it was an explicit request
> > from the end customer.
>
> > I could replace the boost attributes with a platform_data entry where it
> > really belongs.
>
> I really think this is much better for mainline.

Agreed.

> > There is a use case (beyond testing/integration) for keeping the (lvled)
> > outputs configurable from userspace, in that it provides a way to
> > synchronise LED activity such as blinking. So I still want to keep those,
> > at least for the lvleds.
>
> I'm not sure exactly which control that is?

That would be the output_lvled[n] (n = 1..5) attributes. For example, to
have all five low-voltage sinks blink synchronously, you could assign 0
to all these five attributes, and set a timer trigger for the led device
which has id 0.

Thanks,
Johan

2012-05-03 16:57:55