Re: [PATCH] video: imxfb: correct the bitmask for DMACR_HM/_TM

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):

quoted

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

quoted

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/  |