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 X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 73EECC43441 for ; Wed, 14 Nov 2018 10:04:26 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 35F222145D for ; Wed, 14 Nov 2018 10:04:26 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 35F222145D Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=lucaceresoli.net Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732800AbeKNUG5 (ORCPT ); Wed, 14 Nov 2018 15:06:57 -0500 Received: from srv-hp10-72.netsons.net ([94.141.22.72]:38325 "EHLO srv-hp10-72.netsons.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1732664AbeKNUG5 (ORCPT ); Wed, 14 Nov 2018 15:06:57 -0500 Received: from [109.168.11.45] (port=41866 helo=[192.168.101.125]) by srv-hp10.netsons.net with esmtpsa (TLSv1.2:ECDHE-RSA-AES128-GCM-SHA256:128) (Exim 4.91) (envelope-from ) id 1gMs1u-009JTI-Fj; Wed, 14 Nov 2018 11:04:14 +0100 Subject: Re: [PATCH v4 3/4] media: i2c: Add MAX9286 driver To: kieran.bingham@ideasonboard.com, linux-renesas-soc@vger.kernel.org, linux-media@vger.kernel.org, devicetree@vger.kernel.org, sakari.ailus@iki.fi Cc: =?UTF-8?Q?Niklas_S=c3=b6derlund?= , Jacopo Mondi , Laurent Pinchart , Kieran Bingham , linux-kernel@vger.kernel.org, Jacopo Mondi , Laurent Pinchart , =?UTF-8?Q?Niklas_S=c3=b6derlund?= References: <20181102154723.23662-1-kieran.bingham@ideasonboard.com> <20181102154723.23662-4-kieran.bingham@ideasonboard.com> <5238fa80-7678-97a8-47ee-6a26970d862d@lucaceresoli.net> <07ee8a2c-81a8-ca32-96cf-67d6a883e3f5@ideasonboard.com> From: Luca Ceresoli Message-ID: Date: Wed, 14 Nov 2018 11:04:13 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <07ee8a2c-81a8-ca32-96cf-67d6a883e3f5@ideasonboard.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - srv-hp10.netsons.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - lucaceresoli.net X-Get-Message-Sender-Via: srv-hp10.netsons.net: authenticated_id: luca@lucaceresoli.net X-Authenticated-Sender: srv-hp10.netsons.net: luca@lucaceresoli.net X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Kieran, On 14/11/18 01:46, Kieran Bingham wrote: > Hi Luca, > > Thank you for your review, > > On 13/11/2018 14:49, Luca Ceresoli wrote: >> Hi Kieran, All, >> >> below a few minor questions, and a big one at the bottom. >> >> On 02/11/18 16:47, Kieran Bingham wrote: >>> From: Kieran Bingham >>> >>> The MAX9286 is a 4-channel GMSL deserializer with coax or STP input and >>> CSI-2 output. The device supports multicamera streaming applications, >>> and features the ability to synchronise the attached cameras. >>> >>> CSI-2 output can be configured with 1 to 4 lanes, and a control channel >>> is supported over I2C, which implements an I2C mux to facilitate >>> communications with connected cameras across the reverse control >>> channel. >>> >>> Signed-off-by: Jacopo Mondi >>> Signed-off-by: Kieran Bingham >>> Signed-off-by: Laurent Pinchart >>> Signed-off-by: Niklas Söderlund >> >> [...] >> >>> +struct max9286_device { >>> + struct i2c_client *client; >>> + struct v4l2_subdev sd; >>> + struct media_pad pads[MAX9286_N_PADS]; >>> + struct regulator *regulator; >>> + bool poc_enabled; >>> + int streaming; >>> + >>> + struct i2c_mux_core *mux; >>> + unsigned int mux_channel; >>> + >>> + struct v4l2_ctrl_handler ctrls; >>> + >>> + struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS]; >> >> 5 pads, 4 formats. Why does the source node have no fmt? > > The source pad is a CSI2 link - so a 'frame format' would be inappropriate. Ok, thanks for the clarification. >>> +static int max9286_probe(struct i2c_client *client, >>> + const struct i2c_device_id *did) >>> +{ >>> + struct max9286_device *dev; >>> + unsigned int i; >>> + int ret; >>> + >>> + dev = kzalloc(sizeof(*dev), GFP_KERNEL); >>> + if (!dev) >>> + return -ENOMEM; >>> + >>> + dev->client = client; >>> + i2c_set_clientdata(client, dev); >>> + >>> + for (i = 0; i < MAX9286_N_SINKS; i++) >>> + max9286_init_format(&dev->fmt[i]); >>> + >>> + ret = max9286_parse_dt(dev); >>> + if (ret) >>> + return ret; >>> + >>> + dev->regulator = regulator_get(&client->dev, "poc"); >>> + if (IS_ERR(dev->regulator)) { >>> + if (PTR_ERR(dev->regulator) != -EPROBE_DEFER) >>> + dev_err(&client->dev, >>> + "Unable to get PoC regulator (%ld)\n", >>> + PTR_ERR(dev->regulator)); >>> + ret = PTR_ERR(dev->regulator); >>> + goto err_free; >>> + } >>> + >>> + /* >>> + * We can have multiple MAX9286 instances on the same physical I2C >>> + * bus, and I2C children behind ports of separate MAX9286 instances >>> + * having the same I2C address. As the MAX9286 starts by default with >>> + * all ports enabled, we need to disable all ports on all MAX9286 >>> + * instances before proceeding to further initialize the devices and >>> + * instantiate children. >>> + * >>> + * Start by just disabling all channels on the current device. Then, >>> + * if all other MAX9286 on the parent bus have been probed, proceed >>> + * to initialize them all, including the current one. >>> + */ >>> + max9286_i2c_mux_close(dev); >>> + >>> + /* >>> + * The MAX9286 initialises with auto-acknowledge enabled by default. >>> + * This means that if multiple MAX9286 devices are connected to an I2C >>> + * bus, another MAX9286 could ack I2C transfers meant for a device on >>> + * the other side of the GMSL links for this MAX9286 (such as a >>> + * MAX9271). To prevent that disable auto-acknowledge early on; it >>> + * will be enabled later as needed. >>> + */ >>> + max9286_configure_i2c(dev, false); >>> + >>> + ret = device_for_each_child(client->dev.parent, &client->dev, >>> + max9286_is_bound); >>> + if (ret) >>> + return 0; >>> + >>> + dev_dbg(&client->dev, >>> + "All max9286 probed: start initialization sequence\n"); >>> + ret = device_for_each_child(client->dev.parent, NULL, >>> + max9286_init); >> >> I can't manage to like this initialization sequence, sorry. If at all >> possible, each max9286 should initialize itself independently from each >> other, like any normal driver. > > Yes, I think we're in agreement here, but unfortunately this section is > a workaround for the fact that our devices share a common address space. > > We (currently) *must* disable both devices before we start the > initialisation process for either on our platform currently... The model I proposed in my review to patch 1/4 (split remote physical address from local address pool) allows to avoid this workaround. > That said - I think this section needs to be removed from the upstream > part at least for now. I think we should probably carry this > 'workaround' separately. > > This part is the core issue that I talked about in my presentation at > ALS-Japan [0] > > [0] https://sched.co/EaXa Oh, interesting, I hadn't noticed that you gave this talk -- at the same conference as Vladimir's talk! No video recording apparently, but are slides available at least? >> First, it requires that each chip on the remote side can configure its >> own slave address. Not all chips do. >> >> Second, using a static i2c address map does not scale well and limits >> hotplugging, as I discussed in my reply to patch 1/4. The problem should >> be solvable cleanly if the MAX9286 supports address translation like the >> TI chips. > > I don't think we can treat GMSL as hot-pluggable currently ... But as we > discussed - I see that we should think about this for FPD-Link I've been mixing hotplug and DT overlays and that generated confusion, sorry. My point exists even with no hotplug, see the reply to patch 1/4. -- Luca