On Fri, Jun 02, 2023 at 01:30:39AM +0300, Maksim Kiselev wrote:
From: Maxim Kiselev <redacted>
The General Purpose ADC (GPADC) can convert the external signal into
a certain proportion of digital value, to realize the measurement of
analog signal, which can be applied to power detection and key detection.
Theoretically, this ADC can support up to 16 channels. All SoCs below
contain this GPADC IP. The only difference between them is the number
of available channels:
T113 - 1 channel
D1 - 2 channels
R329 - 4 channels
T507 - 4 channels
...
quoted hunk ↗ jump to hunk
+struct sun20i_gpadc_iio {
+ struct regmap *regmap;
+ struct completion completion;
+ struct mutex lock;
The locks should be explained (what are they for? what do they protect?).
+ int lastch;
+};
...
quoted hunk ↗ jump to hunk
+static const struct regmap_config sun20i_gpadc_regmap_config = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+ .fast_io = true,
I forgot if I asked about regmap lock do you need it?
+};
...
+ if (!wait_for_completion_timeout(&info->completion,
+ msecs_to_jiffies(100))) {
Dunno if it's better to have this parameter to be defined with self-explanatory
name.
quoted hunk ↗ jump to hunk
+ ret = -ETIMEDOUT;
+ goto err;
+ }
...
+err:
err_unlock:
quoted hunk ↗ jump to hunk
+ mutex_unlock(&info->lock);
+
+ return ret;
...
quoted hunk ↗ jump to hunk
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = sun20i_gpadc_adc_read(info, chan, val);
+ return ret;
return sun...;
quoted hunk ↗ jump to hunk
+ case IIO_CHAN_INFO_SCALE:
+ /* value in mv = 1800mV / 4096 raw */
+ *val = 1800;
+ *val2 = 12;
+ return IIO_VAL_FRACTIONAL_LOG2;
+ default:
+ return -EINVAL;
+ }
...
quoted hunk ↗ jump to hunk
+ if (num_channels > SUN20I_GPADC_MAX_CHANNELS) {
+ dev_err(dev, "num of channel children out of range");
+ return -EINVAL;
+ }
Is it really critical error?
...
+ channels = devm_kcalloc(dev, num_channels,
+ sizeof(*channels), GFP_KERNEL);
At least one more parameter can be located on the previous line.
+ if (!channels)
+ return -ENOMEM;
...
+err_child_out:
err_put_child:
The goto labels should be self-explanatory of what to expect when goto.
quoted hunk ↗ jump to hunk
+ fwnode_handle_put(node);
+
+ return ret;
...
quoted hunk ↗ jump to hunk
+ ret = devm_request_irq(dev, irq, sun20i_gpadc_irq_handler,
+ 0, dev_name(dev), info);
+ if (ret < 0)
Here...
+ return dev_err_probe(dev, ret,
+ "failed requesting irq %d\n", irq);
...
+ ret = devm_iio_device_register(dev, indio_dev);
+ if (ret < 0)
...and here, do the positive returned values even possible?
+ return dev_err_probe(dev, ret,
+ "could not register the device\n");
...
+ { .compatible = "allwinner,sun20i-d1-gpadc", },
Inner comma is not needed.
--
With Best Regards,
Andy Shevchenko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel