Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 17 Sep 2022 23:01:48 +0200
From:   Wolfram Sang <wsa@kernel.org>
To:     Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
Cc:     linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
        andriy.shevchenko@linux.intel.com, krzk@kernel.org,
        jarkko.nikula@linux.intel.com, robh@kernel.org,
        semen.protsenko@linaro.org, sven@svenpeter.dev, jsd@semihalf.com,
        rafal@milecki.pl, olof@lixom.net, arnd@arndb.de,
        UNGLinuxDriver@microchip.com
Subject: Re: [PATCH v4 i2c-master] i2c: microchip: pci1xxxx: Add driver for
 I2C host controller in multifunction endpoint of pci1xxxx switch
Message-ID: <YyY1vESf5JalT6GR@shikoro>
Mail-Followup-To: Wolfram Sang <wsa@kernel.org>,
        Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>,
        linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
        andriy.shevchenko@linux.intel.com, krzk@kernel.org,
        jarkko.nikula@linux.intel.com, robh@kernel.org,
        semen.protsenko@linaro.org, sven@svenpeter.dev, jsd@semihalf.com,
        rafal@milecki.pl, olof@lixom.net, arnd@arndb.de,
        UNGLinuxDriver@microchip.com
References: <20220907161143.897289-1-tharunkumar.pasumarthi@microchip.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="eFVKGzIn+ZlvcRG0"
Content-Disposition: inline
In-Reply-To: <20220907161143.897289-1-tharunkumar.pasumarthi@microchip.com>
Precedence: bulk


--eFVKGzIn+ZlvcRG0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hi,

On Wed, Sep 07, 2022 at 09:41:43PM +0530, Tharun Kumar P wrote:
> Microchip pci1xxxx is an unmanaged PCIe3.1a Switch for Consumer,
> Industrial and Automotive applications. This switch has multiple
> downstream ports. In one of the Switch's Downstream port, there
> is a multifunction endpoint for peripherals which includes an I2C
> host controller. The I2C function in the endpoint operates at 100KHz,
> 400KHz and 1 MHz and has buffer depth of 128 bytes.
> This patch provides the I2C controller driver for the I2C function
> of the switch.
>=20
> Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>

Thank you for the driver and thanks to Andy for reviewing it this far.
He is right, some nits are there which can be fixed later. But a few
things I need to comment on:

> +static u32 pci1xxxx_i2c_get_funcs(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_PROTOCOL_MANGLING |
> +		I2C_FUNC_SMBUS_BLOCK_PROC_CALL |
> +		I2C_FUNC_SMBUS_READ_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE |
> +		I2C_FUNC_SMBUS_READ_BYTE_DATA |
> +		I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> +		I2C_FUNC_SMBUS_READ_WORD_DATA |
> +		I2C_FUNC_SMBUS_WRITE_WORD_DATA |
> +		I2C_FUNC_SMBUS_PROC_CALL | I2C_FUNC_SMBUS_READ_BLOCK_DATA |
> +		I2C_FUNC_SMBUS_WRITE_BLOCK_DATA;
> +}

If you can't do I2C_FUNC_SMBUS_QUICK, then you need an i2c_adapter_quirk
struct saying that your HW cannot do 0 byte length transfers.

Also, a lot can be simplified here, e.g. I2C_FUNC_SMBUS_BYTE covers both
cases, read and write.

=2E..

> +	/* Register the isr. We are not using any isr flags here. */

Why not use then simply 0 instead of a define and a comment?

> +	ret =3D devm_request_irq(dev, pci_irq_vector(pdev, 0), pci1xxxx_i2c_isr,
> +			       PCI1XXXX_IRQ_FLAGS, pci_name(pdev), i2c);
> +	if (ret)
> +		return ret;
> +
> +	i2c->adap =3D pci1xxxx_i2c_ops;
> +	i2c->adap.class =3D I2C_CLASS_SPD;

I need to make sure: do you intentionally want autoprobing for SPD? I
ask because it could be needed but it costs boottime when not needed.
And changing the class once exposed is troublesome.

Happy hacking,

   Wolfram


--eFVKGzIn+ZlvcRG0
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEOZGx6rniZ1Gk92RdFA3kzBSgKbYFAmMmNbgACgkQFA3kzBSg
KbbCTRAAmBaQg9qrdV42WcrrillciI53u8lW6a8RKn8zH0lhN0GToeAy+VcQ9pr6
8cXxYtqDt2FSsrgWdabCm7hMTOQxsKbRV0xmFBpeOuBKgNnQvHMnr4i424eDbdw4
P/cf5a3SRTPvT1wA85ihPFbszfNtqsElgkPETnvLGP48qw0xgB+TrBqji8TMtQ5L
eFD4AolHXEckEajcJapbHHaL4lCPejTEEMHTFWzy7welGuARBD9fdIKvsQMM/KF9
2Q4ohinZjbbfdWi8JOirFfOtklfb2ywmtUUFdLQd4MCsAyKv4cyFkdbq4Q+UmMBe
N/VuOZyOrqdCFRmedRMEqCdvCWelps432NbywplvdewTOpm9rVgcHCAmKAyckapC
/QMMTNtEvMrmYJL6tuHZJVCG1peDwr7kfAOgD1rl7WN/csqePl2aiTDdmIn8B5xc
cu+ogGlECoiV2PF1c9btduW6St37xRFgokZ5s7IS6CKjbFtYsFfBvq3JrJKTF8Be
zW+7bE7t271ly2eGlH3mwvPcrfdc6Z9tKvaKlH41luE90FhUGEGgl3Mp1QjhR6ez
i1Kl3FdHSA29G+N8HWg1DFomp2+d1kGNKosoRyibbTNzcjpJwNTdkyb3HqTB+tl+
U6kUeXuqS3nO1kqFWJqoxCgawsIsJNYsDYP0O4Y3nnB5V4H29Eg=
=e4fx
-----END PGP SIGNATURE-----

--eFVKGzIn+ZlvcRG0--