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@xxxxxxxxxx>
[...]
> > +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@xxxxxxxxxxxxxxxx/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
