All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dan Carpenter <dan.carpenter@linaro.org>
To: Sunny Luo <sunny.luo@amlogic.com>
Cc: linux-amlogic@lists.infradead.org, linux-spi@vger.kernel.org
Subject: [bug report] spi: Add Amlogic SPISG driver
Date: Fri, 1 Aug 2025 16:02:26 +0300	[thread overview]
Message-ID: <aIy64j1zj316IF_h@stanley.mountain> (raw)

Hello Sunny Luo,

Commit cef9991e04ae ("spi: Add Amlogic SPISG driver") from Jul 18,
2025 (linux-next), leads to the following Smatch static checker
warning:

	drivers/spi/spi-amlogic-spisg.c:652 aml_spisg_clk_init()
	warn: passing zero to 'PTR_ERR'

drivers/spi/spi-amlogic-spisg.c
    640 static int aml_spisg_clk_init(struct spisg_device *spisg, void __iomem *base)
    641 {
    642         struct device *dev = &spisg->pdev->dev;
    643         struct clk_init_data init;
    644         struct clk_divider *div;
    645         struct clk_div_table *tbl;
    646         char name[32];
    647         int ret, i;
    648 
    649         spisg->core = devm_clk_get_enabled(dev, "core");
    650         if (IS_ERR_OR_NULL(spisg->core)) {

This should just be an IS_ERR() check...

    651                 dev_err(dev, "core clock request failed\n");

Otherwise, let's pretend that devm_clk_get_enabled() could really return
NULL because there was a bug in the Kconfig or somewhere else, then it
would need to be treated as success

--> 652                 return PTR_ERR(spisg->core);

This returns success, so I guess that would be fine except that really
it's not fine at all.

For more information see my blog:
https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/

    653         }
    654 
    655         spisg->pclk = devm_clk_get_enabled(dev, "pclk");
    656         if (IS_ERR_OR_NULL(spisg->pclk)) {
    657                 dev_err(dev, "pclk clock request failed\n");
    658                 return PTR_ERR(spisg->pclk);

Same.

    659         }
    660 
    661         clk_set_min_rate(spisg->pclk, SPISG_PCLK_RATE_MIN);
    662 
    663         clk_disable_unprepare(spisg->pclk);
    664 
    665         tbl = devm_kzalloc(dev, sizeof(struct clk_div_table) * (DIV_NUM + 1), GFP_KERNEL);
    666         if (!tbl)
    667                 return -ENOMEM;
    668 
    669         for (i = 0; i < DIV_NUM; i++) {
    670                 tbl[i].val = i + SPISG_CLK_DIV_MIN - 1;
    671                 tbl[i].div = i + SPISG_CLK_DIV_MIN;
    672         }
    673         spisg->tbl = tbl;
    674 
    675         div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
    676         if (!div)
    677                 return -ENOMEM;
    678 
    679         div->flags = CLK_DIVIDER_ROUND_CLOSEST;
    680         div->reg = base + SPISG_REG_CFG_BUS;
    681         div->shift = __bf_shf(CFG_CLK_DIV);
    682         div->width = CLK_DIV_WIDTH;
    683         div->table = tbl;
    684 
    685         /* Register value should not be outside of the table */
    686         regmap_update_bits(spisg->map, SPISG_REG_CFG_BUS, CFG_CLK_DIV,
    687                            FIELD_PREP(CFG_CLK_DIV, SPISG_CLK_DIV_MIN - 1));
    688 
    689         /* Register clk-divider */
    690         snprintf(name, sizeof(name), "%s_div", dev_name(dev));
    691         init.name = name;
    692         init.ops = &clk_divider_ops;
    693         init.flags = CLK_SET_RATE_PARENT;
    694         init.parent_data = &(const struct clk_parent_data) {
    695                                 .fw_name = "pclk",
    696                            };
    697         init.num_parents = 1;
    698         div->hw.init = &init;
    699         ret = devm_clk_hw_register(dev, &div->hw);
    700         if (ret) {
    701                 dev_err(dev, "clock registration failed\n");
    702                 return ret;
    703         }
    704 
    705         spisg->sclk = devm_clk_hw_get_clk(dev, &div->hw, NULL);
    706         if (IS_ERR_OR_NULL(spisg->sclk)) {

Same.

    707                 dev_err(dev, "get clock failed\n");
    708                 return PTR_ERR(spisg->sclk);
    709         }
    710 
    711         clk_prepare_enable(spisg->sclk);
    712 
    713         return 0;
    714 }

regards,
dan carpenter

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

WARNING: multiple messages have this Message-ID (diff)
From: Dan Carpenter <dan.carpenter@linaro.org>
To: Sunny Luo <sunny.luo@amlogic.com>
Cc: linux-amlogic@lists.infradead.org, linux-spi@vger.kernel.org
Subject: [bug report] spi: Add Amlogic SPISG driver
Date: Fri, 1 Aug 2025 16:02:26 +0300	[thread overview]
Message-ID: <aIy64j1zj316IF_h@stanley.mountain> (raw)

Hello Sunny Luo,

Commit cef9991e04ae ("spi: Add Amlogic SPISG driver") from Jul 18,
2025 (linux-next), leads to the following Smatch static checker
warning:

	drivers/spi/spi-amlogic-spisg.c:652 aml_spisg_clk_init()
	warn: passing zero to 'PTR_ERR'

drivers/spi/spi-amlogic-spisg.c
    640 static int aml_spisg_clk_init(struct spisg_device *spisg, void __iomem *base)
    641 {
    642         struct device *dev = &spisg->pdev->dev;
    643         struct clk_init_data init;
    644         struct clk_divider *div;
    645         struct clk_div_table *tbl;
    646         char name[32];
    647         int ret, i;
    648 
    649         spisg->core = devm_clk_get_enabled(dev, "core");
    650         if (IS_ERR_OR_NULL(spisg->core)) {

This should just be an IS_ERR() check...

    651                 dev_err(dev, "core clock request failed\n");

Otherwise, let's pretend that devm_clk_get_enabled() could really return
NULL because there was a bug in the Kconfig or somewhere else, then it
would need to be treated as success

--> 652                 return PTR_ERR(spisg->core);

This returns success, so I guess that would be fine except that really
it's not fine at all.

For more information see my blog:
https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/

    653         }
    654 
    655         spisg->pclk = devm_clk_get_enabled(dev, "pclk");
    656         if (IS_ERR_OR_NULL(spisg->pclk)) {
    657                 dev_err(dev, "pclk clock request failed\n");
    658                 return PTR_ERR(spisg->pclk);

Same.

    659         }
    660 
    661         clk_set_min_rate(spisg->pclk, SPISG_PCLK_RATE_MIN);
    662 
    663         clk_disable_unprepare(spisg->pclk);
    664 
    665         tbl = devm_kzalloc(dev, sizeof(struct clk_div_table) * (DIV_NUM + 1), GFP_KERNEL);
    666         if (!tbl)
    667                 return -ENOMEM;
    668 
    669         for (i = 0; i < DIV_NUM; i++) {
    670                 tbl[i].val = i + SPISG_CLK_DIV_MIN - 1;
    671                 tbl[i].div = i + SPISG_CLK_DIV_MIN;
    672         }
    673         spisg->tbl = tbl;
    674 
    675         div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
    676         if (!div)
    677                 return -ENOMEM;
    678 
    679         div->flags = CLK_DIVIDER_ROUND_CLOSEST;
    680         div->reg = base + SPISG_REG_CFG_BUS;
    681         div->shift = __bf_shf(CFG_CLK_DIV);
    682         div->width = CLK_DIV_WIDTH;
    683         div->table = tbl;
    684 
    685         /* Register value should not be outside of the table */
    686         regmap_update_bits(spisg->map, SPISG_REG_CFG_BUS, CFG_CLK_DIV,
    687                            FIELD_PREP(CFG_CLK_DIV, SPISG_CLK_DIV_MIN - 1));
    688 
    689         /* Register clk-divider */
    690         snprintf(name, sizeof(name), "%s_div", dev_name(dev));
    691         init.name = name;
    692         init.ops = &clk_divider_ops;
    693         init.flags = CLK_SET_RATE_PARENT;
    694         init.parent_data = &(const struct clk_parent_data) {
    695                                 .fw_name = "pclk",
    696                            };
    697         init.num_parents = 1;
    698         div->hw.init = &init;
    699         ret = devm_clk_hw_register(dev, &div->hw);
    700         if (ret) {
    701                 dev_err(dev, "clock registration failed\n");
    702                 return ret;
    703         }
    704 
    705         spisg->sclk = devm_clk_hw_get_clk(dev, &div->hw, NULL);
    706         if (IS_ERR_OR_NULL(spisg->sclk)) {

Same.

    707                 dev_err(dev, "get clock failed\n");
    708                 return PTR_ERR(spisg->sclk);
    709         }
    710 
    711         clk_prepare_enable(spisg->sclk);
    712 
    713         return 0;
    714 }

regards,
dan carpenter

             reply	other threads:[~2025-08-01 13:07 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-08-01 13:02 Dan Carpenter [this message]
2025-08-01 13:02 ` [bug report] spi: Add Amlogic SPISG driver Dan Carpenter
  -- strict thread matches above, loose matches on Subject: below --
2025-08-01 13:04 Dan Carpenter
2025-08-01 13:04 ` Dan Carpenter
2025-08-04  8:14 Dan Carpenter
2025-08-04  8:14 ` Dan Carpenter

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=aIy64j1zj316IF_h@stanley.mountain \
    --to=dan.carpenter@linaro.org \
    --cc=linux-amlogic@lists.infradead.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=sunny.luo@amlogic.com \
    /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: link
Be 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.