On 3/31/22 07:58, Russell King (Oracle) wrote:
On Thu, Mar 31, 2022 at 04:51:47PM +0200, Michael Walle wrote:
Am 2022-03-31 16:45, schrieb Russell King (Oracle):
On Wed, Mar 30, 2022 at 08:23:35AM -0700, Guenter Roeck wrote:
Michael, let's just drop the changes outside drivers/hwmon from
the series, and let's keep hwmon_is_bad_char() in the include file.
Let's just document it, explaining its use case.
Why? There hasn't been any objection to the change. All the discussion
seems to be around the new function (this patch) rather than the actual
conversions in drivers.
I'm entirely in favour of cleaning this up - it irks me that we're doing
exactly the same cleanup everywhere we have a hwmon.
At the very least, I would be completely in favour of keeping the
changes in the sfp and phy code.
FWIW, my plan was to send the hwmon patches first, by then my other
series (the polynomial_calc() one) will also be ready to be picked.
Then I'd ask Guenter for a stable branch with these two series which
hopefully get merged into net-next. Then I can repost the missing
patches on net-next along with the new sensors support for the GPY
and LAN8814 PHYs.
Okay, that's fine. It just sounded like the conversion of other drivers
outside drivers/hwmon was being dropped.
Not dropped, just disconnected. From hwmon perspective, we want a certain
set of characters to be dropped or replaced. Also, from hwmon perspective,
it makes sense to have the helper function allocate the replacement data.
There was disagreement about which characters should be replaced, and if
the helper function should allocate the replacement string or not.
I have no intention to change the set of characters without good reason,
and I feel quite strongly about allocating the replacement in the helper
function. Since potential callers don't _have_ to use the helper and don't
_have_ to provide valid names (and are responsible for the consequences),
I would like that discussion to be separate from hwmon changes.
Note that there's another "sanitisation" of hwmon names in
drivers/net/phy/marvell.c - that converts any non-alnum character to
an underscore. Not sure why the different approach was chosen there.
It actually drops non-alphanumeric characters. The name is derived
from the phy device name, which I think is derived from the name field
in struct phy_driver. That includes spaces and '(', ')'. I honestly
have no idea what libsensors would do with '(' and ')'. Either case,
even if that would create a hiccup in libsensors and we would add
'(' and ')' to the 'forbidden' list of characters, the fact that the
code doesn't replace but drop non-alphanumeric characters means
it won't be able to use a helper anyway since that would result
in a hwmon 'name' attribute change and thus not be backward
compatible. Besides, "Marvell_88E1111__Finisar_" would look a bit
odd anyway, and "Marvell88E1111Finisar" may be at least slightly
better.
Guenter
