Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753558Ab2ESRTK (ORCPT ); Sat, 19 May 2012 13:19:10 -0400 Received: from saturn.retrosnub.co.uk ([178.18.118.26]:32774 "EHLO saturn.retrosnub.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752030Ab2ESRTF (ORCPT ); Sat, 19 May 2012 13:19:05 -0400 Message-ID: <4FB79F6C.1020808@kernel.org> Date: Sat, 19 May 2012 14:26:04 +0100 From: Jonathan Cameron User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20120430 Thunderbird/12.0.1 MIME-Version: 1.0 To: Johan Hovold CC: Jonathan Cameron , Rob Landley , Richard Purdie , Samuel Ortiz , Greg Kroah-Hartman , Florian Tobias Schandinat , Arnd Bergmann , Andrew Morton , Mark Brown , linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org Subject: Re: [PATCH v4] iio: add LM3533 ambient-light-sensor driver References: <1337100396-29024-1-git-send-email-jhovold@gmail.com> <1337346461-31220-1-git-send-email-jhovold@gmail.com> <4FB75E57.9020003@kernel.org> <20120519163008.GA21033@localhost> In-Reply-To: <20120519163008.GA21033@localhost> X-Enigmail-Version: 1.4.1 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 29064 Lines: 913 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 >>> --- >>> >>> 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 >>> +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 >>> +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 >>> +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 >>> +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 >>> +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 >>> +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 "); >>> +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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/