From: Lee Jones <lee.jones@linaro.org>
To: Campion Kang <campion.kang@advantech.com.tw>
Cc: chia-lin.kao@canonical.com, devicetree@vger.kernel.org,
jdelvare@suse.com, linux-hwmon@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-watchdog@vger.kernel.org,
linux@roeck-us.net, robh+dt@kernel.org, wim@linux-watchdog.org,
Campion.Kang@gmail.com
Subject: Re: [PATCH v6 4/6] mfd: ahc1ec0: Add support for Advantech embedded controller
Date: Fri, 19 Mar 2021 14:14:43 +0000 [thread overview]
Message-ID: <20210319141443.GI2916463@dell> (raw)
In-Reply-To: <20210319100105.18278-1-campion.kang@advantech.com.tw>
On Fri, 19 Mar 2021, Campion Kang wrote:
>
> Please check [Campion] text in below as my reply.
This is a mess. Please setup your mailer to quote correctly.
> Sorry, due to the mail was rejected by vger.kernel.org as SPAM before
> so I reply the mail late and had some test email before.
>
> -----------------------------------------------------------------------------------------
> Date: Tue, 9 Mar 2021 16:07:55 +0000
> From: Lee Jones <lee.jones@linaro.org>
[...]
> > +enum {
> > + ADVEC_SUBDEV_BRIGHTNESS = 0,
> > + ADVEC_SUBDEV_EEPROM,
> > + ADVEC_SUBDEV_GPIO,
> > + ADVEC_SUBDEV_HWMON,
> > + ADVEC_SUBDEV_LED,
> > + ADVEC_SUBDEV_WDT,
> > + ADVEC_SUBDEV_MAX,
> > +};
>
> Are these arbitrary?
> [Campion] cannot arbitrary there, due to it is a index to identify its number of sub device.
I'm asking what this is dictated by.
Are these ID/index values written into H/W?
[...]
> > +int write_acpi_value(struct adv_ec_platform_data *adv_ec_data, unsigned char addr,
> > + unsigned char value)
> > +{
> > + int ret;
> > +
> > + mutex_lock(&adv_ec_data->lock);
> > +
> > + ret = ec_wait_write();
> > + if (ret)
> > + goto error;
> > + outb(EC_ACPI_DATA_WRITE, EC_COMMAND_PORT);
> > +
> > + ret = ec_wait_write();
> > + if (ret)
> > + goto error;
> > + outb(addr, EC_STATUS_PORT);
> > +
> > + ret = ec_wait_write();
> > + if (ret)
> > + goto error;
> > + outb(value, EC_STATUS_PORT);
> > +
> > + mutex_unlock(&adv_ec_data->lock);
> > + return 0;
> > +
> > +error:
> > + mutex_unlock(&adv_ec_data->lock);
> > +
> > + dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> > + __LINE__);
> > +
> > + return ret;
> > +}
>
> EXPORT?
>
> I think this API (i.e. all of the functions above) should be moved
> into drivers/platform. They really don't have a place in MFD.
>
> [Campion] this is a common function for upper HWMON and brightness control used.
> So far this API only used by HWMON, but then it will be used by
> brightness in next stage. So i put this API here, OK?
I think it belongs in drivers/platform. Take a look at some of the
other Embedded Controller code that lives there.
> > +int read_gpio_status(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> > + unsigned char *pvalue)
> > +{
> > +}
> > +
> > +int write_gpio_status(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> > + unsigned char value)
> > +{
> > +}
> > +
> > +int read_gpio_dir(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> > + unsigned char *pvalue)
> > +{
> > +}
> > +
> > +int write_gpio_dir(struct adv_ec_platform_data *adv_ec_data, unsigned char PinNumber,
> > + unsigned char value)
> > +{
> > +}
>
> All of the GPIO functions above should move into drivers/gpio.
>
> [Campion] Due to it has a flow need to cowork with EC chip and firmware, it cannot be interrupt
> by other functions. So it needs to keep in here.
Please provide more details.
> > +int write_hwram_command(struct adv_ec_platform_data *adv_ec_data, unsigned char data)
> > +{
> > + int ret;
> > +
> > + mutex_lock(&adv_ec_data->lock);
> > +
> > + ret = ec_wait_write();
> > + if (ret)
> > + goto error;
> > + outb(data, EC_COMMAND_PORT);
> > +
> > + mutex_unlock(&adv_ec_data->lock);
> > + return 0;
> > +
> > +error:
> > + mutex_unlock(&adv_ec_data->lock);
> > +
> > + dev_warn(adv_ec_data->dev, "%s: Wait for IBF or OBF too long. line: %d\n", __func__,
> > + __LINE__);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(write_hwram_command);
> > +
> > +static int adv_ec_get_productname(struct adv_ec_platform_data *adv_ec_data, char *product)
> > +{
> > + const char *vendor, *device;
> > + int length = 0;
> > +
> > + /* Check it is Advantech board */
> > + vendor = dmi_get_system_info(DMI_SYS_VENDOR);
> > + if (memcmp(vendor, "Advantech", sizeof("Advantech")) != 0)
> > + return -ENODEV;
> > +
> > + /* Get product model name */
> > + device = dmi_get_system_info(DMI_PRODUCT_NAME);
> > + if (device) {
> > + while ((device[length] != ' ')
> > + && (length < AMI_ADVANTECH_BOARD_ID_LENGTH))
> > + length++;
> > + memset(product, 0, AMI_ADVANTECH_BOARD_ID_LENGTH);
> > + memmove(product, device, length);
> > +
> > + dev_info(adv_ec_data->dev, "BIOS Product Name = %s\n", product);
> > +
> > + return 0;
> > + }
> > +
> > + dev_warn(adv_ec_data->dev, "This device is not Advantech Board (%s)!\n", product);
> > +
> > + return -ENODEV;
> > +}
>
> These should go into drivers/platform.
>
> [Campion] This is a simple function to get module name from BIOS DMI table, it is not need to
> access EC chip. But it can get once and other drivers can get this information,
> donot call DMI every time. Can it keep in here?
I thought this driver was for the EC chip.
> > +static const struct mfd_cell adv_ec_sub_cells[] = {
> > + { .name = "adv-ec-brightness", },
> > + { .name = "adv-ec-eeprom", },
> > + { .name = "adv-ec-gpio", },
> > + { .name = "ahc1ec0-hwmon", },
> > + { .name = "adv-ec-led", },
> > + { .name = "ahc1ec0-wdt", },
> > +};
> > +
> > +static int adv_ec_init_ec_data(struct adv_ec_platform_data *adv_ec_data)
> > +{
> > + int ret;
> > +
> > + adv_ec_data->sub_dev_mask = 0;
> > + adv_ec_data->sub_dev_nb = 0;
> > + adv_ec_data->dym_tbl = NULL;
> > + adv_ec_data->bios_product_name = NULL;
>
> Why are pre-initialising these?
>
> [Campion] Just make sure they have empty pointer, but I will remove it.
There's no need to do that if they are being allocated below.
> > + mutex_init(&adv_ec_data->lock);
> > +
> > + /* Get product name */
> > + adv_ec_data->bios_product_name =
> > + devm_kzalloc(adv_ec_data->dev, AMI_ADVANTECH_BOARD_ID_LENGTH, GFP_KERNEL);
> > + if (!adv_ec_data->bios_product_name)
> > + return -ENOMEM;
> > +
> > + memset(adv_ec_data->bios_product_name, 0, AMI_ADVANTECH_BOARD_ID_LENGTH);
>
> Why are you doing this?
>
> [Campion] Just make sure the memory is null all
devm_kzalloc() does that for you - that's what the 'z' means.
> > + ret = adv_ec_get_productname(adv_ec_data, adv_ec_data->bios_product_name);
> > + if (ret)
> > + return ret;
> > +
> > + /* Get pin table */
> > + adv_ec_data->dym_tbl = devm_kzalloc(adv_ec_data->dev,
> > + EC_MAX_TBL_NUM * sizeof(struct ec_dynamic_table),
> > + GFP_KERNEL);
> > + if (!adv_ec_data->dym_tbl)
> > + return -ENOMEM;
>
> What does a dynamic table do?
>
> [Campion] The dynamic table is reterived from EC firmware according to different platform HW device.
> it will based on dynamic table information to get HW pin definition based on its function.
> The pin value will retrive to calcute the value, for example, voltage value, vcore value.
>
>
> > + ret = adv_get_dynamic_tab(adv_ec_data);
>
> return adv_get_dynamic_tab();
>
> [Campion] OK
>
> > + return ret;
> > +}
> > +
> > +static int adv_ec_parse_prop(struct adv_ec_platform_data *adv_ec_data)
> > +{
> > + int i, ret;
> > + u32 nb, sub_dev[ADVEC_SUBDEV_MAX];
> > +
> > + ret = device_property_read_u32(adv_ec_data->dev, "advantech,sub-dev-nb", &nb);
>
> Indexing devices is generally not a good strategy.
>
> ---------------------------------------------------------------------------
> [Campion] Yes, I will remove it, just use the following that defined in ahc1ec0.yaml.
> I ever feedback related mail for "https://lore.kernel.org/linux-watchdog/20210118123749.4769-6-campion.kang@advantech.com.tw/t/#m5126adbc2453e3ab3e6bda717c257fab364b9f45".
> But vger.kernel.org returned the mail to mail as spam mail.
> I will modity it as the following, is it OK?
> examples:
> - |
> #include <dt-bindings/mfd/ahc1ec0-dt.h>
> ahc1ec0 {
> compatible = "advantech,ahc1ec0";
>
> advantech,hwmon-profile = <AHC1EC0_HWMON_PRO_UNO2271G>;
> advantech,watchdog = true;
Shouldn't the watchdog be it's own sub-node?
Is that functionality not probably at all?
Surely it has it's own register set?
[...]
> > + /* check whether this EC has the following subdevices. */
> > + for (i = 0; i < ARRAY_SIZE(adv_ec_sub_cells); i++) {
> > + if (adv_ec_data->sub_dev_mask & BIT(i)) {
> > + ret = mfd_add_hotplug_devices(dev, &adv_ec_sub_cells[i], 1);
>
> Why have you chosen to use hotplug here?
>
> [Campion] There is a information in BIOS ACPI table according to different device to decide
> which function drivers need to be probe. May be a device has HWMON, it will dynamic
> to load HWMON driver, but other device may not.
The only thing hotplug does is hard-code the platform ID.
It's more of a win if you can omit the mfd_remove_devices() call.
> > + dev_info(adv_ec_data->dev, "mfd_add_hotplug_devices[%d] %s\n", i,
> > + adv_ec_sub_cells[i].name);
> > + if (ret)
> > + dev_err(dev, "failed to add %s subdevice: %d\n",
> > + adv_ec_sub_cells[i].name, ret);
> > + }
> > + }
>
> This is a mess!
>
> Where are you pulling these devices from?
>
> [Campion] decide which drivers need to mount from BIOS ACPI table. This drive would built in Linux Kernel.
> I am not sure what's your meaning, can you describe more? Thank you
I really don't like the sub_dev_mask idea.
Are the ACPI tables available?
[...]
> > +struct adv_ec_platform_data {
> > + char *bios_product_name;
>
> Where is this used?
>
> [Campion] Get the module name once and upper application can get it by EC driver.
From DMI? What do you use it for? Debug prints or something else?
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
next prev parent reply other threads:[~2021-03-19 14:15 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-18 12:37 [PATCH v6 1/6] MAINTAINERS: Add Advantech AHC1EC0 embedded controller entry Campion Kang
2021-01-18 12:37 ` [PATCH v6 2/6] mfd: ahc1ec0: Add Advantech EC include file used by dt-bindings Campion Kang
2021-02-04 10:32 ` Lee Jones
2021-01-18 12:37 ` [PATCH v6 3/6] dt-bindings: mfd: ahc1ec0.yaml: Add Advantech embedded controller - AHC1EC0 Campion Kang
2021-02-05 21:21 ` Rob Herring
2021-01-18 12:37 ` [PATCH v6 4/6] mfd: ahc1ec0: Add support for Advantech embedded controller Campion Kang
2021-01-19 7:25 ` AceLan Kao
2021-01-19 8:23 ` Lee Jones
2021-03-09 16:07 ` Lee Jones
[not found] ` <22025d6fb74e49a1835f89cfa0849990@Taipei11.ADVANTECH.CORP>
2021-03-12 7:14 ` FW: " Campion Kang
2021-03-19 10:01 ` Campion Kang
2021-03-19 14:14 ` Lee Jones [this message]
[not found] ` <f47a66c80d8b4cf6b9c28a4519a10521@Taipei11.ADVANTECH.CORP>
2021-03-24 8:44 ` Lee Jones
2021-01-18 12:37 ` [PATCH v6 5/6] hwmon: ahc1ec0-hwmon: Add sub-device hwmon " Campion Kang
2021-01-19 7:26 ` AceLan Kao
2021-01-23 16:35 ` Guenter Roeck
[not found] ` <66338d379bb14c2fbd8a6507075384ce@Taipei11.ADVANTECH.CORP>
2021-01-28 15:13 ` Guenter Roeck
2021-01-18 12:37 ` [PATCH v6 6/6] watchdog: ahc1ec0-wdt: Add sub-device watchdog " Campion Kang
2021-01-19 7:26 ` AceLan Kao
2021-01-23 17:00 ` Guenter Roeck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210319141443.GI2916463@dell \
--to=lee.jones@linaro.org \
--cc=Campion.Kang@gmail.com \
--cc=campion.kang@advantech.com.tw \
--cc=chia-lin.kao@canonical.com \
--cc=devicetree@vger.kernel.org \
--cc=jdelvare@suse.com \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=robh+dt@kernel.org \
--cc=wim@linux-watchdog.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).