On Tue, 2018-02-27 at 00:40 +0800, John Garry wrote:
Based on the previous patches, this patch supports the
LPC host on hip06/hip07 for ACPI FW.
It is the responsibility of the LPC host driver to
enumerate the child devices, as the ACPI scan code will
not enumerate children of "indirect IO" hosts.
The ACPI table for the LPC host controller and the child
devices is in the following format:
Device (LPC0) {
Name (_HID, "HISI0191") // HiSi LPC
Name (_CRS, ResourceTemplate () {
Memory32Fixed (ReadWrite, 0xa01b0000, 0x1000)
})
}
Device (LPC0.IPMI) {
Name (_HID, "IPI0001")
Name (LORS, ResourceTemplate() {
QWordIO (
ResourceConsumer,
MinNotFixed, // _MIF
MaxNotFixed, // _MAF
PosDecode,
EntireRange,
0x0, // _GRA
0xe4, // _MIN
0x3fff, // _MAX
0x0, // _TRA
0x04, // _LEN
, ,
BTIO
)
})
Since the IO resources of the child devices need to be
translated from LPC bus addresses to logical PIO addresses,
and we shouldn't modify the resources of the devices
generated in the FW scan, a per-child MFD is created as
a substitute. The MFD IO resources will be the translated
bus addresses of the ACPI child.
Ok, this needs to be thought about a bit more.
I guess I understand what's is the problem with PNP IDs in the driver.
You probe your LPC device quite late.
One option is to move from classical probe to a event-driven model, i.e.
via registering a notifier (see acpi_lpss.c), preparing necessary stuff
at earlier stages and then register devices by their enumeration and
appearance.
Though, if I would be you I would seek a opinion from Rafael and Mika
(maybe others as well).
See also some comments below.
+#ifdef CONFIG_ACPI
+#define MFD_CHILD_NAME_PREFIX DRV_NAME"-"
+#define MFD_CHILD_NAME_LEN (ACPI_ID_LEN +
sizeof(MFD_CHILD_NAME_PREFIX))
..._PREFIX) - 1)
?
+static int hisi_lpc_acpi_xlat_io_res(struct acpi_device *adev,
+ struct acpi_device *host,
+ struct resource *res)
+{
+ unsigned long sys_port;
+ resource_size_t len = res->end - res->start;
resource_size()
+ return 0;
+}
+static int hisi_lpc_acpi_set_io_res(struct device *child,
+ struct device *hostdev,
+ const struct resource **res,
+ int *num_res)
+{
+ /*
+ * The following code segment to retrieve the resources is
common to
+ * acpi_create_platform_device(), so consider a common helper
function
+ * in future.
+ */
+ count = acpi_dev_get_resources(adev, &resource_list, NULL,
NULL);
+ if (count <= 0) {
+ dev_dbg(child, "failed to get resources\n");
+ return count ? count : -EIO;
count == 0 --> return 0;
Is it by design? (I didn't check acpi_create_platform_device() though)
+ }
+
+ resources = devm_kcalloc(hostdev, count, sizeof(*resources),
+ GFP_KERNEL);
+ if (!resources) {
+ dev_warn(hostdev, "could not allocate memory for %d
resources\n",
+ count);
+ acpi_dev_free_resource_list(&resource_list);
+ return -ENOMEM;
+ }
+ count = 0;
+ list_for_each_entry(rentry, &resource_list, node)
+ resources[count++] = *rentry->res;
+
+ acpi_dev_free_resource_list(&resource_list);
+
+ return 0;
+}
+
+/*
+ * hisi_lpc_acpi_probe - probe children for ACPI FW
+ * @hostdev: LPC host device pointer
+ *
+ * Returns 0 when successful, and a negative value for failure.
+ *
+ * Scan all child devices and create a per-device MFD with
+ * logical PIO translated IO resources.
+ */
+static int hisi_lpc_acpi_probe(struct device *hostdev)
+{
+ struct acpi_device *adev = ACPI_COMPANION(hostdev);
+ struct hisi_lpc_mfd_cell *hisi_lpc_mfd_cells;
+ struct mfd_cell *mfd_cells;
+ struct acpi_device *child;
+ int size, ret, count = 0, cell_num = 0;
+
+ list_for_each_entry(child, &adev->children, node)
+ cell_num++;
+
+ /* allocate the mfd cell and companion acpi info, one per
child */
+ size = sizeof(*mfd_cells) + sizeof(*hisi_lpc_mfd_cells);
+ mfd_cells = devm_kcalloc(hostdev, cell_num, size,
GFP_KERNEL);
+ if (!mfd_cells)
+ return -ENOMEM;
+
+ hisi_lpc_mfd_cells = (struct hisi_lpc_mfd_cell *)
+ &mfd_cells[cell_num];
One line, please.
Just noticed that calloc() memory layout is not the same how you are
using it.
+ /* Only consider the children of the host */
+ list_for_each_entry(child, &adev->children, node) {
+ struct mfd_cell *mfd_cell = &mfd_cells[count];
+ struct hisi_lpc_mfd_cell *hisi_lpc_mfd_cell =
+ &hisi_lpc_mfd_cells[count];
+ struct mfd_cell_acpi_match *acpi_match =
+ &hisi_lpc_mfd_cell-
acpi_match;+ char *name = hisi_lpc_mfd_cell[count].name;
+ char *pnpid = hisi_lpc_mfd_cell[count].pnpid;
+ struct mfd_cell_acpi_match match = {
+ .pnpid = pnpid,
+ };
+
+ snprintf(name, MFD_CHILD_NAME_LEN,
MFD_CHILD_NAME_PREFIX"%s",
+ acpi_device_hid(child));
No possibility of identical devices?
+ snprintf(pnpid, ACPI_ID_LEN, "%s",
acpi_device_hid(child));
+
+ memcpy(acpi_match, &match, sizeof(*acpi_match));
+ mfd_cell->name = name;
+ mfd_cell->acpi_match = acpi_match;
+
+ ret = hisi_lpc_acpi_set_io_rsses(&child->dev, &adev-
dev,+ &mfd_cell->resources,
+ &mfd_cell-
num_resources);+ if (ret) {
+ dev_warn(&child->dev, "set resource
fail(%d)\n", ret);
+ return ret;
+ }
+ count++;
+ }
+
+ ret = mfd_add_devices(hostdev,
PLATFORM_DEVID_NONE,
You mean it's not possible to have more than one identical device?
+ mfd_cells, cell_num, NULL, 0, NULL);
+ if (ret) {
+ dev_err(hostdev, "failed to add mfd cells (%d)\n",
ret);
+ return ret;
+ }