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;
MIME-Version: 1.0
References: <20220823145603.2606475-1-tharunkumar.pasumarthi@microchip.com>
 <YwTsnCdxM5I6BoJu@smile.fi.intel.com> <80debd4bbf819cbfa16681172919436df476558a.camel@microchip.com>
In-Reply-To: <80debd4bbf819cbfa16681172919436df476558a.camel@microchip.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 24 Aug 2022 21:31:37 +0300
Message-ID: <CAHp75VevAifCp_Bz5HN1MmGm7UEJWFR017kngzARfBh+Z8aAAA@mail.gmail.com>
Subject: Re: [PATCH RFC i2c-master] i2c: microchip: pci1xxxx: Add driver for
 I2C host controller in multifunction endpoint of pci1xxxx switch
To:     Tharunkumar.Pasumarthi@microchip.com
Cc:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>,
        Wolfram Sang <wsa@kernel.org>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        Sven Peter <sven@svenpeter.dev>, Rob Herring <robh@kernel.org>,
        Sam Protsenko <semen.protsenko@linaro.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Jarkko Nikula <jarkko.nikula@linux.intel.com>,
        Olof Johansson <olof@lixom.net>,
        linux-i2c <linux-i2c@vger.kernel.org>,
        Jan Dabros <jsd@semihalf.com>, Arnd Bergmann <arnd@arndb.de>,
        =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <rafal@milecki.pl>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Aug 24, 2022 at 6:04 PM <Tharunkumar.Pasumarthi@microchip.com> wrot=
e:
> On Tue, 2022-08-23 at 18:05 +0300, Andy Shevchenko wrote:
> > On Tue, Aug 23, 2022 at 08:26:03PM +0530, Tharun Kumar P wrote:

...

> > > + * Author: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> > > + *         Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.co=
m>
> >
> > Single or many?
>
> There are 2 authors for this file

Do you see the issue now?

...

> > > +#define SMB_CORE_CTRL_ESO    0x40
> > > +#define SMB_CORE_CTRL_FW_ACK 0x10
> >
> > Are they bits or numbers?
>
> These are bits

Use BIT() then. Ditto for the rest of the bits.

...

> > > +#define SMB_IDLE_SCALING_100KHZ              0x03E803C9
> > > +#define SMB_IDLE_SCALING_400KHZ              0x01F4009D
> > > +#define SMB_IDLE_SCALING_1000KHZ     0x01F4009D
> >
> > Shouldn't these magics be decimals?

This Q seems unanswered.

> > Ditto for the rest similar stuff.
>
> There is no direct correlation between the hex value and time. Configurin=
g
> registers with these values in driver will set the time in device.

...

> > > +#define I2C_DIR_WRITE                0
> > > +#define I2C_DIR_READ         1

https://elixir.bootlin.com/linux/v6.0-rc2/source/include/uapi/linux/i2c.h#L=
24

> > Namespace collision. Doesn't I=C2=B2C core provide these?
>
> I am unable to find any existing MACROs for WRITE and READ in I2C core. K=
indly
> let me know the MACROs

...

> > > +#define PCI1XXXX_I2C_TIMEOUT 1000
> >
> > Units? Same to the rest similar cases.
>
> Unit is milliseconds

So you know what to do, right?

...

> > > +#define SMBUS_PERI_LOCK              BIT(3)
> >
> > BIT() out of a sudden. See above.
>
> Will use hex value for this like in other places to maintain uniformity

See above.

...

> > > +     pci1xxxx_i2c_shutdown(i2c);
> >
> > Do you really need this in ->remove()? I would expect something in
> > the ->suspend() / ->shutdown().
>
> pci1xxxx_i2c_shutdown API will reset the registers that are set as part o=
f
> pci1xxxx_i2c_init. So, this API is present in ->remove() and not in ->sus=
pend()
> callback

I understand that, but it doesn't really answer my question.

--=20
With Best Regards,
Andy Shevchenko