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) ?
...
> +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)?
> + 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)
> + 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?
--
With Best Regards,
Andy Shevchenko
