From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id DD9C9315D5A; Fri, 5 Sep 2025 18:50:33 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1757098234; cv=none; b=Y7WFrzEcd3wi6sofxeucHV01880dc+WsIj/VlgZNvTZyERf2Hq/9x3f1fuyp+XDNQClqDlH/fmytW28q5zkZNsOywxmdqkqcjYDoAPIkPCrKkgx7DJnPF9QfxGMsVzbeiM+JPWI6eCnenPw5d3zYsmHNK08TZ1rwkjDu6CjilRc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1757098234; c=relaxed/simple; bh=/+is5jxNJVPx7UiHsW2765/QY6JX77gNcTLZ+G6khaQ=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=o8eKIiejVHkWb/MAdy3XOnvryiMDElUtlxG3QGDJ4siMgi5PEIVSmI0eHFeywuhJz1ekBj7/+P/4YMYr00v7Do0CPtZx8b/WqPuhYV1Ii4T+ylvGa0iSL7O/VKvlPD+BaMZECC2jmYJOI007W8rke/cDNLLhfzIdOrp4mpc2hP8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=VXxNLfPE; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="VXxNLfPE" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 18E2AC4CEF7; Fri, 5 Sep 2025 18:50:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1757098233; bh=/+is5jxNJVPx7UiHsW2765/QY6JX77gNcTLZ+G6khaQ=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=VXxNLfPEv0afBpCYlzaX/fYMiTb8MI/wrVUkGr8FVCnxLid4rw/W/8ci/h3BMQqiV YsiBQGRWS39TGFcERhOBvoPoMGx3oKpQMPkVI4UPF4MeJVMFKjxE7NE7FIEmu41XXY khgE0OI91T8y6Ihks5ep9rRSnhvViWmIXnlvQ0LGv6MQIcBdfQezGUez/YpgBjTBXV Nee5antpqhFZeZCKB8AWQ4phN8mZ8Vci0LhgjzQsia18H/SDdi75pdQPEN+tUYMt+2 12XJ4mL9Pllk32RD1eXis9PbNZKQnrejoZCUo7+e6ELRaXQJkYddxgM9btYbnPihkn 2Qr4d4kPOBnZA== Message-ID: <3f8e5fbc-fc33-43a2-93f2-be087f8a343e@kernel.org> Date: Fri, 5 Sep 2025 20:50:29 +0200 Precedence: bulk X-Mailing-List: linux-i2c@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 3/3] i2c: Add Intel USBIO I2C driver To: Sakari Ailus Cc: Israel Cepeda , Wolfram Sang , Andi Shyti , Greg Kroah-Hartman , Bartosz Golaszewski , Linus Walleij , Stanislaw Gruszka , Richard Hughes , linux-i2c@vger.kernel.org, linux-usb@vger.kernel.org, linux-gpio@vger.kernel.org References: <20250809102326.6032-1-hansg@kernel.org> <20250809102326.6032-4-hansg@kernel.org> From: Hans de Goede Content-Language: en-US, nl In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Hi, On 11-Aug-25 9:16 AM, Sakari Ailus wrote: > Hi Hans, Israel, > > My comments on newlines and parentheses apply to this one, too; I'm not > making new ones in similar locations on that subject anymore for this > patch. Ack I've added the necessary new-line changes, thank you for the review. > On Sat, Aug 09, 2025 at 12:23:26PM +0200, Hans de Goede wrote: >> From: Israel Cepeda >> >> Add a a driver for the I2C auxbus child device of the Intel USBIO USB >> IO-expander used by the MIPI cameras on various new (Meteor Lake and >> later) Intel laptops. >> >> Co-developed-by: Hans de Goede >> Signed-off-by: Hans de Goede >> Signed-off-by: Israel Cepeda ... >> diff --git a/drivers/i2c/busses/i2c-usbio.c b/drivers/i2c/busses/i2c-usbio.c >> new file mode 100644 >> index 000000000000..82c4769852f8 >> --- /dev/null >> +++ b/drivers/i2c/busses/i2c-usbio.c >> @@ -0,0 +1,344 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2025 Intel Corporation. >> + * Copyright (c) 2025 Red Hat, Inc. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#define I2C_RW_OVERHEAD (sizeof(struct usbio_bulk_packet) + sizeof(struct usbio_i2c_rw)) >> + >> +struct usbio_i2c { >> + u32 speed; > > You could declare this with the smaller fields for better struct packing. Ack, fixed for v2. >> + struct i2c_adapter adap; >> + struct auxiliary_device *adev; >> + struct usbio_i2c_rw *rwbuf; >> + bool init_supports_ack_flag; >> + u16 txbuf_len; >> + u16 rxbuf_len; >> +}; ... >> +static void usbio_i2c_uninit(struct i2c_adapter *adap, struct i2c_msg *msg) >> +{ >> + struct usbio_i2c *i2c = i2c_get_adapdata(adap); >> + struct usbio_i2c_uninit ubuf; >> + >> + ubuf.busid = i2c->adev->id; >> + ubuf.config = msg->addr; > > You can initialise this in declaration. With the changes to use __le16 and _le32 for multi-byte words which Greg rightfully asked for this now looks like this: ubuf.busid = i2c->adev->id; ubuf.config = cpu_to_le16(msg->addr); having a function call in a struct initializer looks weird / wrong, so I'm going to keep these one as well as the other places as-is. Also in the usbio_i2c_read() / write() cases the struct may get re-initialized several times if needing to split the i2c-transfer into multiple bulk transfers. Using struct initilization for the first one in that case feels rather inconsistent. ... >> +static int usbio_i2c_probe(struct auxiliary_device *adev, >> + const struct auxiliary_device_id *adev_id) >> +{ >> + struct usbio_i2c_bus_desc *i2c_desc; >> + struct device *dev = &adev->dev; >> + u8 dummy_read_buf; >> + struct i2c_msg dummy_read = { >> + .addr = 0x08, >> + .flags = I2C_M_RD, >> + .len = 1, >> + .buf = &dummy_read_buf, >> + }; >> + struct usbio_i2c *i2c; >> + u32 max_speed; >> + int ret; >> + >> + i2c_desc = dev_get_platdata(dev); >> + if (!i2c_desc) >> + return -EINVAL; >> + >> + /* Some USBIO chips have caps set to 0, but all chips can do 400KHz */ >> + if (!i2c_desc->caps) >> + max_speed = I2C_MAX_FAST_MODE_FREQ; >> + else >> + max_speed = usbio_i2c_speeds[i2c_desc->caps & USBIO_I2C_BUS_MODE_CAP_MASK]; >> + >> + i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL); >> + if (!i2c) >> + return -ENOMEM; > > Same comment on devm memory allocation than on the GPIO driver: I think you > need to use the release callback of struct device here. We unregister the adapter in remove() after that no callbacks into the driver can be made, so using devm() managed memory here is fine. Regards, Hans