Message-ID: <alpine.DEB.2.02.1609050924370.12528@pmeerw.net>
Date:   Mon, 5 Sep 2016 10:07:48 +0200 (CEST)
From:   Peter Meerwald-Stadler <pmeerw@...erw.net>
To:     Quentin Schulz <quentin.schulz@...e-electrons.com>
cc:     thomas.petazzoni@...e-electrons.com, lars@...afoo.de,
        linux-iio@...r.kernel.org, antoine.tenart@...e-electrons.com,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        knaack.h@....de, maxime.ripard@...e-electrons.com, jic23@...nel.org
Subject: Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC


> >> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> >> controller and a thermal sensor. This patch adds the ADC driver which is
> >> based on the MFD for the same SoCs ADC.
> > 
> > nitpicking ahead

> [...]
> >> +
> >> +const unsigned int sun4i_gpadc_chan_select(unsigned int chan)
> > 
> > static instead of const?

> static const then?

no, the const is redundant and ignored
-Wignored-qualifiers gives a warning

just static, no const

see
http://stackoverflow.com/questions/12052468/type-qualifiers-on-function-return-type

> >> +{
> >> +	return SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(chan);
> >> +}
> >> +
> >> +struct soc_specific {
> >> +	const int		temp_offset;
> > 
> > wondering why you constify every member?
> > 
> 
> Because they're supposed to be fixed values? It won't change in runtime.
> Is there any reason why I shouldn't do that?

yes, but using the entire struct as const has the same effect;
constifying individual members makes more sense if there are also 
non-const members

nothing wrong, just unusual

> >> +	const int		temp_scale;
> >> +	const unsigned int	tp_mode_en;
> >> +	const unsigned int	tp_adc_select;
> >> +	const unsigned int	(*adc_chan_select)(unsigned int chan);
> >> +};
> [...]
> >> +static int sun4i_gpadc_read(struct iio_dev *indio_dev, int channel, int *val,
> >> +			    unsigned int irq)
> >> +{
> >> +	struct sun4i_gpadc_dev *info = iio_priv(indio_dev);
> >> +	int ret = 0;
> >> +
> >> +	pm_runtime_get_sync(indio_dev->dev.parent);
> >> +	mutex_lock(&info->mutex);
> >> +
> >> +	reinit_completion(&info->completion);
> >> +
> >> +	regmap_write(info->regmap, SUN4I_GPADC_INT_FIFOC,
> >> +		     SUN4I_GPADC_INT_FIFOC_TP_FIFO_TRIG_LEVEL(1) |
> >> +		     SUN4I_GPADC_INT_FIFOC_TP_FIFO_FLUSH);
> > 
> > check retval? here and elsewhere for regmap_write()
> > 
> 
> ACK. What should I do with the retval then?
> 
> There are some in:
> - sun4i_gpadc_read called for read_raws => return which error code (or
> -ret only?)?

I'd just return ret; in the other cases I think it's ok to ignore errors

> - sun4i_gpadc_runtime_suspend => return value of ret, but that would
> cancel the suspend right?
> - sun4i_gpadc_runtime_resume => return value of ret, but that would
> cancel resume right?
> - sun4i_gpadc_probe in the error gotos => probe already failing so do we
> actually care if regmap_update_bits fails? If yes, what's the expected
> behaviour?
> - sun4i_gpadc_remove => return value of ret, but that would mean the
> remove failed right?
> 
> [...]
> >> +static int sun4i_gpadc_read_raw(struct iio_dev *indio_dev,
> >> +				struct iio_chan_spec const *chan, int *val,
> >> +				int *val2, long mask)
> >> +{
> >> +	int ret;
> >> +
> >> +	switch (mask) {
> >> +	case IIO_CHAN_INFO_OFFSET:
> >> +		ret = sun4i_gpadc_temp_offset(indio_dev, val);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		return IIO_VAL_INT;
> >> +	case IIO_CHAN_INFO_RAW:
> >> +		if (chan->type == IIO_VOLTAGE) {
> >> +			ret = sun4i_gpadc_adc_read(indio_dev, chan->channel,
> >> +						   val);
> >> +			if (ret)
> >> +				return ret;
> > 
> > could share the "if (ret) return ret;" between code path
> > 
> 
> ACK.
> 
> [...]
> >> +static int sun4i_irq_init(struct platform_device *pdev, const char *name,
> >> +			  irq_handler_t handler, const char *devname,
> >> +			  unsigned int *irq, atomic_t *atomic)
> >> +{
> >> +	int ret;
> >> +	struct sun4i_gpadc_mfd_dev *mfd_dev = dev_get_drvdata(pdev->dev.parent);
> >> +	struct sun4i_gpadc_dev *info = iio_priv(dev_get_drvdata(&pdev->dev));
> >> +
> >> +	/*
> >> +	 * Once the interrupt is activated, the IP continuously performs
> >> +	 * conversions thus throws interrupts. The interrupt is activated right
> >> +	 * after being requested but we want to control when these interrupts
> >> +	 * occurs thus we disable it right after being requested. However, an
> > 
> > occur
> > 
> 
> ACK for all typos.
> Thanks!
> 
> Quentin
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

Powered by blists - more mailing lists