On Fri, Mar 20, 2020 at 05:01:15PM +0200, Alexandru Lazar wrote:
> Add driver for the Maxim MAX1241 12-bit, single-channel ADC. The driver
> includes support for this device's low-power operation mode.
...
> +#include <linux/regulator/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
I think you can keep them sorted.
...
> +#define MAX1241_VAL_MASK 0xFFF
GENMASK() ?
...
> + if (adc->shdn) {
> + gpiod_set_value(adc->shdn, 0);
> + udelay(MAX1241_SHDN_DELAY_USEC);
> + }
> +
> + ret = max1241_read(adc);
> +
> + if (adc->shdn)
> + gpiod_set_value(adc->shdn, 1);
I guess easier to read in a way
if () {
gpio...
read();
gpio...
} else {
read();
}
Or actually introduce runtime PM and move these gpio calls there.
...
> +static int max1241_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct max1241 *adc;
> + int ret = 0;
Redundant assignment.
> + ret = regulator_enable(adc->reg);
> + if (ret)
> + return ret;
> +
> + ret = devm_add_action_or_reset(&spi->dev, max1241_disable_reg_action,
> + adc);
Introducing
struct device *dev = &spi->dev;
will simplifies such lines like above by making them on one line.
> + if (ret) {
> + dev_err(&spi->dev, "could not set up regulator cleanup action!\n");
> + return ret;
> + }
> + adc->shdn = devm_gpiod_get_optional(&spi->dev, "shdn", GPIOD_OUT_HIGH);
> +
Redundant blank line.
> + if (IS_ERR(adc->shdn))
> + return PTR_ERR(adc->shdn);
> + if (!adc->shdn)
Why not to use positive conditional?
> + dev_dbg(&spi->dev, "no shdn pin passed, low-power mode disabled");
> + else
> + dev_dbg(&spi->dev, "shdn pin passed, low-power mode enabled");
> +}
...
> +static const struct spi_device_id max1241_id[] = {
> + { "max1241", max1241 },
> + {},
Terminators better w/o comma.
> +};
> +
> +static const struct of_device_id max1241_dt_ids[] = {
> + { .compatible = "maxim,max1241" },
> + {},
Ditto.
> +};
--
With Best Regards,
Andy Shevchenko
