Re: [PATCH v4] iio: add LM3533 ambient-light-sensor driver
From: Jonathan Cameron
Date: Sat May 19 2012 - 13:19:35 EST
On 05/19/2012 05:30 PM, Johan Hovold wrote:
> On Sat, May 19, 2012 at 09:48:23AM +0100, Jonathan Cameron wrote:
>> On 05/18/2012 02:07 PM, 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.
>>
>> Hi Johan,
>>
>> I hate to be a pain with this one, but it's a complex beast and I'd
>> really like to get the interface right first time - particularly as
>> you are going in after the move out of staging.
>>
>>
>> Queries for you.
>> 1) Ordering in the probe function. Normally expect iio_device_register
>> to be the last call. Why not here?
>> 2) Worth combining enable / disable into one as very similar functions?
>> 3) Suspicious code in als_set_input_mode
>
> Good points. See my answers inline below.
>
>> Naming of the target values. I think we can make the naming of these
>> fit in much better with the normal abi which is going to be all for the
>> good in the long run. They are basically current output channels
>> with a controllable set of steps (where we don't have direct control
>> of which one we are in). This is very similar to the frequency controls
>> on some of Analog's dds that we have a well defined interface for.
>>
>> More detail below, but in summary.
>>
>> out_currentX_currentY_raw for channel X value for zone Y.
>
> Interesting. I'll have to think this through and get back to you.
>
> Thanks for looking at this so quickly!
>
> Johan
>
>> Jonathan
>>>
>>> Signed-off-by: Johan Hovold <jhovold@xxxxxxxxx>
>>> ---
>>>
>>> Note that addition of r_select to the platform data probably needs to go
>>> in via mfd.
>>>
>>>
>>> v2:
>>> - reimplement using iio
>>> - add sysfs-ABI documentation
>>> v3
>>> - use indexed channel
>>> - fix sysfs-ABI documentation typo and style
>>> - replace gain attribute with in_illuminance0_calibscale
>>> - add calibscale to platform data
>>> - fix adc register definitions
>>> - replace to_lm3533_dev_attr macro with inline function
>>> - fix device used for error reporting at irq allocation
>>> - use iio device for error reporting during setup/enable
>>> - rebase on staging-next
>>> - fix header include paths
>>> - use dev_to_iio_dev
>>> - add IIO_CHAN_INFO_RAW to info mask
>>> - use iio_device_{alloc,free}
>>> v4
>>> - move to driver/iio/light
>>> - add events/in_illuminance0_threshY_hysteresis attributes
>>> - fix device zone-boundary quirkiness
>>> - clean up attribute handling
>>> - replace calibscale with device-specific r_select attribute
>>>
>>>
>>> .../ABI/testing/sysfs-bus-iio-light-lm3533-als | 64 ++
>>> drivers/iio/Kconfig | 1 +
>>> drivers/iio/Makefile | 1 +
>>> drivers/iio/light/Kconfig | 22 +
>>> drivers/iio/light/Makefile | 5 +
>>> drivers/iio/light/lm3533-als.c | 941 ++++++++++++++++++++
>>> include/linux/mfd/lm3533.h | 1 +
>>> 7 files changed, 1035 insertions(+), 0 deletions(-)
>>> create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-light-lm3533-als
>>> create mode 100644 drivers/iio/light/Kconfig
>>> create mode 100644 drivers/iio/light/Makefile
>>> create mode 100644 drivers/iio/light/lm3533-als.c
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-light-lm3533-als b/Documentation/ABI/testing/sysfs-bus-iio-light-lm3533-als
>>> new file mode 100644
>>> index 0000000..7ea1770
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-light-lm3533-als
>>> @@ -0,0 +1,64 @@
>>> +What: /sys/bus/iio/devices/iio:deviceX/r_select
>>> +Date: April 2012
>>> +KernelVersion: 3.5
>>> +Contact: Johan Hovold <jhovold@xxxxxxxxx>
>>> +Description:
>>> + Set the ALS internal pull-down resistor for analog input mode
>>> + (1..127), such that,
>>> +
>>> + R_als = 200000 / r_select (ohm)
>>> +
>>> + This setting is ignored in PWM-mode (input is always high
>>> + impedance in PWM-mode).
>>> +
>>> +What: /sys/.../events/in_illuminance0_thresh_either_en
>>> +Date: April 2012
>>> +KernelVersion: 3.5
>>> +Contact: Johan Hovold <jhovold@xxxxxxxxx>
>>> +Description:
>>> + Event generated when channel passes one of the four thresholds
>>> + in each direction (rising|falling) and a zone change occurs.
>>> + The corresponding light zone can be read from
>>> + in_illuminance0_zone.
>>> +
>>> +What: /sys/.../events/in_illuminance0_threshY_hysteresis
>>> +Date: May 2012
>>> +KernelVersion: 3.5
>>> +Contact: Johan Hovold <jhovold@xxxxxxxxx>
>>> +Description:
>>> + Get the hysteresis for thresholds Y, that is,
>>> +
>>> + threshY_hysteresis = threshY_raising - threshY_falling
>>> +
>>> +What: /sys/.../events/illuminance_threshY_falling_value
>>> +What: /sys/.../events/illuminance_threshY_raising_value
>>> +Date: April 2012
>>> +KernelVersion: 3.5
>>> +Contact: Johan Hovold <jhovold@xxxxxxxxx>
>>> +Description:
>>> + Specifies the value of threshold that the device is
>>> + comparing against for the events enabled by
>>> + in_illuminance0_thresh_either_en, where Y in 0..3.
>>> +
>>> + Note that threshY_falling must be less than or equal to
>>> + threshY_raising.
>>> +
>>> + These thresholds correspond to the eight zone-boundary
>>> + registers (boundaryY_{low,high}) and defines the five light
>>> + zones.
>>> +
>>> +What: /sys/bus/iio/devices/iio:deviceX/in_illuminance0_zone
>>> +Date: April 2012
>>> +KernelVersion: 3.5
>>> +Contact: Johan Hovold <jhovold@xxxxxxxxx>
>>> +Description:
>>> + Get the current light zone (0..4) as defined by the
>>> + in_illuminance0_threshY_{falling,rising} thresholds.
>>> +
>>> +What: /sys/bus/iio/devices/iio:deviceX/targetY_Z
>>> +Date: April 2012
>>> +KernelVersion: 3.5
>>> +Contact: Johan Hovold <jhovold@xxxxxxxxx>
>>> +Description:
>>> + Set the target brightness for ALS-mapper Y in light zone Z
>>> + (0..255), where Y in 1..3 and Z in 0..4.
>>
>> What are the units of this? Also arguably is it not the als that this
>> is related to, but rather the light source? A quick datasheet browse says
>> that these are current targets? If so I wonder if we can make that
>> explicit... Could treat them as 3 output channels and have indexed values
>> like we do for frequencies in dds devices (where external hardware is
>> controlling them.
>>
>> Hmm. lets see.
>>
>> out_currentX_currentY_raw
>> (the double naming is a bit clunky but corresponds to frequency devices
>> where we have
>> out_altvoltageX_frequencyY_raw
>>
>> Hence we'd treat you 3 mapers as indexed channels 0,1,2 and the zones
>> as indexed values they can take 0,1,2,3,4
>> out_currentX_raw is not read only and gives you the current for whichever
>> zone the device is currently in.
>>
>> This may seem convoluted but I'd really rather have something generalizable
>> for this if we possibly can. We'd still need documentation to say what is
>> controlling these, but at least they'd fit within our more general abi.
>>
>> What do you think?
>
> I'll get back to you on this shortly.
>
> [...]
>
>>> +struct lm3533_als {
>>> + struct lm3533 *lm3533;
>>> +
>>> + unsigned long flags;
>>> + int irq;
>>> +
>> Boolean might be better as it's not a though this will save
>> space!
>
> I've used bit fields consistently for such flags in lm3533, although the
> type below should have been unsigned. The space required is the same
> either way in this case. I'll go with bool.
indeed it should be unsigned. It's fine if you want to keep it
as a bitfield for consistency (as good an arguement as any!)
>
>>> + int pwm_mode:1;
>>> +
>>> + atomic_t zone;
>>> + struct mutex thresh_mutex;
>>> +};
>> Rarely a reason for more than one blank line in my opinion...
>
> Now you're picky. :)
>
> Separating defines, declarations and definitions using an additional
> blank line should be pretty uncontroversial. But again, if you insist,
> I'll drop it.
I'm not insisting (hence opinion bit ;)
>
>>> +
>>> +
>> May be roll this into it's one call site. will make for marginally
>> less code I think..
>
> I did before, but separated it out when I added calibscale. I prefer to
> keep it separate for readability reasons (especially if we're going to
> add output channels).
fair enough.
>
>>> +static int lm3533_als_get_adc(struct iio_dev *indio_dev, bool average,
>>> + int *adc)
>>> +{
>>> + struct lm3533_als *als = iio_priv(indio_dev);
>>> + u8 reg;
>>> + u8 val;
>>> + int ret;
>>> +
>>> + if (average)
>>> + reg = LM3533_REG_ALS_READ_ADC_AVERAGE;
>>> + else
>>> + reg = LM3533_REG_ALS_READ_ADC_RAW;
>>> +
>>> + ret = lm3533_read(als->lm3533, reg, &val);
>>> + if (ret) {
>>> + dev_err(&indio_dev->dev, "failed to read adc\n");
>>> + return ret;
>>> + }
>>> +
>>> + *adc = val;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int lm3533_als_read_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan,
>>> + int *val, int *val2, long mask)
>>> +{
>>> + int ret;
>>> +
>>> + switch (mask) {
>>> + case 0:
>>> + ret = lm3533_als_get_adc(indio_dev, false, val);
>>> + break;
>>> + case IIO_CHAN_INFO_AVERAGE_RAW:
>>> + ret = lm3533_als_get_adc(indio_dev, true, val);
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return IIO_VAL_INT;
>>> +}
>>> +
>> Why have an array? Just use the address and set the num_channels = 1;
>
> For consistency. If you ever add channels you'd need to turn it into an
> array again anyway (and we are considering output channels now).
>
> This is a pretty common practise for example in board files.
fair enough
>
>>> +static const struct iio_chan_spec lm3533_als_channels[] = {
>>> + {
>>> + .type = IIO_LIGHT,
>>> + .channel = 0,
>>> + .indexed = 1,
>>> + .info_mask = (IIO_CHAN_INFO_AVERAGE_RAW_SEPARATE_BIT |
>>> + IIO_CHAN_INFO_RAW_SEPARATE_BIT),
>>> + }
>>> +};
>>> +
>>> +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;
>>> +}
>>> +
>> could just roll this into the one call point, it's not exactly complex.
>
> For readability and consistency reasons I'd like to keep it separate. If
> you look at the call point in store_thresh_either_en it really makes
> sense to break this one out. The consistency argument is that most
> register accesses have their dedicated set/get functions.
>
> [ The reordering of probe / remove discussed below will also introduce
> a second call point. ]
>
>>> +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);
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +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;
>>> +}
>>> +
>> Given only accessed from one place, why not just roll it in there?
>
> I've dropped this one completely along with the r_select sysfs attribute
> as it is now write only (from platform data).
>
>>> +static int lm3533_als_get_resistor(struct iio_dev *indio_dev, u8 *val)
>>> +{
>>> + struct lm3533_als *als = iio_priv(indio_dev);
>>> + int ret;
>>> +
>>> + if (als->pwm_mode) {
>>> + *val = 0; /* always high impedance */
>>> + return 0;
>>> + }
>>> +
>>> + ret = lm3533_read(als->lm3533, LM3533_REG_ALS_RESISTOR_SELECT, val);
>>> + if (ret) {
>>> + dev_err(&indio_dev->dev, "failed to get resistor\n");
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int lm3533_als_set_resistor(struct iio_dev *indio_dev, u8 val)
>>> +{
>>> + struct lm3533_als *als = iio_priv(indio_dev);
>>> + int ret;
>>> +
>>> + if (als->pwm_mode)
>>> + return -EPERM; /* always high impedance */
>>> +
>>> + if (val < LM3533_ALS_RESISTOR_MIN || val > LM3533_ALS_RESISTOR_MAX)
>>> + return -EINVAL;
>>> +
>>> + ret = lm3533_write(als->lm3533, LM3533_REG_ALS_RESISTOR_SELECT, val);
>>> + if (ret) {
>>> + dev_err(&indio_dev->dev, "failed to set resistor\n");
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> +}
>
> [...]
>
>>> +static int lm3533_als_set_target(struct iio_dev *indio_dev, unsigned nr,
>>> + unsigned zone, u8 val)
>>> +{
>>> + struct lm3533_als *als = iio_priv(indio_dev);
>>> + u8 reg;
>>> + int ret;
>>> +
>>> + if (nr < LM3533_ALS_MAPPER_MIN || nr > LM3533_ALS_MAPPER_MAX)
>>> + return -EINVAL;
>>> +
>>> + if (zone > LM3533_ALS_ZONE_MAX)
>>> + return -EINVAL;
>>> +
>>> + reg = lm3533_als_get_target_reg(nr, zone);
>>> + ret = lm3533_write(als->lm3533, reg, val);
>> I wouldn't bother with the intermediate but up to you...
>>
>> ret = lm3533_write(als->lm3533, lm3533_als_get_target_reg(nr, zone), val);
>
> I prefer the expanded form.
>
>>> + if (ret)
>>> + dev_err(&indio_dev->dev, "failed to set target brightness\n");
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static inline u8 lm3533_als_get_threshold_reg(unsigned nr, bool raising)
>>> +{
>>> + u8 offset;
>>> +
>> offset = !raising;
>
> Ok.
>
>>> + if (raising)
>>> + offset = 0;
>>> + else
>>> + offset = 1;
>>> +
>>> + return LM3533_REG_ALS_BOUNDARY_BASE + 2 * nr + offset;
>>> +}
>>> +
>>> +static int lm3533_als_get_threshold(struct iio_dev *indio_dev, unsigned nr,
>>> + bool raising, u8 *val)
>>> +{
>>> + struct lm3533_als *als = iio_priv(indio_dev);
>>> + u8 reg;
>>> + int ret;
>>> +
>>> + if (nr > LM3533_ALS_THRESH_MAX)
>>> + return -EINVAL;
>>> +
>>> + reg = lm3533_als_get_threshold_reg(nr, raising);
>>> + ret = lm3533_read(als->lm3533, reg, val);
>>> + if (ret)
>>> + dev_err(&indio_dev->dev, "failed to get threshold\n");
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int lm3533_als_set_threshold(struct iio_dev *indio_dev, unsigned nr,
>>> + bool raising, u8 val)
>>> +{
>>> + struct lm3533_als *als = iio_priv(indio_dev);
>>> + u8 val2;
>>> + u8 reg;
>>> + u8 reg2;
>> u8 val2, reg, reg2; Shorter and still obvious.
>
> I try to avoid doing multiple declarations on a single line, but in this
> case, perhaps
>
> u8 val2;
> u8 reg, reg2;
>
> would be consistent with the rest of the code.
>
>>> + int ret;
>>> +
>>> + if (nr > LM3533_ALS_THRESH_MAX)
>>> + return -EINVAL;
>>> +
>>> + reg = lm3533_als_get_threshold_reg(nr, raising);
>>> + reg2 = lm3533_als_get_threshold_reg(nr, !raising);
>>> +
>>> + mutex_lock(&als->thresh_mutex);
>>> + ret = lm3533_read(als->lm3533, reg2, &val2);
>>> + if (ret) {
>>> + dev_err(&indio_dev->dev, "failed to get threshold\n");
>>> + goto out;
>>> + }
>>> + /*
>>> + * This device does not allow negative hysteresis (in fact, it uses
>>> + * whichever value is smaller as the lower bound) so we need to make
>>> + * sure that thresh_falling <= thresh_raising.
>>> + */
>>> + if ((raising && (val < val2)) || (!raising && (val > val2))) {
>>> + ret = -EINVAL;
>>> + goto out;
>>> + }
>>> +
>>> + ret = lm3533_write(als->lm3533, reg, val);
>>> + if (ret) {
>>> + dev_err(&indio_dev->dev, "failed to set threshold\n");
>>> + goto out;
>>> + }
>>> +out:
>>> + mutex_unlock(&als->thresh_mutex);
>>> +
>>> + return ret;
>>> +}
>
> [...]
>
>>> +static ssize_t store_als_attr(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t len)
>>> +{
>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> + struct lm3533_als_attribute *als_attr = to_lm3533_als_attr(attr);
>>> + u8 val;
>>> + int ret;
>>> +
>>> + if (kstrtou8(buf, 0, &val))
>>> + return -EINVAL;
>>> +
>>> + switch (als_attr->type) {
>>> + case LM3533_ATTR_TYPE_TARGET:
>>> + ret = lm3533_als_set_target(indio_dev, als_attr->val1,
>>> + als_attr->val2, val);
>>> + break;
>>> + case LM3533_ATTR_TYPE_THRESH_FALLING:
>>> + ret = lm3533_als_set_threshold(indio_dev, als_attr->val1,
>>> + false, val);
>>> + break;
>>> + case LM3533_ATTR_TYPE_THRESH_RAISING:
>>> + ret = lm3533_als_set_threshold(indio_dev, als_attr->val1,
>>> + true, val);
>>> + break;
>>> + default:
>> I'd be tempted to drop this. It is easy to verify whether it will occur.
>
> Drop the WARN and simply return -ENXIO, you mean?
yup
>
>>> + WARN(1, "%s - bad attribute type %d\n", __func__,
>>> + als_attr->type);
>>> + ret = -ENXIO;
>>> + }
>>> +
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return len;
>>> +}
>
> [...]
>
>>> +static struct attribute *lm3533_als_event_attributes[] = {
>>> + &dev_attr_in_illuminance0_thresh_either_en.attr,
>>> + &lm3533_als_attr_in_illuminance0_thresh0_falling_value.dev_attr.attr,
>>> + &lm3533_als_attr_in_illuminance0_thresh0_hysteresis.dev_attr.attr,
>> Just to verify the hysteresis applies to bother thresh0_falling and
>> thresh0_rising?
>
> Yes, we have the following situation (just to reiterate):
>
> ...
> zone 1
>
> thresh0_raising 52
>
> thresh0_falling 50
>
> zone 0
>
> If the ALS is in zone 0 and the (8-bit) average input raises above
> thresh0_raising (52) it enters zone 1. The ALS will not re-enter zone 0 until
> the input drops below thresh0_falling (50). The hysteresis is the
> difference between the two thresholds, that is, 52 - 50 = 2 in this
> case.
>
>>> + &lm3533_als_attr_in_illuminance0_thresh0_raising_value.dev_attr.attr,
>>> + &lm3533_als_attr_in_illuminance0_thresh1_falling_value.dev_attr.attr,
>>> + &lm3533_als_attr_in_illuminance0_thresh1_hysteresis.dev_attr.attr,
>>> + &lm3533_als_attr_in_illuminance0_thresh1_raising_value.dev_attr.attr,
>>> + &lm3533_als_attr_in_illuminance0_thresh2_falling_value.dev_attr.attr,
>>> + &lm3533_als_attr_in_illuminance0_thresh2_hysteresis.dev_attr.attr,
>>> + &lm3533_als_attr_in_illuminance0_thresh2_raising_value.dev_attr.attr,
>>> + &lm3533_als_attr_in_illuminance0_thresh3_falling_value.dev_attr.attr,
>>> + &lm3533_als_attr_in_illuminance0_thresh3_hysteresis.dev_attr.attr,
>>> + &lm3533_als_attr_in_illuminance0_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[] = {
>>> + &dev_attr_r_select.attr,
>> Just to keep info in one place. We agreed in other branch
>> of thread that r_select would be done with platform data.
>
> Indeed. I've dropped it from sysfs.
>
>> I wonder if we can make the naming a little clearer for these.. hmm.
>>> + &lm3533_als_attr_target1_0.dev_attr.attr,
>>> + &lm3533_als_attr_target1_1.dev_attr.attr,
>>> + &lm3533_als_attr_target1_2.dev_attr.attr,
>>> + &lm3533_als_attr_target1_3.dev_attr.attr,
>>> + &lm3533_als_attr_target1_4.dev_attr.attr,
>>> + &lm3533_als_attr_target2_0.dev_attr.attr,
>>> + &lm3533_als_attr_target2_1.dev_attr.attr,
>>> + &lm3533_als_attr_target2_2.dev_attr.attr,
>>> + &lm3533_als_attr_target2_3.dev_attr.attr,
>>> + &lm3533_als_attr_target2_4.dev_attr.attr,
>>> + &lm3533_als_attr_target3_0.dev_attr.attr,
>>> + &lm3533_als_attr_target3_1.dev_attr.attr,
>>> + &lm3533_als_attr_target3_2.dev_attr.attr,
>>> + &lm3533_als_attr_target3_3.dev_attr.attr,
>>> + &lm3533_als_attr_target3_4.dev_attr.attr,
>>> + &dev_attr_in_illuminance0_zone.attr,
>>> + NULL
>>> +};
>>> +
>>> +static struct attribute_group lm3533_als_attribute_group = {
>>> + .attrs = lm3533_als_attributes
>>> +};
>>> +
>>> +static int __devinit lm3533_als_set_input_mode(struct iio_dev *indio_dev,
>>> + int pwm_mode)
>>> +{
>>> + struct lm3533_als *als = iio_priv(indio_dev);
>>> + u8 mask = LM3533_ALS_INPUT_MODE_MASK;
>>> + u8 val;
>> Just use mask directly, why introduce val as well
>> (particularly as you don't use it ;)
>>> + int ret;
>>> +
>>> + if (pwm_mode)
>>> + val = mask; /* pwm input */
>>> + else
>>> + val = 0; /* analog input */
>>> +
>> Why have val?
>>> + ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, mask, mask);
>
> This is supposed to read lm3533_update(..., val, mask). Thanks for
> catching this.
>
>>> + if (ret) {
>>> + dev_err(&indio_dev->dev,
>>> + "failed to set input mode %d\n", pwm_mode);
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int __devinit lm3533_als_setup(struct iio_dev *indio_dev,
>>> + struct lm3533_als_platform_data *pdata)
>>> +{
>>> + int ret;
>>> +
>>> + ret = lm3533_als_set_input_mode(indio_dev, pdata->pwm_mode);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /* ALS input is always high impedance in PWM-mode. */
>>> + if (!pdata->pwm_mode) {
>>> + ret = lm3533_als_set_resistor(indio_dev, pdata->r_select);
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>> This enable / disable pair does rather look like it could
>> be combined into one function and save a few lines of repeated
>> code
>
> They could but at the expense of some readability (als_enable(...,
> false) rather than als_disable(...)) and really not much gain in terms
> of fewer line of code as you'd need to add conditionals both for the
> register value and error message.
>
> This is the style I've used in the other drivers so if you don't mind
> terribly, I'd suggest not merging them if only for consistency.
I don't really care. Just like to kill off code duplication.
>
>>> +static int __devinit lm3533_als_enable(struct iio_dev *indio_dev)
>>> +{
>>> + struct lm3533_als *als = iio_priv(indio_dev);
>>> + u8 mask = LM3533_ALS_ENABLE_MASK;
>>> + int ret;
>>> +
>>> + ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, mask, mask);
>>> + if (ret)
>>> + dev_err(&indio_dev->dev, "failed to enable ALS\n");
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int __devexit lm3533_als_disable(struct iio_dev *indio_dev)
>>> +{
>>> + struct lm3533_als *als = iio_priv(indio_dev);
>>> + u8 mask = LM3533_ALS_ENABLE_MASK;
>>> + int ret;
>>> +
>>> + ret = lm3533_update(als->lm3533, LM3533_REG_ALS_CONF, 0, mask);
>>> + if (ret)
>>> + dev_err(&indio_dev->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_device_alloc(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;
>>> + als->pwm_mode = pdata->pwm_mode;
>>> + atomic_set(&als->zone, 0);
>>> + mutex_init(&als->thresh_mutex);
>>> +
>>> + 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(&pdev->dev, "failed to request irq %d\n",
>>> + als->irq);
>>> + goto err_free_dev;
>>> + }
>>> + }
>>> +
>>> + platform_set_drvdata(pdev, indio_dev);
>>> +
>> amongst other things this register does the creation of the
>> sysfs attributes. Is there a race here if a read on one of
>> those occurs before the setup stuff below?
>
> The enable call simply enables the adc so, for example, target or
> threshold values could be updated before. Worst case is that you get a
> zero reading from the adc before the adc is enabled. The zone algorithm
> takes some time to settle either way.
>
>> Normally I'd expect this call to the last one in the probe
>> function. Why did you chose this ordering?
>
> The main reason in this case was to be able to use the iio device for
> error reporting. The setup function called set_resistor which was also
> possible to set from sysfs (and for consistency all error reporting
> after probe is done using the iio device).
fair enough on the error reporting. This is the one fiddly corner.
Personally I verge in the direction of far fewer error messages
partly because I'm forever trying to unify drivers and it's a lot
easier if you don't have lots of error messages about!
>
> But since we've now dropped the r_select attribute, I could use the
> platform device for reporting all errors during setup and make sure the
> iio device is registered last. I'll do this.
>
>>> + ret = iio_device_register(indio_dev);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "failed to register ALS\n");
>>> + goto err_free_irq;
>>> + }
>>> +
>>> + ret = lm3533_als_setup(indio_dev, pdata);
>>> + if (ret)
>>> + goto err_unregister;
>>> +
>>> + ret = lm3533_als_enable(indio_dev);
>>> + if (ret)
>>> + goto err_unregister;
>>> +
>>> + return 0;
>>> +
>>> +err_unregister:
>>> + iio_device_unregister(indio_dev);
>>> +err_free_irq:
>>> + if (als->irq)
>>> + free_irq(als->irq, indio_dev);
>>> +err_free_dev:
>>> + iio_device_free(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__);
>>> +
>>> + lm3533_als_disable(indio_dev);
>>> + iio_device_unregister(indio_dev);
>>> + if (als->irq)
>>> + free_irq(als->irq, indio_dev);
>>> + iio_device_free(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 <jhovold@xxxxxxxxx>");
>>> +MODULE_DESCRIPTION("LM3533 Ambient Light Sensor driver");
>>> +MODULE_LICENSE("GPL");
>>> +MODULE_ALIAS("platform:lm3533-als");
>>> diff --git a/include/linux/mfd/lm3533.h b/include/linux/mfd/lm3533.h
>>> index 9660feb..594bc59 100644
>>> --- a/include/linux/mfd/lm3533.h
>>> +++ b/include/linux/mfd/lm3533.h
>>> @@ -43,6 +43,7 @@ struct lm3533_ctrlbank {
>>>
>>> struct lm3533_als_platform_data {
>>> unsigned pwm_mode:1; /* PWM input mode (default analog) */
>>> + u8 r_select; /* 1 - 127 (ignored in PWM-mode) */
>>> };
>>>
>>> struct lm3533_bl_platform_data {
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/