* Re: [PATCH v6 4/5] phy: rockchip-typec: support variable phy config value
From: Enric Balletbo Serra @ 2018-05-21 14:13 UTC (permalink / raw)
To: Lin Huang
Cc: devicetree@vger.kernel.org, David Airlie, linux-kernel,
Brian Norris, Doug Anderson, Kishon Vijay Abraham I,
open list:ARM/Rockchip SoC..., Rob Herring, dri-devel,
Chris Zhong, daniel.vetter, Linux ARM
In-Reply-To: <1526895424-22894-4-git-send-email-hl@rock-chips.com>
Hi Lin,
2018-05-21 11:37 GMT+02:00 Lin Huang <hl@rock-chips.com>:
> the phy config values used to fix in dp firmware, but some boards
> need change these values to do training and get the better eye diagram
> result. So support that in phy driver.
>
> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
> Changes in v2:
> - update patch following Enric suggest
> Changes in v3:
> - delete need_software_training variable
> - add default phy config value, if dts do not define phy config value, use these value
> Changes in v4:
> - rename variable config to tcphy_default_config
> Changes in v5:
> - None
> Changes in v6:
> - split the header file to new patch
>
> drivers/phy/rockchip/phy-rockchip-typec.c | 261 ++++++++++++++++++++++++------
> 1 file changed, 208 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/phy/rockchip/phy-rockchip-typec.c b/drivers/phy/rockchip/phy-rockchip-typec.c
> index 795055f..4c4b925 100644
> --- a/drivers/phy/rockchip/phy-rockchip-typec.c
> +++ b/drivers/phy/rockchip/phy-rockchip-typec.c
> @@ -324,21 +324,29 @@
> * clock 0: PLL 0 div 1
> * clock 1: PLL 1 div 2
> */
> -#define CLK_PLL_CONFIG 0X30
> +#define CLK_PLL1_DIV1 0x20
> +#define CLK_PLL1_DIV2 0x30
> #define CLK_PLL_MASK 0x33
>
> #define CMN_READY BIT(0)
>
> +#define DP_PLL_CLOCK_ENABLE_ACK BIT(3)
> #define DP_PLL_CLOCK_ENABLE BIT(2)
> +#define DP_PLL_ENABLE_ACK BIT(1)
> #define DP_PLL_ENABLE BIT(0)
> #define DP_PLL_DATA_RATE_RBR ((2 << 12) | (4 << 8))
> #define DP_PLL_DATA_RATE_HBR ((2 << 12) | (4 << 8))
> #define DP_PLL_DATA_RATE_HBR2 ((1 << 12) | (2 << 8))
> +#define DP_PLL_DATA_RATE_MASK 0xff00
>
> -#define DP_MODE_A0 BIT(4)
> -#define DP_MODE_A2 BIT(6)
> -#define DP_MODE_ENTER_A0 0xc101
> -#define DP_MODE_ENTER_A2 0xc104
> +#define DP_MODE_MASK 0xf
> +#define DP_MODE_ENTER_A0 BIT(0)
> +#define DP_MODE_ENTER_A2 BIT(2)
> +#define DP_MODE_ENTER_A3 BIT(3)
> +#define DP_MODE_A0_ACK BIT(4)
> +#define DP_MODE_A2_ACK BIT(6)
> +#define DP_MODE_A3_ACK BIT(7)
> +#define DP_LINK_RESET_DEASSERTED BIT(8)
>
> #define PHY_MODE_SET_TIMEOUT 100000
>
> @@ -350,6 +358,8 @@
> #define MODE_DFP_USB BIT(1)
> #define MODE_DFP_DP BIT(2)
>
> +#define DP_DEFAULT_RATE 162000
> +
> struct phy_reg {
> u16 value;
> u32 addr;
> @@ -372,15 +382,15 @@ struct phy_reg usb3_pll_cfg[] = {
> { 0x8, CMN_DIAG_PLL0_LF_PROG },
> };
>
> -struct phy_reg dp_pll_cfg[] = {
> +struct phy_reg dp_pll_rbr_cfg[] = {
> { 0xf0, CMN_PLL1_VCOCAL_INIT },
> { 0x18, CMN_PLL1_VCOCAL_ITER },
> { 0x30b9, CMN_PLL1_VCOCAL_START },
> - { 0x21c, CMN_PLL1_INTDIV },
> + { 0x87, CMN_PLL1_INTDIV },
> { 0, CMN_PLL1_FRACDIV },
> - { 0x5, CMN_PLL1_HIGH_THR },
> - { 0x35, CMN_PLL1_SS_CTRL1 },
> - { 0x7f1e, CMN_PLL1_SS_CTRL2 },
> + { 0x22, CMN_PLL1_HIGH_THR },
> + { 0x8000, CMN_PLL1_SS_CTRL1 },
> + { 0, CMN_PLL1_SS_CTRL2 },
> { 0x20, CMN_PLL1_DSM_DIAG },
> { 0, CMN_PLLSM1_USER_DEF_CTRL },
> { 0, CMN_DIAG_PLL1_OVRD },
> @@ -391,9 +401,52 @@ struct phy_reg dp_pll_cfg[] = {
> { 0x8, CMN_DIAG_PLL1_LF_PROG },
> { 0x100, CMN_DIAG_PLL1_PTATIS_TUNE1 },
> { 0x7, CMN_DIAG_PLL1_PTATIS_TUNE2 },
> - { 0x4, CMN_DIAG_PLL1_INCLK_CTRL },
> + { 0x1, CMN_DIAG_PLL1_INCLK_CTRL },
> };
>
> +struct phy_reg dp_pll_hbr_cfg[] = {
> + { 0xf0, CMN_PLL1_VCOCAL_INIT },
> + { 0x18, CMN_PLL1_VCOCAL_ITER },
> + { 0x30b4, CMN_PLL1_VCOCAL_START },
> + { 0xe1, CMN_PLL1_INTDIV },
> + { 0, CMN_PLL1_FRACDIV },
> + { 0x5, CMN_PLL1_HIGH_THR },
> + { 0x8000, CMN_PLL1_SS_CTRL1 },
> + { 0, CMN_PLL1_SS_CTRL2 },
> + { 0x20, CMN_PLL1_DSM_DIAG },
> + { 0x1000, CMN_PLLSM1_USER_DEF_CTRL },
> + { 0, CMN_DIAG_PLL1_OVRD },
> + { 0, CMN_DIAG_PLL1_FBH_OVRD },
> + { 0, CMN_DIAG_PLL1_FBL_OVRD },
> + { 0x7, CMN_DIAG_PLL1_V2I_TUNE },
> + { 0x45, CMN_DIAG_PLL1_CP_TUNE },
> + { 0x8, CMN_DIAG_PLL1_LF_PROG },
> + { 0x1, CMN_DIAG_PLL1_PTATIS_TUNE1 },
> + { 0x1, CMN_DIAG_PLL1_PTATIS_TUNE2 },
> + { 0x1, CMN_DIAG_PLL1_INCLK_CTRL },
> +};
> +
> +struct phy_reg dp_pll_hbr2_cfg[] = {
> + { 0xf0, CMN_PLL1_VCOCAL_INIT },
> + { 0x18, CMN_PLL1_VCOCAL_ITER },
> + { 0x30b4, CMN_PLL1_VCOCAL_START },
> + { 0xe1, CMN_PLL1_INTDIV },
> + { 0, CMN_PLL1_FRACDIV },
> + { 0x5, CMN_PLL1_HIGH_THR },
> + { 0x8000, CMN_PLL1_SS_CTRL1 },
> + { 0, CMN_PLL1_SS_CTRL2 },
> + { 0x20, CMN_PLL1_DSM_DIAG },
> + { 0x1000, CMN_PLLSM1_USER_DEF_CTRL },
> + { 0, CMN_DIAG_PLL1_OVRD },
> + { 0, CMN_DIAG_PLL1_FBH_OVRD },
> + { 0, CMN_DIAG_PLL1_FBL_OVRD },
> + { 0x7, CMN_DIAG_PLL1_V2I_TUNE },
> + { 0x45, CMN_DIAG_PLL1_CP_TUNE },
> + { 0x8, CMN_DIAG_PLL1_LF_PROG },
> + { 0x1, CMN_DIAG_PLL1_PTATIS_TUNE1 },
> + { 0x1, CMN_DIAG_PLL1_PTATIS_TUNE2 },
> + { 0x1, CMN_DIAG_PLL1_INCLK_CTRL },
> +};
> static const struct rockchip_usb3phy_port_cfg rk3399_usb3phy_port_cfgs[] = {
> {
> .reg = 0xff7c0000,
> @@ -418,6 +471,24 @@ static const struct rockchip_usb3phy_port_cfg rk3399_usb3phy_port_cfgs[] = {
> { /* sentinel */ }
> };
>
> +/* default phy config */
> +static const struct phy_config tcphy_default_config[3][4] = {
> + {{ .swing = 0x2a, .pe = 0x00 },
> + { .swing = 0x1f, .pe = 0x15 },
> + { .swing = 0x14, .pe = 0x22 },
> + { .swing = 0x02, .pe = 0x2b } },
> +
> + {{ .swing = 0x21, .pe = 0x00 },
> + { .swing = 0x12, .pe = 0x15 },
> + { .swing = 0x02, .pe = 0x22 },
> + { .swing = 0, .pe = 0 } },
> +
> + {{ .swing = 0x15, .pe = 0x00 },
> + { .swing = 0x00, .pe = 0x15 },
> + { .swing = 0, .pe = 0 },
> + { .swing = 0, .pe = 0 } },
> +};
> +
> static void tcphy_cfg_24m(struct rockchip_typec_phy *tcphy)
> {
> u32 i, rdata;
> @@ -439,7 +510,7 @@ static void tcphy_cfg_24m(struct rockchip_typec_phy *tcphy)
>
> rdata = readl(tcphy->base + CMN_DIAG_HSCLK_SEL);
> rdata &= ~CLK_PLL_MASK;
> - rdata |= CLK_PLL_CONFIG;
> + rdata |= CLK_PLL1_DIV2;
> writel(rdata, tcphy->base + CMN_DIAG_HSCLK_SEL);
> }
>
> @@ -453,17 +524,44 @@ static void tcphy_cfg_usb3_pll(struct rockchip_typec_phy *tcphy)
> tcphy->base + usb3_pll_cfg[i].addr);
> }
>
> -static void tcphy_cfg_dp_pll(struct rockchip_typec_phy *tcphy)
> +static void tcphy_cfg_dp_pll(struct rockchip_typec_phy *tcphy, int link_rate)
> {
> - u32 i;
> + struct phy_reg *phy_cfg;
> + u32 clk_ctrl;
> + u32 i, cfg_size, hsclk_sel;
> +
> + hsclk_sel = readl(tcphy->base + CMN_DIAG_HSCLK_SEL);
> + hsclk_sel &= ~CLK_PLL_MASK;
> +
> + switch (link_rate) {
> + case 162000:
> + clk_ctrl = DP_PLL_DATA_RATE_RBR;
> + hsclk_sel |= CLK_PLL1_DIV2;
> + phy_cfg = dp_pll_rbr_cfg;
> + cfg_size = ARRAY_SIZE(dp_pll_rbr_cfg);
> + break;
> + case 270000:
> + clk_ctrl = DP_PLL_DATA_RATE_HBR;
> + hsclk_sel |= CLK_PLL1_DIV2;
> + phy_cfg = dp_pll_hbr_cfg;
> + cfg_size = ARRAY_SIZE(dp_pll_hbr_cfg);
> + break;
> + case 540000:
> + clk_ctrl = DP_PLL_DATA_RATE_HBR2;
> + hsclk_sel |= CLK_PLL1_DIV1;
> + phy_cfg = dp_pll_hbr2_cfg;
> + cfg_size = ARRAY_SIZE(dp_pll_hbr2_cfg);
> + break;
If someone calls this function with a link_rate different to the ones
in the switch statement, some variables will be uninitialized and the
kernel will crash. I'd add a default case or return an error if a
different link_rate is passed to the function.
> + }
>
> - /* set the default mode to RBR */
> - writel(DP_PLL_CLOCK_ENABLE | DP_PLL_ENABLE | DP_PLL_DATA_RATE_RBR,
> - tcphy->base + DP_CLK_CTL);
> + clk_ctrl |= DP_PLL_CLOCK_ENABLE | DP_PLL_ENABLE;
> + writel(clk_ctrl, tcphy->base + DP_CLK_CTL);
> +
> + writel(hsclk_sel, tcphy->base + CMN_DIAG_HSCLK_SEL);
>
> /* load the configuration of PLL1 */
> - for (i = 0; i < ARRAY_SIZE(dp_pll_cfg); i++)
> - writel(dp_pll_cfg[i].value, tcphy->base + dp_pll_cfg[i].addr);
> + for (i = 0; i < cfg_size; i++)
> + writel(phy_cfg[i].value, tcphy->base + phy_cfg[i].addr);
> }
>
> static void tcphy_tx_usb3_cfg_lane(struct rockchip_typec_phy *tcphy, u32 lane)
> @@ -490,9 +588,10 @@ static void tcphy_rx_usb3_cfg_lane(struct rockchip_typec_phy *tcphy, u32 lane)
> writel(0xfb, tcphy->base + XCVR_DIAG_BIDI_CTRL(lane));
> }
>
> -static void tcphy_dp_cfg_lane(struct rockchip_typec_phy *tcphy, u32 lane)
> +static void tcphy_dp_cfg_lane(struct rockchip_typec_phy *tcphy, int link_rate,
> + u8 swing, u8 pre_emp, u32 lane)
> {
> - u16 rdata;
> + u16 val;
>
Just use rdata in your added code, you don't need to rename the
variable. That will result in a smaller diff.
> writel(0xbefc, tcphy->base + XCVR_PSM_RCTRL(lane));
> writel(0x6799, tcphy->base + TX_PSC_A0(lane));
> @@ -500,25 +599,31 @@ static void tcphy_dp_cfg_lane(struct rockchip_typec_phy *tcphy, u32 lane)
> writel(0x98, tcphy->base + TX_PSC_A2(lane));
> writel(0x98, tcphy->base + TX_PSC_A3(lane));
>
> - writel(0, tcphy->base + TX_TXCC_MGNFS_MULT_000(lane));
> - writel(0, tcphy->base + TX_TXCC_MGNFS_MULT_001(lane));
> - writel(0, tcphy->base + TX_TXCC_MGNFS_MULT_010(lane));
> - writel(0, tcphy->base + TX_TXCC_MGNFS_MULT_011(lane));
> - writel(0, tcphy->base + TX_TXCC_MGNFS_MULT_100(lane));
> - writel(0, tcphy->base + TX_TXCC_MGNFS_MULT_101(lane));
> - writel(0, tcphy->base + TX_TXCC_MGNFS_MULT_110(lane));
> - writel(0, tcphy->base + TX_TXCC_MGNFS_MULT_111(lane));
> - writel(0, tcphy->base + TX_TXCC_CPOST_MULT_10(lane));
> - writel(0, tcphy->base + TX_TXCC_CPOST_MULT_01(lane));
> - writel(0, tcphy->base + TX_TXCC_CPOST_MULT_00(lane));
> - writel(0, tcphy->base + TX_TXCC_CPOST_MULT_11(lane));
> -
> - writel(0x128, tcphy->base + TX_TXCC_CAL_SCLR_MULT(lane));
> - writel(0x400, tcphy->base + TX_DIAG_TX_DRV(lane));
> -
> - rdata = readl(tcphy->base + XCVR_DIAG_PLLDRC_CTRL(lane));
> - rdata = (rdata & 0x8fff) | 0x6000;
> - writel(rdata, tcphy->base + XCVR_DIAG_PLLDRC_CTRL(lane));
> + writel(tcphy->config[swing][pre_emp].swing,
> + tcphy->base + TX_TXCC_MGNFS_MULT_000(lane));
> + writel(tcphy->config[swing][pre_emp].pe,
> + tcphy->base + TX_TXCC_CPOST_MULT_00(lane));
> +
> + if (swing == 2 && pre_emp == 0 && link_rate != 540000) {
> + writel(0x700, tcphy->base + TX_DIAG_TX_DRV(lane));
> + writel(0x13c, tcphy->base + TX_TXCC_CAL_SCLR_MULT(lane));
> + } else {
> + writel(0x128, tcphy->base + TX_TXCC_CAL_SCLR_MULT(lane));
> + writel(0x0400, tcphy->base + TX_DIAG_TX_DRV(lane));
> + }
> +
> + val = readl(tcphy->base + XCVR_DIAG_PLLDRC_CTRL(lane));
> + val = val & 0x8fff;
> + switch (link_rate) {
> + case 162000:
> + case 270000:
> + val |= (6 << 12);
> + break;
> + case 540000:
> + val |= (4 << 12);
> + break;
If link_rate is another value you will write (val & 0x8fff), is ok?
Before this patch, we were writing (rdata & 0x8fff) | 0x6000 by
default, maybe we should default to this value?
> + }
> + writel(val, tcphy->base + XCVR_DIAG_PLLDRC_CTRL(lane));
> }
>
> static inline int property_enable(struct rockchip_typec_phy *tcphy,
> @@ -709,30 +814,33 @@ static int tcphy_phy_init(struct rockchip_typec_phy *tcphy, u8 mode)
> tcphy_cfg_24m(tcphy);
>
> if (mode == MODE_DFP_DP) {
> - tcphy_cfg_dp_pll(tcphy);
> + tcphy_cfg_dp_pll(tcphy, DP_DEFAULT_RATE);
> for (i = 0; i < 4; i++)
> - tcphy_dp_cfg_lane(tcphy, i);
> + tcphy_dp_cfg_lane(tcphy, DP_DEFAULT_RATE, 0, 0, i);
>
> writel(PIN_ASSIGN_C_E, tcphy->base + PMA_LANE_CFG);
> } else {
> tcphy_cfg_usb3_pll(tcphy);
> - tcphy_cfg_dp_pll(tcphy);
> + tcphy_cfg_dp_pll(tcphy, DP_DEFAULT_RATE);
> if (tcphy->flip) {
> tcphy_tx_usb3_cfg_lane(tcphy, 3);
> tcphy_rx_usb3_cfg_lane(tcphy, 2);
> - tcphy_dp_cfg_lane(tcphy, 0);
> - tcphy_dp_cfg_lane(tcphy, 1);
> + tcphy_dp_cfg_lane(tcphy, DP_DEFAULT_RATE, 0, 0, 0);
> + tcphy_dp_cfg_lane(tcphy, DP_DEFAULT_RATE, 0, 0, 1);
> } else {
> tcphy_tx_usb3_cfg_lane(tcphy, 0);
> tcphy_rx_usb3_cfg_lane(tcphy, 1);
> - tcphy_dp_cfg_lane(tcphy, 2);
> - tcphy_dp_cfg_lane(tcphy, 3);
> + tcphy_dp_cfg_lane(tcphy, DP_DEFAULT_RATE, 0, 0, 2);
> + tcphy_dp_cfg_lane(tcphy, DP_DEFAULT_RATE, 0, 0, 3);
> }
>
> writel(PIN_ASSIGN_D_F, tcphy->base + PMA_LANE_CFG);
> }
>
> - writel(DP_MODE_ENTER_A2, tcphy->base + DP_MODE_CTL);
> + val = readl(tcphy->base + DP_MODE_CTL);
> + val &= ~DP_MODE_MASK;
> + val |= DP_MODE_ENTER_A2 | DP_LINK_RESET_DEASSERTED;
> + writel(val, tcphy->base + DP_MODE_CTL);
>
> reset_control_deassert(tcphy->uphy_rst);
>
> @@ -945,7 +1053,7 @@ static int rockchip_dp_phy_power_on(struct phy *phy)
> property_enable(tcphy, &cfg->uphy_dp_sel, 1);
>
> ret = readx_poll_timeout(readl, tcphy->base + DP_MODE_CTL,
> - val, val & DP_MODE_A2, 1000,
> + val, val & DP_MODE_A2_ACK, 1000,
> PHY_MODE_SET_TIMEOUT);
> if (ret < 0) {
> dev_err(tcphy->dev, "failed to wait TCPHY enter A2\n");
> @@ -954,13 +1062,19 @@ static int rockchip_dp_phy_power_on(struct phy *phy)
>
> tcphy_dp_aux_calibration(tcphy);
>
> - writel(DP_MODE_ENTER_A0, tcphy->base + DP_MODE_CTL);
> + /* enter A0 mode */
> + val = readl(tcphy->base + DP_MODE_CTL);
> + val &= ~DP_MODE_MASK;
> + val |= DP_MODE_ENTER_A0;
> + writel(val, tcphy->base + DP_MODE_CTL);
>
> ret = readx_poll_timeout(readl, tcphy->base + DP_MODE_CTL,
> - val, val & DP_MODE_A0, 1000,
> + val, val & DP_MODE_A0_ACK, 1000,
> PHY_MODE_SET_TIMEOUT);
> if (ret < 0) {
> - writel(DP_MODE_ENTER_A2, tcphy->base + DP_MODE_CTL);
> + val &= ~DP_MODE_MASK;
> + val |= DP_MODE_ENTER_A2;
> + writel(val, tcphy->base + DP_MODE_CTL);
> dev_err(tcphy->dev, "failed to wait TCPHY enter A0\n");
> goto power_on_finish;
> }
> @@ -978,6 +1092,7 @@ static int rockchip_dp_phy_power_on(struct phy *phy)
> static int rockchip_dp_phy_power_off(struct phy *phy)
> {
> struct rockchip_typec_phy *tcphy = phy_get_drvdata(phy);
> + u32 val;
>
> mutex_lock(&tcphy->lock);
>
> @@ -986,7 +1101,10 @@ static int rockchip_dp_phy_power_off(struct phy *phy)
>
> tcphy->mode &= ~MODE_DFP_DP;
>
> - writel(DP_MODE_ENTER_A2, tcphy->base + DP_MODE_CTL);
> + val = readl(tcphy->base + DP_MODE_CTL);
> + val &= ~DP_MODE_MASK;
> + val |= DP_MODE_ENTER_A2;
> + writel(val, tcphy->base + DP_MODE_CTL);
>
> if (tcphy->mode == MODE_DISCONNECT)
> tcphy_phy_deinit(tcphy);
> @@ -1002,9 +1120,35 @@ static const struct phy_ops rockchip_dp_phy_ops = {
> .owner = THIS_MODULE,
> };
>
> +static int typec_dp_phy_config(struct phy *phy, int link_rate,
> + int lanes, u8 swing, u8 pre_emp)
> +{
> + struct rockchip_typec_phy *tcphy = phy_get_drvdata(phy);
> + u8 i;
> +
> + tcphy_cfg_dp_pll(tcphy, link_rate);
> +
> + if (tcphy->mode == MODE_DFP_DP) {
> + for (i = 0; i < 4; i++)
> + tcphy_dp_cfg_lane(tcphy, link_rate, swing, pre_emp, i);
> + } else {
> + if (tcphy->flip) {
> + tcphy_dp_cfg_lane(tcphy, link_rate, swing, pre_emp, 0);
> + tcphy_dp_cfg_lane(tcphy, link_rate, swing, pre_emp, 1);
> + } else {
> + tcphy_dp_cfg_lane(tcphy, link_rate, swing, pre_emp, 2);
> + tcphy_dp_cfg_lane(tcphy, link_rate, swing, pre_emp, 3);
> + }
> + }
> +
> + return 0;
> +}
> +
> static int tcphy_parse_dt(struct rockchip_typec_phy *tcphy,
> struct device *dev)
> {
> + int ret;
> +
> tcphy->grf_regs = syscon_regmap_lookup_by_phandle(dev->of_node,
> "rockchip,grf");
> if (IS_ERR(tcphy->grf_regs)) {
> @@ -1042,6 +1186,16 @@ static int tcphy_parse_dt(struct rockchip_typec_phy *tcphy,
> return PTR_ERR(tcphy->tcphy_rst);
> }
>
> + /*
> + * check if phy_config pass from dts, if no,
> + * use default phy config value.
nit: Check if phy-config is passed from the dts, if no, use the
default phy configuration.
> + */
> + ret = of_property_read_u32_array(dev->of_node, "rockchip,phy-config",
> + (u32 *)tcphy->config, sizeof(tcphy->config) / sizeof(u32));
> + if (ret)
> + memcpy(tcphy->config, tcphy_default_config,
> + sizeof(tcphy->config));
> +
> return 0;
> }
>
> @@ -1126,6 +1280,7 @@ static int rockchip_typec_phy_probe(struct platform_device *pdev)
> }
> }
>
> + tcphy->typec_phy_config = typec_dp_phy_config;
> pm_runtime_enable(dev);
>
> for_each_available_child_of_node(np, child_np) {
> --
> 2.7.4
>
Best regards,
Enric
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v7 2/2] PCI: mediatek: Using chained IRQ to setup IRQ handle
From: Lorenzo Pieralisi @ 2018-05-21 14:03 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: honghui.zhang, marc.zyngier, bhelgaas, matthias.bgg,
linux-arm-kernel, linux-mediatek, linux-pci, linux-kernel,
devicetree, yingjoe.chen, eddie.huang, ryder.lee, hongkun.cao,
youlin.pei, yong.wu, yt.shen, sean.wang, xinping.qian
In-Reply-To: <20180518195154.GB41790@bhelgaas-glaptop.roam.corp.google.com>
On Fri, May 18, 2018 at 02:51:54PM -0500, Bjorn Helgaas wrote:
> On Fri, May 04, 2018 at 01:47:33PM +0800, honghui.zhang@mediatek.com wrote:
> > From: Honghui Zhang <honghui.zhang@mediatek.com>
> >
> > Using irq_chip solution to setup IRQs in order to consist
> > with IRQ framework.
> >
> > Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>
> > ---
> > drivers/pci/host/pcie-mediatek.c | 206 ++++++++++++++++++++++-----------------
> > 1 file changed, 115 insertions(+), 91 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c
> > index c3dc549..dabf1086 100644
> > --- a/drivers/pci/host/pcie-mediatek.c
> > +++ b/drivers/pci/host/pcie-mediatek.c
> > ...
>
> > -static int mtk_pcie_msi_map(struct irq_domain *domain, unsigned int irq,
> > - irq_hw_number_t hwirq)
> > +static struct msi_domain_info mtk_msi_domain_info = {
>
> I think this patch should be amended to include this:
>
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 0d0177ce436c..368b70d9371b 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -193,7 +193,7 @@ config PCIE_MEDIATEK
> bool "MediaTek PCIe controller"
> depends on (ARM || ARM64) && (ARCH_MEDIATEK || COMPILE_TEST)
> depends on OF
> - depends on PCI
Actually I amended your patch, since I do not think the deletion above
belongs in this commit (I agree that's completely useless but that's
not this patch that we should remove it).
Lorenzo
> + depends on PCI_MSI_IRQ_DOMAIN
^ permalink raw reply
* Re: [PATCH v6 9/9] iio: counter: Remove IIO counter subdirectory
From: William Breathitt Gray @ 2018-05-21 13:58 UTC (permalink / raw)
To: Jonathan Cameron
Cc: benjamin.gaignard, fabrice.gasnier, linux-iio, linux-kernel,
devicetree, linux-arm-kernel
In-Reply-To: <20180520165302.101d37ce@archlinux>
On Sun, May 20, 2018 at 04:53:02PM +0100, Jonathan Cameron wrote:
>On Wed, 16 May 2018 13:52:39 -0400
>William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
>
>> This patch removes the IIO counter subdirectory which is now superceded
>> by the Counter subsystem. Deprecation warnings are added to the
>> documentation of the relevant IIO counter sysfs attributes.
>>
>> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
>
>Please drop the directory when it becomes empty rather than in a later
>patch. IIRC there are some issues with empty Makefiles that will
>make building inbetween tricky.
>
>For the deprecated markings.
>
>Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
I'll have the directory removal occur with the removal of the last
module then when the directory becomes empty.
Regarding the deprecation markings, should I select a specific kernel
version to date the removal of these attributes, or leave the future
date open as this patch is now?
William Breathitt Gray
>
>> ---
>> Documentation/ABI/testing/sysfs-bus-iio | 8 ++++++++
>> .../ABI/testing/sysfs-bus-iio-counter-104-quad-8 | 16 ++++++++++++++++
>> drivers/iio/Kconfig | 1 -
>> drivers/iio/Makefile | 1 -
>> drivers/iio/counter/Kconfig | 8 --------
>> drivers/iio/counter/Makefile | 5 -----
>> 6 files changed, 24 insertions(+), 15 deletions(-)
>> delete mode 100644 drivers/iio/counter/Kconfig
>> delete mode 100644 drivers/iio/counter/Makefile
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
>> index 731146c3b138..6115d97b075e 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>> @@ -1637,6 +1637,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_countY_raw
>> KernelVersion: 4.10
>> Contact: linux-iio@vger.kernel.org
>> Description:
>> + This interface is deprecated; please use the Counter subsystem.
>> +
>> Raw counter device counts from channel Y. For quadrature
>> counters, multiplication by an available [Y]_scale results in
>> the counts of a single quadrature signal phase from channel Y.
>> @@ -1645,6 +1647,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_indexY_raw
>> KernelVersion: 4.10
>> Contact: linux-iio@vger.kernel.org
>> Description:
>> + This interface is deprecated; please use the Counter subsystem.
>> +
>> Raw counter device index value from channel Y. This attribute
>> provides an absolute positional reference (e.g. a pulse once per
>> revolution) which may be used to home positional systems as
>> @@ -1654,6 +1658,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_count_count_direction_available
>> KernelVersion: 4.12
>> Contact: linux-iio@vger.kernel.org
>> Description:
>> + This interface is deprecated; please use the Counter subsystem.
>> +
>> A list of possible counting directions which are:
>> - "up" : counter device is increasing.
>> - "down": counter device is decreasing.
>> @@ -1662,4 +1668,6 @@ What: /sys/bus/iio/devices/iio:deviceX/in_countY_count_direction
>> KernelVersion: 4.12
>> Contact: linux-iio@vger.kernel.org
>> Description:
>> + This interface is deprecated; please use the Counter subsystem.
>> +
>> Raw counter device counters direction for channel Y.
>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-counter-104-quad-8 b/Documentation/ABI/testing/sysfs-bus-iio-counter-104-quad-8
>> index 7fac2c268d9a..bac3d0d48b7b 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-iio-counter-104-quad-8
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-counter-104-quad-8
>> @@ -6,6 +6,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_index_synchronous_mode_available
>> KernelVersion: 4.10
>> Contact: linux-iio@vger.kernel.org
>> Description:
>> + This interface is deprecated; please use the Counter subsystem.
>> +
>> Discrete set of available values for the respective counter
>> configuration are listed in this file.
>>
>> @@ -13,6 +15,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_countY_count_mode
>> KernelVersion: 4.10
>> Contact: linux-iio@vger.kernel.org
>> Description:
>> + This interface is deprecated; please use the Counter subsystem.
>> +
>> Count mode for channel Y. Four count modes are available:
>> normal, range limit, non-recycle, and modulo-n. The preset value
>> for channel Y is used by the count mode where required.
>> @@ -47,6 +51,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_countY_noise_error
>> KernelVersion: 4.10
>> Contact: linux-iio@vger.kernel.org
>> Description:
>> + This interface is deprecated; please use the Counter subsystem.
>> +
>> Read-only attribute that indicates whether excessive noise is
>> present at the channel Y count inputs in quadrature clock mode;
>> irrelevant in non-quadrature clock mode.
>> @@ -55,6 +61,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_countY_preset
>> KernelVersion: 4.10
>> Contact: linux-iio@vger.kernel.org
>> Description:
>> + This interface is deprecated; please use the Counter subsystem.
>> +
>> If the counter device supports preset registers, the preset
>> count for channel Y is provided by this attribute.
>>
>> @@ -62,6 +70,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_countY_quadrature_mode
>> KernelVersion: 4.10
>> Contact: linux-iio@vger.kernel.org
>> Description:
>> + This interface is deprecated; please use the Counter subsystem.
>> +
>> Configure channel Y counter for non-quadrature or quadrature
>> clock mode. Selecting non-quadrature clock mode will disable
>> synchronous load mode. In quadrature clock mode, the channel Y
>> @@ -83,6 +93,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_countY_set_to_preset_on_index
>> KernelVersion: 4.10
>> Contact: linux-iio@vger.kernel.org
>> Description:
>> + This interface is deprecated; please use the Counter subsystem.
>> +
>> Whether to set channel Y counter with channel Y preset value
>> when channel Y index input is active, or continuously count.
>> Valid attribute values are boolean.
>> @@ -91,6 +103,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_indexY_index_polarity
>> KernelVersion: 4.10
>> Contact: linux-iio@vger.kernel.org
>> Description:
>> + This interface is deprecated; please use the Counter subsystem.
>> +
>> Active level of channel Y index input; irrelevant in
>> non-synchronous load mode.
>>
>> @@ -98,6 +112,8 @@ What: /sys/bus/iio/devices/iio:deviceX/in_indexY_synchronous_mode
>> KernelVersion: 4.10
>> Contact: linux-iio@vger.kernel.org
>> Description:
>> + This interface is deprecated; please use the Counter subsystem.
>> +
>> Configure channel Y counter for non-synchronous or synchronous
>> load mode. Synchronous load mode cannot be selected in
>> non-quadrature clock mode.
>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>> index d69e85a8bdc3..1152efad91a1 100644
>> --- a/drivers/iio/Kconfig
>> +++ b/drivers/iio/Kconfig
>> @@ -74,7 +74,6 @@ source "drivers/iio/afe/Kconfig"
>> source "drivers/iio/amplifiers/Kconfig"
>> source "drivers/iio/chemical/Kconfig"
>> source "drivers/iio/common/Kconfig"
>> -source "drivers/iio/counter/Kconfig"
>> source "drivers/iio/dac/Kconfig"
>> source "drivers/iio/dummy/Kconfig"
>> source "drivers/iio/frequency/Kconfig"
>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>> index d8cba9c229c0..7bdd31f1b88f 100644
>> --- a/drivers/iio/Makefile
>> +++ b/drivers/iio/Makefile
>> @@ -20,7 +20,6 @@ obj-y += amplifiers/
>> obj-y += buffer/
>> obj-y += chemical/
>> obj-y += common/
>> -obj-y += counter/
>> obj-y += dac/
>> obj-y += dummy/
>> obj-y += gyro/
>> diff --git a/drivers/iio/counter/Kconfig b/drivers/iio/counter/Kconfig
>> deleted file mode 100644
>> index 95a7a0df6cac..000000000000
>> --- a/drivers/iio/counter/Kconfig
>> +++ /dev/null
>> @@ -1,8 +0,0 @@
>> -#
>> -# Counter devices
>> -#
>> -# When adding new entries keep the list in alphabetical order
>> -
>> -menu "Counters"
>> -
>> -endmenu
>> diff --git a/drivers/iio/counter/Makefile b/drivers/iio/counter/Makefile
>> deleted file mode 100644
>> index 8fd3d954775a..000000000000
>> --- a/drivers/iio/counter/Makefile
>> +++ /dev/null
>> @@ -1,5 +0,0 @@
>> -#
>> -# Makefile for IIO counter devices
>> -#
>> -
>> -# When adding new entries keep the list in alphabetical order
>
^ permalink raw reply
* Re: [PATCH v6 4/9] counter: 104-quad-8: Add Generic Counter interface support
From: William Breathitt Gray @ 2018-05-21 13:51 UTC (permalink / raw)
To: Jonathan Cameron
Cc: benjamin.gaignard, fabrice.gasnier, linux-iio, linux-kernel,
devicetree, linux-arm-kernel
In-Reply-To: <20180520164253.5432d2a4@archlinux>
On Sun, May 20, 2018 at 04:42:53PM +0100, Jonathan Cameron wrote:
>On Wed, 16 May 2018 13:51:25 -0400
>William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
>
>> This patch adds support for the Generic Counter interface to the
>> 104-QUAD-8 driver. The existing 104-QUAD-8 device interface should not
>> be affected by this patch; all changes are intended as supplemental
>> additions as perceived by the user.
>>
>> Generic Counter Counts are created for the eight quadrature channel
>> counts, as well as their respective quadrature A and B Signals (which
>> are associated via respective Synapse structures) and respective index
>> Signals.
>>
>> The new Generic Counter interface sysfs attributes are intended to
>> expose the same functionality and data available via the existing
>> 104-QUAD-8 IIO device interface; the Generic Counter interface serves
>> to provide the respective functionality and data in a standard way
>> expected of counter devices.
>>
>> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
>
>A few general comments that applied just as well to the original driver
>as they do to the modified version.
>
>I wonder if this would be easier to review as two patches.
>Move the driver then add the counter interfaces?
>
>Right now people kind of have to review the old IIO driver and
>all the new stuff which is a big job..
>
>Jonathan
This looks like more simple cleanup, so I expect to incorporate your
suggestions without problem here as well. I'll also try to make the code
easier to read by writing some defines for the magic numbers throughout.
William Breathitt Gray
>> ---
>> MAINTAINERS | 4 +-
>> drivers/counter/104-quad-8.c | 1335 ++++++++++++++++++++++++++++++
>> drivers/counter/Kconfig | 21 +
>> drivers/counter/Makefile | 2 +
>> drivers/iio/counter/104-quad-8.c | 596 -------------
>> drivers/iio/counter/Kconfig | 17 -
>> drivers/iio/counter/Makefile | 1 -
>> 7 files changed, 1360 insertions(+), 616 deletions(-)
>> create mode 100644 drivers/counter/104-quad-8.c
>> delete mode 100644 drivers/iio/counter/104-quad-8.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7a01aa63fb33..f11bf7885aeb 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -266,12 +266,12 @@ L: linux-gpio@vger.kernel.org
>> S: Maintained
>> F: drivers/gpio/gpio-104-idio-16.c
>>
>> -ACCES 104-QUAD-8 IIO DRIVER
>> +ACCES 104-QUAD-8 DRIVER
>> M: William Breathitt Gray <vilhelm.gray@gmail.com>
>> L: linux-iio@vger.kernel.org
>> S: Maintained
>> F: Documentation/ABI/testing/sysfs-bus-iio-counter-104-quad-8
>> -F: drivers/iio/counter/104-quad-8.c
>> +F: drivers/counter/104-quad-8.c
>>
>> ACCES PCI-IDIO-16 GPIO DRIVER
>> M: William Breathitt Gray <vilhelm.gray@gmail.com>
>> diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
>> new file mode 100644
>> index 000000000000..7c72fb72d660
>> --- /dev/null
>> +++ b/drivers/counter/104-quad-8.c
>> @@ -0,0 +1,1335 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>
>If you are happy with SPDX drop the GPL text below to keep things
>short.
>
>> +/*
>> + * IIO driver for the ACCES 104-QUAD-8
>> + * Copyright (C) 2016 William Breathitt Gray
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License, version 2, as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * General Public License for more details.
>> + *
>> + * This driver supports the ACCES 104-QUAD-8 and ACCES 104-QUAD-4.
>> + */
>> +#include <linux/bitops.h>
>...
>> +static int quad8_probe(struct device *dev, unsigned int id)
>> +{
>> + struct iio_dev *indio_dev;
>> + struct quad8_iio *quad8iio;
>> + int i, j;
>> + unsigned int base_offset;
>> + int err;
>> +
>> + if (!devm_request_region(dev, base[id], QUAD8_EXTENT, dev_name(dev))) {
>> + dev_err(dev, "Unable to lock port addresses (0x%X-0x%X)\n",
>> + base[id], base[id] + QUAD8_EXTENT);
>> + return -EBUSY;
>> + }
>> +
>> + /* Allocate IIO device; this also allocates driver data structure */
>> + indio_dev = devm_iio_device_alloc(dev, sizeof(*quad8iio));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + /* Initialize IIO device */
>> + indio_dev->info = &quad8_info;
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> + indio_dev->num_channels = ARRAY_SIZE(quad8_channels);
>> + indio_dev->channels = quad8_channels;
>> + indio_dev->name = dev_name(dev);
>> + indio_dev->dev.parent = dev;
>> +
>> + /* Initialize Counter device and driver data */
>> + quad8iio = iio_priv(indio_dev);
>> + quad8iio->counter.name = dev_name(dev);
>> + quad8iio->counter.parent = dev;
>> + quad8iio->counter.ops = &quad8_ops;
>> + quad8iio->counter.counts = quad8_counts;
>> + quad8iio->counter.num_counts = ARRAY_SIZE(quad8_counts);
>> + quad8iio->counter.signals = quad8_signals;
>> + quad8iio->counter.num_signals = ARRAY_SIZE(quad8_signals);
>> + quad8iio->counter.priv = quad8iio;
>> + quad8iio->base = base[id];
>> +
>> + /* Reset all counters and disable interrupt function */
>> + outb(0x01, base[id] + 0x11);
>> + /* Set initial configuration for all counters */
>> + for (i = 0; i < QUAD8_NUM_COUNTERS; i++) {
>> + base_offset = base[id] + 2 * i;
>> + /* Reset Byte Pointer */
>> + outb(0x01, base_offset + 1);
>
>I'm going to be fussy. There are lots of values
>in here that look like register bits and you could exchange much of
>this documentation for a some good defines...
>
>Taking base_offset + 1 bits 5 and 6 look to select the actual register
>and the rest of them do the control.
>
>Anyhow, not critical but the readability of this code could be improved
>somewhat.
>
>> + /* Reset Preset Register */
>> + for (j = 0; j < 3; j++)
>> + outb(0x00, base_offset);
>> + /* Reset Borrow, Carry, Compare, and Sign flags */
>> + outb(0x04, base_offset + 1);
>> + /* Reset Error flag */
>> + outb(0x06, base_offset + 1);
>> + /* Binary encoding; Normal count; non-quadrature mode */
>> + outb(0x20, base_offset + 1);
>> + /* Disable A and B inputs; preset on index; FLG1 as Carry */
>> + outb(0x40, base_offset + 1);
>> + /* Disable index function; negative index polarity */
>> + outb(0x60, base_offset + 1);
>> + }
>> + /* Enable all counters */
>> + outb(0x00, base[id] + 0x11);
>> +
>> + /* Register IIO device */
>> + err = devm_iio_device_register(dev, indio_dev);
>> + if (err)
>> + return err;
>> +
>> + /* Register Counter device */
>> + return devm_counter_register(dev, &quad8iio->counter);
>> +}
>> +
>> +static struct isa_driver quad8_driver = {
>> + .probe = quad8_probe,
>> + .driver = {
>> + .name = "104-quad-8"
>> + }
>> +};
>> +
>> +module_isa_driver(quad8_driver, num_quad8);
>> +
>> +MODULE_AUTHOR("William Breathitt Gray <vilhelm.gray@gmail.com>");
>> +MODULE_DESCRIPTION("ACCES 104-QUAD-8 IIO driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
>> index 65fa92abd5a4..73f03372484f 100644
>> --- a/drivers/counter/Kconfig
>> +++ b/drivers/counter/Kconfig
>> @@ -16,3 +16,24 @@ menuconfig COUNTER
>> consumption. The Generic Counter interface enables drivers to support
>> and expose a common set of components and functionality present in
>> counter devices.
>> +
>> +if COUNTER
>> +
>> +config 104_QUAD_8
>> + tristate "ACCES 104-QUAD-8 driver"
>> + depends on PC104 && X86 && IIO
>> + select ISA_BUS_API
>> + help
>> + Say yes here to build support for the ACCES 104-QUAD-8 quadrature
>> + encoder counter/interface device family (104-QUAD-8, 104-QUAD-4).
>> +
>> + Performing a write to a counter's IIO_CHAN_INFO_RAW sets the counter and
>> + also clears the counter's respective error flag. Although the counters
>> + have a 25-bit range, only the lower 24 bits may be set, either directly
>> + or via a counter's preset attribute. Interrupts are not supported by
>> + this driver.
>
>This text probably wants to be updated to reflect the new counter subsystem support..
>
>> +
>> + The base port addresses for the devices may be configured via the base
>> + array module parameter.
>> +
>> +endif # COUNTER
>> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
>> index ad1ba7109cdc..23a4f6263e45 100644
>> --- a/drivers/counter/Makefile
>> +++ b/drivers/counter/Makefile
>> @@ -6,3 +6,5 @@
>>
>> obj-$(CONFIG_COUNTER) += counter.o
>> counter-y := generic-counter.o
>> +
>> +obj-$(CONFIG_104_QUAD_8) += 104-quad-8.o
>> diff --git a/drivers/iio/counter/104-quad-8.c b/drivers/iio/counter/104-quad-8.c
>...
^ permalink raw reply
* Re: [PATCH v6 3/5] soc: rockchip: split rockchip_typec_phy struct to separate header
From: Enric Balletbo Serra @ 2018-05-21 13:50 UTC (permalink / raw)
To: Lin Huang
Cc: devicetree@vger.kernel.org, David Airlie, linux-kernel,
Brian Norris, Doug Anderson, Kishon Vijay Abraham I,
open list:ARM/Rockchip SoC..., Rob Herring, dri-devel,
Chris Zhong, daniel.vetter, Linux ARM
In-Reply-To: <1526895424-22894-3-git-send-email-hl@rock-chips.com>
Hi Lin,
2018-05-21 11:37 GMT+02:00 Lin Huang <hl@rock-chips.com>:
> we may use rockchip_phy_typec struct in other driver, so split
> it to separate header.
>
That patch does more than just split some structs to a public header,
it also introduces new structs and new parameters related to the
phy_config feature. IMHO you should first move the current structs and
introduce the new phy_config stuff in the following patch (4/5). I am
not sure about the maintainer preferences, but at least, if the
maintainer is fine like is now, I'd explain that you introduce new
elements in the commit message.
Best regards,
Enric
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
> Changes in v2:
> - None
> Changes in v3:
> - None
> Changes in v4:
> - None
> Changes in v5:
> - None
> Changes in v6:
> - new patch here
>
> drivers/phy/rockchip/phy-rockchip-typec.c | 47 +----------------------
> include/soc/rockchip/rockchip_phy_typec.h | 63 +++++++++++++++++++++++++++++++
> 2 files changed, 64 insertions(+), 46 deletions(-)
> create mode 100644 include/soc/rockchip/rockchip_phy_typec.h
>
> diff --git a/drivers/phy/rockchip/phy-rockchip-typec.c b/drivers/phy/rockchip/phy-rockchip-typec.c
> index 76a4b58..795055f 100644
> --- a/drivers/phy/rockchip/phy-rockchip-typec.c
> +++ b/drivers/phy/rockchip/phy-rockchip-typec.c
> @@ -63,6 +63,7 @@
>
> #include <linux/mfd/syscon.h>
> #include <linux/phy/phy.h>
> +#include <soc/rockchip/rockchip_phy_typec.h>
>
> #define CMN_SSM_BANDGAP (0x21 << 2)
> #define CMN_SSM_BIAS (0x22 << 2)
> @@ -349,52 +350,6 @@
> #define MODE_DFP_USB BIT(1)
> #define MODE_DFP_DP BIT(2)
>
> -struct usb3phy_reg {
> - u32 offset;
> - u32 enable_bit;
> - u32 write_enable;
> -};
> -
> -/**
> - * struct rockchip_usb3phy_port_cfg: usb3-phy port configuration.
> - * @reg: the base address for usb3-phy config.
> - * @typec_conn_dir: the register of type-c connector direction.
> - * @usb3tousb2_en: the register of type-c force usb2 to usb2 enable.
> - * @external_psm: the register of type-c phy external psm clock.
> - * @pipe_status: the register of type-c phy pipe status.
> - * @usb3_host_disable: the register of type-c usb3 host disable.
> - * @usb3_host_port: the register of type-c usb3 host port.
> - * @uphy_dp_sel: the register of type-c phy DP select control.
> - */
> -struct rockchip_usb3phy_port_cfg {
> - unsigned int reg;
> - struct usb3phy_reg typec_conn_dir;
> - struct usb3phy_reg usb3tousb2_en;
> - struct usb3phy_reg external_psm;
> - struct usb3phy_reg pipe_status;
> - struct usb3phy_reg usb3_host_disable;
> - struct usb3phy_reg usb3_host_port;
> - struct usb3phy_reg uphy_dp_sel;
> -};
> -
> -struct rockchip_typec_phy {
> - struct device *dev;
> - void __iomem *base;
> - struct extcon_dev *extcon;
> - struct regmap *grf_regs;
> - struct clk *clk_core;
> - struct clk *clk_ref;
> - struct reset_control *uphy_rst;
> - struct reset_control *pipe_rst;
> - struct reset_control *tcphy_rst;
> - const struct rockchip_usb3phy_port_cfg *port_cfgs;
> - /* mutex to protect access to individual PHYs */
> - struct mutex lock;
> -
> - bool flip;
> - u8 mode;
> -};
> -
> struct phy_reg {
> u16 value;
> u32 addr;
> diff --git a/include/soc/rockchip/rockchip_phy_typec.h b/include/soc/rockchip/rockchip_phy_typec.h
> new file mode 100644
> index 0000000..be6af0e
> --- /dev/null
> +++ b/include/soc/rockchip/rockchip_phy_typec.h
> @@ -0,0 +1,63 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
> + * Author: Lin Huang <hl@rock-chips.com>
> + */
> +
> +#ifndef __SOC_ROCKCHIP_PHY_TYPEC_H
> +#define __SOC_ROCKCHIP_PHY_TYPEC_H
> +
> +struct usb3phy_reg {
> + u32 offset;
> + u32 enable_bit;
> + u32 write_enable;
> +};
> +
> +/**
> + * struct rockchip_usb3phy_port_cfg: usb3-phy port configuration.
> + * @reg: the base address for usb3-phy config.
> + * @typec_conn_dir: the register of type-c connector direction.
> + * @usb3tousb2_en: the register of type-c force usb2 to usb2 enable.
> + * @external_psm: the register of type-c phy external psm clock.
> + * @pipe_status: the register of type-c phy pipe status.
> + * @usb3_host_disable: the register of type-c usb3 host disable.
> + * @usb3_host_port: the register of type-c usb3 host port.
> + * @uphy_dp_sel: the register of type-c phy DP select control.
> + */
> +struct rockchip_usb3phy_port_cfg {
> + unsigned int reg;
> + struct usb3phy_reg typec_conn_dir;
> + struct usb3phy_reg usb3tousb2_en;
> + struct usb3phy_reg external_psm;
> + struct usb3phy_reg pipe_status;
> + struct usb3phy_reg usb3_host_disable;
> + struct usb3phy_reg usb3_host_port;
> + struct usb3phy_reg uphy_dp_sel;
> +};
> +
> +struct phy_config {
> + int swing;
> + int pe;
> +};
> +
> +struct rockchip_typec_phy {
> + struct device *dev;
> + void __iomem *base;
> + struct extcon_dev *extcon;
> + struct regmap *grf_regs;
> + struct clk *clk_core;
> + struct clk *clk_ref;
> + struct reset_control *uphy_rst;
> + struct reset_control *pipe_rst;
> + struct reset_control *tcphy_rst;
> + const struct rockchip_usb3phy_port_cfg *port_cfgs;
> + /* mutex to protect access to individual PHYs */
> + struct mutex lock;
> + struct phy_config config[3][4];
> + bool flip;
> + u8 mode;
> + int (*typec_phy_config)(struct phy *phy, int link_rate,
> + int lanes, u8 swing, u8 pre_emp);
> +};
> +
> +#endif
> --
> 2.7.4
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))
From: Johan Hovold @ 2018-05-21 13:48 UTC (permalink / raw)
To: Tony Lindgren
Cc: Johan Hovold, Sebastian Reichel, H. Nikolaus Schaller,
Andreas Kemnade, Mark Rutland, Arnd Bergmann, Pavel Machek,
linux-kernel@vger.kernel.org,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Rob Herring, linux-serial, linux-omap,
linux-pm
In-Reply-To: <20180517171038.GL98604@atomide.com>
On Thu, May 17, 2018 at 10:10:38AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [180517 10:12]:
> > [ Sorry about the late reply. ]
> >
> > On Wed, May 09, 2018 at 06:57:06AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold <johan@kernel.org> [180509 13:12]:
> >
> > > > It seems we really should not be using the negative autosuspend to
> > > > configure the RPM behaviour the way these drivers do. Perhaps a new
> > > > mechanism is needed.
> > >
> > > Hmm well simply defaulting to "on" instead of "auto" and setting the
> > > autosuspend_ms to 3000 by default might be doable. I think that way
> > > we can keep use_autosuspend() in probe. Let's hope there are no
> > > existing use cases that would break with that.
> >
> > No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
> > either as that would also prevent the device from runtime suspending
> > just as the current negative autosuspend delay does.
>
> Well in that case we should just stick with -1 value for the
> autosuspend. And just do pm_runtime_put_sync_suspend() after
> probe and on close.
That won't work either as a negative autosuspend delay prevents runtime
suspend completely (by grabbing an extra RPM reference).
> > I fail to see how we can implement this using the current toolbox. What
> > you're after here is really a mechanism for selecting between two
> > different runtime PM schemes at runtime:
> >
> > 1. normal serial RPM, where the controller is active while the
> > port is open (this should be the safe default)
>
> Agreed. And that is the case already.
Yes, but it's not really the case today as since omap-serial (and
8250-omap) sets a negative autosuspend at probe and hence does not
runtime-suspend when the port is closed. So that's the long-standing bug
which needs fixing.
> > 2. aggressive serial RPM, where the controller is allowed to
> > suspend while the port is open even though this may result in
> > lost characters when waking up on incoming data
>
> In this case it seems that the only thing needed is to just
> configure the autosuspend delay for the parent port. The use of
> -1 has been around since the start of runtime PM AFAIK, so maybe
> we should just document it. I guess we could also introduce
> pm_runtime_block_autoidle_unless_configured() :)
The implications of a negative autosuspend delay are already documented
(in Documentation/power/runtime_pm.txt); it's just the omap drivers that
gets it wrong when trying to do things which aren't currently supported
(and never have been).
So I still think we need a new mechanism for this.
> > For normal ttys, we need a user-space interface for selecting between
> > the two, and for serdev we may want a way to select the RPM scheme from
> > within the kernel.
> >
> > Note that with my serdev controller runtime PM patch, serdev core could
> > always opt for aggressive PM (as by default serdev core holds an RPM
> > reference for the controller while the port is open).
>
> So if your serdev controller was to set the parent autosuspend
> delay on open() and set it back on close() this should work?
Is it really the job of a serdev driver to set the autosuspend delay of
a parent controller? Isn't this somethings which depends on the
characteristics of the controller (possibly configurable by user space)
such as the cost of runtime suspending and resuming?
The patch I posted works with what we have today; if a parent serial
controller driver uses aggressive runtime PM by default or after having
been configured through sysfs to do so.
What I'm getting at here is that the delay should be set by the serial
driver implementing aggressive runtime PM. Then all we need is a
mechanism to determine whether an extra RPM reference should be taken at
tty open or not (configurable by user space, defaulting to yes).
Specifically, the serial drivers themselves would always use
autosuspend and not have to deal with supporting the two RPM schemes
(normal vs aggressive runtime PM).
Thanks,
Johan
^ permalink raw reply
* Re: [PATCH v6 3/9] docs: Add Generic Counter interface documentation
From: William Breathitt Gray @ 2018-05-21 13:47 UTC (permalink / raw)
To: Jonathan Cameron
Cc: benjamin.gaignard, fabrice.gasnier, linux-iio, linux-kernel,
devicetree, linux-arm-kernel
In-Reply-To: <20180520163109.22b11af8@archlinux>
On Sun, May 20, 2018 at 04:31:09PM +0100, Jonathan Cameron wrote:
>On Wed, 16 May 2018 13:51:06 -0400
>William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
>
>> This patch adds high-level documentation about the Generic Counter
>> interface.
>>
>> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
>
>Various comments inline. I've been doing a lot long reviews recently
>(outside of the kernel world) and keep discovering the old rule that
>everytime you read a document you'll find something else to
>improve :(
>
>Jonathan
But it is good too to have multiple eyes on this -- I have found as an
author my brain tends to skip over minor errors while rereading
passages, so having persons reading it for both the first and subsequent
times helps catch those mistakes I may have overlooked in my mind. :)
>> ---
>> Documentation/driver-api/generic-counter.rst | 336 +++++++++++++++++++
>> Documentation/driver-api/index.rst | 1 +
>> MAINTAINERS | 1 +
>> 3 files changed, 338 insertions(+)
>> create mode 100644 Documentation/driver-api/generic-counter.rst
>>
>> diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst
>> new file mode 100644
>> index 000000000000..5c6b9c008c06
>> --- /dev/null
>> +++ b/Documentation/driver-api/generic-counter.rst
>> @@ -0,0 +1,336 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +=========================
>> +Generic Counter Interface
>> +=========================
>> +
>> +Introduction
>> +============
>> +
>> +Counter devices are prevalent within a diverse spectrum of industries.
>> +The ubiquitous presence of these devices necessitates a common interface
>> +and standard of interaction and exposure. This driver API attempts to
>> +resolve the issue of duplicate code found among existing counter device
>> +drivers by introducing a generic counter interface for consumption. The
>> +Generic Counter interface enables drivers to support and expose a common
>> +set of components and functionality present in counter devices.
>> +
>> +Theory
>> +======
>> +
>> +Counter devices can vary greatly in design, but regardless of whether
>> +some devices are quadrature encoder counters or tally counters, all
>> +counter devices consist of a core set of components. This core set of
>> +components, shared by all counter devices, is what forms the essence of
>> +the Generic Counter interface.
>> +
>> +There are three core components to a counter:
>
>Enumerate them here. If people are reading this as a paged document (pdf etc)
>then the list of 3 as titles of next few sections may not be clear.
>
>* Count
>
>* Signal
>
>* Synapse
>
>> +
>> +COUNT
>> +-----
>> +A Count represents the count data for a set of Signals. The Generic
>> +Counter interface provides the following available count data types:
>> +
>> +* COUNT_POSITION_UNSIGNED:
>> + Unsigned integer value representing position.
>> +
>> +* COUNT_POSITION_SIGNED:
>> + Signed integer value representing position.
>
>Just a thought: As the '0' position is effectively arbitrary is there any
>actual difference between signed and unsigned? If we defined all counters
>to be unsigned and just offset any signed ones so the range still fitted
>would we end up with a simpler interface - there would be no real loss
>of meaning that I can see.. I suppose it might not be what people expect
>though when they see their counters start at a large positive value...
This is something I've been on the fence about for a while. I would
actually prefer the interface to have simply a COUNT_POSITION data type
to represent position and leave it as unsigned; distinguishing between
signed and unsigned position seems ultimately arbitrary given that it's
mathematically just an offset as you have pointed out.
If we were to go down this route, then we'd have a count value that may
always be represented using an unsigned data type, with an offset value
that may always be represented using a signed data type -- the
relationship being such: position = count + offset. Is that correct?
My reason for giving the option for either signed or unsigned position
was to help minimize the work userspace would have to do in order to get
the value in which they're actually interested. I suppose it was a
question of how abstract I want to make the interface -- although,
making it simpler for userspace put more of a burden on the kernel side.
However, the "offset" value route may actually be more robust in the end
because userspace would have to know whether they want a signed or
unsigned position regardless in order to parse, so with count and offet
defined we know they will always be unsigned and signed respectively.
>
>
>
>
>> +
>> +A Count has a count function mode which represents the update behavior
>> +for the count data. The Generic Counter interface provides the following
>> +available count function modes:
>> +
>> +* Increase:
>> + Accumulated count is incremented.
>> +
>> +* Decrease:
>> + Accumulated count is decremented.
>> +
>> +* Pulse-Direction:
>> + Rising edges on quadrature pair signal A updates the respective count.
>> + The input level of quadrature pair signal B determines direction.
>> +
>Perhaps group the quadrature modes for the point of view of this document?
>Might be slightly easier to read?
>
>* Quadrature Modes
>
> - x1 A: etc.
>
>> +* Quadrature x1 A:
>> + If direction is forward, rising edges on quadrature pair signal A
>> + updates the respective count; if the direction is backward, falling
>> + edges on quadrature pair signal A updates the respective count.
>> + Quadrature encoding determines the direction.
>> +
>> +* Quadrature x1 B:
>> + If direction is forward, rising edges on quadrature pair signal B
>> + updates the respective count; if the direction is backward, falling
>> + edges on quadrature pair signal B updates the respective count.
>> + Quadrature encoding determines the direction.
>> +
>> +* Quadrature x2 A:
>> + Any state transition on quadrature pair signal A updates the
>> + respective count. Quadrature encoding determines the direction.
>> +
>> +* Quadrature x2 B:
>> + Any state transition on quadrature pair signal B updates the
>> + respective count. Quadrature encoding determines the direction.
>> +
>> +* Quadrature x2 Rising:
>> + Rising edges on either quadrature pair signals updates the respective
>> + count. Quadrature encoding determines the direction.
>
>This one I've never met. Really? There are devices who do this form
>of crazy? It gives really uneven counting and I'm failing to see when
>it would ever make sense... References for these odd corner cases
>would be good.
>
>
>__|---|____|-----|____
>____|----|____|-----|____
>
>001122222223334444444
That's the same reaction I had when I discovered this -- in fact the
STM32 LP Timer is the first time I've come across such a quadrature
mode. I'm not sure of the use case for this mode, because positioning
wouldn't be precise as you've pointed out. Perhaps Fabrice or Benjamin
can probe the ST guys responsible for this design choice to figure out
the rationale.
I'm leaving in these modes for now, as they do exist in the STM32 LP
Timer, but it does make me curious what the intentions for them were
(perhaps use cases outside of traditional quadrature encoder
positioning).
>
>
>> +
>> +* Quadrature x2 Falling:
>> + Falling edges on either quadrature pair signals updates the respective
>> + count. Quadrature encoding determines the direction.
>> +
>> +* Quadrature x4:
>> + Any state transition on either quadrature pair signals updates the
>> + respective count. Quadrature encoding determines the direction.
>> +
>> +A Count has a set of one or more associated Signals.
>> +
>> +SIGNAL
>> +------
>> +A Signal represents a counter input data; this is the input data that is
>> +analyzed by the counter to determine the count data; e.g. a quadrature
>> +signal output line of a rotary encoder. Not all counter devices provide
>> +user access to the Signal data.
>> +
>> +The Generic Counter interface provides the following available signal
>> +data types for when the Signal data is available for user access:
>> +
>> +* SIGNAL_LEVEL_LOW:
>> + Signal line is in a low state.
>> +
>> +* SIGNAL_LEVEL_HIGH:
>> + Signal line is in a high state.
>> +
>> +A Signal may be associated with one or more Counts.
>> +
>> +SYNAPSE
>> +-------
>> +A Synapse represents the association of a Signal with a respective
>> +Count. Signal data affects respective Count data, and the Synapse
>> +represents this relationship.
>> +
>> +The Synapse action mode specifies the Signal data condition which
>> +triggers the respective Count's count function evaluation to update the
>> +count data. The Generic Counter interface provides the following
>> +available action modes:
>> +
>> +* None:
>> + Signal does not trigger the count function. In Pulse-Direction count
>> + function mode, this Signal is evaluated as Direction.
>> +
>> +* Rising Edge:
>> + Low state transitions to high state.
>> +
>> +* Falling Edge:
>> + High state transitions to low state.
>> +
>> +* Both Edges:
>> + Any state transition.
>> +
>> +A counter is defined as a set of input signals associated with count
>> +data that are generated by the evaluation of the state of the associated
>> +input signals as defined by the respective count functions. Within the
>> +context of the Generic Counter interface, a counter consists of Counts
>> +each associated with a set of Signals, whose respective Synapse
>> +instances represent the count function update conditions for the
>> +associated Counts.
>> +
>> +Paradigm
>> +========
>> +
>> +The most basic counter device may be expressed as a single Count
>> +associated with a single Signal via a single Synapse. Take for example
>> +a counter device which simply accumulates a count of rising edges on a
>> +source input line::
>> +
>> + Count Synapse Signal
>> + ----- ------- ------
>> + +---------------------+
>> + | Data: Count | Rising Edge ________
>> + | Function: Increase | <------------- / Source \
>> + | | ____________
>> + +---------------------+
>> +
>> +In this example, the Signal is a source input line with a pulsing
>> +voltage, while the Count is a persistent count value which is repeatedly
>> +incremented. The Signal is associated with the respective Count via a
>> +Synapse. The increase function is triggered by the Signal data condition
>> +specified by the Synapse -- in this case a rising edge condition on the
>> +voltage input line. In summary, the counter device existence and
>> +behavior is aptly represented by respective Count, Signal, and Synapse
>> +components: a rising edge condition triggers an increase function on an
>> +accumulating count datum.
>> +
>> +A counter device is not limited to a single Signal; in fact, in theory
>> +many Signals may be associated with even a single Count. For example, a
>> +quadrature encoder counter device can keep track of position based on
>> +the states of two input lines::
>> +
>> + Count Synapse Signal
>> + ----- ------- ------
>> + +-------------------------+
>> + | Data: Position | Both Edges ___
>> + | Function: Quadrature x4 | <------------ / A \
>> + | | _______
>> + | |
>> + | | Both Edges ___
>> + | | <------------ / B \
>> + | | _______
>> + +-------------------------+
>> +
>> +In this example, two Signals (quadrature encoder lines A and B) are
>> +associated with a single Count: a rising or falling edge on either A or
>> +B triggers the "Quadrature x4" function which determines the direction
>> +of movement and updates the respective position data. The "Quadrature
>> +x4" function is likely implemented in the hardware of the quadrature
>> +encoder counter device; the Count, Signals, and Synapses simply
>> +represent this hardware behavior and functionality.
>> +
>> +Signals associated with the same Count can have differing Synapse action
>> +mode conditions. For example, a quadrature encoder counter device
>> +operating in a non-quadrature Pulse-Direction mode could have one input
>> +line dedicated for movement and a second input line dedicated for
>> +direction::
>> +
>> + Count Synapse Signal
>> + ----- ------- ------
>> + +---------------------------+
>> + | Data: Position | Rising Edge ___
>> + | Function: Pulse-Direction | <------------- / A \ (Movement)
>> + | | _______
>> + | |
>> + | | None ___
>> + | | <------------- / B \ (Direction)
>> + | | _______
>> + +---------------------------+
>> +
>> +Only Signal A triggers the "Pulse-Direction" update function, but the
>> +instantaneous state of Signal B is still required in order to know the
>> +direction so that the position data may be properly updated. Ultimately,
>> +both Signals are associated with the same Count via two respective
>> +Synapses, but only one Synapse has an active action mode condition which
>> +triggers the respective count function while the other is left with a
>> +"None" condition action mode to indicate its respective Signal's
>> +availability for state evaluation despite its non-triggering mode.
>> +
>> +Keep in mind that the Signal, Synapse, and Count are abstract
>> +representations which do not need to be closely married to their
>> +respective physical sources. This allows the user of a counter to
>> +divorce themselves from the nuances of physical components (such as
>> +whether an input line is differential or single-ended) and instead focus
>> +on the core idea of what the data and process represent (e.g. position
>> +as interpreted from quadrature encoding data).
>> +
>> +Userspace Interface
>> +===================
>> +
>> +Several sysfs attributes are generated by the Generic Counter interface,
>> +and reside under the /sys/bus/counter/devices/counterX directory, where
>> +counterX refers to the respective counter device. Please see
>> +Documentation/ABI/testing/sys-bus-counter-generic-sysfs for detailed
>> +information on each Generic Counter interface sysfs attribute.
>> +
>> +Through these sysfs attributes, programs and scripts may interact with
>> +the Generic Counter paradigm Counts, Signals, and Synapses of respective
>> +counter devices.
>> +
>> +Driver API
>> +==========
>> +
>> +Driver authors may utilize the Generic Counter interface in their code
>> +by including the include/linux/iio/counter.h header file. This header
>
>Didn't this move?
Yes you are correct, looks like an oversight I made. I'll cleanup this
and the rest with the next revision then.
William Breathitt Gray
>
>> +file provides several core data structures, function prototypes, and
>> +macros for defining a counter device.
>> +
>> +.. kernel-doc:: include/linux/counter.h
>> + :internal:
>> +
>> +.. kernel-doc:: drivers/counter/generic-counter.c
>> + :export:
>> +
>> +Implementation
>> +==============
>> +
>> +To support a counter device, a driver must first allocate the available
>> +Counter Signals via counter_signal structures. These Signals should
>> +be stored as an array and set to the signals array member of an
>> +allocated counter_device structure before the Counter is registered to
>> +the system.
>> +
>> +Counter Counts may be allocated via counter_count structures, and
>> +respective Counter Signal associations (Synapses) made via
>> +counter_synapse structures. Associated counter_synapse structures are
>> +stored as an array and set to the the synapses array member of the
>> +respective counter_count structure. These counter_count structures are
>> +set to the counts array member of an allocated counter_device structure
>> +before the Counter is registered to the system.
>> +
>> +Driver callbacks should be provided to the counter_device structure via
>> +a constant counter_ops structure in order to communicate with the
>> +device: to read and write various Signals and Counts, and to set and get
>> +the "action mode" and "function mode" for various Synapses and Counts
>> +respectively.
>> +
>> +A defined counter_device structure may be registered to the system by
>> +passing it to the counter_register function, and unregistered by passing
>> +it to the counter_unregister function. Similarly, the
>> +devm_counter_register and devm_counter_unregister functions may be used
>> +if device memory-managed registration is desired.
>> +
>> +Extension sysfs attributes can be created for auxiliary functionality
>> +and data by passing in defined counter_device_ext, counter_count_ext,
>> +and counter_signal_ext structures. In these cases, the
>> +counter_device_ext structure is used for global configuration of the
>> +respective Counter device, while the counter_count_ext and
>> +counter_signal_ext structures allow for auxiliary exposure and
>> +configuration of a specific Count or Signal respectively.
>> +
>> +Architecture
>> +============
>> +
>> +When the Generic Counter interface counter module is loaded, the
>> +counter_init function is called which registers a bus_type named
>> +"counter" to the system. Subsequently, when the module is unloaded, the
>> +counter_exit function is called which unregisters the bus_type named
>> +"counter" from the system.
>> +
>> +Counter devices are registered to the system via the counter_register
>> +function, and later removed via the counter_unregister function. The
>> +counter_register function establishes a unique ID for the Counter
>> +device and creates a respective sysfs directory, where X is the
>> +mentioned unique ID:
>> +
>> + /sys/bus/counter/devices/counterX
>> +
>> +Sysfs attributes are created within the counterX directory to expose
>> +functionality, configurations, and data relating to the Counts, Signals,
>> +and Synapses of the Counter device, as well as options and information
>> +for the Counter device itself.
>> +
>> +Each Signal has a directory created to house its relevant sysfs
>> +attributes, where Y is the unique ID of the respective Signal:
>> +
>> + /sys/bus/counter/devices/counterX/signalY
>> +
>> +Similarly, each Count has a directory created to house its relevant
>> +sysfs attributes, where Y is the unique ID of the respective Count:
>> +
>> + /sys/bus/counter/devices/counterX/countY
>> +
>> +For a more detailed breakdown of the available Generic Counter interface
>> +sysfs attributes, please refer to the
>> +Documentation/ABI/testing/sys-bus-counter file.
>> +
>> +The Signals and Counts associated with the Counter device are registered
>> +to the system as well by the counter_register function. The
>> +signal_read/signal_write driver callbacks are associated with their
>> +respective Signal attributes, while the count_read/count_write and
>> +function_get/function_set driver callbacks are associated with their
>> +respective Count attributes; similarly, the same is true for the
>> +action_get/action_set driver callbacks and their respective Synapse
>> +attributes. If a driver callback is left undefined, then the respective
>> +read/write permission is left disabled for the relevant attributes.
>> +
>> +Similarly, extension sysfs attributes are created for the defined
>> +counter_device_ext, counter_count_ext, and counter_signal_ext
>> +structures that are passed in.
>> diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst
>> index 6d8352c0f354..5fd747c4f2ce 100644
>> --- a/Documentation/driver-api/index.rst
>> +++ b/Documentation/driver-api/index.rst
>> @@ -25,6 +25,7 @@ available subsections can be seen below.
>> frame-buffer
>> regulator
>> iio/index
>> + generic-counter
>> input
>> usb/index
>> pci
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 1413e3eb49e5..7a01aa63fb33 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -3674,6 +3674,7 @@ M: William Breathitt Gray <vilhelm.gray@gmail.com>
>> L: linux-iio@vger.kernel.org
>> S: Maintained
>> F: Documentation/ABI/testing/sysfs-bus-counter*
>> +F: Documentation/driver-api/generic-counter.rst
>> F: drivers/counter/
>> F: include/linux/counter.h
>>
>
^ permalink raw reply
* Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding
From: Gilad Ben-Yossef @ 2018-05-21 13:43 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Simon Horman, Magnus Damm, Rob Herring, Mark Rutland,
Catalin Marinas, Will Deacon, Geert Uytterhoeven,
Michael Turquette, Stephen Boyd, Herbert Xu, David S. Miller,
Ofir Drang, Linux-Renesas,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, linux-clk
In-Reply-To: <CAMuHMdVM55S7+PdnKusX-qkTxioc=0kQ6EGSbwv+kbLk5RCUYw@mail.gmail.com>
On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Indeed. From a quick glance, it looks like drivers/crypto/ccree/cc_driver.c
> does not distinguish between the absence of the clock property, and an
> actual error in getting the clock, and never considers any error a failure
> (incl. -PROBE_DEFER).
>
> As of_clk_get() returns -ENOENT for both a missing clock property and a
> missing clock, you should use (devm_)clk_get() instead, and distinguish
> between NULL (no clock property) and IS_ERR() (actual failure -> abort).
>
I was trying to do as you suggested but I didn't quite get what is the
dev_id (2nd) parameter to devm_clk_get parameter is supposed to be.
I see what of_clk_get() is doing, so can replicate that but it seems
an over kill.
Any ideas?
Thanks,
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
-- Jean-Baptiste Queru
^ permalink raw reply
* Re: [PATCH v11 0/4] iommu/arm-smmu: Add runtime pm/sleep support
From: Robin Murphy @ 2018-05-21 13:42 UTC (permalink / raw)
To: Vivek Gautam, joro-zLv9SwRftAIdnm+yROfE0A,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
will.deacon-5wv7dgnIgG8,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: rjw-LthD3rsA81gm4RdzfppkhA, sboyd-sgV2jX0FEOL9JmXXK+q4OQ,
lukas-JFq808J9C/izQB+pC5nmwQ,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20180322102204.14760-1-vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
On 22/03/18 10:22, Vivek Gautam wrote:
> This series provides the support for turning on the arm-smmu's
> clocks/power domains using runtime pm. This is done using the
> recently introduced device links patches, which lets the smmu's
> runtime to follow the master's runtime pm, so the smmu remains
> powered only when the masters use it.
> As not all implementations support clock/power gating, we are checking
> for a valid 'smmu->dev's pm_domain' to conditionally enable the runtime
> power management for such smmu implementations that can support it.
>
> This series also adds support for Qcom's arm-smmu-v2 variant that
> has different clocks and power requirements.
>
> Took some reference from the exynos runtime patches [1].
>
> With conditional runtime pm now, we avoid touching dev->power.lock
> in fastpaths for smmu implementations that don't need to do anything
> useful with pm_runtime.
> This lets us to use the much-argued pm_runtime_get_sync/put_sync()
> calls in map/unmap callbacks so that the clients do not have to
> worry about handling any of the arm-smmu's power.
>
> Previous version of this patch series is @ [5].
>
> [v11]
> * Some more cleanups for device link. We don't need an explicit
> delete for device link from the driver, but just set the flag
> DL_FLAG_AUTOREMOVE.
> device_link_add() API description says -
> "If the DL_FLAG_AUTOREMOVE is set, the link will be removed
> automatically when the consumer device driver unbinds."
But then if the consumer subsequently rebinds, there's no opportunity to
recreate the link, and PM will just quietly stop functioning properly.
Given that it's a lot more reasonable for users to unload and reload
modules (or otherwise switch drivers) for a consumer device than it is
to unbind the SMMU driver or expect actual device hotplug, I'd rather do
nothing and theoretically leak links than autoremove them incorrectly.
AFAICS the previous discussion on this got as far as Lukas' suggestion
of adding an equivalent flag for supplier-managed links (which FWIW
sounds good to me), but seems to have stalled there.
Robin.
> * Addressed the comments for 'smmu' in arm_smmu_map/unmap().
> * Dropped the patch [10] that introduced device_link_del_dev() API.
>
> [v10]
> * Introduce device_link_del_dev() API to delete the link between
> given consumer and supplier devices. The users of device link
> do not need to store link pointer to delete the link later.
> They can straightaway use this API by passing consumer and
> supplier devices.
> * Made corresponding changes to arm-smmu driver patch handling the
> device links.
> * Dropped the patch [9] that was adding device_link_find() API to
> device core layer. device_link_del_dev() serves the purpose to
> directly delete the link between two given devices.
>
> [v9]
> * Removed 'rpm_supported' flag, instead checking on pm_domain
> to enable runtime pm.
> * Creating device link only when the runtime pm is enabled, as we
> don't need a device link besides managing the power dependency
> between supplier and consumer devices.
> * Introducing a patch to add device_link_find() API that finds
> and existing link between supplier and consumer devices.
> Also, made necessary change to device_link_add() to use this API.
> * arm_smmu_remove_device() now uses this device_link_find() to find
> the device link between smmu device and the master device, and then
> delete this link.
> * Dropped the destroy_domain_context() fix [8] as it was rather,
> introducing catastrophically bad problem by destroying
> 'good dev's domain context.
> * Added 'Reviwed-by' tag for Tomasz's review.
>
> [v8]
> * Major change -
> - Added a flag 'rpm_supported' which each platform that supports
> runtime pm, can enable, and we enable runtime_pm over arm-smmu
> only when this flag is set.
> - Adding the conditional pm_runtime_get/put() calls to .map, .unmap
> and .attach_dev ops.
> - Dropped the patch [6] that exported pm_runtim_get/put_suupliers(),
> and also dropped the user driver patch [7] for these APIs.
>
> * Clock code further cleanup
> - doing only clk_bulk_enable() and clk_bulk_disable() in runtime pm
> callbacks. We shouldn't be taking a slow path (clk_prepare/unprepare())
> from these runtime pm callbacks. Thereby, moved clk_bulk_prepare() to
> arm_smmu_device_probe(), and clk_bulk_unprepare() to
> arm_smmu_device_remove().
> - clk data filling to a common method arm_smmu_fill_clk_data() that
> fills the clock ids and number of clocks.
>
> * Addressed other nits and comments
> - device_link_add() error path fixed.
> - Fix for checking negative error value from pm_runtime_get_sync().
> - Documentation redo.
>
> * Added another patch fixing the error path in arm_smmu_attach_dev()
> to destroy allocated domain context.
>
> [v7]
> * Addressed review comments given by Robin Murphy -
> - Added device_link_del() in .remove_device path.
> - Error path cleanup in arm_smmu_add_device().
> - Added pm_runtime_get/put_sync() in .remove path, and replaced
> pm_runtime_force_suspend() with pm_runtime_disable().
> - clk_names cleanup in arm_smmu_init_clks()
> * Added 'Reviewed-by' given by Rob H.
>
> [V6]
> * Added Ack given by Rafael to first patch in the series.
> * Addressed Rob Herring's comment for adding soc specific compatible
> string as well besides 'qcom,smmu-v2'.
>
> [V5]
> * Dropped runtime pm calls from "arm_smmu_unmap" op as discussed over
> the list [3] for the last patch series.
> * Added a patch to export pm_runtime_get/put_suppliers() APIs to the
> series as agreed with Rafael [4].
> * Added the related patch for msm drm iommu layer to use
> pm_runtime_get/put_suppliers() APIs in msm_mmu_funcs.
> * Dropped arm-mmu500 clock patch since that would break existing
> platforms.
> * Changed compatible 'qcom,msm8996-smmu-v2' to 'qcom,smmu-v2' to reflect
> the IP version rather than the platform on which it is used.
> The same IP is used across multiple platforms including msm8996,
> and sdm845 etc.
> * Using clock bulk APIs to handle the clocks available to the IP as
> suggested by Stephen Boyd.
> * The first patch in v4 version of the patch-series:
> ("iommu/arm-smmu: Fix the error path in arm_smmu_add_device") has
> already made it to mainline.
>
> [V4]
> * Reworked the clock handling part. We now take clock names as data
> in the driver for supported compatible versions, and loop over them
> to get, enable, and disable the clocks.
> * Using qcom,msm8996 based compatibles for bindings instead of a generic
> qcom compatible.
> * Refactor MMU500 patch to just add the necessary clock names data and
> corresponding bindings.
> * Added the pm_runtime_get/put() calls in .unmap iommu op (fix added by
> Stanimir on top of previous patch version.
> * Added a patch to fix error path in arm_smmu_add_device()
> * Removed patch 3/5 of V3 patch series that added qcom,smmu-v2 bindings.
>
> [V3]
> * Reworked the patches to keep the clocks init/enabling function
> separately for each compatible.
>
> * Added clocks bindings for MMU40x/500.
>
> * Added a new compatible for qcom,smmu-v2 implementation and
> the clock bindings for the same.
>
> * Rebased on top of 4.11-rc1
>
> [V2]
> * Split the patches little differently.
>
> * Addressed comments.
>
> * Removed the patch #4 [2] from previous post
> for arm-smmu context save restore. Planning to
> post this separately after reworking/addressing Robin's
> feedback.
>
> * Reversed the sequence to disable clocks than enabling.
> This was required for those cases where the
> clocks are populated in a dependent order from DT.
>
> [1] https://lkml.org/lkml/2016/10/20/70
> [2] https://patchwork.kernel.org/patch/9389717/
> [3] https://patchwork.kernel.org/patch/10204925/
> [4] https://patchwork.kernel.org/patch/10102445/
> [5] https://lkml.org/lkml/2018/3/14/127
> [6] https://patchwork.kernel.org/patch/10204945/
> [7] https://patchwork.kernel.org/patch/10204925/
> [8] https://patchwork.kernel.org/patch/10254105/
> [9] https://patchwork.kernel.org/patch/10277975/
> [10] https://patchwork.kernel.org/patch/10281613/
>
> Sricharan R (3):
> iommu/arm-smmu: Add pm_runtime/sleep ops
> iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
> iommu/arm-smmu: Add the device_link between masters and smmu
>
> Vivek Gautam (1):
> iommu/arm-smmu: Add support for qcom,smmu-v2 variant
>
> .../devicetree/bindings/iommu/arm,smmu.txt | 42 +++++
> drivers/iommu/arm-smmu.c | 178 +++++++++++++++++++--
> 2 files changed, 210 insertions(+), 10 deletions(-)
>
^ permalink raw reply
* [PATCH net-next 7/7] net: dsa: qca8k: Remove rudundant parentheses
From: Michal Vokáč @ 2018-05-21 13:28 UTC (permalink / raw)
To: netdev, michal.vokac
Cc: linux-kernel, devicetree, f.fainelli, vivien.didelot, andrew,
mark.rutland, robh+dt, davem
In-Reply-To: <1526909293-56377-1-git-send-email-michal.vokac@ysoft.com>
Fix warning reported by checkpatch.
Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
drivers/net/dsa/qca8k.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index c834893..c0da402 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -513,7 +513,7 @@ qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
pr_debug("qca: port %i set status %i\n", port, enable);
/* Port 0 and 6 have no internal PHY */
- if ((port > 0) && (port < 6))
+ if (port > 0 && port < 6)
mask |= QCA8K_PORT_STATUS_LINK_AUTO;
if (enable)
--
2.1.4
^ permalink raw reply related
* [PATCH net-next 6/7] net: dsa: qca8k: Replace GPL boilerplate by SPDX
From: Michal Vokáč @ 2018-05-21 13:28 UTC (permalink / raw)
To: netdev, michal.vokac
Cc: linux-kernel, devicetree, f.fainelli, vivien.didelot, andrew,
mark.rutland, robh+dt, davem
In-Reply-To: <1526909293-56377-1-git-send-email-michal.vokac@ysoft.com>
Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
drivers/net/dsa/qca8k.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 7eba987..c834893 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1,17 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0
/*
* Copyright (C) 2009 Felix Fietkau <nbd@nbd.name>
* Copyright (C) 2011-2012 Gabor Juhos <juhosg@openwrt.org>
* Copyright (c) 2015, The Linux Foundation. All rights reserved.
* Copyright (c) 2016 John Crispin <john@phrozen.org>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 and
- * only version 2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
*/
#define DEBUG
--
2.1.4
^ permalink raw reply related
* [PATCH net-next 5/7] net: dsa: qca8k: Allow overwriting CPU port setting
From: Michal Vokáč @ 2018-05-21 13:28 UTC (permalink / raw)
To: netdev, michal.vokac
Cc: linux-kernel, devicetree, f.fainelli, vivien.didelot, andrew,
mark.rutland, robh+dt, davem
In-Reply-To: <1526909293-56377-1-git-send-email-michal.vokac@ysoft.com>
Implement adjust_link function that allows to overwrite default CPU port
setting using fixed-link device tree subnode.
Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
drivers/net/dsa/qca8k.c | 43 +++++++++++++++++++++++++++++++++++++++++++
drivers/net/dsa/qca8k.h | 1 +
2 files changed, 44 insertions(+)
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 14a108b38..7eba987 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -636,6 +636,47 @@ qca8k_setup(struct dsa_switch *ds)
return 0;
}
+static void
+qca8k_adjust_link(struct dsa_switch *ds, int port, struct phy_device *phy)
+{
+ struct qca8k_priv *priv = ds->priv;
+ u32 reg;
+
+ /* Force fixed-link setting for CPU port, skip others. */
+ if (!phy_is_pseudo_fixed_link(phy))
+ return;
+
+ /* Set port speed */
+ switch (phy->speed) {
+ case 10:
+ reg = QCA8K_PORT_STATUS_SPEED_10;
+ break;
+ case 100:
+ reg = QCA8K_PORT_STATUS_SPEED_100;
+ break;
+ case 1000:
+ reg = QCA8K_PORT_STATUS_SPEED_1000;
+ break;
+ default:
+ dev_dbg(priv->dev, "port%d link speed %dMbps not supported.\n",
+ port, phy->speed);
+ return;
+ }
+
+ /* Set duplex mode */
+ if (phy->duplex == DUPLEX_FULL)
+ reg |= QCA8K_PORT_STATUS_DUPLEX;
+
+ /* Force flow control */
+ if (dsa_is_cpu_port(ds, port))
+ reg |= QCA8K_PORT_STATUS_RXFLOW | QCA8K_PORT_STATUS_TXFLOW;
+
+ /* Force link down before changing MAC options */
+ qca8k_port_set_status(priv, port, 0);
+ qca8k_write(priv, QCA8K_REG_PORT_STATUS(port), reg);
+ qca8k_port_set_status(priv, port, 1);
+}
+
static int
qca8k_phy_read(struct dsa_switch *ds, int phy, int regnum)
{
@@ -909,6 +950,7 @@ qca8k_get_tag_protocol(struct dsa_switch *ds, int port)
static const struct dsa_switch_ops qca8k_switch_ops = {
.get_tag_protocol = qca8k_get_tag_protocol,
.setup = qca8k_setup,
+ .adjust_link = qca8k_adjust_link,
.get_strings = qca8k_get_strings,
.phy_read = qca8k_phy_read,
.phy_write = qca8k_phy_write,
@@ -942,6 +984,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
return -ENOMEM;
priv->bus = mdiodev->bus;
+ priv->dev = &mdiodev->dev;
/* read the switches ID register */
id = qca8k_read(priv, QCA8K_REG_MASK_CTRL);
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 5bda165..613fe5c5 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -167,6 +167,7 @@ struct qca8k_priv {
struct ar8xxx_port_status port_sts[QCA8K_NUM_PORTS];
struct dsa_switch *ds;
struct mutex reg_mutex;
+ struct device *dev;
};
struct qca8k_mib_desc {
--
2.1.4
^ permalink raw reply related
* [PATCH net-next 4/7] net: dsa: qca8k: Force CPU port to its highest bandwidth
From: Michal Vokáč @ 2018-05-21 13:28 UTC (permalink / raw)
To: netdev, michal.vokac
Cc: linux-kernel, devicetree, f.fainelli, vivien.didelot, andrew,
mark.rutland, robh+dt, davem
In-Reply-To: <1526909293-56377-1-git-send-email-michal.vokac@ysoft.com>
By default autonegotiation is enabled to configure MAC on all ports.
For the CPU port autonegotiation can not be used so we need to set
some sensible defaults manually.
This patch forces the default setting of the CPU port to 1000Mbps/full
duplex which is the chip maximum capability.
Also correct size of the bit field used to configure link speed.
Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
drivers/net/dsa/qca8k.c | 6 +++++-
drivers/net/dsa/qca8k.h | 6 ++++--
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 0d224f3..14a108b38 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -537,6 +537,7 @@ qca8k_setup(struct dsa_switch *ds)
{
struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
int ret, i, phy_mode = -1;
+ u32 mask;
pr_debug("qca: setup\n");
@@ -564,7 +565,10 @@ qca8k_setup(struct dsa_switch *ds)
if (ret < 0)
return ret;
- /* Enable CPU Port */
+ /* Enable CPU Port, force it to maximum bandwidth and full-duplex */
+ mask = QCA8K_PORT_STATUS_SPEED_1000 | QCA8K_PORT_STATUS_TXFLOW |
+ QCA8K_PORT_STATUS_RXFLOW | QCA8K_PORT_STATUS_DUPLEX;
+ qca8k_write(priv, QCA8K_REG_PORT_STATUS(QCA8K_CPU_PORT), mask);
qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
qca8k_port_set_status(priv, QCA8K_CPU_PORT, 1);
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 1cf8a92..5bda165 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -51,8 +51,10 @@
#define QCA8K_GOL_MAC_ADDR0 0x60
#define QCA8K_GOL_MAC_ADDR1 0x64
#define QCA8K_REG_PORT_STATUS(_i) (0x07c + (_i) * 4)
-#define QCA8K_PORT_STATUS_SPEED GENMASK(2, 0)
-#define QCA8K_PORT_STATUS_SPEED_S 0
+#define QCA8K_PORT_STATUS_SPEED GENMASK(1, 0)
+#define QCA8K_PORT_STATUS_SPEED_10 0
+#define QCA8K_PORT_STATUS_SPEED_100 0x1
+#define QCA8K_PORT_STATUS_SPEED_1000 0x2
#define QCA8K_PORT_STATUS_TXMAC BIT(2)
#define QCA8K_PORT_STATUS_RXMAC BIT(3)
#define QCA8K_PORT_STATUS_TXFLOW BIT(4)
--
2.1.4
^ permalink raw reply related
* [PATCH net-next 3/7] net: dsa: qca8k: Enable RXMAC when bringing up a port
From: Michal Vokáč @ 2018-05-21 13:28 UTC (permalink / raw)
To: netdev, michal.vokac
Cc: linux-kernel, devicetree, f.fainelli, vivien.didelot, andrew,
mark.rutland, robh+dt, davem
In-Reply-To: <1526909293-56377-1-git-send-email-michal.vokac@ysoft.com>
When a port is brought up/down do not enable/disable only the TXMAC
but the RXMAC as well. This is essential for the CPU port to work.
Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
drivers/net/dsa/qca8k.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 6a3ffb2..0d224f3 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -516,7 +516,7 @@ qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int mode)
static void
qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
{
- u32 mask = QCA8K_PORT_STATUS_TXMAC;
+ u32 mask = QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC;
pr_debug("qca: port %i set status %i\n", port, enable);
--
2.1.4
^ permalink raw reply related
* [PATCH net-next 2/7] net: dsa: qca8k: Add support for QCA8334 switch
From: Michal Vokáč @ 2018-05-21 13:28 UTC (permalink / raw)
To: netdev, michal.vokac
Cc: linux-kernel, devicetree, f.fainelli, vivien.didelot, andrew,
mark.rutland, robh+dt, davem
In-Reply-To: <1526909293-56377-1-git-send-email-michal.vokac@ysoft.com>
Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
drivers/net/dsa/qca8k.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 3684e56..6a3ffb2 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1010,6 +1010,7 @@ static SIMPLE_DEV_PM_OPS(qca8k_pm_ops,
qca8k_suspend, qca8k_resume);
static const struct of_device_id qca8k_of_match[] = {
+ { .compatible = "qca,qca8334" },
{ .compatible = "qca,qca8337" },
{ /* sentinel */ },
};
--
2.1.4
^ permalink raw reply related
* [PATCH net-next 1/7] net: dsa: qca8k: Add QCA8334 binding documentation
From: Michal Vokáč @ 2018-05-21 13:28 UTC (permalink / raw)
To: netdev, michal.vokac
Cc: linux-kernel, devicetree, f.fainelli, vivien.didelot, andrew,
mark.rutland, robh+dt, davem
In-Reply-To: <1526909293-56377-1-git-send-email-michal.vokac@ysoft.com>
Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
Documentation/devicetree/bindings/net/dsa/qca8k.txt | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
index 9c67ee4..3d73cd0 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
@@ -2,7 +2,10 @@
Required properties:
-- compatible: should be "qca,qca8337"
+- compatible: should be one of:
+ "qca,qca8334"
+ "qca,qca8337"
+
- #size-cells: must be 0
- #address-cells: must be 1
--
2.1.4
^ permalink raw reply related
* [PATCH net-next 0/7] Add support for QCA8334 switch
From: Michal Vokáč @ 2018-05-21 13:28 UTC (permalink / raw)
To: netdev, michal.vokac
Cc: linux-kernel, devicetree, f.fainelli, vivien.didelot, andrew,
mark.rutland, robh+dt, davem
This series basically adds support for a QCA8334 ethernet switch to the
qca8k driver. It is a four-port variant of the already supported seven
port QCA8337. Register map is the same for the whole familly and all chips
have the same device ID.
Major part of this series enhances the CPU port setting. Currently the CPU
port is not set to any sensible defaults compatible with the xGMII
interface. This series forces the CPU port to its maximum bandwidth and
also allows to adjust the new defaults using fixed-link device tree
sub-node.
Alongside these changes I fixed two checkpatch warnings regarding SPDX and
redundant parentheses.
Michal Vokáč (7):
net: dsa: qca8k: Add QCA8334 binding documentation
net: dsa: qca8k: Add support for QCA8334 switch
net: dsa: qca8k: Enable RXMAC when bringing up a port
net: dsa: qca8k: Force CPU port to its highest bandwidth
net: dsa: qca8k: Allow overwriting CPU port setting
net: dsa: qca8k: Replace GPL boilerplate by SPDX
net: dsa: qca8k: Remove rudundant parentheses
.../devicetree/bindings/net/dsa/qca8k.txt | 5 +-
drivers/net/dsa/qca8k.c | 64 ++++++++++++++++++----
drivers/net/dsa/qca8k.h | 7 ++-
3 files changed, 61 insertions(+), 15 deletions(-)
--
2.1.4
^ permalink raw reply
* Re: [PATCH v9 01/15] soc: qcom: Separate kryo l2 accessors from PMU driver
From: Will Deacon @ 2018-05-21 13:17 UTC (permalink / raw)
To: Ilia Lin
Cc: mturquette, sboyd, robh, mark.rutland, viresh.kumar, nm,
lgirdwood, broonie, andy.gross, david.brown, catalin.marinas, rjw,
linux-clk, devicetree, rnayak, linux-pm, linux-arm-msm,
linux-kernel, amit.kucheria, tfinkel, nicolas.dechesne, celster,
linux-soc, linux-arm-kernel
In-Reply-To: <1526901932-9514-2-git-send-email-ilialin@codeaurora.org>
On Mon, May 21, 2018 at 02:25:18PM +0300, Ilia Lin wrote:
> The driver provides kernel level API for other drivers
> to access the MSM8996 L2 cache registers.
> Separating the L2 access code from the PMU driver and
> making it public to allow other drivers use it.
> The accesses must be separated with a single spinlock,
> maintained in this driver.
>
> Signed-off-by: Ilia Lin <ilialin@codeaurora.org>
> ---
> drivers/perf/Kconfig | 1 +
> drivers/perf/qcom_l2_pmu.c | 90 ++++++++++--------------------------
I'm fine with the perf bits:
Acked-by: Will Deacon <will.deacon@arm.com>
Will
^ permalink raw reply
* Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power and data config
From: Heikki Krogerus @ 2018-05-21 13:12 UTC (permalink / raw)
To: Jun Li
Cc: Mats Karrman, robh+dt@kernel.org, gregkh@linuxfoundation.org,
linux@roeck-us.net, a.hajda@samsung.com, cw00.choi@samsung.com,
shufan_lee@richtek.com, Peter Chen, gsomlo@gmail.com,
devicetree@vger.kernel.org, linux-usb@vger.kernel.org,
dl-linux-imx
In-Reply-To: <VI1PR0402MB3917F929BBD8C79966B0D7F089910@VI1PR0402MB3917.eurprd04.prod.outlook.com>
Hi Jun,
Sorry for the delay.
On Thu, May 17, 2018 at 02:41:31PM +0000, Jun Li wrote:
> Hi
> > -----Original Message-----
> > From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com]
> > Sent: 2018??5??17?? 22:24
> > To: Jun Li <jun.li@nxp.com>
> > Cc: Mats Karrman <mats.dev.list@gmail.com>; robh+dt@kernel.org;
> > gregkh@linuxfoundation.org; linux@roeck-us.net; a.hajda@samsung.com;
> > cw00.choi@samsung.com; shufan_lee@richtek.com; Peter Chen
> > <peter.chen@nxp.com>; gsomlo@gmail.com; devicetree@vger.kernel.org;
> > linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH v5 05/14] usb: typec: add API to get typec basic port power
> > and data config
> >
> > On Thu, May 17, 2018 at 02:05:41PM +0000, Jun Li wrote:
> > > Hi Heikki,
> > >
> > > > > I reread this patch and tried to see it more in the context of the
> > > > > other patches and the existing code. The naming of the existing
> > > > > string tables doesn't help in getting this right, however I have a proposal:
> > > > >
> > > > > typec_find_port_power_role() to get to TYPEC_PORT_SRC/SNK/DRP
> > > > > typec_find_port_data_role() to get to TYPEC_PORT_DFP/UFP/DRD
> > > > > typec_find_power_role() to get to TYPEC_SINK/SOURCE
> > > > >
> > > > > and sometime, if the use should arise
> > > > >
> > > > > typec_find_data_role() to get to TYPEC_DEVICE/HOST
> > > > >
> > > > > I think it is fairly comprehensible, *_port_* concerns a
> > > > > capability and without *_port_* it is an actual state. Plus it
> > > > > matches the names of the constants.
> > > >
> > > > Well, there are now four things to consider:
> > > >
> > > > 1) the roles (power and data) the port is capable of supporting
> > > > 2) Try.SRC and Try.SNK, i.e. the preferred role
> > > > 3) the current roles
> > > > 4) the role(s) the user want's to limit the use of a port with DRP
> > > > ports
> > > >
> > > > The last one I don't know if it's relevant with these functions.
> > > > It's not information that we would get for example from firmware.
> > > >
> > > > I also don't think we need to use these functions with the current
> > > > roles the port is in.
> > > >
> > > > If the preferred role is "sink" or "source", so just like the power
> > > > role, we don't need separate function for it here.
> > > >
> > > > So isn't two functions all we need here: one for the power and one
> > > > for data role?
> > >
> > > Take power as an example, can we use one function to only look up
> > > typec_port_types[]? as capability(typec_port_type) and
> > > state(typec_role) are different enum, and the allowed strings are different.
> > >
> > > static const char * const typec_roles[] = {
> > > [TYPEC_SINK] = "sink",
> > > [TYPEC_SOURCE] = "source",
> > > };
> > >
> > > static const char * const typec_port_types[] = {
> > > [TYPEC_PORT_SRC] = "source",
> > > [TYPEC_PORT_SNK] = "sink",
> > > [TYPEC_PORT_DRP] = "dual",
> > > };
> >
> > Where out side the class code we need to convert the current role, the "state",
> > with these functions?
>
> Currently, the only call site is getting the preferred power role from firmware.
My point was that if we used enum typec_port_type with the prefered
role, we could use the helper for power role with prefered role. But
never mind.
Thanks,
--
heikki
^ permalink raw reply
* Re: [PATCH v3 2/3] arm64: dts: renesas: draak: Describe CVBS input
From: Laurent Pinchart @ 2018-05-21 13:10 UTC (permalink / raw)
To: jacopo mondi
Cc: Jacopo Mondi, niklas.soderlund, horms, geert, magnus.damm,
robh+dt, linux-renesas-soc, devicetree, linux-arm-kernel,
linux-kernel
In-Reply-To: <20180521123340.GA15035@w540>
Hi Jacopo,
On Monday, 21 May 2018 15:33:40 EEST jacopo mondi wrote:
> On Mon, May 21, 2018 at 01:54:55PM +0300, Laurent Pinchart wrote:
> > On Monday, 21 May 2018 12:57:05 EEST jacopo mondi wrote:
> >> On Fri, May 18, 2018 at 06:12:15PM +0300, Laurent Pinchart wrote:
> >>> On Friday, 18 May 2018 17:47:57 EEST Jacopo Mondi wrote:
> >>>> Describe CVBS video input through analog video decoder ADV7180
> >>>> connected to video input interface VIN4.
> >>>>
> >>>> The video input signal path is shared with HDMI video input, and
> >>>> selected by on-board switches SW-53 and SW-54 with CVBS input
> >>>> selected by the default switches configuration.
> >>>>
> >>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>>>
> >>>> ---
> >>>> v2 -> v3:
> >>>> - Add comment to describe the shared input video path
> >>>> - Add my SoB and Niklas' R-b tags
> >>>> ---
> >>>>
> >>>> arch/arm64/boot/dts/renesas/r8a77995-draak.dts | 42 ++++++++++++++++++
> >>>> 1 file changed, 42 insertions(+)
> >>>>
> >>>> diff --git a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> >>>> b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts index
> >>>> 9d73de8..95745fc
> >>>> 100644
> >>>> --- a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> >>>> +++ b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> >>>> @@ -142,6 +142,11 @@
> >>>> groups = "usb0";
> >>>> function = "usb0";
> >>>> };
> >>>> +
> >>>> + vin4_pins_cvbs: vin4 {
> >>>> + groups = "vin4_data8", "vin4_sync", "vin4_clk";
> >>>> + function = "vin4";
> >>>> + };
> >>>> };
> >>>>
> >>>> &i2c0 {
> >>>> @@ -154,6 +159,23 @@
> >>>> reg = <0x50>;
> >>>> pagesize = <8>;
> >>>> };
> >>>> +
> >>>> + analog-video@20 {
> >>>> + compatible = "adi,adv7180";
> >>>> + reg = <0x20>;
> >>>> +
> >>>> + port {
> >>>
> >>> The adv7180 DT bindings document the output port as 3 or 6
> >>> (respectively for the CP and ST versions of the chip). You should thus
> >>> number the port. Apart from that the patch looks good.
> >>
> >> I admit I have barely copied this from Gen-2 boards DTS, but reading
> >> the driver code and binding description again, I think this is
> >> correct, as the output port numbering and mandatory input port (which
> >> is missing here) only apply to adv7180cp/st chip versions.
> >>
> >> Here we describe plain adv7180, no need to number output ports afaict.
> >
> > Indeed, my bad.
> >
> > Shouldn't you however use "adi,adv7180cp" as that's the chip we are using
> > ?
> > The "adi,adv7180" has no port documented in its DT bindings, so it
> > shouldn't have any port node at all.
>
> I'm a bit confused here.
>
> The only Gen-2 board using the "adi,adv7180cp" compatible string is
> Gose, which is also the only one I can get schematics for. That board
> indeed feature an ADV7180WBCP32Z, as the Draak does. I cannot get
> schematics for any other Gen-2 board, to compare the ADV7180 variant
> installed there. If anyone can confirm that all other Gen-2 board uses
> a different version (or that all other Gen-2 board DTS file use a
> wrong compatible string value), I'll re-send this with a different
> compatible value and proper port nodes numbering.
Other Gen2 boards use a ADV7180WBCP32Z as well. The issue here isn't that the
chip you're trying to support is different. The DT bindings that were
initially written for the adi,adv7180 didn't have port nodes. When it was time
to add them, we realized that two variants of the chip existed with different
connectivity. We have thus added two new compatible strings to differentiate
them, with different port numbers. The old compatible string should be
deprecated in favour of the new ones.
> >>>> + /*
> >>>> + * The VIN4 video input path is shared between
> >>>> + * CVBS and HDMI inputs through SW[49-54] switches.
> >>>> + *
> >>>> + * CVBS is the default selection, link it to VIN4 here.
> >>>> + */
> >>>> + adv7180_out: endpoint {
> >>>> + remote-endpoint = <&vin4_in>;
> >>>> + };
> >>>> + };
> >>>> + };
> >>>> };
> >>>>
> >>>> &i2c1 {
> >>>> @@ -246,3 +268,23 @@
> >>>> timeout-sec = <60>;
> >>>> status = "okay";
> >>>> };
>>>> +
> >>>> +&vin4 {
> >>>> + pinctrl-0 = <&vin4_pins_cvbs>;
> >>>> + pinctrl-names = "default";
> >>>> +
> >>>> + status = "okay";
> >>>> +
> >>>> + ports {
> >>>> + #address-cells = <1>;
> >>>> + #size-cells = <0>;
> >>>> +
> >>>> + port@0 {
> >>>> + reg = <0>;
> >>>> +
> >>>> + vin4_in: endpoint {
> >>>> + remote-endpoint = <&adv7180_out>;
> >>>> + };
> >>>> + };
> >>>> + };
> >>>> +};
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v6 1/9] counter: Introduce the Generic Counter interface
From: William Breathitt Gray @ 2018-05-21 13:06 UTC (permalink / raw)
To: Jonathan Cameron
Cc: benjamin.gaignard, fabrice.gasnier, linux-iio, linux-kernel,
devicetree, linux-arm-kernel
In-Reply-To: <20180520160652.1896f5a1@archlinux>
On Sun, May 20, 2018 at 04:06:52PM +0100, Jonathan Cameron wrote:
>On Wed, 16 May 2018 13:50:43 -0400
>William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
>
>> This patch introduces the Generic Counter interface for supporting
>> counter devices.
>>
>> In the context of the Generic Counter interface, a counter is defined as
>> a device that reports one or more "counts" based on the state changes of
>> one or more "signals" as evaluated by a defined "count function."
>>
>> Driver callbacks should be provided to communicate with the device: to
>> read and write various Signals and Counts, and to set and get the
>> "action mode" and "count function" for various Synapses and Counts
>> respectively.
>>
>> To support a counter device, a driver must first allocate the available
>> Counter Signals via counter_signal structures. These Signals should
>> be stored as an array and set to the signals array member of an
>> allocated counter_device structure before the Counter is registered to
>> the system.
>>
>> Counter Counts may be allocated via counter_count structures, and
>> respective Counter Signal associations (Synapses) made via
>> counter_synapse structures. Associated counter_synapse structures are
>> stored as an array and set to the the synapses array member of the
>> respective counter_count structure. These counter_count structures are
>> set to the counts array member of an allocated counter_device structure
>> before the Counter is registered to the system.
>>
>> A counter device is registered to the system by passing the respective
>> initialized counter_device structure to the counter_register function;
>> similarly, the counter_unregister function unregisters the respective
>> Counter. The devm_counter_register and devm_counter_unregister functions
>> serve as device memory-managed versions of the counter_register and
>> counter_unregister functions respectively.
>>
>> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
>
>A few minor comments inline. I do somewhat wonder if we can cut
>back on the huge amount of 'similar' code in here, but there tend to
>be just enough small differences to make that really tricky...
>
>Nothing major enough in here that I really plan on reading it again
>(though you never know if you change lots ;)
>
>Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
These look like quick tidy-up suggestions so I expect to incorporate
them in the next revision without trouble. I'll also look around to
organize the repetitive code into functions for reuse where possible, in
order to help clean up the code and make the logic clearer. In short
though, I don't expect any major changes in the next revision, just
minor formatting and cleanup.
William Breathitt Gray
>> ---
>> MAINTAINERS | 7 +
>> drivers/Kconfig | 2 +
>> drivers/Makefile | 1 +
>> drivers/counter/Kconfig | 18 +
>> drivers/counter/Makefile | 8 +
>> drivers/counter/generic-counter.c | 1541 +++++++++++++++++++++++++++++
>> include/linux/counter.h | 554 +++++++++++
>> 7 files changed, 2131 insertions(+)
>> create mode 100644 drivers/counter/Kconfig
>> create mode 100644 drivers/counter/Makefile
>> create mode 100644 drivers/counter/generic-counter.c
>> create mode 100644 include/linux/counter.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 4b65225d443a..2a016d73ab72 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -3669,6 +3669,13 @@ W: http://www.fi.muni.cz/~kas/cosa/
>> S: Maintained
>> F: drivers/net/wan/cosa*
>>
>> +COUNTER SUBSYSTEM
>> +M: William Breathitt Gray <vilhelm.gray@gmail.com>
>> +L: linux-iio@vger.kernel.org
>> +S: Maintained
>> +F: drivers/counter/
>> +F: include/linux/counter.h
>> +
>> CPMAC ETHERNET DRIVER
>> M: Florian Fainelli <f.fainelli@gmail.com>
>> L: netdev@vger.kernel.org
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index 95b9ccc08165..70b3cc88dc0b 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -165,6 +165,8 @@ source "drivers/memory/Kconfig"
>>
>> source "drivers/iio/Kconfig"
>>
>> +source "drivers/counter/Kconfig"
>> +
>Same comment as below.
>
>> source "drivers/ntb/Kconfig"
>>
>> source "drivers/vme/Kconfig"
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index 24cd47014657..5914c78688c3 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -165,6 +165,7 @@ obj-$(CONFIG_PM_DEVFREQ) += devfreq/
>> obj-$(CONFIG_EXTCON) += extcon/
>> obj-$(CONFIG_MEMORY) += memory/
>> obj-$(CONFIG_IIO) += iio/
>> +obj-$(CONFIG_COUNTER) += counter/
>
>I can see your logic in putting this here, but I think the convention
>is to go at the end rather than grouping.
>
>> obj-$(CONFIG_VME_BUS) += vme/
>> obj-$(CONFIG_IPACK_BUS) += ipack/
>> obj-$(CONFIG_NTB) += ntb/
>> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
>> new file mode 100644
>> index 000000000000..65fa92abd5a4
>> --- /dev/null
>> +++ b/drivers/counter/Kconfig
>> @@ -0,0 +1,18 @@
>> +#
>> +# Counter devices
>> +#
>> +# When adding new entries keep the list in alphabetical order
>> +
>> +menuconfig COUNTER
>> + tristate "Counter support"
>> + help
>> + Provides Generic Counter interface support for counter devices.
>> +
>> + Counter devices are prevalent within a diverse spectrum of industries.
>> + The ubiquitous presence of these devices necessitates a common
>> + interface and standard of interaction and exposure. This driver API
>> + attempts to resolve the issue of duplicate code found among existing
>> + counter device drivers by providing a generic counter interface for
>> + consumption. The Generic Counter interface enables drivers to support
>> + and expose a common set of components and functionality present in
>> + counter devices.
>> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
>> new file mode 100644
>> index 000000000000..ad1ba7109cdc
>> --- /dev/null
>> +++ b/drivers/counter/Makefile
>> @@ -0,0 +1,8 @@
>> +#
>> +# Makefile for Counter devices
>> +#
>> +
>> +# When adding new entries keep the list in alphabetical order
>> +
>> +obj-$(CONFIG_COUNTER) += counter.o
>> +counter-y := generic-counter.o
>> diff --git a/drivers/counter/generic-counter.c b/drivers/counter/generic-counter.c
>> new file mode 100644
>> index 000000000000..0d83b862453f
>> --- /dev/null
>> +++ b/drivers/counter/generic-counter.c
>> @@ -0,0 +1,1541 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Generic Counter interface
>> + * Copyright (C) 2017 William Breathitt Gray
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License, version 2, as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * General Public License for more details.
>
>As below, SPDX and license seems silly. Unless you are feeling paranoid
>just drop the license text.
>
>> + */
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/export.h>
>> +#include <linux/fs.h>
>> +#include <linux/gfp.h>
>> +#include <linux/idr.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/list.h>
>> +#include <linux/module.h>
>> +#include <linux/printk.h>
>> +#include <linux/slab.h>
>> +#include <linux/string.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/types.h>
>> +
>> +#include <linux/counter.h>
>> +
>> +const char *const count_direction_str[2] = {
>> + [COUNT_DIRECTION_FORWARD] = "forward",
>> + [COUNT_DIRECTION_BACKWARD] = "backward"
>> +};
>> +EXPORT_SYMBOL(count_direction_str);
>> +
>> +const char *const count_mode_str[4] = {
>> + [COUNT_MODE_NORMAL] = "normal",
>> + [COUNT_MODE_RANGE_LIMIT] = "range limit",
>> + [COUNT_MODE_NON_RECYCLE] = "non-recycle",
>> + [COUNT_MODE_MODULO_N] = "modulo-n"
>> +};
>> +EXPORT_SYMBOL(count_mode_str);
>> +
>> +ssize_t counter_signal_enum_read(struct counter_device *counter,
>> + struct counter_signal *signal, void *priv,
>> + char *buf)
>> +{
>> + const struct counter_signal_enum_ext *const e = priv;
>> + int err;
>> + size_t index;
>> +
>> + if (!e->get)
>> + return -EINVAL;
>> +
>> + err = e->get(counter, signal, &index);
>> + if (err)
>> + return err;
>> +
>> + if (index >= e->num_items)
>> + return -EINVAL;
>> +
>> + return scnprintf(buf, PAGE_SIZE, "%s\n", e->items[index]);
>> +}
>> +EXPORT_SYMBOL(counter_signal_enum_read);
>> +
>> +ssize_t counter_signal_enum_write(struct counter_device *counter,
>> + struct counter_signal *signal, void *priv,
>> + const char *buf, size_t len)
>> +{
>> + const struct counter_signal_enum_ext *const e = priv;
>> + ssize_t index;
>> + int err;
>> +
>> + if (!e->set)
>> + return -EINVAL;
>> +
>> + index = __sysfs_match_string(e->items, e->num_items, buf);
>> + if (index < 0)
>> + return index;
>> +
>> + err = e->set(counter, signal, index);
>> + if (err)
>> + return err;
>> +
>> + return len;
>> +}
>> +EXPORT_SYMBOL(counter_signal_enum_write);
>> +
>> +ssize_t counter_signal_enum_available_read(struct counter_device *counter,
>> + struct counter_signal *signal,
>> + void *priv, char *buf)
>> +{
>> + const struct counter_signal_enum_ext *const e = priv;
>> + size_t i;
>> + size_t len = 0;
>> +
>> + if (!e->num_items)
>> + return 0;
>> +
>> + for (i = 0; i < e->num_items; i++)
>> + len += scnprintf(buf + len, PAGE_SIZE - len, "%s\n",
>> + e->items[i]);
>> +
>> + return len;
>> +}
>> +EXPORT_SYMBOL(counter_signal_enum_available_read);
>> +
>> +ssize_t counter_count_enum_read(struct counter_device *counter,
>> + struct counter_count *count, void *priv,
>> + char *buf)
>> +{
>> + const struct counter_count_enum_ext *const e = priv;
>> + int err;
>> + size_t index;
>> +
>> + if (!e->get)
>> + return -EINVAL;
>> +
>> + err = e->get(counter, count, &index);
>> + if (err)
>> + return err;
>> +
>> + if (index >= e->num_items)
>> + return -EINVAL;
>> +
>> + return scnprintf(buf, PAGE_SIZE, "%s\n", e->items[index]);
>> +}
>> +EXPORT_SYMBOL(counter_count_enum_read);
>> +
>> +ssize_t counter_count_enum_write(struct counter_device *counter,
>> + struct counter_count *count, void *priv,
>> + const char *buf, size_t len)
>> +{
>> + const struct counter_count_enum_ext *const e = priv;
>> + ssize_t index;
>> + int err;
>> +
>> + if (!e->set)
>> + return -EINVAL;
>> +
>> + index = __sysfs_match_string(e->items, e->num_items, buf);
>> + if (index < 0)
>> + return index;
>> +
>> + err = e->set(counter, count, index);
>> + if (err)
>> + return err;
>> +
>> + return len;
>> +}
>> +EXPORT_SYMBOL(counter_count_enum_write);
>> +
>> +ssize_t counter_count_enum_available_read(struct counter_device *counter,
>> + struct counter_count *count,
>> + void *priv, char *buf)
>> +{
>> + const struct counter_count_enum_ext *const e = priv;
>> + size_t i;
>> + size_t len = 0;
>> +
>> + if (!e->num_items)
>> + return 0;
>> +
>> + for (i = 0; i < e->num_items; i++)
>> + len += scnprintf(buf + len, PAGE_SIZE - len, "%s\n",
>> + e->items[i]);
>> +
>> + return len;
>> +}
>> +EXPORT_SYMBOL(counter_count_enum_available_read);
>> +
>> +ssize_t counter_device_enum_read(struct counter_device *counter, void *priv,
>> + char *buf)
>> +{
>> + const struct counter_device_enum_ext *const e = priv;
>> + int err;
>> + size_t index;
>> +
>> + if (!e->get)
>> + return -EINVAL;
>> +
>> + err = e->get(counter, &index);
>> + if (err)
>> + return err;
>> +
>> + if (index >= e->num_items)
>> + return -EINVAL;
>> +
>> + return scnprintf(buf, PAGE_SIZE, "%s\n", e->items[index]);
>> +}
>> +EXPORT_SYMBOL(counter_device_enum_read);
>> +
>> +ssize_t counter_device_enum_write(struct counter_device *counter, void *priv,
>> + const char *buf, size_t len)
>> +{
>> + const struct counter_device_enum_ext *const e = priv;
>> + ssize_t index;
>> + int err;
>> +
>> + if (!e->set)
>> + return -EINVAL;
>> +
>> + index = __sysfs_match_string(e->items, e->num_items, buf);
>> + if (index < 0)
>> + return index;
>> +
>> + err = e->set(counter, index);
>> + if (err)
>> + return err;
>> +
>> + return len;
>> +}
>> +EXPORT_SYMBOL(counter_device_enum_write);
>> +
>> +ssize_t counter_device_enum_available_read(struct counter_device *counter,
>> + void *priv, char *buf)
>> +{
>> + const struct counter_device_enum_ext *const e = priv;
>> + size_t i;
>> + size_t len = 0;
>> +
>> + if (!e->num_items)
>> + return 0;
>> +
>> + for (i = 0; i < e->num_items; i++)
>> + len += scnprintf(buf + len, PAGE_SIZE - len, "%s\n",
>> + e->items[i]);
>> +
>> + return len;
>> +}
>> +EXPORT_SYMBOL(counter_device_enum_available_read);
>> +
>> +static const char *const signal_level_str[] = {
>> + [SIGNAL_LEVEL_LOW] = "low",
>> + [SIGNAL_LEVEL_HIGH] = "high"
>> +};
>> +
>> +/**
>> + * set_signal_read_value - set signal_read_value data
>> + * @val: signal_read_value structure to set
>> + * @type: property Signal data represents
>> + * @data: Signal data
>> + *
>> + * This function sets an opaque signal_read_value structure with the provided
>> + * Signal data.
>> + */
>> +void set_signal_read_value(struct signal_read_value *const val,
>> + const enum signal_value_type type, void *const data)
>> +{
>> + if (type == SIGNAL_LEVEL)
>> + val->len = scnprintf(val->buf, PAGE_SIZE, "%s\n",
>> + signal_level_str[*(enum signal_level *)data]);
>> + else
>> + val->len = 0;
>> +}
>> +EXPORT_SYMBOL(set_signal_read_value);
>> +
>> +/**
>> + * set_count_read_value - set count_read_value data
>> + * @val: count_read_value structure to set
>> + * @type: property Count data represents
>> + * @data: Count data
>> + *
>> + * This function sets an opaque count_read_value structure with the provided
>> + * Count data.
>> + */
>> +void set_count_read_value(struct count_read_value *const val,
>> + const enum count_value_type type, void *const data)
>> +{
>> + switch (type) {
>> + case COUNT_POSITION_UNSIGNED:
>> + val->len = scnprintf(val->buf, PAGE_SIZE, "%lu\n",
>> + *(unsigned long *)data);
>> + break;
>> + case COUNT_POSITION_SIGNED:
>> + val->len = scnprintf(val->buf, PAGE_SIZE, "%ld\n",
>> + *(long *)data);
>> + break;
>> + default:
>> + val->len = 0;
>> + }
>> +}
>> +EXPORT_SYMBOL(set_count_read_value);
>> +
>> +/**
>> + * get_count_write_value - get count_write_value data
>> + * @data: Count data
>> + * @type: property Count data represents
>> + * @val: count_write_value structure containing data
>> + *
>> + * This function extracts Count data from the provided opaque count_write_value
>> + * structure and stores it at the address provided by @data.
>> + *
>> + * RETURNS:
>> + * 0 on success, negative error number on failure.
>> + */
>> +int get_count_write_value(void *const data, const enum count_value_type type,
>> + const struct count_write_value *const val)
>> +{
>> + int err;
>> +
>> + switch (type) {
>> + case COUNT_POSITION_UNSIGNED:
>> + err = kstrtoul(val->buf, 0, data);
>> + if (err)
>> + return err;
>> + break;
>> + case COUNT_POSITION_SIGNED:
>> + err = kstrtol(val->buf, 0, data);
>> + if (err)
>> + return err;
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(get_count_write_value);
>> +
>> +struct counter_device_attr {
>> + struct device_attribute dev_attr;
>> + struct list_head l;
>> + void *component;
>> +};
>> +
>> +static int counter_attribute_create(
>> + struct counter_device_attr_group *const group,
>> + const char *const prefix,
>> + const char *const name,
>> + ssize_t (*show)(struct device *dev, struct device_attribute *attr,
>> + char *buf),
>> + ssize_t (*store)(struct device *dev, struct device_attribute *attr,
>> + const char *buf, size_t len),
>> + void *const component)
>> +{
>> + struct counter_device_attr *counter_attr;
>> + struct device_attribute *dev_attr;
>> + int err;
>> + struct list_head *const attr_list = &group->attr_list;
>> +
>> + /* Allocate a Counter device attribute */
>> + counter_attr = kzalloc(sizeof(*counter_attr), GFP_KERNEL);
>> + if (!counter_attr)
>> + return -ENOMEM;
>> + dev_attr = &counter_attr->dev_attr;
>> +
>> + sysfs_attr_init(&dev_attr->attr);
>> +
>> + /* Configure device attribute */
>> + dev_attr->attr.name = kasprintf(GFP_KERNEL, "%s%s", prefix, name);
>> + if (!dev_attr->attr.name) {
>> + err = -ENOMEM;
>> + goto err_free_counter_attr;
>> + }
>> + if (show) {
>> + dev_attr->attr.mode |= 0444;
>> + dev_attr->show = show;
>> + }
>> + if (store) {
>> + dev_attr->attr.mode |= 0200;
>> + dev_attr->store = store;
>> + }
>> +
>> + /* Store associated Counter component with attribute */
>> + counter_attr->component = component;
>> +
>> + /* Keep track of the attribute for later cleanup */
>> + list_add(&counter_attr->l, attr_list);
>> + group->num_attr++;
>> +
>> + return 0;
>> +
>> +err_free_counter_attr:
>> + kfree(counter_attr);
>> + return err;
>> +}
>> +
>> +#define to_counter_attr(_dev_attr) \
>> + container_of(_dev_attr, struct counter_device_attr, dev_attr)
>> +
>> +struct signal_comp_t {
>> + struct counter_signal *signal;
>> +};
>> +
>> +static ssize_t counter_signal_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct counter_device *const counter = dev_get_drvdata(dev);
>> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
>> + const struct signal_comp_t *const component = devattr->component;
>> + struct counter_signal *const signal = component->signal;
>> + int err;
>> + struct signal_read_value val = { .buf = buf };
>> +
>> + err = counter->ops->signal_read(counter, signal, &val);
>> + if (err)
>> + return err;
>> +
>> + return val.len;
>> +}
>> +
>> +struct name_comp_t {
>> + const char *name;
>> +};
>> +
>> +static ssize_t counter_device_attr_name_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + const struct name_comp_t *const comp = to_counter_attr(attr)->component;
>> +
>> + return scnprintf(buf, PAGE_SIZE, "%s\n", comp->name);
>> +}
>> +
>> +static int counter_name_attribute_create(
>> + struct counter_device_attr_group *const group,
>> + const char *const name)
>> +{
>> + struct name_comp_t *name_comp;
>> + int err;
>> +
>> + /* Skip if no name */
>> + if (!name)
>> + return 0;
>> +
>> + /* Allocate name attribute component */
>> + name_comp = kmalloc(sizeof(*name_comp), GFP_KERNEL);
>> + if (!name_comp)
>> + return -ENOMEM;
>> + name_comp->name = name;
>> +
>> + /* Allocate Signal name attribute */
>> + err = counter_attribute_create(group, "", "name",
>> + counter_device_attr_name_show, NULL,
>> + name_comp);
>> + if (err)
>> + goto err_free_name_comp;
>> +
>> + return 0;
>> +
>> +err_free_name_comp:
>> + kfree(name_comp);
>> + return err;
>> +}
>> +
>> +struct signal_ext_comp_t {
>> + struct counter_signal *signal;
>> + const struct counter_signal_ext *ext;
>> +};
>> +
>> +static ssize_t counter_signal_ext_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
>> + const struct signal_ext_comp_t *const comp = devattr->component;
>> + const struct counter_signal_ext *const ext = comp->ext;
>> +
>> + return ext->read(dev_get_drvdata(dev), comp->signal, ext->priv, buf);
>> +}
>> +
>> +static ssize_t counter_signal_ext_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t len)
>> +{
>> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
>> + const struct signal_ext_comp_t *const comp = devattr->component;
>> + const struct counter_signal_ext *const ext = comp->ext;
>> +
>> + return ext->write(dev_get_drvdata(dev), comp->signal, ext->priv, buf,
>> + len);
>> +}
>> +
>> +static void free_counter_device_attr_list(struct list_head *attr_list)
>> +{
>> + struct counter_device_attr *p, *n;
>> +
>> + list_for_each_entry_safe(p, n, attr_list, l) {
>> + kfree(p->dev_attr.attr.name);
>> + kfree(p->component);
>> + list_del(&p->l);
>> + kfree(p);
>> + }
>> +}
>> +
>> +static int counter_signal_ext_register(
>> + struct counter_device_attr_group *const group,
>> + struct counter_signal *const signal)
>> +{
>> + const size_t num_ext = signal->num_ext;
>> + size_t i;
>> + const struct counter_signal_ext *ext;
>> + struct signal_ext_comp_t *signal_ext_comp;
>> + int err;
>> +
>> + /* Create an attribute for each extension */
>> + for (i = 0 ; i < num_ext; i++) {
>> + ext = signal->ext + i;
>> +
>> + /* Allocate signal_ext attribute component */
>> + signal_ext_comp = kmalloc(sizeof(*signal_ext_comp), GFP_KERNEL);
>> + if (!signal_ext_comp) {
>> + err = -ENOMEM;
>> + goto err_free_attr_list;
>> + }
>> + signal_ext_comp->signal = signal;
>> + signal_ext_comp->ext = ext;
>> +
>> + /* Allocate a Counter device attribute */
>> + err = counter_attribute_create(group, "", ext->name,
>> + (ext->read) ? counter_signal_ext_show : NULL,
>> + (ext->write) ? counter_signal_ext_store : NULL,
>> + signal_ext_comp);
>> + if (err) {
>> + kfree(signal_ext_comp);
>> + goto err_free_attr_list;
>> + }
>> + }
>> +
>> + return 0;
>> +
>> +err_free_attr_list:
>> + free_counter_device_attr_list(&group->attr_list);
>> + return err;
>> +}
>> +
>> +static int counter_signal_attributes_create(
>> + struct counter_device_attr_group *const group,
>> + const struct counter_device *const counter,
>> + struct counter_signal *const signal)
>> +{
>> + struct signal_comp_t *signal_comp;
>> + int err;
>> +
>> + /* Allocate Signal attribute component */
>> + signal_comp = kmalloc(sizeof(*signal_comp), GFP_KERNEL);
>> + if (!signal_comp)
>> + return -ENOMEM;
>> + signal_comp->signal = signal;
>> +
>> + /* Create main Signal attribute */
>> + err = counter_attribute_create(group, "", "signal",
>> + (counter->ops->signal_read) ? counter_signal_show : NULL, NULL,
>> + signal_comp);
>> + if (err) {
>> + kfree(signal_comp);
>> + return err;
>> + }
>> +
>> + /* Create Signal name attribute */
>> + err = counter_name_attribute_create(group, signal->name);
>> + if (err)
>> + goto err_free_attr_list;
>> +
>> + /* Register Signal extension attributes */
>> + err = counter_signal_ext_register(group, signal);
>> + if (err)
>> + goto err_free_attr_list;
>> +
>> + return 0;
>> +
>> +err_free_attr_list:
>> + free_counter_device_attr_list(&group->attr_list);
>> + return err;
>> +}
>> +
>> +static int counter_signals_register(
>> + struct counter_device_attr_group *const groups_list,
>> + const struct counter_device *const counter)
>> +{
>> + const size_t num_signals = counter->num_signals;
>> + size_t i;
>> + struct counter_signal *signal;
>> + const char *name;
>> + int err;
>> +
>> + /* Register each Signal */
>> + for (i = 0; i < num_signals; i++) {
>> + signal = counter->signals + i;
>> +
>> + /* Generate Signal attribute directory name */
>> + name = kasprintf(GFP_KERNEL, "signal%d", signal->id);
>> + if (!name) {
>> + err = -ENOMEM;
>> + goto err_free_attr_groups;
>> + }
>> + groups_list[i].attr_group.name = name;
>> +
>> + /* Create all attributes associated with Signal */
>> + err = counter_signal_attributes_create(groups_list + i, counter,
>> + signal);
>> + if (err)
>> + goto err_free_attr_groups;
>> + }
>> +
>> + return 0;
>> +
>> +err_free_attr_groups:
>> + do {
>> + kfree(groups_list[i].attr_group.name);
>> + free_counter_device_attr_list(&groups_list[i].attr_list);
>> + } while (i--);
>> + return err;
>> +}
>> +
>> +static const char *const synapse_action_str[] = {
>> + [SYNAPSE_ACTION_NONE] = "none",
>> + [SYNAPSE_ACTION_RISING_EDGE] = "rising edge",
>> + [SYNAPSE_ACTION_FALLING_EDGE] = "falling edge",
>> + [SYNAPSE_ACTION_BOTH_EDGES] = "both edges"
>> +};
>> +
>> +struct action_comp_t {
>> + struct counter_synapse *synapse;
>> + struct counter_count *count;
>> +};
>> +
>> +static ssize_t counter_action_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
>> + int err;
>> + struct counter_device *const counter = dev_get_drvdata(dev);
>> + const struct action_comp_t *const component = devattr->component;
>> + struct counter_count *const count = component->count;
>> + struct counter_synapse *const synapse = component->synapse;
>> + size_t action_index;
>> + enum synapse_action action;
>> +
>> + err = counter->ops->action_get(counter, count, synapse, &action_index);
>> + if (err)
>> + return err;
>> +
>> + synapse->action = action_index;
>> +
>> + action = synapse->actions_list[action_index];
>> + return scnprintf(buf, PAGE_SIZE, "%s\n", synapse_action_str[action]);
>> +}
>> +
>> +static ssize_t counter_action_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t len)
>> +{
>> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
>> + const struct action_comp_t *const component = devattr->component;
>> + struct counter_synapse *const synapse = component->synapse;
>> + size_t action_index;
>> + const size_t num_actions = synapse->num_actions;
>> + enum synapse_action action;
>> + int err;
>> + struct counter_device *const counter = dev_get_drvdata(dev);
>> + struct counter_count *const count = component->count;
>> +
>> + /* Find requested action mode */
>> + for (action_index = 0; action_index < num_actions; action_index++) {
>> + action = synapse->actions_list[action_index];
>> + if (sysfs_streq(buf, synapse_action_str[action]))
>> + break;
>> + }
>> + /* If requested action mode not found */
>> + if (action_index >= num_actions)
>> + return -EINVAL;
>> +
>> + err = counter->ops->action_set(counter, count, synapse, action_index);
>> + if (err)
>> + return err;
>> +
>> + synapse->action = action_index;
>> +
>> + return len;
>> +}
>> +
>> +struct action_avail_comp_t {
>> + const enum synapse_action *actions_list;
>> + size_t num_actions;
>> +};
>> +
>> +static ssize_t counter_synapse_action_available_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
>> + const struct action_avail_comp_t *const component = devattr->component;
>> + const enum synapse_action *const actions_list = component->actions_list;
>
>I'm not sure this local variable helps much either...
>
>> + const size_t num_actions = component->num_actions;
>
>Local variable adds nothing as far as I can see.. Just use it inline.
>
>
>> + size_t i;
>> + enum synapse_action action;
>> + ssize_t len = 0;
>> +
>> + for (i = 0; i < num_actions; i++) {
>> + action = actions_list[i];
>> + len += scnprintf(buf + len, PAGE_SIZE - len, "%s\n",
>> + synapse_action_str[action]);
>> + }
>> +
>> + return len;
>> +}
>> +
>> +static int counter_synapses_register(
>> + struct counter_device_attr_group *const group,
>> + const struct counter_device *const counter,
>> + struct counter_count *const count, const char *const count_attr_name)
>> +{
>> + const size_t num_synapses = count->num_synapses;
>
>Local variable doesn't add anything. Only used once.
>
>> + size_t i;
>> + struct counter_synapse *synapse;
>> + const char *prefix;
>> + struct action_comp_t *action_comp;
>> + int err;
>> + struct action_avail_comp_t *avail_comp;
>> +
>> + /* Register each Synapse */
>> + for (i = 0; i < num_synapses; i++) {
>> + synapse = count->synapses + i;
>> +
>> + /* Generate attribute prefix */
>> + prefix = kasprintf(GFP_KERNEL, "signal%d_",
>> + synapse->signal->id);
>> + if (!prefix) {
>> + err = -ENOMEM;
>> + goto err_free_attr_list;
>> + }
>> +
>> + /* Allocate action attribute component */
>> + action_comp = kmalloc(sizeof(*action_comp), GFP_KERNEL);
>> + if (!action_comp) {
>> + err = -ENOMEM;
>> + goto err_free_prefix;
>> + }
>> + action_comp->synapse = synapse;
>> + action_comp->count = count;
>> +
>> + /* Create action attribute */
>> + err = counter_attribute_create(group, prefix, "action",
>> + (counter->ops->action_get) ? counter_action_show : NULL,
>> + (counter->ops->action_set) ? counter_action_store : NULL,
>> + action_comp);
>> + if (err) {
>> + kfree(action_comp);
>> + goto err_free_prefix;
>> + }
>> +
>> + /* Allocate action available attribute component */
>> + avail_comp = kmalloc(sizeof(*avail_comp), GFP_KERNEL);
>> + if (!avail_comp) {
>> + err = -ENOMEM;
>> + goto err_free_prefix;
>> + }
>> + avail_comp->actions_list = synapse->actions_list;
>> + avail_comp->num_actions = synapse->num_actions;
>> +
>> + /* Create action_available attribute */
>> + err = counter_attribute_create(group, prefix,
>> + "action_available",
>> + counter_synapse_action_available_show, NULL,
>> + avail_comp);
>> + if (err) {
>> + kfree(avail_comp);
>> + goto err_free_prefix;
>> + }
>> +
>> + kfree(prefix);
>> + }
>> +
>> + return 0;
>> +
>> +err_free_prefix:
>> + kfree(prefix);
>> +err_free_attr_list:
>> + free_counter_device_attr_list(&group->attr_list);
>> + return err;
>> +}
>> +
>> +struct count_comp_t {
>> + struct counter_count *count;
>> +};
>> +
>> +static ssize_t counter_count_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct counter_device *const counter = dev_get_drvdata(dev);
>> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
>> + const struct count_comp_t *const component = devattr->component;
>> + struct counter_count *const count = component->count;
>> + int err;
>> + struct count_read_value val = { .buf = buf };
>> +
>> + err = counter->ops->count_read(counter, count, &val);
>> + if (err)
>> + return err;
>> +
>> + return val.len;
>> +}
>> +
>> +static ssize_t counter_count_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t len)
>> +{
>> + struct counter_device *const counter = dev_get_drvdata(dev);
>> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
>> + const struct count_comp_t *const component = devattr->component;
>> + struct counter_count *const count = component->count;
>> + int err;
>> + struct count_write_value val = { .buf = buf };
>> +
>> + err = counter->ops->count_write(counter, count, &val);
>> + if (err)
>> + return err;
>> +
>> + return len;
>> +}
>> +
>> +static const char *const count_function_str[] = {
>> + [COUNT_FUNCTION_INCREASE] = "increase",
>> + [COUNT_FUNCTION_DECREASE] = "decrease",
>> + [COUNT_FUNCTION_PULSE_DIRECTION] = "pulse-direction",
>> + [COUNT_FUNCTION_QUADRATURE_X1_A] = "quadrature x1 a",
>> + [COUNT_FUNCTION_QUADRATURE_X1_B] = "quadrature x1 b",
>> + [COUNT_FUNCTION_QUADRATURE_X2_A] = "quadrature x2 a",
>> + [COUNT_FUNCTION_QUADRATURE_X2_B] = "quadrature x2 b",
>> + [COUNT_FUNCTION_QUADRATURE_X2_RISING] = "quadrature x2 rising",
>> + [COUNT_FUNCTION_QUADRATURE_X2_FALLING] = "quadrature x2 falling",
>> + [COUNT_FUNCTION_QUADRATURE_X4] = "quadrature x4"
>> +};
>> +
>> +static ssize_t counter_function_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + int err;
>> + struct counter_device *const counter = dev_get_drvdata(dev);
>> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
>> + const struct count_comp_t *const component = devattr->component;
>> + struct counter_count *const count = component->count;
>> + size_t func_index;
>> + enum count_function function;
>> +
>> + err = counter->ops->function_get(counter, count, &func_index);
>> + if (err)
>> + return err;
>> +
>> + count->function = func_index;
>> +
>> + function = count->functions_list[func_index];
>> + return scnprintf(buf, PAGE_SIZE, "%s\n", count_function_str[function]);
>> +}
>> +
>> +static ssize_t counter_function_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t len)
>> +{
>> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
>> + const struct count_comp_t *const component = devattr->component;
>> + struct counter_count *const count = component->count;
>> + const size_t num_functions = count->num_functions;
>> + size_t func_index;
>> + enum count_function function;
>> + int err;
>> + struct counter_device *const counter = dev_get_drvdata(dev);
>> +
>> + /* Find requested Count function mode */
>> + for (func_index = 0; func_index < num_functions; func_index++) {
>> + function = count->functions_list[func_index];
>> + if (sysfs_streq(buf, count_function_str[function]))
>> + break;
>> + }
>> + /* Return error if requested Count function mode not found */
>> + if (func_index >= num_functions)
>> + return -EINVAL;
>> +
>> + err = counter->ops->function_set(counter, count, func_index);
>> + if (err)
>> + return err;
>> +
>> + count->function = func_index;
>> +
>> + return len;
>> +}
>> +
>> +struct count_ext_comp_t {
>> + struct counter_count *count;
>> + const struct counter_count_ext *ext;
>> +};
>> +
>> +static ssize_t counter_count_ext_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
>> + const struct count_ext_comp_t *const comp = devattr->component;
>> + const struct counter_count_ext *const ext = comp->ext;
>> +
>> + return ext->read(dev_get_drvdata(dev), comp->count, ext->priv, buf);
>> +}
>> +
>> +static ssize_t counter_count_ext_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t len)
>> +{
>> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
>> + const struct count_ext_comp_t *const comp = devattr->component;
>> + const struct counter_count_ext *const ext = comp->ext;
>> +
>> + return ext->write(dev_get_drvdata(dev), comp->count, ext->priv, buf,
>> + len);
>> +}
>> +
>> +static int counter_count_ext_register(
>> + struct counter_device_attr_group *const group,
>> + struct counter_count *const count)
>> +{
>> + const size_t num_ext = count->num_ext;
>
>Used in one place, just put it inline?
>
>> + size_t i;
>> + const struct counter_count_ext *ext;
>> + struct count_ext_comp_t *count_ext_comp;
>> + int err;
>> +
>> + /* Create an attribute for each extension */
>> + for (i = 0 ; i < num_ext; i++) {
>> + ext = count->ext + i;
>> +
>> + /* Allocate count_ext attribute component */
>> + count_ext_comp = kmalloc(sizeof(*count_ext_comp), GFP_KERNEL);
>> + if (!count_ext_comp) {
>> + err = -ENOMEM;
>> + goto err_free_attr_list;
>> + }
>> + count_ext_comp->count = count;
>> + count_ext_comp->ext = ext;
>> +
>> + /* Allocate count_ext attribute */
>> + err = counter_attribute_create(group, "", ext->name,
>> + (ext->read) ? counter_count_ext_show : NULL,
>> + (ext->write) ? counter_count_ext_store : NULL,
>> + count_ext_comp);
>> + if (err) {
>> + kfree(count_ext_comp);
>> + goto err_free_attr_list;
>> + }
>> + }
>> +
>> + return 0;
>> +
>> +err_free_attr_list:
>> + free_counter_device_attr_list(&group->attr_list);
>> + return err;
>> +}
>> +
>> +struct func_avail_comp_t {
>> + const enum count_function *functions_list;
>> + size_t num_functions;
>> +};
>> +
>> +static ssize_t counter_count_function_available_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
>> + const struct func_avail_comp_t *const component = devattr->component;
>> + const enum count_function *const func_list = component->functions_list;
>> + const size_t num_functions = component->num_functions;
>> + size_t i;
>> + enum count_function function;
>> + ssize_t len = 0;
>> +
>> + for (i = 0; i < num_functions; i++) {
>> + function = func_list[i];
>> + len += scnprintf(buf + len, PAGE_SIZE - len, "%s\n",
>> + count_function_str[function]);
>> + }
>> +
>> + return len;
>> +}
>> +
>> +static int counter_count_attributes_create(
>> + struct counter_device_attr_group *const group,
>> + const struct counter_device *const counter,
>> + struct counter_count *const count)
>> +{
>> + struct count_comp_t *count_comp;
>> + int err;
>> + struct count_comp_t *func_comp;
>> + struct func_avail_comp_t *avail_comp;
>> +
>> + /* Allocate count attribute component */
>> + count_comp = kmalloc(sizeof(*count_comp), GFP_KERNEL);
>> + if (!count_comp)
>> + return -ENOMEM;
>> + count_comp->count = count;
>> +
>> + /* Create main Count attribute */
>> + err = counter_attribute_create(group, "", "count",
>> + (counter->ops->count_read) ? counter_count_show : NULL,
>> + (counter->ops->count_write) ? counter_count_store : NULL,
>> + count_comp);
>> + if (err) {
>> + kfree(count_comp);
>> + return err;
>> + }
>> +
>> + /* Allocate function attribute component */
>> + func_comp = kmalloc(sizeof(*func_comp), GFP_KERNEL);
>> + if (!func_comp) {
>> + err = -ENOMEM;
>> + goto err_free_attr_list;
>> + }
>> + func_comp->count = count;
>> +
>> + /* Create Count function attribute */
>> + err = counter_attribute_create(group, "", "function",
>> + (counter->ops->function_get) ? counter_function_show : NULL,
>> + (counter->ops->function_set) ? counter_function_store : NULL,
>> + func_comp);
>> + if (err) {
>> + kfree(func_comp);
>> + goto err_free_attr_list;
>> + }
>> +
>> + /* Allocate function available attribute component */
>> + avail_comp = kmalloc(sizeof(*avail_comp), GFP_KERNEL);
>> + if (!avail_comp) {
>> + err = -ENOMEM;
>> + goto err_free_attr_list;
>> + }
>> + avail_comp->functions_list = count->functions_list;
>> + avail_comp->num_functions = count->num_functions;
>> +
>> + /* Create Count function_available attribute */
>> + err = counter_attribute_create(group, "", "function_available",
>> + counter_count_function_available_show,
>> + NULL, avail_comp);
>> + if (err) {
>> + kfree(avail_comp);
>> + goto err_free_attr_list;
>> + }
>> +
>> + /* Create Count name attribute */
>> + err = counter_name_attribute_create(group, count->name);
>> + if (err)
>> + goto err_free_attr_list;
>> +
>> + /* Register Count extension attributes */
>> + err = counter_count_ext_register(group, count);
>> + if (err)
>> + goto err_free_attr_list;
>> +
>> + return 0;
>> +
>> +err_free_attr_list:
>> + free_counter_device_attr_list(&group->attr_list);
>> + return err;
>> +}
>> +
>> +static int counter_counts_register(
>> + struct counter_device_attr_group *const groups_list,
>> + const struct counter_device *const counter)
>> +{
>> + const size_t num_counts = counter->num_counts;
>
>I think this is only used on one place. Not sure having
>a local variable is worthwhile.
>
>> + size_t i;
>> + struct counter_count *count;
>> + const char *name;
>> + int err;
>> +
>> + /* Register each Count */
>> + for (i = 0; i < num_counts; i++) {
>> + count = counter->counts + i;
>> +
>> + /* Generate Count attribute directory name */
>> + name = kasprintf(GFP_KERNEL, "count%d", count->id);
>> + if (!name) {
>> + err = -ENOMEM;
>> + goto err_free_attr_groups;
>> + }
>> + groups_list[i].attr_group.name = name;
>> +
>> + /* Register the Synapses associated with each Count */
>> + err = counter_synapses_register(groups_list + i, counter, count,
>> + name);
>> + if (err)
>> + goto err_free_attr_groups;
>> +
>> + /* Create all attributes associated with Count */
>> + err = counter_count_attributes_create(groups_list + i, counter,
>> + count);
>> + if (err)
>> + goto err_free_attr_groups;
>> + }
>> +
>> + return 0;
>> +
>> +err_free_attr_groups:
>> + do {
>> + kfree(groups_list[i].attr_group.name);
>> + free_counter_device_attr_list(&groups_list[i].attr_list);
>> + } while (i--);
>> + return err;
>> +}
>> +
>> +struct size_comp_t {
>> + size_t size;
>> +};
>> +
>> +static ssize_t counter_device_attr_size_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + const struct size_comp_t *const comp = to_counter_attr(attr)->component;
>> +
>> + return scnprintf(buf, PAGE_SIZE, "%zu\n", comp->size);
>> +}
>> +
>> +static int counter_size_attribute_create(
>> + struct counter_device_attr_group *const group,
>> + const size_t size, const char *const name)
>> +{
>> + struct size_comp_t *size_comp;
>> + int err;
>> +
>> + /* Allocate size attribute component */
>> + size_comp = kmalloc(sizeof(*size_comp), GFP_KERNEL);
>> + if (!size_comp)
>> + return -ENOMEM;
>> + size_comp->size = size;
>> +
>> + err = counter_attribute_create(group, "", name,
>> + counter_device_attr_size_show, NULL,
>> + size_comp);
>> + if (err)
>> + goto err_free_size_comp;
>> +
>> + return 0;
>> +
>> +err_free_size_comp:
>> + kfree(size_comp);
>> + return err;
>> +}
>> +
>> +struct ext_comp_t {
>> + const struct counter_device_ext *ext;
>> +};
>> +
>> +static ssize_t counter_device_ext_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
>> + const struct ext_comp_t *const component = devattr->component;
>> + const struct counter_device_ext *const ext = component->ext;
>> +
>> + return ext->read(dev_get_drvdata(dev), ext->priv, buf);
>> +}
>> +
>> +static ssize_t counter_device_ext_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t len)
>> +{
>> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
>> + const struct ext_comp_t *const component = devattr->component;
>> + const struct counter_device_ext *const ext = component->ext;
>> +
>> + return ext->write(dev_get_drvdata(dev), ext->priv, buf, len);
>> +}
>> +
>> +static int counter_device_ext_register(
>> + struct counter_device_attr_group *const group,
>> + struct counter_device *const counter)
>> +{
>> + const size_t num_ext = counter->num_ext;
>
>num_ext only used in one place so if anything the local variable
>is hurting readability.
>
>> + struct ext_comp_t *ext_comp;
>> + size_t i;
>> + const struct counter_device_ext *ext;
>> + int err;
>> +
>> + /* Create an attribute for each extension */
>> + for (i = 0 ; i < num_ext; i++) {
>> + ext = counter->ext + i;
>
>This local variable isn't gaining us much that I can see. Just
>use the value directly.
>
>> +
>> + /* Allocate extension attribute component */
>> + ext_comp = kmalloc(sizeof(*ext_comp), GFP_KERNEL);
>> + if (!ext_comp) {
>> + err = -ENOMEM;
>> + goto err_free_attr_list;
>> + }
>> +
>> + ext_comp->ext = ext;
>> +
>> + /* Allocate extension attribute */
>> + err = counter_attribute_create(group, "", ext->name,
>> + (ext->read) ? counter_device_ext_show : NULL,
>> + (ext->write) ? counter_device_ext_store : NULL,
>> + ext_comp);
>> + if (err) {
>> + kfree(ext_comp);
>> + goto err_free_attr_list;
>> + }
>> + }
>> +
>> + return 0;
>> +
>> +err_free_attr_list:
>> + free_counter_device_attr_list(&group->attr_list);
>> + return err;
>> +}
>> +
>> +static int counter_global_attr_register(
>> + struct counter_device_attr_group *const group,
>> + struct counter_device *const counter)
>> +{
>> + int err;
>> +
>> + /* Create name attribute */
>> + err = counter_name_attribute_create(group, counter->name);
>> + if (err)
>> + return err;
>> +
>> + /* Create num_counts attribute */
>> + err = counter_size_attribute_create(group, counter->num_counts,
>> + "num_counts");
>> + if (err)
>> + goto err_free_attr_list;
>> +
>> + /* Create num_signals attribute */
>> + err = counter_size_attribute_create(group, counter->num_signals,
>> + "num_signals");
>> + if (err)
>> + goto err_free_attr_list;
>> +
>> + /* Register Counter device extension attributes */
>> + err = counter_device_ext_register(group, counter);
>> + if (err)
>> + goto err_free_attr_list;
>> +
>> + return 0;
>> +
>> +err_free_attr_list:
>> + free_counter_device_attr_list(&group->attr_list);
>> + return err;
>> +}
>> +
>> +static void free_counter_device_groups_list(
>> + struct counter_device_attr_group *const groups_list,
>> + const size_t num_groups)
>> +{
>> + struct counter_device_attr_group *group;
>> + size_t i;
>> +
>> + for (i = 0; i < num_groups; i++) {
>> + group = groups_list + i;
>> +
>
>I'd like to see a comment somewhere here on the fact this cleans up both
>the registered per counter stuff and the global attributes (that took
>a bit of chasing to check it did...)
>
>> + kfree(group->attr_group.name);
>> + kfree(group->attr_group.attrs);
>> + free_counter_device_attr_list(&group->attr_list);
>> + }
>> +
>> + kfree(groups_list);
>> +}
>> +
>> +static int prepare_counter_device_groups_list(
>> + struct counter_device *const counter)
>> +{
>> + const size_t total_num_groups =
>> + counter->num_signals + counter->num_counts + 1;
>> + struct counter_device_attr_group *groups_list;
>> + size_t i;
>> + int err;
>> + size_t num_groups = 0;
>> +
>> + /* Allocate space for attribute groups (signals. counts, and ext) */
>> + groups_list = kcalloc(total_num_groups, sizeof(*groups_list),
>> + GFP_KERNEL);
>> + if (!groups_list)
>> + return -ENOMEM;
>> +
>> + /* Initialize attribute lists */
>> + for (i = 0; i < total_num_groups; i++)
>> + INIT_LIST_HEAD(&groups_list[i].attr_list);
>> +
>> + /* Register Signals */
>> + err = counter_signals_register(groups_list, counter);
>> + if (err)
>> + goto err_free_groups_list;
>> + num_groups += counter->num_signals;
>> +
>> + /* Register Counts and respective Synapses */
>> + err = counter_counts_register(groups_list + num_groups, counter);
>> + if (err)
>> + goto err_free_groups_list;
>> + num_groups += counter->num_counts;
>> +
>> + /* Register Counter global attributes */
>> + err = counter_global_attr_register(groups_list + num_groups, counter);
>> + if (err)
>> + goto err_free_groups_list;
>> + num_groups++;
>> +
>> + /* Store groups_list in device_state */
>> + counter->device_state->groups_list = groups_list;
>> + counter->device_state->num_groups = num_groups;
>> +
>> + return 0;
>> +
>> +err_free_groups_list:
>> + free_counter_device_groups_list(groups_list, num_groups);
>
>Consistent naming would be good.
>
>counter_device_groups_list_free.
>
>I would tidy this up throughout. I know from experience that
>you'll probably end up doing so eventually and it is easier whilst
>there isn't too much code.
>
>> + return err;
>> +}
>> +
>> +static int prepare_counter_device_groups(
>> + struct counter_device_state *const device_state)
>> +{
>> + size_t i;
>> + struct counter_device_attr_group *group;
>> + int err;
>> + size_t j;
>
>Tidy this up a little,
>size_t i, j;
>
>> + struct counter_device_attr *p;
>> +
>> + /* Allocate attribute groups for association with device */
>> + device_state->groups = kcalloc(device_state->num_groups + 1,
>> + sizeof(*device_state->groups),
>> + GFP_KERNEL);
>> + if (!device_state->groups)
>> + return -ENOMEM;
>> +
>> + /* Prepare each group of attributes for association */
>> + for (i = 0; i < device_state->num_groups; i++) {
>> + group = device_state->groups_list + i;
>> +
>> + /* Allocate space for attribute pointers in attribute group */
>> + group->attr_group.attrs = kcalloc(group->num_attr + 1,
>> + sizeof(*group->attr_group.attrs), GFP_KERNEL);
>> + if (!group->attr_group.attrs) {
>> + err = -ENOMEM;
>> + goto err_free_groups;
>> + }
>> +
>> + /* Add attribute pointers to attribute group */
>> + j = 0;
>> + list_for_each_entry(p, &group->attr_list, l)
>> + group->attr_group.attrs[j++] = &p->dev_attr.attr;
>> +
>> + /* Group attributes in attribute group */
>> + device_state->groups[i] = &group->attr_group;
>> + }
>> + /* Associate attributes with device */
>> + device_state->dev.groups = device_state->groups;
>> +
>> + return 0;
>> +
>> +err_free_groups:
>
>I'm not convinced this is cleaning up properly. What about
>the kcalloc of group->attr_group.attrs for previous loops?
>
>> + kfree(device_state->groups);
>> + return err;
>> +}
>> +
>> +/* Provides a unique ID for each counter device */
>> +static DEFINE_IDA(counter_ida);
>> +
>> +static void counter_device_release(struct device *dev)
>> +{
>> + struct counter_device *const counter = dev_get_drvdata(dev);
>> + struct counter_device_state *const device_state = counter->device_state;
>> +
>> + kfree(device_state->groups);
>> + free_counter_device_groups_list(device_state->groups_list,
>> + device_state->num_groups);
>> + ida_simple_remove(&counter_ida, device_state->id);
>> + kfree(device_state);
>> +}
>> +
>> +static struct device_type counter_device_type = {
>> + .name = "counter_device",
>> + .release = counter_device_release
>> +};
>> +
>> +static struct bus_type counter_bus_type = {
>> + .name = "counter"
>> +};
>> +
>> +/**
>> + * counter_register - register Counter to the system
>> + * @counter: pointer to Counter to register
>> + *
>> + * This function registers a Counter to the system. A sysfs "counter" directory
>> + * will be created and populated with sysfs attributes correlating with the
>> + * Counter Signals, Synapses, and Counts respectively.
>> + */
>> +int counter_register(struct counter_device *const counter)
>> +{
>> + struct counter_device_state *device_state;
>> + int err;
>> +
>> + /* Allocate internal state container for Counter device */
>> + device_state = kzalloc(sizeof(*device_state), GFP_KERNEL);
>> + if (!device_state)
>> + return -ENOMEM;
>> + counter->device_state = device_state;
>> +
>> + /* Acquire unique ID */
>> + device_state->id = ida_simple_get(&counter_ida, 0, 0, GFP_KERNEL);
>> + if (device_state->id < 0) {
>> + err = device_state->id;
>> + goto err_free_device_state;
>> + }
>> +
>> + /* Configure device structure for Counter */
>> + device_state->dev.type = &counter_device_type;
>> + device_state->dev.bus = &counter_bus_type;
>> + if (counter->parent) {
>> + device_state->dev.parent = counter->parent;
>> + device_state->dev.of_node = counter->parent->of_node;
>> + }
>> + dev_set_name(&device_state->dev, "counter%d", device_state->id);
>> + device_initialize(&device_state->dev);
>> + dev_set_drvdata(&device_state->dev, counter);
>> +
>> + /* Prepare device attributes */
>> + err = prepare_counter_device_groups_list(counter);
>> + if (err)
>> + goto err_free_id;
>> +
>> + /* Organize device attributes to groups and match to device */
>> + err = prepare_counter_device_groups(device_state);
>> + if (err)
>> + goto err_free_groups_list;
>> +
>> + /* Add device to system */
>> + err = device_add(&device_state->dev);
>> + if (err)
>> + goto err_free_groups;
>> +
>> + return 0;
>> +
>> +err_free_groups:
>> + kfree(device_state->groups);
>> +err_free_groups_list:
>> + free_counter_device_groups_list(device_state->groups_list,
>> + device_state->num_groups);
>> +err_free_id:
>> + ida_simple_remove(&counter_ida, device_state->id);
>> +err_free_device_state:
>> + kfree(device_state);
>> + return err;
>> +}
>> +EXPORT_SYMBOL(counter_register);
>> +
>> +/**
>> + * counter_unregister - unregister Counter from the system
>> + * @counter: pointer to Counter to unregister
>> + *
>> + * The Counter is unregistered from the system; all allocated memory is freed.
>> + */
>> +void counter_unregister(struct counter_device *const counter)
>> +{
>> + if (counter)
>> + device_del(&counter->device_state->dev);
>> +}
>> +EXPORT_SYMBOL(counter_unregister);
>> +
>> +static void devm_counter_unreg(struct device *dev, void *res)
>> +{
>> + counter_unregister(*(struct counter_device **)res);
>> +}
>> +
>> +/**
>> + * devm_counter_register - Resource-managed counter_register
>> + * @dev: device to allocate counter_device for
>> + * @counter: pointer to Counter to register
>> + *
>> + * Managed counter_register. The Counter registered with this function is
>> + * automatically unregistered on driver detach. This function calls
>> + * counter_register internally. Refer to that function for more information.
>> + *
>> + * If an Counter registered with this function needs to be unregistered
>> + * separately, devm_counter_unregister must be used.
>> + *
>> + * RETURNS:
>> + * 0 on success, negative error number on failure.
>> + */
>> +int devm_counter_register(struct device *dev,
>> + struct counter_device *const counter)
>> +{
>> + struct counter_device **ptr;
>> + int ret;
>> +
>> + ptr = devres_alloc(devm_counter_unreg, sizeof(*ptr), GFP_KERNEL);
>> + if (!ptr)
>> + return -ENOMEM;
>> +
>> + ret = counter_register(counter);
>> + if (!ret) {
>> + *ptr = counter;
>> + devres_add(dev, ptr);
>> + } else {
>> + devres_free(ptr);
>> + }
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(devm_counter_register);
>> +
>> +static int devm_counter_match(struct device *dev, void *res, void *data)
>> +{
>> + struct counter_device **r = res;
>> +
>> + if (!r || !*r) {
>> + WARN_ON(!r || !*r);
>> + return 0;
>> + }
>> +
>> + return *r == data;
>> +}
>> +
>> +/**
>> + * devm_counter_unregister - Resource-managed counter_unregister
>> + * @dev: device this counter_device belongs to
>> + * @counter: pointer to Counter associated with the device
>> + *
>> + * Unregister Counter registered with devm_counter_register.
>> + */
>> +void devm_counter_unregister(struct device *dev,
>> + struct counter_device *const counter)
>> +{
>> + int rc;
>> +
>> + rc = devres_release(dev, devm_counter_unreg, devm_counter_match,
>> + counter);
>> + WARN_ON(rc);
>> +}
>> +EXPORT_SYMBOL(devm_counter_unregister);
>> +
>> +static int __init counter_init(void)
>> +{
>> + return bus_register(&counter_bus_type);
>> +}
>> +
>> +static void __exit counter_exit(void)
>> +{
>> + bus_unregister(&counter_bus_type);
>> +}
>> +
>> +subsys_initcall(counter_init);
>> +module_exit(counter_exit);
>> +
>> +MODULE_AUTHOR("William Breathitt Gray <vilhelm.gray@gmail.com>");
>> +MODULE_DESCRIPTION("Generic Counter interface");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/counter.h b/include/linux/counter.h
>> new file mode 100644
>> index 000000000000..a0b0349d098a
>> --- /dev/null
>> +++ b/include/linux/counter.h
>> @@ -0,0 +1,554 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Counter interface
>> + * Copyright (C) 2017 William Breathitt Gray
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License, version 2, as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * General Public License for more details.
>
>It's a bit controversial, but most people consider SPDX headers equivalent
>of the license statement. As such you normally don't have both and just
>go for the SPDK header.
>
>I thought we were also standardising on
>// SPDX-...
>
>
>> + */
>> +#ifndef _COUNTER_H_
>> +#define _COUNTER_H_
>> +
>> +#include <linux/device.h>
>> +#include <linux/types.h>
>> +
>> +enum count_direction {
>> + COUNT_DIRECTION_FORWARD = 0,
>> + COUNT_DIRECTION_BACKWARD
>> +};
>> +extern const char *const count_direction_str[2];
>> +
>> +enum count_mode {
>> + COUNT_MODE_NORMAL = 0,
>> + COUNT_MODE_RANGE_LIMIT,
>> + COUNT_MODE_NON_RECYCLE,
>> + COUNT_MODE_MODULO_N
>> +};
>> +extern const char *const count_mode_str[4];
>> +
>> +struct counter_device;
>> +struct counter_signal;
>> +
>> +/**
>> + * struct counter_signal_ext - Counter Signal extensions
>> + * @name: attribute name
>> + * @read: read callback for this attribute; may be NULL
>> + * @write: write callback for this attribute; may be NULL
>> + * @priv: data private to the driver
>> + */
>> +struct counter_signal_ext {
>> + const char *name;
>> + ssize_t (*read)(struct counter_device *counter,
>> + struct counter_signal *signal, void *priv,
>> + char *buf);
>> + ssize_t (*write)(struct counter_device *counter,
>> + struct counter_signal *signal, void *priv,
>> + const char *buf, size_t len);
>> + void *priv;
>> +};
>> +
>> +/**
>> + * struct counter_signal - Counter Signal node
>> + * @id: unique ID used to identify signal
>> + * @name: device-specific Signal name; ideally, this should match the name
>> + * as it appears in the datasheet documentation
>> + * @ext: optional array of Counter Signal extensions
>> + * @num_ext: number of Counter Signal extensions specified in @ext
>> + * @priv: optional private data supplied by driver
>> + */
>> +struct counter_signal {
>> + int id;
>> + const char *name;
>> +
>> + const struct counter_signal_ext *ext;
>> + size_t num_ext;
>> +
>> + void *priv;
>> +};
>> +
>> +/**
>> + * struct counter_signal_enum_ext - Signal enum extension attribute
>> + * @items: Array of strings
>> + * @num_items: Number of items specified in @items
>> + * @set: Set callback function; may be NULL
>> + * @get: Get callback function; may be NULL
>> + *
>> + * The counter_signal_enum_ext structure can be used to implement enum style
>> + * Signal extension attributes. Enum style attributes are those which have a set
>> + * of strings that map to unsigned integer values. The Generic Counter Signal
>> + * enum extension helper code takes care of mapping between value and string, as
>> + * well as generating a "_available" file which contains a list of all available
>> + * items. The get callback is used to query the currently active item; the index
>> + * of the item within the respective items array is returned via the 'item'
>> + * parameter. The set callback is called when the attribute is updated; the
>> + * 'item' parameter contains the index of the newly activated item within the
>> + * respective items array.
>> + */
>> +struct counter_signal_enum_ext {
>> + const char * const *items;
>> + size_t num_items;
>> + int (*get)(struct counter_device *counter,
>> + struct counter_signal *signal,
>> + size_t *item);
>> + int (*set)(struct counter_device *counter,
>> + struct counter_signal *signal,
>> + size_t item);
>> +};
>> +
>> +ssize_t counter_signal_enum_read(struct counter_device *counter,
>> + struct counter_signal *signal, void *priv,
>> + char *buf);
>> +ssize_t counter_signal_enum_write(struct counter_device *counter,
>> + struct counter_signal *signal, void *priv,
>> + const char *buf, size_t len);
>> +
>> +/**
>> + * COUNTER_SIGNAL_ENUM() - Initialize Signal enum extension
>> + * @_name: Attribute name
>> + * @_e: Pointer to a counter_count_enum structure
>> + *
>> + * This should usually be used together with COUNTER_SIGNAL_ENUM_AVAILABLE()
>> + */
>> +#define COUNTER_SIGNAL_ENUM(_name, _e) \
>> +{ \
>> + .name = (_name), \
>> + .read = counter_signal_enum_read, \
>> + .write = counter_signal_enum_write, \
>> + .priv = (_e) \
>> +}
>> +
>> +ssize_t counter_signal_enum_available_read(struct counter_device *counter,
>> + struct counter_signal *signal,
>> + void *priv, char *buf);
>> +
>> +/**
>> + * COUNTER_SIGNAL_ENUM_AVAILABLE() - Initialize Signal enum available extension
>> + * @_name: Attribute name ("_available" will be appended to the name)
>> + * @_e: Pointer to a counter_signal_enum structure
>> + *
>> + * Creates a read only attribute that lists all the available enum items in a
>> + * newline separated list. This should usually be used together with
>> + * COUNTER_SIGNAL_ENUM()
>> + */
>> +#define COUNTER_SIGNAL_ENUM_AVAILABLE(_name, _e) \
>> +{ \
>> + .name = (_name "_available"), \
>> + .read = counter_signal_enum_available_read, \
>> + .priv = (_e) \
>> +}
>> +
>> +enum synapse_action {
>> + SYNAPSE_ACTION_NONE = 0,
>> + SYNAPSE_ACTION_RISING_EDGE,
>> + SYNAPSE_ACTION_FALLING_EDGE,
>> + SYNAPSE_ACTION_BOTH_EDGES
>> +};
>> +
>> +/**
>> + * struct counter_synapse - Counter Synapse node
>> + * @action: index of current action mode
>> + * @actions_list: array of available action modes
>> + * @num_actions: number of action modes specified in @actions_list
>> + * @signal: pointer to associated signal
>> + */
>> +struct counter_synapse {
>> + size_t action;
>> + const enum synapse_action *actions_list;
>> + size_t num_actions;
>> +
>> + struct counter_signal *signal;
>> +};
>> +
>> +struct counter_count;
>> +
>> +/**
>> + * struct counter_count_ext - Counter Count extension
>> + * @name: attribute name
>> + * @read: read callback for this attribute; may be NULL
>> + * @write: write callback for this attribute; may be NULL
>> + * @priv: data private to the driver
>> + */
>> +struct counter_count_ext {
>> + const char *name;
>> + ssize_t (*read)(struct counter_device *counter,
>> + struct counter_count *count, void *priv,
>> + char *buf);
>> + ssize_t (*write)(struct counter_device *counter,
>> + struct counter_count *count, void *priv,
>> + const char *buf, size_t len);
>> + void *priv;
>> +};
>> +
>> +enum count_function {
>> + COUNT_FUNCTION_INCREASE = 0,
>> + COUNT_FUNCTION_DECREASE,
>> + COUNT_FUNCTION_PULSE_DIRECTION,
>> + COUNT_FUNCTION_QUADRATURE_X1_A,
>> + COUNT_FUNCTION_QUADRATURE_X1_B,
>> + COUNT_FUNCTION_QUADRATURE_X2_A,
>> + COUNT_FUNCTION_QUADRATURE_X2_B,
>> + COUNT_FUNCTION_QUADRATURE_X2_RISING,
>> + COUNT_FUNCTION_QUADRATURE_X2_FALLING,
>> + COUNT_FUNCTION_QUADRATURE_X4
>> +};
>> +
>> +/**
>> + * struct counter_count - Counter Count node
>> + * @id: unique ID used to identify Count
>> + * @name: device-specific Count name; ideally, this should match
>> + * the name as it appears in the datasheet documentation
>> + * @function: index of current function mode
>> + * @functions_list: array available function modes
>> + * @num_functions: number of function modes specified in @functions_list
>> + * @synapses: array of synapses for initialization
>> + * @num_synapses: number of synapses specified in @synapses
>> + * @ext: optional array of Counter Count extensions
>> + * @num_ext: number of Counter Count extensions specified in @ext
>> + * @priv: optional private data supplied by driver
>> + */
>> +struct counter_count {
>> + int id;
>> + const char *name;
>> +
>> + size_t function;
>> + const enum count_function *functions_list;
>> + size_t num_functions;
>
>There is a very good illustration here of the issues with using
>tabs to pretty print structure elements. I would drop them entirely as
>personally I'm not sure they help readability and you will forever be having
>more noise in patches because of the need to change the number of tabs
>due to name changes etc.
>
>> +
>> + struct counter_synapse *synapses;
>> + size_t num_synapses;
>> +
>> + const struct counter_count_ext *ext;
>> + size_t num_ext;
>> +
>> + void *priv;
>> +};
>> +
>> +/**
>> + * struct counter_count_enum_ext - Count enum extension attribute
>> + * @items: Array of strings
>> + * @num_items: Number of items specified in @items
>> + * @set: Set callback function; may be NULL
>> + * @get: Get callback function; may be NULL
>> + *
>> + * The counter_count_enum_ext structure can be used to implement enum style
>> + * Count extension attributes. Enum style attributes are those which have a set
>> + * of strings that map to unsigned integer values. The Generic Counter Count
>> + * enum extension helper code takes care of mapping between value and string, as
>> + * well as generating a "_available" file which contains a list of all available
>> + * items. The get callback is used to query the currently active item; the index
>> + * of the item within the respective items array is returned via the 'item'
>> + * parameter. The set callback is called when the attribute is updated; the
>> + * 'item' parameter contains the index of the newly activated item within the
>> + * respective items array.
>> + */
>> +struct counter_count_enum_ext {
>> + const char * const *items;
>> + size_t num_items;
>> + int (*get)(struct counter_device *counter,
>> + struct counter_count *count,
>> + size_t *item);
>> + int (*set)(struct counter_device *counter,
>> + struct counter_count *count,
>> + size_t item);
>> +};
>> +
>> +ssize_t counter_count_enum_read(struct counter_device *counter,
>> + struct counter_count *count, void *priv,
>> + char *buf);
>> +ssize_t counter_count_enum_write(struct counter_device *counter,
>> + struct counter_count *count, void *priv,
>> + const char *buf, size_t len);
>> +
>> +/**
>> + * COUNTER_COUNT_ENUM() - Initialize Count enum extension
>> + * @_name: Attribute name
>> + * @_e: Pointer to a counter_count_enum structure
>> + *
>> + * This should usually be used together with COUNTER_COUNT_ENUM_AVAILABLE()
>> + */
>> +#define COUNTER_COUNT_ENUM(_name, _e) \
>> +{ \
>> + .name = (_name), \
>> + .read = counter_count_enum_read, \
>> + .write = counter_count_enum_write, \
>> + .priv = (_e) \
>> +}
>> +
>> +ssize_t counter_count_enum_available_read(struct counter_device *counter,
>> + struct counter_count *count,
>> + void *priv, char *buf);
>> +
>> +/**
>> + * COUNTER_COUNT_ENUM_AVAILABLE() - Initialize Count enum available extension
>> + * @_name: Attribute name ("_available" will be appended to the name)
>> + * @_e: Pointer to a counter_count_enum structure
>> + *
>> + * Creates a read only attribute that lists all the available enum items in a
>> + * newline separated list. This should usually be used together with
>> + * COUNTER_COUNT_ENUM()
>> + */
>> +#define COUNTER_COUNT_ENUM_AVAILABLE(_name, _e) \
>> +{ \
>> + .name = (_name "_available"), \
>> + .read = counter_count_enum_available_read, \
>> + .priv = (_e) \
>> +}
>> +
>> +/**
>> + * struct counter_device_attr_group - internal container for attribute group
>> + * @attr_group: Counter sysfs attributes group
>> + * @attr_list: list to keep track of created Counter sysfs attributes
>> + * @num_attr: number of Counter sysfs attributes
>> + */
>> +struct counter_device_attr_group {
>> + struct attribute_group attr_group;
>> + struct list_head attr_list;
>> + size_t num_attr;
>> +};
>> +
>> +/**
>> + * struct counter_device_state - internal state container for a Counter device
>> + * @id: unique ID used to identify the Counter
>> + * @dev: internal device structure
>> + * @groups_list attribute groups list (groups for Signals, Counts, and ext)
>
>Run kernel-doc script over these files. You are missing some :s and it would
>have told you that.
>
>> + * @num_groups number of attribute groups containers
>> + * @groups: Counter sysfs attribute groups (used to populate @dev.groups)
>> + */
>> +struct counter_device_state {
>> + int id;
>> + struct device dev;
>> + struct counter_device_attr_group *groups_list;
>> + size_t num_groups;
>> + const struct attribute_group **groups;
>> +};
>> +
>> +/**
>> + * struct signal_read_value - Opaque Signal read value
>> + * @buf: string representation of Signal read value
>> + * @len: length of string in @buf
>> + */
>> +struct signal_read_value {
>> + char *buf;
>> + size_t len;
>> +};
>> +
>> +/**
>> + * struct count_read_value - Opaque Count read value
>> + * @buf: string representation of Count read value
>> + * @len: length of string in @buf
>> + */
>> +struct count_read_value {
>> + char *buf;
>> + size_t len;
>> +};
>> +
>> +/**
>> + * struct count_write_value - Opaque Count write value
>> + * @buf: string representation of Count write value
>> + */
>> +struct count_write_value {
>> + const char *buf;
>> +};
>> +
>> +/**
>> + * struct counter_ops - Callbacks from driver
>> + * @signal_read: optional read callback for Signal attribute. The read
>> + * value of the respective Signal should be passed back via
>> + * the val parameter. val points to an opaque type which
>> + * should be set only via the set_signal_read_value
>> + * function.
>
>This last part had me a little confused. I would make it clear that this
>set_signal_read_value function should be called to set the value within this
>signal_read callback rather than elsewhere...
>
>> + * @count_read: optional read callback for Count attribute. The read
>> + * value of the respective Count should be passed back via
>> + * the val parameter. val points to an opaque type which
>> + * should be set only via the set_count_read_value
>> + * function.
>> + * @count_write: optional write callback for Count attribute. The write
>> + * value for the respective Count is passed in via the val
>> + * parameter. val points to an opaque type which should be
>> + * access only via the get_count_write_value function.
>> + * @function_get: function to get the current count function mode. Returns
>> + * 0 on success and negative error code on error. The index
>> + * of the respective Count's returned function mode should
>> + * be passed back via the function parameter.
>> + * @function_set: function to set the count function mode. function is the
>> + * index of the requested function mode from the respective
>> + * Count's functions_list array.
>> + * @action_get: function to get the current action mode. Returns 0 on
>> + * success and negative error code on error. The index of
>> + * the respective Signal's returned action mode should be
>> + * passed back via the action parameter.
>> + * @action_set: function to set the action mode. action is the index of
>> + * the requested action mode from the respective Synapse's
>> + * actions_list array.
>> + */
>> +struct counter_ops {
>> + int (*signal_read)(struct counter_device *counter,
>> + struct counter_signal *signal,
>> + struct signal_read_value *val);
>> + int (*count_read)(struct counter_device *counter,
>> + struct counter_count *count,
>> + struct count_read_value *val);
>> + int (*count_write)(struct counter_device *counter,
>> + struct counter_count *count,
>> + struct count_write_value *val);
>> + int (*function_get)(struct counter_device *counter,
>> + struct counter_count *count, size_t *function);
>> + int (*function_set)(struct counter_device *counter,
>> + struct counter_count *count, size_t function);
>> + int (*action_get)(struct counter_device *counter,
>> + struct counter_count *count,
>> + struct counter_synapse *synapse, size_t *action);
>> + int (*action_set)(struct counter_device *counter,
>> + struct counter_count *count,
>> + struct counter_synapse *synapse, size_t action);
>> +};
>> +
>> +/**
>> + * struct counter_device_ext - Counter device extension
>> + * @name: attribute name
>> + * @read: read callback for this attribute; may be NULL
>> + * @write: write callback for this attribute; may be NULL
>> + * @priv: data private to the driver
>> + */
>> +struct counter_device_ext {
>> + const char *name;
>> + ssize_t (*read)(struct counter_device *counter, void *priv,
>> + char *buf);
>> + ssize_t (*write)(struct counter_device *counter, void *priv,
>> + const char *buf, size_t len);
>> + void *priv;
>> +};
>> +
>> +/**
>> + * struct counter_device_enum_ext - Counter enum extension attribute
>> + * @items: Array of strings
>> + * @num_items: Number of items specified in @items
>> + * @set: Set callback function; may be NULL
>> + * @get: Get callback function; may be NULL
>> + *
>> + * The counter_device_enum_ext structure can be used to implement enum style
>> + * Counter extension attributes. Enum style attributes are those which have a
>> + * set of strings that map to unsigned integer values. The Generic Counter enum
>> + * extension helper code takes care of mapping between value and string, as well
>> + * as generating a "_available" file which contains a list of all available
>> + * items. The get callback is used to query the currently active item; the index
>> + * of the item within the respective items array is returned via the 'item'
>> + * parameter. The set callback is called when the attribute is updated; the
>> + * 'item' parameter contains the index of the newly activated item within the
>> + * respective items array.
>> + */
>> +struct counter_device_enum_ext {
>> + const char * const *items;
>> + size_t num_items;
>> + int (*get)(struct counter_device *counter,
>> + size_t *item);
>> + int (*set)(struct counter_device *counter,
>> + size_t item);
>> +};
>> +
>> +ssize_t counter_device_enum_read(struct counter_device *counter, void *priv,
>> + char *buf);
>> +ssize_t counter_device_enum_write(struct counter_device *counter, void *priv,
>> + const char *buf, size_t len);
>> +
>> +/**
>> + * COUNTER_DEVICE_ENUM() - Initialize Counter enum extension
>> + * @_name: Attribute name
>> + * @_e: Pointer to a counter_device_enum structure
>> + *
>> + * This should usually be used together with COUNTER_DEVICE_ENUM_AVAILABLE()
>> + */
>> +#define COUNTER_DEVICE_ENUM(_name, _e) \
>> +{ \
>> + .name = (_name), \
>> + .read = counter_device_enum_read, \
>> + .write = counter_device_enum_write, \
>> + .priv = (_e) \
>> +}
>> +
>> +ssize_t counter_device_enum_available_read(struct counter_device *counter,
>> + void *priv, char *buf);
>> +
>> +/**
>> + * COUNTER_DEVICE_ENUM_AVAILABLE() - Initialize Counter enum available extension
>> + * @_name: Attribute name ("_available" will be appended to the name)
>> + * @_e: Pointer to a counter_device_enum structure
>> + *
>> + * Creates a read only attribute that lists all the available enum items in a
>> + * newline separated list. This should usually be used together with
>> + * COUNTER_DEVICE_ENUM()
>> + */
>> +#define COUNTER_DEVICE_ENUM_AVAILABLE(_name, _e) \
>> +{ \
>> + .name = (_name "_available"), \
>> + .read = counter_device_enum_available_read, \
>> + .priv = (_e) \
>> +}
>> +
>> +/**
>> + * struct counter_device - Counter data structure
>> + * @name: name of the device as it appears in the datasheet
>> + * @parent: optional parent device providing the counters
>> + * @device_state: internal device state container
>> + * @ops: callbacks from driver
>> + * @signals: array of Signals
>> + * @num_signals: number of Signals specified in @signals
>> + * @counts: array of Counts
>> + * @num_counts: number of Counts specified in @counts
>> + * @ext: optional array of Counter device extensions
>> + * @num_ext: number of Counter device extensions specified in @ext
>> + * @priv: optional private data supplied by driver
>> + */
>> +struct counter_device {
>> + const char *name;
>> + struct device *parent;
>> + struct counter_device_state *device_state;
>> +
>> + const struct counter_ops *ops;
>> +
>> + struct counter_signal *signals;
>> + size_t num_signals;
>> + struct counter_count *counts;
>> + size_t num_counts;
>> +
>> + const struct counter_device_ext *ext;
>> + size_t num_ext;
>> +
>> + void *priv;
>> +};
>> +
>> +enum signal_level {
>> + SIGNAL_LEVEL_LOW = 0,
>> + SIGNAL_LEVEL_HIGH
>> +};
>> +
>> +enum signal_value_type {
>> + SIGNAL_LEVEL = 0
>> +};
>> +
>> +enum count_value_type {
>> + COUNT_POSITION_UNSIGNED = 0,
>> + COUNT_POSITION_SIGNED
>> +};
>> +
>> +void set_signal_read_value(struct signal_read_value *const val,
>> + const enum signal_value_type type, void *const data);
>> +void set_count_read_value(struct count_read_value *const val,
>> + const enum count_value_type type, void *const data);
>> +int get_count_write_value(void *const data, const enum count_value_type type,
>> + const struct count_write_value *const val);
>
>I wonder if naming wise, we would be better sticking to the
>noun_verb naming format.
>
>signal_read_value_set
>count_read_value_set
>count_write_value_get
>
>for example?
>
>> +
>> +int counter_register(struct counter_device *const counter);
>> +void counter_unregister(struct counter_device *const counter);
>> +int devm_counter_register(struct device *dev,
>> + struct counter_device *const counter);
>> +void devm_counter_unregister(struct device *dev,
>> + struct counter_device *const counter);
>> +
>> +#endif /* _COUNTER_H_ */
>
^ permalink raw reply
* Re: [PATCH] cpufreq: Add Kryo CPU scaling driver
From: Sudeep Holla @ 2018-05-21 13:04 UTC (permalink / raw)
To: ilialin, mturquette, sboyd, robh, mark.rutland, viresh.kumar, nm,
lgirdwood, broonie, andy.gross, david.brown, catalin.marinas,
will.deacon, rjw, linux-clk
Cc: Sudeep Holla, devicetree, linux-kernel, linux-pm, linux-arm-msm,
linux-soc, linux-arm-kernel, rnayak, amit.kucheria,
nicolas.dechesne, celster, tfinkel
In-Reply-To: <000f01d3f103$3ff78ba0$bfe6a2e0$@codeaurora.org>
On 21/05/18 13:57, ilialin@codeaurora.org wrote:
>
[...]
>>> +#include <linux/cpu.h>
>>> +#include <linux/err.h>
>>> +#include <linux/init.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/nvmem-consumer.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/pm_opp.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/soc/qcom/smem.h>
>>> +
>>> +#define MSM_ID_SMEM 137
>>> +#define SILVER_LEAD 0
>>> +#define GOLD_LEAD 2
>>> +
>>
>> So I gather form other emails, that these are physical cpu number(not even
>> unique identifier like MPIDR). Will this work on parts or platforms that need
>> to boot in GOLD LEAD cpus.
>
> The driver is for Kryo CPU, which (and AFAIK all multicore MSMs)
> always boots on the CPU0.
That may be true and I am not that bothered about it. But assuming
physical ordering from the logical cpu number is *incorrect* and will
break if kernel decides to change the allocation algorithm. Kernel
provides no guarantee on that, so you need to depend on some physical ID
or may be DT to achieve what your want. But the current code as it
stands is wrong.
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH 00/27] Add multi-channel and overheat IRQ support to Armada thermal driver
From: Zhang Rui @ 2018-05-21 13:01 UTC (permalink / raw)
To: Miquel Raynal, Gregory CLEMENT
Cc: Mark Rutland, Andrew Lunn, Jason Cooper, Nadav Haklai, devicetree,
Antoine Tenart, Catalin Marinas, linux-pm, Will Deacon,
Maxime Chevallier, Eduardo Valentin, David Sniatkiwicz,
Rob Herring, Thomas Petazzoni, linux-arm-kernel,
Sebastian Hesselbarth
In-Reply-To: <20180518114935.38d6e883@xps13>
On 五, 2018-05-18 at 11:49 +0200, Miquel Raynal wrote:
> Hi Zhang, Eduardo & Gregory,
>
> On Wed, 16 May 2018 19:28:45 +0200, Gregory CLEMENT
> <gregory.clement@bootlin.com> wrote:
>
> >
> > Hi Miquel,
> >
> > On sam., avril 21 2018, Miquel Raynal <miquel.raynal@bootlin.com>
> > wrote:
> >
> > >
> > > The only capability of the Armada thermal driver is currently
> > > just to
> > > read one sensor (the default one) per AP and one per CP.
> > >
> > > Actually, there is one sensor per core in the AP806 plus one
> > > sensor in
> > > the thermal IP itself. The CP110 just features one thermal sensor
> > > in its
> > > own thermal IP.
> > >
> > > Also, there is no need for the thermal core to poll the
> > > temperature of
> > > each sensor by software as this IP (at least for AP806 and CP110
> > > compatibles) features an hardware overheat interrupt.
> > >
> > > This series first improves the readability of this driver, then
> > > adds
> > > support for multi-channel thermal IPs, and finally adds support
> > > for the
> > > hardware overheat interrupt. The bindings and the device-trees
> > > are
> > > updated accordingly.
> > >
> > > Please note that the thermal IP raises SEI interrupts, from which
> > > the
> > > support as just been contributed and not merged yet. Applying the
> > > last
> > > DT patches referring to the 'sei' and 'icu_sei' nodes will
> > > require this
> > > feature [1] to have been accepted first.
> > >
> > > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-Ap
> > > ril/572852.html
> > >
> > > Thank you,
> > > Miquèl
> > >
> > >
> > > Miquel Raynal (27):
> > > thermal: armada: add a function that sanitizes the thermal zone
> > > name
> > > thermal: armada: remove useless register accesses
> > > thermal: armada: remove misleading comments
> > > thermal: armada: rename the initialization routine
> > > thermal: armada: dissociate a380 and cp110 ->init() hooks
> > > thermal: armada: average over samples to avoid glitches
> > > thermal: armada: convert driver to syscon register accesses
> > > thermal: armada: use the resource managed registration helper
> > > alternative
> > > thermal: armada: add multi-channel sensors support
> > > thermal: armada: remove sensors validity from the IP
> > > initialization
> > > thermal: armada: move validity check out of the read function
> > > thermal: armada: get rid of the ->is_valid() pointer
> > > thermal: armada: add overheat interrupt support
> > > dt-bindings: cp110: rename cp110 syscon file
> > > dt-bindings: ap806: prepare the syscon file to list other
> > > syscons
> > > nodes
> > > dt-bindings: cp110: prepare the syscon file to list other
> > > syscons
> > > nodes
> > > dt-bindings: ap806: add the thermal node in the syscon file
> > > dt-bindings: cp110: update documentation since DT de-
> > > duplication
> > > dt-bindings: cp110: add the thermal node in the syscon file
> > > dt-bindings: thermal: armada: add reference to new bindings
> > > arm64: dts: marvell: rename ap806 syscon node
> > > arm64: dts: marvell: move AP806/CP110 thermal nodes into a new
> > > syscon
> > > arm64: dts: marvell: add thermal-zone node in ap806 DTSI file
> > > arm64: dts: marvell: add macro to make distinction between node
> > > names
> > > arm64: dts: marvell: add thermal-zone node in cp110 DTSI file
> > > arm64: dts: marvell: add interrupt support to ap806 thermal
> > > node
> > > arm64: dts: marvell: add interrupt support to cp110 thermal
> > > node
> > >
> > > .../arm/marvell/ap806-system-controller.txt | 55 +-
> > > ...controller0.txt => cp110-system-controller.txt} | 66 +-
> > > .../devicetree/bindings/thermal/armada-thermal.txt | 5 +
> > > arch/arm64/boot/dts/marvell/armada-ap806.dtsi | 85 +-
> > > arch/arm64/boot/dts/marvell/armada-common.dtsi | 1 +
> > > arch/arm64/boot/dts/marvell/armada-cp110.dtsi | 45 +-
> > > drivers/thermal/armada_thermal.c | 875
> > > ++++++++++++++++++---
> > > 7 files changed, 976 insertions(+), 156 deletions(-)
> > > rename Documentation/devicetree/bindings/arm/marvell/{cp110-
> > > system-controller0.txt => cp110-system-controller.txt} (83%)
> > What is the status of this series?
> > I am especially interested in the dt part.
> > Do you expect sending a new series modifying them?
> I have not received any feedback yet on the thermal part, bindings
> have
> been partially acked by Rob (one request, I will probably add a reg
> property in the AP node) so please do not take the DTS changes of
> this iteration.
>
> Zhang, Eduardo, could you please share the status of this series?
>
hmmm, this should go through Eduardo' thermal-soc tree, thus I'd expect
Eduardo to review this patch set.
Eduardo, what's your comments for this series?
thanks,
rui
> Thanks,
> Miquèl
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH] cpufreq: Add Kryo CPU scaling driver
From: ilialin @ 2018-05-21 12:57 UTC (permalink / raw)
To: 'Sudeep Holla', mturquette, sboyd, robh, mark.rutland,
viresh.kumar, nm, lgirdwood, broonie, andy.gross, david.brown,
catalin.marinas, will.deacon, rjw, linux-clk
Cc: devicetree, linux-kernel, linux-pm, linux-arm-msm, linux-soc,
linux-arm-kernel, rnayak, amit.kucheria, nicolas.dechesne,
celster, tfinkel
In-Reply-To: <153cc316-dcb5-972f-5a2f-c91fe0f6348b@arm.com>
> -----Original Message-----
> From: Sudeep Holla <sudeep.holla@arm.com>
> Sent: Monday, May 21, 2018 15:50
> To: Ilia Lin <ilialin@codeaurora.org>; mturquette@baylibre.com;
> sboyd@kernel.org; robh@kernel.org; mark.rutland@arm.com;
> viresh.kumar@linaro.org; nm@ti.com; lgirdwood@gmail.com;
> broonie@kernel.org; andy.gross@linaro.org; david.brown@linaro.org;
> catalin.marinas@arm.com; will.deacon@arm.com; rjw@rjwysocki.net; linux-
> clk@vger.kernel.org
> Cc: Sudeep Holla <sudeep.holla@arm.com>; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-pm@vger.kernel.org; linux-arm-
> msm@vger.kernel.org; linux-soc@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; rnayak@codeaurora.org;
> amit.kucheria@linaro.org; nicolas.dechesne@linaro.org;
> celster@codeaurora.org; tfinkel@codeaurora.org
> Subject: Re: [PATCH] cpufreq: Add Kryo CPU scaling driver
>
>
>
> On 19/05/18 12:35, Ilia Lin wrote:
> > In Certain QCOM SoCs like apq8096 and msm8996 that have KRYO
> > processors, the CPU frequency subset and voltage value of each OPP
> > varies based on the silicon variant in use. Qualcomm Process Voltage
> > Scaling Tables defines the voltage and frequency value based on the
> > msm-id in SMEM and speedbin blown in the efuse combination.
> > The qcom-cpufreq-kryo driver reads the msm-id and efuse value from the
> > SoC to provide the OPP framework with required information.
> > This is used to determine the voltage and frequency value for each OPP
> > of
> > operating-points-v2 table when it is parsed by the OPP framework.
> >
> > Signed-off-by: Ilia Lin <ilialin@codeaurora.org>
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > drivers/cpufreq/Kconfig.arm | 10 +++
> > drivers/cpufreq/Makefile | 1 +
> > drivers/cpufreq/cpufreq-dt-platdev.c | 3 +
> > drivers/cpufreq/qcom-cpufreq-kryo.c | 164
> > +++++++++++++++++++++++++++++++++++
> > 4 files changed, 178 insertions(+)
> > create mode 100644 drivers/cpufreq/qcom-cpufreq-kryo.c
> >
>
> [..]
>
> > +
> > +/*
> > + * In Certain QCOM SoCs like apq8096 and msm8996 that have KRYO
> > +processors,
> > + * the CPU frequency subset and voltage value of each OPP varies
> > + * based on the silicon variant in use. Qualcomm Process Voltage
> > +Scaling Tables
> > + * defines the voltage and frequency value based on the msm-id in
> > +SMEM
> > + * and speedbin blown in the efuse combination.
> > + * The qcom-cpufreq-kryo driver reads the msm-id and efuse value from
> > +the SoC
> > + * to provide the OPP framework with required information.
> > + * This is used to determine the voltage and frequency value for each
> > +OPP of
> > + * operating-points-v2 table when it is parsed by the OPP framework.
> > + */
> > +
> > +#include <linux/cpu.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_opp.h>
> > +#include <linux/slab.h>
> > +#include <linux/soc/qcom/smem.h>
> > +
> > +#define MSM_ID_SMEM 137
> > +#define SILVER_LEAD 0
> > +#define GOLD_LEAD 2
> > +
>
> So I gather form other emails, that these are physical cpu number(not even
> unique identifier like MPIDR). Will this work on parts or platforms that need
> to boot in GOLD LEAD cpus.
The driver is for Kryo CPU, which (and AFAIK all multicore MSMs) always boots on the CPU0.
>
> [...]
>
> > +
> > +static int __init qcom_cpufreq_kryo_driver_init(void)
> > +{
> > + struct device *cpu_dev_silver, *cpu_dev_gold;
> > + struct opp_table *opp_silver, *opp_gold;
> > + enum _msm8996_version msm8996_version;
> > + struct nvmem_cell *speedbin_nvmem;
> > + struct platform_device *pdev;
> > + struct device_node *np;
> > + u8 *speedbin;
> > + u32 versions;
> > + size_t len;
> > + int ret;
> > +
> > + cpu_dev_silver = get_cpu_device(SILVER_LEAD);
> > + if (IS_ERR_OR_NULL(cpu_dev_silver))
> > + return PTR_ERR(cpu_dev_silver);
> > +
> > + cpu_dev_gold = get_cpu_device(SILVER_LEAD);
>
> s/SILVER/GOLD/ ?
Yes, you are right. This is already fixed in the respin.
>
> --
> Regards,
> Sudeep
^ permalink raw reply
* Re: [PATCH] cpufreq: Add Kryo CPU scaling driver
From: Sudeep Holla @ 2018-05-21 12:50 UTC (permalink / raw)
To: Ilia Lin, mturquette, sboyd, robh, mark.rutland, viresh.kumar, nm,
lgirdwood, broonie, andy.gross, david.brown, catalin.marinas,
will.deacon, rjw, linux-clk
Cc: Sudeep Holla, devicetree, linux-kernel, linux-pm, linux-arm-msm,
linux-soc, linux-arm-kernel, rnayak, amit.kucheria,
nicolas.dechesne, celster, tfinkel
In-Reply-To: <1526729701-8589-1-git-send-email-ilialin@codeaurora.org>
On 19/05/18 12:35, Ilia Lin wrote:
> In Certain QCOM SoCs like apq8096 and msm8996 that have KRYO processors,
> the CPU frequency subset and voltage value of each OPP varies
> based on the silicon variant in use. Qualcomm Process Voltage Scaling Tables
> defines the voltage and frequency value based on the msm-id in SMEM
> and speedbin blown in the efuse combination.
> The qcom-cpufreq-kryo driver reads the msm-id and efuse value from the SoC
> to provide the OPP framework with required information.
> This is used to determine the voltage and frequency value for each OPP of
> operating-points-v2 table when it is parsed by the OPP framework.
>
> Signed-off-by: Ilia Lin <ilialin@codeaurora.org>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> drivers/cpufreq/Kconfig.arm | 10 +++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/cpufreq-dt-platdev.c | 3 +
> drivers/cpufreq/qcom-cpufreq-kryo.c | 164 +++++++++++++++++++++++++++++++++++
> 4 files changed, 178 insertions(+)
> create mode 100644 drivers/cpufreq/qcom-cpufreq-kryo.c
>
[..]
> +
> +/*
> + * In Certain QCOM SoCs like apq8096 and msm8996 that have KRYO processors,
> + * the CPU frequency subset and voltage value of each OPP varies
> + * based on the silicon variant in use. Qualcomm Process Voltage Scaling Tables
> + * defines the voltage and frequency value based on the msm-id in SMEM
> + * and speedbin blown in the efuse combination.
> + * The qcom-cpufreq-kryo driver reads the msm-id and efuse value from the SoC
> + * to provide the OPP framework with required information.
> + * This is used to determine the voltage and frequency value for each OPP of
> + * operating-points-v2 table when it is parsed by the OPP framework.
> + */
> +
> +#include <linux/cpu.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <linux/slab.h>
> +#include <linux/soc/qcom/smem.h>
> +
> +#define MSM_ID_SMEM 137
> +#define SILVER_LEAD 0
> +#define GOLD_LEAD 2
> +
So I gather form other emails, that these are physical cpu number(not
even unique identifier like MPIDR). Will this work on parts or platforms
that need to boot in GOLD LEAD cpus.
[...]
> +
> +static int __init qcom_cpufreq_kryo_driver_init(void)
> +{
> + struct device *cpu_dev_silver, *cpu_dev_gold;
> + struct opp_table *opp_silver, *opp_gold;
> + enum _msm8996_version msm8996_version;
> + struct nvmem_cell *speedbin_nvmem;
> + struct platform_device *pdev;
> + struct device_node *np;
> + u8 *speedbin;
> + u32 versions;
> + size_t len;
> + int ret;
> +
> + cpu_dev_silver = get_cpu_device(SILVER_LEAD);
> + if (IS_ERR_OR_NULL(cpu_dev_silver))
> + return PTR_ERR(cpu_dev_silver);
> +
> + cpu_dev_gold = get_cpu_device(SILVER_LEAD);
s/SILVER/GOLD/ ?
--
Regards,
Sudeep
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox