| Message ID | 20211102094907.31271-11-hdegoede@redhat.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add support for X86/ACPI camera sensor/PMIC setup with clk and regulator platform data | expand |
On Tue, Nov 2, 2021 at 11:50 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Pass tps68470_regulator_platform_data to the tps68470-regulator > MFD-cell, specifying the voltages of the various regulators and > tying the regulators to the sensor supplies so that sensors which use > the TPS68470 can find their regulators. > > Since the voltages and supply connections are board-specific, this > introduces a DMI matches int3472_tps68470_board_data struct which > contains the necessary per-board info. > > This per-board info also includes GPIO lookup information for the > sensor IO lines which may be connected to the tps68470 GPIOs. ... > + board_data = int3472_tps68470_get_board_data(dev_name(&client->dev)); > + if (!board_data) { > + dev_err(&client->dev, "No board-data found for this laptop/tablet model\n"); > + return -ENODEV; It's fine to use dev_err_probe() for known error codes. > + } ... > + cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata; Do we need casting? ... > +#include <linux/dmi.h> > +#include <linux/gpio/machine.h> > +#include <linux/platform_data/tps68470.h> > +#include <linux/regulator/machine.h> string.h for strcmp() ? kernel.h for ARRAY_SIZE() ?
Hi, On 11/2/21 15:34, Andy Shevchenko wrote: > On Tue, Nov 2, 2021 at 11:50 AM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Pass tps68470_regulator_platform_data to the tps68470-regulator >> MFD-cell, specifying the voltages of the various regulators and >> tying the regulators to the sensor supplies so that sensors which use >> the TPS68470 can find their regulators. >> >> Since the voltages and supply connections are board-specific, this >> introduces a DMI matches int3472_tps68470_board_data struct which >> contains the necessary per-board info. >> >> This per-board info also includes GPIO lookup information for the >> sensor IO lines which may be connected to the tps68470 GPIOs. > > ... > >> + board_data = int3472_tps68470_get_board_data(dev_name(&client->dev)); >> + if (!board_data) { >> + dev_err(&client->dev, "No board-data found for this laptop/tablet model\n"); >> + return -ENODEV; > > It's fine to use dev_err_probe() for known error codes. > >> + } > > ... > >> + cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata; > > Do we need casting? Yes, the cast casts away a "const", the const is correct since the data only ever gets read by the regulator driver, but platform_data pointers are normally not const, so it is either the cast, or loose the const on the definition of the struct to which board_data->tps68470_regulator_pdata points... So not good choice here really, only chosing between bad options and I picked the lets do the cast "least worse" option (at least to me). I'm open to changing this. > ... > >> +#include <linux/dmi.h> >> +#include <linux/gpio/machine.h> >> +#include <linux/platform_data/tps68470.h> >> +#include <linux/regulator/machine.h> > > string.h for strcmp() ? > kernel.h for ARRAY_SIZE() ? Ack. Regards, Hans
On Tue, Nov 2, 2021 at 4:59 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 11/2/21 15:34, Andy Shevchenko wrote: > > On Tue, Nov 2, 2021 at 11:50 AM Hans de Goede <hdegoede@redhat.com> wrote: ... > >> + board_data = int3472_tps68470_get_board_data(dev_name(&client->dev)); > >> + if (!board_data) { > >> + dev_err(&client->dev, "No board-data found for this laptop/tablet model\n"); > >> + return -ENODEV; > > > > It's fine to use dev_err_probe() for known error codes. > > > >> + } > > > > ... > > > >> + cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata; > > > > Do we need casting? > > Yes, the cast casts away a "const", the const is correct > since the data only ever gets read by the regulator driver, > but platform_data pointers are normally not const, so it > is either the cast, or loose the const on the definition > of the struct to which board_data->tps68470_regulator_pdata > points... > > So not good choice here really, only chosing between bad > options and I picked the lets do the cast "least worse" > option (at least to me). I'm open to changing this. Hmm... Okay, I was under the impression that MFD is using const for that field...
Hi, On 11/2/21 16:52, Andy Shevchenko wrote: > On Tue, Nov 2, 2021 at 4:59 PM Hans de Goede <hdegoede@redhat.com> wrote: >> On 11/2/21 15:34, Andy Shevchenko wrote: >>> On Tue, Nov 2, 2021 at 11:50 AM Hans de Goede <hdegoede@redhat.com> wrote: > > ... > >>>> + board_data = int3472_tps68470_get_board_data(dev_name(&client->dev)); >>>> + if (!board_data) { >>>> + dev_err(&client->dev, "No board-data found for this laptop/tablet model\n"); >>>> + return -ENODEV; >>> >>> It's fine to use dev_err_probe() for known error codes. >>> >>>> + } >>> >>> ... >>> >>>> + cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata; >>> >>> Do we need casting? >> >> Yes, the cast casts away a "const", the const is correct >> since the data only ever gets read by the regulator driver, >> but platform_data pointers are normally not const, so it >> is either the cast, or loose the const on the definition >> of the struct to which board_data->tps68470_regulator_pdata >> points... >> >> So not good choice here really, only chosing between bad >> options and I picked the lets do the cast "least worse" >> option (at least to me). I'm open to changing this. > > Hmm... Okay, I was under the impression that MFD is using const for > that field... Nope, I just double-checked and it is a plain "void *" Regards, Hans
On Tue, Nov 02, 2021 at 03:59:41PM +0100, Hans de Goede wrote: > Hi, > > On 11/2/21 15:34, Andy Shevchenko wrote: > > On Tue, Nov 2, 2021 at 11:50 AM Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> Pass tps68470_regulator_platform_data to the tps68470-regulator > >> MFD-cell, specifying the voltages of the various regulators and > >> tying the regulators to the sensor supplies so that sensors which use > >> the TPS68470 can find their regulators. > >> > >> Since the voltages and supply connections are board-specific, this > >> introduces a DMI matches int3472_tps68470_board_data struct which > >> contains the necessary per-board info. > >> > >> This per-board info also includes GPIO lookup information for the > >> sensor IO lines which may be connected to the tps68470 GPIOs. > > > > ... > > > >> + board_data = int3472_tps68470_get_board_data(dev_name(&client->dev)); > >> + if (!board_data) { > >> + dev_err(&client->dev, "No board-data found for this laptop/tablet model\n"); > >> + return -ENODEV; > > > > It's fine to use dev_err_probe() for known error codes. > > > >> + } > > > > ... > > > >> + cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata; > > > > Do we need casting? > > Yes, the cast casts away a "const", the const is correct > since the data only ever gets read by the regulator driver, > but platform_data pointers are normally not const, so it > is either the cast, or loose the const on the definition > of the struct to which board_data->tps68470_regulator_pdata > points... > > So not good choice here really, only chosing between bad > options and I picked the lets do the cast "least worse" > option (at least to me). I'm open to changing this. Maybe a comment explaining this briefly?
Hi, On 11/2/21 17:17, Sakari Ailus wrote: > On Tue, Nov 02, 2021 at 03:59:41PM +0100, Hans de Goede wrote: >> Hi, >> >> On 11/2/21 15:34, Andy Shevchenko wrote: >>> On Tue, Nov 2, 2021 at 11:50 AM Hans de Goede <hdegoede@redhat.com> wrote: >>>> >>>> Pass tps68470_regulator_platform_data to the tps68470-regulator >>>> MFD-cell, specifying the voltages of the various regulators and >>>> tying the regulators to the sensor supplies so that sensors which use >>>> the TPS68470 can find their regulators. >>>> >>>> Since the voltages and supply connections are board-specific, this >>>> introduces a DMI matches int3472_tps68470_board_data struct which >>>> contains the necessary per-board info. >>>> >>>> This per-board info also includes GPIO lookup information for the >>>> sensor IO lines which may be connected to the tps68470 GPIOs. >>> >>> ... >>> >>>> + board_data = int3472_tps68470_get_board_data(dev_name(&client->dev)); >>>> + if (!board_data) { >>>> + dev_err(&client->dev, "No board-data found for this laptop/tablet model\n"); >>>> + return -ENODEV; >>> >>> It's fine to use dev_err_probe() for known error codes. >>> >>>> + } >>> >>> ... >>> >>>> + cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata; >>> >>> Do we need casting? >> >> Yes, the cast casts away a "const", the const is correct >> since the data only ever gets read by the regulator driver, >> but platform_data pointers are normally not const, so it >> is either the cast, or loose the const on the definition >> of the struct to which board_data->tps68470_regulator_pdata >> points... >> >> So not good choice here really, only chosing between bad >> options and I picked the lets do the cast "least worse" >> option (at least to me). I'm open to changing this. > > Maybe a comment explaining this briefly? Yes, I was thinking the same myself, I'll add this for the next version (which I expect to be the final version). Regards, Hans
diff --git a/drivers/platform/x86/intel/int3472/Makefile b/drivers/platform/x86/intel/int3472/Makefile index 771e720528a0..cfec7784c5c9 100644 --- a/drivers/platform/x86/intel/int3472/Makefile +++ b/drivers/platform/x86/intel/int3472/Makefile @@ -1,4 +1,4 @@ obj-$(CONFIG_INTEL_SKL_INT3472) += intel_skl_int3472_discrete.o \ intel_skl_int3472_tps68470.o intel_skl_int3472_discrete-y := discrete.o clk_and_regulator.o common.o -intel_skl_int3472_tps68470-y := tps68470.o common.o +intel_skl_int3472_tps68470-y := tps68470.o tps68470_board_data.o common.o diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c index 05fcf35c1662..49eea7bb98c1 100644 --- a/drivers/platform/x86/intel/int3472/tps68470.c +++ b/drivers/platform/x86/intel/int3472/tps68470.c @@ -9,6 +9,7 @@ #include <linux/regmap.h> #include "common.h" +#include "tps68470.h" #define DESIGNED_FOR_CHROMEOS 1 #define DESIGNED_FOR_WINDOWS 2 @@ -102,6 +103,7 @@ static int skl_int3472_tps68470_calc_type(struct acpi_device *adev) static int skl_int3472_tps68470_probe(struct i2c_client *client) { struct acpi_device *adev = ACPI_COMPANION(&client->dev); + const struct int3472_tps68470_board_data *board_data; struct tps68470_clk_platform_data clk_pdata = {}; struct mfd_cell *cells; struct regmap *regmap; @@ -130,6 +132,12 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client) device_type = skl_int3472_tps68470_calc_type(adev); switch (device_type) { case DESIGNED_FOR_WINDOWS: + board_data = int3472_tps68470_get_board_data(dev_name(&client->dev)); + if (!board_data) { + dev_err(&client->dev, "No board-data found for this laptop/tablet model\n"); + return -ENODEV; + } + cells = kcalloc(TPS68470_WIN_MFD_CELL_COUNT, sizeof(*cells), GFP_KERNEL); if (!cells) return -ENOMEM; @@ -144,12 +152,20 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client) cells[0].platform_data = &clk_pdata; cells[0].pdata_size = sizeof(clk_pdata); cells[1].name = "tps68470-regulator"; + cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata; + cells[1].pdata_size = sizeof(struct tps68470_regulator_platform_data); cells[2].name = "tps68470-gpio"; + gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_table); + ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE, cells, TPS68470_WIN_MFD_CELL_COUNT, NULL, 0, NULL); kfree(cells); + + if (ret) + gpiod_remove_lookup_table(board_data->tps68470_gpio_lookup_table); + break; case DESIGNED_FOR_CHROMEOS: ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE, @@ -164,6 +180,17 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client) return ret; } +static int skl_int3472_tps68470_remove(struct i2c_client *client) +{ + const struct int3472_tps68470_board_data *board_data; + + board_data = int3472_tps68470_get_board_data(dev_name(&client->dev)); + if (board_data) + gpiod_remove_lookup_table(board_data->tps68470_gpio_lookup_table); + + return 0; +} + static const struct acpi_device_id int3472_device_id[] = { { "INT3472", 0 }, { } @@ -176,6 +203,7 @@ static struct i2c_driver int3472_tps68470 = { .acpi_match_table = int3472_device_id, }, .probe_new = skl_int3472_tps68470_probe, + .remove = skl_int3472_tps68470_remove, }; module_i2c_driver(int3472_tps68470); diff --git a/drivers/platform/x86/intel/int3472/tps68470.h b/drivers/platform/x86/intel/int3472/tps68470.h new file mode 100644 index 000000000000..cfd33eb62740 --- /dev/null +++ b/drivers/platform/x86/intel/int3472/tps68470.h @@ -0,0 +1,25 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * TI TPS68470 PMIC platform data definition. + * + * Copyright (c) 2021 Red Hat Inc. + * + * Red Hat authors: + * Hans de Goede <hdegoede@redhat.com> + */ + +#ifndef _INTEL_SKL_INT3472_TPS68470_H +#define _INTEL_SKL_INT3472_TPS68470_H + +struct gpiod_lookup_table; +struct tps68470_regulator_platform_data; + +struct int3472_tps68470_board_data { + const char *dev_name; + struct gpiod_lookup_table *tps68470_gpio_lookup_table; + const struct tps68470_regulator_platform_data *tps68470_regulator_pdata; +}; + +const struct int3472_tps68470_board_data *int3472_tps68470_get_board_data(const char *dev_name); + +#endif diff --git a/drivers/platform/x86/intel/int3472/tps68470_board_data.c b/drivers/platform/x86/intel/int3472/tps68470_board_data.c new file mode 100644 index 000000000000..20615c342875 --- /dev/null +++ b/drivers/platform/x86/intel/int3472/tps68470_board_data.c @@ -0,0 +1,134 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * TI TPS68470 PMIC platform data definition. + * + * Copyright (c) 2021 Dan Scally <djrscally@gmail.com> + * Copyright (c) 2021 Red Hat Inc. + * + * Red Hat authors: + * Hans de Goede <hdegoede@redhat.com> + */ + +#include <linux/dmi.h> +#include <linux/gpio/machine.h> +#include <linux/platform_data/tps68470.h> +#include <linux/regulator/machine.h> +#include "tps68470.h" + +static struct regulator_consumer_supply int347a_core_consumer_supplies[] = { + REGULATOR_SUPPLY("dvdd", "i2c-INT347A:00"), +}; + +static struct regulator_consumer_supply int347a_ana_consumer_supplies[] = { + REGULATOR_SUPPLY("avdd", "i2c-INT347A:00"), +}; + +static struct regulator_consumer_supply int347a_vcm_consumer_supplies[] = { + REGULATOR_SUPPLY("vdd", "i2c-INT347A:00-VCM"), +}; + +static struct regulator_consumer_supply int347a_vsio_consumer_supplies[] = { + REGULATOR_SUPPLY("dovdd", "i2c-INT347A:00"), +}; + +static const struct regulator_init_data surface_go_tps68470_core_reg_init_data = { + .constraints = { + .min_uV = 1200000, + .max_uV = 1200000, + .apply_uV = 1, + .valid_ops_mask = REGULATOR_CHANGE_STATUS, + }, + .num_consumer_supplies = ARRAY_SIZE(int347a_core_consumer_supplies), + .consumer_supplies = int347a_core_consumer_supplies, +}; + +static const struct regulator_init_data surface_go_tps68470_ana_reg_init_data = { + .constraints = { + .min_uV = 2815200, + .max_uV = 2815200, + .apply_uV = 1, + .valid_ops_mask = REGULATOR_CHANGE_STATUS, + }, + .num_consumer_supplies = ARRAY_SIZE(int347a_ana_consumer_supplies), + .consumer_supplies = int347a_ana_consumer_supplies, +}; + +static const struct regulator_init_data surface_go_tps68470_vcm_reg_init_data = { + .constraints = { + .min_uV = 2815200, + .max_uV = 2815200, + .apply_uV = 1, + .valid_ops_mask = REGULATOR_CHANGE_STATUS, + }, + .num_consumer_supplies = ARRAY_SIZE(int347a_vcm_consumer_supplies), + .consumer_supplies = int347a_vcm_consumer_supplies, +}; + +static const struct regulator_init_data surface_go_tps68470_vsio_reg_init_data = { + .constraints = { + .min_uV = 1800600, + .max_uV = 1800600, + .apply_uV = 1, + .valid_ops_mask = REGULATOR_CHANGE_STATUS, + }, + .num_consumer_supplies = ARRAY_SIZE(int347a_vsio_consumer_supplies), + .consumer_supplies = int347a_vsio_consumer_supplies, +}; + +static const struct tps68470_regulator_platform_data surface_go_tps68470_pdata = { + .reg_init_data = { + [TPS68470_CORE] = &surface_go_tps68470_core_reg_init_data, + [TPS68470_ANA] = &surface_go_tps68470_ana_reg_init_data, + [TPS68470_VCM] = &surface_go_tps68470_vcm_reg_init_data, + [TPS68470_VSIO] = &surface_go_tps68470_vsio_reg_init_data, + }, +}; + +static struct gpiod_lookup_table surface_go_tps68470_gpios = { + .dev_id = "i2c-INT347A:00", + .table = { + GPIO_LOOKUP("tps68470-gpio", 9, "reset", GPIO_ACTIVE_LOW), + GPIO_LOOKUP("tps68470-gpio", 7, "powerdown", GPIO_ACTIVE_LOW) + } +}; + +static const struct int3472_tps68470_board_data surface_go_tps68470_board_data = { + .dev_name = "i2c-INT3472:05", + .tps68470_gpio_lookup_table = &surface_go_tps68470_gpios, + .tps68470_regulator_pdata = &surface_go_tps68470_pdata, +}; + +static const struct dmi_system_id int3472_tps68470_board_data_table[] = { + { + .matches = { + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"), + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Go"), + }, + .driver_data = (void *)&surface_go_tps68470_board_data, + }, + { + .matches = { + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"), + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Go 2"), + }, + .driver_data = (void *)&surface_go_tps68470_board_data, + }, + { } +}; + +const struct int3472_tps68470_board_data *int3472_tps68470_get_board_data(const char *dev_name) +{ + const struct int3472_tps68470_board_data *board_data; + const struct dmi_system_id *match; + + match = dmi_first_match(int3472_tps68470_board_data_table); + while (match) { + board_data = match->driver_data; + if (strcmp(board_data->dev_name, dev_name) == 0) + return board_data; + + dmi_first_match(++match); + } + + return NULL; +}
Pass tps68470_regulator_platform_data to the tps68470-regulator MFD-cell, specifying the voltages of the various regulators and tying the regulators to the sensor supplies so that sensors which use the TPS68470 can find their regulators. Since the voltages and supply connections are board-specific, this introduces a DMI matches int3472_tps68470_board_data struct which contains the necessary per-board info. This per-board info also includes GPIO lookup information for the sensor IO lines which may be connected to the tps68470 GPIOs. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v5: - Also pass regulator_init_data for the regulator used for the VCM (Voice Coil Motor) used for the focus of the back-camera lens --- drivers/platform/x86/intel/int3472/Makefile | 2 +- drivers/platform/x86/intel/int3472/tps68470.c | 28 ++++ drivers/platform/x86/intel/int3472/tps68470.h | 25 ++++ .../x86/intel/int3472/tps68470_board_data.c | 134 ++++++++++++++++++ 4 files changed, 188 insertions(+), 1 deletion(-) create mode 100644 drivers/platform/x86/intel/int3472/tps68470.h create mode 100644 drivers/platform/x86/intel/int3472/tps68470_board_data.c