Message-ID: <4FB79F6C.1020808@kernel.org>
Date:	Sat, 19 May 2012 14:26:04 +0100
From:	Jonathan Cameron <jic23@...nel.org>
To:	Johan Hovold <jhovold@...il.com>
CC:	Jonathan Cameron <jic23@....ac.uk>, Rob Landley <rob@...dley.net>,
	Richard Purdie <rpurdie@...ys.net>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Florian Tobias Schandinat <FlorianSchandinat@....de>,
	Arnd Bergmann <arnd@...db.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-iio@...r.kernel.org
Subject: Re: [PATCH v4] iio: add LM3533 ambient-light-sensor driver

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@...il.com>
>>> ---
>>>
>>> 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@...il.com>
>>> +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@...il.com>
>>> +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@...il.com>
>>> +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@...il.com>
>>> +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@...il.com>
>>> +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@...il.com>
>>> +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@...il.com>");
>>> +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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists