Message-ID: <CAHp75Vcp5ERvTMDob3gUPqi+WPUz_BKoytO5faSbtNeX3LWy_Q@mail.gmail.com>
Date:   Fri, 4 Sep 2020 10:40:31 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     AceLan Kao <acelan.kao@...onical.com>
Cc:     Alexandru Ardelean <alexandru.ardelean@...log.com>,
        William Sung <william.sung@...antech.com.tw>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Hartmut Knaack <knaack.h@....de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Campion Kang <Campion.Kang@...antech.com.tw>
Subject: Re: [PATCH] iio: dac: ad5593r: Dynamically set AD5593R channel modes

On Fri, Sep 4, 2020 at 5:25 AM AceLan Kao <acelan.kao@...onical.com> wrote:
> Here is the updated ads5593r.asl, but I still got some troubles when
> using configfs.
> The modification I made
> 1. add DefinitionBlock() and External()
> 2. align _SB.I2C1
> 3. set _CRS serialized
>
> The issues I got are,
> 1. The ADS5593 is defined in DSDT table, but I can't compile the asl
> code when define it as DSDT table

Is it part of DSDT? I think you are trying to make it as separate
table. According to ACPI specification you may have only one DSDT. It
means that additional tables should have SSDT signature. This also
explained in the documentation [7] regarding to ACPI SSDT overlays.

[7]: https://www.kernel.org/doc/html/latest/admin-guide/acpi/ssdt-overlays.html


> $ iasl ads5593r.asl
>
> Intel ACPI Component Architecture
> ASL+ Optimizing Compiler/Disassembler version 20190509
> Copyright (c) 2000 - 2019 Intel Corporation
>
> ads5593r.asl      5:     Scope (_SB.I2C1)
> Error    6148 -                       ^ Illegal open scope on external
> object from within DSDT

Again, look at the examples in [4]. You need to attach your device to
the controller and refer to it from I2cSerialBus() resource.


> ASL Input:     ads5593r.asl -    7917 bytes     30 keywords    257 source lines
>
> Compilation failed. 1 Errors, 0 Warnings, 0 Remarks
> No AML files were generated due to compiler error(s)
>
> 2. I got below errors in dmesg if I set it as SSDT
> [  410.220993] ACPI: Host-directed Dynamic ACPI Table Load:
> [  410.221013] ACPI: SSDT 0xFFFF89A4ADDE7C00 00035D (v01
> ADS5593R 00000001 INTL 20190509)
> [  410.221106] ACPI BIOS Error (bug): Failure creating named object
> [\_SB.I2C1.I2CG], AE_ALREADY_EXISTS (20190816/dswload2-326)
> [  410.221324] ACPI Error: AE_ALREADY_EXISTS, During name
> lookup/catalog (20190816/psobject-220)
> [  410.221468] ACPI: Skipping parse of AML opcode: Device (0x5B82)
>
> Do you have any suggestions?

Yes, you need to find proper device node in the DSDT which represents
the I²C controller. The DAC device should be subnode of it.

> We may have a chance to convince BIOS to move ADS5593 to the SSDT
> table, do you think it's a good idea?

I'm not sure I understand how BIOS is involved here. If the ADS5593 is
a part of platform and you can change DSDT in the BIOS, just make it
there. If it's an attachable component (like for your development
cycle) SSDT is best approach. Note that amount of SSDT in the system
is limited.

> BTW, the driver set the channels mode while probing, I'm not sure if
> configfs will make the driver probe again when new table is loaded
> If we can't use configfs, is there any other way we could try?

All possible ways are the following (depending on your needs):
 - BIOS provides DSDT with the component reference included
 - Bootloader provides, rewrites or updates tables
 - OS via: a) initramfs, b) ConfigFS
 - EFI keeps it in variable which OS or BIOS sets

> > > > I spent some time studying/reading what you wrote, but I still don't
> > > > understand how to leverage meta-acpi.
> > >
> > > meta-acpi is a Yocto layer to support provided ACPI tables for the
> > > build. My point here is to have it as a collection of ASL examples.
> > > It's what you asked for below in this email.
> >
> > > Also we can collect your ASL example under board (presumably new) folder.
> >
> > Actually it seems Baytrail, so, minnowboard-max is good enough.
> >
> > ...
> >
> > > On the first glance I didn't see any issues with it, but on second
> > > look here is one. Look into this [5] example.
> > > If you noticed it uses the same path in Scope and in the reference in
> > > I2cSerialBus() while in your ASL they are different.
> >
> > Also there is an _ADR value wrong for the second channel (I'm not sure
> > if it affects anyhow the rest).
> >
> > > Do you have issues with loading it (as is and after above addressed)?
> > >
> > > [5]: https://github.com/westeri/meta-acpi/blob/master/recipes-bsp/acpi-tables/samples/edison/ft6236.asli
> >
> > Also a link [6] to our Buildroot repository which allows to create an
> > initramfs with ASL compiled. Maybe used as a reference how we created
> > initramfs and compile ASLs.
> >
> > [6]: https://github.com/andy-shev/buildroot/tree/intel/board/intel/common
> > ...
> >
> > > > > One more useful link is SO answers on the topic:
> > > > > https://stackoverflow.com/search?tab=newest&q=prp0001
> > > > >
> > > > > > [1]: https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html#device-tree-namespace-link-device-id
> > > > > > [2]: https://elixir.bootlin.com/linux/v5.9-rc3/source/Documentation/devicetree/bindings/iio/dac/ad5592r.txt
> > > > > > [3]: https://github.com/westeri/meta-acpi
> > > > > > [4]: https://github.com/westeri/meta-acpi/tree/master/recipes-bsp/acpi-tables/samples
> >
> > > > > > > 1. https://www.advantech.com/products/9a0cc561-8fc2-4e22-969c-9df90a3952b5/uno-420/mod_2d6a546b-39e3-4bc4-bbf4-ac89e6b7667c


-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists