Re: [PATCH v3 1/2] iio: light: driver for Vishay VEML6040

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 27 May 2024 17:12:08 +0200
Arthur Becker via B4 Relay <devnull+arthur.becker.sentec.com@kernel.org> wrote:

> From: Arthur Becker <arthur.becker@sentec.com>
> 
> Implements driver for the Vishay VEML6040 rgbw light sensor.
> 
> Included functionality: setting the integration time and reading the raw
> values for the four channels
> 
> Not yet implemented: setting the measurements to 'Manual Force Mode' (Auto
> measurements off, and adding a measurement trigger)
> 
> Datasheet: https://www.vishay.com/docs/84276/veml6040.pdf
> Signed-off-by: Arthur Becker <arthur.becker@sentec.com>

Hi Arthur,

A few really trivial things inline. I'd have just tidied them up whilst
applying, but I think you are doing a v4 anyway to merge the bindings
so I'll leave the requested tweaks to you.

Jonathan


> diff --git a/drivers/iio/light/veml6040.c b/drivers/iio/light/veml6040.c
> new file mode 100644
> index 000000000000..2ea00d57c38b
> --- /dev/null
> +++ b/drivers/iio/light/veml6040.c



> +static int veml6040_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct veml6040_data *data;
> +	struct iio_dev *indio_dev;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> +		return dev_err_probe(dev, -EOPNOTSUPP,
> +				     "I2C adapter doesn't support plain I2C\n");
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return dev_err_probe(dev, -ENOMEM,
> +				     "IIO device allocation failed\n");
> +
> +	regmap = devm_regmap_init_i2c(client, &veml6040_regmap_config);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap),
> +				     "Regmap setup failed\n");
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);

I don't think this is used. If it isn't, don't set it.

> +	data->client = client;
> +	data->regmap = regmap;
> +
> +	indio_dev->name = "veml6040";
> +	indio_dev->info = &veml6040_info;
> +	indio_dev->channels = veml6040_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(veml6040_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return ret;
> +
> +	int init_config =

Generally we are still sticking to traditional C rules so keep the
local variable definition at the top of the file.
The only common exception is when cleanup.h functionality is involved and
we want to ensure ordering by moving the variable definitions into the code.

> +		FIELD_PREP(VEML6040_CONF_IT_MSK, VEML6040_CONF_IT_40_MS) |
> +		FIELD_PREP(VEML6040_CONF_AF_MSK, 0) |
> +		FIELD_PREP(VEML6040_CONF_SD_MSK, 0);
> +
> +	ret = regmap_write(regmap, VEML6040_CONF_REG, init_config);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Could not set initial config\n");
> +
> +	ret = devm_add_action_or_reset(dev, veml6040_shutdown_action, data);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

> +
> +static struct i2c_driver veml6040_driver = {
> +	.probe = veml6040_probe,
> +	.id_table = veml6040_id_table,
> +	.driver = {
> +		.name = "veml6040",
> +		.of_match_table = veml6040_of_match,
> +	},
> +};
> +

Trivial: Common practice is no new line here as the macro is
very tightly coupled to the structure.

> +module_i2c_driver(veml6040_driver);
> +
> +MODULE_DESCRIPTION("veml6040 RGBW light sensor driver");
> +MODULE_AUTHOR("Arthur Becker <arthur.becker@sentec.com>");
> +MODULE_LICENSE("GPL");
>