Re: [PATCH v7 2/2] hwmon: (pmbus/q54sj108a2) Add support for q50sn12072 and q54sn120a1

[Brian Chiang]

On Thu, Apr 30, 2026 at 07:01:22AM -0700, Guenter Roeck wrote:
> On 4/29/26 23:58, Brian Chiang wrote:
> > On Wed, Apr 29, 2026 at 11:29:37AM +0000, Brian Chiang wrote:
> > > From: Jack Cheng <cheng.jackhy@xxxxxxxxxxxx>
> > >
> > > The Q50SN12072 and Q54SN120A1 are high-efficiency, high-density DC-DC power
> > > module from Delta Power Modules.
> > >
> > > The Q50SN12072, quarter brick, single output 12V. This product provides up
> > > to 1200 watts of output power at 38~60V. The Q50SN12072 offers peak
> > > efficiency up to 98.3%@54Vin.
> > >
> > > The Q54SN120A1, quarter brick, single output 12V. This product provides up
> > > to 1300 watts of output power at 40~60V. The Q54SN120A1 offers peak
> > > efficiency up to 98.1%@54Vin.
> > >
> > > Add support for them to q54sj108a2 driver.
> >
> > Greetings, I received the feedback from Sashiko for this patch:
> >
> > ```
> > This isn't a bug, but the commit message only mentions adding support for
> > the new modules. However, the patch also includes several other changes:
> > adding PMBus locking in the debugfs read/write paths, fixing the
> > WRITE_PROTECT restore logic, modifying the configuration for the existing
> > q54sj108a2 module, and refactoring the device identification logic.
> > Could the commit message be updated to describe these additional changes,
> > or should they be split into separate patches?
> > ```
> >
> > I'm wondering if it is more appropriate to split only `fixing the WRITE_PROTECT restore logic` into separate patch? Since disabling WRITE_PROTECT was introduced in previous commit. And maybe keeping
> > other changes Sashiko mentioned in this patch and record them in the commit message?
> >
> > Please let me know if you have any suggestion, thanks.
>
> Sorry, I seem to be missing something. I dont understand the logic above.
> What does fixing a bug have to do with how or when it was introduced ?
>
> The missing lock in debugfs functions is a pre-existing bug, isn't it ?
> All pre-existing bugs should be fixed first in separate patches so they
> can be backported.

Got it, I'll split them into standalone fix patches at the start of the
series.

> Please use guard(pmbus_lock)(client) instead of pmbus_lock_interruptible().

Sure, I'll do this in v8.

> Also, your patch does not apply anymore. Support for q54sn120a1 was already
> added in a separate patch. Please rebase your patches to v7.1-rc1.

Thank you for the notice, I'll rebase the patchset to lateset tag.


Thanks again for the review and suggestion and apologies for the delayed
response.

> Thanks,
> Guenter

Best Regards,
Brian