Message-ID: <BY5PR02MB6916E4CF9E34190CF7D24AE4A9999@BY5PR02MB6916.namprd02.prod.outlook.com>
Date:   Tue, 16 Nov 2021 20:29:21 +0000
From:   Anand Ashok Dumbre <ANANDASH@...inx.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jic23@...nel.org" <jic23@...nel.org>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        git <git@...inx.com>, Michal Simek <michals@...inx.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "rafael@...nel.org" <rafael@...nel.org>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        "heikki.krogerus@...ux.intel.com" <heikki.krogerus@...ux.intel.com>,
        Manish Narani <MNARANI@...inx.com>
Subject: RE: [PATCH v9 3/5] iio: adc: Add Xilinx AMS driver

Hi Andy,

Thanks for the review.

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> Sent: Tuesday 16 November 2021 5:39 PM
> To: Anand Ashok Dumbre <ANANDASH@...inx.com>
> Cc: linux-kernel@...r.kernel.org; jic23@...nel.org; lars@...afoo.de; linux-
> iio@...r.kernel.org; git <git@...inx.com>; Michal Simek
> <michals@...inx.com>; gregkh@...uxfoundation.org; rafael@...nel.org;
> linux-acpi@...r.kernel.org; heikki.krogerus@...ux.intel.com; Manish Narani
> <MNARANI@...inx.com>
> Subject: Re: [PATCH v9 3/5] iio: adc: Add Xilinx AMS driver
> 
> On Tue, Nov 16, 2021 at 03:08:40PM +0000, Anand Ashok Dumbre wrote:
> > The AMS includes an ADC as well as on-chip sensors that can be used to
> > sample external voltages and monitor on-die operating conditions, such
> > as temperature and supply voltage levels. The AMS has two SYSMON
> blocks.
> > PL-SYSMON block is capable of monitoring off chip voltage and
> > temperature.
> > PL-SYSMON block has DRP, JTAG and I2C interface to enable monitoring
> > from an external master. Out of these interfaces currently only DRP is
> > supported.
> > Other block PS-SYSMON is memory mapped to PS.
> > The AMS can use internal channels to monitor voltage and temperature
> > as well as one primary and up to 16 auxiliary channels for measuring
> > external voltages.
> > The voltage and temperature monitoring channels also have event
> > capability which allows to generate an interrupt when their value
> > falls below or raises above a set threshold.
> 
> Something with indentation / paragraph splitting went wrong.
> 
> ...
> 
> > +#define AMS_ALARM_THR_MIN		0x0000
> > +#define AMS_ALARM_THR_MAX		0xFFFF
> 
> If this is limited by hardware register, I would rather use (BIT(16) - 1)
> notation. It will give immediately amount of bits used for the value.
> 

So ~(BIT(16) - 1) for AMS_ALARM_THR_MIN
(BIT(16) - 1) for AMS_ALARM_THR_MAX

> ...
> 
> > +#define AMS_REGCFG1_ALARM_MASK
> 	(AMS_CONF1_ALARM_2_TO_0_MASK |	\
> > +
> 	AMS_CONF1_ALARM_6_TO_3_MASK | BIT(0))
> 
> Better to write as
> 
> #define AMS_REGCFG1_ALARM_MASK \
> 	(AMS_CONF1_ALARM_2_TO_0_MASK |
> AMS_CONF1_ALARM_6_TO_3_MASK | BIT(0))
> 

Will do.

> ...
> 
> > +#define AMS_PL_CSTS_ACCESS_MASK		0x00000001U
> 
> BIT()
> 
Will fix.

> ...
> 
> > +	u32 reg;
> > +	int ret;
> 
> 	u32 expect = AMS_PS_CSTS_PS_READY;
> 
> (Use similar approach for other readX_poll_timeout() cases)
> 
> > +		ret = readl_poll_timeout(ams->base + AMS_PS_CSTS, reg,
> > +					 (reg & AMS_PS_CSTS_PS_READY) ==
> > +					 AMS_PS_CSTS_PS_READY, 0,
> > +					 AMS_INIT_TIMEOUT_US);
> 
> 		ret = readl_poll_timeout(ams->base + AMS_PS_CSTS, reg,
> 					 (reg & expect) == expect,
> 					 0, AMS_INIT_TIMEOUT_US);
> 
> 0?!
> 
> 
> > +		if (ret)
> > +			return ret;
> 
> ...
> 
> > +		ret = readl(ams->base + AMS_PL_CSTS);
> > +		if (ret == 0)
> > +			return ret;
> 
> Assigning u32 to int seems wrong.

It's a single bit register.
Even if I use u32 here, the return type is int. 
So, is it ok if I read using u32 and return it by typecasting to int?

> 
> ...
> 
> > +static int ams_enable_single_channel(struct ams *ams, unsigned int
> > +offset) {
> > +	u8 channel_num = 0;
> 
> Assignment does not bring any value.

Agreed. 
> 
> > +	switch (offset) {
> > +	case AMS_VCC_PSPLL0:
> > +		channel_num = AMS_VCC_PSPLL0_CH;
> > +		break;
> > +	case AMS_VCC_PSPLL3:
> > +		channel_num = AMS_VCC_PSPLL3_CH;
> > +		break;
> > +	case AMS_VCCINT:
> > +		channel_num = AMS_VCCINT_CH;
> > +		break;
> > +	case AMS_VCCBRAM:
> > +		channel_num = AMS_VCCBRAM_CH;
> > +		break;
> > +	case AMS_VCCAUX:
> > +		channel_num = AMS_VCCAUX_CH;
> > +		break;
> > +	case AMS_PSDDRPLL:
> > +		channel_num = AMS_PSDDRPLL_CH;
> > +		break;
> > +	case AMS_PSINTFPDDR:
> > +		channel_num = AMS_PSINTFPDDR_CH;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* set single channel, sequencer off mode */
> > +	ams_ps_update_reg(ams, AMS_REG_CONFIG1,
> AMS_CONF1_SEQ_MASK,
> > +			  AMS_CONF1_SEQ_SINGLE_CHANNEL);
> > +
> > +	/* write the channel number */
> > +	ams_ps_update_reg(ams, AMS_REG_CONFIG0,
> AMS_CONF0_CHANNEL_NUM_MASK,
> > +			  channel_num);
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +					regval = readl(ams->pl_base +
> > +						       AMS_REG_CONFIG4);
> 
> One line?
> 
> > +					regval = readl(ams->pl_base +
> > +						       AMS_REG_CONFIG4);
> 
> Ditto and so on...
> 
It goes over 80 chars per line.

> ...
> 
> > +static int ams_get_alarm_mask(int scan_index) {
> > +	int bit = 0;
> > +
> > +	if (scan_index >= AMS_PS_SEQ_MAX) {
> > +		bit = AMS_PL_ALARM_START;
> > +		scan_index -= AMS_PS_SEQ_MAX;
> > +	}
> > +
> > +	switch (scan_index) {
> > +	case AMS_SEQ_TEMP:
> > +		return BIT(AMS_ALARM_BIT_TEMP + bit);
> > +	case AMS_SEQ_SUPPLY1:
> > +		return BIT(AMS_ALARM_BIT_SUPPLY1 + bit);
> > +	case AMS_SEQ_SUPPLY2:
> > +		return BIT(AMS_ALARM_BIT_SUPPLY2 + bit);
> > +	case AMS_SEQ_SUPPLY3:
> > +		return BIT(AMS_ALARM_BIT_SUPPLY3 + bit);
> > +	case AMS_SEQ_SUPPLY4:
> > +		return BIT(AMS_ALARM_BIT_SUPPLY4 + bit);
> > +	case AMS_SEQ_SUPPLY5:
> > +		return BIT(AMS_ALARM_BIT_SUPPLY5 + bit);
> > +	case AMS_SEQ_SUPPLY6:
> > +		return BIT(AMS_ALARM_BIT_SUPPLY6 + bit);
> > +	case AMS_SEQ_SUPPLY7:
> > +		return BIT(AMS_ALARM_BIT_SUPPLY7 + bit);
> > +	case AMS_SEQ_SUPPLY8:
> > +		return BIT(AMS_ALARM_BIT_SUPPLY8 + bit);
> > +	case AMS_SEQ_SUPPLY9:
> > +		return BIT(AMS_ALARM_BIT_SUPPLY9 + bit);
> > +	case AMS_SEQ_SUPPLY10:
> > +		return BIT(AMS_ALARM_BIT_SUPPLY10 + bit);
> > +	case AMS_SEQ_VCCAMS:
> > +		return BIT(AMS_ALARM_BIT_VCCAMS + bit);
> > +	case AMS_SEQ_TEMP_REMOTE:
> > +		return BIT(AMS_ALARM_BIT_TEMP_REMOTE + bit);
> > +	default:
> > +		return 0;
> > +	}
> 
> > +	return 0;
> 
> Dead code.

Will remove return statement.

> 
> > +}
> 
> ...
> 
> > +	return (ams->alarm_mask & ams_get_alarm_mask(chan-
> >scan_index)) ? 1
> > +: 0;
> 
> 	return !!(...);
> 
> simply shorter.

Sure.
> 
> ...
> 
> > +	schedule_delayed_work(&ams->ams_unmask_work,
> > +			      msecs_to_jiffies(AMS_UNMASK_TIMEOUT_MS));
> 
> Can be one line.

Over 80 characters. 

Oh! I just saw that upto 100 chars is ok.

> 
> ...
> 
> > +	struct fwnode_handle *child_node = NULL,
> 
> You may drop _node from the name.
> 
> > +			     *fwnode = dev_fwnode(&pdev->dev);
> 
> ...
> 
> > +	if (check_mul_overflow(num_chan, sizeof(struct iio_chan_spec),
> > +			       &ams_chan_size))
> > +		return -EINVAL;
> > +
> > +	/* Initialize buffer for channel specification */
> > +	ams_channels = kzalloc(ams_chan_size, GFP_KERNEL);
> 
> Simply use array_size(). Or why not kcalloc()?
> 
> > +	if (!ams_channels)
> > +		return -ENOMEM;
> 
> ...
> 
> > +	if (check_mul_overflow((size_t)num_channels, sizeof(struct
> iio_chan_spec),
> > +			       &dev_chan_size))
> > +		return -EINVAL;
> > +
> > +	dev_channels = devm_kzalloc(&pdev->dev, dev_chan_size,
> GFP_KERNEL);
> 
> Why not devm_kcalloc()?
> 
> > +	if (!dev_channels) {
> > +		ret = -ENOMEM;
> > +		goto err;
> > +	}
> 
> ...
> 
> > +	ret = 0;
> > +err:
> 
> Use better naming, you should describe what is going to be after goto.
Sure. Will do

> 
> > +	kfree(ams_channels);
> > +
> > +	return ret;
> 
> ...
> 
> > +	ret = devm_add_action_or_reset(&pdev->dev,
> ams_clk_disable_unprepare,
> > +				       ams->clk);
> 
> One line?
> 
> > +	if (ret < 0)
> > +		return ret;
> 
> ...
> 
> > +	ret = ams_init_device(ams);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "failed to initialize AMS\n");
> > +		return ret;
> 
> It's fine to use dev_err_probe() for known error codes.
> 
> > +	}
> 
> ...
> 
> > +	ret = devm_request_irq(&pdev->dev, irq, &ams_irq, 0, "ams-irq",
> > +			       indio_dev);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "failed to register interrupt\n");
> > +		return ret;
> 
> Ditto.
> 
> > +	}
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Thanks,
Anand

Powered by blists - more mailing lists