On Wed, 2022-08-24 at 21:31 +0300, Andy Shevchenko wrote: > > On Wed, Aug 24, 2022 at 6:04 PM <Tharunkumar.Pasumarthi@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> > > > > + * Kumaravel Thiagarajan <kumaravel.thiagarajan@xxxxxxxxxxxxx> > > > > > > Single or many? > > > > There are 2 authors for this file > > Do you see the issue now? Yes, I will change 'Author' to 'Authors' > ... > > > > > +#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. Okay > ... > > > > > +#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. These magic numbers need not be decimals. Configuring registers with these values in driver will set the time in device. However, these values do not convey any meaning when represented in decimals. > > > 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 I2C_M_RD is used in driver. But the purpose of these MACROs is different. I2C_DIR_WRITE is used inside both pci1xxxx_i2c_write as well as in pci1xxxx_i2c_read (for sending slave address). Thus these MACROs are required > > > 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? Yes, I will change MACRO to PCI1XXXX_I2C_TIMEOUT_MS > ... > > > > > +#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. Okay. > ... > > > > > + 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. I will remove this API in ->remove() callback and add in ->suspend() callback Thanks, Tharun Kumar P
