Re: [PATCH v2 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver

[Chi-Wen Weng <cwweng.linux@gmail.com>]

Hi Andy,

Thank you for the detailed review.

 > bitmap.h implies bitops.h...
 >
 >> +#include <linux/bitops.h>
 >
 > ...so you may drop this one.

Will do. I will drop linux/bitops.h. I will also drop
linux/mod_devicetable.h since platform_device.h is enough here.

 > Please, make all register offsets fixed width, exempli gratia
 >
 > define MA35D1_EADC_STATUS2        0x0f8

Will fix all register offsets to use fixed-width hex values.

 >> +#define MA35D1_EADC_INTSRC0_SPLIEN(n)    BIT(n)
 >
 > Useless? Can't BIT() be used directly?

The field selects which sample module raises ADINT0. This initial driver
only uses sample module 0, so I will replace this helper with a fixed
named bit for SPLIEN0.

 >> +#define MA35D1_EADC_INTERNAL_VREF_MV    1600
 >
 > _mV (yes, as per SI).

Will rename the macro and field to use _mV.

 >> +#define MA35D1_EADC_REF_STABLE_US    1000
 >
 > (1 * USEC_PER_MSEC) ?
 > If go this way, include time.h for the multiplier definition.

Will change this to use USEC_PER_MSEC and include linux/time.h.

 >> +static const struct regmap_config ma35d1_adc_regmap_config = {
 >> +    .reg_bits = 32,
 >> +    .val_bits = 32,
 >> +    .reg_stride = 4,
 >> +    .max_register = MA35D1_EADC_REFADJCTL,
 >
 > No cache?

No register cache is intended for this MMIO ADC block. I will make that
explicit by setting .cache_type = REGCACHE_NONE.

 >> +static bool ma35d1_adc_valid_diff_pair(unsigned int vinp, unsigned 
int vinn)
 >
 > Wondering if this is just
 >
 >    return (vinp >= 0 && vinp < 4 && (vinn == vinp + 4));

Yes, the valid differential pairs are 0-4, 1-5, 2-6 and 3-7. Since vinp
is unsigned, I will simplify this to:

     return vinp < 4 && vinn == vinp + 4;

 >> +static int ma35d1_adc_set_bits(...)
 >> +static int ma35d1_adc_clear_bits(...)
 >
 > What's the point in these wrappers?

There is no strong reason to keep them. I will remove the wrappers and
use regmap_set_bits() / regmap_clear_bits() directly.

 >> +static int ma35d1_adc_setup_reference(struct ma35d1_adc *adc)
 >
 > Add a short comment with reference to a datasheet table/section/et 
cetera.

Will add a short comment describing the REFADJCTL/PDREF and VREFSEL
settings according to the MA35D1 EADC reference control description.

 > With
 >
 >    static regmap *map = adc->regmap;
 >
 > this becomes ...

Will use a local regmap pointer in functions that access several
registers. That should make the code shorter and easier to read.

 >> +    ret = regmap_update_bits(adc->regmap, MA35D1_EADC_CTL,
 >> +                 MA35D1_EADC_CTL_DIFFEN |
 >> +                 MA35D1_EADC_CTL_DMOF, ctl);
 >
 > regmap_assign_bits()

Will use regmap_assign_bits() for the differential mode bits.

 >> +static int ma35d1_adc_update_scan_mode(...)
 >
 > This is interesting check...

I will rework this to explicitly accept exactly one IIO_VOLTAGE scan
channel. This avoids depending on the last bit found in the scan mask.

 >> +    guard(mutex)(&adc->lock);
 >> +    if (adc->suspended || !adc->scan_chan)
 >> +        goto done;
 >
 > This is usually leads to a mess in the compiler...

Will fix this by removing the goto from the trigger handler and using a
scoped_guard() block instead.

 >> +    chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 >> +                     BIT(IIO_CHAN_INFO_SCALE);
 >
 > Broken indentation...

Will fix the indentation.

 >> +static struct iio_chan_spec
 >> +ma35d1_adc_timestamp_channel(unsigned int scan_index)
 >
 > Useless wrapper. Just put _SOFT_TIMESTAMP() in place.

Will remove the helper and assign the timestamp channel directly using a
compound literal.

 >> +    if (!num_channels)
 >> +        return dev_err_probe(dev, -ENODATA,
 >> +                     "no ADC channels configured\n");
 >
 > I would return -ENOENT...

Will change this to -ENOENT.

 >> +    channels = devm_kcalloc(dev, num_channels + 1, 
sizeof(*channels), GFP_KERNEL);
 >
 > size_add() ?

Will use size_add(num_channels, 1).

 >> +        differential = false;
 >> +        vinn = 0;
 >
 > Make it an 'else' branch.

Will do.

 >> +    adc->vref = devm_regulator_get_optional(dev, "vref");
 >
 > It can be unnested:

Will unnest the optional regulator handling in v3.

 >> +    adc->vref_mv = ret / 1000;
 >
 > (MICRO / MILLI)
 >
 > instead of plain 1000.

Will fix this as well. The regulator API returns the voltage in
microvolts, while the driver stores the reference in millivolts for
IIO scale reporting. I will rename the field to vref_mV and use:

     adc->vref_mV = ret / (MICRO / MILLI);

with linux/units.h included, instead of using the plain 1000 constant.

Thanks again for the review. I will address these comments in the next
revision.

Best regards,
Chi-Wen