Re: [PATCH] iio: add support for HMC5883/HMC5883L to HMC5843 driver

[Lars-Peter Clausen <lars@metafoo.de>]

On 05/02/2012 12:57 AM, Peter Meerwald wrote:
> this patch aims at adding support for the HMC5883/HMC5883L magnetometer 
> to the HMC5843 driver; the devices are pretty similar, so the existing driver
> is extended (see also http://www.spinics.net/lists/linux-iio/msg04778.html)
> 
> the patch is big; 
> * some of the changes are cleanup/preparation
> * there are bug fixes along the line of 62d2feb9803f18c4e3c8a1a2c7e30a54df8a1d72 
>   (i2c_get_clientdata(client) points to iio_dev, not hmc5843_data)
> * and finally the code to suppor the new devices
> 
> I'd appreciate to get feedback on the general approach to extend the driver; if 
> necessary I can try to split up the patch, starting with fixes

I think the changes in this driver look good, but yes this should definitely be
split up into multiple patches. Having it in multiple patches also makes
reviewing easier.

A few comments inline.

> 
> I only have a HMC5883L, so I cannot test if the HMC5843 code still works
> 
> ---
>  endmenu
> diff --git a/drivers/staging/iio/magnetometer/hmc5843.c b/drivers/staging/iio/magnetometer/hmc5843.c
> index e7cc9e0..019c631 100644
> --- a/drivers/staging/iio/magnetometer/hmc5843.c
> +++ b/drivers/staging/iio/magnetometer/hmc5843.c
> [...]
>  
>  /* Return the measurement value from the specified channel */
> @@ -152,32 +255,31 @@ static int hmc5843_read_measurement(struct iio_dev *indio_dev,
>  	s32 result;
>  
>  	mutex_lock(&data->lock);
> -	result = i2c_smbus_read_byte_data(client, HMC5843_STATUS_REG);
> -	while (!(result & DATA_READY))
> -		result = i2c_smbus_read_byte_data(client, HMC5843_STATUS_REG);
> +	result = i2c_smbus_read_byte_data(client, HMC58X3_STATUS_REG);
> +	while (!(result & HMC58X3_DATA_READY))
> +		result = i2c_smbus_read_byte_data(client, HMC58X3_STATUS_REG);

This is not a problem with your patch, but it would be good if you could
address it in a separate patch. We need some kind of a timeout here, maybe also
change it to a do {} while loop.

>  
>  	result = i2c_smbus_read_word_data(client, address);
>  	mutex_unlock(&data->lock);
>  	if (result < 0)
>  		return -EINVAL;
>  
> -	*val	= (s16)swab16((u16)result);
> +	*val = (s16)swab16((u16)result);
>  	return IIO_VAL_INT;
>  }
>  
> [...]
> - */
> -static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.5 1 2 5 10 20 50");
> +static ssize_t hmc5843_show_sampling_frequencies_available(struct device *dev,
> +						struct device_attribute *attr,
> +						char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct hmc5843_data *data = iio_priv(indio_dev);
> +
> +	char str[128] = "";
> +	int i;
> +
> +	for (i = 0; i < HMC58X3_RATE_NOT_USED; i++) {
> +		strcat(str, data->regval_to_sample_freq[i]);
> +		strcat(str, " ");
> +	}

I think you can do without the temp buffer here. Maybe something like:
	n = sprintf(buf, "%s ", data->regval_to_sample_freq[i]);
	buf += n;


> +	return sprintf(buf, "%s", str);
> +}
> +
> +static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(hmc5843_show_sampling_frequencies_available);
>  
>  static s32 hmc5843_set_rate(struct i2c_client *client,
>  				u8 rate)
>  {
> -	struct hmc5843_data *data = i2c_get_clientdata(client);
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct hmc5843_data *data = iio_priv(indio_dev);
>  	u8 reg_val;
>  
> -	reg_val = (data->meas_conf) |  (rate << RATE_OFFSET);
> -	if (rate >= RATE_NOT_USED) {
> +	if (rate >= HMC58X3_RATE_NOT_USED) {
>  		dev_err(&client->dev,
> -			"This data output rate is not supported\n");
> +			"data output rate is not supported\n");
>  		return -EINVAL;
>  	}
> -	return i2c_smbus_write_byte_data(client, HMC5843_CONFIG_REG_A, reg_val);
> +	reg_val = data->meas_conf | (rate << HMC58X3_RATE_OFFSET);
> +	return i2c_smbus_write_byte_data(client, HMC58X3_CONFIG_REG_A, reg_val);
>  }
>  
> -static ssize_t set_sampling_frequency(struct device *dev,
> +static int hmc5843_check_sampling_frequency(struct hmc5843_data *data,
> +						const char *buf)
> +{
> +	const char * const *samp_freq = data->regval_to_sample_freq;
> +	int i;
> +
> +	for (i = 0; i < HMC58X3_RATE_NOT_USED; i++) {
> +		if (strncmp(buf, samp_freq[i],
> +			strlen(samp_freq[i])) == 0)

This should use sysfs_streq. sysfs_streq will ignore trailing newlines when
comparing the two strings.

> +			return i;
> +	}
> +
> +	return -EINVAL;
> +}
> [...]