Re: [PATCH v5 6/8] i2c: amd-asf: Add routine to handle the ASF slave process

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

On 9/14/2024 00:47, Andy Shevchenko wrote:
> On Fri, Sep 13, 2024 at 05:41:08PM +0530, Shyam Sundar S K wrote:
>> Add support for handling ASF slave process events as described in the AMD
>> ASF databook. This involves implementing the correct programming sequence
>> to manage each ASF packet appropriately.
> 
> ...
> 
>>  /* ASF address offsets */
>> +#define ASFINDEX	(7 + piix4_smba)
> 
> 0x07
> 
> ...
> 
>> +#define ASF_ERROR_STATUS		0xE
> 
> So, according to the usage this seems to be a mask, then perhaps GENMASK(3, 1) ?
> 

GENMASK() works here.

> ...
> 
>> +static void amd_asf_process_target(struct work_struct *work)
>> +{
>> +	struct amd_asf_dev *dev = container_of(work, struct amd_asf_dev, work_buf.work);
>> +	unsigned short piix4_smba = dev->port_addr->start;
>> +	u8 data[ASF_BLOCK_MAX_BYTES];
> 
>> +	u8 len, idx, val = 0;
> 
> Hmm... Does val = 0 assignment is due to false positive (or missing error check)?
> 

I can remove the explicit assignment to zero.

>> +	u8 bank, reg, cmd;
>> +
>> +	/* Read target status register */
>> +	reg = inb_p(ASFSLVSTA);
>> +
>> +	/* Check if no error bits are set in target status register */
>> +	if (reg & ASF_ERROR_STATUS) {
>> +		/* Set bank as full */
>> +		cmd = 0;
>> +		reg = reg | GENMASK(3, 2);
>> +		outb_p(reg, ASFDATABNKSEL);
>> +	} else {
>> +		/* Read data bank */
>> +		reg = inb_p(ASFDATABNKSEL);
>> +		bank = (reg & BIT(3)) ? 1 : 0;
>> +
>> +		/* Set read data bank */
>> +		if (bank) {
>> +			reg = reg | BIT(4);
>> +			reg = reg & ~BIT(3);
>> +		} else {
>> +			reg = reg & ~BIT(4);
>> +			reg = reg & ~BIT(2);
>> +		}
>> +
>> +		/* Read command register */
>> +		outb_p(reg, ASFDATABNKSEL);
>> +		cmd = inb_p(ASFINDEX);
>> +		len = inb_p(ASFDATARWPTR);
>> +		for (idx = 0; idx < len; idx++)
>> +			data[idx] = inb_p(ASFINDEX);
>> +
>> +		/* Clear data bank status */
>> +		if (bank) {
>> +			reg = reg | BIT(3);
>> +			outb_p(reg, ASFDATABNKSEL);
>> +		} else {
>> +			reg = reg | BIT(2);
>> +			outb_p(reg, ASFDATABNKSEL);
>> +		}
>> +	}
>> +
>> +	outb_p(0, ASFSETDATARDPTR);
>> +	if (cmd & BIT(0))
>> +		return;
>> +
>> +	i2c_slave_event(dev->target, I2C_SLAVE_WRITE_REQUESTED, &val);
> 
> Can this fail / return an error code? (I haven't checked)

i2c_slave_event() returns an error code, but here it is done with the
workqueue callback context. Hence I skipped the error checking part.

> 
>> +	for (idx = 0; idx < len; idx++) {
>> +		val = data[idx];
>> +		i2c_slave_event(dev->target, I2C_SLAVE_WRITE_RECEIVED, &val);
>> +	}
>> +	i2c_slave_event(dev->target, I2C_SLAVE_STOP, &val);
>> +}
> 
> ...
> 
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (!irq)
> 
> Incorrect check.
> 
>> +		return dev_err_probe(&pdev->dev, -EINVAL, "missing IRQ resources\n");
> 
> Shadower real error code.
> 
> ...
> 
>> +static void amd_asf_remove(struct platform_device *pdev)
>> +{
>> +	struct amd_asf_dev *dev = platform_get_drvdata(pdev);
>> +
>> +	cancel_delayed_work_sync(&dev->work_buf);
>> +}
> 
> Wouldn't devm-helpers.h APIs help with avoiding ->remove() creation?
>