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