Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754880AbaKENco (ORCPT ); Wed, 5 Nov 2014 08:32:44 -0500 Received: from mailapp01.imgtec.com ([195.59.15.196]:44452 "EHLO mailapp01.imgtec.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751198AbaKENcm (ORCPT ); Wed, 5 Nov 2014 08:32:42 -0500 Message-ID: <545A2680.2@imgtec.com> Date: Wed, 5 Nov 2014 13:30:40 +0000 From: Zubair Lutfullah Kakakhel User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Andy Yan , , , , CC: Greg Kroah-Hartman , Grant Likely , Rob Herring , Philipp Zabel , Shawn Guo , Josh Boyer , Sean Paul , Inki Dae , Dave Airlie , Arnd Bergmann , Lucas Stach , , , , , , , Subject: Re: [PATCH V2 1/2] imx-drm: imx-hdmi: split imx soc specific code from imx-hdmi References: <1415192385-6572-1-git-send-email-andy.yan@rock-chips.com> In-Reply-To: <1415192385-6572-1-git-send-email-andy.yan@rock-chips.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit X-Originating-IP: [192.168.154.89] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org This one patch does too much to be reviewed easily. One patch is supposed to modify/add one thing at a time in the kernel. Separating platform specific code from imx-drm/imx-hdmi is one thing. Adding support for multi-byte register access is something different. i.e. Something like. 1/3 split platform specific code out. 2/3 move/rename imx-hdmi outside the folder 3/3 add support for multi byte register width access. If there are other things that are not directly relevant to the patch, it goes in a different patch. Bug fixes are also separate. This should result in readable patches that can be reviewed easily. Also, the approach with 4 byte access is ok. But you could use reg-shifts as well perhaps. Then you won't have to change so much of the code. static inline void hdmi_writeb(struct dwc_hdmi *hdmi, u8 val, int offset) +{ + writeb(val, hdmi->regs + (offset << hdmi->reg_shift)); +} + +static inline u8 hdmi_readb(struct dwc_hdmi *hdmi, int offset) +{ + return readb(hdmi->regs + (offset << hdmi->reg_shift)); +} And then in probe +hdmi->reg_shift = 0; + + if (of_property_read_u32(np, "reg-shift", &hdmi->reg_shift)) + dev_warn(hdmi->dev, "No reg-shift\n"); This way the reg-shift property can be defined using DT Cheers, ZubairLK On 05/11/14 12:59, Andy Yan wrote: > imx6 and rockchip rk3288 and JZ4780 (Ingenic Xburst/MIPS) > use the interface compatible Designware HDMI IP, but they > also have some lightly difference, such as phy pll configuration, > register width(imx hdmi register is one byte, but rk3288 is 4 > bytes width), 4K support(imx6 doesn't support 4k, but rk3288 does), > clk useage,and the crtc mux configuration is also platform specific. > > To reuse the imx hdmi driver, split the platform specific code out > to dw_hdmi-imx.c. > > Signed-off-by: Andy Yan > --- > drivers/staging/imx-drm/Makefile | 2 +- > drivers/staging/imx-drm/dw_hdmi-imx.c | 214 ++++++++++ > drivers/staging/imx-drm/imx-hdmi.c | 726 ++++++++++++++-------------------- > include/drm/bridge/dw_hdmi.h | 114 ++++++ > 4 files changed, 634 insertions(+), 422 deletions(-) > create mode 100644 drivers/staging/imx-drm/dw_hdmi-imx.c > create mode 100644 include/drm/bridge/dw_hdmi.h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/