Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 30 Apr 2020 15:11:35 +0800
From:   Xin Ji <xji@analogixsemi.com>
To:     Sam Ravnborg <sam@ravnborg.org>
Cc:     devel@driverdev.osuosl.org,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Dan Carpenter <dan.carpenter@oracle.com>,
        Andrzej Hajda <a.hajda@samsung.com>,
        Neil Armstrong <narmstrong@baylibre.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        Jernej Skrabec <jernej.skrabec@siol.net>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org,
        Pi-Hsun Shih <pihsun@chromium.org>,
        Sheng Pan <span@analogixsemi.com>
Subject: Re: [PATCH v8 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to
 DP bridge driver
Message-ID: <20200430071135.GA9227@xin-VirtualBox>
References: <cover.1587880280.git.xji@analogixsemi.com>
 <4d14400b6c19f17c28267f6ebdbce9673333c05c.1587880280.git.xji@analogixsemi.com>
 <20200427200044.GC15880@ravnborg.org>
 <20200430062416.GD6645@xin-VirtualBox>
 <20200430070324.GA9545@ravnborg.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200430070324.GA9545@ravnborg.org>
User-Agent: Mutt/1.5.24 (2015-08-30)
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from xin-VirtualBox (114.247.245.254) by HK0PR03CA0119.apcprd03.prod.outlook.com (2603:1096:203:b0::35) with Microsoft SMTP Server (version=TLS1_0, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA) id 15.20.2958.20 via Frontend Transport; Thu, 30 Apr 2020 07:11:42 +0000
X-Originating-IP: [114.247.245.254]
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: 5a32fb6e-4f42-4283-b494-08d7ecd5bc53
X-MS-TrafficTypeDiagnostic: BY5PR04MB6898:
X-MS-Exchange-Transport-Forked: True
X-Microsoft-Antispam-PRVS: <BY5PR04MB6898917B4C14CDAFE1135FE3C7AA0@BY5PR04MB6898.namprd04.prod.outlook.com>
X-MS-Oob-TLC-OOBClassifiers: OLM:3968;
X-Forefront-PRVS: 0389EDA07F
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: l2BKUNAD/XT51yDC5exT1UY0/41zSzLzEa3T8NuqK12ZoPGkcPsglDZVrB9EDNyAlRS5SymR047E62bjqwuSJj+mjjY0DDqzguidQUHbcY66ej/M7Nszus5kHEfap4IxqzdgXmEQ/UKWhhA+1KR4JjmMAxrNIlQg75fh8YC4i6ZR/qkEVRD0EYeUqiiYflCyOZZL4DjBuq7tG9pnj4+h5xyXwXTHf5uCHhhozkvglgKcujZAWygZUvhRPMcfSHlSN9M6hT7JTrywt7iUEp9/jrFQntJ4s9jVBsSB4vJZnkjUrmJ+IRAcn4sb5y4n4YKhd+kbcPbFdUmrOD1C1m53rkUHVnyGjVG5Idyr64HV5IkvNEttBAFf7K/yhFtI6on3IT883/O/tpKlaerOE+7vI75N++0Z4Bfxv0ijWJ89UwU2JZ9cwprHWEfgfUV3gqwo
X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:BY5PR04MB6739.namprd04.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(376002)(136003)(39840400004)(366004)(346002)(396003)(6666004)(66946007)(16526019)(2906002)(33656002)(186003)(86362001)(66476007)(66556008)(6916009)(52116002)(6496006)(4326008)(33716001)(956004)(1076003)(55016002)(54906003)(26005)(478600001)(8936002)(316002)(8676002)(107886003)(5660300002)(7416002)(9686003);DIR:OUT;SFP:1102;
X-MS-Exchange-AntiSpam-MessageData: EKd2MEA3/VXN/DOjZFwPxpTViFCBp06Ox/+hSS0ygJHwCuqixKBKqGO1uIUOOVviBYZnE4KQE3ynNtdSVQYVVe1LlQxoFUbUkMwvl1MFQEqzCDm9kLUjhJnsaDWz0dDWvencKm7eew609gL0LUF+HJnd3m/uI+AXv+cgOw36rjODYfxWW5j3dIPg1REa7tfP/HHAa2f+AmZTvCOQBrq/cPBuYMtv+lgP/PoGBG/IZPArMWtZmIluB45XifbNzUB0Mf0PXiJf3u9gQjla2kecYJa6N3aITPeuKsjfIigCaTRksdGes413Wo4NFxlgFPXMreSOBEioIFHWlxBz13+Zi8IL4KN4ErpwphBFQz4KNrDL4Zcmcev+Lm0MSFpVcrJp6RW4+AXPo59KFr4TALxXtavykyP7gQ4IcAOIh3PRQ6oXGm6dSzXW676/bkm8DpqSGRicVlTt6ibI9Vttxv6nVLViprUGzrEQmnAQGDUmMknh+8xGUyGHHVhb0+Anx1CS4SOGQQ8RUL4kKb1gsD0F0UC0fZIv9LCMON4/tlWxI55ZMI68H11UF7kBNLSpMpotoO62hhtXlnktE4jVLwnpsVX+S+DuytD/g72BFFbbXqADQrH2GlkP93K90+5Ozcr0U3hyKQECuTAkkrXi7DILv+rGwt2izh38+uTtYPKvmvQOlBzwr6DIii7akax4et/roVttwLykuI8g/nqnQT6kGjxC8E08ZEvdHSPVm2INGAE0VN4pFrFGXCN8p8V6VI9FhFHkIYqN4KjE0ftsv/pwM22cjGt1D5mpD08PzKjZNcy+EGlrb5M+JbxetKSF1twz
X-OriginatorOrg: analogixsemi.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5a32fb6e-4f42-4283-b494-08d7ecd5bc53
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Apr 2020 07:11:43.1580
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: b099b0b4-f26c-4cf5-9a0f-d5be9acab205
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: kPugLoz4p1CPMWos7TNJhXFfK0mbBvGi5yTAu1s5rfy8ddNu8fyIlhZqP7vCpKRg/+X2EPSYFT6/ESDFCYBt+Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR04MB6898
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Apr 30, 2020 at 09:03:24AM +0200, Sam Ravnborg wrote:
> Hi Xin Ji.
> 
> > > > +static void anx7625_power_on_init(struct anx7625_data *ctx)
> > > > +{
> > > > +	int retry_count, i;
> > > > +	int ret;
> > > > +	struct device *dev = &ctx->client->dev;
> > > > +
> > > > +	for (retry_count = 0; retry_count < 3; retry_count++) {
> > > > +		anx7625_power_on(ctx);
> > > > +		anx7625_config(ctx);
> > > > +
> > > > +		for (i = 0; i < OCM_LOADING_TIME; i++) {
> > > Code in this for loop is a candidate for a helper function.
> > I didn't find any helper function can be used, so I'll keep it.
> I was not very clear in my way to express this, sorry.
> 
> > > 
> > > > +			/* check interface workable */
> > > > +			ret = anx7625_reg_read(ctx, ctx->i2c.rx_p0_client,
> > > > +					       FLASH_LOAD_STA);
> > > > +			if (ret < 0) {
> > > > +				DRM_ERROR("IO error : access flash load.\n");
> > > > +				return;
> > > > +			}
> > > > +			if ((ret & FLASH_LOAD_STA_CHK) == FLASH_LOAD_STA_CHK) {
> > > > +				anx7625_disable_pd_protocol(ctx);
> > > > +				DRM_DEV_DEBUG_DRIVER(dev,
> > > > +						     "Firmware ver %02x%02x,",
> > > > +					anx7625_reg_read(ctx,
> > > > +							 ctx->i2c.rx_p0_client,
> > > > +							 OCM_FW_VERSION),
> > > > +					anx7625_reg_read(ctx,
> > > > +							 ctx->i2c.rx_p0_client,
> > > > +							 OCM_FW_REVERSION));
> > > > +				DRM_DEV_DEBUG_DRIVER(dev, "Driver version %s\n",
> > > > +						     ANX7625_DRV_VERSION);
> > > > +
> > > > +				return;
> > > > +			}
> > > > +			usleep_range(1000, 1100);
> > > > +		}
> What I wanted to express is that the for loop is heavily indented.
> So create a small function like:
> 
> anx7625_power_on_interface(ctx)
> {
> 	/* check interface workable */
> 	ret = anx7625_reg_read(ctx, ctx->i2c.rx_p0_client, FLASH_LOAD_STA);
> 	if (ret < 0) {
> 	        DRM_ERROR("IO error : access flash load.\n");
> 	        return;
> 	}
> 	if ((ret & FLASH_LOAD_STA_CHK) == FLASH_LOAD_STA_CHK) {
> 	        anx7625_disable_pd_protocol(ctx);
> 	        DRM_DEV_DEBUG_DRIVER(dev, "Firmware ver %02x%02x,",
> 	                anx7625_reg_read(ctx, ctx->i2c.rx_p0_client,
>                                          OCM_FW_VERSION), anx7625_reg_read(ctx,
> 	                                 ctx->i2c.rx_p0_client, OCM_FW_REVERSION));
> 	        DRM_DEV_DEBUG_DRIVER(dev, "Driver version %s\n",
> 	                             ANX7625_DRV_VERSION);
> 		retunrn 1;
> 	}
> 	return 0;
> }
> 
> and then
> 
> 	for (i = 0; i < OCM_LOADING_TIME; i++) {
> 		if (anx7625_power_on_interface(ctx))
> 			return;
> 		else
> 			usleep_range(1000, 1100);
> 	}
> 
> Or something like that. To make it more readable.
> I think you get the idea now.
OK, got it, thanks.
> 
> 
> > > > +		container_of(work, struct anx7625_data, extcon_wq);
> > > > +	int state = extcon_get_state(ctx->extcon, EXTCON_DISP_DP);
> > > > +
> > > > +	mutex_lock(&ctx->lock);
> > > > +	anx7625_chip_control(ctx, state);
> > > > +	mutex_unlock(&ctx->lock);
> > > I tried to follow the locking - but failed.
> > > Could you check that locking seems correct.
> > > 
> > > A standard bridge driver do not need locking,
> > > but this is no small bridge driver so I do not imply that
> > > locking is not needed. Only that I would like you
> > > to check it again as I could not follow it.
> > OK, it seems lock is not necessary, I'll remove itA
> It has a worker, so please be careful in you analysis.
OK, I'll double check it.
> 
> > > 
> > > > +
> > > > +	if (pdata->panel_flags == 1)
> > > > +		pdata->internal_panel = 1;
> > > > +	else if (pdata->panel_flags == 2)
> > > > +		pdata->extcon_supported = 1;
> > > > +	DRM_DEV_DEBUG_DRIVER(dev, "%s support extcon, %s internal panel\n",
> > > > +			     pdata->extcon_supported ? "" : "not",
> > > > +			     pdata->internal_panel ? "has" : "no");
> > > > +
> > > The way the internal panel - versus external connector is modelled
> > > looks like it could use some of the abstractions used by other bridge
> > > drivers.
> > > 
> > > The connector_type shall for example for internal panels come
> > > form the panel.
> > > And use the panel bridge driver - see examples in patches I referenced
> > > before.
> > > 
> > > And external connectors may beneft from using the
> > > display-connector bridge driver.
> > I'm not familiar with it, the extcon interface is Google engineer give
> > to me, I just follow their sample driver. If you think it is not good,
> > I'll remove the extcon support.
> It would be better to start without, and then add it later
> so we end up with a clean design.
> 
> I for one would have an easier time reviewing.
> 
> So please go ahead and remove it for now.
OK
> 
> 
> 	Sam