| Message ID | 20200824054347.3805-1-william.sung@advantech.com.tw (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: dac: ad5593r: Dynamically set AD5593R channel modes | expand |
Hi William, Please read my comment below. William Sung <william.sung@advantech.com.tw> 於 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 <william.sung@advantech.com.tw> > --- > 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 <william.sung@advantech.com.tw> 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.
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 <andy.shevchenko@gmail.com> 於 2020年8月31日 週一 上午1:07寫道： > > On Mon, Aug 24, 2020 at 8:54 AM William Sung > <william.sung@advantech.com.tw> 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 <acelan.kao@canonical.com> 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
On Mon, Aug 31, 2020 at 3:45 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Aug 31, 2020 at 2:28 PM AceLan Kao <acelan.kao@canonical.com> 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
> -----Original Message----- > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Montag, 31. August 2020 14:46 > To: AceLan Kao <acelan.kao@canonical.com>; Ardelean, Alexandru > <alexandru.Ardelean@analog.com> > Cc: William Sung <william.sung@advantech.com.tw>; Lars-Peter Clausen > <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>; > Jonathan Cameron <jic23@kernel.org>; Hartmut Knaack <knaack.h@gmx.de>; > Peter Meerwald-Stadler <pmeerw@pmeerw.net>; linux-iio <linux- > iio@vger.kernel.org>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; > Campion Kang <Campion.Kang@advantech.com.tw> > Subject: Re: [PATCH] iio: dac: ad5593r: Dynamically set AD5593R channel > modes > > On Mon, Aug 31, 2020 at 2:28 PM AceLan Kao <acelan.kao@canonical.com> > 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: michael.hennerich@analog.com 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 <Michael.Hennerich@analog.com> wrote: > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > Sent: Montag, 31. August 2020 14:46 > > On Mon, Aug 31, 2020 at 2:28 PM AceLan Kao <acelan.kao@canonical.com> > > 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.
> -----Original Message----- > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Mittwoch, 2. September 2020 10:52 > To: Hennerich, Michael <Michael.Hennerich@analog.com> > Cc: AceLan Kao <acelan.kao@canonical.com>; Ardelean, Alexandru > <alexandru.Ardelean@analog.com>; William Sung > <william.sung@advantech.com.tw>; Lars-Peter Clausen <lars@metafoo.de>; > Jonathan Cameron <jic23@kernel.org>; Hartmut Knaack <knaack.h@gmx.de>; > Peter Meerwald-Stadler <pmeerw@pmeerw.net>; linux-iio <linux- > iio@vger.kernel.org>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; > Campion Kang <Campion.Kang@advantech.com.tw> > Subject: Re: [PATCH] iio: dac: ad5593r: Dynamically set AD5593R channel > modes > > [External] > > On Wed, Sep 2, 2020 at 11:09 AM Hennerich, Michael > <Michael.Hennerich@analog.com> wrote: > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > > Sent: Montag, 31. August 2020 14:46 > > > On Mon, Aug 31, 2020 at 2:28 PM AceLan Kao > > > <acelan.kao@canonical.com> > > > 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: michael.hennerich@analog.com 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 <Michael.Hennerich@analog.com> wrote: > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > Sent: Mittwoch, 2. September 2020 10:52 > > On Wed, Sep 2, 2020 at 11:09 AM Hennerich, Michael > > <Michael.Hennerich@analog.com> 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!
On Wed, 2 Sep 2020 12:28:02 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Sep 2, 2020 at 12:12 PM Hennerich, Michael > <Michael.Hennerich@analog.com> wrote: > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > > Sent: Mittwoch, 2. September 2020 10:52 > > > On Wed, Sep 2, 2020 at 11:09 AM Hennerich, Michael > > > <Michael.Hennerich@analog.com> 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 <andy.shevchenko@gmail.com> 於 2020年8月31日 週一 下午8:48寫道： > > On Mon, Aug 31, 2020 at 3:45 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Mon, Aug 31, 2020 at 2:28 PM AceLan Kao <acelan.kao@canonical.com> 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 <acelan.kao@canonical.com> 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 <andy.shevchenko@gmail.com> 於 2020年8月31日 週一 下午8:48寫道： > > > > On Mon, Aug 31, 2020 at 3:45 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Mon, Aug 31, 2020 at 2:28 PM AceLan Kao <acelan.kao@canonical.com> 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
On Thu, Sep 3, 2020 at 1:42 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Sep 3, 2020 at 10:37 AM AceLan Kao <acelan.kao@canonical.com> 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
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 <andy.shevchenko@gmail.com> 於 2020年9月3日 週四 下午6:54寫道： > > On Thu, Sep 3, 2020 at 1:42 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Thu, Sep 3, 2020 at 10:37 AM AceLan Kao <acelan.kao@canonical.com> 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 <acelan.kao@canonical.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
On Fri, Sep 4, 2020 at 5:34 AM William.Sung <William.Sung@advantech.com.tw> wrote: > > -----Original Message----- > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > Sent: Thursday, September 3, 2020 6:42 PM > > On Thu, Sep 3, 2020 at 10:37 AM AceLan Kao <acelan.kao@canonical.com> > > 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 <andy.shevchenko@gmail.com> 於 2020年8月31日 週 > > 一 下午8:48寫道： > > > > > > > > On Mon, Aug 31, 2020 at 3:45 PM Andy Shevchenko > > > > <andy.shevchenko@gmail.com> wrote: > > > > > On Mon, Aug 31, 2020 at 2:28 PM AceLan Kao > > <acelan.kao@canonical.com> 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
On Fri, Sep 4, 2020 at 12:09 PM William.Sung <William.Sung@advantech.com.tw> wrote: > > -----Original Message----- > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > Sent: Friday, September 4, 2020 3:48 PM > > To: William.Sung <William.Sung@advantech.com.tw> > > On Fri, Sep 4, 2020 at 5:34 AM William.Sung > > <William.Sung@advantech.com.tw> wrote: > > > > -----Original Message----- > > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > > > Sent: Thursday, September 3, 2020 6:42 PM On Thu, Sep 3, 2020 at > > > > 10:37 AM AceLan Kao <acelan.kao@canonical.com> > > > > wrote: I think we may return to mailing list now (I slightly censored your reply to me just in case). ... > > > > > 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)? > > The attachment is out original DSDT except ADS5593. Thank you, it's very helpful! > And the section of ADS5593 is what > we got, and we do a little modification such as i2c bus reference > and mode setting to fix our platform. After adding the part of ADS5593, we could successfully > probe up ad5593r with the driver in the current upstream source code. > > > And can you elaborate a bit about I²C host controllers on your platform and > > how they got enumerated (ACPI / PCI)? > > I'm sorry that I don't clearly know how the I2C host controller to be enumerated in our platform. > The things I know getting from our BIOS team is that they implement in PCI mode (not ACPI mode). > And the messages I got from dmesg show something like: > ACPI: bus type PCI registered > pci 0000:00:18.1 [8086:0f41] type 00 class 0x0c8000 // the i2c controller we used. > After catting the ADR from the fwnode of I2C controller, I got "0x00180001", so I do know it reference > to D022. But I do have no idea how it can be done. This is almost it, only one little part is missed in above is the ACPI ASL code, i.e. Device (D022) { Name (_ADR, 0x00180001) // _ADR: Address } that's how you got your firmware node for certain I²C host controller and that's why you need that device node. So, it means that your Scope should also point to this node. And if you wish you may consider ADS5593 code to be part of DSDT (just defined somewhere after D022). > > As I explained AceLan in previous reply the ADS5593 should be a child node of > > the host controller node in DSDT. > > > > Yes, you're right. > > > > 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. > May I know more about it? Sure. > In ads5592r-base.c, it parse the properties by: > device_for_each_child_node(st->dev, child) { > ret = fwnode_property_read_u32(child, "reg", ®); > if (ret || reg >= ARRAY_SIZE(st->channel_modes)) > continue; > > ret = fwnode_property_read_u32(child, "adi,mode", &tmp); > if (!ret) > st->channel_modes[reg] = tmp; > > fwnode_property_read_u32(child, "adi,off-state", &tmp); > if (!ret) > st->channel_offstate[reg] = tmp; > } > In my understanding, it looks like ad5592r-base parse the properties no matter what > he ADR value is. Is there a proper way that we could use ADR value instead? What I meant here is some API instead of ret = fwnode_property_read_u32(child, "reg", ®); do something like ret = fwnode_property_get_child_address(child, ®); and behind the scenes it will try 'reg' property followed by _ADR evalution. > > > > [5]: > > > > https://github.com/westeri/meta-acpi/blob/master/recipes-bsp/acpi-ta > > > > bles/sa > > > > mples/edison/ft6236.asli > > > > > > > > > Thanks. > > > > > > > > > > 1. Documentation/admin-guide/acpi/initrd_table_override.rst > > > > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2020年8月31日 > > 週 > > > > 一 下午8:48寫道： > > > > > > > > > > > > On Mon, Aug 31, 2020 at 3:45 PM Andy Shevchenko > > > > > > <andy.shevchenko@gmail.com> wrote: > > > > > > > On Mon, Aug 31, 2020 at 2:28 PM AceLan Kao > > > > <acelan.kao@canonical.com> 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/enu > > > > > > > mera 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/a > > > > > > > cpi- > > > > > > > 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-9 > > > > > > > > df90 > > > > > > > > a3952b5/uno-420/mod_2d6a546b-39e3-4bc4-bbf4-ac89e6b7667c
On Fri, Sep 4, 2020 at 2:38 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Sep 4, 2020 at 12:09 PM William.Sung > <William.Sung@advantech.com.tw> wrote: ... > > And the section of ADS5593 is what > > we got, and we do a little modification such as i2c bus reference > > and mode setting to fix our platform. After adding the part of ADS5593, we could successfully > > probe up ad5593r with the driver in the current upstream source code. And just to emphasize one more time, you may send us (meta-acpi, [3]) a PR against minnowboard-max the example how to enable ADS5593 on Intel Baytrail machines that anybody else can use. [3]: https://github.com/westeri/meta-acpi
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); }
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 <william.sung@advantech.com.tw> --- 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(-)