Re: [PATCH] video: imxfb: correct the bitmask for DMACR_HM/_TM - Uwe Kleine-König

From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Martin Kaiser <martin@kaiser.cx>
Cc: linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	Sascha Hauer <kernel@pengutronix.de>
Subject: Re: [PATCH] video: imxfb: correct the bitmask for DMACR_HM/_TM
Date: Wed, 23 Nov 2016 10:50:52 +0100	[thread overview]
Message-ID: <20161123095052.u63j26ixgc54qhkd@pengutronix.de> (raw)
In-Reply-To: <20161123093113.GA10071@botnar.kaiser.cx>

On Wed, Nov 23, 2016 at 10:31:13AM +0100, Martin Kaiser wrote:
> Hello Uwe, all,
> 
> Thus wrote Uwe Kleine-König (u.kleine-koenig@pengutronix.de):
> 
> > For the MX1 which is also supported by this driver, the definitions are
> > right.
> 
> ok, understood. I wasn't able to dig up an imx1 specification. Do you
> know if it's publicly available?

http://www.nxp.com/assets/documents/data/en/reference-manuals/MC9328MX1RM.pdf

> > So this needs a more sophisticated patch. Also I wonder why the
> > register definition is in include/linux/platform_data and not in the
> > driver directly.
> 
> The DMACR_HM() and _TM() macros are meant to be used when we initialize
> imx_fb_platform_data's dmacr component for a platform device. It's not
> straightforward to distinguish between imx1 and imx21 at initialization
> time.

So you put the values to use in the device tree? Then the right thing to
do is to check the device type in the driver and mask accordingly when
the values are written to the hardware.

> We could modify imx_fb_platform_data to use different components for
> dmacr_burst, dmacr_hm, dmacr_tm and calculate the dmacr register value
> in the driver where is_imx1_fb() is available. Device tree is also using
> a single dmacr entry, it's probably not a good idea to do this
> differently for platform devices...
> 
> We could also define DMACR_HM_IMX1(), DMACR_HM_IMX21(), ...
> 
> Or we could just remove the macros, they are not used by any boards in
> the mainline kernel. If we don't want to break proprietary board
> definitions, we could at least add a comment that the macros are
> incorrect for imx21.

IMHO dropping the macros is the right thing to do. Maybe even just
believing the dt and write this value into the register might be
acceptable.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |