Re: [PATCH v6 4/8] mfd: fsl imx25 Touchscreen ADC driver

["Lothar Waßmann" <LW@KARO-electronics.de>]

Hi,

Markus Pargmann wrote:
> This is the core driver for imx25 touchscreen/adc driver. The module
> has one shared ADC and two different conversion queues which use the
> ADC. The two queues are identical. Both can be used for general purpose
> ADC but one is meant to be used for touchscreens.
> 
> This driver is the core which manages the central components and
> registers of the TSC/ADC unit. It manages the IRQs and forwards them to
> the correct components.
> 
[...]
> diff --git a/drivers/mfd/fsl-imx25-tsadc.c b/drivers/mfd/fsl-imx25-tsadc.c
> new file mode 100644
> index 000000000000..8e4013d57500
> --- /dev/null
> +++ b/drivers/mfd/fsl-imx25-tsadc.c
> @@ -0,0 +1,167 @@
[...]
> +#define MX25_TGCR_POWERMODE_MASK	(3 << 8)
> +#define MX25_TGCR_POWERMODE_SAVE	BIT(8)
> +#define MX25_TGCR_POWERMODE_ON		(2 << 8)
>
This looks a bit weird and conceals the fact, that
MX25_TGCR_POWERMODE_SAVE is in fact one of the possible settings
of a two bit bitfield. For consistency I would write:
#define MX25_TGCR_POWERMODE_MASK	(3 << 8)
#define MX25_TGCR_POWERMODE_SAVE	(1 << 8)
#define MX25_TGCR_POWERMODE_ON		(2 << 8)

[...]
> +#define MX25_ADCQ_CFG_YPLL_HIGH	0
> +#define MX25_ADCQ_CFG_YPLL_OFF		BIT(12)
> +#define MX25_ADCQ_CFG_YPLL_LOW		(3 << 12)
>
dto.

> +#define MX25_ADCQ_CFG_XNUR_HIGH	0
> +#define MX25_ADCQ_CFG_XNUR_OFF		BIT(10)
> +#define MX25_ADCQ_CFG_XNUR_LOW		(3 << 10)
>
dto.

> +#define MX25_ADCQ_CFG_XPUL_OFF		BIT(9)
> +#define MX25_ADCQ_CFG_XPUL_HIGH	0
>
|#define MX25_ADCQ_CFG_XPUL_OFF		(1 << 9)
|#define MX25_ADCQ_CFG_XPUL_HIGH	(0 << 9)
would make it more clear, that these refer to the two states of the same
bit.

> +#define MX25_ADCQ_CFG_REFP(sel)		(sel << 7)
>
missing () around macro argument

> +#define MX25_ADCQ_CFG_REFP_YP		0
> +#define MX25_ADCQ_CFG_REFP_XP		(1 << 7)
> +#define MX25_ADCQ_CFG_REFP_EXT		(2 << 7)
> +#define MX25_ADCQ_CFG_REFP_INT		(3 << 7)
> +#define MX25_ADCQ_CFG_REFP_MASK		(3 << 7)
>
see my previous comment.

> +#define MX25_ADCQ_CFG_IN(sel)		(sel << 4)
>
missing () around macro argument

> +#define MX25_ADCQ_CFG_IN_XP		0
> +#define MX25_ADCQ_CFG_IN_YP		(1 << 4)
> +#define MX25_ADCQ_CFG_IN_XN		(2 << 4)
> +#define MX25_ADCQ_CFG_IN_YN		(3 << 4)
>
see my previous comment.

> +#define MX25_ADCQ_CFG_IN_WIPER		(4 << 4)
> +#define MX25_ADCQ_CFG_IN_AUX0		(5 << 4)
> +#define MX25_ADCQ_CFG_IN_AUX1		(6 << 4)
> +#define MX25_ADCQ_CFG_IN_AUX2		(7 << 4)
> +#define MX25_ADCQ_CFG_REFN(sel)		(sel << 2)
>
missing () around macro argument

> +#define MX25_ADCQ_CFG_REFN_XN		0
> +#define MX25_ADCQ_CFG_REFN_YN		(1 << 2)
> +#define MX25_ADCQ_CFG_REFN_NGND		(2 << 2)
> +#define MX25_ADCQ_CFG_REFN_NGND2	(3 << 2)
> +#define MX25_ADCQ_CFG_REFN_MASK		(3 << 2)
>
see my previous comment.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________