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

[Lars-Peter Clausen]

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
--
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/