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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

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?

> +	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?

> +{
> +	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;
> +}

-- 
With Best Regards,
Andy Shevchenko