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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 416B1CCF2D4 for ; Sun, 18 Oct 2026 23:35:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:MIME-Version:References:In-Reply-To:Message-ID:Subject:Cc:To: From:Date:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=DqFqrz+5RnxxxPjeHegXNpR9f1B3r0EVBiH+BJlB7yE=; b=q+VRUZgKp4saBKVVTWCWLIQ7L2 iQ4WPGJmoGrqg4qKaWlWx8lWRF4OIMe0B5KwzY+stytdDDP//GX9bfxaK3KSOcU2w+TBLJEcG++sW 0hIo4MMchGkph47FJYPZTjHk1ZXtK8pylIGK+V+27/5FC6bI8Ny3dxxCOY1eFAIr5uJWQV9cmRahb MWKathE1yN6dUgSfPIlvRxqNZ7JsQb4o29AqN5Joc6Klpayx4DTHAaEAMMMQHV71VNRn4zCWBZBQV 8NX/AE9NXaI1EinGVNOkRw4eRhG/4JVHMXAoJNyeilV4DB+5EYXUlW/evqRKgubvHfYer/PWahlTU IhIc6MTQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.99.1 #2 (Red Hat Linux)) id 1xIaPp-00000006J00-27qE; Sun, 18 Oct 2026 23:35:45 +0000 Received: from sea.source.kernel.org ([2600:3c0a:e001:78e:0:1991:8:25]) by bombadil.infradead.org with esmtps (Exim 4.99.1 #2 (Red Hat Linux)) id 1xIaPn-00000006Izq-3hLO for linux-arm-kernel@lists.infradead.org; Sun, 18 Oct 2026 23:35:44 +0000 Received: from smtp.kernel.org (quasi.space.kernel.org [100.103.45.18]) by sea.source.kernel.org (Postfix) with ESMTP id 3C193413E7; Sun, 18 Oct 2026 23:35:43 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 559671F000C4; Sun, 18 Oct 2026 23:35:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1792366543; bh=DqFqrz+5RnxxxPjeHegXNpR9f1B3r0EVBiH+BJlB7yE=; h=Date:From:To:Cc:Subject:In-Reply-To:References; b=Wez2HOfoFU4XTOe6YBt3+wOwOwc/WebQBLERf13nobmiQlB3vu07JOCiUo6VhiooW FckmMczvz15qF6Gd71fg0dmgcmmT8GHAnj3Hi9yqRZFF2qAX4WCMxqUjofBf2L4p0d 3rCAj4yM2BZ1YotRpc1T7dcKml+wUB8JZGKFU0bd4HQDIHOibmMfC0e/n+z/oZm2EG 5TWSUTn5ghO/a00LFqmSO5YDOtCxNTWB+3NU56E40q880emHEZ1J6Imc5cdR9mIsjS GUGRLRMj08XvAOdwigFR9+1oSrRdDoW1C26SYq2TpgdYqgUeXEht4Vz8yYeIdLp8E4 xcFVJDBqZw89A== Date: Mon, 19 Oct 2026 00:35:37 +0100 From: Jonathan Cameron To: Dileep Kumar Nagavarapu Cc: David Lechner , Nuno =?UTF-8?B?U8Oh?= , Andy Shevchenko , Michal Simek , Conall O'Griofa , , , , , Sai Krishna Potthuri Subject: Re: [PATCH v4 15/15] iio: adc: xilinx-xadc: Add I2C interface support Message-ID: <20261019003537.617423a8@jic23-hlaptop> In-Reply-To: <20261005094205.1045874-16-dileepkumar.nagavarapu@amd.com> References: <20261005094205.1045874-1-dileepkumar.nagavarapu@amd.com> <20261005094205.1045874-16-dileepkumar.nagavarapu@amd.com> X-Mailer: Claws Mail 4.4.0 (GTK 3.24.52; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, 5 Oct 2026 15:12:05 +0530 Dileep Kumar Nagavarapu wrote: > Add I2C interface support for Xilinx System Management Wizard IP along > with the existing AXI memory-mapped interface. This support enables > monitoring the voltage and temperature on UltraScale+ devices where the > System Management Wizard is connected via I2C. > > Key changes: > - Implement 32-bit DRP (Dynamic Reconfiguration Port) packet format > as per Xilinx PG185 specification. > - Add separate I2C probe with xadc_i2c_of_match_table to handle same > compatible string ("xlnx,system-management-wiz-1.3") on I2C bus. > - Add NULL checks for get_dclk_rate callback function in sampling rate > functions to support interfaces without clock control > - Create separate iio_info structure(xadc_i2c_info) without event > callbacks for I2C devices > - Add xadc_i2c_transaction() function to handle I2C read/write operations > - Add XADC_TYPE_US_I2C type to distinguish I2C interface from AXI > - Defer XADC hardware initialization until first device access, since the > I2C path reaches the PL SysMon indirectly through a platform controller > DRP bridge that may not yet be operational at probe() time. Initializing > on first read/write instead avoids probe failures caused by transient > unavailability of that downstream path. > > Co-developed-by: Sai Krishna Potthuri > Signed-off-by: Sai Krishna Potthuri > Signed-off-by: Dileep Kumar Nagavarapu Hi Dileep, Various comments inline - mostly stuff where we can make things a little more readable or extensible. Jonathan > diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c > index 2addb61cf5b1..80199c4ddf69 100644 > --- a/drivers/iio/adc/xilinx-xadc-core.c > +++ b/drivers/iio/adc/xilinx-xadc-core.c > @@ -227,7 +227,7 @@ static int xadc_power_adc_b(struct xadc *xadc, unsigned int seq_mode) > * non-existing ADC-B powers down the main ADC, so just return and don't > * do anything. > */ > - if (xadc->ops->type == XADC_TYPE_US) > + if (xadc->ops->type == XADC_TYPE_US || xadc->ops->type == XADC_TYPE_US_I2C) > return 0; Maybe add what ever this is similar to below. If it's in the ops structure we avoid having to add new types like this. > > /* Powerdown the ADC-B when it is not needed. */ > @@ -249,7 +249,7 @@ static int xadc_get_seq_mode(struct xadc *xadc, unsigned long scan_mode) > unsigned int aux_scan_mode = scan_mode >> 16; > > /* UltraScale has only one ADC and supports only continuous mode */ > - if (xadc->ops->type == XADC_TYPE_US) > + if (xadc->ops->type == XADC_TYPE_US || xadc->ops->type == XADC_TYPE_US_I2C) > return XADC_CONF1_SEQ_CONTINUOUS; Perhaps add this to the ops structure as well as a field called something like bool continuous_only; > > if (xadc->external_mux_mode == XADC_EXTERNAL_MUX_DUAL) > > struct iio_dev *xadc_device_setup(struct device *dev, int size, const struct xadc_ops **ops) > @@ -784,7 +797,11 @@ struct iio_dev *xadc_device_setup(struct device *dev, int size, const struct xad > return ERR_PTR(-ENOMEM); > > indio_dev->name = xadc_type_names[(*ops)->type]; > - indio_dev->info = &xadc_info; > + if ((*ops)->type == XADC_TYPE_US_I2C) Why not put the info in the ops structure? Then you can set it unconditionally here. Generally matching a 'type' ends up messier than just adding more stuff to the type specific structure. > + indio_dev->info = &xadc_i2c_info; > + else > + indio_dev->info = &xadc_info; > diff --git a/drivers/iio/adc/xilinx-xadc-i2c.c b/drivers/iio/adc/xilinx-xadc-i2c.c > new file mode 100644 > index 000000000000..cee2f7b3f9bd > --- /dev/null > +++ b/drivers/iio/adc/xilinx-xadc-i2c.c > + > +#include > + > +#include "xilinx-xadc.h" > + > +#define XADC_I2C_READ_DATA_SIZE 2 > +#define XADC_I2C_WRITE_DATA_SIZE 4 /* 32-bit DRP packet */ Hmm. Put this as a terminating entry in an enum with the DRP elements below. The it becomes clear how they are associated with each other. > +#define XADC_I2C_INSTR_READ BIT(2) > +#define XADC_I2C_INSTR_WRITE BIT(3) > + > +#define XADC_I2C_DRP_DATA0_MASK GENMASK(7, 0) > +#define XADC_I2C_DRP_DATA1_MASK GENMASK(15, 8) > +#define XADC_I2C_DRP_ADDR_MASK GENMASK(7, 0) All of these seem unnecessary. See below > + > +/* Byte offsets within the 32-bit DRP packet, per Xilinx PG185 */ > +#define XADC_I2C_DRP_DATA_LOW 0 > +#define XADC_I2C_DRP_DATA_HIGH 1 > +#define XADC_I2C_DRP_ADDR 2 > +#define XADC_I2C_DRP_INSTR 3 > + > +static int xadc_i2c_read_transaction(struct xadc *xadc, unsigned int reg, u16 *val) > +{ > + struct xadc_i2c *xadc_i2c = container_of(xadc, struct xadc_i2c, xadc); > + u8 write_buffer[XADC_I2C_WRITE_DATA_SIZE] = { 0 }; Might as well set the values you have below here rather than init to 0 and then override. > + struct i2c_client *client = xadc_i2c->client; > + u8 read_buffer[XADC_I2C_READ_DATA_SIZE]; > + int ret; > + > + write_buffer[XADC_I2C_DRP_ADDR] = FIELD_GET(XADC_I2C_DRP_ADDR_MASK, reg); As below. Not seeing much value in this FIELD_PREP as the reg should always fit I think? > + write_buffer[XADC_I2C_DRP_INSTR] = XADC_I2C_INSTR_READ; > + > + ret = i2c_master_send(client, write_buffer, XADC_I2C_WRITE_DATA_SIZE); > + if (ret < 0) > + return ret; > + > + ret = i2c_master_recv(client, read_buffer, XADC_I2C_READ_DATA_SIZE); > + if (ret < 0) > + return ret; > + > + *val = FIELD_PREP(XADC_I2C_DRP_DATA0_MASK, read_buffer[XADC_I2C_DRP_DATA_LOW]) | > + FIELD_PREP(XADC_I2C_DRP_DATA1_MASK, read_buffer[XADC_I2C_DRP_DATA_HIGH]); *val = get_unaligned_le16(&read_buffer[XADC_I2C_DRP_DATA_LOW]); > + > + return 0; > +} > + > +static int xadc_i2c_write_transaction(struct xadc *xadc, unsigned int reg, u16 val) > +{ > + struct xadc_i2c *xadc_i2c = container_of(xadc, struct xadc_i2c, xadc); > + struct i2c_client *client = xadc_i2c->client; > + u8 write_buffer[XADC_I2C_WRITE_DATA_SIZE]; > + int ret; > + > + /* low byte of the 16-bit DRP data value */ > + write_buffer[XADC_I2C_DRP_DATA_LOW] = FIELD_GET(XADC_I2C_DRP_DATA0_MASK, val); > + /* high byte of the 16-bit DRP data value */ > + write_buffer[XADC_I2C_DRP_DATA_HIGH] = FIELD_GET(XADC_I2C_DRP_DATA1_MASK, val); put_unaligned_le16(val, &write_buffer[XADC_I2C_DRP_DATA_LOW]); and no comments needed. > + write_buffer[XADC_I2C_DRP_ADDR] = FIELD_GET(XADC_I2C_DRP_ADDR_MASK, reg); Given this is just a clamp to size of something that I assume always fits, I'd drop the FIELD_PREP as providing no value. > + write_buffer[XADC_I2C_DRP_INSTR] = XADC_I2C_INSTR_WRITE; > + > + ret = i2c_master_send(client, write_buffer, XADC_I2C_WRITE_DATA_SIZE); I guess in this particular case we know what the i2c master is so can assume this never runs short? (in practice they never do but I think the interface allows for that case). > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int xadc_i2c_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + unsigned int conf0, bipolar_mask; > + const struct xadc_ops *ops; > + struct iio_dev *indio_dev; > + struct xadc_i2c *xadc_i2c; > + struct xadc *xadc; > + int ret; > + > + indio_dev = xadc_device_setup(dev, sizeof(*xadc_i2c), &ops); > + if (IS_ERR(indio_dev)) > + return PTR_ERR(indio_dev); > + > + xadc_i2c = iio_priv(indio_dev); > + xadc_i2c->client = client; > + xadc = &xadc_i2c->xadc; It might be worth a little helper for here... > + xadc->ops = ops; > + > + ret = devm_mutex_init(dev, &xadc->mutex); > + if (ret) > + return ret; > + > + spin_lock_init(&xadc->lock); to here to initialize the xadc structure as I don't think the individual drivers should really know about xadc->lock etc. > + > + ret = xadc_device_configure(indio_dev, 0, &conf0, &bipolar_mask); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to setup the device\n"); > + > + xadc_i2c->conf0 = conf0; > + xadc_i2c->bipolar_mask = bipolar_mask; > + xadc_i2c->hw_initialized = false; > + > + return devm_iio_device_register(dev, indio_dev); > +} > + > +static const struct of_device_id xadc_i2c_of_match_table[] = { > + { > + .compatible = "xlnx,system-management-wiz-1.3", > + .data = &xadc_system_mgmt_wiz_i2c_ops, > + }, > + {} Trivial but local IIO preference is { } I wanted to standardise these on something, and happened to pick that a few years back. > +}; > +MODULE_DEVICE_TABLE(of, xadc_i2c_of_match_table);