| Message ID | 20250402202510.432888-9-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | platform/x86: int3472: Add handshake pin support | expand |
Hi Andy, Thank you for all the reviews. On 2-Apr-25 10:56 PM, Andy Shevchenko wrote: > On Wed, Apr 2, 2025 at 11:25 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> 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 > > high, > > (note comma) ? > >> the FPGA "firmware" needs 25ms to comlpete the power-on sequence. > > complete > >> Add support for this modelling the handshake pin as a GPIO driven "dvdd" >> regulator with a 25 ms enable time. This model was chosen because: >> >> 1. Sensor chips don't have a handshake pin, so we need to abstract this >> in some way which does not require modification to the sensor drivers, >> sensor drivers using the bulk-regulator API to get avdd + vddio + dvdd >> is normal. So this will work to get the right value set to the handshake >> pin without requiring sensor driver modifications. >> >> 2. Sensors typically wait only a small time for the sensor to power-on >> after de-asserting reset. Not the 25ms the Lattice chip requires. >> Using the regulator framework's enable_time allows hiding the need for >> this delay from the sensor drivers. > > ... > >> if (ret) >> err_msg = "Failed to map regulator to sensor\n"; >> >> + break; >> + case INT3472_GPIO_TYPE_HANDSHAKE: >> + /* Setups using a handshake pin need 25 ms enable delay */ >> + ret = skl_int3472_register_regulator(int3472, gpio, >> + 25 * USEC_PER_MSEC, >> + con_id, NULL); >> + if (ret) >> + err_msg = "Failed to map regulator to sensor\n"; > > A copy and paste mistake? Yes, I know that they are both represented > as regulators, but don't we want a bit of uniqueness in the messages? I actually did this on purpose to allow the compiler to use a single string for these saving some space. The difference of which case we hit should be clear from the earlier printed (dbg) message printed above the switch-case. As for all your other remarks I agree and I'll fix them for v3. Regards, Hans
On Wed, Apr 16, 2025 at 2:06 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 2-Apr-25 10:56 PM, Andy Shevchenko wrote: > > On Wed, Apr 2, 2025 at 11:25 PM Hans de Goede <hdegoede@redhat.com> wrote: ... > >> if (ret) > >> err_msg = "Failed to map regulator to sensor\n"; > >> > >> + break; > >> + case INT3472_GPIO_TYPE_HANDSHAKE: > >> + /* Setups using a handshake pin need 25 ms enable delay */ > >> + ret = skl_int3472_register_regulator(int3472, gpio, > >> + 25 * USEC_PER_MSEC, > >> + con_id, NULL); > >> + if (ret) > >> + err_msg = "Failed to map regulator to sensor\n"; > > > > A copy and paste mistake? Yes, I know that they are both represented > > as regulators, but don't we want a bit of uniqueness in the messages? > > I actually did this on purpose to allow the compiler to use a single string > for these saving some space. The difference of which case we hit should be clear > from the earlier printed (dbg) message printed above the switch-case. I understand the idea, but if debug messages are off, how do we know that? Or err_msg here is also a debug leveled one?
On Wed, Apr 16, 2025 at 4:09 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Apr 16, 2025 at 2:06 PM Hans de Goede <hdegoede@redhat.com> wrote: > > On 2-Apr-25 10:56 PM, Andy Shevchenko wrote: > > > On Wed, Apr 2, 2025 at 11:25 PM Hans de Goede <hdegoede@redhat.com> wrote: ... > > >> if (ret) > > >> err_msg = "Failed to map regulator to sensor\n"; > > >> > > >> + break; > > >> + case INT3472_GPIO_TYPE_HANDSHAKE: > > >> + /* Setups using a handshake pin need 25 ms enable delay */ > > >> + ret = skl_int3472_register_regulator(int3472, gpio, > > >> + 25 * USEC_PER_MSEC, > > >> + con_id, NULL); > > >> + if (ret) > > >> + err_msg = "Failed to map regulator to sensor\n"; > > > > > > A copy and paste mistake? Yes, I know that they are both represented > > > as regulators, but don't we want a bit of uniqueness in the messages? > > > > I actually did this on purpose to allow the compiler to use a single string > > for these saving some space. The difference of which case we hit should be clear > > from the earlier printed (dbg) message printed above the switch-case. > > I understand the idea, but if debug messages are off, how do we know > that? Or err_msg here is also a debug leveled one? FWIW, you may use "... %s ...", regulator->name (maybe, I haven't checked) or similar, so the format string is the same.
Hi, On 16-Apr-25 3:09 PM, Andy Shevchenko wrote: > On Wed, Apr 16, 2025 at 4:09 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Wed, Apr 16, 2025 at 2:06 PM Hans de Goede <hdegoede@redhat.com> wrote: >>> On 2-Apr-25 10:56 PM, Andy Shevchenko wrote: >>>> On Wed, Apr 2, 2025 at 11:25 PM Hans de Goede <hdegoede@redhat.com> wrote: > > ... > >>>>> if (ret) >>>>> err_msg = "Failed to map regulator to sensor\n"; >>>>> >>>>> + break; >>>>> + case INT3472_GPIO_TYPE_HANDSHAKE: >>>>> + /* Setups using a handshake pin need 25 ms enable delay */ >>>>> + ret = skl_int3472_register_regulator(int3472, gpio, >>>>> + 25 * USEC_PER_MSEC, >>>>> + con_id, NULL); >>>>> + if (ret) >>>>> + err_msg = "Failed to map regulator to sensor\n"; >>>> >>>> A copy and paste mistake? Yes, I know that they are both represented >>>> as regulators, but don't we want a bit of uniqueness in the messages? >>> >>> I actually did this on purpose to allow the compiler to use a single string >>> for these saving some space. The difference of which case we hit should be clear >>> from the earlier printed (dbg) message printed above the switch-case. >> >> I understand the idea, but if debug messages are off, how do we know >> that? Or err_msg here is also a debug leveled one? > > FWIW, you may use "... %s ...", regulator->name (maybe, I haven't > checked) or similar, so the format string is the same. That is a good idea, since I've already send out v3, I'll do so for v4. But first I'll give you some time to review the unreviewed / modified patches in v3, so that v4 can hopefully be the last version. Regards, Hans
diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c index e4bb93892104..04cc52d28f16 100644 --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c @@ -224,6 +224,7 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472, } init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS; + init_data.constraints.enable_time = enable_time; init_data.consumer_supplies = regulator->supply_map; init_data.num_consumer_supplies = j; diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h index cd94346270c5..c7b8f0224320 100644 --- a/drivers/platform/x86/intel/int3472/common.h +++ b/drivers/platform/x86/intel/int3472/common.h @@ -22,6 +22,7 @@ #define INT3472_GPIO_TYPE_POWER_ENABLE 0x0b #define INT3472_GPIO_TYPE_CLK_ENABLE 0x0c #define INT3472_GPIO_TYPE_PRIVACY_LED 0x0d +#define INT3472_GPIO_TYPE_HANDSHAKE 0x12 #define INT3472_PDEV_MAX_NAME_LEN 23 #define INT3472_MAX_SENSOR_GPIOS 3 diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c index a2db4fae0e6d..4dddba4e1fde 100644 --- a/drivers/platform/x86/intel/int3472/discrete.c +++ b/drivers/platform/x86/intel/int3472/discrete.c @@ -191,6 +191,10 @@ static void int3472_get_con_id_and_polarity(struct acpi_device *adev, u8 *type, *con_id = "avdd"; *gpio_flags = GPIO_ACTIVE_HIGH; break; + case INT3472_GPIO_TYPE_HANDSHAKE: + *con_id = "dvdd"; + *gpio_flags = GPIO_ACTIVE_HIGH; + break; default: *con_id = "unknown"; *gpio_flags = GPIO_ACTIVE_HIGH; @@ -290,6 +294,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, case INT3472_GPIO_TYPE_CLK_ENABLE: case INT3472_GPIO_TYPE_PRIVACY_LED: case INT3472_GPIO_TYPE_POWER_ENABLE: + case INT3472_GPIO_TYPE_HANDSHAKE: gpio = skl_int3472_gpiod_get_from_temp_lookup(int3472, agpio, con_id, gpio_flags); if (IS_ERR(gpio)) { ret = PTR_ERR(gpio); @@ -318,6 +323,15 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, if (ret) err_msg = "Failed to map regulator to sensor\n"; + break; + case INT3472_GPIO_TYPE_HANDSHAKE: + /* Setups using a handshake pin need 25 ms enable delay */ + ret = skl_int3472_register_regulator(int3472, gpio, + 25 * USEC_PER_MSEC, + con_id, NULL); + if (ret) + err_msg = "Failed to map regulator to sensor\n"; + break; default: /* Never reached */ ret = -EINVAL;