On Thu, Jun 23, 2016 at 01:03:30AM -0700, Bin Gao wrote:
> This patch adds operation region driver for Intel BXT WhiskeyCove
> PMIC. The register mapping is done as per the BXT WC data sheet.
>
> Signed-off-by: Ajay Thomas <ajay.thomas.david.rajamanic...@intel.com>
> Signed-off-by: Felipe Balbi <felipe.ba...@linux.intel.com>
> Signed-off-by: Chandra Sekhar Anagani <chandra.sekhar.anag...@intel.com>
> Signed-off-by: Bin Gao <bin....@intel.com>
> ---
> Changes in v3:
> - Added regs_read() and regs_write() methods to the
> intel_pmic_opregion_data{} structure.
> Changs in v2:
> - Replaced module_init() with device_initcall().
> drivers/acpi/Kconfig | 6 +
> drivers/acpi/Makefile | 1 +
> drivers/acpi/pmic/intel_pmic.h | 2 +
> drivers/acpi/pmic/intel_pmic_bxtwc.c | 449
> +++++++++++++++++++++++++++++++++++
> 4 files changed, 458 insertions(+)
> create mode 100644 drivers/acpi/pmic/intel_pmic_bxtwc.c
>
> diff --git a/drivers/acpi/pmic/intel_pmic_bxtwc.c
> b/drivers/acpi/pmic/intel_pmic_bxtwc.c
> new file mode 100644
> index 0000000..51d5bcc
> --- /dev/null
> +++ b/drivers/acpi/pmic/intel_pmic_bxtwc.c
> +static int
> +intel_bxtwc_pmic_regs_read(struct regmap *regmap, u16 address,
> + unsigned int *value)
> +{
> + unsigned int data;
> +
> + if (regmap_read(regmap, address, &data))
> + return -EIO;
> +
> + *value = data;
> + return 0;
> +}
It looks like you can:
static int
intel_bxtwc_pmic_regs_read(struct regmap *regmap, u16 address,
unsigned int *value)
{
return regmap_read(regmap, address, value);
}
But then why adding a new function for no actual use? Or you really need
to return -EIO for all possible regmap_read's error return values? I
don't see such a need.
BTW, the callback functions defined in intel_pmic_opregion_data is there
due to different PMICs have different ways to do the same thing like
turn on/off a power rail, get raw temperature reading, etc. It's not
supposed to host helper functions like the two here.
> +
> +static int
> +intel_bxtwc_pmic_regs_write(struct regmap *regmap, u16 address,
> + unsigned int value)
> +{
> + if (regmap_write(regmap, address, value))
> + return -EIO;
> +
> + return 0;
> +}
Ditto.
> +static struct intel_pmic_opregion_data intel_bxtwc_pmic_opregion_data = {
> + .get_power = intel_bxtwc_pmic_get_power,
> + .update_power = intel_bxtwc_pmic_update_power,
> + .get_raw_temp = intel_bxtwc_pmic_get_raw_temp,
> + .update_aux = intel_bxtwc_pmic_update_aux,
> + .regs_read = intel_bxtwc_pmic_regs_read,
> + .regs_write = intel_bxtwc_pmic_regs_write,
As said above, please drop the two callbacks.
> + .get_policy = intel_bxtwc_pmic_get_policy,
> + .update_policy = intel_bxtwc_pmic_update_policy,
> + .power_table = power_table,
> + .power_table_count = ARRAY_SIZE(power_table),
> + .thermal_table = thermal_table,
> + .thermal_table_count = ARRAY_SIZE(thermal_table),
> +};
