| Message ID | 20180716144206.30985-2-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16/07/2018 16:41, Miquel Raynal wrote: > Thermal zone names must follow certain rules imposed by the framework. > They are limited in length and shall not have any hyphen '-'. > > This is done in a separate function for future use in another location. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> Why do you have to provide a function to test that? Logically, the one who did the change to add a thermal name, should check its code works. Without a proper name that won't work. So this function is testing something which should be already tested, no? > --- > drivers/thermal/armada_thermal.c | 36 +++++++++++++++++++++++++++++++++++- > 1 file changed, 35 insertions(+), 1 deletion(-) > > diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c > index 4c275ec10ac5..077e8e562306 100644 > --- a/drivers/thermal/armada_thermal.c > +++ b/drivers/thermal/armada_thermal.c > @@ -70,6 +70,7 @@ struct armada_thermal_priv { > void __iomem *status; > void __iomem *control0; > void __iomem *control1; > + char zone_name[THERMAL_NAME_LENGTH]; > struct armada_thermal_data *data; > }; > > @@ -353,6 +354,36 @@ static const struct of_device_id armada_thermal_id_table[] = { > }; > MODULE_DEVICE_TABLE(of, armada_thermal_id_table); > > +static void armada_set_sane_name(struct platform_device *pdev, > + struct armada_thermal_priv *priv) > +{ > + const char *name = dev_name(&pdev->dev); > + char *insane_char; > + > + if (strlen(name) > THERMAL_NAME_LENGTH) { > + /* > + * When inside a system controller, the device name has the > + * form: f06f8000.system-controller:ap-thermal so stripping > + * after the ':' should give us a shorter but meaningful name. > + */ > + name = strrchr(name, ':'); > + if (!name) > + name = "armada_thermal"; > + else > + name++; > + } > + > + /* Save the name locally */ > + strncpy(priv->zone_name, name, THERMAL_NAME_LENGTH); > + > + /* Then check there are no '-' or hwmon core will complain */ > + do { > + insane_char = strpbrk(priv->zone_name, "-"); > + if (insane_char) > + *insane_char = '_'; > + } while (insane_char); > +} > + > static int armada_thermal_probe(struct platform_device *pdev) > { > void __iomem *control = NULL; > @@ -381,6 +412,9 @@ static int armada_thermal_probe(struct platform_device *pdev) > > priv->data = (struct armada_thermal_data *)match->data; > > + /* Ensure device name is correct for the thermal core */ > + armada_set_sane_name(pdev, priv); > + > /* > * Legacy DT bindings only described "control1" register (also referred > * as "control MSB" on old documentation). New bindings cover > @@ -402,7 +436,7 @@ static int armada_thermal_probe(struct platform_device *pdev) > > priv->data->init_sensor(pdev, priv); > > - thermal = thermal_zone_device_register(dev_name(&pdev->dev), 0, 0, priv, > + thermal = thermal_zone_device_register(priv->zone_name, 0, 0, priv, > &ops, NULL, 0, 0); > if (IS_ERR(thermal)) { > dev_err(&pdev->dev, >
Hi Daniel, Daniel Lezcano <daniel.lezcano@linaro.org> wrote on Fri, 27 Jul 2018 13:34:19 +0200: > On 16/07/2018 16:41, Miquel Raynal wrote: > > Thermal zone names must follow certain rules imposed by the framework. > > They are limited in length and shall not have any hyphen '-'. > > > > This is done in a separate function for future use in another location. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > Why do you have to provide a function to test that? > > Logically, the one who did the change to add a thermal name, should > check its code works. Without a proper name that won't work. What do you mean "the one who did the change"? I think the thermal core should not care that much to what is given as name and should probably not be so strict. Also, I don't choose what dev_name() returns, it's in the device tree and the device tree do not care about the implementation, it's just a descriptive file. > > So this function is testing something which should be already tested, no? I don't think it is. Without this function the probe will simply fail. The explanation of what fails is in the code: > > + /* > > + * When inside a system controller, the device name has the > > + * form: f06f8000.system-controller:ap-thermal so stripping > > + * after the ':' should give us a shorter but meaningful name. > > + */ > > + name = strrchr(name, ':'); > > + if (!name) > > + name = "armada_thermal"; > > + else > > + name++; [...] > > + > > + /* Then check there are no '-' or hwmon core will complain */ Thanks, Miquèl
On 27/07/2018 13:52, Miquel Raynal wrote: > Hi Daniel, > > Daniel Lezcano <daniel.lezcano@linaro.org> wrote on Fri, 27 Jul 2018 > 13:34:19 +0200: > >> On 16/07/2018 16:41, Miquel Raynal wrote: >>> Thermal zone names must follow certain rules imposed by the framework. >>> They are limited in length and shall not have any hyphen '-'. >>> >>> This is done in a separate function for future use in another location. >>> >>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> >> >> Why do you have to provide a function to test that? >> >> Logically, the one who did the change to add a thermal name, should >> check its code works. Without a proper name that won't work. > > What do you mean "the one who did the change"? > I think the thermal core should not care that much to what is given as > name and should probably not be so strict. > > Also, I don't choose what dev_name() returns, it's in the device tree > and the device tree do not care about the implementation, it's just a > descriptive file. > >> >> So this function is testing something which should be already tested, no? > > I don't think it is. Without this function the probe will simply fail. > > The explanation of what fails is in the code: > >>> + /* >>> + * When inside a system controller, the device name has the >>> + * form: f06f8000.system-controller:ap-thermal so stripping >>> + * after the ':' should give us a shorter but meaningful name. >>> + */ >>> + name = strrchr(name, ':'); >>> + if (!name) >>> + name = "armada_thermal"; >>> + else >>> + name++; > > [...] I'm not in favor of this, potentially it can introduce a break which can be exploited later. You are creating the thermal zones manually from the driver but want to rely on the temperature controller name to give a name to the thermal zone. Why not create the thermal zone in the DT with the correct name and use devm_thermal_zone_of_sensor_register ? Or alternatively hardcode the thermal zone name ?
Hi Daniel, Daniel Lezcano <daniel.lezcano@linaro.org> wrote on Fri, 27 Jul 2018 17:29:52 +0200: > On 27/07/2018 13:52, Miquel Raynal wrote: > > Hi Daniel, > > > > Daniel Lezcano <daniel.lezcano@linaro.org> wrote on Fri, 27 Jul 2018 > > 13:34:19 +0200: > > > >> On 16/07/2018 16:41, Miquel Raynal wrote: > >>> Thermal zone names must follow certain rules imposed by the framework. > >>> They are limited in length and shall not have any hyphen '-'. > >>> > >>> This is done in a separate function for future use in another location. > >>> > >>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > >> > >> Why do you have to provide a function to test that? > >> > >> Logically, the one who did the change to add a thermal name, should > >> check its code works. Without a proper name that won't work. > > > > What do you mean "the one who did the change"? > > I think the thermal core should not care that much to what is given as > > name and should probably not be so strict. > > > > Also, I don't choose what dev_name() returns, it's in the device tree > > and the device tree do not care about the implementation, it's just a > > descriptive file. > > > >> > >> So this function is testing something which should be already tested, no? > > > > I don't think it is. Without this function the probe will simply fail. > > > > The explanation of what fails is in the code: > > > >>> + /* > >>> + * When inside a system controller, the device name has the > >>> + * form: f06f8000.system-controller:ap-thermal so stripping > >>> + * after the ':' should give us a shorter but meaningful name. > >>> + */ > >>> + name = strrchr(name, ':'); > >>> + if (!name) > >>> + name = "armada_thermal"; > >>> + else > >>> + name++; > > > > [...] > > I'm not in favor of this, potentially it can introduce a break which can > be exploited later. I know, I don't like this neither but that was the best way IMHO to handle the core's limitations. > > You are creating the thermal zones manually from the driver but want to > rely on the temperature controller name to give a name to the thermal zone. > > Why not create the thermal zone in the DT with the correct name and use > devm_thermal_zone_of_sensor_register ? That's exactly what is done later in the series actually, but for DT backward compatibility and in order to continue adding feature to this driver, we must ensure there won't be any drawback for people using old DT with deprecated thermal representation. Thanks for reviewing, if you don't mind I could copy you in future changes? Unfortunately for this series it's already been applied and I would like not to do any deep changes before it's merged, but for sure this function could be enhanced. Kind regards, Miquèl
On 29/07/2018 21:27, Miquel Raynal wrote: > Hi Daniel, [ ... ] > That's exactly what is done later in the series actually, but for DT > backward compatibility and in order to continue adding feature to this > driver, we must ensure there won't be any drawback for people using old > DT with deprecated thermal representation. > > > Thanks for reviewing, if you don't mind I could copy you in future > changes? Yes, sure. > Unfortunately for this series it's already been applied and I > would like not to do any deep changes before it's merged, but for sure > this function could be enhanced. Or find a way to remove it ? :)
diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c index 4c275ec10ac5..077e8e562306 100644 --- a/drivers/thermal/armada_thermal.c +++ b/drivers/thermal/armada_thermal.c @@ -70,6 +70,7 @@ struct armada_thermal_priv { void __iomem *status; void __iomem *control0; void __iomem *control1; + char zone_name[THERMAL_NAME_LENGTH]; struct armada_thermal_data *data; }; @@ -353,6 +354,36 @@ static const struct of_device_id armada_thermal_id_table[] = { }; MODULE_DEVICE_TABLE(of, armada_thermal_id_table); +static void armada_set_sane_name(struct platform_device *pdev, + struct armada_thermal_priv *priv) +{ + const char *name = dev_name(&pdev->dev); + char *insane_char; + + if (strlen(name) > THERMAL_NAME_LENGTH) { + /* + * When inside a system controller, the device name has the + * form: f06f8000.system-controller:ap-thermal so stripping + * after the ':' should give us a shorter but meaningful name. + */ + name = strrchr(name, ':'); + if (!name) + name = "armada_thermal"; + else + name++; + } + + /* Save the name locally */ + strncpy(priv->zone_name, name, THERMAL_NAME_LENGTH); + + /* Then check there are no '-' or hwmon core will complain */ + do { + insane_char = strpbrk(priv->zone_name, "-"); + if (insane_char) + *insane_char = '_'; + } while (insane_char); +} + static int armada_thermal_probe(struct platform_device *pdev) { void __iomem *control = NULL; @@ -381,6 +412,9 @@ static int armada_thermal_probe(struct platform_device *pdev) priv->data = (struct armada_thermal_data *)match->data; + /* Ensure device name is correct for the thermal core */ + armada_set_sane_name(pdev, priv); + /* * Legacy DT bindings only described "control1" register (also referred * as "control MSB" on old documentation). New bindings cover @@ -402,7 +436,7 @@ static int armada_thermal_probe(struct platform_device *pdev) priv->data->init_sensor(pdev, priv); - thermal = thermal_zone_device_register(dev_name(&pdev->dev), 0, 0, priv, + thermal = thermal_zone_device_register(priv->zone_name, 0, 0, priv, &ops, NULL, 0, 0); if (IS_ERR(thermal)) { dev_err(&pdev->dev,
Thermal zone names must follow certain rules imposed by the framework. They are limited in length and shall not have any hyphen '-'. This is done in a separate function for future use in another location. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/thermal/armada_thermal.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)