Re: [PATCH edk2-platforms 05/39] Silicon/NXP: Add support for I2c driver

[Leif Lindholm <leif.lindholm@linaro.org>]

On Mon, Apr 23, 2018 at 08:21:22AM +0000, Meenakshi Aggarwal wrote:
> > > +/**
> > > +  Function to read data using i2c bus
> > > +
> > > +  @param   I2cBus          I2c Controller number
> > > +  @param   Chip            Address of slave device from where data to be read
> > > +  @param   Offset          Offset of slave memory
> > > +  @param   Alen            Address length of slave
> > > +  @param   Buffer          A pointer to the destination buffer for the data
> > > +  @param   Len             Length of data to be read
> > > +
> > > +  @retval  EFI_NOT_READY   Arbitration lost
> > > +  @retval  EFI_TIMEOUT     Failed to initialize data transfer in predefined
> > time
> > > +  @retval  EFI_NOT_FOUND   ACK was not recieved
> > > +  @retval  EFI_SUCCESS     Read was successful
> > > +
> > > +**/
> > > +STATIC
> > > +EFI_STATUS
> > > +I2cDataRead (
> > > +  IN  UINT32               I2cBus,
> > > +  IN  UINT8                Chip,
> > > +  IN  UINT32               Offset,
> > > +  IN  UINT32               Alen,
> > > +  IN  UINT8                *Buffer,
> > > +  IN  UINT32               Len
> > > +  )
> > > +{
> > > +  EFI_STATUS               Status;
> > > +  UINT32                   Temp;
> > > +  INT32                    I;
> > > +  I2C_REGS                 *I2cRegs;
> > > +
> > > +  I2cRegs = (I2C_REGS *)(FixedPcdGet64 (PcdI2c0BaseAddr +
> > > +                         (I2cBus * FixedPcdGet32 (PcdI2cSize))));
> > 
> > Please get rid of this hardcoded base address and use NonDiscoverable
> > Have a look at Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/ and
> > Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/
> > for example.
> > 
> I have checked SynQuacer code and i dont see its adding much advantage.

What it gives is the ability to cover more than one controller by this
driver, regardless of whether you need it for this particular platform
port or not.

> There also base address is hard coded SYNQUACER_I2C1_BASE in
> PlatformDxe driver.

Yes, the PlatformDxe driver statically instantiates dynamic drivers
for non-discoverable buses. But there is no longer any hard-coded
addresses in the device driver itself.

/
    Leif