Messages in this thread | | | From | Andy Shevchenko <> | Date | Wed, 24 Aug 2022 21:31:37 +0300 | Subject | Re: [PATCH RFC i2c-master] i2c: microchip: pci1xxxx: Add driver for I2C host controller in multifunction endpoint of pci1xxxx switch |
| |
On Wed, Aug 24, 2022 at 6:04 PM <Tharunkumar.Pasumarthi@microchip.com> wrote: > 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.com> > > > > 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. Configuring > 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#L24
> > Namespace collision. Doesn't I²C core provide these? > > I am unable to find any existing MACROs for WRITE and READ in I2C core. Kindly > 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 of > pci1xxxx_i2c_init. So, this API is present in ->remove() and not in ->suspend() > callback
I understand that, but it doesn't really answer my question.
-- With Best Regards, Andy Shevchenko
| |