diff mbox series

[v3,18/19] power: reset: syscon-poweroff: Move device data into a struct

Message ID 20240208170410.67975-19-afd@ti.com
State New
Headers
Series Remove pm_power_off use in drivers/power/reset |

Commit Message

Andrew Davis Feb. 8, 2024, 5:04 p.m. UTC 
  Currently all these device data elements are top level global variables.
Move these into a struct. This will be used in the next patch when
the global variable usage is removed. Doing this in two steps makes
the patches easier to read.

Signed-off-by: Andrew Davis <afd@ti.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/power/reset/syscon-poweroff.c | 36 +++++++++++++++------------
 1 file changed, 20 insertions(+), 16 deletions(-)

Comments

Sebastian Reichel Feb. 11, 2024, 11:52 p.m. UTC | #1 
Hi Andrew,

On Thu, Feb 08, 2024 at 11:04:09AM -0600, Andrew Davis wrote:
> Currently all these device data elements are top level global variables.
> Move these into a struct. This will be used in the next patch when
> the global variable usage is removed. Doing this in two steps makes
> the patches easier to read.
> 
> Signed-off-by: Andrew Davis <afd@ti.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/power/reset/syscon-poweroff.c | 36 +++++++++++++++------------
>  1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/power/reset/syscon-poweroff.c b/drivers/power/reset/syscon-poweroff.c
> index 1b2ce7734260c..4899a019256e8 100644
> --- a/drivers/power/reset/syscon-poweroff.c
> +++ b/drivers/power/reset/syscon-poweroff.c
> @@ -15,15 +15,19 @@
>  #include <linux/pm.h>
>  #include <linux/regmap.h>
>  
> -static struct regmap *map;
> -static u32 offset;
> -static u32 value;
> -static u32 mask;
> +struct syscon_poweroff_data {
> +	struct regmap *map;
> +	u32 offset;
> +	u32 value;
> +	u32 mask;
> +};
> +
> +static struct syscon_poweroff_data *data;

This patch is broken without the follow-up patch, since data is
never allocated. You need to move the memory allocation from the
next patch to this one.

Greetings,

-- Sebastian
Andrew Davis Feb. 12, 2024, 4:13 p.m. UTC | #2 
On 2/11/24 5:52 PM, Sebastian Reichel wrote:
> Hi Andrew,
> 
> On Thu, Feb 08, 2024 at 11:04:09AM -0600, Andrew Davis wrote:
>> Currently all these device data elements are top level global variables.
>> Move these into a struct. This will be used in the next patch when
>> the global variable usage is removed. Doing this in two steps makes
>> the patches easier to read.
>>
>> Signed-off-by: Andrew Davis <afd@ti.com>
>> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/power/reset/syscon-poweroff.c | 36 +++++++++++++++------------
>>   1 file changed, 20 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/power/reset/syscon-poweroff.c b/drivers/power/reset/syscon-poweroff.c
>> index 1b2ce7734260c..4899a019256e8 100644
>> --- a/drivers/power/reset/syscon-poweroff.c
>> +++ b/drivers/power/reset/syscon-poweroff.c
>> @@ -15,15 +15,19 @@
>>   #include <linux/pm.h>
>>   #include <linux/regmap.h>
>>   
>> -static struct regmap *map;
>> -static u32 offset;
>> -static u32 value;
>> -static u32 mask;
>> +struct syscon_poweroff_data {
>> +	struct regmap *map;
>> +	u32 offset;
>> +	u32 value;
>> +	u32 mask;
>> +};
>> +
>> +static struct syscon_poweroff_data *data;
> 
> This patch is broken without the follow-up patch, since data is
> never allocated. You need to move the memory allocation from the
> next patch to this one.

Ah, yes, seems I meant to make this struct point to a definition
not just a declaration. But just moving the allocation to this
patch seems to make this easier too, will do that, v4 on the way.

Thanks,
Andrew

> 
> Greetings,
> 
> -- Sebastian
diff mbox series

Patch

diff --git a/drivers/power/reset/syscon-poweroff.c b/drivers/power/reset/syscon-poweroff.c
index 1b2ce7734260c..4899a019256e8 100644
--- a/drivers/power/reset/syscon-poweroff.c
+++ b/drivers/power/reset/syscon-poweroff.c
@@ -15,15 +15,19 @@ 
 #include <linux/pm.h>
 #include <linux/regmap.h>
 
-static struct regmap *map;
-static u32 offset;
-static u32 value;
-static u32 mask;
+struct syscon_poweroff_data {
+	struct regmap *map;
+	u32 offset;
+	u32 value;
+	u32 mask;
+};
+
+static struct syscon_poweroff_data *data;
 
 static void syscon_poweroff(void)
 {
 	/* Issue the poweroff */
-	regmap_update_bits(map, offset, mask, value);
+	regmap_update_bits(data->map, data->offset, data->mask, data->value);
 
 	mdelay(1000);
 
@@ -35,22 +39,22 @@  static int syscon_poweroff_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int mask_err, value_err;
 
-	map = syscon_regmap_lookup_by_phandle(dev->of_node, "regmap");
-	if (IS_ERR(map)) {
-		map = syscon_node_to_regmap(dev->parent->of_node);
-		if (IS_ERR(map)) {
+	data->map = syscon_regmap_lookup_by_phandle(dev->of_node, "regmap");
+	if (IS_ERR(data->map)) {
+		data->map = syscon_node_to_regmap(dev->parent->of_node);
+		if (IS_ERR(data->map)) {
 			dev_err(dev, "unable to get syscon");
-			return PTR_ERR(map);
+			return PTR_ERR(data->map);
 		}
 	}
 
-	if (of_property_read_u32(dev->of_node, "offset", &offset)) {
+	if (of_property_read_u32(dev->of_node, "offset", &data->offset)) {
 		dev_err(dev, "unable to read 'offset'");
 		return -EINVAL;
 	}
 
-	value_err = of_property_read_u32(dev->of_node, "value", &value);
-	mask_err = of_property_read_u32(dev->of_node, "mask", &mask);
+	value_err = of_property_read_u32(dev->of_node, "value", &data->value);
+	mask_err = of_property_read_u32(dev->of_node, "mask", &data->mask);
 	if (value_err && mask_err) {
 		dev_err(dev, "unable to read 'value' and 'mask'");
 		return -EINVAL;
@@ -58,11 +62,11 @@  static int syscon_poweroff_probe(struct platform_device *pdev)
 
 	if (value_err) {
 		/* support old binding */
-		value = mask;
-		mask = 0xFFFFFFFF;
+		data->value = data->mask;
+		data->mask = 0xFFFFFFFF;
 	} else if (mask_err) {
 		/* support value without mask*/
-		mask = 0xFFFFFFFF;
+		data->mask = 0xFFFFFFFF;
 	}
 
 	if (pm_power_off) {