Re: [PATCH v12 3/4] thinkpad_acpi: Add support for battery thresholds

["Ognjen Galić" <smclt30p@gmail.com>]

On Wed, Jan 03, 2018 at 04:42:35PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 3, 2018 at 1:59 PM, Ognjen Galic <smclt30p@gmail.com> wrote:
> > thinkpad_acpi registers two new attributes for each battery:
> >
> > 1) Charge start threshold
> > /sys/class/power_supply/BATN/charge_start_threshold
> >
> > Valid values are [0, 99]. A value of 0 turns off the
> > start threshold wear control.
> >
> > 2) Charge stop threshold
> > /sys/class/power_supply/BATN/charge_stop_threshold
> >
> > Valid values are [1, 100]. A value of 100 turns off
> > the stop threshold wear control. This must be
> > configured first.
> >
> > Signed-off-by: Ognjen Galic <smclt30p@gmail.com>
> 
> Thanks for an update!
> 
> > ---
> >
> > Notes:
> 
> >     * Change int to acpi_status in tpacpi_battery_acpi_eval
> 
> Then you need to check return code with ACPI_FAILURE() or ACPI_SUCCESS() macros.
> That is one of the additional burden while I can't see usefulness of
> ACPI return codes in that function and why I suggested to use plain
> int.

Thos macros basically do the same stuff I do manually. I think that most
of the kernel does errors on a non-0 basis, and ACPI is no exception.

That being said, I think that using a simple non-0 check if more clean
than using those macros.

Isn't this:

if (!acpi_eval....

nicer than this?

if ACPI_FAILURE(acpi_eval...

It just adds more stuff to get wrong. And I don't really think that the 
ACPI implementation will change those macros anytime soon, or ever.

Keeping it simple.

> 
> One more question: why don't you use dev_err()/dev_*() macros instead
> of pr_*() ones?
> (Note, it's a question, needs a bit of discussion, I would like to
> hear a rationale of this, I think it might be one)

Techically, I'm not adding a device this I did not use those macros. I'm
only adding more attributes to a device that the battery module manages.

> 
> -- 
> With Best Regards,
> Andy Shevchenko