* [bug report] spi: Add Amlogic SPISG driver
@ 2025-08-01 13:02 Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2025-08-01 13:02 UTC (permalink / raw)
To: Sunny Luo; +Cc: linux-amlogic, linux-spi
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* [bug report] spi: Add Amlogic SPISG driver
@ 2025-08-01 13:04 Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2025-08-01 13:04 UTC (permalink / raw)
To: Sunny Luo; +Cc: linux-amlogic, linux-spi
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:764 aml_spisg_probe()
warn: missing unwind goto?
drivers/spi/spi-amlogic-spisg.c
716 static int aml_spisg_probe(struct platform_device *pdev)
717 {
718 struct spi_controller *ctlr;
719 struct spisg_device *spisg;
720 struct device *dev = &pdev->dev;
721 void __iomem *base;
722 int ret, irq;
723
724 const struct regmap_config aml_regmap_config = {
725 .reg_bits = 32,
726 .val_bits = 32,
727 .reg_stride = 4,
728 .max_register = SPISG_MAX_REG,
729 };
730
731 if (of_property_read_bool(dev->of_node, "spi-slave"))
732 ctlr = spi_alloc_target(dev, sizeof(*spisg));
733 else
734 ctlr = spi_alloc_host(dev, sizeof(*spisg));
735 if (!ctlr)
736 return dev_err_probe(dev, -ENOMEM, "controller allocation failed\n");
737
738 spisg = spi_controller_get_devdata(ctlr);
739 spisg->controller = ctlr;
740
741 spisg->pdev = pdev;
742 platform_set_drvdata(pdev, spisg);
743
744 base = devm_platform_ioremap_resource(pdev, 0);
745 if (IS_ERR(base))
746 return dev_err_probe(dev, PTR_ERR(base), "resource ioremap failed\n");
This should be goto out_controller
747
748 spisg->map = devm_regmap_init_mmio(dev, base, &aml_regmap_config);
749 if (IS_ERR(spisg->map))
750 return dev_err_probe(dev, PTR_ERR(spisg->map), "regmap init failed\n");
751
752 irq = platform_get_irq(pdev, 0);
753 if (irq < 0) {
754 ret = irq;
755 goto out_controller;
756 }
757
758 ret = device_reset_optional(dev);
759 if (ret)
760 return dev_err_probe(dev, ret, "reset dev failed\n");
Same
761
762 ret = aml_spisg_clk_init(spisg, base);
763 if (ret)
--> 764 return dev_err_probe(dev, ret, "clock init failed\n");
Same.
765
766 spisg->cfg_spi = 0;
767 spisg->cfg_start = 0;
768 spisg->cfg_bus = 0;
769
770 spisg->cfg_spi = FIELD_PREP(CFG_SFLASH_WP, 1) |
771 FIELD_PREP(CFG_SFLASH_HD, 1);
772 if (spi_controller_is_target(ctlr)) {
773 spisg->cfg_spi |= FIELD_PREP(CFG_SLAVE_EN, 1);
774 spisg->cfg_bus = FIELD_PREP(CFG_TX_TUNING, 0xf);
775 }
776 /* default pending */
777 spisg->cfg_start = FIELD_PREP(CFG_PEND, 1);
778
779 pm_runtime_set_active(&spisg->pdev->dev);
780 pm_runtime_enable(&spisg->pdev->dev);
781 pm_runtime_resume_and_get(&spisg->pdev->dev);
782
783 ctlr->num_chipselect = 4;
784 ctlr->dev.of_node = pdev->dev.of_node;
785 ctlr->mode_bits = SPI_CPHA | SPI_CPOL | SPI_LSB_FIRST |
786 SPI_3WIRE | SPI_TX_QUAD | SPI_RX_QUAD;
787 ctlr->max_speed_hz = 1000 * 1000 * 100;
788 ctlr->min_speed_hz = 1000 * 10;
789 ctlr->setup = aml_spisg_setup;
790 ctlr->cleanup = aml_spisg_cleanup;
791 ctlr->prepare_message = aml_spisg_prepare_message;
792 ctlr->transfer_one_message = aml_spisg_transfer_one_message;
793 ctlr->target_abort = aml_spisg_target_abort;
794 ctlr->can_dma = aml_spisg_can_dma;
795 ctlr->max_dma_len = SPISG_BLOCK_MAX;
796 ctlr->auto_runtime_pm = true;
797
798 dma_set_max_seg_size(&pdev->dev, SPISG_BLOCK_MAX);
799
800 ret = devm_request_irq(&pdev->dev, irq, aml_spisg_irq, 0, NULL, spisg);
801 if (ret) {
802 dev_err(&pdev->dev, "irq request failed\n");
803 goto out_clk;
804 }
805
806 ret = devm_spi_register_controller(dev, ctlr);
807 if (ret) {
808 dev_err(&pdev->dev, "spi controller registration failed\n");
809 goto out_clk;
810 }
811
812 init_completion(&spisg->completion);
813
814 pm_runtime_put(&spisg->pdev->dev);
815
816 return 0;
817 out_clk:
818 if (spisg->core)
819 clk_disable_unprepare(spisg->core);
820 clk_disable_unprepare(spisg->pclk);
821 out_controller:
822 spi_controller_put(ctlr);
823
824 return ret;
825 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* [bug report] spi: Add Amlogic SPISG driver
@ 2025-08-04 8:14 Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2025-08-04 8:14 UTC (permalink / raw)
To: Sunny Luo; +Cc: linux-amlogic, linux-spi
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:314 aml_spisg_setup_transfer()
error: we previously assumed 'exdesc' could be null (see line 261)
drivers/spi/spi-amlogic-spisg.c
248 static int aml_spisg_setup_transfer(struct spisg_device *spisg,
249 struct spi_transfer *xfer,
250 struct spisg_descriptor *desc,
251 struct spisg_descriptor_extra *exdesc)
252 {
253 int block_size, blocks;
254 struct device *dev = &spisg->pdev->dev;
255 struct spisg_sg_link *ccsg;
256 int ccsg_len;
257 dma_addr_t paddr;
258 int ret;
259
260 memset(desc, 0, sizeof(*desc));
261 if (exdesc)
^^^^^^
Checked for NULL
262 memset(exdesc, 0, sizeof(*exdesc));
263 aml_spisg_set_speed(spisg, xfer->speed_hz);
264 xfer->effective_speed_hz = spisg->effective_speed_hz;
265
266 desc->cfg_start = spisg->cfg_start;
267 desc->cfg_bus = spisg->cfg_bus;
268
269 block_size = xfer->bits_per_word >> 3;
270 blocks = xfer->len / block_size;
271
272 desc->cfg_start |= FIELD_PREP(CFG_EOC, 0);
273 desc->cfg_bus |= FIELD_PREP(CFG_KEEP_SS, !xfer->cs_change);
274 desc->cfg_bus |= FIELD_PREP(CFG_NULL_CTL, 0);
275
276 if (xfer->tx_buf || xfer->tx_dma) {
277 desc->cfg_bus |= FIELD_PREP(CFG_LANE, nbits_to_lane[xfer->tx_nbits]);
278 desc->cfg_start |= FIELD_PREP(CFG_OP_MODE, SPISG_OP_MODE_WRITE);
279 }
280 if (xfer->rx_buf || xfer->rx_dma) {
281 desc->cfg_bus |= FIELD_PREP(CFG_LANE, nbits_to_lane[xfer->rx_nbits]);
282 desc->cfg_start |= FIELD_PREP(CFG_OP_MODE, SPISG_OP_MODE_READ);
283 }
284
285 if (FIELD_GET(CFG_OP_MODE, desc->cfg_start) == SPISG_OP_MODE_READ_STS) {
286 desc->cfg_start |= FIELD_PREP(CFG_BLOCK_SIZE, blocks) |
287 FIELD_PREP(CFG_BLOCK_NUM, 1);
288 } else {
289 blocks = min_t(int, blocks, SPISG_BLOCK_MAX);
290 desc->cfg_start |= FIELD_PREP(CFG_BLOCK_SIZE, block_size & 0x7) |
291 FIELD_PREP(CFG_BLOCK_NUM, blocks);
292 }
293
294 if (xfer->tx_sg.nents && xfer->tx_sg.sgl) {
295 ccsg_len = xfer->tx_sg.nents * sizeof(struct spisg_sg_link);
296 ccsg = kzalloc(ccsg_len, GFP_KERNEL | GFP_DMA);
297 if (!ccsg) {
298 dev_err(dev, "alloc tx_ccsg failed\n");
299 return -ENOMEM;
300 }
301
302 aml_spisg_sg_xlate(&xfer->tx_sg, ccsg);
303 paddr = dma_map_single(dev, (void *)ccsg,
304 ccsg_len, DMA_TO_DEVICE);
305 ret = dma_mapping_error(dev, paddr);
306 if (ret) {
307 kfree(ccsg);
308 dev_err(dev, "tx ccsg map failed\n");
309 return ret;
310 }
311
312 desc->tx_paddr = paddr;
313 desc->cfg_start |= FIELD_PREP(CFG_TXD_MODE, SPISG_DATA_MODE_SG);
--> 314 exdesc->tx_ccsg = ccsg;
^^^^^^^^
Unchecked dereference
315 exdesc->tx_ccsg_len = ccsg_len;
316 dma_sync_sgtable_for_device(spisg->controller->cur_tx_dma_dev,
317 &xfer->tx_sg, DMA_TO_DEVICE);
318 } else if (xfer->tx_buf || xfer->tx_dma) {
319 paddr = xfer->tx_dma;
320 if (!paddr) {
321 paddr = dma_map_single(dev, (void *)xfer->tx_buf,
322 xfer->len, DMA_TO_DEVICE);
323 ret = dma_mapping_error(dev, paddr);
324 if (ret) {
325 dev_err(dev, "tx buf map failed\n");
326 return ret;
327 }
328 }
329 desc->tx_paddr = paddr;
330 desc->cfg_start |= FIELD_PREP(CFG_TXD_MODE, SPISG_DATA_MODE_MEM);
331 }
332
333 if (xfer->rx_sg.nents && xfer->rx_sg.sgl) {
334 ccsg_len = xfer->rx_sg.nents * sizeof(struct spisg_sg_link);
335 ccsg = kzalloc(ccsg_len, GFP_KERNEL | GFP_DMA);
336 if (!ccsg) {
337 dev_err(dev, "alloc rx_ccsg failed\n");
338 return -ENOMEM;
339 }
340
341 aml_spisg_sg_xlate(&xfer->rx_sg, ccsg);
342 paddr = dma_map_single(dev, (void *)ccsg,
343 ccsg_len, DMA_TO_DEVICE);
344 ret = dma_mapping_error(dev, paddr);
345 if (ret) {
346 kfree(ccsg);
347 dev_err(dev, "rx ccsg map failed\n");
348 return ret;
349 }
350
351 desc->rx_paddr = paddr;
352 desc->cfg_start |= FIELD_PREP(CFG_RXD_MODE, SPISG_DATA_MODE_SG);
353 exdesc->rx_ccsg = ccsg;
354 exdesc->rx_ccsg_len = ccsg_len;
355 dma_sync_sgtable_for_device(spisg->controller->cur_rx_dma_dev,
356 &xfer->rx_sg, DMA_FROM_DEVICE);
357 } else if (xfer->rx_buf || xfer->rx_dma) {
358 paddr = xfer->rx_dma;
359 if (!paddr) {
360 paddr = dma_map_single(dev, xfer->rx_buf,
361 xfer->len, DMA_FROM_DEVICE);
362 ret = dma_mapping_error(dev, paddr);
363 if (ret) {
364 dev_err(dev, "rx buf map failed\n");
365 return ret;
366 }
367 }
368
369 desc->rx_paddr = paddr;
370 desc->cfg_start |= FIELD_PREP(CFG_RXD_MODE, SPISG_DATA_MODE_MEM);
371 }
372
373 return 0;
374 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-08-04 8:14 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-01 13:04 [bug report] spi: Add Amlogic SPISG driver Dan Carpenter
-- strict thread matches above, loose matches on Subject: below --
2025-08-04 8:14 Dan Carpenter
2025-08-01 13:02 Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).