Re: [PATCH v3 2/5] staging:iio:hmc5843: Split hmc5843.c to multiple files

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

On 07/08/2014 03:39 PM, Josef Gajdusek wrote:
[...]
> diff --git a/drivers/staging/iio/magnetometer/Kconfig b/drivers/staging/iio/magnetometer/Kconfig
> index ad88d61..28c2612 100644
> --- a/drivers/staging/iio/magnetometer/Kconfig
> +++ b/drivers/staging/iio/magnetometer/Kconfig
> @@ -5,15 +5,23 @@ menu "Magnetometer sensors"
>
>   config SENSORS_HMC5843
>   	tristate "Honeywell HMC5843/5883/5883L 3-Axis Magnetometer"
> -	depends on I2C
> +	depends on (I2C || SPI_MASTER)
>   	select IIO_BUFFER
>   	select IIO_TRIGGERED_BUFFER
> -	select REGMAP_I2C
> +	select SENSORS_HMC5843_I2C if (I2C)

This approach doesn't work to well and will cause randconfig build failures. 
If SPI_MASTER is 'y' and I2C is 'm' you'll be able to select this driver as 
built-in, which in turn will also select SENSORS_HMC5843_I2C as built-in, 
which means you'll get unresolved symbols during linking since core I2C 
support is built as a module. A better approach is to have a user-selectable 
symbol per bus and a non-user-selectable symbol for the core infrastructure 
of the driver. e.g.

config SENSORS_HMC5843
	tristate
	select IIO_BUFFER
	...

config SENSORS_HMC5843_I2C
	tristate "Honeywell HMC5843/5883/5883L 3-Axis Magnetometer (I2C)"
	select SENSORS_HMC5843
	select REGMAP_I2C

config SENSORS_HMC5843_SPI
	tristate "Honeywell HMC5843/5883/5883L 3-Axis Magnetometer (SPI)"
	select SENSORS_HMC5843
	select REGMAP_SPI



 > +struct regmap_config hmc5843_i2c_regmap_config = {

static

 > +		.reg_bits = 8,
 > +		.val_bits = 8,
 > +
 > +		.rd_table = &hmc5843_readable_table,
 > +		.wr_table = &hmc5843_writable_table,
 > +		.volatile_table = &hmc5843_volatile_table,
 > +
 > +		.cache_type = REGCACHE_RBTREE,
 > +};


 > +static int hmc5843_i2c_probe(struct i2c_client *client,
 > +			 const struct i2c_device_id *id)
 > +{
 > +	struct hmc5843_data *data;
 > +	struct iio_dev *indio_dev;
 > +
 > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 > +	if (indio_dev == NULL)
 > +		return -ENOMEM;
 > +
 > +	i2c_set_clientdata(client, indio_dev);
 > +
 > +	data = iio_priv(indio_dev);
 > +	data->dev = &client->dev;
 > +	data->regmap = devm_regmap_init_i2c(client, &hmc5843_i2c_regmap_config);
 > +
 > +	indio_dev->name = id->name;
 > +	indio_dev->dev.parent = &client->dev;
 > +
 > +	return hmc5843_common_probe(indio_dev, id->driver_data);
 > +}

If you do the allocation of the IIO device in the common function this can 
be simplified a bit. E.g.

static int hmc5843_i2c_probe(struct i2c_client *client,
			 const struct i2c_device_id *id)
{
	return hmc5853_common_probe(&client->dev,
		devm_regmap_init_i2c(client, &mc5843_i2c_regmap_config),
		id->driver_data);
}


 > +#ifdef CONFIG_PM_SLEEP
 > +static int hmc5843_i2c_suspend(struct device *dev)
 > +{
 > +	return hmc5843_common_suspend(i2c_get_clientdata(to_i2c_client(dev)));
 > +}
 > +
 > +static int hmc5843_i2c_resume(struct device *dev)
 > +{
 > +	return hmc5843_common_resume(i2c_get_clientdata(to_i2c_client(dev)));
 > +}
 > +
 > +static SIMPLE_DEV_PM_OPS(hmc5843_pm_ops,
 > +		hmc5843_i2c_suspend, hmc5843_i2c_resume);
 > +#define HMC5843_PM_OPS (&hmc5843_pm_ops)
 > +#else
 > +#define HMC5843_PM_OPS NULL
 > +#endif

Those ops will be the same for both SPI and I2C. 
i2c_get_clientdata(to_i2c_client(dev)) is the same as dev_get_drvdata(dev), 
so this can go into the core driver.


Also as a hint for future patches, if you rename a file use 
'git-format-patch -M', this will make the patch a bit more legible.

- Lars