[PATCH] staging: mfd: hi6421-spmi-pmic: fix error return code in hi6421_spmi_pmic_probe()
Fix to return a negative error code from the error handling
case instead of 0, as done elsewhere in this function.
Fixes: 4524ac56cdca ("staging: mfd: add a PMIC driver for HiSilicon 6421 SPMI
version")
Reported-by: Hulk Robot
Signed-off-by: Wang Hai
---
drivers/staging/hikey9xx/hi6421-spmi-pmic.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
b/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
index 64b30d263c8d..4f34a5282970 100644
--- a/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
+++ b/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
@@ -262,8 +262,10 @@ static int hi6421_spmi_pmic_probe(struct spmi_device *pdev)
hi6421_spmi_pmic_irq_prc(pmic);
pmic->irqs = devm_kzalloc(dev, HISI_IRQ_NUM * sizeof(int), GFP_KERNEL);
- if (!pmic->irqs)
+ if (!pmic->irqs) {
+ ret = -ENOMEM;
goto irq_malloc;
+ }
pmic->domain = irq_domain_add_simple(np, HISI_IRQ_NUM, 0,
&hi6421_spmi_domain_ops, pmic);
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: mfd: hi6421-spmi-pmic: fix error return code in hi6421_spmi_pmic_probe()
Not necessarily related to your patch but it should just return -ENOMEM
instead of the "goto irq_malloc;".
drivers/staging/hikey9xx/hi6421-spmi-pmic.c
251 if (!gpio_is_valid(pmic->gpio))
252 return -EINVAL;
253
254 ret = devm_gpio_request_one(dev, pmic->gpio, GPIOF_IN, "pmic");
255 if (ret < 0) {
256 dev_err(dev, "failed to request gpio%d\n", pmic->gpio);
257 return ret;
This is a direct return.
258 }
259
260 pmic->irq = gpio_to_irq(pmic->gpio);
[ Edit. Actually I can see that the original author must have thought
that this needed to be released but it doesn't. ]
261
262 hi6421_spmi_pmic_irq_prc(pmic);
263
264 pmic->irqs = devm_kzalloc(dev, HISI_IRQ_NUM * sizeof(int),
GFP_KERNEL);
265 if (!pmic->irqs) {
266 ret = -ENOMEM;
267 goto irq_malloc;
This is a goto with a ComeFrom style label name, which says where it
is called from (The goto is at the place where irq_malloc fails). This
is a useless label name because we can see from the line before that
the alloc failed. What we want to know is what the goto does!
268 }
269
270 pmic->domain = irq_domain_add_simple(np, HISI_IRQ_NUM, 0,
271 &hi6421_spmi_domain_ops,
pmic);
272 if (!pmic->domain) {
273 dev_err(dev, "failed irq domain add simple!\n");
274 ret = -ENODEV;
275 goto irq_malloc;
Here the label name is even more useless here because "irq_malloc"
didn't fail on the line before. #Confusing But we still don't know
what the goto does.
If we scroll down then we see that "goto irq_malloc" releases the IRQ.
A better name would be "goto err_irq;"
276 }
277
278 for (i = 0; i < HISI_IRQ_NUM; i++) {
279 virq = irq_create_mapping(pmic->domain, i);
280 if (!virq) {
281 dev_err(dev, "Failed mapping hwirq\n");
282 ret = -ENOSPC;
283 goto irq_malloc;
284 }
285 pmic->irqs[i] = virq;
286 dev_dbg(dev, "%s: pmic->irqs[%d] = %d\n",
287 __func__, i, pmic->irqs[i]);
288 }
289
290 ret = request_threaded_irq(pmic->irq, hi6421_spmi_irq_handler,
NULL,
291 IRQF_TRIGGER_LOW | IRQF_SHARED |
IRQF_NO_SUSPEND,
292 "pmic", pmic);
Except it turns out that we don't actually request the IRQ until this
line. So those earlier "goto err_irq;" things are bogus.
293 if (ret < 0) {
294 dev_err(dev, "could not claim pmic IRQ: error %d\n",
ret);
295 goto irq_malloc;
296 }
297
298 dev_set_drvdata(&pdev->dev, pmic);
299
300 /*
301 * The logic below will rely that the pmic is already stored at
302 * drvdata.
303 */
304 dev_dbg(&pdev->dev, "SPMI-PMIC: adding children for %pOF\n",
305 pdev->dev.of_node);
306 ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
307 hi6421v600_devs,
ARRAY_SIZE(hi6421v600_devs),
308 NULL, 0, NULL);
309 if (!ret)
310 return 0;
This is "success handling" anti-pattern and "last condition is weird"
anti-pattern. We should always do failure handling. The code should
look like:
success();
success();
success();
success();
if () {
failure();
failure();
failure();
}
success();
success();
if () {
failure();
failure();
failure();
}
Failure is indented twice and success once.
311
312 dev_err(dev, "Failed to add child devices: %d\n", ret);
313
314 irq_malloc:
315 free_irq(pmic->irq, pmic);
This free should only be done if devm_mfd_add_devices() fails. I don't
know what happens if you free an IRQ which has not been requested. I
think it triggers a WARN().
316
317 return ret;
318 }
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] media: atomisp: Fixed error handling path
On Wed, Nov 4, 2020 at 7:32 AM Souptick Joarder wrote:
>
> Inside alloc_user_pages() based on flag value either pin_user_pages()
> or get_user_pages_fast() will be called. However, these API might fail.
>
> But free_user_pages() called in error handling path doesn't bother
> about return value and will try to unpin bo->pgnr pages, which is
> incorrect.
>
> Fix this by passing the page_nr to free_user_pages(). If page_nr > 0
> pages will be unpinned based on bo->mem_type. This will also take care
> of non error handling path.
>
> Fixes: 14a638ab96c5 ("media: atomisp: use pin_user_pages() for memory
> allocation")
> Signed-off-by: Souptick Joarder
> Reviewed-by: Dan Carpenter
> Cc: John Hubbard
> Cc: Ira Weiny
> Cc: Dan Carpenter
> ---
> v2:
> Added review tag.
Any further comment ? If no, can we get this patch in queue for 5.11 ?
>
> drivers/staging/media/atomisp/pci/hmm/hmm_bo.c | 13 -
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> index f13af23..0168f98 100644
> --- a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> +++ b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> @@ -857,16 +857,17 @@ static void free_private_pages(struct hmm_buffer_object
> *bo,
> kfree(bo->page_obj);
> }
>
> -static void free_user_pages(struct hmm_buffer_object *bo)
> +static void free_user_pages(struct hmm_buffer_object *bo,
> + unsigned int page_nr)
> {
> int i;
>
> hmm_mem_stat.usr_size -= bo->pgnr;
>
> if (bo->mem_type == HMM_BO_MEM_TYPE_PFN) {
> - unpin_user_pages(bo->pages, bo->pgnr);
> + unpin_user_pages(bo->pages, page_nr);
> } else {
> - for (i = 0; i < bo->pgnr; i++)
> + for (i = 0; i < page_nr; i++)
> put_page(bo->pages[i]);
> }
> kfree(bo->pages);
> @@ -942,6 +943,8 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
> dev_err(atomisp_dev,
> "get_user_pages err: bo->pgnr = %d, pgnr actually
> pinned = %d.\n",
> bo->pgnr, page_nr);
> + if (page_nr < 0)
> + page_nr = 0;
> goto out_of_mem;
> }
>
> @@ -954,7 +957,7 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
>
> out_of_mem:
>
> - free_user_pages(bo);
> + free_user_pages(bo, page_nr);
>
> return -ENOMEM;
> }
> @@ -1037,7 +1040,7 @@ void hmm_bo_free_pages(struct hmm_buffer_object *bo)
> if (bo->type == HMM_BO_PRIVATE)
> free_private_pages(bo, &dynamic_pool, &reserved_pool);
> else if (bo->type == HMM_BO_USER)
> - free_user_pages(bo);
> + free_user_pages(bo, bo->pgnr);
> else
> dev_err(atomisp_dev, "invalid buffer type.\n");
> mutex_unlock(&bo->mutex);
> --
> 1.9.1
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 2/4] phy: ralink: Add PHY driver for MT7621 PCIe PHY
On 31-10-20, 13:22, Sergio Paracuellos wrote:
> +#define RG_PE1_PIPE_REG 0x02c
> +#define RG_PE1_PIPE_RST BIT(12)
> +#define RG_PE1_PIPE_CMD_FRC BIT(4)
> +
> +#define RG_P0_TO_P1_WIDTH0x100
> +#define RG_PE1_H_LCDDS_REG 0x49c
> +#define RG_PE1_H_LCDDS_PCW GENMASK(30, 0)
> +#define RG_PE1_H_LCDDS_PCW_VAL(x)((0x7fff & (x)) << 0)
Pls use FIELD_{GET|PREP} instead of coding like this, you already
defined the mask, use it to set and get the reg field ;)
> +
> +#define RG_PE1_FRC_H_XTAL_REG0x400
> +#define RG_PE1_FRC_H_XTAL_TYPE BIT(8)
> +#define RG_PE1_H_XTAL_TYPE GENMASK(10, 9)
> +#define RG_PE1_H_XTAL_TYPE_VAL(x)((0x3 & (x)) << 9)
> +
> +#define RG_PE1_FRC_PHY_REG 0x000
> +#define RG_PE1_FRC_PHY_ENBIT(4)
> +#define RG_PE1_PHY_ENBIT(5)
> +
> +#define RG_PE1_H_PLL_REG 0x490
> +#define RG_PE1_H_PLL_BC GENMASK(23, 22)
> +#define RG_PE1_H_PLL_BC_VAL(x) ((0x3 & (x)) << 22)
> +#define RG_PE1_H_PLL_BP GENMASK(21, 18)
> +#define RG_PE1_H_PLL_BP_VAL(x) ((0xf & (x)) << 18)
> +#define RG_PE1_H_PLL_IR GENMASK(15, 12)
> +#define RG_PE1_H_PLL_IR_VAL(x) ((0xf & (x)) << 12)
> +#define RG_PE1_H_PLL_IC GENMASK(11, 8)
> +#define RG_PE1_H_PLL_IC_VAL(x) ((0xf & (x)) << 8)
> +#define RG_PE1_H_PLL_PREDIV GENMASK(7, 6)
> +#define RG_PE1_H_PLL_PREDIV_VAL(x) ((0x3 & (x)) << 6)
> +#define RG_PE1_PLL_DIVEN GENMASK(3, 1)
> +#define RG_PE1_PLL_DIVEN_VAL(x) ((0x7 & (x)) << 1)
> +
> +#define RG_PE1_H_PLL_FBKSEL_REG 0x4bc
> +#define RG_PE1_H_PLL_FBKSEL GENMASK(5, 4)
> +#define RG_PE1_H_PLL_FBKSEL_VAL(x) ((0x3 & (x)) << 4)
> +
> +#define RG_PE1_H_LCDDS_SSC_PRD_REG 0x4a4
> +#define RG_PE1_H_LCDDS_SSC_PRD GENMASK(15, 0)
> +#define RG_PE1_H_LCDDS_SSC_PRD_VAL(x)((0x & (x)) << 0)
> +
> +#define RG_PE1_H_LCDDS_SSC_DELTA_REG 0x4a8
> +#define RG_PE1_H_LCDDS_SSC_DELTA GENMASK(11, 0)
> +#define RG_PE1_H_LCDDS_SSC_DELTA_VAL(x) ((0xfff & (x)) << 0)
> +#define RG_PE1_H_LCDDS_SSC_DELTA1GENMASK(27, 16)
> +#define RG_PE1_H_LCDDS_SSC_DELTA1_VAL(x) ((0xff & (x)) << 16)
> +
> +#define RG_PE1_LCDDS_CLK_PH_INV_REG 0x4a0
> +#define RG_PE1_LCDDS_CLK_PH_INV BIT(5)
> +
> +#define RG_PE1_H_PLL_BR_REG 0x4ac
> +#define RG_PE1_H_PLL_BR GENMASK(18, 16)
> +#define RG_PE1_H_PLL_BR_VAL(x) ((0x7 & (x)) << 16)
> +
> +#define RG_PE1_MSTCKDIV_REG 0x414
> +#define RG_PE1_MSTCKDIV GENMASK(7, 6)
> +#define RG_PE1_MSTCKDIV_VAL(x) ((0x3 & (x)) << 6)
> +
> +#define RG_PE1_FRC_MSTCKDIV BIT(5)
> +
> +#define XTAL_MODE_SEL_SHIFT 6
Bonus you dont need to define shifts if you use stuff defined in
bitfield.h
> +struct mt7621_pci_phy {
> + struct device *dev;
> + struct regmap *regmap;
> + struct phy *phy;
> + void __iomem *port_base;
> + bool has_dual_port;
> + bool bypass_pipe_rst;
> +};
> +
> +static inline u32 phy_read(struct mt7621_pci_phy *phy, u32 reg)
> +{
> + u32 val;
> +
> + regmap_read(phy->regmap, reg, &val);
> +
> + return val;
> +}
> +
> +static inline void phy_write(struct mt7621_pci_phy *phy, u32 val, u32 reg)
> +{
> + regmap_write(phy->regmap, reg, val);
Why not use regmap_ calls directly and avoid the dummy wrappers..?
> +}
> +
> +static inline void mt7621_phy_rmw(struct mt7621_pci_phy *phy,
> + u32 reg, u32 clr, u32 set)
> +{
> + u32 val = phy_read(phy, reg);
> +
> + val &= ~clr;
> + val |= set;
> + phy_write(phy, val, reg);
why not use regmap_update_bits() instead
> +static void mt7621_set_phy_for_ssc(struct mt7621_pci_phy *phy)
> +{
> + struct device *dev = phy->dev;
> + u32 xtal_mode;
> +
> + xtal_mode = (rt_sysc_r32(SYSC_REG_SYSTEM_CONFIG0)
> + >> XTAL_MODE_SEL_SHIFT) & XTAL_MODE_SEL_MASK;
> +
> + /* Set PCIe Port PHY to disable SSC */
> + /* Debug Xtal Type */
> + mt7621_phy_rmw(phy, RG_PE1_FRC_H_XTAL_REG,
> +RG_PE1_FRC_H_XTAL_TYPE | RG_PE1_H_XTAL_TYPE,
> +RG_PE1_FRC_H_XTAL_TYPE | RG_PE1_H_XTAL_TYPE_VAL(0x00));
> +
> + /* disable port */
> + mt7621_phy_rmw(phy, RG_PE1_FRC_PHY_REG,
> +RG_PE1_PHY_EN, RG_PE1_FRC_PHY_EN);
> +
> + if (phy-
Re: [PATCH v4 2/4] phy: ralink: Add PHY driver for MT7621 PCIe PHY
Hi Vinod,
Thanks for the review.
On Thu, Nov 19, 2020 at 6:31 AM Vinod Koul wrote:
>
> On 31-10-20, 13:22, Sergio Paracuellos wrote:
>
> > +#define RG_PE1_PIPE_REG 0x02c
> > +#define RG_PE1_PIPE_RST BIT(12)
> > +#define RG_PE1_PIPE_CMD_FRC BIT(4)
> > +
> > +#define RG_P0_TO_P1_WIDTH0x100
> > +#define RG_PE1_H_LCDDS_REG 0x49c
> > +#define RG_PE1_H_LCDDS_PCW GENMASK(30, 0)
> > +#define RG_PE1_H_LCDDS_PCW_VAL(x)((0x7fff & (x)) << 0)
>
> Pls use FIELD_{GET|PREP} instead of coding like this, you already
> defined the mask, use it to set and get the reg field ;)
Will change this and properly remove all of these *_VAL defines.
>
> > +
> > +#define RG_PE1_FRC_H_XTAL_REG0x400
> > +#define RG_PE1_FRC_H_XTAL_TYPE BIT(8)
> > +#define RG_PE1_H_XTAL_TYPE GENMASK(10, 9)
> > +#define RG_PE1_H_XTAL_TYPE_VAL(x)((0x3 & (x)) << 9)
> > +
> > +#define RG_PE1_FRC_PHY_REG 0x000
> > +#define RG_PE1_FRC_PHY_ENBIT(4)
> > +#define RG_PE1_PHY_ENBIT(5)
> > +
> > +#define RG_PE1_H_PLL_REG 0x490
> > +#define RG_PE1_H_PLL_BC GENMASK(23, 22)
> > +#define RG_PE1_H_PLL_BC_VAL(x) ((0x3 & (x)) << 22)
> > +#define RG_PE1_H_PLL_BP GENMASK(21, 18)
> > +#define RG_PE1_H_PLL_BP_VAL(x) ((0xf & (x)) << 18)
> > +#define RG_PE1_H_PLL_IR GENMASK(15, 12)
> > +#define RG_PE1_H_PLL_IR_VAL(x) ((0xf & (x)) << 12)
> > +#define RG_PE1_H_PLL_IC GENMASK(11, 8)
> > +#define RG_PE1_H_PLL_IC_VAL(x) ((0xf & (x)) << 8)
> > +#define RG_PE1_H_PLL_PREDIV GENMASK(7, 6)
> > +#define RG_PE1_H_PLL_PREDIV_VAL(x) ((0x3 & (x)) << 6)
> > +#define RG_PE1_PLL_DIVEN GENMASK(3, 1)
> > +#define RG_PE1_PLL_DIVEN_VAL(x) ((0x7 & (x)) << 1)
> > +
> > +#define RG_PE1_H_PLL_FBKSEL_REG 0x4bc
> > +#define RG_PE1_H_PLL_FBKSEL GENMASK(5, 4)
> > +#define RG_PE1_H_PLL_FBKSEL_VAL(x) ((0x3 & (x)) << 4)
> > +
> > +#define RG_PE1_H_LCDDS_SSC_PRD_REG 0x4a4
> > +#define RG_PE1_H_LCDDS_SSC_PRD GENMASK(15, 0)
> > +#define RG_PE1_H_LCDDS_SSC_PRD_VAL(x)((0x & (x)) << 0)
> > +
> > +#define RG_PE1_H_LCDDS_SSC_DELTA_REG 0x4a8
> > +#define RG_PE1_H_LCDDS_SSC_DELTA GENMASK(11, 0)
> > +#define RG_PE1_H_LCDDS_SSC_DELTA_VAL(x) ((0xfff & (x)) << 0)
> > +#define RG_PE1_H_LCDDS_SSC_DELTA1GENMASK(27, 16)
> > +#define RG_PE1_H_LCDDS_SSC_DELTA1_VAL(x) ((0xff & (x)) << 16)
> > +
> > +#define RG_PE1_LCDDS_CLK_PH_INV_REG 0x4a0
> > +#define RG_PE1_LCDDS_CLK_PH_INV BIT(5)
> > +
> > +#define RG_PE1_H_PLL_BR_REG 0x4ac
> > +#define RG_PE1_H_PLL_BR GENMASK(18, 16)
> > +#define RG_PE1_H_PLL_BR_VAL(x) ((0x7 & (x)) << 16)
> > +
> > +#define RG_PE1_MSTCKDIV_REG 0x414
> > +#define RG_PE1_MSTCKDIV GENMASK(7, 6)
> > +#define RG_PE1_MSTCKDIV_VAL(x) ((0x3 & (x)) << 6)
> > +
> > +#define RG_PE1_FRC_MSTCKDIV BIT(5)
> > +
> > +#define XTAL_MODE_SEL_SHIFT 6
> Bonus you dont need to define shifts if you use stuff defined in
> bitfield.h
I will also check how to remove this SHIFT.
>
> > +struct mt7621_pci_phy {
> > + struct device *dev;
> > + struct regmap *regmap;
> > + struct phy *phy;
> > + void __iomem *port_base;
> > + bool has_dual_port;
> > + bool bypass_pipe_rst;
> > +};
> > +
> > +static inline u32 phy_read(struct mt7621_pci_phy *phy, u32 reg)
> > +{
> > + u32 val;
> > +
> > + regmap_read(phy->regmap, reg, &val);
> > +
> > + return val;
> > +}
> > +
> > +static inline void phy_write(struct mt7621_pci_phy *phy, u32 val, u32 reg)
> > +{
> > + regmap_write(phy->regmap, reg, val);
>
> Why not use regmap_ calls directly and avoid the dummy wrappers..?
This is because name was the dummy names are a bit shorter :) but if
it is also necessary I will use directly regmap_ functions.
>
> > +}
> > +
> > +static inline void mt7621_phy_rmw(struct mt7621_pci_phy *phy,
> > + u32 reg, u32 clr, u32 set)
> > +{
> > + u32 val = phy_read(phy, reg);
> > +
> > + val &= ~clr;
> > + val |= set;
> > + phy_write(phy, val, reg);
>
> why not use regmap_update_bits() instead
True. Will change.
>
> > +static void mt7621_set_phy_for_ssc(struct mt7621_pci_phy *phy)
> > +{
> > + struct device *dev = phy->dev;
> > + u32 xtal_mode;
>
