Message ID | 20231114150130.497915-6-sui.jingfeng@linux.dev |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:6358:a59:b0:164:83eb:24d7 with SMTP id 25csp1987616rwb; Tue, 14 Nov 2023 07:10:48 -0800 (PST) X-Google-Smtp-Source: AGHT+IH6oRq+sPJM1lzp8J17PTHdK2kL3yxU32Ziaw9LSNmJFKJqAQzh/et5m0Lm3VEO6CTPsYrj X-Received: by 2002:a17:90b:3850:b0:280:e2e1:f955 with SMTP id nl16-20020a17090b385000b00280e2e1f955mr10576649pjb.35.1699974647715; Tue, 14 Nov 2023 07:10:47 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1699974647; cv=none; d=google.com; s=arc-20160816; b=BvrwVQ2fize0C/7/ZgTMz583xMsgz/Z0hlulp8WX9i0Fo+C1kohOOG+SPOD5GCSQNM Lo5q9pcSkeeFohjP+82bLdPL7+s7PUVCrTjUOVJCfpZs+Qjx57NWkv/7/1xHFA/4Po8F hfKj4xami1J84fPbdHnmrHfNG1X3PB+a+LzmXgFnYy6yuSc1IM5WUS4NCc/OXhSvVcSA EsPFNv+kaTSa8He3BLnMMmO/mKwayWegaQVcjIvo/xKO3DlF6ThXlm2csgW1q9oRjrlj /ezGY1JO6WF4ckjwWvDpFszdfVfFPbm99dxxLiytpgKC6O3eU436jE0VB/wiRtFKJxIS PkgA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=NjLEQ2aBUBcn+Sqc95rmrPoW3NH5ZZ26cu6iyzwpGc0=; fh=AsQUxY17UP4I4ANjAjzQGhV+/8Tz+vG6DJFG+Cn0IYE=; b=kUwD+ygCBT+aExR1JhxotR1JGqJdlq9ze7DDrfiEkOncw3re1XgMnWJJ4yzmV9Aqvz yk+B2qRvcMw6jJHU4TdJeMxab64piPKz5hJkHwrm8cJWj9vsSQ5yNBEzvN533Z1LVUpl qrnWnxHhcD9Cgw4X7UprX4GNbIOwpI03Xl1COLwP1djUDGrlrWQt9UX1K4BHmYZxcU7Q eGmpEmEL5MD9S0k1JGvr51qWgswnuJS+WabxmHJ2VAA7eH9+9yy3yUlEkcHruVFnyd2v 6EfJTO0sQLApcl13jAk9tAxNlSjl+Su/D/NrgnScRvu9uButhW2oKzKZvRkJ1Z7uOq4z yL8g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linux.dev header.s=key1 header.b=kuURKlYb; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linux.dev Received: from fry.vger.email (fry.vger.email. [23.128.96.38]) by mx.google.com with ESMTPS id u8-20020a170903124800b001ce2fc160cfsi3206125plh.434.2023.11.14.07.10.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 14 Nov 2023 07:10:47 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38; Authentication-Results: mx.google.com; dkim=pass header.i=@linux.dev header.s=key1 header.b=kuURKlYb; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linux.dev Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by fry.vger.email (Postfix) with ESMTP id BA9F08096BAF; Tue, 14 Nov 2023 07:10:19 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at fry.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233493AbjKNPJi (ORCPT <rfc822;lhua1029@gmail.com> + 29 others); Tue, 14 Nov 2023 10:09:38 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39834 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229504AbjKNPJg (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 14 Nov 2023 10:09:36 -0500 X-Greylist: delayed 453 seconds by postgrey-1.37 at lindbergh.monkeyblade.net; Tue, 14 Nov 2023 07:09:32 PST Received: from out-177.mta1.migadu.com (out-177.mta1.migadu.com [IPv6:2001:41d0:203:375::b1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7D3B118A for <linux-kernel@vger.kernel.org>; Tue, 14 Nov 2023 07:09:32 -0800 (PST) X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1699974138; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=NjLEQ2aBUBcn+Sqc95rmrPoW3NH5ZZ26cu6iyzwpGc0=; b=kuURKlYbDIZkkE2VlZVqW0eLF4/yKYOCNNFEuflSWrkD8XOv6U9BWn3iVZmIUtoJqSidkD tHAtBcRbv/f+SkUPe+gNkccy0E/Nv7Yd/65IhYW7jtAtFcDG5baoSGFxCLWjC+195kewFe Ihq1rL5hTg0Go/FFQ+xyAE9m8Z5jvmw= From: Sui Jingfeng <sui.jingfeng@linux.dev> To: Phong LE <ple@baylibre.com>, Neil Armstrong <neil.armstrong@linaro.org> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>, Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Sui Jingfeng <suijingfeng@loongson.cn> Subject: [PATCH 5/8] drm/bridge: it66121: Add a helper function to read chip id Date: Tue, 14 Nov 2023 23:01:27 +0800 Message-Id: <20231114150130.497915-6-sui.jingfeng@linux.dev> In-Reply-To: <20231114150130.497915-1-sui.jingfeng@linux.dev> References: <20231114150130.497915-1-sui.jingfeng@linux.dev> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Tue, 14 Nov 2023 07:10:19 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1782552616204870252 X-GMAIL-MSGID: 1782552616204870252 |
Series |
Allow link the it66121 display bridge driver as a lib
|
|
Commit Message
Sui Jingfeng
Nov. 14, 2023, 3:01 p.m. UTC
From: Sui Jingfeng <suijingfeng@loongson.cn> Read the required chip id data back by calling regmap_bulk_read() once, reduce the number of local variables needed in it66121_probe() function. And store its values into struct it66121_ctx, as it will be used latter. Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn> --- drivers/gpu/drm/bridge/ite-it66121.c | 47 ++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 13 deletions(-)
Comments
On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > > From: Sui Jingfeng <suijingfeng@loongson.cn> > > Read the required chip id data back by calling regmap_bulk_read() once, > reduce the number of local variables needed in it66121_probe() function. > And store its values into struct it66121_ctx, as it will be used latter. > > Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn> > --- > drivers/gpu/drm/bridge/ite-it66121.c | 47 ++++++++++++++++++++-------- > 1 file changed, 34 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c > index 7e473beefc79..f36d05331f25 100644 > --- a/drivers/gpu/drm/bridge/ite-it66121.c > +++ b/drivers/gpu/drm/bridge/ite-it66121.c > @@ -313,6 +313,9 @@ struct it66121_ctx { > bool auto_cts; > } audio; > const struct it66121_chip_info *info; > + u16 vender_id; > + u16 device_id; > + u8 revision; There is no need to store them, they are not used by the driver anywhere. > }; > > static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge) > @@ -399,6 +402,30 @@ static void it66121_hw_reset(struct it66121_ctx *ctx) > gpiod_set_value(ctx->gpio_reset, 0); > } > > +static int it66121_read_chip_id(struct it66121_ctx *ctx, bool verbose) > +{ > + u8 id[4]; > + int ret; > + > + ret = regmap_bulk_read(ctx->regmap, IT66121_VENDOR_ID0_REG, id, 4); > + if (ret < 0) { > + dev_err(ctx->dev, "Failed to read chip ID: %d\n", ret); > + return ret; > + } > + > + ctx->vender_id = (u16)id[1] << 8 | id[0]; > + ctx->device_id = ((u16)(id[3] & IT66121_DEVICE_ID1_MASK) << 8 | id[2]); > + /* Revision is shared with DEVICE_ID1 */ > + ctx->revision = FIELD_GET(IT66121_REVISION_MASK, id[3]); > + > + if (verbose) { > + dev_info(ctx->dev, "Found ITE66121: 0x%x%x, revision: %u\n", > + ctx->vender_id, ctx->device_id, ctx->revision); > + } > + > + return 0; > +} > + > static inline int it66121_preamble_ddc(struct it66121_ctx *ctx) > { > return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST); > @@ -1561,7 +1588,6 @@ static const char * const it66121_supplies[] = { > > static int it66121_probe(struct i2c_client *client) > { > - u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 }; > int ret; > struct it66121_ctx *ctx; > struct device *dev = &client->dev; > @@ -1603,19 +1629,13 @@ static int it66121_probe(struct i2c_client *client) > if (IS_ERR(ctx->regmap)) > return PTR_ERR(ctx->regmap); > > - regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]); > - regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]); > - regmap_read(ctx->regmap, IT66121_DEVICE_ID0_REG, &device_ids[0]); > - regmap_read(ctx->regmap, IT66121_DEVICE_ID1_REG, &device_ids[1]); > - > - /* Revision is shared with DEVICE_ID1 */ > - revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]); > - device_ids[1] &= IT66121_DEVICE_ID1_MASK; > + ret = it66121_read_chip_id(ctx, false); > + if (ret) > + return ret; > > - if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid || > - (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) { > + if (ctx->vender_id != ctx->info->vid || > + ctx->device_id != ctx->info->pid) > return -ENODEV; > - } > > ctx->bridge.funcs = &it66121_bridge_funcs; > ctx->bridge.of_node = dev->of_node; > @@ -1633,7 +1653,8 @@ static int it66121_probe(struct i2c_client *client) > > drm_bridge_add(&ctx->bridge); > > - dev_info(dev, "IT66121 revision %d probed\n", revision_id); > + dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n", > + ctx->vender_id, ctx->device_id, ctx->revision); > > return 0; > } > -- > 2.34.1 >
Hi, On 2023/11/15 00:06, Dmitry Baryshkov wrote: > On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: >> From: Sui Jingfeng <suijingfeng@loongson.cn> >> >> Read the required chip id data back by calling regmap_bulk_read() once, >> reduce the number of local variables needed in it66121_probe() function. >> And store its values into struct it66121_ctx, as it will be used latter. >> >> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn> >> --- >> drivers/gpu/drm/bridge/ite-it66121.c | 47 ++++++++++++++++++++-------- >> 1 file changed, 34 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c >> index 7e473beefc79..f36d05331f25 100644 >> --- a/drivers/gpu/drm/bridge/ite-it66121.c >> +++ b/drivers/gpu/drm/bridge/ite-it66121.c >> @@ -313,6 +313,9 @@ struct it66121_ctx { >> bool auto_cts; >> } audio; >> const struct it66121_chip_info *info; >> + u16 vender_id; >> + u16 device_id; >> + u8 revision; > There is no need to store them, they are not used by the driver anywhere. > >> }; >> >> static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge) >> @@ -399,6 +402,30 @@ static void it66121_hw_reset(struct it66121_ctx *ctx) >> gpiod_set_value(ctx->gpio_reset, 0); >> } >> >> +static int it66121_read_chip_id(struct it66121_ctx *ctx, bool verbose) >> +{ >> + u8 id[4]; >> + int ret; >> + >> + ret = regmap_bulk_read(ctx->regmap, IT66121_VENDOR_ID0_REG, id, 4); >> + if (ret < 0) { >> + dev_err(ctx->dev, "Failed to read chip ID: %d\n", ret); >> + return ret; >> + } >> + >> + ctx->vender_id = (u16)id[1] << 8 | id[0]; >> + ctx->device_id = ((u16)(id[3] & IT66121_DEVICE_ID1_MASK) << 8 | id[2]); >> + /* Revision is shared with DEVICE_ID1 */ >> + ctx->revision = FIELD_GET(IT66121_REVISION_MASK, id[3]); >> + >> + if (verbose) { >> + dev_info(ctx->dev, "Found ITE66121: 0x%x%x, revision: %u\n", >> + ctx->vender_id, ctx->device_id, ctx->revision); >> + } >> + >> + return 0; >> +} >> + >> static inline int it66121_preamble_ddc(struct it66121_ctx *ctx) >> { >> return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST); >> @@ -1561,7 +1588,6 @@ static const char * const it66121_supplies[] = { >> >> static int it66121_probe(struct i2c_client *client) >> { >> - u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 }; >> int ret; >> struct it66121_ctx *ctx; >> struct device *dev = &client->dev; >> @@ -1603,19 +1629,13 @@ static int it66121_probe(struct i2c_client *client) >> if (IS_ERR(ctx->regmap)) >> return PTR_ERR(ctx->regmap); >> >> - regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]); >> - regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]); >> - regmap_read(ctx->regmap, IT66121_DEVICE_ID0_REG, &device_ids[0]); >> - regmap_read(ctx->regmap, IT66121_DEVICE_ID1_REG, &device_ids[1]); >> - >> - /* Revision is shared with DEVICE_ID1 */ >> - revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]); >> - device_ids[1] &= IT66121_DEVICE_ID1_MASK; >> + ret = it66121_read_chip_id(ctx, false); >> + if (ret) >> + return ret; >> >> - if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid || >> - (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) { >> + if (ctx->vender_id != ctx->info->vid || >> + ctx->device_id != ctx->info->pid) Q: There is no need to store them, they are not used by the driver anywhere. A: Here it is used, it is also used by the 0007-patch to get the entity(instance)-specific data. Since it6610 was introduced, this is used for chip identifying. It can also be used with in debugfs context, to show who I am. >> return -ENODEV; >> - } >> >> ctx->bridge.funcs = &it66121_bridge_funcs; >> ctx->bridge.of_node = dev->of_node; >> @@ -1633,7 +1653,8 @@ static int it66121_probe(struct i2c_client *client) >> >> drm_bridge_add(&ctx->bridge); >> >> - dev_info(dev, "IT66121 revision %d probed\n", revision_id); >> + dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n", >> + ctx->vender_id, ctx->device_id, ctx->revision); >> >> return 0; >> } >> -- >> 2.34.1 >> >
On Thu, 16 Nov 2023 at 14:18, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > > Hi, > > > On 2023/11/15 00:06, Dmitry Baryshkov wrote: > > On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > >> From: Sui Jingfeng <suijingfeng@loongson.cn> > >> > >> Read the required chip id data back by calling regmap_bulk_read() once, > >> reduce the number of local variables needed in it66121_probe() function. > >> And store its values into struct it66121_ctx, as it will be used latter. > >> > >> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn> > >> --- > >> drivers/gpu/drm/bridge/ite-it66121.c | 47 ++++++++++++++++++++-------- > >> 1 file changed, 34 insertions(+), 13 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c > >> index 7e473beefc79..f36d05331f25 100644 > >> --- a/drivers/gpu/drm/bridge/ite-it66121.c > >> +++ b/drivers/gpu/drm/bridge/ite-it66121.c > >> @@ -313,6 +313,9 @@ struct it66121_ctx { > >> bool auto_cts; > >> } audio; > >> const struct it66121_chip_info *info; > >> + u16 vender_id; > >> + u16 device_id; > >> + u8 revision; > > There is no need to store them, they are not used by the driver anywhere. > > > >> }; > >> > >> static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge) > >> @@ -399,6 +402,30 @@ static void it66121_hw_reset(struct it66121_ctx *ctx) > >> gpiod_set_value(ctx->gpio_reset, 0); > >> } > >> > >> +static int it66121_read_chip_id(struct it66121_ctx *ctx, bool verbose) > >> +{ > >> + u8 id[4]; > >> + int ret; > >> + > >> + ret = regmap_bulk_read(ctx->regmap, IT66121_VENDOR_ID0_REG, id, 4); > >> + if (ret < 0) { > >> + dev_err(ctx->dev, "Failed to read chip ID: %d\n", ret); > >> + return ret; > >> + } > >> + > >> + ctx->vender_id = (u16)id[1] << 8 | id[0]; > >> + ctx->device_id = ((u16)(id[3] & IT66121_DEVICE_ID1_MASK) << 8 | id[2]); > >> + /* Revision is shared with DEVICE_ID1 */ > >> + ctx->revision = FIELD_GET(IT66121_REVISION_MASK, id[3]); > >> + > >> + if (verbose) { > >> + dev_info(ctx->dev, "Found ITE66121: 0x%x%x, revision: %u\n", > >> + ctx->vender_id, ctx->device_id, ctx->revision); > >> + } > >> + > >> + return 0; > >> +} > >> + > >> static inline int it66121_preamble_ddc(struct it66121_ctx *ctx) > >> { > >> return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST); > >> @@ -1561,7 +1588,6 @@ static const char * const it66121_supplies[] = { > >> > >> static int it66121_probe(struct i2c_client *client) > >> { > >> - u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 }; > >> int ret; > >> struct it66121_ctx *ctx; > >> struct device *dev = &client->dev; > >> @@ -1603,19 +1629,13 @@ static int it66121_probe(struct i2c_client *client) > >> if (IS_ERR(ctx->regmap)) > >> return PTR_ERR(ctx->regmap); > >> > >> - regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]); > >> - regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]); > >> - regmap_read(ctx->regmap, IT66121_DEVICE_ID0_REG, &device_ids[0]); > >> - regmap_read(ctx->regmap, IT66121_DEVICE_ID1_REG, &device_ids[1]); > >> - > >> - /* Revision is shared with DEVICE_ID1 */ > >> - revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]); > >> - device_ids[1] &= IT66121_DEVICE_ID1_MASK; > >> + ret = it66121_read_chip_id(ctx, false); > >> + if (ret) > >> + return ret; > >> > >> - if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid || > >> - (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) { > >> + if (ctx->vender_id != ctx->info->vid || > >> + ctx->device_id != ctx->info->pid) > > Q: There is no need to store them, they are not used by the driver anywhere. > > A: Here it is used, it is also used by the 0007-patch to get the entity(instance)-specific data. And the patch 7 will be changed once you have proper i2c client struct registered. > > > Since it6610 was introduced, this is used for chip identifying. > It can also be used with in debugfs context, to show who I am. I'd say, there is little point in whoami debugfs files. Debugfs is for the useful information. > > > >> return -ENODEV; > >> - } > >> > >> ctx->bridge.funcs = &it66121_bridge_funcs; > >> ctx->bridge.of_node = dev->of_node; > >> @@ -1633,7 +1653,8 @@ static int it66121_probe(struct i2c_client *client) > >> > >> drm_bridge_add(&ctx->bridge); > >> > >> - dev_info(dev, "IT66121 revision %d probed\n", revision_id); > >> + dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n", > >> + ctx->vender_id, ctx->device_id, ctx->revision); > >> > >> return 0; > >> } > >> -- > >> 2.34.1 > >> > >
Hi, On 2023/11/16 21:00, Dmitry Baryshkov wrote: > On Thu, 16 Nov 2023 at 14:18, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: >> Hi, >> >> >> On 2023/11/15 00:06, Dmitry Baryshkov wrote: >>> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: >>>> From: Sui Jingfeng <suijingfeng@loongson.cn> >>>> >>>> Read the required chip id data back by calling regmap_bulk_read() once, >>>> reduce the number of local variables needed in it66121_probe() function. >>>> And store its values into struct it66121_ctx, as it will be used latter. >>>> >>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn> >>>> --- >>>> drivers/gpu/drm/bridge/ite-it66121.c | 47 ++++++++++++++++++++-------- >>>> 1 file changed, 34 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c >>>> index 7e473beefc79..f36d05331f25 100644 >>>> --- a/drivers/gpu/drm/bridge/ite-it66121.c >>>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c >>>> @@ -313,6 +313,9 @@ struct it66121_ctx { >>>> bool auto_cts; >>>> } audio; >>>> const struct it66121_chip_info *info; >>>> + u16 vender_id; >>>> + u16 device_id; >>>> + u8 revision; >>> There is no need to store them, they are not used by the driver anywhere. >>> >>>> }; >>>> >>>> static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge) >>>> @@ -399,6 +402,30 @@ static void it66121_hw_reset(struct it66121_ctx *ctx) >>>> gpiod_set_value(ctx->gpio_reset, 0); >>>> } >>>> >>>> +static int it66121_read_chip_id(struct it66121_ctx *ctx, bool verbose) >>>> +{ >>>> + u8 id[4]; >>>> + int ret; >>>> + >>>> + ret = regmap_bulk_read(ctx->regmap, IT66121_VENDOR_ID0_REG, id, 4); >>>> + if (ret < 0) { >>>> + dev_err(ctx->dev, "Failed to read chip ID: %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + ctx->vender_id = (u16)id[1] << 8 | id[0]; >>>> + ctx->device_id = ((u16)(id[3] & IT66121_DEVICE_ID1_MASK) << 8 | id[2]); >>>> + /* Revision is shared with DEVICE_ID1 */ >>>> + ctx->revision = FIELD_GET(IT66121_REVISION_MASK, id[3]); >>>> + >>>> + if (verbose) { >>>> + dev_info(ctx->dev, "Found ITE66121: 0x%x%x, revision: %u\n", >>>> + ctx->vender_id, ctx->device_id, ctx->revision); >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static inline int it66121_preamble_ddc(struct it66121_ctx *ctx) >>>> { >>>> return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST); >>>> @@ -1561,7 +1588,6 @@ static const char * const it66121_supplies[] = { >>>> >>>> static int it66121_probe(struct i2c_client *client) >>>> { >>>> - u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 }; >>>> int ret; >>>> struct it66121_ctx *ctx; >>>> struct device *dev = &client->dev; >>>> @@ -1603,19 +1629,13 @@ static int it66121_probe(struct i2c_client *client) >>>> if (IS_ERR(ctx->regmap)) >>>> return PTR_ERR(ctx->regmap); >>>> >>>> - regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]); >>>> - regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]); >>>> - regmap_read(ctx->regmap, IT66121_DEVICE_ID0_REG, &device_ids[0]); >>>> - regmap_read(ctx->regmap, IT66121_DEVICE_ID1_REG, &device_ids[1]); >>>> - >>>> - /* Revision is shared with DEVICE_ID1 */ >>>> - revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]); >>>> - device_ids[1] &= IT66121_DEVICE_ID1_MASK; >>>> + ret = it66121_read_chip_id(ctx, false); >>>> + if (ret) >>>> + return ret; >>>> >>>> - if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid || >>>> - (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) { >>>> + if (ctx->vender_id != ctx->info->vid || >>>> + ctx->device_id != ctx->info->pid) >> Q: There is no need to store them, they are not used by the driver anywhere. >> >> A: Here it is used, it is also used by the 0007-patch to get the entity(instance)-specific data. > And the patch 7 will be changed once you have proper i2c client struct > registered. > We will create a "wrong"i2c client intentionally by calling i2c_new_client_device, the "wrong" here means that 'board_info.type' is not equal to "it66121" or "it6610". I probably know what you means, but please note that we are not object you at here. The reason why we prefer theit66121_get_match_data() over the i2c_get_match_data() is that our it66121_get_match_data() works with *the least dependency*. Ourversion don't rely on the DT to provide compatible strings. Because the it66121 itself already provided constant hard-coded vender id and device id on-chip register. This is a bit similar with the PCIe devices. This chip id speak everything about chip identify. By re-arranging the variants into array, searching and matching against became straight-forward. We can also utilize the vender id and device id information to dynamic bind the instance specific functions *at the setup(probe) time*. Which helps to untangle the it66121 and it6610 cases with function object. Currently they are tangled. This is not a problem simply because the model supported is too small(less). >> >> Since it6610 was introduced, this is used for chip identifying. >> It can also be used with in debugfs context, to show who I am. > I'd say, there is little point in whoami debugfs files. Debugfs is for > the useful information. > >> >>>> return -ENODEV; >>>> - } >>>> >>>> ctx->bridge.funcs = &it66121_bridge_funcs; >>>> ctx->bridge.of_node = dev->of_node; >>>> @@ -1633,7 +1653,8 @@ static int it66121_probe(struct i2c_client *client) >>>> >>>> drm_bridge_add(&ctx->bridge); >>>> >>>> - dev_info(dev, "IT66121 revision %d probed\n", revision_id); >>>> + dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n", >>>> + ctx->vender_id, ctx->device_id, ctx->revision); >>>> >>>> return 0; >>>> } >>>> -- >>>> 2.34.1 >>>> > >
On Thu, 16 Nov 2023 at 20:29, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > > Hi, > > > On 2023/11/16 21:00, Dmitry Baryshkov wrote: > > On Thu, 16 Nov 2023 at 14:18, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > >> Hi, > >> > >> > >> On 2023/11/15 00:06, Dmitry Baryshkov wrote: > >>> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > >>>> From: Sui Jingfeng <suijingfeng@loongson.cn> > >>>> > >>>> Read the required chip id data back by calling regmap_bulk_read() once, > >>>> reduce the number of local variables needed in it66121_probe() function. > >>>> And store its values into struct it66121_ctx, as it will be used latter. > >>>> > >>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn> > >>>> --- > >>>> drivers/gpu/drm/bridge/ite-it66121.c | 47 ++++++++++++++++++++-------- > >>>> 1 file changed, 34 insertions(+), 13 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c > >>>> index 7e473beefc79..f36d05331f25 100644 > >>>> --- a/drivers/gpu/drm/bridge/ite-it66121.c > >>>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c > >>>> @@ -313,6 +313,9 @@ struct it66121_ctx { > >>>> bool auto_cts; > >>>> } audio; > >>>> const struct it66121_chip_info *info; > >>>> + u16 vender_id; > >>>> + u16 device_id; > >>>> + u8 revision; > >>> There is no need to store them, they are not used by the driver anywhere. > >>> > >>>> }; > >>>> > >>>> static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge) > >>>> @@ -399,6 +402,30 @@ static void it66121_hw_reset(struct it66121_ctx *ctx) > >>>> gpiod_set_value(ctx->gpio_reset, 0); > >>>> } > >>>> > >>>> +static int it66121_read_chip_id(struct it66121_ctx *ctx, bool verbose) > >>>> +{ > >>>> + u8 id[4]; > >>>> + int ret; > >>>> + > >>>> + ret = regmap_bulk_read(ctx->regmap, IT66121_VENDOR_ID0_REG, id, 4); > >>>> + if (ret < 0) { > >>>> + dev_err(ctx->dev, "Failed to read chip ID: %d\n", ret); > >>>> + return ret; > >>>> + } > >>>> + > >>>> + ctx->vender_id = (u16)id[1] << 8 | id[0]; > >>>> + ctx->device_id = ((u16)(id[3] & IT66121_DEVICE_ID1_MASK) << 8 | id[2]); > >>>> + /* Revision is shared with DEVICE_ID1 */ > >>>> + ctx->revision = FIELD_GET(IT66121_REVISION_MASK, id[3]); > >>>> + > >>>> + if (verbose) { > >>>> + dev_info(ctx->dev, "Found ITE66121: 0x%x%x, revision: %u\n", > >>>> + ctx->vender_id, ctx->device_id, ctx->revision); > >>>> + } > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> static inline int it66121_preamble_ddc(struct it66121_ctx *ctx) > >>>> { > >>>> return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST); > >>>> @@ -1561,7 +1588,6 @@ static const char * const it66121_supplies[] = { > >>>> > >>>> static int it66121_probe(struct i2c_client *client) > >>>> { > >>>> - u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 }; > >>>> int ret; > >>>> struct it66121_ctx *ctx; > >>>> struct device *dev = &client->dev; > >>>> @@ -1603,19 +1629,13 @@ static int it66121_probe(struct i2c_client *client) > >>>> if (IS_ERR(ctx->regmap)) > >>>> return PTR_ERR(ctx->regmap); > >>>> > >>>> - regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]); > >>>> - regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]); > >>>> - regmap_read(ctx->regmap, IT66121_DEVICE_ID0_REG, &device_ids[0]); > >>>> - regmap_read(ctx->regmap, IT66121_DEVICE_ID1_REG, &device_ids[1]); > >>>> - > >>>> - /* Revision is shared with DEVICE_ID1 */ > >>>> - revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]); > >>>> - device_ids[1] &= IT66121_DEVICE_ID1_MASK; > >>>> + ret = it66121_read_chip_id(ctx, false); > >>>> + if (ret) > >>>> + return ret; > >>>> > >>>> - if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid || > >>>> - (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) { > >>>> + if (ctx->vender_id != ctx->info->vid || > >>>> + ctx->device_id != ctx->info->pid) > >> Q: There is no need to store them, they are not used by the driver anywhere. > >> > >> A: Here it is used, it is also used by the 0007-patch to get the entity(instance)-specific data. > > And the patch 7 will be changed once you have proper i2c client struct > > registered. > > > We will create a "wrong"i2c client intentionally by calling i2c_new_client_device, the "wrong" > here means that 'board_info.type' is not equal to "it66121" or "it6610". > > I probably know what you means, but please note that we are not object you at here. > The reason why we prefer theit66121_get_match_data() over the i2c_get_match_data() is that our > it66121_get_match_data() works with *the least dependency*. Ourversion > don't rely on the DT to provide compatible strings. Because the it66121 itself already > provided constant hard-coded vender id and device id on-chip register. This is a bit > similar with the PCIe devices. This chip id speak everything about chip identify. > By re-arranging the variants into array, searching and matching against became > straight-forward. We can also utilize the vender id and device id information to > dynamic bind the instance specific functions *at the setup(probe) time*. Which helps > to untangle the it66121 and it6610 cases with function object. Currently they are > tangled. This is not a problem simply because the model supported is too small(less). The difference is that PCI or USB are discoverable buses, but I2C and SPI are not. The kernel tends to use generic mechanisms if possible. And for the I2C / SPI buses the generic mechanism is match data. What you are doing is nice if there was just the it66121 driver. However there is more than that. Many more drivers. And the generic approach becomes important, as it helps other developers to work with better efficiency. > >> Since it6610 was introduced, this is used for chip identifying. > >> It can also be used with in debugfs context, to show who I am. > > I'd say, there is little point in whoami debugfs files. Debugfs is for > > the useful information. > > > >> > >>>> return -ENODEV; > >>>> - } > >>>> > >>>> ctx->bridge.funcs = &it66121_bridge_funcs; > >>>> ctx->bridge.of_node = dev->of_node; > >>>> @@ -1633,7 +1653,8 @@ static int it66121_probe(struct i2c_client *client) > >>>> > >>>> drm_bridge_add(&ctx->bridge); > >>>> > >>>> - dev_info(dev, "IT66121 revision %d probed\n", revision_id); > >>>> + dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n", > >>>> + ctx->vender_id, ctx->device_id, ctx->revision); > >>>> > >>>> return 0; > >>>> } > >>>> -- > >>>> 2.34.1 > >>>> > > > >
Hi, On 2023/11/16 21:00, Dmitry Baryshkov wrote: > On Thu, 16 Nov 2023 at 14:18, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: >> Hi, >> >> >> On 2023/11/15 00:06, Dmitry Baryshkov wrote: >>> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: >>>> From: Sui Jingfeng <suijingfeng@loongson.cn> >>>> >>>> Read the required chip id data back by calling regmap_bulk_read() once, >>>> reduce the number of local variables needed in it66121_probe() function. >>>> And store its values into struct it66121_ctx, as it will be used latter. >>>> >>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn> >>>> --- >>>> drivers/gpu/drm/bridge/ite-it66121.c | 47 ++++++++++++++++++++-------- >>>> 1 file changed, 34 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c >>>> index 7e473beefc79..f36d05331f25 100644 >>>> --- a/drivers/gpu/drm/bridge/ite-it66121.c >>>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c >>>> @@ -313,6 +313,9 @@ struct it66121_ctx { >>>> bool auto_cts; >>>> } audio; >>>> const struct it66121_chip_info *info; >>>> + u16 vender_id; >>>> + u16 device_id; >>>> + u8 revision; >>> There is no need to store them, they are not used by the driver anywhere. >>> >>>> }; >>>> >>>> static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge) >>>> @@ -399,6 +402,30 @@ static void it66121_hw_reset(struct it66121_ctx *ctx) >>>> gpiod_set_value(ctx->gpio_reset, 0); >>>> } >>>> >>>> +static int it66121_read_chip_id(struct it66121_ctx *ctx, bool verbose) >>>> +{ >>>> + u8 id[4]; >>>> + int ret; >>>> + >>>> + ret = regmap_bulk_read(ctx->regmap, IT66121_VENDOR_ID0_REG, id, 4); >>>> + if (ret < 0) { >>>> + dev_err(ctx->dev, "Failed to read chip ID: %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + ctx->vender_id = (u16)id[1] << 8 | id[0]; >>>> + ctx->device_id = ((u16)(id[3] & IT66121_DEVICE_ID1_MASK) << 8 | id[2]); >>>> + /* Revision is shared with DEVICE_ID1 */ >>>> + ctx->revision = FIELD_GET(IT66121_REVISION_MASK, id[3]); >>>> + >>>> + if (verbose) { >>>> + dev_info(ctx->dev, "Found ITE66121: 0x%x%x, revision: %u\n", >>>> + ctx->vender_id, ctx->device_id, ctx->revision); >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static inline int it66121_preamble_ddc(struct it66121_ctx *ctx) >>>> { >>>> return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST); >>>> @@ -1561,7 +1588,6 @@ static const char * const it66121_supplies[] = { >>>> >>>> static int it66121_probe(struct i2c_client *client) >>>> { >>>> - u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 }; >>>> int ret; >>>> struct it66121_ctx *ctx; >>>> struct device *dev = &client->dev; >>>> @@ -1603,19 +1629,13 @@ static int it66121_probe(struct i2c_client *client) >>>> if (IS_ERR(ctx->regmap)) >>>> return PTR_ERR(ctx->regmap); >>>> >>>> - regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]); >>>> - regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]); >>>> - regmap_read(ctx->regmap, IT66121_DEVICE_ID0_REG, &device_ids[0]); >>>> - regmap_read(ctx->regmap, IT66121_DEVICE_ID1_REG, &device_ids[1]); >>>> - >>>> - /* Revision is shared with DEVICE_ID1 */ >>>> - revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]); >>>> - device_ids[1] &= IT66121_DEVICE_ID1_MASK; >>>> + ret = it66121_read_chip_id(ctx, false); >>>> + if (ret) >>>> + return ret; >>>> >>>> - if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid || >>>> - (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) { >>>> + if (ctx->vender_id != ctx->info->vid || >>>> + ctx->device_id != ctx->info->pid) >> Q: There is no need to store them, they are not used by the driver anywhere. >> >> A: Here it is used, it is also used by the 0007-patch to get the entity(instance)-specific data. > And the patch 7 will be changed once you have proper i2c client struct > registered. > >> >> Since it6610 was introduced, this is used for chip identifying. >> It can also be used with in debugfs context, to show who I am. > I'd say, there is little point in whoami debugfs files. Debugfs is for > the useful information. Sinceit6610 was introduced, how do you know what the device it66121 driver is binding? Printing model specific information is common practice for a large driver. Especially if you can only able to debug remotely where only a SSH is given. You could see debugfs of drm/etnaviv for a reference. It is common to testing a large driver running on 20+ machines with various hardware model. >> >>>> return -ENODEV; >>>> - } >>>> >>>> ctx->bridge.funcs = &it66121_bridge_funcs; >>>> ctx->bridge.of_node = dev->of_node; >>>> @@ -1633,7 +1653,8 @@ static int it66121_probe(struct i2c_client *client) >>>> >>>> drm_bridge_add(&ctx->bridge); >>>> >>>> - dev_info(dev, "IT66121 revision %d probed\n", revision_id); >>>> + dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n", >>>> + ctx->vender_id, ctx->device_id, ctx->revision); >>>> >>>> return 0; >>>> } >>>> -- >>>> 2.34.1 >>>> > >
On Thu, 23 Nov 2023 at 07:37, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > > Hi, > > > On 2023/11/16 21:00, Dmitry Baryshkov wrote: > > On Thu, 16 Nov 2023 at 14:18, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > >> Hi, > >> > >> > >> On 2023/11/15 00:06, Dmitry Baryshkov wrote: > >>> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > >>>> From: Sui Jingfeng <suijingfeng@loongson.cn> > >>>> > >>>> Read the required chip id data back by calling regmap_bulk_read() once, > >>>> reduce the number of local variables needed in it66121_probe() function. > >>>> And store its values into struct it66121_ctx, as it will be used latter. > >>>> > >>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn> > >>>> --- > >>>> drivers/gpu/drm/bridge/ite-it66121.c | 47 ++++++++++++++++++++-------- > >>>> 1 file changed, 34 insertions(+), 13 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c > >>>> index 7e473beefc79..f36d05331f25 100644 > >>>> --- a/drivers/gpu/drm/bridge/ite-it66121.c > >>>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c > >>>> @@ -313,6 +313,9 @@ struct it66121_ctx { > >>>> bool auto_cts; > >>>> } audio; > >>>> const struct it66121_chip_info *info; > >>>> + u16 vender_id; > >>>> + u16 device_id; > >>>> + u8 revision; > >>> There is no need to store them, they are not used by the driver anywhere. > >>> > >>>> }; > >>>> > >>>> static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge) > >>>> @@ -399,6 +402,30 @@ static void it66121_hw_reset(struct it66121_ctx *ctx) > >>>> gpiod_set_value(ctx->gpio_reset, 0); > >>>> } > >>>> > >>>> +static int it66121_read_chip_id(struct it66121_ctx *ctx, bool verbose) > >>>> +{ > >>>> + u8 id[4]; > >>>> + int ret; > >>>> + > >>>> + ret = regmap_bulk_read(ctx->regmap, IT66121_VENDOR_ID0_REG, id, 4); > >>>> + if (ret < 0) { > >>>> + dev_err(ctx->dev, "Failed to read chip ID: %d\n", ret); > >>>> + return ret; > >>>> + } > >>>> + > >>>> + ctx->vender_id = (u16)id[1] << 8 | id[0]; > >>>> + ctx->device_id = ((u16)(id[3] & IT66121_DEVICE_ID1_MASK) << 8 | id[2]); > >>>> + /* Revision is shared with DEVICE_ID1 */ > >>>> + ctx->revision = FIELD_GET(IT66121_REVISION_MASK, id[3]); > >>>> + > >>>> + if (verbose) { > >>>> + dev_info(ctx->dev, "Found ITE66121: 0x%x%x, revision: %u\n", > >>>> + ctx->vender_id, ctx->device_id, ctx->revision); > >>>> + } > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> static inline int it66121_preamble_ddc(struct it66121_ctx *ctx) > >>>> { > >>>> return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST); > >>>> @@ -1561,7 +1588,6 @@ static const char * const it66121_supplies[] = { > >>>> > >>>> static int it66121_probe(struct i2c_client *client) > >>>> { > >>>> - u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 }; > >>>> int ret; > >>>> struct it66121_ctx *ctx; > >>>> struct device *dev = &client->dev; > >>>> @@ -1603,19 +1629,13 @@ static int it66121_probe(struct i2c_client *client) > >>>> if (IS_ERR(ctx->regmap)) > >>>> return PTR_ERR(ctx->regmap); > >>>> > >>>> - regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]); > >>>> - regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]); > >>>> - regmap_read(ctx->regmap, IT66121_DEVICE_ID0_REG, &device_ids[0]); > >>>> - regmap_read(ctx->regmap, IT66121_DEVICE_ID1_REG, &device_ids[1]); > >>>> - > >>>> - /* Revision is shared with DEVICE_ID1 */ > >>>> - revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]); > >>>> - device_ids[1] &= IT66121_DEVICE_ID1_MASK; > >>>> + ret = it66121_read_chip_id(ctx, false); > >>>> + if (ret) > >>>> + return ret; > >>>> > >>>> - if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid || > >>>> - (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) { > >>>> + if (ctx->vender_id != ctx->info->vid || > >>>> + ctx->device_id != ctx->info->pid) > >> Q: There is no need to store them, they are not used by the driver anywhere. > >> > >> A: Here it is used, it is also used by the 0007-patch to get the entity(instance)-specific data. > > And the patch 7 will be changed once you have proper i2c client struct > > registered. > > > >> > >> Since it6610 was introduced, this is used for chip identifying. > >> It can also be used with in debugfs context, to show who I am. > > I'd say, there is little point in whoami debugfs files. Debugfs is for > > the useful information. > > Sinceit6610 was introduced, how do you know what the device it66121 driver is > binding? Printing model specific information is common practice for a > large driver. Especially if you can only able to debug remotely where > only a SSH is given. cat /sys/bus/i2c/devices/.../name > You could see debugfs of drm/etnaviv for a > reference. It is common to testing a large driver running on 20+ > machines with various hardware model. > > > >> > >>>> return -ENODEV; > >>>> - } > >>>> > >>>> ctx->bridge.funcs = &it66121_bridge_funcs; > >>>> ctx->bridge.of_node = dev->of_node; > >>>> @@ -1633,7 +1653,8 @@ static int it66121_probe(struct i2c_client *client) > >>>> > >>>> drm_bridge_add(&ctx->bridge); > >>>> > >>>> - dev_info(dev, "IT66121 revision %d probed\n", revision_id); > >>>> + dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n", > >>>> + ctx->vender_id, ctx->device_id, ctx->revision); > >>>> > >>>> return 0; > >>>> } > >>>> -- > >>>> 2.34.1 > >>>> > > > >
Hi, On 2023/11/23 15:48, Dmitry Baryshkov wrote: > On Thu, 23 Nov 2023 at 07:37, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: >> Hi, >> >> >> On 2023/11/16 21:00, Dmitry Baryshkov wrote: >>> On Thu, 16 Nov 2023 at 14:18, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: >>>> Hi, >>>> >>>> >>>> On 2023/11/15 00:06, Dmitry Baryshkov wrote: >>>>> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: >>>>>> From: Sui Jingfeng <suijingfeng@loongson.cn> >>>>>> >>>>>> Read the required chip id data back by calling regmap_bulk_read() once, >>>>>> reduce the number of local variables needed in it66121_probe() function. >>>>>> And store its values into struct it66121_ctx, as it will be used latter. >>>>>> >>>>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn> >>>>>> --- >>>>>> drivers/gpu/drm/bridge/ite-it66121.c | 47 ++++++++++++++++++++-------- >>>>>> 1 file changed, 34 insertions(+), 13 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c >>>>>> index 7e473beefc79..f36d05331f25 100644 >>>>>> --- a/drivers/gpu/drm/bridge/ite-it66121.c >>>>>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c >>>>>> @@ -313,6 +313,9 @@ struct it66121_ctx { >>>>>> bool auto_cts; >>>>>> } audio; >>>>>> const struct it66121_chip_info *info; >>>>>> + u16 vender_id; >>>>>> + u16 device_id; >>>>>> + u8 revision; >>>>> There is no need to store them, they are not used by the driver anywhere. >>>>> >>>>>> }; >>>>>> >>>>>> static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge) >>>>>> @@ -399,6 +402,30 @@ static void it66121_hw_reset(struct it66121_ctx *ctx) >>>>>> gpiod_set_value(ctx->gpio_reset, 0); >>>>>> } >>>>>> >>>>>> +static int it66121_read_chip_id(struct it66121_ctx *ctx, bool verbose) >>>>>> +{ >>>>>> + u8 id[4]; >>>>>> + int ret; >>>>>> + >>>>>> + ret = regmap_bulk_read(ctx->regmap, IT66121_VENDOR_ID0_REG, id, 4); >>>>>> + if (ret < 0) { >>>>>> + dev_err(ctx->dev, "Failed to read chip ID: %d\n", ret); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + ctx->vender_id = (u16)id[1] << 8 | id[0]; >>>>>> + ctx->device_id = ((u16)(id[3] & IT66121_DEVICE_ID1_MASK) << 8 | id[2]); >>>>>> + /* Revision is shared with DEVICE_ID1 */ >>>>>> + ctx->revision = FIELD_GET(IT66121_REVISION_MASK, id[3]); >>>>>> + >>>>>> + if (verbose) { >>>>>> + dev_info(ctx->dev, "Found ITE66121: 0x%x%x, revision: %u\n", >>>>>> + ctx->vender_id, ctx->device_id, ctx->revision); >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> static inline int it66121_preamble_ddc(struct it66121_ctx *ctx) >>>>>> { >>>>>> return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST); >>>>>> @@ -1561,7 +1588,6 @@ static const char * const it66121_supplies[] = { >>>>>> >>>>>> static int it66121_probe(struct i2c_client *client) >>>>>> { >>>>>> - u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 }; >>>>>> int ret; >>>>>> struct it66121_ctx *ctx; >>>>>> struct device *dev = &client->dev; >>>>>> @@ -1603,19 +1629,13 @@ static int it66121_probe(struct i2c_client *client) >>>>>> if (IS_ERR(ctx->regmap)) >>>>>> return PTR_ERR(ctx->regmap); >>>>>> >>>>>> - regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]); >>>>>> - regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]); >>>>>> - regmap_read(ctx->regmap, IT66121_DEVICE_ID0_REG, &device_ids[0]); >>>>>> - regmap_read(ctx->regmap, IT66121_DEVICE_ID1_REG, &device_ids[1]); >>>>>> - >>>>>> - /* Revision is shared with DEVICE_ID1 */ >>>>>> - revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]); >>>>>> - device_ids[1] &= IT66121_DEVICE_ID1_MASK; >>>>>> + ret = it66121_read_chip_id(ctx, false); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> >>>>>> - if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid || >>>>>> - (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) { >>>>>> + if (ctx->vender_id != ctx->info->vid || >>>>>> + ctx->device_id != ctx->info->pid) >>>> Q: There is no need to store them, they are not used by the driver anywhere. >>>> >>>> A: Here it is used, it is also used by the 0007-patch to get the entity(instance)-specific data. >>> And the patch 7 will be changed once you have proper i2c client struct >>> registered. >>> >>>> Since it6610 was introduced, this is used for chip identifying. >>>> It can also be used with in debugfs context, to show who I am. >>> I'd say, there is little point in whoami debugfs files. Debugfs is for >>> the useful information. >> Sinceit6610 was introduced, how do you know what the device it66121 driver is >> binding? Printing model specific information is common practice for a >> large driver. Especially if you can only able to debug remotely where >> only a SSH is given. > cat /sys/bus/i2c/devices/.../name No, this is not enough. This is also not straight-forward. Because the system will have multiple i2c device. This is a bit hard to find which one is corresponding to the it66121. This is needed during the development and debug phase. We will also want to know the vendor id, device id and revision id. As those chip id are constant values, matching those chip id against the values in datasheet is a kind of sanity test. Which helps us to know if the i2c regmap(the communication channel) has been setup successfully or not. >> You could see debugfs of drm/etnaviv for a >> reference. It is common to testing a large driver running on 20+ >> machines with various hardware model. >> >> >>>>>> return -ENODEV; >>>>>> - } >>>>>> >>>>>> ctx->bridge.funcs = &it66121_bridge_funcs; >>>>>> ctx->bridge.of_node = dev->of_node; >>>>>> @@ -1633,7 +1653,8 @@ static int it66121_probe(struct i2c_client *client) >>>>>> >>>>>> drm_bridge_add(&ctx->bridge); >>>>>> >>>>>> - dev_info(dev, "IT66121 revision %d probed\n", revision_id); >>>>>> + dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n", >>>>>> + ctx->vender_id, ctx->device_id, ctx->revision); >>>>>> >>>>>> return 0; >>>>>> } >>>>>> -- >>>>>> 2.34.1 >>>>>> >>> > >
diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c index 7e473beefc79..f36d05331f25 100644 --- a/drivers/gpu/drm/bridge/ite-it66121.c +++ b/drivers/gpu/drm/bridge/ite-it66121.c @@ -313,6 +313,9 @@ struct it66121_ctx { bool auto_cts; } audio; const struct it66121_chip_info *info; + u16 vender_id; + u16 device_id; + u8 revision; }; static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge) @@ -399,6 +402,30 @@ static void it66121_hw_reset(struct it66121_ctx *ctx) gpiod_set_value(ctx->gpio_reset, 0); } +static int it66121_read_chip_id(struct it66121_ctx *ctx, bool verbose) +{ + u8 id[4]; + int ret; + + ret = regmap_bulk_read(ctx->regmap, IT66121_VENDOR_ID0_REG, id, 4); + if (ret < 0) { + dev_err(ctx->dev, "Failed to read chip ID: %d\n", ret); + return ret; + } + + ctx->vender_id = (u16)id[1] << 8 | id[0]; + ctx->device_id = ((u16)(id[3] & IT66121_DEVICE_ID1_MASK) << 8 | id[2]); + /* Revision is shared with DEVICE_ID1 */ + ctx->revision = FIELD_GET(IT66121_REVISION_MASK, id[3]); + + if (verbose) { + dev_info(ctx->dev, "Found ITE66121: 0x%x%x, revision: %u\n", + ctx->vender_id, ctx->device_id, ctx->revision); + } + + return 0; +} + static inline int it66121_preamble_ddc(struct it66121_ctx *ctx) { return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST); @@ -1561,7 +1588,6 @@ static const char * const it66121_supplies[] = { static int it66121_probe(struct i2c_client *client) { - u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 }; int ret; struct it66121_ctx *ctx; struct device *dev = &client->dev; @@ -1603,19 +1629,13 @@ static int it66121_probe(struct i2c_client *client) if (IS_ERR(ctx->regmap)) return PTR_ERR(ctx->regmap); - regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]); - regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]); - regmap_read(ctx->regmap, IT66121_DEVICE_ID0_REG, &device_ids[0]); - regmap_read(ctx->regmap, IT66121_DEVICE_ID1_REG, &device_ids[1]); - - /* Revision is shared with DEVICE_ID1 */ - revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]); - device_ids[1] &= IT66121_DEVICE_ID1_MASK; + ret = it66121_read_chip_id(ctx, false); + if (ret) + return ret; - if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid || - (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) { + if (ctx->vender_id != ctx->info->vid || + ctx->device_id != ctx->info->pid) return -ENODEV; - } ctx->bridge.funcs = &it66121_bridge_funcs; ctx->bridge.of_node = dev->of_node; @@ -1633,7 +1653,8 @@ static int it66121_probe(struct i2c_client *client) drm_bridge_add(&ctx->bridge); - dev_info(dev, "IT66121 revision %d probed\n", revision_id); + dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n", + ctx->vender_id, ctx->device_id, ctx->revision); return 0; }