To use ad5593r more flexibly, we use the module parameter to setting the
channel modes dynamically whenever the module probe up. Users can pass
the channel modes to the module parameter for allocating the
functionality of channels as desired.
For example:
* Use in the kernel command line:
Users can add the module parameter in the kernel command line such as
"ad5593r.ch_mode_cmdline=88001122"
"88001122" means the channel mode setting for each channel. The most
left side indicates the mode of channel 7, and the most right side
indicates the mode of channel 0.
* Use when manually probe the module:
Similar to the kernel command line usage, users can enter
"modprobe ad5593r ch_mode_probe=88001122"
to start the ad5593r module with the desired channel mode setting.
Signed-off-by: William Sung <[email protected]>
---
drivers/iio/dac/ad5592r-base.c | 33 ++++++++++++++++++--
drivers/iio/dac/ad5592r-base.h | 4 +++
drivers/iio/dac/ad5593r.c | 55 ++++++++++++++++++++++++++++++++++
3 files changed, 89 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/dac/ad5592r-base.c b/drivers/iio/dac/ad5592r-base.c
index cc4875660a69..cd69a34fa21e 100644
--- a/drivers/iio/dac/ad5592r-base.c
+++ b/drivers/iio/dac/ad5592r-base.c
@@ -21,6 +21,10 @@
#include "ad5592r-base.h"
+/* Parameters for dynamic channel mode setting */
+static u8 update_channel_mode;
+static u8 new_channel_modes[AD559XR_CHANNEL_NR];
+
static int ad5592r_gpio_get(struct gpio_chip *chip, unsigned offset)
{
struct ad5592r_state *st = gpiochip_get_data(chip);
@@ -132,7 +136,7 @@ static int ad5592r_gpio_init(struct ad5592r_state *st)
st->gpiochip.label = dev_name(st->dev);
st->gpiochip.base = -1;
- st->gpiochip.ngpio = 8;
+ st->gpiochip.ngpio = AD559XR_CHANNEL_NR;
st->gpiochip.parent = st->dev;
st->gpiochip.can_sleep = true;
st->gpiochip.direction_input = ad5592r_gpio_direction_input;
@@ -287,6 +291,14 @@ static int ad5592r_set_channel_modes(struct ad5592r_state *st)
return ret;
}
+static void ad5592r_set_def_channel_modes(struct ad5592r_state *st)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(st->channel_modes); i++)
+ st->channel_modes[i] = new_channel_modes[i];
+}
+
static int ad5592r_reset_channel_modes(struct ad5592r_state *st)
{
int i;
@@ -532,6 +544,10 @@ static int ad5592r_alloc_channels(struct iio_dev *iio_dev)
st->channel_offstate[reg] = tmp;
}
+ /* Update default channel modes set by external module */
+ if (update_channel_mode == 1)
+ ad5592r_set_def_channel_modes(st);
+
channels = devm_kcalloc(st->dev,
1 + 2 * num_channels, sizeof(*channels),
GFP_KERNEL);
@@ -567,7 +583,7 @@ static int ad5592r_alloc_channels(struct iio_dev *iio_dev)
}
channels[curr_channel].type = IIO_TEMP;
- channels[curr_channel].channel = 8;
+ channels[curr_channel].channel = AD559XR_CHANNEL_NR;
channels[curr_channel].info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE) |
BIT(IIO_CHAN_INFO_OFFSET);
@@ -589,6 +605,17 @@ static void ad5592r_init_scales(struct ad5592r_state *st, int vref_mV)
div_s64_rem(tmp * 2, 1000000000LL, &st->scale_avail[1][1]);
}
+void ad5592r_update_default_channel_modes(u8 *new_modes)
+{
+ int idx = 0;
+
+ update_channel_mode = 1;
+ for (idx = 0; idx < AD559XR_CHANNEL_NR; idx++)
+ new_channel_modes[idx] = new_modes[idx];
+
+}
+EXPORT_SYMBOL_GPL(ad5592r_update_default_channel_modes);
+
int ad5592r_probe(struct device *dev, const char *name,
const struct ad5592r_rw_ops *ops)
{
@@ -603,7 +630,7 @@ int ad5592r_probe(struct device *dev, const char *name,
st = iio_priv(iio_dev);
st->dev = dev;
st->ops = ops;
- st->num_channels = 8;
+ st->num_channels = AD559XR_CHANNEL_NR;
dev_set_drvdata(dev, iio_dev);
st->reg = devm_regulator_get_optional(dev, "vref");
diff --git a/drivers/iio/dac/ad5592r-base.h b/drivers/iio/dac/ad5592r-base.h
index 23dac2f1ff8a..40ad6369e660 100644
--- a/drivers/iio/dac/ad5592r-base.h
+++ b/drivers/iio/dac/ad5592r-base.h
@@ -39,6 +39,9 @@ enum ad5592r_registers {
#define AD5592R_REG_CTRL_ADC_RANGE BIT(5)
#define AD5592R_REG_CTRL_DAC_RANGE BIT(4)
+/* Define quantity of channels of AD5592R/AD5593R */
+#define AD559XR_CHANNEL_NR 8
+
struct ad5592r_rw_ops {
int (*write_dac)(struct ad5592r_state *st, unsigned chan, u16 value);
int (*read_adc)(struct ad5592r_state *st, unsigned chan, u16 *value);
@@ -69,6 +72,7 @@ struct ad5592r_state {
__be16 spi_msg_nop;
};
+void ad5592r_update_default_channel_modes(u8 *new_modes);
int ad5592r_probe(struct device *dev, const char *name,
const struct ad5592r_rw_ops *ops);
int ad5592r_remove(struct device *dev);
diff --git a/drivers/iio/dac/ad5593r.c b/drivers/iio/dac/ad5593r.c
index 1fbe9c019c7f..dfc453a75ad6 100644
--- a/drivers/iio/dac/ad5593r.c
+++ b/drivers/iio/dac/ad5593r.c
@@ -21,6 +21,10 @@
#define AD5593R_MODE_GPIO_READBACK (6 << 4)
#define AD5593R_MODE_REG_READBACK (7 << 4)
+/* Parameters for dynamic channel mode setting */
+static char *ch_mode = "";
+module_param(ch_mode, charp, 0400);
+
static int ad5593r_write_dac(struct ad5592r_state *st, unsigned chan, u16 value)
{
struct i2c_client *i2c = to_i2c_client(st->dev);
@@ -92,9 +96,60 @@ static const struct ad5592r_rw_ops ad5593r_rw_ops = {
.gpio_read = ad5593r_gpio_read,
};
+static void ad5593r_check_new_channel_mode(void)
+{
+ char *new_mode[2] = {NULL, NULL}, tmp[2];
+ u8 new_ch_modes[AD559XR_CHANNEL_NR];
+ int idx = 0, cnt = 0, i;
+
+ if (strlen(ch_mode) == AD559XR_CHANNEL_NR)
+ new_mode[cnt++] = ch_mode;
+
+ for (i = 0; i < cnt; i++) {
+ /* Check if all channel modes are valid */
+ for (idx = 0; idx < AD559XR_CHANNEL_NR; idx++) {
+ switch (new_mode[i][idx]) {
+ case '0':
+ case '1':
+ case '2':
+ case '3':
+ case '8':
+ continue;
+ default:
+ /* There is invalid mode exist, ignore the settings */
+ pr_err("%s: invalid(%c) in index(%d)\n",
+ __func__, new_mode[i][idx], idx);
+ goto inval_para;
+ }
+ }
+
+inval_para:
+ /* There is invalid parameters setting in current parameter, so ignore it */
+ if (idx < AD559XR_CHANNEL_NR)
+ continue;
+
+ /* Set the new modes to ad5592r-base driver to setup the new channe modes */
+ memset(tmp, 0, 2);
+ for (idx = 0; idx < AD559XR_CHANNEL_NR; idx++) {
+ tmp[0] = new_mode[i][idx];
+ if (kstrtou8(tmp, 10, &new_ch_modes[AD559XR_CHANNEL_NR - idx - 1])) {
+ pr_err("%s: kstr error idx(%d)\n", __func__, idx);
+ break;
+ }
+ }
+ /* Something error when convering the string to integer, ignore the settings */
+ if (idx < AD559XR_CHANNEL_NR)
+ continue;
+
+ ad5592r_update_default_channel_modes(new_ch_modes);
+ break;
+ }
+}
+
static int ad5593r_i2c_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)
{
+ ad5593r_check_new_channel_mode();
return ad5592r_probe(&i2c->dev, id->name, &ad5593r_rw_ops);
}
--
2.17.1
Hi William,
Please read my comment below.
William Sung <[email protected]> 於 2020年8月24日 週一 下午1:43寫道：
>
> To use ad5593r more flexibly, we use the module parameter to setting the
> channel modes dynamically whenever the module probe up. Users can pass
> the channel modes to the module parameter for allocating the
> functionality of channels as desired.
>
> For example:
> * Use in the kernel command line:
> Users can add the module parameter in the kernel command line such as
>
> "ad5593r.ch_mode_cmdline=88001122"
>
> "88001122" means the channel mode setting for each channel. The most
> left side indicates the mode of channel 7, and the most right side
> indicates the mode of channel 0.
>
> * Use when manually probe the module:
> Similar to the kernel command line usage, users can enter
>
> "modprobe ad5593r ch_mode_probe=88001122"
The usage of the parameters do not match the one written in the code,
which is "ch_mode"
>
> to start the ad5593r module with the desired channel mode setting.
>
> Signed-off-by: William Sung <[email protected]>
> ---
> drivers/iio/dac/ad5592r-base.c | 33 ++++++++++++++++++--
> drivers/iio/dac/ad5592r-base.h | 4 +++
> drivers/iio/dac/ad5593r.c | 55 ++++++++++++++++++++++++++++++++++
> 3 files changed, 89 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/dac/ad5592r-base.c b/drivers/iio/dac/ad5592r-base.c
> index cc4875660a69..cd69a34fa21e 100644
> --- a/drivers/iio/dac/ad5592r-base.c
> +++ b/drivers/iio/dac/ad5592r-base.c
> @@ -21,6 +21,10 @@
>
> #include "ad5592r-base.h"
>
> +/* Parameters for dynamic channel mode setting */
> +static u8 update_channel_mode;
> +static u8 new_channel_modes[AD559XR_CHANNEL_NR];
> +
> static int ad5592r_gpio_get(struct gpio_chip *chip, unsigned offset)
> {
> struct ad5592r_state *st = gpiochip_get_data(chip);
> @@ -132,7 +136,7 @@ static int ad5592r_gpio_init(struct ad5592r_state *st)
>
> st->gpiochip.label = dev_name(st->dev);
> st->gpiochip.base = -1;
> - st->gpiochip.ngpio = 8;
> + st->gpiochip.ngpio = AD559XR_CHANNEL_NR;
> st->gpiochip.parent = st->dev;
> st->gpiochip.can_sleep = true;
> st->gpiochip.direction_input = ad5592r_gpio_direction_input;
> @@ -287,6 +291,14 @@ static int ad5592r_set_channel_modes(struct ad5592r_state *st)
> return ret;
> }
>
> +static void ad5592r_set_def_channel_modes(struct ad5592r_state *st)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(st->channel_modes); i++)
> + st->channel_modes[i] = new_channel_modes[i];
> +}
> +
> static int ad5592r_reset_channel_modes(struct ad5592r_state *st)
> {
> int i;
> @@ -532,6 +544,10 @@ static int ad5592r_alloc_channels(struct iio_dev *iio_dev)
> st->channel_offstate[reg] = tmp;
> }
>
> + /* Update default channel modes set by external module */
> + if (update_channel_mode == 1)
> + ad5592r_set_def_channel_modes(st);
> +
> channels = devm_kcalloc(st->dev,
> 1 + 2 * num_channels, sizeof(*channels),
> GFP_KERNEL);
> @@ -567,7 +583,7 @@ static int ad5592r_alloc_channels(struct iio_dev *iio_dev)
> }
>
> channels[curr_channel].type = IIO_TEMP;
> - channels[curr_channel].channel = 8;
> + channels[curr_channel].channel = AD559XR_CHANNEL_NR;
> channels[curr_channel].info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> BIT(IIO_CHAN_INFO_SCALE) |
> BIT(IIO_CHAN_INFO_OFFSET);
> @@ -589,6 +605,17 @@ static void ad5592r_init_scales(struct ad5592r_state *st, int vref_mV)
> div_s64_rem(tmp * 2, 1000000000LL, &st->scale_avail[1][1]);
> }
>
> +void ad5592r_update_default_channel_modes(u8 *new_modes)
> +{
> + int idx = 0;
> +
> + update_channel_mode = 1;
> + for (idx = 0; idx < AD559XR_CHANNEL_NR; idx++)
> + new_channel_modes[idx] = new_modes[idx];
> +
> +}
> +EXPORT_SYMBOL_GPL(ad5592r_update_default_channel_modes);
> +
> int ad5592r_probe(struct device *dev, const char *name,
> const struct ad5592r_rw_ops *ops)
> {
> @@ -603,7 +630,7 @@ int ad5592r_probe(struct device *dev, const char *name,
> st = iio_priv(iio_dev);
> st->dev = dev;
> st->ops = ops;
> - st->num_channels = 8;
> + st->num_channels = AD559XR_CHANNEL_NR;
> dev_set_drvdata(dev, iio_dev);
>
> st->reg = devm_regulator_get_optional(dev, "vref");
> diff --git a/drivers/iio/dac/ad5592r-base.h b/drivers/iio/dac/ad5592r-base.h
> index 23dac2f1ff8a..40ad6369e660 100644
> --- a/drivers/iio/dac/ad5592r-base.h
> +++ b/drivers/iio/dac/ad5592r-base.h
> @@ -39,6 +39,9 @@ enum ad5592r_registers {
> #define AD5592R_REG_CTRL_ADC_RANGE BIT(5)
> #define AD5592R_REG_CTRL_DAC_RANGE BIT(4)
>
> +/* Define quantity of channels of AD5592R/AD5593R */
> +#define AD559XR_CHANNEL_NR 8
> +
> struct ad5592r_rw_ops {
> int (*write_dac)(struct ad5592r_state *st, unsigned chan, u16 value);
> int (*read_adc)(struct ad5592r_state *st, unsigned chan, u16 *value);
> @@ -69,6 +72,7 @@ struct ad5592r_state {
> __be16 spi_msg_nop;
> };
>
> +void ad5592r_update_default_channel_modes(u8 *new_modes);
> int ad5592r_probe(struct device *dev, const char *name,
> const struct ad5592r_rw_ops *ops);
> int ad5592r_remove(struct device *dev);
> diff --git a/drivers/iio/dac/ad5593r.c b/drivers/iio/dac/ad5593r.c
> index 1fbe9c019c7f..dfc453a75ad6 100644
> --- a/drivers/iio/dac/ad5593r.c
> +++ b/drivers/iio/dac/ad5593r.c
> @@ -21,6 +21,10 @@
> #define AD5593R_MODE_GPIO_READBACK (6 << 4)
> #define AD5593R_MODE_REG_READBACK (7 << 4)
>
> +/* Parameters for dynamic channel mode setting */
> +static char *ch_mode = "";
> +module_param(ch_mode, charp, 0400);
> +
> static int ad5593r_write_dac(struct ad5592r_state *st, unsigned chan, u16 value)
> {
> struct i2c_client *i2c = to_i2c_client(st->dev);
> @@ -92,9 +96,60 @@ static const struct ad5592r_rw_ops ad5593r_rw_ops = {
> .gpio_read = ad5593r_gpio_read,
> };
>
> +static void ad5593r_check_new_channel_mode(void)
> +{
> + char *new_mode[2] = {NULL, NULL}, tmp[2];
> + u8 new_ch_modes[AD559XR_CHANNEL_NR];
> + int idx = 0, cnt = 0, i;
> +
> + if (strlen(ch_mode) == AD559XR_CHANNEL_NR)
> + new_mode[cnt++] = ch_mode;
> +
> + for (i = 0; i < cnt; i++) {
> + /* Check if all channel modes are valid */
> + for (idx = 0; idx < AD559XR_CHANNEL_NR; idx++) {
> + switch (new_mode[i][idx]) {
> + case '0':
> + case '1':
> + case '2':
> + case '3':
> + case '8':
> + continue;
> + default:
> + /* There is invalid mode exist, ignore the settings */
> + pr_err("%s: invalid(%c) in index(%d)\n",
> + __func__, new_mode[i][idx], idx);
> + goto inval_para;
> + }
> + }
It looks like the for loop is redundant, the cnt is 1 or 0, you can
return it directly if cnt equals to 0
> +
> +inval_para:
> + /* There is invalid parameters setting in current parameter, so ignore it */
> + if (idx < AD559XR_CHANNEL_NR)
> + continue;
> +
> + /* Set the new modes to ad5592r-base driver to setup the new channe modes */
> + memset(tmp, 0, 2);
> + for (idx = 0; idx < AD559XR_CHANNEL_NR; idx++) {
> + tmp[0] = new_mode[i][idx];
> + if (kstrtou8(tmp, 10, &new_ch_modes[AD559XR_CHANNEL_NR - idx - 1])) {
> + pr_err("%s: kstr error idx(%d)\n", __func__, idx);
> + break;
> + }
> + }
> + /* Something error when convering the string to integer, ignore the settings */
> + if (idx < AD559XR_CHANNEL_NR)
> + continue;
> +
> + ad5592r_update_default_channel_modes(new_ch_modes);
> + break;
> + }
> +}
> +
> static int ad5593r_i2c_probe(struct i2c_client *i2c,
> const struct i2c_device_id *id)
> {
> + ad5593r_check_new_channel_mode();
> return ad5592r_probe(&i2c->dev, id->name, &ad5593r_rw_ops);
> }
>
> --
> 2.17.1
>
On Mon, Aug 24, 2020 at 8:54 AM William Sung
<[email protected]> wrote:
>
> To use ad5593r more flexibly, we use the module parameter to setting the
> channel modes dynamically whenever the module probe up.
> Users can pass
> the channel modes to the module parameter for allocating the
> functionality of channels as desired.
NAK. We have a sysfs interface.
> For example:
> * Use in the kernel command line:
> Users can add the module parameter in the kernel command line such as
>
> "ad5593r.ch_mode_cmdline=88001122"
>
> "88001122" means the channel mode setting for each channel. The most
> left side indicates the mode of channel 7, and the most right side
> indicates the mode of channel 0.
>
> * Use when manually probe the module:
> Similar to the kernel command line usage, users can enter
>
> "modprobe ad5593r ch_mode_probe=88001122"
>
> to start the ad5593r module with the desired channel mode setting.
Again NAK, this basically should come from Device Tree or ACPI.
--
With Best Regards,
Andy Shevchenko
Hi Jonathan and Andy,
This patch is mainly for Advantech's UNO-420[1] which is a x86-based platform.
This platform is more like a development platform for customers to
customize their products,
so, specify the channel modes in ACPI table is not generic enough,
that's why William submit this patch.
Are there other ways to specify or pass values to the module without
using module parameters?
It's good if we can leverage sysfs, but I don't know if there is one
for this scenario.
1. https://www.advantech.com/products/9a0cc561-8fc2-4e22-969c-9df90a3952b5/uno-420/mod_2d6a546b-39e3-4bc4-bbf4-ac89e6b7667c
Andy Shevchenko <[email protected]> 於 2020年8月31日 週一 上午1:07寫道：
>
> On Mon, Aug 24, 2020 at 8:54 AM William Sung
> <[email protected]> wrote:
> >
> > To use ad5593r more flexibly, we use the module parameter to setting the
> > channel modes dynamically whenever the module probe up.
>
> > Users can pass
> > the channel modes to the module parameter for allocating the
> > functionality of channels as desired.
>
> NAK. We have a sysfs interface.
>
> > For example:
> > * Use in the kernel command line:
> > Users can add the module parameter in the kernel command line such as
> >
> > "ad5593r.ch_mode_cmdline=88001122"
> >
> > "88001122" means the channel mode setting for each channel. The most
> > left side indicates the mode of channel 7, and the most right side
> > indicates the mode of channel 0.
> >
> > * Use when manually probe the module:
> > Similar to the kernel command line usage, users can enter
> >
> > "modprobe ad5593r ch_mode_probe=88001122"
> >
> > to start the ad5593r module with the desired channel mode setting.
>
> Again NAK, this basically should come from Device Tree or ACPI.
>
> --
> With Best Regards,
> Andy Shevchenko
On Mon, Aug 31, 2020 at 2:28 PM AceLan Kao <[email protected]> wrote:
> This patch is mainly for Advantech's UNO-420[1] which is a x86-based platform.
> This platform is more like a development platform for customers to
> customize their products,
> so, specify the channel modes in ACPI table is not generic enough,
> that's why William submit this patch.
>
> Are there other ways to specify or pass values to the module without
> using module parameters?
> It's good if we can leverage sysfs, but I don't know if there is one
> for this scenario.
Can we provide DT bindings for that and use then in ACPI? ACPI has a
possibility to reuse DT properties and compatible strings [1]. As far
as I can see the driver uses fwnode API, so it supports ACPI case
already [2]. So, what prevents you to utilize 'adi,mode' property?
Also, we accept examples of ASL excerpt in meta-acpi project [3]. It
has already plenty of examples [4] how to use PRP0001 for DIY /
development boards.
So, take all together I think this patch is simple redundant.
[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
P.S. Jonathan, it seems this driver has artificial ACPI HID. We
probably have to remove it. However, ADS is indeed reserved for Analog
Devices in PNP registry. Can we have AD's official answer on this?
Cc'ing additional AD people.
> 1. https://www.advantech.com/products/9a0cc561-8fc2-4e22-969c-9df90a3952b5/uno-420/mod_2d6a546b-39e3-4bc4-bbf4-ac89e6b7667c
--
With Best Regards,
Andy Shevchenko
On Mon, Aug 31, 2020 at 3:45 PM Andy Shevchenko
<[email protected]> wrote:
> On Mon, Aug 31, 2020 at 2:28 PM AceLan Kao <[email protected]> wrote:
> > This patch is mainly for Advantech's UNO-420[1] which is a x86-based platform.
> > This platform is more like a development platform for customers to
> > customize their products,
> > so, specify the channel modes in ACPI table is not generic enough,
> > that's why William submit this patch.
> >
> > Are there other ways to specify or pass values to the module without
> > using module parameters?
> > It's good if we can leverage sysfs, but I don't know if there is one
> > for this scenario.
>
> Can we provide DT bindings for that and use then in ACPI? ACPI has a
> possibility to reuse DT properties and compatible strings [1]. As far
> as I can see the driver uses fwnode API, so it supports ACPI case
> already [2]. So, what prevents you to utilize 'adi,mode' property?
>
> Also, we accept examples of ASL excerpt in meta-acpi project [3]. It
> has already plenty of examples [4] how to use PRP0001 for DIY /
> development boards.
>
> So, take all together I think this patch is simple redundant.
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
>
> P.S. Jonathan, it seems this driver has artificial ACPI HID. We
> probably have to remove it. However, ADS is indeed reserved for Analog
> Devices in PNP registry. Can we have AD's official answer on this?
> Cc'ing additional AD people.
>
> > 1. https://www.advantech.com/products/9a0cc561-8fc2-4e22-969c-9df90a3952b5/uno-420/mod_2d6a546b-39e3-4bc4-bbf4-ac89e6b7667c
--
With Best Regards,
Andy Shevchenko
> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Montag, 31. August 2020 14:46
> To: AceLan Kao <[email protected]>; Ardelean, Alexandru
> <[email protected]>
> Cc: William Sung <[email protected]>; Lars-Peter Clausen
> <[email protected]>; Hennerich, Michael <[email protected]>;
> Jonathan Cameron <[email protected]>; Hartmut Knaack <[email protected]>;
> Peter Meerwald-Stadler <[email protected]>; linux-iio <linux-
> [email protected]>; Linux Kernel Mailing List <[email protected]>;
> Campion Kang <[email protected]>
> Subject: Re: [PATCH] iio: dac: ad5593r: Dynamically set AD5593R channel
> modes
>
> On Mon, Aug 31, 2020 at 2:28 PM AceLan Kao <[email protected]>
> wrote:
> > This patch is mainly for Advantech's UNO-420[1] which is a x86-based
> platform.
> > This platform is more like a development platform for customers to
> > customize their products, so, specify the channel modes in ACPI table
> > is not generic enough, that's why William submit this patch.
> >
> > Are there other ways to specify or pass values to the module without
> > using module parameters?
> > It's good if we can leverage sysfs, but I don't know if there is one
> > for this scenario.
>
> Can we provide DT bindings for that and use then in ACPI? ACPI has a possibility
> to reuse DT properties and compatible strings [1]. As far as I can see the driver
> uses fwnode API, so it supports ACPI case already [2]. So, what prevents you to
> utilize 'adi,mode' property?
>
> Also, we accept examples of ASL excerpt in meta-acpi project [3]. It has already
> plenty of examples [4] how to use PRP0001 for DIY / development boards.
>
> So, take all together I think this patch is simple redundant.
>
> [1]:
> https://urldefense.com/v3/__https://www.kernel.org/doc/html/latest/firmwar
> e-guide/acpi/enumeration.html*device-tree-namespace-link-device-
> id__;Iw!!A3Ni8CS0y2Y!oe0bVwE-
> D8Y6QiWYU06pwxclGSgFpFQ10Rdozy5pCKZYmQ3SvTvEn8OyjaL1efojRqroyg$
> [2]: https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v5.9-
> rc3/source/Documentation/devicetree/bindings/iio/dac/ad5592r.txt__;!!A3Ni8
> CS0y2Y!oe0bVwE-
> D8Y6QiWYU06pwxclGSgFpFQ10Rdozy5pCKZYmQ3SvTvEn8OyjaL1efr_TTE4CQ
> $
> [3]: https://urldefense.com/v3/__https://github.com/westeri/meta-
> acpi__;!!A3Ni8CS0y2Y!oe0bVwE-
> D8Y6QiWYU06pwxclGSgFpFQ10Rdozy5pCKZYmQ3SvTvEn8OyjaL1efpZnQjgBg
> $
> [4]: https://urldefense.com/v3/__https://github.com/westeri/meta-
> acpi/tree/master/recipes-bsp/acpi-tables/samples__;!!A3Ni8CS0y2Y!oe0bVwE-
> D8Y6QiWYU06pwxclGSgFpFQ10Rdozy5pCKZYmQ3SvTvEn8OyjaL1efqpzDeR7A
> $
>
> P.S. Jonathan, it seems this driver has artificial ACPI HID. We probably have to
> remove it. However, ADS is indeed reserved for Analog Devices in PNP registry.
> Can we have AD's official answer on this?
> Cc'ing additional AD people.
Agreed, this ID was chosen under the PNP ID Vendor Space for Analog Devices Inc.
Days back, I did a quick kernel grep, and there are many drivers which use the 3-letter
PNP ID as acpi_device_id. So, I thought this being not an issue.
I'm against removing it since I know people shipping this in their ACPI tables already.
Regarding ACPI DSD customization, one way to do this is to move this into the BIOS.
This way the particular piece of HW can be customized rather than manage HW
connections in software.
Sorry for the misformatted email reply.
>
> > 1.
> >
> https://urldefense.com/v3/__https://www.advantech.com/products/9a0cc56
> > 1-8fc2-4e22-969c-9df90a3952b5/uno-420/mod_2d6a546b-39e3-4bc4-
> bbf4-ac89
> > e6b7667c__;!!A3Ni8CS0y2Y!oe0bVwE-
> D8Y6QiWYU06pwxclGSgFpFQ10Rdozy5pCKZYm
> > Q3SvTvEn8OyjaL1efp-eZqbcQ$
>
>
> --
> With Best Regards,
> Andy Shevchenko
Best Regards,
Michael
Analog Devices GmbH
Michael Hennerich
Otl-Aicher Strasse 60-64
D-80807 Muenchen; Germany
mail: [email protected]
http://www.analog.com
Sitz der Gesellschaft München, Registergericht München HRB 40368,
Geschäftsführer: Stefan Steyerl, Thomas Edward Cribben, Michael Paul Sondel
On Wed, Sep 2, 2020 at 11:09 AM Hennerich, Michael
<[email protected]> wrote:
> > From: Andy Shevchenko <[email protected]>
> > Sent: Montag, 31. August 2020 14:46
> > On Mon, Aug 31, 2020 at 2:28 PM AceLan Kao <[email protected]>
> > wrote:
...
> > P.S. Jonathan, it seems this driver has artificial ACPI HID. We probably have to
> > remove it. However, ADS is indeed reserved for Analog Devices in PNP registry.
> > Can we have AD's official answer on this?
> > Cc'ing additional AD people.
>
> Agreed, this ID was chosen under the PNP ID Vendor Space for Analog Devices Inc.
> Days back, I did a quick kernel grep, and there are many drivers which use the 3-letter
> PNP ID as acpi_device_id. So, I thought this being not an issue.
No, no, the use of PNP ID is not an issue. The point is if the ID is
artificial or official.
> I'm against removing it since I know people shipping this in their ACPI tables already.
I see. Can we consider this email as the official answer from AD that
this ID is being allocated for this certain component?
> Regarding ACPI DSD customization, one way to do this is to move this into the BIOS.
> This way the particular piece of HW can be customized rather than manage HW
> connections in software.
Assuming the confirmation on the above, indeed, one may use ACPI HID
with DSD properties in the firmware. Main purpose of PRP0001 is the
*development* stage of the product at which a vendor should take care
about allocation of proper ACPI IDs for the components in use. Yes, I
know that this is not always feasible b/c some HW component vendors
don't care about ACPI at all.
--
With Best Regards,
Andy Shevchenko
> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Mittwoch, 2. September 2020 10:52
> To: Hennerich, Michael <[email protected]>
> Cc: AceLan Kao <[email protected]>; Ardelean, Alexandru
> <[email protected]>; William Sung
> <[email protected]>; Lars-Peter Clausen <[email protected]>;
> Jonathan Cameron <[email protected]>; Hartmut Knaack <[email protected]>;
> Peter Meerwald-Stadler <[email protected]>; linux-iio <linux-
> [email protected]>; Linux Kernel Mailing List <[email protected]>;
> Campion Kang <[email protected]>
> Subject: Re: [PATCH] iio: dac: ad5593r: Dynamically set AD5593R channel
> modes
>
> [External]
>
> On Wed, Sep 2, 2020 at 11:09 AM Hennerich, Michael
> <[email protected]> wrote:
> > > From: Andy Shevchenko <[email protected]>
> > > Sent: Montag, 31. August 2020 14:46
> > > On Mon, Aug 31, 2020 at 2:28 PM AceLan Kao
> > > <[email protected]>
> > > wrote:
>
> ...
>
> > > P.S. Jonathan, it seems this driver has artificial ACPI HID. We
> > > probably have to remove it. However, ADS is indeed reserved for Analog
> Devices in PNP registry.
> > > Can we have AD's official answer on this?
> > > Cc'ing additional AD people.
> >
> > Agreed, this ID was chosen under the PNP ID Vendor Space for Analog Devices
> Inc.
> > Days back, I did a quick kernel grep, and there are many drivers which
> > use the 3-letter PNP ID as acpi_device_id. So, I thought this being not an issue.
>
> No, no, the use of PNP ID is not an issue. The point is if the ID is artificial or
> official.
>
> > I'm against removing it since I know people shipping this in their ACPI tables
> already.
>
> I see. Can we consider this email as the official answer from AD that this ID is
> being allocated for this certain component?
To my knowledge only PNP/ACPI vendor IDs need to be registered with the UEFI
ACPI working group. AD part numbers are unique. The ID chosen uses the
part number prefixed with the PNP Vendor ID. ADxxxx->ADSxxxx
Please consider this as allocated.
>
> > Regarding ACPI DSD customization, one way to do this is to move this into the
> BIOS.
> > This way the particular piece of HW can be customized rather than
> > manage HW connections in software.
>
> Assuming the confirmation on the above, indeed, one may use ACPI HID with
> DSD properties in the firmware. Main purpose of PRP0001 is the
> *development* stage of the product at which a vendor should take care about
> allocation of proper ACPI IDs for the components in use. Yes, I know that this is
> not always feasible b/c some HW component vendors don't care about ACPI at
> all.
>
> --
> With Best Regards,
> Andy Shevchenko
Best Regards,
Michael
Analog Devices GmbH
Michael Hennerich
Otl-Aicher Strasse 60-64
D-80807 Muenchen; Germany
mail: [email protected]
http://www.analog.com
Sitz der Gesellschaft München, Registergericht München HRB 40368,
Geschäftsführer: Stefan Steyerl, Thomas Edward Cribben, Michael Paul Sondel
On Wed, Sep 2, 2020 at 12:12 PM Hennerich, Michael
<[email protected]> wrote:
> > From: Andy Shevchenko <[email protected]>
> > Sent: Mittwoch, 2. September 2020 10:52
> > On Wed, Sep 2, 2020 at 11:09 AM Hennerich, Michael
> > <[email protected]> wrote:
...
> > I see. Can we consider this email as the official answer from AD that this ID is
> > being allocated for this certain component?
>
> To my knowledge only PNP/ACPI vendor IDs need to be registered with the UEFI
> ACPI working group.
Correct.
> AD part numbers are unique. The ID chosen uses the
> part number prefixed with the PNP Vendor ID. ADxxxx->ADSxxxx
> Please consider this as allocated.
Thank you for confirming this!
--
With Best Regards,
Andy Shevchenko
On Wed, 2 Sep 2020 12:28:02 +0300
Andy Shevchenko <[email protected]> wrote:
> On Wed, Sep 2, 2020 at 12:12 PM Hennerich, Michael
> <[email protected]> wrote:
> > > From: Andy Shevchenko <[email protected]>
> > > Sent: Mittwoch, 2. September 2020 10:52
> > > On Wed, Sep 2, 2020 at 11:09 AM Hennerich, Michael
> > > <[email protected]> wrote:
>
> ...
>
> > > I see. Can we consider this email as the official answer from AD that this ID is
> > > being allocated for this certain component?
> >
> > To my knowledge only PNP/ACPI vendor IDs need to be registered with the UEFI
> > ACPI working group.
>
> Correct.
>
> > AD part numbers are unique. The ID chosen uses the
> > part number prefixed with the PNP Vendor ID. ADxxxx->ADSxxxx
> > Please consider this as allocated.
>
> Thank you for confirming this!
>
Great to clear this particular one up.
One side note on this for anyone who might find this email thread.
If we do have an ACPI ID that doesn't use an official
PNP or ACPI ID, but there are boards shipping with it, then we can't
drop the ID. We can however add a note to the driver making the
point that it is not an official ID and so we may remove it if
all known boards that used it are now considered obsolete.
Even better to list a product it does occur in so we avoid
having the conversation again!
A lot of these got through my reviews due to lack of knowledge of how
ACPI IDs worked until a few years ago. Still like all ABI we are
stuck with it until we can remove it without anyone noticing :(
Jonathan
Hi Andy,
I spent some time studying/reading what you wrote, but I still don't
understand how to leverage meta-acpi.
From what I understand from the following discussion in the thread,
ADS5593 could be used,
so we can keep using it and don't have to introduce PRP0001 in the table, right?
Here is the ADS5593 asl code, but I have no idea how to re-use it
after it's been modified,
the only way I know is to override the ACPI tables via initrd[1].
Could you share some examples in real cases that I can follow?
Thanks.
1. Documentation/admin-guide/acpi/initrd_table_override.rst
Andy Shevchenko <[email protected]> 於 2020年8月31日 週一 下午8:48寫道：
>
> On Mon, Aug 31, 2020 at 3:45 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Mon, Aug 31, 2020 at 2:28 PM AceLan Kao <[email protected]> wrote:
> > > This patch is mainly for Advantech's UNO-420[1] which is a x86-based platform.
> > > This platform is more like a development platform for customers to
> > > customize their products,
> > > so, specify the channel modes in ACPI table is not generic enough,
> > > that's why William submit this patch.
> > >
> > > Are there other ways to specify or pass values to the module without
> > > using module parameters?
> > > It's good if we can leverage sysfs, but I don't know if there is one
> > > for this scenario.
> >
> > Can we provide DT bindings for that and use then in ACPI? ACPI has a
> > possibility to reuse DT properties and compatible strings [1]. As far
> > as I can see the driver uses fwnode API, so it supports ACPI case
> > already [2]. So, what prevents you to utilize 'adi,mode' property?
> >
> > Also, we accept examples of ASL excerpt in meta-acpi project [3]. It
> > has already plenty of examples [4] how to use PRP0001 for DIY /
> > development boards.
> >
> > So, take all together I think this patch is simple redundant.
>
> 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
> >
> > P.S. Jonathan, it seems this driver has artificial ACPI HID. We
> > probably have to remove it. However, ADS is indeed reserved for Analog
> > Devices in PNP registry. Can we have AD's official answer on this?
> > Cc'ing additional AD people.
> >
> > > 1. https://www.advantech.com/products/9a0cc561-8fc2-4e22-969c-9df90a3952b5/uno-420/mod_2d6a546b-39e3-4bc4-bbf4-ac89e6b7667c
>
> --
> With Best Regards,
> Andy Shevchenko
On Thu, Sep 3, 2020 at 10:37 AM AceLan Kao <[email protected]> wrote:
>
> Hi Andy,
>
> 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.
> From what I understand from the following discussion in the thread,
> ADS5593 could be used,
> so we can keep using it and don't have to introduce PRP0001 in the table, right?
Precisely!
> Here is the ADS5593 asl code, but I have no idea how to re-use it
> after it's been modified,
> the only way I know is to override the ACPI tables via initrd[1].
There is also Config FS approach (like overlays) to do it at runtime.
That what we are using in Yocto build for Intel Edison.
> Could you share some examples in real cases that I can follow?
Yes, like I mentioned StackOverflow search results (maybe G will give better).
But let's see what you have in your ASL code first.
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.
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
> Thanks.
>
> 1. Documentation/admin-guide/acpi/initrd_table_override.rst
>
> Andy Shevchenko <[email protected]> 於 2020年8月31日 週一 下午8:48寫道：
> >
> > On Mon, Aug 31, 2020 at 3:45 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Mon, Aug 31, 2020 at 2:28 PM AceLan Kao <[email protected]> wrote:
> > > > This patch is mainly for Advantech's UNO-420[1] which is a x86-based platform.
> > > > This platform is more like a development platform for customers to
> > > > customize their products,
> > > > so, specify the channel modes in ACPI table is not generic enough,
> > > > that's why William submit this patch.
> > > >
> > > > Are there other ways to specify or pass values to the module without
> > > > using module parameters?
> > > > It's good if we can leverage sysfs, but I don't know if there is one
> > > > for this scenario.
> > >
> > > Can we provide DT bindings for that and use then in ACPI? ACPI has a
> > > possibility to reuse DT properties and compatible strings [1]. As far
> > > as I can see the driver uses fwnode API, so it supports ACPI case
> > > already [2]. So, what prevents you to utilize 'adi,mode' property?
> > >
> > > Also, we accept examples of ASL excerpt in meta-acpi project [3]. It
> > > has already plenty of examples [4] how to use PRP0001 for DIY /
> > > development boards.
> > >
> > > So, take all together I think this patch is simple redundant.
> >
> > 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
> > >
> > > P.S. Jonathan, it seems this driver has artificial ACPI HID. We
> > > probably have to remove it. However, ADS is indeed reserved for Analog
> > > Devices in PNP registry. Can we have AD's official answer on this?
> > > Cc'ing additional AD people.
> > >
> > > > 1. https://www.advantech.com/products/9a0cc561-8fc2-4e22-969c-9df90a3952b5/uno-420/mod_2d6a546b-39e3-4bc4-bbf4-ac89e6b7667c
--
With Best Regards,
Andy Shevchenko
On Thu, Sep 3, 2020 at 1:42 PM Andy Shevchenko
<[email protected]> wrote:
> On Thu, Sep 3, 2020 at 10:37 AM AceLan Kao <[email protected]> wrote:
Couple additional notes.
...
> > 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
Hi Andy,
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
$ 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
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?
We may have a chance to convince BIOS to move ADS5593 to the SSDT
table, do you think it's a good idea?
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?
Thanks.
Andy Shevchenko <[email protected]> 於 2020年9月3日 週四 下午6:54寫道：
>
> On Thu, Sep 3, 2020 at 1:42 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Thu, Sep 3, 2020 at 10:37 AM AceLan Kao <[email protected]> wrote:
>
> Couple additional notes.
>
> ...
>
> > > 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
On Fri, Sep 4, 2020 at 5:25 AM AceLan Kao <[email protected]> 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
On Fri, Sep 4, 2020 at 5:34 AM William.Sung
<[email protected]> wrote:
> > -----Original Message-----
> > From: Andy Shevchenko <[email protected]>
> > Sent: Thursday, September 3, 2020 6:42 PM
> > On Thu, Sep 3, 2020 at 10:37 AM AceLan Kao <[email protected]>
> > wrote:
...
> > > Here is the ADS5593 asl code, but I have no idea how to re-use it
> > > after it's been modified, the only way I know is to override the ACPI
> > > tables via initrd[1].
> >
> > There is also Config FS approach (like overlays) to do it at runtime.
> > That what we are using in Yocto build for Intel Edison.
> >
> > > Could you share some examples in real cases that I can follow?
> >
> > Yes, like I mentioned StackOverflow search results (maybe G will give better).
> > But let's see what you have in your ASL code first.
> >
> > 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.
> >
> > Do you have issues with loading it (as is and after above addressed)?
> >
>
> Maybe I can explain it.
> In the beginning, I set I2C1 to both scope and reference in I2cSerialBus but it
> doesn't work. Then I check the probe progress of the i2c controller and found that
> the fwnode of i2c controller has replaced by \\_SB.PCI0.D022. After modifying the reference
> path to it, the ad5593r driver works.
Okay, so do I understand this right that you now have working case?
About the reference node. Can I see DSDT (you may send it privately if
you consider it not for others)?
And can you elaborate a bit about I²C host controllers on your
platform and how they got enumerated (ACPI / PCI)?
As I explained AceLan in previous reply the ADS5593 should be a child
node of the host controller node in DSDT.
> I also did the different tests by switching the path of scope and reference between
> I2C1 and PCI0.D022, and the result seems like only the reference path of I2cSerialBus
> would impact to the ad5593r.
Because we have a workaround in the kernel to find all I2cSerialBus()
resources over the ACPI namespace. It *does not* mean you can leave it
like this. You basically need to understand device hierarchy in your
DSDT. It seems that actual controller is defined as D022 under PCI0 in
SB (System Bus) scope.
> I will fix it on the same path for consistency and also preventing to have doubts about these.
>
> About the second _ADR value wrong, from now on the second channel can be set to the proper
> mode. But it still needs to be fixed since it was a typo wrong.
Basically the ACPI way is to use _ADR, but DT uses 'reg' property for
that. I hope at some point we will get some unification between those
two in Linux kernel to have common API.
> > [5]:
> > https://github.com/westeri/meta-acpi/blob/master/recipes-bsp/acpi-tables/sa
> > mples/edison/ft6236.asli
> >
> > > Thanks.
> > >
> > > 1. Documentation/admin-guide/acpi/initrd_table_override.rst
> > >
> > > Andy Shevchenko <[email protected]> 於 2020年8月31日 週
> > 一 下午8:48寫道：
> > > >
> > > > On Mon, Aug 31, 2020 at 3:45 PM Andy Shevchenko
> > > > <[email protected]> wrote:
> > > > > On Mon, Aug 31, 2020 at 2:28 PM AceLan Kao
> > <[email protected]> wrote:
> > > > > > This patch is mainly for Advantech's UNO-420[1] which is a x86-based
> > platform.
> > > > > > This platform is more like a development platform for customers
> > > > > > to customize their products, so, specify the channel modes in
> > > > > > ACPI table is not generic enough, that's why William submit this
> > > > > > patch.
> > > > > >
> > > > > > Are there other ways to specify or pass values to the module
> > > > > > without using module parameters?
> > > > > > It's good if we can leverage sysfs, but I don't know if there is
> > > > > > one for this scenario.
> > > > >
> > > > > Can we provide DT bindings for that and use then in ACPI? ACPI has
> > > > > a possibility to reuse DT properties and compatible strings [1].
> > > > > As far as I can see the driver uses fwnode API, so it supports
> > > > > ACPI case already [2]. So, what prevents you to utilize 'adi,mode'
> > property?
> > > > >
> > > > > Also, we accept examples of ASL excerpt in meta-acpi project [3].
> > > > > It has already plenty of examples [4] how to use PRP0001 for DIY /
> > > > > development boards.
> > > > >
> > > > > So, take all together I think this patch is simple redundant.
> > > >
> > > > 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/enumera
> > > > > tion.html#device-tree-namespace-link-device-id
> > > > > [2]:
> > > > > https://elixir.bootlin.com/linux/v5.9-rc3/source/Documentation/dev
> > > > > icetree/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
> > > > >
> > > > > P.S. Jonathan, it seems this driver has artificial ACPI HID. We
> > > > > probably have to remove it. However, ADS is indeed reserved for
> > > > > Analog Devices in PNP registry. Can we have AD's official answer on this?
> > > > > Cc'ing additional AD people.
> > > > >
> > > > > > 1.
> > > > > > https://www.advantech.com/products/9a0cc561-8fc2-4e22-969c-9df90
> > > > > > a3952b5/uno-420/mod_2d6a546b-39e3-4bc4-bbf4-ac89e6b7667c
--
With Best Regards,
Andy Shevchenko