Message ID | 20250417111337.38142-6-hdegoede@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | platform/x86: int3472: Add handshake pin support | expand |
On Thu, 17 Apr 2025, Hans de Goede wrote: > This is a preparation patch for registering multiple regulators, which > requires a different supply-name for each regulator. Make supply-name > a parameter to skl_int3472_register_regulator() and use con-id to set it > so that the existing int3472_gpio_map remapping can be used with it. > > Since supply-name is a parameter now, drop the fixed > skl_int3472_regulator_map_supplies[] array and instead add lower- and > upper-case mappings of the passed-in supply-name to the regulator. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v4: > - At static_assert(GPIO_REGULATOR_SUPPLY_MAP_COUNT == 2) since the code > assumes that > > Changes in v3: > - Add comment explaining value of 12 in GPIO_REGULATOR_NAME_LENGTH > - Some other comment / commitmsg tweaks > > Changes in v2: > - Use E2BIG instead of EOVERFLOW for too long supply-names > --- > .../x86/intel/int3472/clk_and_regulator.c | 50 ++++++++----------- > drivers/platform/x86/intel/int3472/common.h | 8 ++- > drivers/platform/x86/intel/int3472/discrete.c | 4 +- > 3 files changed, 31 insertions(+), 31 deletions(-) > > diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c > index 374a99dea7d1..b7a1abc2919c 100644 > --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c > +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c > @@ -185,42 +185,37 @@ void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472) > clk_unregister(int3472->clock.clk); > } > > -/* > - * The INT3472 device is going to be the only supplier of a regulator for > - * the sensor device. But unlike the clk framework the regulator framework > - * does not allow matching by consumer-device-name only. > - * > - * Ideally all sensor drivers would use "avdd" as supply-id. But for drivers > - * where this cannot be changed because another supply-id is already used in > - * e.g. DeviceTree files an alias for the other supply-id can be added here. > - * > - * Do not forget to update GPIO_REGULATOR_SUPPLY_MAP_COUNT when changing this. > - */ > -static const char * const skl_int3472_regulator_map_supplies[] = { > - "avdd", > - "AVDD", > -}; > - > -static_assert(ARRAY_SIZE(skl_int3472_regulator_map_supplies) == > - GPIO_REGULATOR_SUPPLY_MAP_COUNT); > - > int skl_int3472_register_regulator(struct int3472_discrete_device *int3472, > struct gpio_desc *gpio, > + const char *supply_name, > const char *second_sensor) > { > struct regulator_init_data init_data = { }; > + struct int3472_gpio_regulator *regulator; > struct regulator_config cfg = { }; > int i, j; > > - for (i = 0, j = 0; i < ARRAY_SIZE(skl_int3472_regulator_map_supplies); i++) { > - int3472->regulator.supply_map[j].supply = skl_int3472_regulator_map_supplies[i]; > - int3472->regulator.supply_map[j].dev_name = int3472->sensor_name; > + if (strlen(supply_name) >= GPIO_SUPPPLY_NAME_LENGTH) { > + dev_err(int3472->dev, "supply-name '%s' length too long\n", supply_name); > + return -E2BIG; > + } > + > + regulator = &int3472->regulator; > + string_upper(regulator->supply_name_upper, supply_name); > + > + /* The below code assume that map-count is 2 (upper- and lower-case) */ > + static_assert(GPIO_REGULATOR_SUPPLY_MAP_COUNT == 2); > + > + for (i = 0, j = 0; i < GPIO_REGULATOR_SUPPLY_MAP_COUNT; i++) { > + const char *supply = i ? regulator->supply_name_upper : supply_name; > + > + regulator->supply_map[j].supply = supply; > + regulator->supply_map[j].dev_name = int3472->sensor_name; > j++; > > if (second_sensor) { > - int3472->regulator.supply_map[j].supply = > - skl_int3472_regulator_map_supplies[i]; > - int3472->regulator.supply_map[j].dev_name = second_sensor; > + regulator->supply_map[j].supply = supply; > + regulator->supply_map[j].dev_name = second_sensor; > j++; > } > } > @@ -229,9 +224,8 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472, > init_data.consumer_supplies = int3472->regulator.supply_map; > init_data.num_consumer_supplies = j; > > - snprintf(int3472->regulator.regulator_name, > - sizeof(int3472->regulator.regulator_name), "%s-regulator", > - acpi_dev_name(int3472->adev)); > + snprintf(regulator->regulator_name, sizeof(regulator->regulator_name), "%s-%s", > + acpi_dev_name(int3472->adev), supply_name); > > int3472->regulator.rdesc = INT3472_REGULATOR( > int3472->regulator.regulator_name, > diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h > index 3728f3864a84..b750a309ee16 100644 > --- a/drivers/platform/x86/intel/int3472/common.h > +++ b/drivers/platform/x86/intel/int3472/common.h > @@ -26,7 +26,11 @@ > #define INT3472_PDEV_MAX_NAME_LEN 23 > #define INT3472_MAX_SENSOR_GPIOS 3 > > -#define GPIO_REGULATOR_NAME_LENGTH 21 > +/* E.g. "avdd\0" */ > +#define GPIO_SUPPPLY_NAME_LENGTH 5 FYI, I've fixed this typo for you while applying to the review-ilpo-next branch. -- i. > +/* 12 chars for acpi_dev_name() + "-", e.g. "ABCD1234:00-" */ > +#define GPIO_REGULATOR_NAME_LENGTH (12 + GPIO_SUPPPLY_NAME_LENGTH) > +/* lower- and upper-case mapping */ > #define GPIO_REGULATOR_SUPPLY_MAP_COUNT 2 > > #define INT3472_LED_MAX_NAME_LEN 32 > @@ -85,6 +89,7 @@ struct int3472_discrete_device { > struct int3472_gpio_regulator { > /* SUPPLY_MAP_COUNT * 2 to make room for second sensor mappings */ > struct regulator_consumer_supply supply_map[GPIO_REGULATOR_SUPPLY_MAP_COUNT * 2]; > + char supply_name_upper[GPIO_SUPPPLY_NAME_LENGTH]; > char regulator_name[GPIO_REGULATOR_NAME_LENGTH]; > struct regulator_dev *rdev; > struct regulator_desc rdesc; > @@ -129,6 +134,7 @@ void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472); > > int skl_int3472_register_regulator(struct int3472_discrete_device *int3472, > struct gpio_desc *gpio, > + const char *supply_name, > const char *second_sensor); > void skl_int3472_unregister_regulator(struct int3472_discrete_device *int3472); > > diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c > index cbf39459bdf0..f6dae82739e5 100644 > --- a/drivers/platform/x86/intel/int3472/discrete.c > +++ b/drivers/platform/x86/intel/int3472/discrete.c > @@ -188,7 +188,7 @@ static void int3472_get_con_id_and_polarity(struct acpi_device *adev, u8 *type, > *gpio_flags = GPIO_ACTIVE_HIGH; > break; > case INT3472_GPIO_TYPE_POWER_ENABLE: > - *con_id = "power-enable"; > + *con_id = "avdd"; > *gpio_flags = GPIO_ACTIVE_HIGH; > break; > default: > @@ -311,7 +311,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, > > break; > case INT3472_GPIO_TYPE_POWER_ENABLE: > - ret = skl_int3472_register_regulator(int3472, gpio, > + ret = skl_int3472_register_regulator(int3472, gpio, con_id, > int3472->quirks.avdd_second_sensor); > if (ret) > err_msg = "Failed to map regulator to sensor\n"; >
Hi, On 24-Apr-25 4:06 PM, Ilpo Järvinen wrote: > On Thu, 17 Apr 2025, Hans de Goede wrote: > >> This is a preparation patch for registering multiple regulators, which >> requires a different supply-name for each regulator. Make supply-name >> a parameter to skl_int3472_register_regulator() and use con-id to set it >> so that the existing int3472_gpio_map remapping can be used with it. >> >> Since supply-name is a parameter now, drop the fixed >> skl_int3472_regulator_map_supplies[] array and instead add lower- and >> upper-case mappings of the passed-in supply-name to the regulator. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v4: >> - At static_assert(GPIO_REGULATOR_SUPPLY_MAP_COUNT == 2) since the code >> assumes that >> >> Changes in v3: >> - Add comment explaining value of 12 in GPIO_REGULATOR_NAME_LENGTH >> - Some other comment / commitmsg tweaks >> >> Changes in v2: >> - Use E2BIG instead of EOVERFLOW for too long supply-names >> --- >> .../x86/intel/int3472/clk_and_regulator.c | 50 ++++++++----------- >> drivers/platform/x86/intel/int3472/common.h | 8 ++- >> drivers/platform/x86/intel/int3472/discrete.c | 4 +- >> 3 files changed, 31 insertions(+), 31 deletions(-) >> >> diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c >> index 374a99dea7d1..b7a1abc2919c 100644 >> --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c >> +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c >> @@ -185,42 +185,37 @@ void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472) >> clk_unregister(int3472->clock.clk); >> } >> >> -/* >> - * The INT3472 device is going to be the only supplier of a regulator for >> - * the sensor device. But unlike the clk framework the regulator framework >> - * does not allow matching by consumer-device-name only. >> - * >> - * Ideally all sensor drivers would use "avdd" as supply-id. But for drivers >> - * where this cannot be changed because another supply-id is already used in >> - * e.g. DeviceTree files an alias for the other supply-id can be added here. >> - * >> - * Do not forget to update GPIO_REGULATOR_SUPPLY_MAP_COUNT when changing this. >> - */ >> -static const char * const skl_int3472_regulator_map_supplies[] = { >> - "avdd", >> - "AVDD", >> -}; >> - >> -static_assert(ARRAY_SIZE(skl_int3472_regulator_map_supplies) == >> - GPIO_REGULATOR_SUPPLY_MAP_COUNT); >> - >> int skl_int3472_register_regulator(struct int3472_discrete_device *int3472, >> struct gpio_desc *gpio, >> + const char *supply_name, >> const char *second_sensor) >> { >> struct regulator_init_data init_data = { }; >> + struct int3472_gpio_regulator *regulator; >> struct regulator_config cfg = { }; >> int i, j; >> >> - for (i = 0, j = 0; i < ARRAY_SIZE(skl_int3472_regulator_map_supplies); i++) { >> - int3472->regulator.supply_map[j].supply = skl_int3472_regulator_map_supplies[i]; >> - int3472->regulator.supply_map[j].dev_name = int3472->sensor_name; >> + if (strlen(supply_name) >= GPIO_SUPPPLY_NAME_LENGTH) { >> + dev_err(int3472->dev, "supply-name '%s' length too long\n", supply_name); >> + return -E2BIG; >> + } >> + >> + regulator = &int3472->regulator; >> + string_upper(regulator->supply_name_upper, supply_name); >> + >> + /* The below code assume that map-count is 2 (upper- and lower-case) */ >> + static_assert(GPIO_REGULATOR_SUPPLY_MAP_COUNT == 2); >> + >> + for (i = 0, j = 0; i < GPIO_REGULATOR_SUPPLY_MAP_COUNT; i++) { >> + const char *supply = i ? regulator->supply_name_upper : supply_name; >> + >> + regulator->supply_map[j].supply = supply; >> + regulator->supply_map[j].dev_name = int3472->sensor_name; >> j++; >> >> if (second_sensor) { >> - int3472->regulator.supply_map[j].supply = >> - skl_int3472_regulator_map_supplies[i]; >> - int3472->regulator.supply_map[j].dev_name = second_sensor; >> + regulator->supply_map[j].supply = supply; >> + regulator->supply_map[j].dev_name = second_sensor; >> j++; >> } >> } >> @@ -229,9 +224,8 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472, >> init_data.consumer_supplies = int3472->regulator.supply_map; >> init_data.num_consumer_supplies = j; >> >> - snprintf(int3472->regulator.regulator_name, >> - sizeof(int3472->regulator.regulator_name), "%s-regulator", >> - acpi_dev_name(int3472->adev)); >> + snprintf(regulator->regulator_name, sizeof(regulator->regulator_name), "%s-%s", >> + acpi_dev_name(int3472->adev), supply_name); >> >> int3472->regulator.rdesc = INT3472_REGULATOR( >> int3472->regulator.regulator_name, >> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h >> index 3728f3864a84..b750a309ee16 100644 >> --- a/drivers/platform/x86/intel/int3472/common.h >> +++ b/drivers/platform/x86/intel/int3472/common.h >> @@ -26,7 +26,11 @@ >> #define INT3472_PDEV_MAX_NAME_LEN 23 >> #define INT3472_MAX_SENSOR_GPIOS 3 >> >> -#define GPIO_REGULATOR_NAME_LENGTH 21 >> +/* E.g. "avdd\0" */ >> +#define GPIO_SUPPPLY_NAME_LENGTH 5 > > FYI, I've fixed this typo for you while applying to the review-ilpo-next > branch. Thank you, I completely missed the triple PPP in there until you pointed it out. I just copy pasted the same mistake everywhere :) Regards, Hans >> +/* 12 chars for acpi_dev_name() + "-", e.g. "ABCD1234:00-" */ >> +#define GPIO_REGULATOR_NAME_LENGTH (12 + GPIO_SUPPPLY_NAME_LENGTH) >> +/* lower- and upper-case mapping */ >> #define GPIO_REGULATOR_SUPPLY_MAP_COUNT 2 >> >> #define INT3472_LED_MAX_NAME_LEN 32 >> @@ -85,6 +89,7 @@ struct int3472_discrete_device { >> struct int3472_gpio_regulator { >> /* SUPPLY_MAP_COUNT * 2 to make room for second sensor mappings */ >> struct regulator_consumer_supply supply_map[GPIO_REGULATOR_SUPPLY_MAP_COUNT * 2]; >> + char supply_name_upper[GPIO_SUPPPLY_NAME_LENGTH]; >> char regulator_name[GPIO_REGULATOR_NAME_LENGTH]; >> struct regulator_dev *rdev; >> struct regulator_desc rdesc; >> @@ -129,6 +134,7 @@ void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472); >> >> int skl_int3472_register_regulator(struct int3472_discrete_device *int3472, >> struct gpio_desc *gpio, >> + const char *supply_name, >> const char *second_sensor); >> void skl_int3472_unregister_regulator(struct int3472_discrete_device *int3472); >> >> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c >> index cbf39459bdf0..f6dae82739e5 100644 >> --- a/drivers/platform/x86/intel/int3472/discrete.c >> +++ b/drivers/platform/x86/intel/int3472/discrete.c >> @@ -188,7 +188,7 @@ static void int3472_get_con_id_and_polarity(struct acpi_device *adev, u8 *type, >> *gpio_flags = GPIO_ACTIVE_HIGH; >> break; >> case INT3472_GPIO_TYPE_POWER_ENABLE: >> - *con_id = "power-enable"; >> + *con_id = "avdd"; >> *gpio_flags = GPIO_ACTIVE_HIGH; >> break; >> default: >> @@ -311,7 +311,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, >> >> break; >> case INT3472_GPIO_TYPE_POWER_ENABLE: >> - ret = skl_int3472_register_regulator(int3472, gpio, >> + ret = skl_int3472_register_regulator(int3472, gpio, con_id, >> int3472->quirks.avdd_second_sensor); >> if (ret) >> err_msg = "Failed to map regulator to sensor\n"; >> >
diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c index 374a99dea7d1..b7a1abc2919c 100644 --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c @@ -185,42 +185,37 @@ void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472) clk_unregister(int3472->clock.clk); } -/* - * The INT3472 device is going to be the only supplier of a regulator for - * the sensor device. But unlike the clk framework the regulator framework - * does not allow matching by consumer-device-name only. - * - * Ideally all sensor drivers would use "avdd" as supply-id. But for drivers - * where this cannot be changed because another supply-id is already used in - * e.g. DeviceTree files an alias for the other supply-id can be added here. - * - * Do not forget to update GPIO_REGULATOR_SUPPLY_MAP_COUNT when changing this. - */ -static const char * const skl_int3472_regulator_map_supplies[] = { - "avdd", - "AVDD", -}; - -static_assert(ARRAY_SIZE(skl_int3472_regulator_map_supplies) == - GPIO_REGULATOR_SUPPLY_MAP_COUNT); - int skl_int3472_register_regulator(struct int3472_discrete_device *int3472, struct gpio_desc *gpio, + const char *supply_name, const char *second_sensor) { struct regulator_init_data init_data = { }; + struct int3472_gpio_regulator *regulator; struct regulator_config cfg = { }; int i, j; - for (i = 0, j = 0; i < ARRAY_SIZE(skl_int3472_regulator_map_supplies); i++) { - int3472->regulator.supply_map[j].supply = skl_int3472_regulator_map_supplies[i]; - int3472->regulator.supply_map[j].dev_name = int3472->sensor_name; + if (strlen(supply_name) >= GPIO_SUPPPLY_NAME_LENGTH) { + dev_err(int3472->dev, "supply-name '%s' length too long\n", supply_name); + return -E2BIG; + } + + regulator = &int3472->regulator; + string_upper(regulator->supply_name_upper, supply_name); + + /* The below code assume that map-count is 2 (upper- and lower-case) */ + static_assert(GPIO_REGULATOR_SUPPLY_MAP_COUNT == 2); + + for (i = 0, j = 0; i < GPIO_REGULATOR_SUPPLY_MAP_COUNT; i++) { + const char *supply = i ? regulator->supply_name_upper : supply_name; + + regulator->supply_map[j].supply = supply; + regulator->supply_map[j].dev_name = int3472->sensor_name; j++; if (second_sensor) { - int3472->regulator.supply_map[j].supply = - skl_int3472_regulator_map_supplies[i]; - int3472->regulator.supply_map[j].dev_name = second_sensor; + regulator->supply_map[j].supply = supply; + regulator->supply_map[j].dev_name = second_sensor; j++; } } @@ -229,9 +224,8 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472, init_data.consumer_supplies = int3472->regulator.supply_map; init_data.num_consumer_supplies = j; - snprintf(int3472->regulator.regulator_name, - sizeof(int3472->regulator.regulator_name), "%s-regulator", - acpi_dev_name(int3472->adev)); + snprintf(regulator->regulator_name, sizeof(regulator->regulator_name), "%s-%s", + acpi_dev_name(int3472->adev), supply_name); int3472->regulator.rdesc = INT3472_REGULATOR( int3472->regulator.regulator_name, diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h index 3728f3864a84..b750a309ee16 100644 --- a/drivers/platform/x86/intel/int3472/common.h +++ b/drivers/platform/x86/intel/int3472/common.h @@ -26,7 +26,11 @@ #define INT3472_PDEV_MAX_NAME_LEN 23 #define INT3472_MAX_SENSOR_GPIOS 3 -#define GPIO_REGULATOR_NAME_LENGTH 21 +/* E.g. "avdd\0" */ +#define GPIO_SUPPPLY_NAME_LENGTH 5 +/* 12 chars for acpi_dev_name() + "-", e.g. "ABCD1234:00-" */ +#define GPIO_REGULATOR_NAME_LENGTH (12 + GPIO_SUPPPLY_NAME_LENGTH) +/* lower- and upper-case mapping */ #define GPIO_REGULATOR_SUPPLY_MAP_COUNT 2 #define INT3472_LED_MAX_NAME_LEN 32 @@ -85,6 +89,7 @@ struct int3472_discrete_device { struct int3472_gpio_regulator { /* SUPPLY_MAP_COUNT * 2 to make room for second sensor mappings */ struct regulator_consumer_supply supply_map[GPIO_REGULATOR_SUPPLY_MAP_COUNT * 2]; + char supply_name_upper[GPIO_SUPPPLY_NAME_LENGTH]; char regulator_name[GPIO_REGULATOR_NAME_LENGTH]; struct regulator_dev *rdev; struct regulator_desc rdesc; @@ -129,6 +134,7 @@ void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472); int skl_int3472_register_regulator(struct int3472_discrete_device *int3472, struct gpio_desc *gpio, + const char *supply_name, const char *second_sensor); void skl_int3472_unregister_regulator(struct int3472_discrete_device *int3472); diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c index cbf39459bdf0..f6dae82739e5 100644 --- a/drivers/platform/x86/intel/int3472/discrete.c +++ b/drivers/platform/x86/intel/int3472/discrete.c @@ -188,7 +188,7 @@ static void int3472_get_con_id_and_polarity(struct acpi_device *adev, u8 *type, *gpio_flags = GPIO_ACTIVE_HIGH; break; case INT3472_GPIO_TYPE_POWER_ENABLE: - *con_id = "power-enable"; + *con_id = "avdd"; *gpio_flags = GPIO_ACTIVE_HIGH; break; default: @@ -311,7 +311,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, break; case INT3472_GPIO_TYPE_POWER_ENABLE: - ret = skl_int3472_register_regulator(int3472, gpio, + ret = skl_int3472_register_regulator(int3472, gpio, con_id, int3472->quirks.avdd_second_sensor); if (ret) err_msg = "Failed to map regulator to sensor\n";
This is a preparation patch for registering multiple regulators, which requires a different supply-name for each regulator. Make supply-name a parameter to skl_int3472_register_regulator() and use con-id to set it so that the existing int3472_gpio_map remapping can be used with it. Since supply-name is a parameter now, drop the fixed skl_int3472_regulator_map_supplies[] array and instead add lower- and upper-case mappings of the passed-in supply-name to the regulator. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v4: - At static_assert(GPIO_REGULATOR_SUPPLY_MAP_COUNT == 2) since the code assumes that Changes in v3: - Add comment explaining value of 12 in GPIO_REGULATOR_NAME_LENGTH - Some other comment / commitmsg tweaks Changes in v2: - Use E2BIG instead of EOVERFLOW for too long supply-names --- .../x86/intel/int3472/clk_and_regulator.c | 50 ++++++++----------- drivers/platform/x86/intel/int3472/common.h | 8 ++- drivers/platform/x86/intel/int3472/discrete.c | 4 +- 3 files changed, 31 insertions(+), 31 deletions(-)