Re: [PATCH v5 5/8] i2c: amd-asf: Add i2c_algorithm operations to support AMD ASF with SMBus

[Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>]

On 9/14/2024 00:38, Andy Shevchenko wrote:
> On Fri, Sep 13, 2024 at 05:41:07PM +0530, Shyam Sundar S K wrote:
>> Implement the i2c_algorithm operations to enable support for AMD ASF
>> (Alert Standard Format) with SMBus. This enhancement includes:
>>
>> - Adding functionality to identify and select the supported ASF functions.
>> - Implementing mechanisms for registering and deregistering I2C slave
>>   devices.
>> - Providing support for data transfer operations over ASF.
>>
>> Additionally, include a 'select' Kconfig entry as the current patch
>> utilizes reg_slave and unreg_slave callbacks, which are controlled by
>> IS_ENABLED(CONFIG_I2C_SLAVE).
> 
> ...
> 
>> +/* ASF address offsets */
>> +#define ASFLISADDR	(9 + piix4_smba)
>> +#define ASFSTA		(0xA + piix4_smba)
>> +#define ASFSLVSTA	(0xD + piix4_smba)
>> +#define ASFDATABNKSEL	(0x13 + piix4_smba)
>> +#define ASFSLVEN	(0x15 + piix4_smba)
> 
> 0x09
> 0x0A
> 0x0D
> 
> ...
> 
>> +static void amd_asf_update_bits(unsigned short piix4_smba, u8 bit,
>> +				unsigned long offset, bool set)
>> +{
>> +	unsigned long reg;
>> +
>> +	reg = inb_p(offset);
>> +	if (set)
>> +		set_bit(bit, &reg);
>> +	else
>> +		clear_bit(bit, &reg);
> 
> + bitops.h
> 
> The above is home made assign_bit().
> Moreover, why atomic version? Wouldn't __assign_bit() suffice?
> 

thanks! __assign_bit() would suffice.

>> +	outb_p(reg, offset);
>> +}
> 
> ...
> 
>> +static void amd_asf_update_bytes(struct amd_asf_dev *dev, u8 bit, bool set)
> 
> I didn't get the naming, the above using IO port with _bits, and this is MMIO
> with _bytes. Are you sure the naming schema is correct?
> 

Thinking to merge both the functions into one, something like this:

enum io_type {
    IO_PORT,
    MMIO
};

static void amd_asf_update_target(struct amd_asf_dev *dev, enum
io_type type, u8 bit, bool set)
{

...

}

>> +{
>> +	unsigned long reg;
>> +
>> +	reg = ioread32(dev->mmio_cfg.addr);
>> +	if (set)
>> +		set_bit(bit, &reg);
>> +	else
>> +		clear_bit(bit, &reg);
>> +	iowrite32(reg, dev->mmio_cfg.addr);
> 
> Ditto (bitops and related things).
> 
>> +}
> 
> ...
> 
>> +static int amd_asf_reg_target(struct i2c_client *target)
>> +{
>> +	struct amd_asf_dev *dev = i2c_get_adapdata(target->adapter);
>> +	unsigned short piix4_smba = dev->port_addr->start;
>> +	int ret;
>> +	u8 reg;
>> +
>> +	if (dev->target)
>> +		return -EBUSY;
>> +
>> +	ret = piix4_sb800_region_request(&target->dev, &dev->mmio_cfg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	reg = (target->addr << 1) | BIT(0);
> 
> Is BIT(0) == I2C_M_RD in this case? If so, use the latter defined constant.
> 
>> +	outb_p(reg, ASFLISADDR);
>> +
>> +	amd_asf_setup_target(dev);
>> +	dev->target = target;
>> +	amd_asf_update_bits(piix4_smba, ASF_DATA_EN, ASFDATABNKSEL, false);
>> +	piix4_sb800_region_release(&target->dev, &dev->mmio_cfg);
>> +
>> +	return 0;
>> +}
>