Received-SPF: pass (google.com: domain of linux-kernel+bounces-147020-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 16 Apr 2024 10:32:10 -0400
From: =?utf-8?B?TsOtY29sYXMgRi4gUi4gQS4=?= Prado <nfraprado@collabora.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Cc: Sebastian Reichel <sre@kernel.org>, kernel@collabora.com,
	linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] power: supply: sbs-battery: Handle unsupported
 PROP_TIME_TO_EMPTY_NOW
Message-ID: <9f805ec3-1611-4682-831e-9499ba8284d5@notapiano>
References: <20240415-sbs-time-empty-now-error-v2-1-32d8a747e308@collabora.com>
 <9ba1b4df-78dc-4f24-bb61-d7b168da18f8@collabora.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <9ba1b4df-78dc-4f24-bb61-d7b168da18f8@collabora.com>

On Tue, Apr 16, 2024 at 09:46:56AM +0200, AngeloGioacchino Del Regno wrote:
> Il 15/04/24 21:05, N?colas F. R. A. Prado ha scritto:
[..]
> > +
> > +static void sbs_update_quirks(struct sbs_info *chip)
> > +{
> > +	const char *model;
> > +	const char *manufacturer;
> > +	unsigned int i = 0;
> 
> Please reorder:
> 
> 	const char *manufacturer;
> 	const char *model;
> 	unsigned int i;
> 
> ...and please remove (like shown, of course) the double initialization of the
> `i` variable, as you're initializing it again later in your for loop.

Ack.

> 
> > +
> > +	/* reset quirks from battery before the hot-plug event */
> > +	chip->quirks = 0;
> > +
> > +	manufacturer = sbs_get_constant_string(chip, POWER_SUPPLY_PROP_MANUFACTURER);
> > +	model = sbs_get_constant_string(chip, POWER_SUPPLY_PROP_MODEL_NAME);
> > +	if (IS_ERR(manufacturer) || IS_ERR(model)) {
> > +		dev_warn(&chip->client->dev, "Couldn't read manufacturer and model to set quirks\n");
> > +		return;
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(sbs_quirks); i++) {
> > +		if (strcmp(manufacturer, sbs_quirks[i].manufacturer))
> > +			continue;
> > +		if (strcmp(model, sbs_quirks[i].model))
> > +			continue;
> > +		chip->quirks |= sbs_quirks[i].flags;
> > +	}
> > +
> > +	if (chip->quirks & SBS_QUIRK_BROKEN_TTE_NOW)
> > +		dev_info(&chip->client->dev, "Added quirk disabling TIME_TO_EMPTY_NOW\n");
> 
> I don't really expect many quirks, but having a dev_info() print for every
> quirk that gets set would make this driver too chatty, IMO.
> 
> Please, either turn that into a dev_dbg() or print a mask of the quirks .. or both.

I wouldn't make this debug as it's pretty important information to be
reported. I'd be ok with having a single message listing all the quirks, but at
the same time I feel like this would be trying to fix a problem that we don't
know will ever exist (one SBS battery that has many quirks). I propose we keep
it the way it is - a nice clear message of what has happened - and adapt in the
future if it turns out to be too much information.

Thanks,
N?colas