Re: [PATCH v2 8/9] platform/x86: int3472: Add handshake pin support

[Hans de Goede <hdegoede@redhat.com>]

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