TS-9370 initial support#326
Conversation
markfeathers
left a comment
There was a problem hiding this comment.
it looks like 6.18 added new checkpatch updates, or some of these may not have been met originally. For linux-lts though, we generally try to keep that passing with the exception:
DT compatible string "XX" appears un-documented
Those will be fixed when we prep for upstream.
Can you review / fixup the remaining checkpatch warnings? Eg:
scripts/checkpatch.pl --git 17cd437bfe05e601ba586d5e370eefa0ffb098aa| ret = regmap_read(data->regmap, SILO_RESERVED0, &version); | ||
| if (ret < 0) | ||
| return ret; | ||
| dev_info(dev, "TS-SILO version %d\n", version); | ||
| if (version < 2) | ||
| dev_warn(dev, "POWER_FAIL ignored without a Wizard interrupt controller.\n"); |
There was a problem hiding this comment.
This should be removed, this version register wasn't being updated, and we shouldn't keep using it when its listed as a reserved register.
There was a problem hiding this comment.
I'd suggest using ./scripts/checkpatch.pl --git fece377^..HEAD to run checkpatch against every commit here rather than just the HEAD 17cd437. There are a decent amount of warnings and errors that pop up overall. Many can be ignored though, probably.
There was a problem hiding this comment.
The checkpatch warnings/errors I left in:
- MAINTAINERS file needs to be updated
- script wanted 4 lines of help text for Kconfig options
- device tree missing docs
- There were errors related to wanting () around the pinfunc.h macros, but those are used in the dts file which wouldn't like the (). I'm surprised checkpatch doesn't have a pincfunc override.
There was a problem hiding this comment.
There were errors related to wanting () around the pinfunc.h macros, but those are used in the dts file which wouldn't like the (). I'm surprised checkpatch doesn't have a pincfunc override.
Yeah, it seems like just something they are dealing with. There may be a day when that comes, but today is not that day.
Everything else you mentioned makes sense and is something to pay closer attention to when upstreaming, case by case.
NXP FlexSPI(FSPI) BUS controller driver. Signed-off-by: Aaron Brice <[email protected]>
Support for FPGA IRQ controller on TS-9370 board. Signed-off-by: Aaron Brice <[email protected]>
Adding pinctrl drivers for ts9370, ts9390 and tsxbar. Signed-off-by: Aaron Brice <[email protected]>
TS-9370 expanded GPIO support. Signed-off-by: Aaron Brice <[email protected]>
Interrupts from embeddedTS wizard microcontroller. Signed-off-by: Aaron Brice <[email protected]>
Add single channel ADC121S021 ADC chip support to the TI ADC128S052 driver. Signed-off-by: Aaron Brice <[email protected]>
Adding driver for TS-SILO supercap power supply. The TS-SILO provides power long enough for the system to be able to shut down gracefully if power is lost. Signed-off-by: Aaron Brice <[email protected]>
Device tree and kernel defconfig for the embeddedTS TS-9370 i.MX93 Single Board Computer. Signed-off-by: Aaron Brice <[email protected]>
calibration offset was using the wrong register.
|
These were mostly just copied as-is from linux-tsimx. The only changes required were:
|
ts-kris
left a comment
There was a problem hiding this comment.
A couple of small fixes, couple of comments that would be good to talk about.
There was a problem hiding this comment.
I'm not sure about the organization of this, e.g. should it be in the freescale/ folder, or our own folder since its IOMUX for our FPGA? should it have imx93- as the prefix, since its a pinfunc for our 9370 FPGA that happens to have an i.MX93 CPU?
@markfeathers if you have any ideas of this, let me know. Otherwise I'll probably dig a little bit more just to find any other platforms that might be doing something like this and how they have their IOMUXes laid out.
| assigned-clock-parents = <&clk IMX93_CLK_SYS_PLL_PFD1>; | ||
| pinctrl-names = "default"; | ||
| pinctrl-0 = <&pinctrl_flexspi>; | ||
| //clock-frequency = <66666666>; |
There was a problem hiding this comment.
Remove this comment unless there is a specific reason to leave it as a reference, if so, please add a comment indicating under what circumstances it may be needed.
| reg = <0x2000 0x200>; | ||
| }; | ||
|
|
||
| fpga_gpio0: gpio@28000040 { |
There was a problem hiding this comment.
I'm only familiar with NXPs IOMUX setup, where they have the pinctrl node connected to each gpio instantiation in the devicetree with gpio-ranges, e.g.:
gpio1: gpio@209c000 {
compatible = "fsl,imx6ul-gpio", "fsl,imx35-gpio";
...
gpio-ranges = <&iomuxc 0 23 10>, <&iomuxc 10 17 6>,
<&iomuxc 16 33 16>;
};
gpio2: gpio@20a0000 {
compatible = "fsl,imx6ul-gpio", "fsl,imx35-gpio";
...
gpio-ranges = <&iomuxc 0 49 16>, <&iomuxc 16 111 6>;
};
Does our driver not need anything like that? Mostly asking just to understand how our driver structure is set up since I otherwise see nothing linking the fpga_xbar and individual GPIO controllers. It would make sense if they don't need to be connected in most circumstances.
| clock-frequency = <5000000>; | ||
|
|
||
| ethphy1: ethernet-phy@1 { | ||
| //compatible = "ethernet-phy-ieee802.3-c22"; |
There was a problem hiding this comment.
Remove commented line unless it makes sense for some reason to leave it in, and if so, add a comment block explaining why it may be useful.
| clock-frequency = <5000000>; | ||
|
|
||
| ethphy2: ethernet-phy@1 { | ||
| //compatible = "ethernet-phy-ieee802.3-c22"; |
| obj-$(CONFIG_TS9370_IRQ) += irq-ts9370.o | ||
| obj-$(CONFIG_WIZARD_IRQ) += irq-wizard.o | ||
| obj-$(CONFIG_TSWEIM_FPGA_INTC) += irq-ts71xxweim.o |
There was a problem hiding this comment.
| obj-$(CONFIG_TS9370_IRQ) += irq-ts9370.o | |
| obj-$(CONFIG_WIZARD_IRQ) += irq-wizard.o | |
| obj-$(CONFIG_TSWEIM_FPGA_INTC) += irq-ts71xxweim.o | |
| obj-$(CONFIG_TS9370_IRQ) += irq-ts9370.o | |
| obj-$(CONFIG_TSWEIM_FPGA_INTC) += irq-ts71xxweim.o | |
| obj-$(CONFIG_WIZARD_IRQ) += irq-wizard.o |
Reorder this to be in numeric/alphabetical order for consistency.
| #define MAX_IRQS 16 | ||
|
|
||
| #define IRQ_STATUS 0 | ||
| #define IRQ_ACK 1 | ||
| #define IRQ_MASK_SET 2 | ||
| #define IRQ_MASK_CLR 3 | ||
| #define IRQ_MASK_RW 4 | ||
|
|
||
| static const struct regmap_irq wizard_irqs[MAX_IRQS] = { | ||
| REGMAP_IRQ_REG(0, 0, BIT(0)), | ||
| REGMAP_IRQ_REG(1, 0, BIT(1)), | ||
| REGMAP_IRQ_REG(2, 0, BIT(2)), | ||
| REGMAP_IRQ_REG(3, 0, BIT(3)), | ||
| REGMAP_IRQ_REG(4, 0, BIT(4)), | ||
| REGMAP_IRQ_REG(5, 0, BIT(5)), | ||
| REGMAP_IRQ_REG(6, 0, BIT(6)), | ||
| REGMAP_IRQ_REG(7, 0, BIT(7)), | ||
| REGMAP_IRQ_REG(8, 0, BIT(8)), | ||
| REGMAP_IRQ_REG(9, 0, BIT(9)), | ||
| REGMAP_IRQ_REG(10, 0, BIT(10)), | ||
| REGMAP_IRQ_REG(11, 0, BIT(11)), | ||
| REGMAP_IRQ_REG(12, 0, BIT(12)), | ||
| REGMAP_IRQ_REG(13, 0, BIT(13)), | ||
| REGMAP_IRQ_REG(14, 0, BIT(14)), | ||
| REGMAP_IRQ_REG(15, 0, BIT(15)), | ||
| }; |
There was a problem hiding this comment.
From what I can tell, the preferred way of handling this, with the offset always being the same (0), is REGMAP_IRQ_REG_LINE() from 43fac32
#define REGMAP_IRQ_REG(_irq, _off, _mask) \
[_irq] = { .reg_offset = (_off), .mask = (_mask) }
#define REGMAP_IRQ_REG_LINE(_id, _reg_bits) \
[_id] = { \
.mask = BIT((_id) % (_reg_bits)), \
.reg_offset = (_id) / (_reg_bits), \
}
| #define MAX_IRQS 16 | |
| #define IRQ_STATUS 0 | |
| #define IRQ_ACK 1 | |
| #define IRQ_MASK_SET 2 | |
| #define IRQ_MASK_CLR 3 | |
| #define IRQ_MASK_RW 4 | |
| static const struct regmap_irq wizard_irqs[MAX_IRQS] = { | |
| REGMAP_IRQ_REG(0, 0, BIT(0)), | |
| REGMAP_IRQ_REG(1, 0, BIT(1)), | |
| REGMAP_IRQ_REG(2, 0, BIT(2)), | |
| REGMAP_IRQ_REG(3, 0, BIT(3)), | |
| REGMAP_IRQ_REG(4, 0, BIT(4)), | |
| REGMAP_IRQ_REG(5, 0, BIT(5)), | |
| REGMAP_IRQ_REG(6, 0, BIT(6)), | |
| REGMAP_IRQ_REG(7, 0, BIT(7)), | |
| REGMAP_IRQ_REG(8, 0, BIT(8)), | |
| REGMAP_IRQ_REG(9, 0, BIT(9)), | |
| REGMAP_IRQ_REG(10, 0, BIT(10)), | |
| REGMAP_IRQ_REG(11, 0, BIT(11)), | |
| REGMAP_IRQ_REG(12, 0, BIT(12)), | |
| REGMAP_IRQ_REG(13, 0, BIT(13)), | |
| REGMAP_IRQ_REG(14, 0, BIT(14)), | |
| REGMAP_IRQ_REG(15, 0, BIT(15)), | |
| }; | |
| #define MAX_IRQS 16 | |
| #define IRQ_STATUS 0 | |
| #define IRQ_ACK 1 | |
| #define IRQ_MASK_SET 2 | |
| #define IRQ_MASK_CLR 3 | |
| #define IRQ_MASK_RW 4 | |
| static const struct regmap_irq wizard_irqs[] = { | |
| REGMAP_IRQ_REG_LINE(0, MAX_IRQS), | |
| REGMAP_IRQ_REG_LINE(1, MAX_IRQS), | |
| REGMAP_IRQ_REG_LINE(2, MAX_IRQS), | |
| REGMAP_IRQ_REG_LINE(3, MAX_IRQS), | |
| REGMAP_IRQ_REG_LINE(4, MAX_IRQS), | |
| REGMAP_IRQ_REG_LINE(5, MAX_IRQS), | |
| REGMAP_IRQ_REG_LINE(6, MAX_IRQS), | |
| REGMAP_IRQ_REG_LINE(7, MAX_IRQS), | |
| REGMAP_IRQ_REG_LINE(8, MAX_IRQS), | |
| REGMAP_IRQ_REG_LINE(9, MAX_IRQS), | |
| REGMAP_IRQ_REG_LINE(10, MAX_IRQS), | |
| REGMAP_IRQ_REG_LINE(11, MAX_IRQS), | |
| REGMAP_IRQ_REG_LINE(12, MAX_IRQS), | |
| REGMAP_IRQ_REG_LINE(13, MAX_IRQS), | |
| REGMAP_IRQ_REG_LINE(14, MAX_IRQS), | |
| REGMAP_IRQ_REG_LINE(15, MAX_IRQS), | |
| } |
The former in our case producing, e.g.
[0] = { .reg_offset = 0, .mask = (1 << 0) },
[1] = { .reg_offset = 0, .mask = (1 << 1) },
[2] = { .reg_offset = 0, .mask = (1 << 2) },
And the latter producing (reordered for consistency):
[0] = { .reg_offset = ((0/16) == 0), .mask = (((1 << 0) % 16) == 1) }
[1] = { .reg_offset = ((1/16) == 0), .mask = (((1 << 1) % 16) == 2) }
[2] = { .reg_offset = ((2/16) == 0), .mask = (((1 << 2) % 16) == 4) }
This may not be worth changing for this PR, but may need to be kept in mind when upstreaming later.
There was a problem hiding this comment.
It looks like our defconfig defaults to having SOC_IMX8 as well as a handful of other default enabled i.MX8 drivers like clocks etc that I don't think we need? Unless the i.MX9 uses those under the hood. Would potentially benefit us to do a sweep and disable things like that which we don't need for sure to save some space.
There was a problem hiding this comment.
I don't have a better answer for this right now, but I'm not a fan of having separate selections for both platforms. It might be cleaner to have a single config option that is per CPU series, since in most cases they would both/all be enabled in our defconfigs, and it would require tuning for specific platforms if a customer wanted that.
It may also be a bright idea to completely hide these (or at least have some reverse dependency saying that if the ts9370 GPIO driver was selected, then select the pinmux driver too).
| int ret; | ||
| int irq; | ||
|
|
||
| data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); |
There was a problem hiding this comment.
Return of devm_kzalloc() is unchecked
Initial support for TS-9370 i.MX93 board. Display, camera, sound not supported.