From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 478C7C197A0 for ; Thu, 16 Nov 2023 12:08:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1345249AbjKPMIW (ORCPT ); Thu, 16 Nov 2023 07:08:22 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59208 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1345243AbjKPMIU (ORCPT ); Thu, 16 Nov 2023 07:08:20 -0500 Received: from out-188.mta0.migadu.com (out-188.mta0.migadu.com [IPv6:2001:41d0:1004:224b::bc]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DBA2CB4 for ; Thu, 16 Nov 2023 04:08:15 -0800 (PST) Message-ID: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1700136493; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=k9oetIw2tFldJiOGCUKM2J/cABhuLHaFmSF5ziLE1Pg=; b=fJN0/POdxJ2qznbqNjU2TFEX8CyZdzr61R3U8qfTAtTrnZ5ECyVrFppcRpHoppC3dUMUZM ARrpL7w5VXh4t4g/uvKOK80LOUSfv5BbyKlUAo1CkCAsOeSEv5MM7H/40kq7FbbEEHz5ze 6UmPZPgVjJfxp2BbOoOKoTC/5htG+oo= Date: Thu, 16 Nov 2023 20:07:59 +0800 MIME-Version: 1.0 Subject: Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib Content-Language: en-US To: Dmitry Baryshkov Cc: Phong LE , Neil Armstrong , Maxime Ripard , Sui Jingfeng , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Laurent Pinchart , Thomas Zimmermann References: <20231114150130.497915-1-sui.jingfeng@linux.dev> <20231114150130.497915-9-sui.jingfeng@linux.dev> <1b59d647-c345-4260-b07b-22abb70ae17a@linux.dev> <7b85d057-3d66-435a-a657-dd69067b6bef@linux.dev> X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Sui Jingfeng In-Reply-To: <7b85d057-3d66-435a-a657-dd69067b6bef@linux.dev> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2023/11/16 19:53, Sui Jingfeng wrote: > Hi, > > > On 2023/11/16 19:29, Dmitry Baryshkov wrote: >> On Thu, 16 Nov 2023 at 13:18, Sui Jingfeng >> wrote: >>> Hi, >>> >>> >>> On 2023/11/15 00:30, Dmitry Baryshkov wrote: >>>>> + >>>>> +               ctx->connector = connector; >>>>> +       } >>>>> >>>>>           if (ctx->info->id == ID_IT66121) { >>>>>                   ret = regmap_write_bits(ctx->regmap, >>>>> IT66121_CLK_BANK_REG, >>>>> @@ -1632,16 +1651,13 @@ static const char * const >>>>> it66121_supplies[] = { >>>>>           "vcn33", "vcn18", "vrf12" >>>>>    }; >>>>> >>>>> -static int it66121_probe(struct i2c_client *client) >>>>> +int it66121_create_bridge(struct i2c_client *client, bool >>>>> of_support, >>>>> +                         bool hpd_support, bool audio_support, >>>>> +                         struct drm_bridge **bridge) >>>>>    { >>>>> +       struct device *dev = &client->dev; >>>>>           int ret; >>>>>           struct it66121_ctx *ctx; >>>>> -       struct device *dev = &client->dev; >>>>> - >>>>> -       if (!i2c_check_functionality(client->adapter, >>>>> I2C_FUNC_I2C)) { >>>>> -               dev_err(dev, "I2C check functionality failed.\n"); >>>>> -               return -ENXIO; >>>>> -       } >>>>> >>>>>           ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); >>>>>           if (!ctx) >>>>> @@ -1649,24 +1665,19 @@ static int it66121_probe(struct i2c_client >>>>> *client) >>>>> >>>>>           ctx->dev = dev; >>>>>           ctx->client = client; >>>>> -       ctx->info = i2c_get_match_data(client); >>>>> - >>>>> -       ret = it66121_of_read_bus_width(dev, &ctx->bus_width); >>>>> -       if (ret) >>>>> -               return ret; >>>>> - >>>>> -       ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge); >>>>> -       if (ret) >>>>> -               return ret; >>>>> - >>>>> -       i2c_set_clientdata(client, ctx); >>>>>           mutex_init(&ctx->lock); >>>>> >>>>> -       ret = devm_regulator_bulk_get_enable(dev, >>>>> ARRAY_SIZE(it66121_supplies), >>>>> - it66121_supplies); >>>>> -       if (ret) { >>>>> -               dev_err(dev, "Failed to enable power supplies\n"); >>>>> -               return ret; >>>>> +       if (of_support) { >>>>> +               ret = it66121_of_read_bus_width(dev, >>>>> &ctx->bus_width); >>>>> +               if (ret) >>>>> +                       return ret; >>>>> + >>>>> +               ret = it66121_of_get_next_bridge(dev, >>>>> &ctx->next_bridge); >>>>> +               if (ret) >>>>> +                       return ret; >>>>> +       } else { >>>>> +               ctx->bus_width = 24; >>>>> +               ctx->next_bridge = NULL; >>>>>           } >>>> A better alternative would be to turn OF calls into fwnode calls and >>>> to populate the fwnode properties. See >>>> drivers/platform/x86/intel/chtwc_int33fe.c for example. >>> >>> Honestly, I don't want to leave any scratch(breadcrumbs). >>> I'm worries about that turn OF calls into fwnode calls will leave >>> something unwanted. >>> >>> Because I am not sure if fwnode calls will make sense in the DT >>> world, while my patch >>> *still* be useful in the DT world. >> fwnode calls work for both DT and non-DT cases. In the DT case they >> work with DT nodes and properties. In the non-DT case, they work with >> manually populated properties. >> >>> Because the newly introduced it66121_create_bridge() >>> function is a core. I think It's better leave this task to a more >>> advance programmer. >>> if there have use case. It can be introduced at a latter time, >>> probably parallel with >>> the DT. >>> >>> I think DT and/or ACPI is best for integrated devices, but it66121 >>> display bridges is >>> a i2c slave device. Personally, I think slave device shouldn't be >>> standalone. I'm more >>> prefer to turn this driver to support hot-plug, even remove the >>> device on the run time >>> freely when detach and allow reattach. Like the I2C EEPROM device in >>> the monitor (which >>> contains the EDID, with I2C slave address 0x50). The I2C EEPROM >>> device *also* don't has >>> a corresponding struct device representation in linux kernel. >> It has. See i2c_client::dev. > > No, what I mean is that there don't have a device driver for > monitor(display) hardware entity. > And the drm_do_probe_ddc_edid() is the static linked driver, which is > similar with the idea > this series want to express. > > >>> so I still think It is best to make this drivers functional as a >>> static lib, but I want >>> to hear you to say more. Why it would be a *better* alternative to >>> turn OF calls into >>> fwnode calls? what are the potential benefits? >> Because then you can populate device properties from your root device. >> Because it allows the platform to specify the bus width instead of >> hardcoding 24 bits (which might work in your case, but might not be >> applicable to another user next week). > > > No, this problem can be easily solved. Simply add another argument. > > ``` > > int it66121_create_bridge(struct i2c_client *client, bool of_support, >                           bool hpd_support, bool audio_support, u32 > bus_width, >                           struct drm_bridge **bridge); > ``` > > >> Anyway, even without fwnode, I'd strongly suggest you to drop the >> it66121_create_bridge() as it is now and start by populating the i2c >> bus from your root device. > > This will force all non-DT users to add the similar code patter at the > display controller side, > which is another kind of duplication. The monitor is also as I2C slave > device, can be abstract > as a identify drm bridges in theory, I guess. > 'identify' -> 'identity' > >> Then you will need some way (fwnode?) to >> discover the bridge chain. And at the last point you will get into the >> device data and/or properties business. >> > No, leave that chance to a more better programmer and forgive me please, > too difficult, I'm afraid of not able to solve. Thanks a lot for the > trust! > >