Message ID | 20240412060812.20412-1-raag.jadav@intel.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [v1] pwm: dwc: allow suspend/resume for 16 channels | expand |
Hello, On Fri, Apr 12, 2024 at 11:38:12AM +0530, Raag Jadav wrote: > diff --git a/drivers/pwm/pwm-dwc.h b/drivers/pwm/pwm-dwc.h > index a8b074841ae8..c6e2df5a6122 100644 > --- a/drivers/pwm/pwm-dwc.h > +++ b/drivers/pwm/pwm-dwc.h > @@ -38,6 +38,12 @@ struct dwc_pwm_info { > unsigned int size; > }; > > +struct dwc_pwm_drvdata { > + const struct dwc_pwm_info *info; > + void __iomem *io_base; .io_base is only used during init time and so doesn't need to be tracked in driver data. Otherwise I (only slightly) dislike > + struct dwc_pwm_drvdata *data; because "data" is very generic. I'd call it ddata. But I don't feel strong here. I'm happy if you change to "ddata" in v2, I will silently apply anyhow if you prefer "data". > + struct pwm_chip *chips[]; > +}; > + > struct dwc_pwm_ctx { > u32 cnt; > u32 cnt2; Best regards Uwe
On Fri, Apr 12, 2024 at 01:12:48PM +0200, Uwe Kleine-König wrote: > On Fri, Apr 12, 2024 at 11:38:12AM +0530, Raag Jadav wrote: ... > > +struct dwc_pwm_drvdata { > > + const struct dwc_pwm_info *info; > > + void __iomem *io_base; > > .io_base is only used during init time and so doesn't need to be tracked > in driver data. It's me who asked for this specific change and I think it's still beneficial as it allows to simplify the code.
On Fri, Apr 12, 2024 at 11:38:12AM +0530, Raag Jadav wrote: > With 16 channel pwm support, we're registering two instances of pwm_chip > with 8 channels each. We need to update PM functions to use both instances > of pwm_chip during power state transitions. > > Introduce struct dwc_pwm_drvdata and use it as driver_data, which will > maintain both instances of pwm_chip along with dwc_pwm_info and allow us > to use them inside suspend/resume handles. ... > + data = devm_kzalloc(dev, struct_size(data, chips, info->nr), GFP_KERNEL); > + if (!data) > + return dev_err_probe(dev, -ENOMEM, "Failed to allocate drvdata\n"); Haven't noticed before, but we do not print messages for -ENOMEM. Just return the code.
On Fri, Apr 12, 2024 at 01:12:48PM +0200, Uwe Kleine-König wrote: > Hello, > > On Fri, Apr 12, 2024 at 11:38:12AM +0530, Raag Jadav wrote: > > diff --git a/drivers/pwm/pwm-dwc.h b/drivers/pwm/pwm-dwc.h > > index a8b074841ae8..c6e2df5a6122 100644 > > --- a/drivers/pwm/pwm-dwc.h > > +++ b/drivers/pwm/pwm-dwc.h > > @@ -38,6 +38,12 @@ struct dwc_pwm_info { > > unsigned int size; > > }; > > > > +struct dwc_pwm_drvdata { > > + const struct dwc_pwm_info *info; > > + void __iomem *io_base; > > .io_base is only used during init time and so doesn't need to be tracked > in driver data. It will be useful for other features which are WIP. > Otherwise I (only slightly) dislike > > + struct dwc_pwm_drvdata *data; > because "data" is very generic. I'd call it ddata. But I don't feel > strong here. I'm happy if you change to "ddata" in v2, I will silently > apply anyhow if you prefer "data". I think "data" is more readable, something like "ddata" would make me re-adjust my glasses ;) Raag
On Fri, Apr 12, 2024 at 06:38:24PM +0300, Raag Jadav wrote: > On Fri, Apr 12, 2024 at 01:12:48PM +0200, Uwe Kleine-König wrote: > > On Fri, Apr 12, 2024 at 11:38:12AM +0530, Raag Jadav wrote: ... > > Otherwise I (only slightly) dislike > > > + struct dwc_pwm_drvdata *data; > > because "data" is very generic. I'd call it ddata. But I don't feel > > strong here. I'm happy if you change to "ddata" in v2, I will silently > > apply anyhow if you prefer "data". > > I think "data" is more readable, something like "ddata" would make me > re-adjust my glasses ;) ddata is _kinda_ idiomatic, there at least several drivers use this name (as of my knowledge). I am bending towards Uwe's suggestion here.
On Fri, Apr 12, 2024 at 06:45:48PM +0300, Andy Shevchenko wrote: > On Fri, Apr 12, 2024 at 06:38:24PM +0300, Raag Jadav wrote: > > On Fri, Apr 12, 2024 at 01:12:48PM +0200, Uwe Kleine-König wrote: > > > On Fri, Apr 12, 2024 at 11:38:12AM +0530, Raag Jadav wrote: > > ... > > > > Otherwise I (only slightly) dislike > > > > + struct dwc_pwm_drvdata *data; > > > because "data" is very generic. I'd call it ddata. But I don't feel > > > strong here. I'm happy if you change to "ddata" in v2, I will silently > > > apply anyhow if you prefer "data". > > > > I think "data" is more readable, something like "ddata" would make me > > re-adjust my glasses ;) > > ddata is _kinda_ idiomatic, there at least several drivers use this name > (as of my knowledge). I am bending towards Uwe's suggestion here. Will update in v2. Raag > > -- > With Best Regards, > Andy Shevchenko > >
On Fri, Apr 12, 2024 at 06:35:51PM +0300, Andy Shevchenko wrote: > On Fri, Apr 12, 2024 at 11:38:12AM +0530, Raag Jadav wrote: > > With 16 channel pwm support, we're registering two instances of pwm_chip > > with 8 channels each. We need to update PM functions to use both instances > > of pwm_chip during power state transitions. > > > > Introduce struct dwc_pwm_drvdata and use it as driver_data, which will > > maintain both instances of pwm_chip along with dwc_pwm_info and allow us > > to use them inside suspend/resume handles. > > ... > > > + data = devm_kzalloc(dev, struct_size(data, chips, info->nr), GFP_KERNEL); > > + if (!data) > > + return dev_err_probe(dev, -ENOMEM, "Failed to allocate drvdata\n"); > > Haven't noticed before, but we do not print messages for -ENOMEM. Just return > the code. Any special case for -ENOMEM? I found a few drivers which still do. Raag
On Fri, Apr 12, 2024 at 07:15:11PM +0300, Raag Jadav wrote: > On Fri, Apr 12, 2024 at 06:35:51PM +0300, Andy Shevchenko wrote: > > On Fri, Apr 12, 2024 at 11:38:12AM +0530, Raag Jadav wrote: ... > > > + data = devm_kzalloc(dev, struct_size(data, chips, info->nr), GFP_KERNEL); > > > + if (!data) > > > + return dev_err_probe(dev, -ENOMEM, "Failed to allocate drvdata\n"); > > > > Haven't noticed before, but we do not print messages for -ENOMEM. Just return > > the code. > > Any special case for -ENOMEM? Yes. But I have no link to the discussion at hand. Just believe me, or look into Git history for changes that drop that messaging. Perhaps one or two will give a hint where it starts from... > I found a few drivers which still do. They should be fixed.
Hello, On Fri, Apr 12, 2024 at 06:32:37PM +0300, Andy Shevchenko wrote: > On Fri, Apr 12, 2024 at 01:12:48PM +0200, Uwe Kleine-König wrote: > > On Fri, Apr 12, 2024 at 11:38:12AM +0530, Raag Jadav wrote: > > ... > > > > +struct dwc_pwm_drvdata { > > > + const struct dwc_pwm_info *info; > > > + void __iomem *io_base; > > > > .io_base is only used during init time and so doesn't need to be tracked > > in driver data. > > It's me who asked for this specific change and I think it's still beneficial > as it allows to simplify the code. Ok then, feel free to keep it for v2 then. Best regards Uwe
diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c index 043736972cb9..c8425493b95d 100644 --- a/drivers/pwm/pwm-dwc-core.c +++ b/drivers/pwm/pwm-dwc-core.c @@ -172,7 +172,6 @@ struct pwm_chip *dwc_pwm_alloc(struct device *dev) dwc->clk_ns = 10; chip->ops = &dwc_pwm_ops; - dev_set_drvdata(dev, chip); return chip; } EXPORT_SYMBOL_GPL(dwc_pwm_alloc); diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c index 676eaf8d7a53..1bf0ee4d3229 100644 --- a/drivers/pwm/pwm-dwc.c +++ b/drivers/pwm/pwm-dwc.c @@ -31,26 +31,34 @@ static const struct dwc_pwm_info ehl_pwm_info = { .size = 0x1000, }; -static int dwc_pwm_init_one(struct device *dev, void __iomem *base, unsigned int offset) +static int dwc_pwm_init_one(struct device *dev, struct dwc_pwm_drvdata *data, unsigned int idx) { struct pwm_chip *chip; struct dwc_pwm *dwc; + int ret; chip = dwc_pwm_alloc(dev); if (IS_ERR(chip)) return PTR_ERR(chip); dwc = to_dwc_pwm(chip); - dwc->base = base + offset; + dwc->base = data->io_base + (data->info->size * idx); - return devm_pwmchip_add(dev, chip); + ret = devm_pwmchip_add(dev, chip); + if (ret) + return ret; + + data->chips[idx] = chip; + return 0; } static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id) { const struct dwc_pwm_info *info; struct device *dev = &pci->dev; - int i, ret; + struct dwc_pwm_drvdata *data; + unsigned int idx; + int ret; ret = pcim_enable_device(pci); if (ret) @@ -63,17 +71,25 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id) return dev_err_probe(dev, ret, "Failed to iomap PCI BAR\n"); info = (const struct dwc_pwm_info *)id->driver_data; + data = devm_kzalloc(dev, struct_size(data, chips, info->nr), GFP_KERNEL); + if (!data) + return dev_err_probe(dev, -ENOMEM, "Failed to allocate drvdata\n"); - for (i = 0; i < info->nr; i++) { - /* - * No need to check for pcim_iomap_table() failure, - * pcim_iomap_regions() already does it for us. - */ - ret = dwc_pwm_init_one(dev, pcim_iomap_table(pci)[0], i * info->size); + /* + * No need to check for pcim_iomap_table() failure, + * pcim_iomap_regions() already does it for us. + */ + data->io_base = pcim_iomap_table(pci)[0]; + data->info = info; + + for (idx = 0; idx < data->info->nr; idx++) { + ret = dwc_pwm_init_one(dev, data, idx); if (ret) return ret; } + dev_set_drvdata(dev, data); + pm_runtime_put(dev); pm_runtime_allow(dev); @@ -88,19 +104,24 @@ static void dwc_pwm_remove(struct pci_dev *pci) static int dwc_pwm_suspend(struct device *dev) { - struct pwm_chip *chip = dev_get_drvdata(dev); - struct dwc_pwm *dwc = to_dwc_pwm(chip); - int i; + struct dwc_pwm_drvdata *data = dev_get_drvdata(dev); + unsigned int idx; - for (i = 0; i < DWC_TIMERS_TOTAL; i++) { - if (chip->pwms[i].state.enabled) { - dev_err(dev, "PWM %u in use by consumer (%s)\n", - i, chip->pwms[i].label); - return -EBUSY; + for (idx = 0; idx < data->info->nr; idx++) { + struct pwm_chip *chip = data->chips[idx]; + struct dwc_pwm *dwc = to_dwc_pwm(chip); + unsigned int i; + + for (i = 0; i < DWC_TIMERS_TOTAL; i++) { + if (chip->pwms[i].state.enabled) { + dev_err(dev, "PWM %u in use by consumer (%s)\n", + i, chip->pwms[i].label); + return -EBUSY; + } + dwc->ctx[i].cnt = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(i)); + dwc->ctx[i].cnt2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(i)); + dwc->ctx[i].ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(i)); } - dwc->ctx[i].cnt = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(i)); - dwc->ctx[i].cnt2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(i)); - dwc->ctx[i].ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(i)); } return 0; @@ -108,14 +129,19 @@ static int dwc_pwm_suspend(struct device *dev) static int dwc_pwm_resume(struct device *dev) { - struct pwm_chip *chip = dev_get_drvdata(dev); - struct dwc_pwm *dwc = to_dwc_pwm(chip); - int i; + struct dwc_pwm_drvdata *data = dev_get_drvdata(dev); + unsigned int idx; - for (i = 0; i < DWC_TIMERS_TOTAL; i++) { - dwc_pwm_writel(dwc, dwc->ctx[i].cnt, DWC_TIM_LD_CNT(i)); - dwc_pwm_writel(dwc, dwc->ctx[i].cnt2, DWC_TIM_LD_CNT2(i)); - dwc_pwm_writel(dwc, dwc->ctx[i].ctrl, DWC_TIM_CTRL(i)); + for (idx = 0; idx < data->info->nr; idx++) { + struct pwm_chip *chip = data->chips[idx]; + struct dwc_pwm *dwc = to_dwc_pwm(chip); + unsigned int i; + + for (i = 0; i < DWC_TIMERS_TOTAL; i++) { + dwc_pwm_writel(dwc, dwc->ctx[i].cnt, DWC_TIM_LD_CNT(i)); + dwc_pwm_writel(dwc, dwc->ctx[i].cnt2, DWC_TIM_LD_CNT2(i)); + dwc_pwm_writel(dwc, dwc->ctx[i].ctrl, DWC_TIM_CTRL(i)); + } } return 0; diff --git a/drivers/pwm/pwm-dwc.h b/drivers/pwm/pwm-dwc.h index a8b074841ae8..c6e2df5a6122 100644 --- a/drivers/pwm/pwm-dwc.h +++ b/drivers/pwm/pwm-dwc.h @@ -38,6 +38,12 @@ struct dwc_pwm_info { unsigned int size; }; +struct dwc_pwm_drvdata { + const struct dwc_pwm_info *info; + void __iomem *io_base; + struct pwm_chip *chips[]; +}; + struct dwc_pwm_ctx { u32 cnt; u32 cnt2;
With 16 channel pwm support, we're registering two instances of pwm_chip with 8 channels each. We need to update PM functions to use both instances of pwm_chip during power state transitions. Introduce struct dwc_pwm_drvdata and use it as driver_data, which will maintain both instances of pwm_chip along with dwc_pwm_info and allow us to use them inside suspend/resume handles. Fixes: ebf2c89eb95e ("pwm: dwc: Add 16 channel support for Intel Elkhart Lake") Signed-off-by: Raag Jadav <raag.jadav@intel.com> --- drivers/pwm/pwm-dwc-core.c | 1 - drivers/pwm/pwm-dwc.c | 82 +++++++++++++++++++++++++------------- drivers/pwm/pwm-dwc.h | 6 +++ 3 files changed, 60 insertions(+), 29 deletions(-)