| Message ID | 20250416124037.90508-1-hdegoede@redhat.com |
|---|---|
| Headers | show |
| Series | platform/x86: int3472: Add handshake pin support | expand |
On Wed, Apr 16, 2025 at 02:40:33PM +0200, 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. ... > + for (i = 0, j = 0; i < GPIO_REGULATOR_SUPPLY_MAP_COUNT; i++) { > + const char *supply = i ? regulator->supply_name_upper : supply_name; But this won't scale, it seems it relies on the fact that GPIO_REGULATOR_SUPPLY_MAP_COUNT <= 2. > + 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++; > } With that in mind, why not unroll the loop? > } ... > +/* lower- and upper-case mapping */ > #define GPIO_REGULATOR_SUPPLY_MAP_COUNT 2 Code seems really relies on this not be bigger than 2, perhaps static assert?
Hi Andy, On 16-Apr-25 8:13 PM, Andy Shevchenko wrote: > On Wed, Apr 16, 2025 at 02:40:33PM +0200, 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. > > ... > >> + for (i = 0, j = 0; i < GPIO_REGULATOR_SUPPLY_MAP_COUNT; i++) { >> + const char *supply = i ? regulator->supply_name_upper : supply_name; > > But this won't scale, it seems it relies on the fact that > GPIO_REGULATOR_SUPPLY_MAP_COUNT <= 2. Ack, I've added a static_assert(GPIO_REGULATOR_SUPPLY_MAP_COUNT == 2) just above the for () {} for this for v4, as you suggest below. > >> + 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++; >> } > > With that in mind, why not unroll the loop? That will lead to code duplication wrt the second_sensor handler and it will make the diff from the previous code bigger, so I've gone with a static_assert() instead, as suggested below. > >> } > > ... > >> +/* lower- and upper-case mapping */ >> #define GPIO_REGULATOR_SUPPLY_MAP_COUNT 2 > > Code seems really relies on this not be bigger than 2, perhaps static assert? Ack, I've added a static assert for this for v4. Regards, Hans
Hi All, New Intel Meteor Lake based laptops with IPU6 cameras have a new type 0x12 pin defined in the INT3472 sensor companion device which describes the sensor's GPIOs. This pin is primarily used on designs with a Lattice FPGA chip which is capable of running the sensor independently of the main CPU for features like presence detection. This pin needs to be driven high to make the FPGA run the power-on sequence of the sensor. After driving the pin high the FPGA "firmware" needs 25ms to comlpete the power-on sequence. This series implements support for this by modelling the handshake GPIO as a GPIO driven 'dvdd' regulator with an enable-time of 25 ms, also see: https://lore.kernel.org/platform-driver-x86/59f672c3-6d87-4ec7-9b7f-f44fe2cce934@redhat.com/ Patch 1 Is an unrelated cleanup which I had lying around Patches 2-8 Prepare + Implement the handshake GPIO Patch 9 Is a small patch adding some extra debugging to GPIO remapping Changes in v3: - Add Andy's Reviewed-by to a few more patches - Some comment & commit-message tweaks - Add comment explaining value of 12 in GPIO_REGULATOR_SUPPLY_MAP_COUNT - Add a comment to int3472/common.h explaining where the 2 ms comes from - s/n_regulators/n_regulator_gpios/ Changes in v2: - Add Andy's Reviewed-by to patches 1-3 - Address Andy's review remarks on patch 5 - Add 2 Tested-by tags to patch 8/9 This series applies on top of Torvald's latest master, for testing with 6.14 this patch needs to be cherry-picked first: https://web.git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?id=81b251c66bdfe263fb5e7a16838512ddaeed77df Regards, Hans Hans de Goede (9): platform/x86: int3472: Add skl_int3472_register_clock() helper platform/x86: int3472: Stop setting a supply-name for GPIO regulators platform/x86: int3472: Drop unused gpio field from struct int3472_gpio_regulator platform/x86: int3472: Rework AVDD second sensor quirk handling platform/x86: int3472: Make regulator supply name configurable platform/x86: int3472: Avoid GPIO regulator spikes platform/x86: int3472: Prepare for registering more than 1 GPIO regulator platform/x86: int3472: Add handshake pin support platform/x86: int3472: Debug log when remapping pins drivers/platform/x86/intel/int3472/Makefile | 3 +- .../x86/intel/int3472/clk_and_regulator.c | 164 ++++++------------ drivers/platform/x86/intel/int3472/common.h | 57 ++++-- drivers/platform/x86/intel/int3472/discrete.c | 39 ++++- .../x86/intel/int3472/discrete_quirks.c | 22 +++ 5 files changed, 154 insertions(+), 131 deletions(-) create mode 100644 drivers/platform/x86/intel/int3472/discrete_quirks.c