From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> To: Balsam CHIHI <bchihi@baylibre.com> Cc: daniel.lezcano@linaro.org, angelogioacchino.delregno@collabora.com, rafael@kernel.org, amitk@kernel.org, rui.zhang@intel.com, matthias.bgg@gmail.com, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, rdunlap@infradead.org, ye.xingchen@zte.com.cn, p.zabel@pengutronix.de, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, khilman@baylibre.com, james.lo@mediatek.com, rex-bc.chen@mediatek.com Subject: Re: [PATCH v12] thermal: drivers: mediatek: Add the Low Voltage Thermal Sensor driver Date: Wed, 1 Feb 2023 18:12:51 +0100 [thread overview] Message-ID: <8662c6bd-b32d-3d3d-b3b7-7f4aeb028309@linaro.org> (raw) In-Reply-To: <CAGuA+oqLiCxb1g7pwf+RwUTWHV37pXdAWUXHV51TnUy1-xUOXQ@mail.gmail.com> On 01/02/2023 17:46, Balsam CHIHI wrote: >>> +#ifdef CONFIG_MTK_LVTS_THERMAL_DEBUGFS >>> + >>> +static struct dentry *root; >> >> How do you handle two instances of driver? > > This root node is the topmost directory for debugfs called 'lvts', the > different driver instances are below this. It is a singleton. Indeed. What about removal? Aren't you remobing entire directory structure on first device removal? (...) >>> + >>> + of_property_for_each_string(np, "nvmem-cell-names", prop, cell_name) { >>> + size_t len; >>> + u8 *efuse; >>> + >>> + cell = of_nvmem_cell_get(np, cell_name); >>> + if (IS_ERR(cell)) { >>> + dev_dbg(dev, "Failed to get cell '%s'\n", cell_name); >> >> Is this an error? If so, why debug? dbg is not for errors. > > AFAIK using dev_dbg does not increase ELF size when DEBUG is disabled. > If this is not a good reason for you, then I will change it to dev_err. But also dev_dbg are not visible in error or warn level logs. If this is not an error, then indeed dev_dbg could be fine. But errors should be verbose. > >> >>> + return PTR_ERR(cell); >>> + } >>> + >>> + efuse = nvmem_cell_read(cell, &len); >>> + >>> + nvmem_cell_put(cell); >>> + >>> + if (IS_ERR(efuse)) { >>> + dev_dbg(dev, "Failed to read cell '%s'\n", cell_name); >>> + return PTR_ERR(efuse); >>> + } >>> + >>> + lvts_td->calib = devm_krealloc(dev, lvts_td->calib, >>> + lvts_td->calib_len + len, GFP_KERNEL); >>> + if (!lvts_td->calib) >>> + return -ENOMEM; >>> + >>> + memcpy(lvts_td->calib + lvts_td->calib_len, efuse, len); >>> + >>> + lvts_td->calib_len += len; >>> + >>> + kfree(efuse); >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static int __init lvts_golden_temp_init(struct device *dev, u32 *value) >> >> You did not test it, right? Build with section mismatch analysis... > > I'm not sure to fully understand this comment. > Would you explain, please? git grep -i "section mismatch" leads to lib/Kconfig.debug and DEBUG_SECTION_MISMATCH (...) >>> +static struct lvts_ctrl_data mt8195_lvts_data_ctrl[] = { >> >> Why this cannot be const? > > I've got the following warning when I added "const" > drivers/thermal/mediatek/lvts_thermal.c:1286:27: warning: > initialization discards ‘const’ qualifier from pointer target type > [-Wdiscarded-qualifiers] > 1286 | .lvts_ctrl = mt8195_lvts_data_ctrl, > | ^~~~~~~~~~~~~~~~~~~~~~~~~ As with every const... Do you need lvts_ctrl to be non-const? If yes, then how do you handle multiple devices (singleton)? Best regards, Krzysztof
WARNING: multiple messages have this Message-ID (diff)
From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> To: Balsam CHIHI <bchihi@baylibre.com> Cc: daniel.lezcano@linaro.org, angelogioacchino.delregno@collabora.com, rafael@kernel.org, amitk@kernel.org, rui.zhang@intel.com, matthias.bgg@gmail.com, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, rdunlap@infradead.org, ye.xingchen@zte.com.cn, p.zabel@pengutronix.de, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, khilman@baylibre.com, james.lo@mediatek.com, rex-bc.chen@mediatek.com Subject: Re: [PATCH v12] thermal: drivers: mediatek: Add the Low Voltage Thermal Sensor driver Date: Wed, 1 Feb 2023 18:12:51 +0100 [thread overview] Message-ID: <8662c6bd-b32d-3d3d-b3b7-7f4aeb028309@linaro.org> (raw) In-Reply-To: <CAGuA+oqLiCxb1g7pwf+RwUTWHV37pXdAWUXHV51TnUy1-xUOXQ@mail.gmail.com> On 01/02/2023 17:46, Balsam CHIHI wrote: >>> +#ifdef CONFIG_MTK_LVTS_THERMAL_DEBUGFS >>> + >>> +static struct dentry *root; >> >> How do you handle two instances of driver? > > This root node is the topmost directory for debugfs called 'lvts', the > different driver instances are below this. It is a singleton. Indeed. What about removal? Aren't you remobing entire directory structure on first device removal? (...) >>> + >>> + of_property_for_each_string(np, "nvmem-cell-names", prop, cell_name) { >>> + size_t len; >>> + u8 *efuse; >>> + >>> + cell = of_nvmem_cell_get(np, cell_name); >>> + if (IS_ERR(cell)) { >>> + dev_dbg(dev, "Failed to get cell '%s'\n", cell_name); >> >> Is this an error? If so, why debug? dbg is not for errors. > > AFAIK using dev_dbg does not increase ELF size when DEBUG is disabled. > If this is not a good reason for you, then I will change it to dev_err. But also dev_dbg are not visible in error or warn level logs. If this is not an error, then indeed dev_dbg could be fine. But errors should be verbose. > >> >>> + return PTR_ERR(cell); >>> + } >>> + >>> + efuse = nvmem_cell_read(cell, &len); >>> + >>> + nvmem_cell_put(cell); >>> + >>> + if (IS_ERR(efuse)) { >>> + dev_dbg(dev, "Failed to read cell '%s'\n", cell_name); >>> + return PTR_ERR(efuse); >>> + } >>> + >>> + lvts_td->calib = devm_krealloc(dev, lvts_td->calib, >>> + lvts_td->calib_len + len, GFP_KERNEL); >>> + if (!lvts_td->calib) >>> + return -ENOMEM; >>> + >>> + memcpy(lvts_td->calib + lvts_td->calib_len, efuse, len); >>> + >>> + lvts_td->calib_len += len; >>> + >>> + kfree(efuse); >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static int __init lvts_golden_temp_init(struct device *dev, u32 *value) >> >> You did not test it, right? Build with section mismatch analysis... > > I'm not sure to fully understand this comment. > Would you explain, please? git grep -i "section mismatch" leads to lib/Kconfig.debug and DEBUG_SECTION_MISMATCH (...) >>> +static struct lvts_ctrl_data mt8195_lvts_data_ctrl[] = { >> >> Why this cannot be const? > > I've got the following warning when I added "const" > drivers/thermal/mediatek/lvts_thermal.c:1286:27: warning: > initialization discards ‘const’ qualifier from pointer target type > [-Wdiscarded-qualifiers] > 1286 | .lvts_ctrl = mt8195_lvts_data_ctrl, > | ^~~~~~~~~~~~~~~~~~~~~~~~~ As with every const... Do you need lvts_ctrl to be non-const? If yes, then how do you handle multiple devices (singleton)? Best regards, Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-02-01 17:13 UTC|newest] Thread overview: 114+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-01-24 13:17 [PATCH v11 0/6] Add LVTS thermal architecture bchihi 2023-01-24 13:17 ` bchihi 2023-01-24 13:17 ` [PATCH v11 1/6] thermal/drivers/mediatek: Relocate driver to mediatek folder bchihi 2023-01-24 13:17 ` bchihi 2023-01-24 15:37 ` AngeloGioacchino Del Regno 2023-01-24 15:37 ` AngeloGioacchino Del Regno 2023-01-25 15:02 ` Balsam CHIHI 2023-01-25 15:02 ` Balsam CHIHI 2023-01-24 13:17 ` [PATCH v11 2/6] dt-bindings/thermal/mediatek: Add LVTS thermal controllers dt-binding definition bchihi 2023-01-24 13:17 ` bchihi 2023-01-25 11:14 ` Daniel Lezcano 2023-01-25 11:14 ` Daniel Lezcano 2023-01-25 20:35 ` Rob Herring 2023-01-25 20:35 ` Rob Herring 2023-01-25 21:13 ` Daniel Lezcano 2023-01-25 21:13 ` Daniel Lezcano 2023-01-25 20:34 ` Rob Herring 2023-01-25 20:34 ` Rob Herring 2023-01-26 10:33 ` Balsam CHIHI 2023-01-26 10:33 ` Balsam CHIHI 2023-01-26 16:10 ` [PATCH v12 2/6] dt-bindings: thermal: mediatek: " bchihi 2023-01-26 16:10 ` bchihi 2023-01-27 22:10 ` Daniel Lezcano 2023-01-27 22:10 ` Daniel Lezcano 2023-01-28 10:50 ` Krzysztof Kozlowski 2023-01-28 10:50 ` Krzysztof Kozlowski 2023-01-30 10:49 ` Balsam CHIHI 2023-01-30 10:49 ` Balsam CHIHI 2023-01-28 10:48 ` Krzysztof Kozlowski 2023-01-28 10:48 ` Krzysztof Kozlowski 2023-01-30 10:40 ` Balsam CHIHI 2023-01-30 10:40 ` Balsam CHIHI 2023-01-30 11:18 ` Matthias Brugger 2023-01-30 11:18 ` Matthias Brugger 2023-01-30 12:19 ` Balsam CHIHI 2023-01-30 12:19 ` Balsam CHIHI 2023-01-30 16:07 ` Matthias Brugger 2023-01-30 16:07 ` Matthias Brugger 2023-01-31 16:53 ` Krzysztof Kozlowski 2023-01-31 16:53 ` Krzysztof Kozlowski 2023-01-31 17:01 ` Daniel Lezcano 2023-01-31 17:01 ` Daniel Lezcano 2023-01-31 14:04 ` [PATCH v3] dt-bindings: thermal: mediatek: Add LVTS thermal controllers bchihi 2023-01-31 14:04 ` bchihi 2023-02-01 7:46 ` Krzysztof Kozlowski 2023-02-01 7:46 ` Krzysztof Kozlowski 2023-02-01 13:34 ` Balsam CHIHI 2023-02-01 13:34 ` Balsam CHIHI 2023-02-01 13:37 ` Krzysztof Kozlowski 2023-02-01 13:37 ` Krzysztof Kozlowski 2023-02-01 13:56 ` Balsam CHIHI 2023-02-01 13:56 ` Balsam CHIHI 2023-03-07 13:42 ` [PATCH] thermal/drivers/mediatek/lvts_thermal: fix memcpy's number of bytes in lvts_calibration_init() bchihi 2023-03-07 13:42 ` bchihi 2023-03-08 9:10 ` AngeloGioacchino Del Regno 2023-03-08 9:10 ` AngeloGioacchino Del Regno 2023-03-09 12:37 ` Dan Carpenter 2023-03-09 12:37 ` Dan Carpenter 2023-01-24 13:17 ` [PATCH v11 3/6] arm64/dts/mt8195: Add efuse node to mt8195 bchihi 2023-01-24 13:17 ` bchihi 2023-01-25 14:25 ` Matthias Brugger 2023-01-25 14:25 ` Matthias Brugger 2023-01-25 15:04 ` Balsam CHIHI 2023-01-25 15:04 ` Balsam CHIHI 2023-01-24 13:17 ` [PATCH v11 4/6] thermal/drivers/mediatek: Add the Low Voltage Thermal Sensor driver bchihi 2023-01-24 13:17 ` bchihi 2023-01-24 15:31 ` AngeloGioacchino Del Regno 2023-01-24 15:31 ` AngeloGioacchino Del Regno 2023-01-25 15:06 ` Balsam CHIHI 2023-01-25 15:06 ` Balsam CHIHI 2023-01-31 15:38 ` [PATCH v12] thermal: drivers: mediatek: " bchihi 2023-01-31 15:38 ` bchihi 2023-02-01 3:09 ` kernel test robot 2023-02-01 3:09 ` kernel test robot 2023-02-01 7:47 ` Krzysztof Kozlowski 2023-02-01 7:47 ` Krzysztof Kozlowski 2023-02-01 15:14 ` Balsam CHIHI 2023-02-01 15:14 ` Balsam CHIHI 2023-02-01 7:55 ` Krzysztof Kozlowski 2023-02-01 7:55 ` Krzysztof Kozlowski 2023-02-01 16:46 ` Balsam CHIHI 2023-02-01 16:46 ` Balsam CHIHI 2023-02-01 16:59 ` Matthias Brugger 2023-02-01 16:59 ` Matthias Brugger 2023-02-03 10:35 ` Balsam CHIHI 2023-02-03 10:35 ` Balsam CHIHI 2023-02-01 17:12 ` Krzysztof Kozlowski [this message] 2023-02-01 17:12 ` Krzysztof Kozlowski 2023-02-03 11:06 ` Balsam CHIHI 2023-02-03 11:06 ` Balsam CHIHI 2023-02-06 14:07 ` Daniel Lezcano 2023-02-06 14:07 ` Daniel Lezcano 2023-02-06 14:30 ` Krzysztof Kozlowski 2023-02-06 14:30 ` Krzysztof Kozlowski 2023-02-06 14:38 ` Daniel Lezcano 2023-02-06 14:38 ` Daniel Lezcano 2023-02-06 14:32 ` Balsam CHIHI 2023-02-06 14:32 ` Balsam CHIHI 2023-01-24 13:17 ` [PATCH v11 5/6] arm64/dts/mt8195: Add thermal zones and thermal nodes bchihi 2023-01-24 13:17 ` bchihi 2023-01-24 15:36 ` AngeloGioacchino Del Regno 2023-01-24 15:36 ` AngeloGioacchino Del Regno 2023-01-25 15:10 ` Balsam CHIHI 2023-01-25 15:10 ` Balsam CHIHI 2023-01-25 19:09 ` Matthias Brugger 2023-01-25 19:09 ` Matthias Brugger 2023-01-26 9:43 ` Balsam CHIHI 2023-01-26 9:43 ` Balsam CHIHI 2023-01-31 15:37 ` [PATCH v12] arm64: dts: mediatek: mt8195: " bchihi 2023-01-31 15:37 ` bchihi 2023-01-24 13:17 ` [PATCH v11 6/6] arm64/dts/mt8195: Add temperature mitigation threshold bchihi 2023-01-24 13:17 ` bchihi 2023-01-24 15:36 ` AngeloGioacchino Del Regno 2023-01-24 15:36 ` AngeloGioacchino Del Regno
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=8662c6bd-b32d-3d3d-b3b7-7f4aeb028309@linaro.org \ --to=krzysztof.kozlowski@linaro.org \ --cc=amitk@kernel.org \ --cc=angelogioacchino.delregno@collabora.com \ --cc=bchihi@baylibre.com \ --cc=daniel.lezcano@linaro.org \ --cc=devicetree@vger.kernel.org \ --cc=james.lo@mediatek.com \ --cc=khilman@baylibre.com \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mediatek@lists.infradead.org \ --cc=linux-pm@vger.kernel.org \ --cc=matthias.bgg@gmail.com \ --cc=p.zabel@pengutronix.de \ --cc=rafael@kernel.org \ --cc=rdunlap@infradead.org \ --cc=rex-bc.chen@mediatek.com \ --cc=robh+dt@kernel.org \ --cc=rui.zhang@intel.com \ --cc=ye.xingchen@zte.com.cn \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.