RE: [PATCH v11 3/5] iio: adc: Add Xilinx AMS driver

[Anand Ashok Dumbre]

Hi Andy,

Thanks for the review.

...
> 
> > +#define AMS_IDR_1			0x02c
> ...
> > +#define AMS_VCC_PSPLL3			0x06C
> ...
> > +#define AMS_VCCBRAM			0x07C
> ...
> > +#define AMS_PSINTFPDDR			0x09C
> ...and so on
> 
> Be consistent with the capitalization in the hex values.

Yes. Will fix all instances in next patch.

> 
> ...
> 
> > +#define AMS_INIT_POLL_TIME		200
> 
> Does it need unit?
> 
> > +#define AMS_SUPPLY_SCALE_1VOLT		1000
> > +#define AMS_SUPPLY_SCALE_3VOLT		3000
> > +#define AMS_SUPPLY_SCALE_6VOLT		6000
> 
> I would rather make units with these:
> 
> #define AMS_SUPPLY_SCALE_1VOLT_mV		1000
> #define AMS_SUPPLY_SCALE_3VOLT_mV		3000
> #define AMS_SUPPLY_SCALE_6VOLT_mV		6000
> 

Will do. 

> ...
> 
> > +#define AMS_PL_AUX_CHAN_VOLTAGE(_auxno) \
> 
> > +	AMS_CHAN_VOLTAGE(PL_SEQ(AMS_SEQ(_auxno)), \
> > +			AMS_REG_VAUX(_auxno), false)
> 
> One line?
> 
> > +#define AMS_CTRL_CHAN_VOLTAGE(_scan_index, _addr) \
> 
> > +
> 	AMS_CHAN_VOLTAGE(PL_SEQ(AMS_SEQ(AMS_SEQ(_scan_index)))
> , \
> > +			_addr, false)
> 
> Ditto.

Will do.

> 
> ...
> 
> > +/**
> > + * struct ams - Driver data for xilinx-ams
> > + * @base: physical base address of device
> > + * @ps_base: physical base address of PS device
> > + * @pl_base: physical base address of PL device
> > + * @clk: clocks associated with the device
> > + * @dev: pointer to device struct
> > + * @lock: to handle multiple user interaction
> > + * @intr_lock: to protect interrupt mask values
> > + * @alarm_mask: alarm configuration
> > + * @current_masked_alarm: currently masked due to alarm
> > + * @intr_mask: interrupt configuration
> > + * @ams_unmask_work: re-enables event once the event condition
> > +disappears
> 
> > + * This structure contains necessary state for Sysmon driver to
> > + operate
> 
> Shouldn't be this "state for Sysmon driver to operate" a summary above?

I don't understand. 

> 
> > + */
> 
> ...
> 
> > +	u32 reg, value;
> > +	u32 expect = AMS_PS_CSTS_PS_READY;
> > +	int ret;
> 
> 	u32 expect = AMS_PS_CSTS_PS_READY;
> 	u32 reg, value;
> 	int ret;
> 
> ...
> 
> > +	u32 reg;
> > +	u32 expect = AMS_ISR1_EOC_MASK;
> > +	int ret;
> 
> Ditto.
> 

Will fix.

> ...
> 
> > +	ret = readl_poll_timeout(ams->base + AMS_ISR_1, reg,
> > +				 (reg & expect), AMS_INIT_POLL_TIME,
> AMS_INIT_TIMEOUT_US);
> 
> Something wrong with line lengths... There is enough space on previous line
> for one parameter.

Accepted.

> 
> > +	if (ret)
> > +		return ret;
> 
> ...
> 
> > +	case IIO_CHAN_INFO_RAW:
> > +		mutex_lock(&ams->lock);
> > +		if (chan->scan_index >= AMS_CTRL_SEQ_BASE) {
> > +			ret = ams_read_vcc_reg(ams, chan->address, val);
> > +			if (ret) {
> 
> > +				mutex_unlock(&ams->lock);
> > +				return ret;
> 
> Can it be
> 				goto out_unlock;
> 
> > +			}
> > +			ams_enable_channel_sequence(indio_dev);
> > +		} else if (chan->scan_index >= AMS_PS_SEQ_MAX)
> > +			*val = readl(ams->pl_base + chan->address);
> > +		else
> > +			*val = readl(ams->ps_base + chan->address);
> 
> 		ret = IIO_VAL_INT;
> out_unlock:
> 
> > +		mutex_unlock(&ams->lock);
> > +
> > +		return IIO_VAL_INT;
> 
> 		mutex_unlock(&ams->lock);
> 		return ret;
> 
> ?

Sure. That looks good.

> 
> Also the question, why mutex only against INFO_RAW case?

All other cases return static values and don't have register configuration involved.

> 
> ...
> 
> > +		switch (chan->type) {
> > +		case IIO_VOLTAGE:
> > +			if (chan->scan_index < AMS_PS_SEQ_MAX) {
> > +				switch (chan->address) {
> > +				case AMS_SUPPLY1:
> > +				case AMS_SUPPLY2:
> > +				case AMS_SUPPLY3:
> > +				case AMS_SUPPLY4:
> > +				case AMS_SUPPLY9:
> > +				case AMS_SUPPLY10:
> > +				case AMS_VCCAMS:
> > +					*val = AMS_SUPPLY_SCALE_3VOLT;
> > +					break;
> > +				case AMS_SUPPLY5:
> > +				case AMS_SUPPLY6:
> > +				case AMS_SUPPLY7:
> > +				case AMS_SUPPLY8:
> > +					*val = AMS_SUPPLY_SCALE_6VOLT;
> > +					break;
> > +				default:
> > +					*val = AMS_SUPPLY_SCALE_1VOLT;
> > +					break;
> > +				}
> > +			} else if (chan->scan_index >= AMS_PS_SEQ_MAX
> &&
> > +				   chan->scan_index < AMS_CTRL_SEQ_BASE)
> {
> > +				switch (chan->address) {
> > +				case AMS_SUPPLY1:
> > +				case AMS_SUPPLY2:
> > +				case AMS_SUPPLY3:
> > +				case AMS_SUPPLY4:
> > +				case AMS_SUPPLY5:
> > +				case AMS_SUPPLY6:
> > +				case AMS_VCCAMS:
> > +				case AMS_VREFP:
> > +				case AMS_VREFN:
> > +					*val = AMS_SUPPLY_SCALE_3VOLT;
> > +					break;
> > +				case AMS_SUPPLY7:
> > +					regval = readl(ams->pl_base +
> AMS_REG_CONFIG4);
> > +					if (FIELD_GET(AMS_VUSER0_MASK,
> regval))
> > +						*val =
> AMS_SUPPLY_SCALE_6VOLT;
> > +					else
> > +						*val =
> AMS_SUPPLY_SCALE_3VOLT;
> > +					break;
> > +				case AMS_SUPPLY8:
> > +					regval = readl(ams->pl_base +
> AMS_REG_CONFIG4);
> > +					if (FIELD_GET(AMS_VUSER1_MASK,
> regval))
> > +						*val =
> AMS_SUPPLY_SCALE_6VOLT;
> > +					else
> > +						*val =
> AMS_SUPPLY_SCALE_3VOLT;
> > +					break;
> > +				case AMS_SUPPLY9:
> > +					regval = readl(ams->pl_base +
> AMS_REG_CONFIG4);
> > +					if (FIELD_GET(AMS_VUSER2_MASK,
> regval))
> > +						*val =
> AMS_SUPPLY_SCALE_6VOLT;
> > +					else
> > +						*val =
> AMS_SUPPLY_SCALE_3VOLT;
> > +					break;
> > +				case AMS_SUPPLY10:
> > +					regval = readl(ams->pl_base +
> AMS_REG_CONFIG4);
> > +					if (FIELD_GET(AMS_VUSER3_MASK,
> regval))
> > +						*val =
> AMS_SUPPLY_SCALE_6VOLT;
> > +					else
> > +						*val =
> AMS_SUPPLY_SCALE_3VOLT;
> > +					break;
> > +				case AMS_VP_VN:
> > +				case AMS_REG_VAUX(0) ...
> AMS_REG_VAUX(15):
> > +					*val = AMS_SUPPLY_SCALE_1VOLT;
> > +					break;
> > +				default:
> > +					*val = AMS_SUPPLY_SCALE_1VOLT;
> > +					break;
> > +				}
> > +			} else {
> > +				switch (chan->address) {
> > +				case AMS_VCC_PSPLL0:
> > +				case AMS_VCC_PSPLL3:
> > +				case AMS_VCCINT:
> > +				case AMS_VCCBRAM:
> > +				case AMS_VCCAUX:
> > +				case AMS_PSDDRPLL:
> > +				case AMS_PSINTFPDDR:
> > +					*val = AMS_SUPPLY_SCALE_3VOLT;
> > +					break;
> > +				default:
> > +					*val = AMS_SUPPLY_SCALE_1VOLT;
> > +					break;
> > +				}
> > +			}
> > +			*val2 = AMS_SUPPLY_SCALE_DIV_BIT;
> > +			return IIO_VAL_FRACTIONAL_LOG2;
> > +		case IIO_TEMP:
> > +			*val = AMS_TEMP_SCALE;
> > +			*val2 = AMS_TEMP_SCALE_DIV_BIT;
> > +			return IIO_VAL_FRACTIONAL_LOG2;
> > +		default:
> > +			return -EINVAL;
> > +		}
> 
> Isn't it a bit too looong for a single switch case?

I agree. Will move them to smaller functions.

> 
> ...
> 
> > +/**
> > + * ams_unmask_worker - ams alarm interrupt unmask worker
> 
> > + * @work :		work to be done
> 
> Be consistent with a style on how you describe parameters in the kernel doc.

Will fix it.

> 
> > + * The ZynqMP threshold interrupts are level sensitive. Since we
> > + can't make the
> > + * threshold condition go way from within the interrupt handler, this
> > + means as
> > + * soon as a threshold condition is present we would enter the
> > + interrupt handler
> > + * again and again. To work around this we mask all active threshold
> > + interrupts
> > + * in the interrupt handler and start a timer. In this timer we poll
> > + the
> > + * interrupt status and only if the interrupt is inactive we unmask it again.
> > + */
> 
> ...
> 
> > +	fwnode_for_each_child_node(chan_node, child) {
> > +		ret = fwnode_property_read_u32(child, "reg", &reg);
> > +		if (ret || reg > AMS_PL_MAX_EXT_CHANNEL + 30)
> > +			continue;
> > +
> > +		chan = &channels[num_channels];
> > +		ext_chan = reg + AMS_PL_MAX_FIXED_CHANNEL - 30;
> > +		memcpy(chan, &ams_pl_channels[ext_chan],
> sizeof(*channels));
> > +
> > +		if (fwnode_property_read_bool(child, "xlnx,bipolar"))
> > +			chan->scan_type.sign =	's';
> 
> Needless double spacing.
> 

Agreed.

> > +		num_channels++;
> > +	}
> 
> ...
> 
> > +		/* add PS channels to iio device channels */
> > +		memcpy(channels, ams_ps_channels,
> > +		       sizeof(ams_ps_channels));
> 
> One line.
> 
> ...
> 
> > +		/* Copy only first 10 fix channels */
> 
> Be consistent with one line comments (pay attention to the capitalization,
> compare to the above).
> 
> > +		memcpy(channels, ams_pl_channels,
> > +		       AMS_PL_MAX_FIXED_CHANNEL * sizeof(*channels));
> 
> One line?
> 
> ...
> 
> > +		/* add AMS channels to iio device channels */
> > +		memcpy(channels, ams_ctrl_channels,
> > +		       sizeof(ams_ctrl_channels));
> 
> One line.
> 
> ...
> 
> > +	fwnode_for_each_child_node(fwnode, child) {
> > +		if (fwnode_device_is_available(child)) {
> 
> > +			ret = ams_init_module(indio_dev, child,
> > +					      ams_channels + num_channels);
> 
> One line?
> 
> > +			if (ret < 0) {
> > +				fwnode_handle_put(child);
> > +				return ret;
> > +			}
> > +
> > +			num_channels += ret;
> > +		}
> > +	}
> 

Will fix as many one liners as I can see in the code.

> ...
> 
> > +	dev_size = sizeof(*dev_channels) * num_channels;
> 
> Here you need to have an array_size(). Or introduce a
> devm_krealloc_array().

Oh yes, you are right.

> 
> > +	dev_channels = devm_krealloc(dev, ams_channels, dev_size,
> GFP_KERNEL);
> > +	if (!dev_channels)
> > +		ret = -ENOMEM;
> 
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Thanks,
Anand