On Wed, Feb 14, 2024 at 03:01:06PM +0530, Bhargav Raviprakash wrote:
> Hi Conor,
>
> On Fri 2/9/2024 10:41 PM, Conor Dooley wrote:
> > On Thu, Feb 08, 2024 at 04:23:33PM +0530, Bhargav Raviprakash wrote:
> > > TPS65224 is a Power Management IC with 4 Buck regulators and 3 LDO
> > > regulators, it includes additional features like GPIOs, watchdog, ESMs
> > > (Error Signal Monitor), and PFSM (Pre-configurable Finite State Machine)
> > > managing the state of the device.
> >
> > > TPS6594 and TPS65224 have significant functional overlap.
> >
> > What does "significant functional overlap" mean? Does one implement a
> > compatible subset of the other? I assume the answer is no, given there
> > seems to be some core looking registers at different addresses.
>
> The intention behind “significant functional overlap” was meant to
> indicate a lot of the features between TPS6594 and TPS65224 overlap,
> while there are some features specific to TPS65224.
> There is compatibility between the PMIC register maps, I2C, PFSM,
> and other drivers even though there are some core registers at
> different addresses.
>
> Would it be more appropriate to say the 2 devices are compatible and have
> sufficient feature overlap rather than significant functional overlap?
If core registers are at different addresses, then it is unlikely that
these devices are compatible. In this context, compatible means that
existing software intended for the 6594 would run without modification
on the 65224, although maybe only supporting a subset of features.
If that's not the case, then the devices are not compatible.
Conor Dooley <[email protected]> writes:
> On Wed, Feb 14, 2024 at 03:01:06PM +0530, Bhargav Raviprakash wrote:
>> Hi Conor,
>>
>> On Fri 2/9/2024 10:41 PM, Conor Dooley wrote:
>> > On Thu, Feb 08, 2024 at 04:23:33PM +0530, Bhargav Raviprakash wrote:
>> > > TPS65224 is a Power Management IC with 4 Buck regulators and 3 LDO
>> > > regulators, it includes additional features like GPIOs, watchdog, ESMs
>> > > (Error Signal Monitor), and PFSM (Pre-configurable Finite State Machine)
>> > > managing the state of the device.
>> >
>> > > TPS6594 and TPS65224 have significant functional overlap.
>> >
>> > What does "significant functional overlap" mean? Does one implement a
>> > compatible subset of the other? I assume the answer is no, given there
>> > seems to be some core looking registers at different addresses.
>>
>> The intention behind “significant functional overlap” was meant to
>> indicate a lot of the features between TPS6594 and TPS65224 overlap,
>> while there are some features specific to TPS65224.
>> There is compatibility between the PMIC register maps, I2C, PFSM,
>> and other drivers even though there are some core registers at
>> different addresses.
>>
>> Would it be more appropriate to say the 2 devices are compatible and have
>> sufficient feature overlap rather than significant functional overlap?
>
> If core registers are at different addresses, then it is unlikely that
> these devices are compatible.
That's not necessarily true. Hardware designers can sometimes be
creative. :)
> In this context, compatible means that existing software intended for
> the 6594 would run without modification on the 65224, although maybe
> only supporting a subset of features. If that's not the case, then
> the devices are not compatible.
Compatible is a fuzzy term... so we need to get into the gray area.
What's going on here is that this new part is derivative in many
signifcant (but not all) ways from an existing similar part. When
writing drivers for new, derivative parts, there's always a choice
between 1) extending the existing driver (using a new compatible string
& match table for the diffs) or 2) creating a new driver which will have
a bunch of duplicated code.
The first verion of this series[1] took the 2nd approach, but due to the
significant functional (and feature) overlap, the recommendation was
instead to take the "reuse" path to avoid signficant amounts of
duplicated code.
Of course, it's possible that while going down the "reuse" path, there
may be a point where creating a separate driver for some aspects might
make sense, but that needs to be justified. Based on a quick glance of
what I see in this series so far (I have not done a detailed review),
the differences with the new device look to me like they can be handled
with chip-specific data in a match table.
Kevin
[1] https://lore.kernel.org/lkml/[email protected]/
On Wed, Feb 14, 2024 at 09:26:13AM -0800, Kevin Hilman wrote:
> Conor Dooley <[email protected]> writes:
> > On Wed, Feb 14, 2024 at 03:01:06PM +0530, Bhargav Raviprakash wrote:
> >> On Fri 2/9/2024 10:41 PM, Conor Dooley wrote:
> >> > On Thu, Feb 08, 2024 at 04:23:33PM +0530, Bhargav Raviprakash wrote:
> >> > > TPS65224 is a Power Management IC with 4 Buck regulators and 3 LDO
> >> > > regulators, it includes additional features like GPIOs, watchdog, ESMs
> >> > > (Error Signal Monitor), and PFSM (Pre-configurable Finite State Machine)
> >> > > managing the state of the device.
> >> >
> >> > > TPS6594 and TPS65224 have significant functional overlap.
> >> >
> >> > What does "significant functional overlap" mean? Does one implement a
> >> > compatible subset of the other? I assume the answer is no, given there
> >> > seems to be some core looking registers at different addresses.
> >>
> >> The intention behind “significant functional overlap” was meant to
> >> indicate a lot of the features between TPS6594 and TPS65224 overlap,
> >> while there are some features specific to TPS65224.
> >> There is compatibility between the PMIC register maps, I2C, PFSM,
> >> and other drivers even though there are some core registers at
> >> different addresses.
> >>
> >> Would it be more appropriate to say the 2 devices are compatible and have
> >> sufficient feature overlap rather than significant functional overlap?
> >
> > If core registers are at different addresses, then it is unlikely that
> > these devices are compatible.
>
> That's not necessarily true. Hardware designers can sometimes be
> creative. :)
Hence "unlikely" in my mail :)
> > In this context, compatible means that existing software intended for
> > the 6594 would run without modification on the 65224, although maybe
> > only supporting a subset of features. If that's not the case, then
> > the devices are not compatible.
>
> Compatible is a fuzzy term... so we need to get into the gray area.
>
> What's going on here is that this new part is derivative in many
> signifcant (but not all) ways from an existing similar part. When
> writing drivers for new, derivative parts, there's always a choice
> between 1) extending the existing driver (using a new compatible string
> & match table for the diffs) or 2) creating a new driver which will have
> a bunch of duplicated code.
>
> The first verion of this series[1] took the 2nd approach, but due to the
> significant functional (and feature) overlap, the recommendation was
> instead to take the "reuse" path to avoid signficant amounts of
> duplicated code.
>
> Of course, it's possible that while going down the "reuse" path, there
> may be a point where creating a separate driver for some aspects might
> make sense, but that needs to be justified. Based on a quick glance of
> what I see in this series so far (I have not done a detailed review),
> the differences with the new device look to me like they can be handled
> with chip-specific data in a match table.
This is all nice information, but not really relevant here - this is a
binding patch, not a driver one & the conversation stemmed from me
making sure that a fallback compatible was not suitable. Whether or not
there are multiple drivers or not is someone else's problem!
Thanks,
Conor.
Conor Dooley <[email protected]> writes:
> On Wed, Feb 14, 2024 at 09:26:13AM -0800, Kevin Hilman wrote:
>> Conor Dooley <[email protected]> writes:
>> > On Wed, Feb 14, 2024 at 03:01:06PM +0530, Bhargav Raviprakash wrote:
>> >> On Fri 2/9/2024 10:41 PM, Conor Dooley wrote:
>> >> > On Thu, Feb 08, 2024 at 04:23:33PM +0530, Bhargav Raviprakash wrote:
>> >> > > TPS65224 is a Power Management IC with 4 Buck regulators and 3 LDO
>> >> > > regulators, it includes additional features like GPIOs, watchdog, ESMs
>> >> > > (Error Signal Monitor), and PFSM (Pre-configurable Finite State Machine)
>> >> > > managing the state of the device.
>> >> >
>> >> > > TPS6594 and TPS65224 have significant functional overlap.
>> >> >
>> >> > What does "significant functional overlap" mean? Does one implement a
>> >> > compatible subset of the other? I assume the answer is no, given there
>> >> > seems to be some core looking registers at different addresses.
>> >>
>> >> The intention behind “significant functional overlap” was meant to
>> >> indicate a lot of the features between TPS6594 and TPS65224 overlap,
>> >> while there are some features specific to TPS65224.
>> >> There is compatibility between the PMIC register maps, I2C, PFSM,
>> >> and other drivers even though there are some core registers at
>> >> different addresses.
>> >>
>> >> Would it be more appropriate to say the 2 devices are compatible and have
>> >> sufficient feature overlap rather than significant functional overlap?
>> >
>> > If core registers are at different addresses, then it is unlikely that
>> > these devices are compatible.
>>
>> That's not necessarily true. Hardware designers can sometimes be
>> creative. :)
>
> Hence "unlikely" in my mail :)
>
>> > In this context, compatible means that existing software intended for
>> > the 6594 would run without modification on the 65224, although maybe
>> > only supporting a subset of features. If that's not the case, then
>> > the devices are not compatible.
>>
>> Compatible is a fuzzy term... so we need to get into the gray area.
>>
>> What's going on here is that this new part is derivative in many
>> signifcant (but not all) ways from an existing similar part. When
>> writing drivers for new, derivative parts, there's always a choice
>> between 1) extending the existing driver (using a new compatible string
>> & match table for the diffs) or 2) creating a new driver which will have
>> a bunch of duplicated code.
>>
>> The first verion of this series[1] took the 2nd approach, but due to the
>> significant functional (and feature) overlap, the recommendation was
>> instead to take the "reuse" path to avoid signficant amounts of
>> duplicated code.
>>
>> Of course, it's possible that while going down the "reuse" path, there
>> may be a point where creating a separate driver for some aspects might
>> make sense, but that needs to be justified. Based on a quick glance of
>> what I see in this series so far (I have not done a detailed review),
>> the differences with the new device look to me like they can be handled
>> with chip-specific data in a match table.
>
> This is all nice information, but not really relevant here - this is a
> binding patch, not a driver one & the conversation stemmed from me
> making sure that a fallback compatible was not suitable.
hehe, oops. <blush>. my fault for mixing the two together
Sorry for the noise.
Kevin
On 2/14/24 18:45, Conor Dooley wrote:
> On Wed, Feb 14, 2024 at 09:26:13AM -0800, Kevin Hilman wrote:
>> Conor Dooley <[email protected]> writes:
>>> On Wed, Feb 14, 2024 at 03:01:06PM +0530, Bhargav Raviprakash wrote:
>>>> On Fri 2/9/2024 10:41 PM, Conor Dooley wrote:
>>>>> On Thu, Feb 08, 2024 at 04:23:33PM +0530, Bhargav Raviprakash wrote:
>>>>>> TPS65224 is a Power Management IC with 4 Buck regulators and 3 LDO
>>>>>> regulators, it includes additional features like GPIOs, watchdog, ESMs
>>>>>> (Error Signal Monitor), and PFSM (Pre-configurable Finite State Machine)
>>>>>> managing the state of the device.
>>>>>> TPS6594 and TPS65224 have significant functional overlap.
>>>>> What does "significant functional overlap" mean? Does one implement a
>>>>> compatible subset of the other? I assume the answer is no, given there
>>>>> seems to be some core looking registers at different addresses.
>>>> The intention behind “significant functional overlap” was meant to
>>>> indicate a lot of the features between TPS6594 and TPS65224 overlap,
>>>> while there are some features specific to TPS65224.
>>>> There is compatibility between the PMIC register maps, I2C, PFSM,
>>>> and other drivers even though there are some core registers at
>>>> different addresses.
>>>>
>>>> Would it be more appropriate to say the 2 devices are compatible and have
>>>> sufficient feature overlap rather than significant functional overlap?
>>> If core registers are at different addresses, then it is unlikely that
>>> these devices are compatible.
>> That's not necessarily true. Hardware designers can sometimes be
>> creative. :)
> Hence "unlikely" in my mail :)
For tps6594 and tps65224, some core registers are at different adresses
indeed, but the code is the same for both MFD I2C/SPI entry points. As an
example, the way CRC is enabled is exactly the same, even if the bit that
must be set belongs to different registers. tps65224 has more resources and
it's as if HW designers had had to re-organize the way bits are distributed
among the registers (due to a lack of space, so to speak).
That said, if we consider that these devices are not compatible, what does it
imply concretely for the next version ? Does that mean that:
1) Only a new binding must be created, even if MFD drivers and most of child
drivers will be re-used ? (then the binding would simply be duplicated, but
the drivers would not)
2) A new binding and new MFD drivers must be created, even if most of child
drivers will be re-used ? (then the binding and MFD drivers would simply be
duplicated, but the child drivers would not)
3) A new binding and new drivers (MFD and child devices) must be created ?
4) Anything else ?
@Conor: I understand that it's not your problem. Anybody can answer, I just
try to make things clear for the next version. :)
Julien
>
>>> In this context, compatible means that existing software intended for
>>> the 6594 would run without modification on the 65224, although maybe
>>> only supporting a subset of features. If that's not the case, then
>>> the devices are not compatible.
>> Compatible is a fuzzy term... so we need to get into the gray area.
>>
>> What's going on here is that this new part is derivative in many
>> signifcant (but not all) ways from an existing similar part. When
>> writing drivers for new, derivative parts, there's always a choice
>> between 1) extending the existing driver (using a new compatible string
>> & match table for the diffs) or 2) creating a new driver which will have
>> a bunch of duplicated code.
>>
>> The first verion of this series[1] took the 2nd approach, but due to the
>> significant functional (and feature) overlap, the recommendation was
>> instead to take the "reuse" path to avoid signficant amounts of
>> duplicated code.
>>
>> Of course, it's possible that while going down the "reuse" path, there
>> may be a point where creating a separate driver for some aspects might
>> make sense, but that needs to be justified. Based on a quick glance of
>> what I see in this series so far (I have not done a detailed review),
>> the differences with the new device look to me like they can be handled
>> with chip-specific data in a match table.
> This is all nice information, but not really relevant here - this is a
> binding patch, not a driver one & the conversation stemmed from me
> making sure that a fallback compatible was not suitable. Whether or not
> there are multiple drivers or not is someone else's problem!
>
> Thanks,
> Conor.
On Fri, Feb 16, 2024 at 10:08:03AM +0100, Julien Panis wrote:
> On 2/14/24 18:45, Conor Dooley wrote:
> > On Wed, Feb 14, 2024 at 09:26:13AM -0800, Kevin Hilman wrote:
> > > Conor Dooley <[email protected]> writes:
> > > > On Wed, Feb 14, 2024 at 03:01:06PM +0530, Bhargav Raviprakash wrote:
> > > > > On Fri 2/9/2024 10:41 PM, Conor Dooley wrote:
> > > > > > On Thu, Feb 08, 2024 at 04:23:33PM +0530, Bhargav Raviprakash wrote:
> > > > > > > TPS65224 is a Power Management IC with 4 Buck regulators and 3 LDO
> > > > > > > regulators, it includes additional features like GPIOs, watchdog, ESMs
> > > > > > > (Error Signal Monitor), and PFSM (Pre-configurable Finite State Machine)
> > > > > > > managing the state of the device.
> > > > > > > TPS6594 and TPS65224 have significant functional overlap.
> > > > > > What does "significant functional overlap" mean? Does one implement a
> > > > > > compatible subset of the other? I assume the answer is no, given there
> > > > > > seems to be some core looking registers at different addresses.
> > > > > The intention behind “significant functional overlap” was meant to
> > > > > indicate a lot of the features between TPS6594 and TPS65224 overlap,
> > > > > while there are some features specific to TPS65224.
> > > > > There is compatibility between the PMIC register maps, I2C, PFSM,
> > > > > and other drivers even though there are some core registers at
> > > > > different addresses.
> > > > >
> > > > > Would it be more appropriate to say the 2 devices are compatible and have
> > > > > sufficient feature overlap rather than significant functional overlap?
> > > > If core registers are at different addresses, then it is unlikely that
> > > > these devices are compatible.
> > > That's not necessarily true. Hardware designers can sometimes be
> > > creative. :)
> > Hence "unlikely" in my mail :)
>
> For tps6594 and tps65224, some core registers are at different adresses
> indeed, but the code is the same for both MFD I2C/SPI entry points. As an
> example, the way CRC is enabled is exactly the same, even if the bit that
> must be set belongs to different registers. tps65224 has more resources and
> it's as if HW designers had had to re-organize the way bits are distributed
> among the registers (due to a lack of space, so to speak).
>
> That said, if we consider that these devices are not compatible, what does it
> imply concretely for the next version ? Does that mean that:
> 1) Only a new binding must be created, even if MFD drivers and most of child
> drivers will be re-used ? (then the binding would simply be duplicated, but
> the drivers would not)
> 2) A new binding and new MFD drivers must be created, even if most of child
> drivers will be re-used ? (then the binding and MFD drivers would simply be
> duplicated, but the child drivers would not)
> 3) A new binding and new drivers (MFD and child devices) must be created ?
> 4) Anything else ?
If they're not compatible the next version of this patch does not need
to change, so option 4 I guess.