| Message ID | 20250225-psy-core-convert-to-fwnode-v1-1-d5e4369936bb@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | power: supply: core: convert to fwnode | expand |
Il 25/02/25 00:21, Sebastian Reichel ha scritto: > This removes .of_node from 'struct power_supply', since there > is already a copy in .dev.of_node and there is no need to have > two copies. > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
On 25/02/2025 01:21, Sebastian Reichel wrote: > This removes .of_node from 'struct power_supply', since there > is already a copy in .dev.of_node and there is no need to have > two copies. > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > --- > drivers/power/supply/power_supply_core.c | 17 ++++++++--------- > include/linux/power_supply.h | 1 - > 2 files changed, 8 insertions(+), 10 deletions(-) > > diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c > index d0bb52a7a0367a8e07787be211691cad14a41a54..11030035da6f121ca76bebf800c06cfd5db57578 100644 > --- a/drivers/power/supply/power_supply_core.c > +++ b/drivers/power/supply/power_supply_core.c > @@ -200,11 +200,11 @@ static int __power_supply_populate_supplied_from(struct power_supply *epsy, > int i = 0; > > do { > - np = of_parse_phandle(psy->of_node, "power-supplies", i++); > + np = of_parse_phandle(psy->dev.of_node, "power-supplies", i++); > if (!np) > break; > > - if (np == epsy->of_node) { > + if (np == epsy->dev.of_node) { > dev_dbg(&psy->dev, "%s: Found supply : %s\n", > psy->desc->name, epsy->desc->name); > psy->supplied_from[i-1] = (char *)epsy->desc->name; > @@ -235,7 +235,7 @@ static int __power_supply_find_supply_from_node(struct power_supply *epsy, > struct device_node *np = data; > > /* returning non-zero breaks out of power_supply_for_each_psy loop */ > - if (epsy->of_node == np) > + if (epsy->dev.of_node == np) > return 1; > > return 0; > @@ -270,13 +270,13 @@ static int power_supply_check_supplies(struct power_supply *psy) > return 0; > > /* No device node found, nothing to do */ > - if (!psy->of_node) > + if (!psy->dev.of_node) > return 0; > > do { > int ret; > > - np = of_parse_phandle(psy->of_node, "power-supplies", cnt++); > + np = of_parse_phandle(psy->dev.of_node, "power-supplies", cnt++); > if (!np) > break; > > @@ -606,8 +606,8 @@ int power_supply_get_battery_info(struct power_supply *psy, > const __be32 *list; > u32 min_max[2]; > > - if (psy->of_node) { > - battery_np = of_parse_phandle(psy->of_node, "monitored-battery", 0); > + if (psy->dev.of_node) { > + battery_np = of_parse_phandle(psy->dev.of_node, "monitored-battery", 0); > if (!battery_np) > return -ENODEV; This reminded me of a change I once did to power_supply - but maybe never got it further than RFC stage. Anyways, do you think it would be possible to decouple the battery info and struct power_suppply (while at it)? I believe that the chargers and especially fuel-gauges which are designed to operate with different batteries (and which get battery details using static battery nodes), would like to get the battery info _before_ registering the power_supply (to avoid sending bogus values while operating on defaults, before the battery info is read and before things are set accordingly). I know this may be a bit much to ask, but I believe it'd be an improvement. Other than that, looks good to me. > > @@ -1544,9 +1544,8 @@ __power_supply_register(struct device *parent, > if (cfg) { > dev->groups = cfg->attr_grp; > psy->drv_data = cfg->drv_data; > - psy->of_node = > + dev->of_node = > cfg->fwnode ? to_of_node(cfg->fwnode) : cfg->of_node; > - dev->of_node = psy->of_node; > psy->supplied_to = cfg->supplied_to; > psy->num_supplicants = cfg->num_supplicants; > } > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h > index 6ed53b292162469d7b357734d5589bff18a201d0..975ccab56597ef579ef0c9dc913dcb0a26b5855a 100644 > --- a/include/linux/power_supply.h > +++ b/include/linux/power_supply.h > @@ -316,7 +316,6 @@ struct power_supply { > > char **supplied_from; > size_t num_supplies; > - struct device_node *of_node; > > /* Driver private data */ > void *drv_data; >
Hi, On Tue, Feb 25, 2025 at 01:14:03PM +0200, Matti Vaittinen wrote: > On 25/02/2025 01:21, Sebastian Reichel wrote: > > This removes .of_node from 'struct power_supply', since there > > is already a copy in .dev.of_node and there is no need to have > > two copies. > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > > --- > > drivers/power/supply/power_supply_core.c | 17 ++++++++--------- > > include/linux/power_supply.h | 1 - > > 2 files changed, 8 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c > > index d0bb52a7a0367a8e07787be211691cad14a41a54..11030035da6f121ca76bebf800c06cfd5db57578 100644 > > --- a/drivers/power/supply/power_supply_core.c > > +++ b/drivers/power/supply/power_supply_core.c > > @@ -200,11 +200,11 @@ static int __power_supply_populate_supplied_from(struct power_supply *epsy, > > int i = 0; > > do { > > - np = of_parse_phandle(psy->of_node, "power-supplies", i++); > > + np = of_parse_phandle(psy->dev.of_node, "power-supplies", i++); > > if (!np) > > break; > > - if (np == epsy->of_node) { > > + if (np == epsy->dev.of_node) { > > dev_dbg(&psy->dev, "%s: Found supply : %s\n", > > psy->desc->name, epsy->desc->name); > > psy->supplied_from[i-1] = (char *)epsy->desc->name; > > @@ -235,7 +235,7 @@ static int __power_supply_find_supply_from_node(struct power_supply *epsy, > > struct device_node *np = data; > > /* returning non-zero breaks out of power_supply_for_each_psy loop */ > > - if (epsy->of_node == np) > > + if (epsy->dev.of_node == np) > > return 1; > > return 0; > > @@ -270,13 +270,13 @@ static int power_supply_check_supplies(struct power_supply *psy) > > return 0; > > /* No device node found, nothing to do */ > > - if (!psy->of_node) > > + if (!psy->dev.of_node) > > return 0; > > do { > > int ret; > > - np = of_parse_phandle(psy->of_node, "power-supplies", cnt++); > > + np = of_parse_phandle(psy->dev.of_node, "power-supplies", cnt++); > > if (!np) > > break; > > @@ -606,8 +606,8 @@ int power_supply_get_battery_info(struct power_supply *psy, > > const __be32 *list; > > u32 min_max[2]; > > - if (psy->of_node) { > > - battery_np = of_parse_phandle(psy->of_node, "monitored-battery", 0); > > + if (psy->dev.of_node) { > > + battery_np = of_parse_phandle(psy->dev.of_node, "monitored-battery", 0); > > if (!battery_np) > > return -ENODEV; > > This reminded me of a change I once did to power_supply - but maybe never > got it further than RFC stage. Anyways, do you think it would be possible to > decouple the battery info and struct power_suppply (while at it)? > > I believe that the chargers and especially fuel-gauges which are designed to > operate with different batteries (and which get battery details using static > battery nodes), would like to get the battery info _before_ registering the > power_supply (to avoid sending bogus values while operating on defaults, > before the battery info is read and before things are set accordingly). > > I know this may be a bit much to ask, but I believe it'd be an improvement. > > Other than that, looks good to me. I was thinking about adding an init function to power_supply_desc, which would be called directly before psy->initialized is set to true in the power-supply registration phase. I think that would be the right place to setup device registers based on battery-info data. But it's definitely not a thing for this series. Greetings, -- Sebastian
On 25/02/2025 15:11, Sebastian Reichel wrote: > Hi, > > On Tue, Feb 25, 2025 at 01:14:03PM +0200, Matti Vaittinen wrote: >> On 25/02/2025 01:21, Sebastian Reichel wrote: >>> This removes .of_node from 'struct power_supply', since there >>> is already a copy in .dev.of_node and there is no need to have >>> two copies. >>> >>> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> >>> --- >>> drivers/power/supply/power_supply_core.c | 17 ++++++++--------- >>> include/linux/power_supply.h | 1 - >>> 2 files changed, 8 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c >>> index d0bb52a7a0367a8e07787be211691cad14a41a54..11030035da6f121ca76bebf800c06cfd5db57578 100644 >>> --- a/drivers/power/supply/power_supply_core.c >>> +++ b/drivers/power/supply/power_supply_core.c >>> @@ -200,11 +200,11 @@ static int __power_supply_populate_supplied_from(struct power_supply *epsy, >>> int i = 0; >>> do { >>> - np = of_parse_phandle(psy->of_node, "power-supplies", i++); >>> + np = of_parse_phandle(psy->dev.of_node, "power-supplies", i++); >>> if (!np) >>> break; >>> - if (np == epsy->of_node) { >>> + if (np == epsy->dev.of_node) { >>> dev_dbg(&psy->dev, "%s: Found supply : %s\n", >>> psy->desc->name, epsy->desc->name); >>> psy->supplied_from[i-1] = (char *)epsy->desc->name; >>> @@ -235,7 +235,7 @@ static int __power_supply_find_supply_from_node(struct power_supply *epsy, >>> struct device_node *np = data; >>> /* returning non-zero breaks out of power_supply_for_each_psy loop */ >>> - if (epsy->of_node == np) >>> + if (epsy->dev.of_node == np) >>> return 1; >>> return 0; >>> @@ -270,13 +270,13 @@ static int power_supply_check_supplies(struct power_supply *psy) >>> return 0; >>> /* No device node found, nothing to do */ >>> - if (!psy->of_node) >>> + if (!psy->dev.of_node) >>> return 0; >>> do { >>> int ret; >>> - np = of_parse_phandle(psy->of_node, "power-supplies", cnt++); >>> + np = of_parse_phandle(psy->dev.of_node, "power-supplies", cnt++); >>> if (!np) >>> break; >>> @@ -606,8 +606,8 @@ int power_supply_get_battery_info(struct power_supply *psy, >>> const __be32 *list; >>> u32 min_max[2]; >>> - if (psy->of_node) { >>> - battery_np = of_parse_phandle(psy->of_node, "monitored-battery", 0); >>> + if (psy->dev.of_node) { >>> + battery_np = of_parse_phandle(psy->dev.of_node, "monitored-battery", 0); >>> if (!battery_np) >>> return -ENODEV; >> >> This reminded me of a change I once did to power_supply - but maybe never >> got it further than RFC stage. Anyways, do you think it would be possible to >> decouple the battery info and struct power_suppply (while at it)? >> >> I believe that the chargers and especially fuel-gauges which are designed to >> operate with different batteries (and which get battery details using static >> battery nodes), would like to get the battery info _before_ registering the >> power_supply (to avoid sending bogus values while operating on defaults, >> before the battery info is read and before things are set accordingly). >> >> I know this may be a bit much to ask, but I believe it'd be an improvement. >> >> Other than that, looks good to me. > > I was thinking about adding an init function to power_supply_desc, > which would be called directly before psy->initialized is set to > true in the power-supply registration phase. I think that would be > the right place to setup device registers based on battery-info data. Hm. I suppose that would work. > But it's definitely not a thing for this series. Fair enough. I thought that might be the case but decided to ask anyways, because, AFAIR there were no real problems what comes to not requiring the struct power_supply for reading the battery_info. But yes, I can see why killing the of_nodes is a big enough series, even without added complexity :) Yours, -- Matti
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c index d0bb52a7a0367a8e07787be211691cad14a41a54..11030035da6f121ca76bebf800c06cfd5db57578 100644 --- a/drivers/power/supply/power_supply_core.c +++ b/drivers/power/supply/power_supply_core.c @@ -200,11 +200,11 @@ static int __power_supply_populate_supplied_from(struct power_supply *epsy, int i = 0; do { - np = of_parse_phandle(psy->of_node, "power-supplies", i++); + np = of_parse_phandle(psy->dev.of_node, "power-supplies", i++); if (!np) break; - if (np == epsy->of_node) { + if (np == epsy->dev.of_node) { dev_dbg(&psy->dev, "%s: Found supply : %s\n", psy->desc->name, epsy->desc->name); psy->supplied_from[i-1] = (char *)epsy->desc->name; @@ -235,7 +235,7 @@ static int __power_supply_find_supply_from_node(struct power_supply *epsy, struct device_node *np = data; /* returning non-zero breaks out of power_supply_for_each_psy loop */ - if (epsy->of_node == np) + if (epsy->dev.of_node == np) return 1; return 0; @@ -270,13 +270,13 @@ static int power_supply_check_supplies(struct power_supply *psy) return 0; /* No device node found, nothing to do */ - if (!psy->of_node) + if (!psy->dev.of_node) return 0; do { int ret; - np = of_parse_phandle(psy->of_node, "power-supplies", cnt++); + np = of_parse_phandle(psy->dev.of_node, "power-supplies", cnt++); if (!np) break; @@ -606,8 +606,8 @@ int power_supply_get_battery_info(struct power_supply *psy, const __be32 *list; u32 min_max[2]; - if (psy->of_node) { - battery_np = of_parse_phandle(psy->of_node, "monitored-battery", 0); + if (psy->dev.of_node) { + battery_np = of_parse_phandle(psy->dev.of_node, "monitored-battery", 0); if (!battery_np) return -ENODEV; @@ -1544,9 +1544,8 @@ __power_supply_register(struct device *parent, if (cfg) { dev->groups = cfg->attr_grp; psy->drv_data = cfg->drv_data; - psy->of_node = + dev->of_node = cfg->fwnode ? to_of_node(cfg->fwnode) : cfg->of_node; - dev->of_node = psy->of_node; psy->supplied_to = cfg->supplied_to; psy->num_supplicants = cfg->num_supplicants; } diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h index 6ed53b292162469d7b357734d5589bff18a201d0..975ccab56597ef579ef0c9dc913dcb0a26b5855a 100644 --- a/include/linux/power_supply.h +++ b/include/linux/power_supply.h @@ -316,7 +316,6 @@ struct power_supply { char **supplied_from; size_t num_supplies; - struct device_node *of_node; /* Driver private data */ void *drv_data;
This removes .of_node from 'struct power_supply', since there is already a copy in .dev.of_node and there is no need to have two copies. Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> --- drivers/power/supply/power_supply_core.c | 17 ++++++++--------- include/linux/power_supply.h | 1 - 2 files changed, 8 insertions(+), 10 deletions(-)