Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Fri, 17 Nov 2023 11:05:33 +0000
From:   Lee Jones <lee@kernel.org>
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     Shree Ramamoorthy <s-ramamoorthy@ti.com>,
        Julien Panis <jpanis@baylibre.com>,
        Gairuboina Sirisha <sirisha.gairuboina@ltts.com>,
        arnd@arndb.de, krzysztof.kozlowski+dt@linaro.org,
        linux-kernel@vger.kernel.org, d-gole@ti.com
Subject: Re: [PATCH v1 0/3] TPS65224 PMIC driver
Message-ID: <20231117110533.GA8822@google.com>
References: <b9449689-73da-41a8-a94e-f82686ab0bb0@baylibre.com>
 <20231107113740.1034738-1-sirisha.gairuboina@Ltts.com>
 <2f467b0a-1d11-4ec7-8ca6-6c4ba66e5887@baylibre.com>
 <818ecf7b-cd91-45ca-bb92-65f10f1ca1f0@ti.com>
 <2023111007-siamese-crepe-9775@gregkh>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <2023111007-siamese-crepe-9775@gregkh>
Precedence: bulk

On Fri, 10 Nov 2023, Greg KH wrote:

> On Thu, Nov 09, 2023 at 10:22:00AM -0600, Shree Ramamoorthy wrote:
> > > I compared 'tps65224.h' with 'tps6594.h', especially register mapping.
> > > There are less resources in TPS65224, but I don't see any incompatibility
> > > between both PMIC register mappings. Some registers are not used by
> > > your TPS65224, and some interrupts are not used either (that's not a
> > > problem, they will not trigger, so). Beyond that, I2C and PFSM drivers
> > > perform the same things for both PMICs. That's why according to me,
> > > nothing prevents from re-using TPS6594 drivers. Even for ADC, which is
> > > specific to your TPS65224 indeed, the register range does not overlap
> > > with any of TPS6594 registers. You could conditionally add this driver
> > > (that's what we did in  'tps6594-core.c' for RTC driver, which is not
> > > used
> > > for one of the compatibles: you can do something similar for ADC).
> > > You will probably add support for others TPS65224 drivers over the next
> > > weeks: SPI, ESM, RTC, GPIOs, regulators, watchdog, and ADC. Most of them
> > > should be compatible with both TPS6594 and TPS65224, I think (even
> > > watchdog driver, which was not developed for TPS6594). ADC will not,
> > > but as explained above you can easily deal with this one thanks to
> > > the compatible.
> > > For 'tps65224-core.c' only, a little bit of work might be necessary to
> > > handle your TPS65224 specific functionalities. By using a different DT
> > > compatible string, your driver can then select different options (or
> > > maybe
> > > even different register ranges) for some features based on the
> > > compatible.
> > > But except for 'tps65xx-core.c', there is "sufficient overlap" to justify
> > > sharing as much as possible between TPS65224 and TPS6594, in my
> > > opinion.
> > 
> > 
> > TI is positioning TPS65224 as a separate family from TPS6594, but shared
> > software drivers for PMICs that have different use cases would lead to
> > confusion.
> 
> Why?  No one cares what a driver's name is, only that it works for their
> hardware.  What different "use case" would cause problems here?
> 
> > Re-scoping the project to accommodate these suggestions would
> > negatively affect the timeline we are trying to meet.
> 
> There are no timelines/deadlines with kernel development, sorry, that's
> not our issue.
> 
> > We want to include the
> > restructure that addresses the compatibility, register maps, and
> > functionality similarities, but it would best solved after the upcoming
> > deadline has been met.
> 
> Again, no deadline here.  Please do the work properly, that's all we
> care about.
> 
> > With the growth of PMIC software device drivers, we
> > would prefer to have a separate series with the suggested changes and proper
> > naming convention to address that while they overlap, the two PMICs devices
> > are not a subset.
> 
> Why does the name matter?  Again, all that a user cares about is if
> their hardware device is supported, the name means nothing here.
> 
> Please do the correct thing and add support for this device to the
> existing drivers, that's the correct thing to do.  You will save time
> and energy and code in the long-run, which is the important thing.
> 
> There is a reason that Linux drivers are, on average, 1/3 smaller than
> other operating systems.  And that's because they share common code with
> other drivers.  You aren't allowed to just copy an existing one and add
> a few changes and make a whole new driver, you need to modify the
> current one.
> 
> thanks,
> 
> greg k-h

Ha!  You took the words right out of my mouth!

Thanks.

-- 
Lee Jones [李琼斯]