On Wed, Sep 04, 2024 at 04:27:29PM +0530, Shyam Sundar S K wrote:
> The AMD ASF controller is presented to the operating system as an ACPI
> device. The piix4 driver can obtain the ASF handle through ACPI to
> retrieve information about the ASF controller's attributes, such as the
> ASF address space and interrupt number, and to handle ASF interrupts.
>
> Currently, the piix4 driver assumes that a specific port address is
> designated for AUX operations. However, with the introduction of ASF, the
> same port address may also be used by the ASF controller. Therefore, a
> check needs to be added to ensure that if ASF is advertised and enabled in
> ACPI, the AUX port is not set up.
> Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Use --cc parameter when sending, instead of this, or alternatively move this
line...
> Co-developed-by: Sanket Goswami <Sanket.Goswami@xxxxxxx>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@xxxxxxx>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
> ---
...to be somewhere here. It will the same effect for you and me
(in case you use `git send-email`).
> drivers/i2c/busses/i2c-piix4.c | 166 ++++++++++++++++++++++++++++++++-
...
> +struct sb800_asf_data {
> + unsigned short addr;
> + int irq;
> +};
> struct i2c_piix4_adapdata {
> struct sb800_mmio_cfg mmio_cfg;
> u8 algo_select;
> struct i2c_client *slave;
> + bool is_asf;
> + struct delayed_work work_buf;
> };
Side note, always run `pahole` when adding new data structures or modifying
existing ones. It might save memory on the running systems.
...
> +static void sb800_asf_process_slave(struct work_struct *work)
> +{
> + struct i2c_piix4_adapdata *adapdata = container_of(work, struct i2c_piix4_adapdata,
> + work_buf.work);
Better split is
struct i2c_piix4_adapdata *adapdata =
container_of(work, struct i2c_piix4_adapdata, work_buf.work);
> + unsigned short piix4_smba = adapdata->smba;
> + u8 data[SB800_ASF_BLOCK_MAX_BYTES];
> + u8 bank, reg, cmd = 0;
> + u8 len, val = 0;
> + int i;
> +
> + /* Read slave status register */
> + reg = inb_p(ASFSLVSTA);
> +
> + /* Check if no error bits are set in slave status register */
> + if (!(reg & SB800_ASF_ERROR_STATUS)) {
Why not use positive conditional? (This will require to swap if/else branches)
> + /* Read data bank */
> + reg = inb_p(ASFDATABNKSEL);
> + bank = (reg & BIT(3)) >> 3;
> +
> + /* Set read data bank */
> + if (bank) {
> + reg = reg | BIT(4);
> + reg = reg & (~BIT(3));
> + } else {
> + reg = reg & (~BIT(4));
> + reg = reg & (~BIT(2));
In all these three cases the outer parentheses are redundant.
> + }
> +
> + /* Read command register */
> + outb_p(reg, ASFDATABNKSEL);
> + cmd = inb_p(ASFINDEX);
> + len = inb_p(ASFDATARWPTR);
> + for (i = 0; i < len; i++)
> + data[i] = 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);
> + }
> + } else {
> + /* Set bank as full */
> + reg = reg | (BIT(3) | BIT(2));
GENMASK() ?
> + outb_p(reg, ASFDATABNKSEL);
> + }
> +
> + outb_p(0, ASFSETDATARDPTR);
> + if (!(cmd & BIT(0))) {
if (cmd & BIT(0))
return;
?
> + i2c_slave_event(adapdata->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
> + for (i = 0; i < len; i++) {
> + val = data[i];
> + i2c_slave_event(adapdata->slave, I2C_SLAVE_WRITE_RECEIVED, &val);
> + }
> + i2c_slave_event(adapdata->slave, I2C_SLAVE_STOP, &val);
> + }
> +}
...
> +static irqreturn_t sb800_asf_irq_handler(int irq, void *ptr)
> +{
> + struct i2c_piix4_adapdata *adapdata = (struct i2c_piix4_adapdata *)ptr;
Unneeded explicit casting.
> + unsigned short piix4_smba = adapdata->smba;
> + u8 slave_int = inb_p(ASFSTA);
> +
> + if ((slave_int & BIT(6))) {
Too many parentheses
> + /* Slave Interrupt */
> + outb_p(slave_int | BIT(6), ASFSTA);
> + schedule_delayed_work(&adapdata->work_buf, HZ);
> + } else {
> + /* Master Interrupt */
> + sb800_asf_update_bits(piix4_smba, SB800_ASF_SLV_INTR, SMBHSTSTS, true);
> + }
> +
> + return IRQ_HANDLED;
> +}
...
> +static int sb800_asf_acpi_resource_cb(struct acpi_resource *resource, void *context)
> +{
> + struct sb800_asf_data *data = context;
> +
> + switch (resource->type) {
> + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> + data->irq = resource->data.extended_irq.interrupts[0];
> + break;
> + case ACPI_RESOURCE_TYPE_IO:
> + data->addr = resource->data.io.minimum;
> + break;
> + }
> +
> + return 0;
> +}
Why do you still need this callback? What for?
...
> +static int sb800_asf_add_adap(struct pci_dev *dev)
> +{
> + struct i2c_piix4_adapdata *adapdata;
> + struct sb800_asf_data data;
> + struct acpi_device *adev;
> + LIST_HEAD(res_list);
> + acpi_status status;
> + acpi_handle handle;
> + int ret;
> +
> + status = acpi_get_handle(NULL, SB800_ASF_ACPI_PATH, &handle);
> + if (ACPI_FAILURE(status))
> + return -ENODEV;
> +
> + adev = acpi_fetch_acpi_dev(handle);
> + if (!adev)
> + return -ENODEV;
> +
> + ret = acpi_dev_get_resources(adev, &res_list, sb800_asf_acpi_resource_cb, &data);
..., NULL, NULL):
> + if (ret < 0) {
> + dev_err(&dev->dev, "Error getting ASF ACPI resource: %d\n", ret);
> + return ret;
> + }
Here iterate over the resources and take the ones you need by their respective
types.
> + acpi_dev_free_resource_list(&res_list);
> + ret = piix4_add_adapter(dev, data.addr, SMBUS_ASF, piix4_adapter_count, false, 0,
> + piix4_main_port_names_sb800[piix4_adapter_count],
> + &piix4_main_adapters[piix4_adapter_count]);
> + if (ret) {
> + dev_err(&dev->dev, "Failed to add ASF adapter: %d\n", ret);
> + return -ENODEV;
> + }
> +
> + adapdata = i2c_get_adapdata(piix4_main_adapters[piix4_adapter_count]);
> + ret = devm_request_irq(&dev->dev, data.irq, sb800_asf_irq_handler, IRQF_SHARED,
> + "sb800_smbus_asf", adapdata);
> + if (ret) {
> + dev_err(&dev->dev, "Unable to request irq: %d for use\n", data.irq);
> + return ret;
> + }
> +
> + INIT_DELAYED_WORK(&adapdata->work_buf, sb800_asf_process_slave);
> + adapdata->is_asf = true;
> + /* Increment the adapter count by 1 as ASF is added to the list */
> + piix4_adapter_count += 1;
++ also suffice.
> + return 1;
> +}
--
With Best Regards,
Andy Shevchenko
