Messages in this thread | | | Date | Mon, 30 May 2022 16:03:47 +0530 | Subject | Re: [PATCH V13 7/9] regulator: Add a regulator driver for the PM8008 PMIC | From | "Satya Priya Kakitapalli (Temp)" <> |
| |
On 5/28/2022 2:32 AM, Stephen Boyd wrote: > HTML mail? Please send plaintext next time. > > Quoting Satya Priya Kakitapalli (Temp) (2022-05-27 01:24:19) >> On 5/21/2022 8:26 AM, Stephen Boyd wrote: >> >> Quoting Satya Priya (2022-05-20 03:49:35) >> >> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c >> new file mode 100644 >> index 0000000..6e815c6 >> --- /dev/null >> +++ b/drivers/regulator/qcom-pm8008-regulator.c >> @@ -0,0 +1,225 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* Copyright (c) 2022, The Linux Foundation. All rights reserved. */ >> + >> +#include <linux/device.h> >> +#include <linux/kernel.h> >> +#include <linux/mfd/qcom_pm8008.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> >> >> What in of_device.h is used? >> >> >> struct of_device_id > That struct is defined in mod_devicetable.h, not of_device.h > >> >> >> +#include <linux/platform_device.h> >> +#include <linux/regmap.h> >> +#include <linux/regulator/driver.h> >> + > [...] >> >> +}; >> + >> +static int pm8008_regulator_get_voltage(struct regulator_dev *rdev) >> +{ >> + struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev); >> + >> + return pm8008_reg->voltage_selector; >> >> Can this read the hardware instead of caching the value from >> pm8008_regulator_set_voltage()? >> >> >> I can use the regmap_bulk_read like below (which was present in the earlier >> versions) > Please do > >> + } >> + >> + pm8008_reg->dev = dev; >> + >> + rc = of_property_read_string(dev->of_node, "regulator-name", &name); >> + if (rc) >> + return rc; >> + >> + /* get the required regulator data */ >> + for (i = 0; i < ARRAY_SIZE(reg_data); i++) >> + if (strstr(name, reg_data[i].name)) >> >> Why not find this via reg/address instead? It would save storing the >> regulator name in the reg_data table. >> >> >> You mean match this using base address? then we should add base address in the >> reg_data table. We will need the name to be stored in reg_data table anyway for >> the pm8008_reg->rdesc.of_match > Why? Now that this driver binds to each node individually the usage of > of_match doesn't make any sense to me. Can you set 'struct > regulator_config::dev' instead and not set of_match?
Currently we are setting regulator_config::dev as dev->parent i.e., pm8008@8, because the parent supplies are present under pm8008@8, to get the regulators mapped correctly to the parent supplies we are using dev->parent.
If we do not set of_match in regulator descriptor, regulator_of_get_init_node() would return NULL, causing init_data to be NULL during regulator_register and regulators are not getting probed. This can be resolved, if we get the init_data during pm8008_probe itself. I'll do that in the next version.
| |