On 21/02/17 18:03, Quentin Schulz wrote:

Hi Jonathan,

On 19/02/2017 13:40, Jonathan Cameron wrote:

quoted

On 14/02/17 09:40, Quentin Schulz wrote:

quoted

The X-Powers AXP20X and AXP22X PMICs have multiple ADCs. They expose the
battery voltage, battery charge and discharge currents, AC-in and VBUS
voltages and currents, 2 GPIOs muxable in ADC mode and PMIC temperature.

This adds support for most of AXP20X and AXP22X ADCs.

Signed-off-by: Quentin Schulz <redacted>
Acked-by: Maxime Ripard <redacted>

Hi Quentin,

A few bits and bobs inline.  The bigest one is that I don't think
you need to carry your struct axp_data pointer around in the iio_priv
structure as it only seems to be used in probe.

Anyhow, tidy these little bits up (quite a few are optional in the
sense that they are a matter or personal taste and I don't feel
strongly about them) and you can add

Reviewed-by: Jonathan Cameron <jic23@kernel.org>

A nice driver.

Jonathan

[...]

quoted

quoted

+static int axp20x_probe(struct platform_device *pdev)
+{
+	struct axp20x_adc_iio *info;
+	struct iio_dev *indio_dev;
+	struct axp20x_dev *axp20x_dev;
+	int ret;
+
+	axp20x_dev = dev_get_drvdata(pdev->dev.parent);
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	info = iio_priv(indio_dev);
+	platform_set_drvdata(pdev, indio_dev);
+
+	info->regmap = axp20x_dev->regmap;
+	indio_dev->dev.parent = &pdev->dev;
+	indio_dev->dev.of_node = pdev->dev.of_node;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	info->data = (struct axp_data *)platform_get_device_id(pdev)->driver_data;

I think this is only actually used in probe, so could be handled more
neatly with a local variable rather than sticking it in info.

[...]

quoted

quoted

+static int axp20x_remove(struct platform_device *pdev)
+{
+	struct axp20x_adc_iio *info;
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+
+	info = iio_priv(indio_dev);

Reorder the two lines above this one and then you could just do this
inline. (really trivial so feel free to not bother ;)

quoted

+
+	iio_device_unregister(indio_dev);
+	iio_map_array_unregister(indio_dev);
+
+	regmap_write(info->regmap, AXP20X_ADC_EN1, 0);
+
+	if (info->data->adc_en2)

data is used here to know if I should disable misc ADCs (if supported by
the PMIC). So kinda need it only for that purpose, maybe I can use a
slightly simpler structure to only store regmap and this adc_en2 boolean.

Ack for everything else.

cool. Not sure how my searching missed that!

Up to you on whether you simplify that structure or not, I'm happy either way.

Thanks,
Quentin