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

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

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