Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Message-ID: <019c84e6-2e77-4b76-b105-fc9ff678c058@kernel.org>
Date:   Fri, 1 Dec 2023 12:57:07 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] USB: typec: tps6598x: use device 'type' field to identify
 devices
Content-Language: en-US
To:     Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc:     Alexandru Ardelean <alex@shruggie.ro>,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
        gregkh@linuxfoundation.org, christophe.jaillet@wanadoo.fr,
        a-govindraju@ti.com, trix@redhat.com, abdelalkuor@geotab.com,
        Bryan O'Donoghue <bryan.odonoghue@linaro.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Rob Herring <robh+dt@kernel.org>
References: <20231123210021.463122-1-alex@shruggie.ro>
 <ZWdKI9UOZ6INP0Tu@kuha.fi.intel.com>
 <47ffbb30-34a7-4f5b-b262-3e068e574c8a@kernel.org>
 <ZWhp9M8165DiTNTd@kuha.fi.intel.com>
 <292f5d48-8567-4b60-ad03-6cf70f71bacc@kernel.org>
 <ZWmVA9KTVhZ4YCPO@kuha.fi.intel.com>
From:   Roger Quadros <rogerq@kernel.org>
In-Reply-To: <ZWmVA9KTVhZ4YCPO@kuha.fi.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

+Rob & Krzysztof

On 01/12/2023 10:10, Heikki Krogerus wrote:
> Hi Roger,
> 
> On Thu, Nov 30, 2023 at 03:30:54PM +0200, Roger Quadros wrote:
>> Hi Heikki,
>>
>> On 30/11/2023 12:54, Heikki Krogerus wrote:
>>> Hi Roger,
>>>
>>>>> Why not just match against the structures themselves?
>>>>>
>>>>>         if (tps->data == &tps25750_data)
>>>>>                 ...
>>>>
>>>> Then you need to declare tps25750_data and friends at the top of the file?
>>>>
>>>> A better approach might be to have type agnostic quirk flags for the special
>>>> behavior required for different types. This way, multiple devices can share
>>>> the same quirk if needed.
>>>>
>>>> e.g.
>>>> NEEDS_POWER_UP instead of TIPD_TYPE_APPLE_CD321X
>>>> SKIP_VID_READ instead of TIPD_TYPE_TI_TPS25750X
>>>> INIT_ON_RESUME instead of TIPD_TYPE_TI_TPS25750X
>>>>
>>>> Also rename cd321x_switch_power_state() to tps6598x_switch_power_state().
>>>
>>> No. Functions like that isolate cd321x specific functionality into an
>>> actual "function" just like they should.
>>>
>>> Quirk flags mean that if something breaks, it will almost always break
>>> for everybody (there is no real isolation with quirk flags), and when
>>> things are fixed and when features are added, we are forced to always
>>> "dance" around those quirk flags - you always have to consider them.
>>>
>>> Platform/device type checks are just as bad IMO, but in one way they
>>> are better than quirk flags. There is no question about what a
>>> platform check is checking, but quirk flags can so easily become
>>> incomprehensible (just what exactly does it mean when you say
>>> NEEDS_POWER_UP, SKIP_VID_READ and so on (you would need to document
>>> those quirks, which is waste of effort, and in reality nobody will do).
>>>
>>> In case of tipd/code.c, it should be converted into a library that
>>> only has the common/shared functionality. CD321, TPS2579x, TPS6598x
>>> and what ever there is, then will have a glue driver that handles
>>> everything that specific for their controller type.
>>
>> Do you mean that you want to treat the 3 devices as different incompatible devices
>> so each one has a separate driver which warrants for a different DT binding
>> for each and also Kconfig symbol?
> 
> I did not consider that, I was thinking that we would still continue
> with just one probe driver for all of these, but now that you
> mentioned this, maybe it would actually make sense to have separate
> full fledged probing drivers for all of these. Do you think it would
> be better like that? Would it be a problem to split the bindings?

I'm no DT expert but looks like an overkill to me.

-- 
cheers,
-roger