[PATCH resend] can: rcar_canfd: fix possible IRQ storm on high load
We have observed rcar_canfd driver entering IRQ storm under high load,
with following scenario:
- rcar_canfd_global_interrupt() in entered due to Rx available,
- napi_schedule_prep() is called, and sets NAPIF_STATE_SCHED in state
- Rx fifo interrupts are masked,
- rcar_canfd_global_interrupt() is entered again, this time due to
error interrupt (e.g. due to overflow),
- since scheduled napi poller has not yet executed, condition for calling
napi_schedule_prep() from rcar_canfd_global_interrupt() remains true,
thus napi_schedule_prep() gets called and sets NAPIF_STATE_MISSED flag
in state,
- later, napi poller function rcar_canfd_rx_poll() gets executed, and
calls napi_complete_done(),
- due to NAPIF_STATE_MISSED flag in state, this call does not clear
NAPIF_STATE_SCHED flag from state,
- on return from napi_complete_done(), rcar_canfd_rx_poll() unmasks Rx
interrutps,
- Rx interrupt happens, rcar_canfd_global_interrupt() gets called
and calls napi_schedule_prep(),
- since NAPIF_STATE_SCHED is set in state at this time, this call
returns false,
- due to that false return, rcar_canfd_global_interrupt() returns
without masking Rx interrupt
- and this results into IRQ storm: unmasked Rx interrupt happens again
and again is misprocessed in the same way.
This patch fixes that scenario by unmasking Rx interrupts only when
napi_complete_done() returns true, which means it has cleared
NAPIF_STATE_SCHED in state.
Signed-off-by: Nikita Yushchenko
---
drivers/net/can/rcar/rcar_canfd.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/can/rcar/rcar_canfd.c
b/drivers/net/can/rcar/rcar_canfd.c
index 05410008aa6b..de34a4b82d4a 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1508,10 +1508,11 @@ static int rcar_canfd_rx_poll(struct napi_struct *napi,
int quota)
/* All packets processed */
if (num_pkts < quota) {
- napi_complete_done(napi, num_pkts);
- /* Enable Rx FIFO interrupts */
- rcar_canfd_set_bit(priv->base, RCANFD_RFCC(ridx),
- RCANFD_RFCC_RFIE);
+ if (napi_complete_done(napi, num_pkts)) {
+ /* Enable Rx FIFO interrupts */
+ rcar_canfd_set_bit(priv->base, RCANFD_RFCC(ridx),
+ RCANFD_RFCC_RFIE);
+ }
}
return num_pkts;
}
--
2.11.0
Re: [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
>> Kernel currently does, but it is caught in >> mv88e6xxx_port_check_hw_vlan() and returns -ENOTSUPP from there. > > But VID 0 has a special meaning for the kernel, it means the port's private > database (when it is isolated, non-bridged), it is not meant to be programmed > in the switch. That's why I would've put that knowledge into the DSA layer, > which job is to translate the kernel operations to the (dumb) DSA drivers. > > I hope I'm seeing things correctly here. I'm ok with either solution.
Re: [PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
31.05.2019 17:37, Andrew Lunn wrote: >> I'm not sure that I like the semantic of it, because the driver can actually >> support VID 0 per-se, only the kernel does not use VLAN 0. Thus I would avoid >> calling the port_vlan_del() ops for VID 0, directly into the upper DSA layer. >> >> Florian, Andrew, wouldn't the following patch be more adequate? >> >> diff --git a/net/dsa/slave.c b/net/dsa/slave.c >> index 1e2ae9d59b88..80f228258a92 100644 >> --- a/net/dsa/slave.c >> +++ b/net/dsa/slave.c >> @@ -1063,6 +1063,10 @@ static int dsa_slave_vlan_rx_kill_vid(struct >> net_device *dev, __be16 proto, >> struct bridge_vlan_info info; >> int ret; >> >> + /* VID 0 has a special meaning and is never programmed in >> hardware */ >> + if (!vid) >> + return 0; >> + >> /* Check for a possible bridge VLAN entry now since there is no >> * need to emulate the switchdev prepare + commit phase. >> */ > > Hi Vivien > > If we put this in rx_kill_vid, we should probably have something > similar in rx_add_vid, just in case the kernel does start using VID 0. Kernel currently does, but it is caught in mv88e6xxx_port_check_hw_vlan() and returns -ENOTSUPP from there.
[PATCH] media: avoid skipping MEDIA_PAD_FL_MUST_CONNECT logic
In the current code, __media_pipeline_start() skips check of
MEDIA_PAD_FL_MUST_CONNECT logic for entity not providing link_validate()
op.
Fix that by checking for existence of link_validate() at different code
location.
Signed-off-by: Nikita Yushchenko
---
drivers/media/mc/mc-entity.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index a998a2e0ea1d..8b4912be30d1 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -449,9 +449,6 @@ __must_check int __media_pipeline_start(struct media_entity
*entity,
if (entity->stream_count > 1)
continue;
- if (!entity->ops || !entity->ops->link_validate)
- continue;
-
bitmap_zero(active, entity->num_pads);
bitmap_fill(has_no_links, entity->num_pads);
@@ -479,6 +476,9 @@ __must_check int __media_pipeline_start(struct media_entity
*entity,
!(link->flags & MEDIA_LNK_FL_ENABLED))
continue;
+ if (!entity->ops || !entity->ops->link_validate)
+ continue;
+
ret = entity->ops->link_validate(link);
if (ret < 0 && ret != -ENOIOCTLCMD) {
dev_dbg(entity->graph_obj.mdev->dev,
--
2.11.0
[PATCH] net: phy: support C45 phys in SIOCGMIIREG/SIOCSMIIREG ioctls
This change allows phytool [1] and similar tools to read and write C45 phy
registers from userspace.
This is useful for debugging and for porting vendor phy diagnostics tools.
[1] https://github.com/wkz/phytool
Signed-off-by: Nikita Yushchenko
---
drivers/net/phy/phy.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e8885429293a..3d991958bde0 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -407,6 +407,7 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq
*ifr, int cmd)
struct mii_ioctl_data *mii_data = if_mii(ifr);
u16 val = mii_data->val_in;
bool change_autoneg = false;
+ int ret;
switch (cmd) {
case SIOCGMIIPHY:
@@ -414,12 +415,28 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq
*ifr, int cmd)
/* fall through */
case SIOCGMIIREG:
+ if (mdio_phy_id_is_c45(mii_data->phy_id)) {
+ ret = phy_read_mmd(phydev,
+ mdio_phy_id_devad(mii_data->phy_id),
+ mii_data->reg_num);
+ if (ret < 0)
+ return ret;
+ mii_data->val_out = ret;
+ return 0;
+ }
mii_data->val_out = mdiobus_read(phydev->mdio.bus,
mii_data->phy_id,
mii_data->reg_num);
return 0;
case SIOCSMIIREG:
+ if (mdio_phy_id_is_c45(mii_data->phy_id)) {
+ ret = phy_write_mmd(phydev,
+ mdio_phy_id_devad(mii_data->phy_id),
+ mii_data->reg_num,
+ mii_data->val_in);
+ return ret;
+ }
if (mii_data->phy_id == phydev->mdio.addr) {
switch (mii_data->reg_num) {
case MII_BMCR:
--
2.11.0
[PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
When non-bridged, non-vlan'ed mv88e6xxx port is moving down, error message is logged: failed to kill vid 0081/0 for device eth_cu_1000_4 This is caused by call from __vlan_vid_del() with vin set to zero, over call chain this results into _mv88e6xxx_port_vlan_del() called with vid=0, and mv88e6xxx_vtu_get() called from there returns -EINVAL. On symmetric path moving port up, call goes through mv88e6xxx_port_vlan_prepare() that calls mv88e6xxx_port_check_hw_vlan() that returns -EOPNOTSUPP for zero vid. This patch changes mv88e6xxx_vtu_get() to also return -EOPNOTSUPP for zero vid, then this error code is explicitly cleared in dsa_slave_vlan_rx_kill_vid() and error message is no longer logged. Signed-off-by: Nikita Yushchenko --- drivers/net/dsa/mv88e6xxx/chip.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 28414db979b0..6b77fde5f0e4 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -1392,7 +1392,7 @@ static int mv88e6xxx_vtu_get(struct mv88e6xxx_chip *chip, u16 vid, int err; if (!vid) - return -EINVAL; + return -EOPNOTSUPP; entry->vid = vid - 1; entry->valid = false; -- 2.11.0
[PATCH] net: dsa: mv88e6xxx: avoid error message on remove from VLAN 0
When non-bridged, non-vlan'ed mv88e6xxx port is moving down, error message is logged: failed to kill vid 0081/0 for device eth_cu_1000_4 This is caused by call from __vlan_vid_del() with vin set to zero, over call chain this results into _mv88e6xxx_port_vlan_del() called with vid=0, and mv88e6xxx_vtu_get() called from there returns -EINVAL. On symmetric path moving port up, call goes through mv88e6xxx_port_vlan_prepare() that calls mv88e6xxx_port_check_hw_vlan() that returns -EOPNOTSUPP for zero vid. This patch changes mv88e6xxx_vtu_get() to also return -EOPNOTSUPP for zero vid, then this error code is explicitly cleared in dsa_slave_vlan_rx_kill_vid() and error message is no longer logged. --- drivers/net/dsa/mv88e6xxx/chip.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 28414db979b0..6b77fde5f0e4 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -1392,7 +1392,7 @@ static int mv88e6xxx_vtu_get(struct mv88e6xxx_chip *chip, u16 vid, int err; if (!vid) - return -EINVAL; + return -EOPNOTSUPP; entry->vid = vid - 1; entry->valid = false; -- 2.11.0
Re: [1/3] ARM: dts: imx51-zii-common: create common include dtsi
> There are three boards that share that configuration almost to a T, > with the only difference is the particular GPIOs used. Putting it into > a common file avoids repeating the boilerplate and makes it explicit > to the reader that those settings are shared. I'd agree if that boilerplate was 100 lines. But here it is small, and mostly containing lines that are required for any i2c-gpio definition. It does not any value of itself. Saving 5 lines at cost of loose of integrity is not something I agree with. > There are at least two boards that use that UART2 as is. Same as above > this was done to reduce boilerplate. Here have choice between two logical blocks - definitions of uart2 in two boards that use them, and two logical blocks - definition in dtsi and undo in board that does not use it. You trade a couple of saved dts lines against keeping things consistent. Nikita P.S. In case of these zii boards I doubt that dtsi worths at all. Despite of all being imx51 boards from ZII, these boards don't seem to have large common logical blocks. Perhaos RDU1 and babbage have more in common - so what, create a dtsi for them?
Re: [1/3] ARM: dts: imx51-zii-common: create common include dtsi
> There are three boards that share that configuration almost to a T, > with the only difference is the particular GPIOs used. Putting it into > a common file avoids repeating the boilerplate and makes it explicit > to the reader that those settings are shared. I'd agree if that boilerplate was 100 lines. But here it is small, and mostly containing lines that are required for any i2c-gpio definition. It does not any value of itself. Saving 5 lines at cost of loose of integrity is not something I agree with. > There are at least two boards that use that UART2 as is. Same as above > this was done to reduce boilerplate. Here have choice between two logical blocks - definitions of uart2 in two boards that use them, and two logical blocks - definition in dtsi and undo in board that does not use it. You trade a couple of saved dts lines against keeping things consistent. Nikita P.S. In case of these zii boards I doubt that dtsi worths at all. Despite of all being imx51 boards from ZII, these boards don't seem to have large common logical blocks. Perhaos RDU1 and babbage have more in common - so what, create a dtsi for them?
Re: [1/3] ARM: dts: imx51-zii-common: create common include dtsi
> + i2c_gpio: i2c-gpio {
> + compatible = "i2c-gpio";
> + pinctrl-names = "default";
> + pinctrl-0 = <_swi2c>;
> + i2c-gpio,delay-us = <50>;
> + status = "okay";
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
You add i2c-gpio node to dtsi file without defining gpios, with reference
to pinctrl not defined inside your dtsi file or it's includes, and without
any usage inside dtsi file.
Saving several text lines that way is a bad idea. Please move it to where
it is fully defined and used.
> +_vbus {
> + regulator-always-on;
usb_vbus is regilator-fixed, what for is this?
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_uart2>;
> + status = "okay";
> +};
In your further patches you include this and then revert by marking as
disabled. Better to enable it in dts for boards that have it.
Same with ecspi2, ipu and maybe more.
> - flash@1 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> - compatible = "atmel,at45db642d", "atmel,at45",
> "atmel,dataflash";
> - spi-max-frequency = <2500>;
> - reg = <1>;
> - };
> + flash@1 {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "atmel,at45", "atmel,dataflash";
> + spi-max-frequency = <2500>;
> + reg = <1>;
> + };
Lost a compatible key?
> - sysled0@3 {
> - reg = <3>;
> - label = "system:green:status";
> - linux,default-trigger = "default-on";
> - };
> + sysled3: led3@3 {
> + reg = <3>;
> + label = "system:red:power";
> + linux,default-trigger = "default-on";
> + };
> + {
> + label = "system:green:status";
What for this label games?
Maybe just define things for boards that use it?
Re: [1/3] ARM: dts: imx51-zii-common: create common include dtsi
> + i2c_gpio: i2c-gpio {
> + compatible = "i2c-gpio";
> + pinctrl-names = "default";
> + pinctrl-0 = <_swi2c>;
> + i2c-gpio,delay-us = <50>;
> + status = "okay";
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
You add i2c-gpio node to dtsi file without defining gpios, with reference
to pinctrl not defined inside your dtsi file or it's includes, and without
any usage inside dtsi file.
Saving several text lines that way is a bad idea. Please move it to where
it is fully defined and used.
> +_vbus {
> + regulator-always-on;
usb_vbus is regilator-fixed, what for is this?
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_uart2>;
> + status = "okay";
> +};
In your further patches you include this and then revert by marking as
disabled. Better to enable it in dts for boards that have it.
Same with ecspi2, ipu and maybe more.
> - flash@1 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> - compatible = "atmel,at45db642d", "atmel,at45",
> "atmel,dataflash";
> - spi-max-frequency = <2500>;
> - reg = <1>;
> - };
> + flash@1 {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "atmel,at45", "atmel,dataflash";
> + spi-max-frequency = <2500>;
> + reg = <1>;
> + };
Lost a compatible key?
> - sysled0@3 {
> - reg = <3>;
> - label = "system:green:status";
> - linux,default-trigger = "default-on";
> - };
> + sysled3: led3@3 {
> + reg = <3>;
> + label = "system:red:power";
> + linux,default-trigger = "default-on";
> + };
> + {
> + label = "system:green:status";
What for this label games?
Maybe just define things for boards that use it?
Re: [PATCH v2] ARM: dts: imx51-zii-rdu1: Make sure SD1_WP is low
> Make sure that MX51_PAD_GPIO1_1 does not remain configure as > ALT0/SD1_WP (it is out of reset). This is needed because of external > pull-up resistor attached to that pad that, when left unchanged, will > drive SD1_WP high preventing eSDHC1/eMMC from working correctly. > > To fix that add a pinmux configuration line configureing the pad to > function as a GPIO. While we are at it, add a corresponding > output-high GPIO hog in an effort to minimize current consumption. > > Cc: Nikita Yushchenko > Cc: Shawn Guo > Cc: Fabio Estevam > Cc: Lucas Stach > Cc: Chris Healy > Cc: Rob Herring > Cc: linux-arm-ker...@lists.infradead.org > Cc: devicet...@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Andrey Smirnov Tested-By: Nikita Yushchenko With this patch, eMMC works correctly. Nikita
Re: [PATCH v2] ARM: dts: imx51-zii-rdu1: Make sure SD1_WP is low
> Make sure that MX51_PAD_GPIO1_1 does not remain configure as > ALT0/SD1_WP (it is out of reset). This is needed because of external > pull-up resistor attached to that pad that, when left unchanged, will > drive SD1_WP high preventing eSDHC1/eMMC from working correctly. > > To fix that add a pinmux configuration line configureing the pad to > function as a GPIO. While we are at it, add a corresponding > output-high GPIO hog in an effort to minimize current consumption. > > Cc: Nikita Yushchenko > Cc: Shawn Guo > Cc: Fabio Estevam > Cc: Lucas Stach > Cc: Chris Healy > Cc: Rob Herring > Cc: linux-arm-ker...@lists.infradead.org > Cc: devicet...@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Andrey Smirnov Tested-By: Nikita Yushchenko With this patch, eMMC works correctly. Nikita
[PATCH] ARM: dts: imx51-zii-rdu1: add rave-sp subdevices
This adds rave-sp powerbutton and backlight devices to RDU1 device tree.
Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com>
---
arch/arm/boot/dts/imx51-zii-rdu1.dts | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/imx51-zii-rdu1.dts
b/arch/arm/boot/dts/imx51-zii-rdu1.dts
index 0c99ac04ad08..98cc107098e0 100644
--- a/arch/arm/boot/dts/imx51-zii-rdu1.dts
+++ b/arch/arm/boot/dts/imx51-zii-rdu1.dts
@@ -581,6 +581,14 @@
watchdog {
compatible = "zii,rave-sp-watchdog";
};
+
+ backlight {
+ compatible = "zii,rave-sp-backlight";
+ };
+
+ pwrbutton {
+ compatible = "zii,rave-sp-pwrbutton";
+ };
};
};
--
2.11.0
[PATCH] ARM: dts: imx51-zii-rdu1: add rave-sp subdevices
This adds rave-sp powerbutton and backlight devices to RDU1 device tree.
Signed-off-by: Nikita Yushchenko
---
arch/arm/boot/dts/imx51-zii-rdu1.dts | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/imx51-zii-rdu1.dts
b/arch/arm/boot/dts/imx51-zii-rdu1.dts
index 0c99ac04ad08..98cc107098e0 100644
--- a/arch/arm/boot/dts/imx51-zii-rdu1.dts
+++ b/arch/arm/boot/dts/imx51-zii-rdu1.dts
@@ -581,6 +581,14 @@
watchdog {
compatible = "zii,rave-sp-watchdog";
};
+
+ backlight {
+ compatible = "zii,rave-sp-backlight";
+ };
+
+ pwrbutton {
+ compatible = "zii,rave-sp-pwrbutton";
+ };
};
};
--
2.11.0
[PATCH] ARM: dts: imx51-zii-rdu1: cleanup eMMC node
On RDU1, sdhc1 is used for eMMC, and that is 3.3V only. Thus configure device node not to probe it as SD/SDIO and not try 1.8V. Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com> --- arch/arm/boot/dts/imx51-zii-rdu1.dts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/arm/boot/dts/imx51-zii-rdu1.dts b/arch/arm/boot/dts/imx51-zii-rdu1.dts index 0c99ac04ad08..ad2a7c9b203e 100644 --- a/arch/arm/boot/dts/imx51-zii-rdu1.dts +++ b/arch/arm/boot/dts/imx51-zii-rdu1.dts @@ -462,7 +462,10 @@ pinctrl-names = "default"; pinctrl-0 = <_esdhc1>; bus-width = <4>; + no-1-8-v; non-removable; + no-sdio; + no-sd; status = "okay"; }; -- 2.11.0
[PATCH] ARM: dts: imx51-zii-rdu1: cleanup eMMC node
On RDU1, sdhc1 is used for eMMC, and that is 3.3V only. Thus configure device node not to probe it as SD/SDIO and not try 1.8V. Signed-off-by: Nikita Yushchenko --- arch/arm/boot/dts/imx51-zii-rdu1.dts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/arm/boot/dts/imx51-zii-rdu1.dts b/arch/arm/boot/dts/imx51-zii-rdu1.dts index 0c99ac04ad08..ad2a7c9b203e 100644 --- a/arch/arm/boot/dts/imx51-zii-rdu1.dts +++ b/arch/arm/boot/dts/imx51-zii-rdu1.dts @@ -462,7 +462,10 @@ pinctrl-names = "default"; pinctrl-0 = <_esdhc1>; bus-width = <4>; + no-1-8-v; non-removable; + no-sdio; + no-sd; status = "okay"; }; -- 2.11.0
[PATCH] ARM: dts: vf610-zii-dev: enable vf610 builtin temp sensor
Vybrid has single internal temperature sensor connected to both internal
ADC modules.
vf610-zii-dev already has ADC0 enabled. Now, to get temperature sensor
captured by iio_hwmon driver, need to configure iio_hwmon node to use
that ADC.
Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com>
---
arch/arm/boot/dts/vf610-zii-dev.dtsi | 4
arch/arm/boot/dts/vfxxx.dtsi | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/vf610-zii-dev.dtsi
b/arch/arm/boot/dts/vf610-zii-dev.dtsi
index 4890b8a5aa44..5ae5abfe1d55 100644
--- a/arch/arm/boot/dts/vf610-zii-dev.dtsi
+++ b/arch/arm/boot/dts/vf610-zii-dev.dtsi
@@ -222,6 +222,10 @@
status = "okay";
};
+ {
+ io-channels = < 16>;
+};
+
{
pinctrl_adc0_ad5: adc0ad5grp {
fsl,pins = <
diff --git a/arch/arm/boot/dts/vfxxx.dtsi b/arch/arm/boot/dts/vfxxx.dtsi
index c3f09b737924..d392794d9c13 100644
--- a/arch/arm/boot/dts/vfxxx.dtsi
+++ b/arch/arm/boot/dts/vfxxx.dtsi
@@ -84,7 +84,7 @@
mask = <0x1000>;
};
- iio-hwmon {
+ tempsensor: iio-hwmon {
compatible = "iio-hwmon";
io-channels = < 16>, < 16>;
};
--
2.11.0
[PATCH] ARM: dts: vf610-zii-dev: enable vf610 builtin temp sensor
Vybrid has single internal temperature sensor connected to both internal
ADC modules.
vf610-zii-dev already has ADC0 enabled. Now, to get temperature sensor
captured by iio_hwmon driver, need to configure iio_hwmon node to use
that ADC.
Signed-off-by: Nikita Yushchenko
---
arch/arm/boot/dts/vf610-zii-dev.dtsi | 4
arch/arm/boot/dts/vfxxx.dtsi | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/vf610-zii-dev.dtsi
b/arch/arm/boot/dts/vf610-zii-dev.dtsi
index 4890b8a5aa44..5ae5abfe1d55 100644
--- a/arch/arm/boot/dts/vf610-zii-dev.dtsi
+++ b/arch/arm/boot/dts/vf610-zii-dev.dtsi
@@ -222,6 +222,10 @@
status = "okay";
};
+ {
+ io-channels = < 16>;
+};
+
{
pinctrl_adc0_ad5: adc0ad5grp {
fsl,pins = <
diff --git a/arch/arm/boot/dts/vfxxx.dtsi b/arch/arm/boot/dts/vfxxx.dtsi
index c3f09b737924..d392794d9c13 100644
--- a/arch/arm/boot/dts/vfxxx.dtsi
+++ b/arch/arm/boot/dts/vfxxx.dtsi
@@ -84,7 +84,7 @@
mask = <0x1000>;
};
- iio-hwmon {
+ tempsensor: iio-hwmon {
compatible = "iio-hwmon";
io-channels = < 16>, < 16>;
};
--
2.11.0
[PATCH] ARM: dts: imx51-zii-rdu1: limit usbh1 to full-speed
On RDU1, imx51 usbh1 interface is either not used, or used via external block that breaks USB2 signalling. To keep things working if high-speed device gets connected to that block, use ChipIdea feature to limit port to full speed. Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com> --- arch/arm/boot/dts/imx51-zii-rdu1.dts | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm/boot/dts/imx51-zii-rdu1.dts b/arch/arm/boot/dts/imx51-zii-rdu1.dts index 36a63ae52e48..c26f053f29b0 100644 --- a/arch/arm/boot/dts/imx51-zii-rdu1.dts +++ b/arch/arm/boot/dts/imx51-zii-rdu1.dts @@ -603,6 +603,7 @@ phy_type = "ulpi"; fsl,usbphy = <>; disable-over-current; + maximum-speed = "full-speed"; vbus-supply = <_5p0v_main>; status = "okay"; }; -- 2.11.0
[PATCH] ARM: dts: imx51-zii-rdu1: limit usbh1 to full-speed
On RDU1, imx51 usbh1 interface is either not used, or used via external block that breaks USB2 signalling. To keep things working if high-speed device gets connected to that block, use ChipIdea feature to limit port to full speed. Signed-off-by: Nikita Yushchenko --- arch/arm/boot/dts/imx51-zii-rdu1.dts | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm/boot/dts/imx51-zii-rdu1.dts b/arch/arm/boot/dts/imx51-zii-rdu1.dts index 36a63ae52e48..c26f053f29b0 100644 --- a/arch/arm/boot/dts/imx51-zii-rdu1.dts +++ b/arch/arm/boot/dts/imx51-zii-rdu1.dts @@ -603,6 +603,7 @@ phy_type = "ulpi"; fsl,usbphy = <>; disable-over-current; + maximum-speed = "full-speed"; vbus-supply = <_5p0v_main>; status = "okay"; }; -- 2.11.0
[PATCH] ARM: dts: imx51-zii-rdu1: fix touchscreen bindings
This fixes errors in RDU1 device tree that cause touch screens not
working.
Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com>
---
arch/arm/boot/dts/imx51-zii-rdu1.dts | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/imx51-zii-rdu1.dts
b/arch/arm/boot/dts/imx51-zii-rdu1.dts
index 0c99ac04ad08..6464f2560e06 100644
--- a/arch/arm/boot/dts/imx51-zii-rdu1.dts
+++ b/arch/arm/boot/dts/imx51-zii-rdu1.dts
@@ -523,7 +523,7 @@
};
touchscreen@20 {
- compatible = "syna,rmi4_i2c";
+ compatible = "syna,rmi4-i2c";
reg = <0x20>;
pinctrl-names = "default";
pinctrl-0 = <_ts>;
@@ -541,8 +541,8 @@
rmi4-f11@11 {
reg = <0x11>;
- touch-inverted-y;
- touch-swapped-x-y;
+ touchscreen-inverted-y;
+ touchscreen-swapped-x-y;
syna,sensor-type = <1>;
};
};
--
2.11.0
[PATCH] ARM: dts: imx51-zii-rdu1: fix touchscreen bindings
This fixes errors in RDU1 device tree that cause touch screens not
working.
Signed-off-by: Nikita Yushchenko
---
arch/arm/boot/dts/imx51-zii-rdu1.dts | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/imx51-zii-rdu1.dts
b/arch/arm/boot/dts/imx51-zii-rdu1.dts
index 0c99ac04ad08..6464f2560e06 100644
--- a/arch/arm/boot/dts/imx51-zii-rdu1.dts
+++ b/arch/arm/boot/dts/imx51-zii-rdu1.dts
@@ -523,7 +523,7 @@
};
touchscreen@20 {
- compatible = "syna,rmi4_i2c";
+ compatible = "syna,rmi4-i2c";
reg = <0x20>;
pinctrl-names = "default";
pinctrl-0 = <_ts>;
@@ -541,8 +541,8 @@
rmi4-f11@11 {
reg = <0x11>;
- touch-inverted-y;
- touch-swapped-x-y;
+ touchscreen-inverted-y;
+ touchscreen-swapped-x-y;
syna,sensor-type = <1>;
};
};
--
2.11.0
[PATCH] ASoC: rsnd: set pm_ops in hibernate-compatible way
Use SET_SYSTEM_SLEEP_PM_OPS() macro instead of direct assignment to
.suspend and .resume fields.
This makes driver working after restore from hibernation.
Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com>
---
sound/soc/sh/rcar/core.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c
index 64d5ecb86528..dabf9a35c25c 100644
--- a/sound/soc/sh/rcar/core.c
+++ b/sound/soc/sh/rcar/core.c
@@ -1560,8 +1560,7 @@ static int rsnd_resume(struct device *dev)
}
static const struct dev_pm_ops rsnd_pm_ops = {
- .suspend= rsnd_suspend,
- .resume = rsnd_resume,
+ SET_SYSTEM_SLEEP_PM_OPS(rsnd_suspend, rsnd_resume)
};
static struct platform_driver rsnd_driver = {
--
2.11.0
[PATCH] ASoC: rsnd: set pm_ops in hibernate-compatible way
Use SET_SYSTEM_SLEEP_PM_OPS() macro instead of direct assignment to
.suspend and .resume fields.
This makes driver working after restore from hibernation.
Signed-off-by: Nikita Yushchenko
---
sound/soc/sh/rcar/core.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c
index 64d5ecb86528..dabf9a35c25c 100644
--- a/sound/soc/sh/rcar/core.c
+++ b/sound/soc/sh/rcar/core.c
@@ -1560,8 +1560,7 @@ static int rsnd_resume(struct device *dev)
}
static const struct dev_pm_ops rsnd_pm_ops = {
- .suspend= rsnd_suspend,
- .resume = rsnd_resume,
+ SET_SYSTEM_SLEEP_PM_OPS(rsnd_suspend, rsnd_resume)
};
static struct platform_driver rsnd_driver = {
--
2.11.0
[PATCH] clk: cs2000: set pm_ops in hibernate-compatible way
Use SET_LATE_SYSTEM_SLEEP_PM_OPS() macro instead of direct assignment to
.resume_early field.
This fixes initialization of CS2000 in restore from hibernation in case
of kernel used to load image did not initialize CS2000 while kernel
being restored had CS2000 initialized.
Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com>
---
drivers/clk/clk-cs2000-cp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/clk-cs2000-cp.c b/drivers/clk/clk-cs2000-cp.c
index e8ea81c30f0c..c58019750b7e 100644
--- a/drivers/clk/clk-cs2000-cp.c
+++ b/drivers/clk/clk-cs2000-cp.c
@@ -549,7 +549,7 @@ static int cs2000_resume(struct device *dev)
}
static const struct dev_pm_ops cs2000_pm_ops = {
- .resume_early = cs2000_resume,
+ SET_LATE_SYSTEM_SLEEP_PM_OPS(NULL, cs2000_resume)
};
static struct i2c_driver cs2000_driver = {
--
2.11.0
[PATCH] clk: cs2000: set pm_ops in hibernate-compatible way
Use SET_LATE_SYSTEM_SLEEP_PM_OPS() macro instead of direct assignment to
.resume_early field.
This fixes initialization of CS2000 in restore from hibernation in case
of kernel used to load image did not initialize CS2000 while kernel
being restored had CS2000 initialized.
Signed-off-by: Nikita Yushchenko
---
drivers/clk/clk-cs2000-cp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/clk-cs2000-cp.c b/drivers/clk/clk-cs2000-cp.c
index e8ea81c30f0c..c58019750b7e 100644
--- a/drivers/clk/clk-cs2000-cp.c
+++ b/drivers/clk/clk-cs2000-cp.c
@@ -549,7 +549,7 @@ static int cs2000_resume(struct device *dev)
}
static const struct dev_pm_ops cs2000_pm_ops = {
- .resume_early = cs2000_resume,
+ SET_LATE_SYSTEM_SLEEP_PM_OPS(NULL, cs2000_resume)
};
static struct i2c_driver cs2000_driver = {
--
2.11.0
cancel_work_sync() can cause priority invertion
Hi For those who care about linux RT behavior: while analyzing traces, just found priority inversion caused by RT task calling cancel_work_sync(), while work item in question is executing in non-RT kworker that was preempted for significant time. WBR, Nikita Yushchenko
cancel_work_sync() can cause priority invertion
Hi For those who care about linux RT behavior: while analyzing traces, just found priority inversion caused by RT task calling cancel_work_sync(), while work item in question is executing in non-RT kworker that was preempted for significant time. WBR, Nikita Yushchenko
Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory Processor
>> I think that trying to make this generic is purely synthetic. This >> information is board-specific per it's nature, it comes from what board >> is designed for, different boards have quite different sets of possible >> reset reasons. What is needed is - pass this board-specific information >> to board-specific user space. >> >> What's proper API for that, if not a sysfs attribute? > > Please go through the thread. > > Sysfs attribute is okay, but: > > 1) it should probably be a string > > 2) it should certainly be superset of all the reasons > > 3) it should be in generic place, say /sys/power/reset_reason > > 4) it should be documented what each state means What I'm concerned here is that a requirement appears for kernel driver to keep and maintain knowledge of what all that codes mean. For me, it looks like information locality breakage. Information in question is application-specific, it definitely changes from board to board and probably will change for particular board over time. It's desirable to keep this information application-private, and have kernel only passed it from hardware to application, unmodified and uninterpreted. Requiring kernel driver to interpret application-specific information only increases complexity (i.e. any changes will have to be maintained in two places - in driver and in application). So question is - is there any proper API to communicate application-private information from hardware through kernel to userspace without any in-kernel interpretation?
Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory Processor
>> I think that trying to make this generic is purely synthetic. This >> information is board-specific per it's nature, it comes from what board >> is designed for, different boards have quite different sets of possible >> reset reasons. What is needed is - pass this board-specific information >> to board-specific user space. >> >> What's proper API for that, if not a sysfs attribute? > > Please go through the thread. > > Sysfs attribute is okay, but: > > 1) it should probably be a string > > 2) it should certainly be superset of all the reasons > > 3) it should be in generic place, say /sys/power/reset_reason > > 4) it should be documented what each state means What I'm concerned here is that a requirement appears for kernel driver to keep and maintain knowledge of what all that codes mean. For me, it looks like information locality breakage. Information in question is application-specific, it definitely changes from board to board and probably will change for particular board over time. It's desirable to keep this information application-private, and have kernel only passed it from hardware to application, unmodified and uninterpreted. Requiring kernel driver to interpret application-specific information only increases complexity (i.e. any changes will have to be maintained in two places - in driver and in application). So question is - is there any proper API to communicate application-private information from hardware through kernel to userspace without any in-kernel interpretation?
Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory Processor
30.08.2017 23:38, Pavel Machek wrote: > Hi! > + * 9 -> Illegal Trap + * 10 -> Unknown + * 11 -> Crew Panel Requested >>> >>> Anyway... If you move management chip to .. I don't know, i2c, the >>> path would change. Also it would be different path on N900. Userland >>> should not have to deal with this. >>> >>> And... this should really be string, as the list will need to grow on >>> different hardware. >> >> I think we have a misunderstanding, with this part of the patch set I >> am not trying to propose a generic ABI that would be useful for any >> other driver but this one. Hence the lack of concern for different > > Yes, but sorry, that's no-go. Kernel should hide differences between > different machiens, and it should be rather easy in this case. There is an interest to have reset reason exported on other ZII hardware as well. I think that trying to make this generic is purely synthetic. This information is board-specific per it's nature, it comes from what board is designed for, different boards have quite different sets of possible reset reasons. What is needed is - pass this board-specific information to board-specific user space. What's proper API for that, if not a sysfs attribute?
Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory Processor
30.08.2017 23:38, Pavel Machek wrote: > Hi! > + * 9 -> Illegal Trap + * 10 -> Unknown + * 11 -> Crew Panel Requested >>> >>> Anyway... If you move management chip to .. I don't know, i2c, the >>> path would change. Also it would be different path on N900. Userland >>> should not have to deal with this. >>> >>> And... this should really be string, as the list will need to grow on >>> different hardware. >> >> I think we have a misunderstanding, with this part of the patch set I >> am not trying to propose a generic ABI that would be useful for any >> other driver but this one. Hence the lack of concern for different > > Yes, but sorry, that's no-go. Kernel should hide differences between > different machiens, and it should be rather easy in this case. There is an interest to have reset reason exported on other ZII hardware as well. I think that trying to make this generic is purely synthetic. This information is board-specific per it's nature, it comes from what board is designed for, different boards have quite different sets of possible reset reasons. What is needed is - pass this board-specific information to board-specific user space. What's proper API for that, if not a sysfs attribute?
[PATCH v2] rtc: ds1307: add basic support for ds1341 chip
This adds support for reading and writing date/time from/to ds1341 chip.
ds1341 chip has other features - alarms, input clock (can be used instead
of intercal oscillator for better accuracy), output clock ("square wave
generation"). However, not all of that is available at the same time.
Same chip pins, CLKIN/nINTA and SQW/nINTB, can be used either for
input/output clocks, or for alarm interrupts. Role of these pins on
particular board depends on hardware wiring.
We can add device tree properties that describe if each of pins is wired
as clock, or as interrupt, or left unconnected, and enable support for
corresponding functionality based on that. But that is cumbersome, requires
hardware for testing, and has to deal with bit enabling/disabling output
clock also affects which pins alarm interrupts are routed to.
Another factor is that there are hardware setups (i.e. ZII RDU2) that
power DS1341 from SuperCap, which makes power saving critical. For such
setups, kernel driver should leave register bits that control mentioned
pins in the state configured by bootloader.
Given all that, it was decided to limit support to "only date/time" for
now. That is enough for common use case. Full (and cumbersome)
implementation can be added later if ever needed.
Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com>
Reviewed-by: Linus Walleij <linus.wall...@linaro.org>
Tested-by: Aleksander Morgado <aleksan...@aleksander.es>
---
drivers/rtc/Kconfig | 10 +-
drivers/rtc/rtc-ds1307.c | 13 +
2 files changed, 18 insertions(+), 5 deletions(-)
===
Changes from v1:
- added information on why support was limited to date/time only to commit
message, as suggested by Linus Walleij
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 72419ac2c52a..db63592a0f57 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -227,14 +227,14 @@ config RTC_DRV_AS3722
will be called rtc-as3722.
config RTC_DRV_DS1307
- tristate "Dallas/Maxim DS1307/37/38/39/40, ST M41T00, EPSON RX-8025,
ISL12057"
+ tristate "Dallas/Maxim DS1307/37/38/39/40/41, ST M41T00, EPSON RX-8025,
ISL12057"
help
If you say yes here you get support for various compatible RTC
chips (often with battery backup) connected with I2C. This driver
- should handle DS1307, DS1337, DS1338, DS1339, DS1340, ST M41T00,
- EPSON RX-8025, Intersil ISL12057 and probably other chips. In some
- cases the RTC must already have been initialized (by manufacturing or
- a bootloader).
+ should handle DS1307, DS1337, DS1338, DS1339, DS1340, DS1341,
+ ST M41T00, EPSON RX-8025, Intersil ISL12057 and probably other chips.
+ In some cases the RTC must already have been initialized (by
+ manufacturing or a bootloader).
The first seven registers on these chips hold an RTC, and other
registers may add features such as NVRAM, a trickle charger for
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 4fac49e55d47..1d11f8000f8a 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -39,6 +39,7 @@ enum ds_type {
ds_1338,
ds_1339,
ds_1340,
+ ds_1341,
ds_1388,
ds_3231,
m41t0,
@@ -179,6 +180,10 @@ static struct chip_desc chips[last_ds_type] = {
.century_bit= DS1340_BIT_CENTURY,
.trickle_charger_reg = 0x08,
},
+ [ds_1341] = {
+ .century_reg= DS1307_REG_MONTH,
+ .century_bit= DS1337_BIT_CENTURY,
+ },
[ds_1388] = {
.trickle_charger_reg = 0x0a,
},
@@ -209,6 +214,7 @@ static const struct i2c_device_id ds1307_id[] = {
{ "ds1339", ds_1339 },
{ "ds1388", ds_1388 },
{ "ds1340", ds_1340 },
+ { "ds1341", ds_1341 },
{ "ds3231", ds_3231 },
{ "m41t0", m41t0 },
{ "m41t00", m41t00 },
@@ -253,6 +259,10 @@ static const struct of_device_id ds1307_of_match[] = {
.data = (void *)ds_1340
},
{
+ .compatible = "dallas,ds1341",
+ .data = (void *)ds_1341
+ },
+ {
.compatible = "maxim,ds3231",
.data = (void *)ds_3231
},
@@ -298,6 +308,7 @@ static const struct acpi_device_id ds1307_acpi_ids[] = {
{ .id = "DS1339", .driver_data = ds_1339 },
{ .id = "DS1388", .driver_data = ds_1388 },
{ .id = "DS1340", .driver_data = ds_1340 },
+ { .id = "DS1341", .driver_data = ds_1341 },
{ .id = "DS3231", .driver_data = ds_3231 },
{ .id = "M41T0", .driver_data = m41t0 },
{ .id = "M41T00", .driver_data = m41t00 },
@@ -132
[PATCH v2] rtc: ds1307: add basic support for ds1341 chip
This adds support for reading and writing date/time from/to ds1341 chip.
ds1341 chip has other features - alarms, input clock (can be used instead
of intercal oscillator for better accuracy), output clock ("square wave
generation"). However, not all of that is available at the same time.
Same chip pins, CLKIN/nINTA and SQW/nINTB, can be used either for
input/output clocks, or for alarm interrupts. Role of these pins on
particular board depends on hardware wiring.
We can add device tree properties that describe if each of pins is wired
as clock, or as interrupt, or left unconnected, and enable support for
corresponding functionality based on that. But that is cumbersome, requires
hardware for testing, and has to deal with bit enabling/disabling output
clock also affects which pins alarm interrupts are routed to.
Another factor is that there are hardware setups (i.e. ZII RDU2) that
power DS1341 from SuperCap, which makes power saving critical. For such
setups, kernel driver should leave register bits that control mentioned
pins in the state configured by bootloader.
Given all that, it was decided to limit support to "only date/time" for
now. That is enough for common use case. Full (and cumbersome)
implementation can be added later if ever needed.
Signed-off-by: Nikita Yushchenko
Reviewed-by: Linus Walleij
Tested-by: Aleksander Morgado
---
drivers/rtc/Kconfig | 10 +-
drivers/rtc/rtc-ds1307.c | 13 +
2 files changed, 18 insertions(+), 5 deletions(-)
===
Changes from v1:
- added information on why support was limited to date/time only to commit
message, as suggested by Linus Walleij
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 72419ac2c52a..db63592a0f57 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -227,14 +227,14 @@ config RTC_DRV_AS3722
will be called rtc-as3722.
config RTC_DRV_DS1307
- tristate "Dallas/Maxim DS1307/37/38/39/40, ST M41T00, EPSON RX-8025,
ISL12057"
+ tristate "Dallas/Maxim DS1307/37/38/39/40/41, ST M41T00, EPSON RX-8025,
ISL12057"
help
If you say yes here you get support for various compatible RTC
chips (often with battery backup) connected with I2C. This driver
- should handle DS1307, DS1337, DS1338, DS1339, DS1340, ST M41T00,
- EPSON RX-8025, Intersil ISL12057 and probably other chips. In some
- cases the RTC must already have been initialized (by manufacturing or
- a bootloader).
+ should handle DS1307, DS1337, DS1338, DS1339, DS1340, DS1341,
+ ST M41T00, EPSON RX-8025, Intersil ISL12057 and probably other chips.
+ In some cases the RTC must already have been initialized (by
+ manufacturing or a bootloader).
The first seven registers on these chips hold an RTC, and other
registers may add features such as NVRAM, a trickle charger for
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 4fac49e55d47..1d11f8000f8a 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -39,6 +39,7 @@ enum ds_type {
ds_1338,
ds_1339,
ds_1340,
+ ds_1341,
ds_1388,
ds_3231,
m41t0,
@@ -179,6 +180,10 @@ static struct chip_desc chips[last_ds_type] = {
.century_bit= DS1340_BIT_CENTURY,
.trickle_charger_reg = 0x08,
},
+ [ds_1341] = {
+ .century_reg= DS1307_REG_MONTH,
+ .century_bit= DS1337_BIT_CENTURY,
+ },
[ds_1388] = {
.trickle_charger_reg = 0x0a,
},
@@ -209,6 +214,7 @@ static const struct i2c_device_id ds1307_id[] = {
{ "ds1339", ds_1339 },
{ "ds1388", ds_1388 },
{ "ds1340", ds_1340 },
+ { "ds1341", ds_1341 },
{ "ds3231", ds_3231 },
{ "m41t0", m41t0 },
{ "m41t00", m41t00 },
@@ -253,6 +259,10 @@ static const struct of_device_id ds1307_of_match[] = {
.data = (void *)ds_1340
},
{
+ .compatible = "dallas,ds1341",
+ .data = (void *)ds_1341
+ },
+ {
.compatible = "maxim,ds3231",
.data = (void *)ds_3231
},
@@ -298,6 +308,7 @@ static const struct acpi_device_id ds1307_acpi_ids[] = {
{ .id = "DS1339", .driver_data = ds_1339 },
{ .id = "DS1388", .driver_data = ds_1388 },
{ .id = "DS1340", .driver_data = ds_1340 },
+ { .id = "DS1341", .driver_data = ds_1341 },
{ .id = "DS3231", .driver_data = ds_3231 },
{ .id = "M41T0", .driver_data = m41t0 },
{ .id = "M41T00", .driver_data = m41t00 },
@@ -1323,6 +1334,7 @@ static int ds1307_probe(struct i2c_client *client,
static const intbbsq
[PATCH] rtc: ds1307: add basic support for ds1341 chip
This adds support for reading and writing date/time from/to ds1314 chip.
Other functionality (alarms, inout clock, output clock) is not added
yet, because availability of that depends on chip connections.
Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com>
---
drivers/rtc/Kconfig | 10 +-
drivers/rtc/rtc-ds1307.c | 13 +
2 files changed, 18 insertions(+), 5 deletions(-)
===
DS1341/1342 chips have additional features, namely
- alarms,
- input clock (can be used instead of intercal oscillator for better
accuracy),
- output clock ("square wave generation")
However, not all of that is available at the same time. Same chip pins,
CLKIN/nINTA and SQW/nINTB, can be used either for input/output clocks,
or for alarm interrupts. Role of these pins on particular board depends
on hardware wiring.
We can add device tree properties that describe if each of pins is wired
as clock, or as interrupt, or left unconnected, and enable support for
corresponding functionality based on that. But that requires hardware setups
for testing, and also is somewhat cumbersome. Additional complexity is
caused by bit enabling/disabling output clock also affects which pins alarm
interrupts are routed to.
Another factor is that there are hardware setups (i.e. ZII RDU2) that
power DS1341 from SuperCap, which makes power saving critical. For such
setups, kernel driver should leave register bits that control mentioned
pins in the state configured by bootloader.
Given all that, it was decided to limit support to "only date/time" for
now. That is enough for common use case. Full (and cumbersome) implementation
can be added later if ever needed.
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 72419ac2c52a..db63592a0f57 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -227,14 +227,14 @@ config RTC_DRV_AS3722
will be called rtc-as3722.
config RTC_DRV_DS1307
- tristate "Dallas/Maxim DS1307/37/38/39/40, ST M41T00, EPSON RX-8025,
ISL12057"
+ tristate "Dallas/Maxim DS1307/37/38/39/40/41, ST M41T00, EPSON RX-8025,
ISL12057"
help
If you say yes here you get support for various compatible RTC
chips (often with battery backup) connected with I2C. This driver
- should handle DS1307, DS1337, DS1338, DS1339, DS1340, ST M41T00,
- EPSON RX-8025, Intersil ISL12057 and probably other chips. In some
- cases the RTC must already have been initialized (by manufacturing or
- a bootloader).
+ should handle DS1307, DS1337, DS1338, DS1339, DS1340, DS1341,
+ ST M41T00, EPSON RX-8025, Intersil ISL12057 and probably other chips.
+ In some cases the RTC must already have been initialized (by
+ manufacturing or a bootloader).
The first seven registers on these chips hold an RTC, and other
registers may add features such as NVRAM, a trickle charger for
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 4fac49e55d47..1d11f8000f8a 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -39,6 +39,7 @@ enum ds_type {
ds_1338,
ds_1339,
ds_1340,
+ ds_1341,
ds_1388,
ds_3231,
m41t0,
@@ -179,6 +180,10 @@ static struct chip_desc chips[last_ds_type] = {
.century_bit= DS1340_BIT_CENTURY,
.trickle_charger_reg = 0x08,
},
+ [ds_1341] = {
+ .century_reg= DS1307_REG_MONTH,
+ .century_bit= DS1337_BIT_CENTURY,
+ },
[ds_1388] = {
.trickle_charger_reg = 0x0a,
},
@@ -209,6 +214,7 @@ static const struct i2c_device_id ds1307_id[] = {
{ "ds1339", ds_1339 },
{ "ds1388", ds_1388 },
{ "ds1340", ds_1340 },
+ { "ds1341", ds_1341 },
{ "ds3231", ds_3231 },
{ "m41t0", m41t0 },
{ "m41t00", m41t00 },
@@ -253,6 +259,10 @@ static const struct of_device_id ds1307_of_match[] = {
.data = (void *)ds_1340
},
{
+ .compatible = "dallas,ds1341",
+ .data = (void *)ds_1341
+ },
+ {
.compatible = "maxim,ds3231",
.data = (void *)ds_3231
},
@@ -298,6 +308,7 @@ static const struct acpi_device_id ds1307_acpi_ids[] = {
{ .id = "DS1339", .driver_data = ds_1339 },
{ .id = "DS1388", .driver_data = ds_1388 },
{ .id = "DS1340", .driver_data = ds_1340 },
+ { .id = "DS1341", .driver_data = ds_1341 },
{ .id = "DS3231", .driver_data = ds_3231 },
{ .id = "M41T0", .driver_data = m41t0 },
{ .id = "M41T00", .driver_data = m41t00 },
@@ -1323,6 +1334,7 @@ static int ds1307_probe(stru
[PATCH] rtc: ds1307: add basic support for ds1341 chip
This adds support for reading and writing date/time from/to ds1314 chip.
Other functionality (alarms, inout clock, output clock) is not added
yet, because availability of that depends on chip connections.
Signed-off-by: Nikita Yushchenko
---
drivers/rtc/Kconfig | 10 +-
drivers/rtc/rtc-ds1307.c | 13 +
2 files changed, 18 insertions(+), 5 deletions(-)
===
DS1341/1342 chips have additional features, namely
- alarms,
- input clock (can be used instead of intercal oscillator for better
accuracy),
- output clock ("square wave generation")
However, not all of that is available at the same time. Same chip pins,
CLKIN/nINTA and SQW/nINTB, can be used either for input/output clocks,
or for alarm interrupts. Role of these pins on particular board depends
on hardware wiring.
We can add device tree properties that describe if each of pins is wired
as clock, or as interrupt, or left unconnected, and enable support for
corresponding functionality based on that. But that requires hardware setups
for testing, and also is somewhat cumbersome. Additional complexity is
caused by bit enabling/disabling output clock also affects which pins alarm
interrupts are routed to.
Another factor is that there are hardware setups (i.e. ZII RDU2) that
power DS1341 from SuperCap, which makes power saving critical. For such
setups, kernel driver should leave register bits that control mentioned
pins in the state configured by bootloader.
Given all that, it was decided to limit support to "only date/time" for
now. That is enough for common use case. Full (and cumbersome) implementation
can be added later if ever needed.
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 72419ac2c52a..db63592a0f57 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -227,14 +227,14 @@ config RTC_DRV_AS3722
will be called rtc-as3722.
config RTC_DRV_DS1307
- tristate "Dallas/Maxim DS1307/37/38/39/40, ST M41T00, EPSON RX-8025,
ISL12057"
+ tristate "Dallas/Maxim DS1307/37/38/39/40/41, ST M41T00, EPSON RX-8025,
ISL12057"
help
If you say yes here you get support for various compatible RTC
chips (often with battery backup) connected with I2C. This driver
- should handle DS1307, DS1337, DS1338, DS1339, DS1340, ST M41T00,
- EPSON RX-8025, Intersil ISL12057 and probably other chips. In some
- cases the RTC must already have been initialized (by manufacturing or
- a bootloader).
+ should handle DS1307, DS1337, DS1338, DS1339, DS1340, DS1341,
+ ST M41T00, EPSON RX-8025, Intersil ISL12057 and probably other chips.
+ In some cases the RTC must already have been initialized (by
+ manufacturing or a bootloader).
The first seven registers on these chips hold an RTC, and other
registers may add features such as NVRAM, a trickle charger for
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 4fac49e55d47..1d11f8000f8a 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -39,6 +39,7 @@ enum ds_type {
ds_1338,
ds_1339,
ds_1340,
+ ds_1341,
ds_1388,
ds_3231,
m41t0,
@@ -179,6 +180,10 @@ static struct chip_desc chips[last_ds_type] = {
.century_bit= DS1340_BIT_CENTURY,
.trickle_charger_reg = 0x08,
},
+ [ds_1341] = {
+ .century_reg= DS1307_REG_MONTH,
+ .century_bit= DS1337_BIT_CENTURY,
+ },
[ds_1388] = {
.trickle_charger_reg = 0x0a,
},
@@ -209,6 +214,7 @@ static const struct i2c_device_id ds1307_id[] = {
{ "ds1339", ds_1339 },
{ "ds1388", ds_1388 },
{ "ds1340", ds_1340 },
+ { "ds1341", ds_1341 },
{ "ds3231", ds_3231 },
{ "m41t0", m41t0 },
{ "m41t00", m41t00 },
@@ -253,6 +259,10 @@ static const struct of_device_id ds1307_of_match[] = {
.data = (void *)ds_1340
},
{
+ .compatible = "dallas,ds1341",
+ .data = (void *)ds_1341
+ },
+ {
.compatible = "maxim,ds3231",
.data = (void *)ds_3231
},
@@ -298,6 +308,7 @@ static const struct acpi_device_id ds1307_acpi_ids[] = {
{ .id = "DS1339", .driver_data = ds_1339 },
{ .id = "DS1388", .driver_data = ds_1388 },
{ .id = "DS1340", .driver_data = ds_1340 },
+ { .id = "DS1341", .driver_data = ds_1341 },
{ .id = "DS3231", .driver_data = ds_3231 },
{ .id = "M41T0", .driver_data = m41t0 },
{ .id = "M41T00", .driver_data = m41t00 },
@@ -1323,6 +1334,7 @@ static int ds1307_probe(struct i2c_client *client,
static const int
Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio
>> - Thinking further on this, I realized that for common case signal >> polarity is something defined by chip, and thus this knowledge belongs >> to chip's driver and not to chip user's device tree. Moreover, device >> tree writer could easily be not aware of signal polarity (too many >> datasheets are NDA-closed), thus hello copy-pasting, try-and-check and >> other counterproductive approaches. >> >> - Then Vladimir pointed real-life case with signal inversion by handmade >> level shifter. Although scope of this is likely limited to hw labs, >> support for this is wanted and thus some way to override polarity in >> chip user's dts be available. Still, this should be optional, without >> requiring dts to always define polarity of each gpio. > > I have real hardware that effectively does the equivalent for interrupt > lines coming into the processor from some sensors. Note quite the same, > but shows these things often do turn up as a fix on 'real' hardware. Well no doubt that sometimes there is need to invert polarity for particular use case. But still forcing explicit definition of polarity of each and every gpio usage is a bad way to solve this. > You are correct, this needs some wider guidance. I've cc'd Linus > Walleij more to make sure he has a heads up than to suggest > we continue this conversation in this thread. As I've already written elsewhere, possible solution is a new pair of gpio flags, GPIO_DEFAULT_POLARITY / GPIO_INVERTED_POLARITY, which means that polarity of signal is, corresponding, chip's default or inverted chip's default. And matching API for drivers to set default when requesting gpio. > > I don't know what the 'correct' way forward is. Funnily enough > my immediate thought is to it it with a big hammer and define > an inverting gpiochip to represent the possible inverter. > So a bit like a gpiochip setting on i2c but in this case > it sits on a gpio. So when you set the front gpio > (representing the inverter) it knows to set the opposite > on the actual gpio on the processor. This may be alternative to GPIO_DEFAULT_POLARITY / GPIO_INVERTED_POLARITY, that perhaps allows for device tree to better follow schematics. Still this does not play well with existing requirement to explicitly set ACTIVE_LOW / ACTIVE_HIGH at gpio usage side. Nikita P.S. Another view is that for such a trivial entity as gpio, we already have - physical gpio values, - logical values that come from ACTIVE_LOW / ACTIVE_HIGH, - for gpios used as interrupts, active low / active high irq's, and discussing addition more to that... for simplification? Joking? At least, if adding default-polarity / inverted-polarity, this should be done mutually-exclusive with active-high / active-low. And if adding inverter gpiochip, then also this should be mutual exclusive with setting active-high / active-low.
Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio
>> - Thinking further on this, I realized that for common case signal >> polarity is something defined by chip, and thus this knowledge belongs >> to chip's driver and not to chip user's device tree. Moreover, device >> tree writer could easily be not aware of signal polarity (too many >> datasheets are NDA-closed), thus hello copy-pasting, try-and-check and >> other counterproductive approaches. >> >> - Then Vladimir pointed real-life case with signal inversion by handmade >> level shifter. Although scope of this is likely limited to hw labs, >> support for this is wanted and thus some way to override polarity in >> chip user's dts be available. Still, this should be optional, without >> requiring dts to always define polarity of each gpio. > > I have real hardware that effectively does the equivalent for interrupt > lines coming into the processor from some sensors. Note quite the same, > but shows these things often do turn up as a fix on 'real' hardware. Well no doubt that sometimes there is need to invert polarity for particular use case. But still forcing explicit definition of polarity of each and every gpio usage is a bad way to solve this. > You are correct, this needs some wider guidance. I've cc'd Linus > Walleij more to make sure he has a heads up than to suggest > we continue this conversation in this thread. As I've already written elsewhere, possible solution is a new pair of gpio flags, GPIO_DEFAULT_POLARITY / GPIO_INVERTED_POLARITY, which means that polarity of signal is, corresponding, chip's default or inverted chip's default. And matching API for drivers to set default when requesting gpio. > > I don't know what the 'correct' way forward is. Funnily enough > my immediate thought is to it it with a big hammer and define > an inverting gpiochip to represent the possible inverter. > So a bit like a gpiochip setting on i2c but in this case > it sits on a gpio. So when you set the front gpio > (representing the inverter) it knows to set the opposite > on the actual gpio on the processor. This may be alternative to GPIO_DEFAULT_POLARITY / GPIO_INVERTED_POLARITY, that perhaps allows for device tree to better follow schematics. Still this does not play well with existing requirement to explicitly set ACTIVE_LOW / ACTIVE_HIGH at gpio usage side. Nikita P.S. Another view is that for such a trivial entity as gpio, we already have - physical gpio values, - logical values that come from ACTIVE_LOW / ACTIVE_HIGH, - for gpios used as interrupts, active low / active high irq's, and discussing addition more to that... for simplification? Joking? At least, if adding default-polarity / inverted-polarity, this should be done mutually-exclusive with active-high / active-low. And if adding inverter gpiochip, then also this should be mutual exclusive with setting active-high / active-low.
Re: [PATCH/RFC] iio: hi8435: do not enable all events by default
>> Still, isn't there subsystem-level default that all events are disabled >> by default? If such, then current hi8435 state breaks subsystem-level >> rules, which is a [userspace-visible] bug. I'm not sure how far should >> we go in bug compatibility. > > It is indeed the subsystem default (as much as we have one) > > This is a moderately obscure chip for linux systems, do we have a good handle > on where it is being used - i.e. are most of the devices under control of > people we can discuss this with? Company I work with, uses this chip in several boards; what they need is a service that monitors all connected chip's outputs and detects changes. They originally wanted gpio-style access to use with userspace polling, and were not pleased with entire IIO thing. However it's important for them to minimize required kernel patches against mainline, thus if mainline supports this chip as IIO device that's ok for them. Questions like default event enable state has little practical impact. It's more about keeping architecture clean. Nikita
Re: [PATCH/RFC] iio: hi8435: do not enable all events by default
>> Still, isn't there subsystem-level default that all events are disabled >> by default? If such, then current hi8435 state breaks subsystem-level >> rules, which is a [userspace-visible] bug. I'm not sure how far should >> we go in bug compatibility. > > It is indeed the subsystem default (as much as we have one) > > This is a moderately obscure chip for linux systems, do we have a good handle > on where it is being used - i.e. are most of the devices under control of > people we can discuss this with? Company I work with, uses this chip in several boards; what they need is a service that monitors all connected chip's outputs and detects changes. They originally wanted gpio-style access to use with userspace polling, and were not pleased with entire IIO thing. However it's important for them to minimize required kernel patches against mainline, thus if mainline supports this chip as IIO device that's ok for them. Questions like default event enable state has little practical impact. It's more about keeping architecture clean. Nikita
Re: [PATCH] ARM: dts: vf610-zii-dev-rev-b: add hi8435 device
>> + {
>> +status = "okay";
>> +};
>> +
>> + {
>
> Please keep these labelled nodes sort alphabetically.
Ok
>> +bus-num = <1>;
>> +pinctrl-names = "default";
>> +pinctrl-0 = <_dspi2>;
>> +status = "okay";
>
> We usually have 'status' line at the bottom of property list.
Ok
>> +spi-num-chipselects = <2>;
>> +
>> +hi8435@1 {
>
> Node name should be something generic, while label can be specific
> model name. The following form might be better.
>
> hi8435: sensor@1
Ok
Although the same file, in lines nearby, has m25p128@0 and at93c46d@1.
Anyway let's delay this a bit, up to the reset polarity description
thing in hi8435 is resolved.
Nikita
Re: [PATCH] ARM: dts: vf610-zii-dev-rev-b: add hi8435 device
>> + {
>> +status = "okay";
>> +};
>> +
>> + {
>
> Please keep these labelled nodes sort alphabetically.
Ok
>> +bus-num = <1>;
>> +pinctrl-names = "default";
>> +pinctrl-0 = <_dspi2>;
>> +status = "okay";
>
> We usually have 'status' line at the bottom of property list.
Ok
>> +spi-num-chipselects = <2>;
>> +
>> +hi8435@1 {
>
> Node name should be something generic, while label can be specific
> model name. The following form might be better.
>
> hi8435: sensor@1
Ok
Although the same file, in lines nearby, has m25p128@0 and at93c46d@1.
Anyway let's delay this a bit, up to the reset polarity description
thing in hi8435 is resolved.
Nikita
Re: [PATCH] ARM: dts: vf610-zii-dev-rev-b: add hi8435 device
>> "Crap origin" here is that in vast majority of cases, polarity is >> per-chip, not per-chip-use, knowledge. And proper location for per-chip >> knowledge is chip's driver. Moving this knowledge to per-chip-use >> location in device trees only provides a source for errors, with little >> gain. >> >> Vladimir Barinov mentions possibility that signal can be inverted by >> board between gpio provider and chip's pin ... but do we have at least >> one practical case of this? And if we even do, it's quite uncommon, and >> something special should be required in device tree for these special >> cases and not for "normal" cases. > > I disagree. Not for hi8435, but I have seen quite some board designs > invert GPIOs before getting them into board level components. That's > why we should define those xxx-gpios properties on board level DTS, > where polarity can be chosen per board design. Even if such, still board specific knowledge is "is gpio as-is or inverted", but knowledge if chip expects signal to be active low or active high, remains chip-specific. I'm thinking of proposing new flags in gpio binding, say GPIO_NATIVE_POLARITY / GPIO_INVERTED_POLARITY, that could be used instead of GPIO_ACTIVE_HIGH / GPIO_ACTIVE_LOW, and leave knowledge about signal polarity to chip's driver, while still allow to describe inversion of needed. Nikita
Re: [PATCH] ARM: dts: vf610-zii-dev-rev-b: add hi8435 device
>> "Crap origin" here is that in vast majority of cases, polarity is >> per-chip, not per-chip-use, knowledge. And proper location for per-chip >> knowledge is chip's driver. Moving this knowledge to per-chip-use >> location in device trees only provides a source for errors, with little >> gain. >> >> Vladimir Barinov mentions possibility that signal can be inverted by >> board between gpio provider and chip's pin ... but do we have at least >> one practical case of this? And if we even do, it's quite uncommon, and >> something special should be required in device tree for these special >> cases and not for "normal" cases. > > I disagree. Not for hi8435, but I have seen quite some board designs > invert GPIOs before getting them into board level components. That's > why we should define those xxx-gpios properties on board level DTS, > where polarity can be chosen per board design. Even if such, still board specific knowledge is "is gpio as-is or inverted", but knowledge if chip expects signal to be active low or active high, remains chip-specific. I'm thinking of proposing new flags in gpio binding, say GPIO_NATIVE_POLARITY / GPIO_INVERTED_POLARITY, that could be used instead of GPIO_ACTIVE_HIGH / GPIO_ACTIVE_LOW, and leave knowledge about signal polarity to chip's driver, while still allow to describe inversion of needed. Nikita
Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio
> Reset GPIO is active low. > > Currently driver uses gpiod_set_value(1) to clean reset, which depends > on device tree to contain GPIO_ACTIVE_HIGH - that does not match reality. > > This fixes driver to use _raw version of gpiod_set_value() to enforce > active-low semantics despite of what's written in device tree. Allowing > device tree to override that only opens possibility for errors and does > not add any value. > > Additionally, use _cansleep version to make things work with i2c-gpio > and other sleeping gpio drivers. The reset gpio comes from platform hence it should be handled by DTS. In driver the gpio should not be raw. Even the hi8435 is active low but platform may invert signal (f.e. by adding trigger on the circuit path). >>> I see. However - isn't this pure theoretic? Does such case exist? >> I assure you that this is frequently used. >> >> Simply search google for "simple voltage level shifter" >> It might be on PNP or NPN transistor, hence logic might be inverted. >> >>> >>> In vast majority of cases, GPIO polarity is chip-specific, not >>> chip-use-specific. Thus this knowlege belongs to driver and not to >>> device tree describing particular chip usage. Having this always >>> defined at usage side is IMO major source of errors. >> GPIO comes from SoC then "circuit path" and finally chip reset input. >> >> What do you propose if h/w circuit path has simple voltage level shifter >> on transistor. How to differentiate PNP and NPN cases? > > Hmm. Ah well, I clearly jumped too fast on this set and should have > left it for a while longer (I rushed a little as I'm away next weekend > and the cycle is moving towards rc3) > > Sorry about that. > > Anyhow, I am tempted to queue a revert of this patch. The level > shifting case hadn't occurred to me (oops). > > Thoughts? Well here is the full story. - I found that chip's reset line is active low per datasheet, but device tree for board I work with states it is active high - I checked driver code and found that driver depends on this incorrect setting, it won't work if device tree will state that gpio is active low - I could revert values in driver code AND in device tree, this way make device tree be correct (against reality) but make dtb files flashed into existing systems incompatible with future kernels - which I disliked - Thus I thought that I can remove explicit definition of polarity from device tree (replacing it with neutrally-looking zero), and change driver to use _raw. I assumed that there is no real gain to let device tree override gpio polarity for signal that is per-datasheet always active low - Thinking further on this, I realized that for common case signal polarity is something defined by chip, and thus this knowledge belongs to chip's driver and not to chip user's device tree. Moreover, device tree writer could easily be not aware of signal polarity (too many datasheets are NDA-closed), thus hello copy-pasting, try-and-check and other counterproductive approaches. - Then Vladimir pointed real-life case with signal inversion by handmade level shifter. Although scope of this is likely limited to hw labs, support for this is wanted and thus some way to override polarity in chip user's dts be available. Still, this should be optional, without requiring dts to always define polarity of each gpio. It becomes obvious that this topic has global scope, it is not something to solve within hi8345 driver or within iio. For patch in question, possibilities are: - revert the patch, restore situation with driver depending on wrong statement in dts, maybe document that in bindings, - replace patch with code assuming that device tree has correct definition of reset gpio polarity; break existing device trees (all are out-of-tree as of today), - keep the patch, thus not break anything and still stop requiring device tree to contain wrong statement, but make entire situation somewhat hacky and loose support for board reverting signal between gpio provider and hi8435's pin (hopefully no such board exists). I don't know. Maintainer should decide. Nikita
Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio
> Reset GPIO is active low. > > Currently driver uses gpiod_set_value(1) to clean reset, which depends > on device tree to contain GPIO_ACTIVE_HIGH - that does not match reality. > > This fixes driver to use _raw version of gpiod_set_value() to enforce > active-low semantics despite of what's written in device tree. Allowing > device tree to override that only opens possibility for errors and does > not add any value. > > Additionally, use _cansleep version to make things work with i2c-gpio > and other sleeping gpio drivers. The reset gpio comes from platform hence it should be handled by DTS. In driver the gpio should not be raw. Even the hi8435 is active low but platform may invert signal (f.e. by adding trigger on the circuit path). >>> I see. However - isn't this pure theoretic? Does such case exist? >> I assure you that this is frequently used. >> >> Simply search google for "simple voltage level shifter" >> It might be on PNP or NPN transistor, hence logic might be inverted. >> >>> >>> In vast majority of cases, GPIO polarity is chip-specific, not >>> chip-use-specific. Thus this knowlege belongs to driver and not to >>> device tree describing particular chip usage. Having this always >>> defined at usage side is IMO major source of errors. >> GPIO comes from SoC then "circuit path" and finally chip reset input. >> >> What do you propose if h/w circuit path has simple voltage level shifter >> on transistor. How to differentiate PNP and NPN cases? > > Hmm. Ah well, I clearly jumped too fast on this set and should have > left it for a while longer (I rushed a little as I'm away next weekend > and the cycle is moving towards rc3) > > Sorry about that. > > Anyhow, I am tempted to queue a revert of this patch. The level > shifting case hadn't occurred to me (oops). > > Thoughts? Well here is the full story. - I found that chip's reset line is active low per datasheet, but device tree for board I work with states it is active high - I checked driver code and found that driver depends on this incorrect setting, it won't work if device tree will state that gpio is active low - I could revert values in driver code AND in device tree, this way make device tree be correct (against reality) but make dtb files flashed into existing systems incompatible with future kernels - which I disliked - Thus I thought that I can remove explicit definition of polarity from device tree (replacing it with neutrally-looking zero), and change driver to use _raw. I assumed that there is no real gain to let device tree override gpio polarity for signal that is per-datasheet always active low - Thinking further on this, I realized that for common case signal polarity is something defined by chip, and thus this knowledge belongs to chip's driver and not to chip user's device tree. Moreover, device tree writer could easily be not aware of signal polarity (too many datasheets are NDA-closed), thus hello copy-pasting, try-and-check and other counterproductive approaches. - Then Vladimir pointed real-life case with signal inversion by handmade level shifter. Although scope of this is likely limited to hw labs, support for this is wanted and thus some way to override polarity in chip user's dts be available. Still, this should be optional, without requiring dts to always define polarity of each gpio. It becomes obvious that this topic has global scope, it is not something to solve within hi8345 driver or within iio. For patch in question, possibilities are: - revert the patch, restore situation with driver depending on wrong statement in dts, maybe document that in bindings, - replace patch with code assuming that device tree has correct definition of reset gpio polarity; break existing device trees (all are out-of-tree as of today), - keep the patch, thus not break anything and still stop requiring device tree to contain wrong statement, but make entire situation somewhat hacky and loose support for board reverting signal between gpio provider and hi8435's pin (hopefully no such board exists). I don't know. Maintainer should decide. Nikita
Re: [PATCH/RFC] iio: hi8435: do not enable all events by default
24.05.2017 22:27, Jonathan Cameron wrote: > On Tue, 23 May 2017 11:08:30 +0300 > Nikita Yushchenko <nikita.yo...@cogentembedded.com> wrote: > >> Having all events enabled by default is misleading. >> Userspace should explicitly enable events they want to receive. >> >> Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com> > I agree in principle, but this is a userspace ABI change. Sadly we > can't do it with out risking breaking userspace code... > > One of those we should have caught in review, but now it's there > we can't actually do anything about it unless we are absolutely > sure no one will notice! I see your point. Still, isn't there subsystem-level default that all events are disabled by default? If such, then current hi8435 state breaks subsystem-level rules, which is a [userspace-visible] bug. I'm not sure how far should we go in bug compatibility. One crazy idea could be - make default selectable via device tree (with default set to all-enabled to keep bug-compatibility). But perhaps that's over-reaction.
Re: [PATCH/RFC] iio: hi8435: do not enable all events by default
24.05.2017 22:27, Jonathan Cameron wrote: > On Tue, 23 May 2017 11:08:30 +0300 > Nikita Yushchenko wrote: > >> Having all events enabled by default is misleading. >> Userspace should explicitly enable events they want to receive. >> >> Signed-off-by: Nikita Yushchenko > I agree in principle, but this is a userspace ABI change. Sadly we > can't do it with out risking breaking userspace code... > > One of those we should have caught in review, but now it's there > we can't actually do anything about it unless we are absolutely > sure no one will notice! I see your point. Still, isn't there subsystem-level default that all events are disabled by default? If such, then current hi8435 state breaks subsystem-level rules, which is a [userspace-visible] bug. I'm not sure how far should we go in bug compatibility. One crazy idea could be - make default selectable via device tree (with default set to all-enabled to keep bug-compatibility). But perhaps that's over-reaction.
Re: [PATCH] iio: hi8435: fix race between event enable and event generation
>> Add locking to avoid interference between reading/processing current >> sence in event enable and event generation paths. >> >> Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com> > Make sense. Could you give a little more detail on the seriousness > of this bug? I need to assess whether we just queue it up for the > next merge window or push it forward quicker as a bug fix and > whether to mark it for stable or not. > > You probably have a better idea of how much it matters than I do! Race was introduced by my previous patch, and noticed by Vladimir. It is not in any released tree. Possible race impact is loss of event or duplicate event. Nikita
Re: [PATCH] iio: hi8435: fix race between event enable and event generation
>> Add locking to avoid interference between reading/processing current >> sence in event enable and event generation paths. >> >> Signed-off-by: Nikita Yushchenko > Make sense. Could you give a little more detail on the seriousness > of this bug? I need to assess whether we just queue it up for the > next merge window or push it forward quicker as a bug fix and > whether to mark it for stable or not. > > You probably have a better idea of how much it matters than I do! Race was introduced by my previous patch, and noticed by Vladimir. It is not in any released tree. Possible race impact is loss of event or duplicate event. Nikita
Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio
>> Reset GPIO is active low. >> >> Currently driver uses gpiod_set_value(1) to clean reset, which depends >> on device tree to contain GPIO_ACTIVE_HIGH - that does not match reality. >> >> This fixes driver to use _raw version of gpiod_set_value() to enforce >> active-low semantics despite of what's written in device tree. Allowing >> device tree to override that only opens possibility for errors and does >> not add any value. >> >> Additionally, use _cansleep version to make things work with i2c-gpio >> and other sleeping gpio drivers. > > The reset gpio comes from platform hence it should be handled by DTS. > > In driver the gpio should not be raw. > > Even the hi8435 is active low but platform may invert signal (f.e. by > adding trigger on the circuit path). I see. However - isn't this pure theoretic? Does such case exist? In vast majority of cases, GPIO polarity is chip-specific, not chip-use-specific. Thus this knowlege belongs to driver and not to device tree describing particular chip usage. Having this always defined at usage side is IMO major source of errors. In this particular case, we already have device trees in the wild with polarity set to active_high. If we now change that to require active_low, we break existing setups. Not sure this is acceptable. I'm unsure what is good way forward here. I believe it is at gpiolib level, which perhaps should change how "logical value" is defined, such that knowledge of chip-specific information is not forced to chip usage scope. Nikita
Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio
>> Reset GPIO is active low. >> >> Currently driver uses gpiod_set_value(1) to clean reset, which depends >> on device tree to contain GPIO_ACTIVE_HIGH - that does not match reality. >> >> This fixes driver to use _raw version of gpiod_set_value() to enforce >> active-low semantics despite of what's written in device tree. Allowing >> device tree to override that only opens possibility for errors and does >> not add any value. >> >> Additionally, use _cansleep version to make things work with i2c-gpio >> and other sleeping gpio drivers. > > The reset gpio comes from platform hence it should be handled by DTS. > > In driver the gpio should not be raw. > > Even the hi8435 is active low but platform may invert signal (f.e. by > adding trigger on the circuit path). I see. However - isn't this pure theoretic? Does such case exist? In vast majority of cases, GPIO polarity is chip-specific, not chip-use-specific. Thus this knowlege belongs to driver and not to device tree describing particular chip usage. Having this always defined at usage side is IMO major source of errors. In this particular case, we already have device trees in the wild with polarity set to active_high. If we now change that to require active_low, we break existing setups. Not sure this is acceptable. I'm unsure what is good way forward here. I believe it is at gpiolib level, which perhaps should change how "logical value" is defined, such that knowledge of chip-specific information is not forced to chip usage scope. Nikita
[PATCH/RFC] iio: hi8435: do not enable all events by default
Having all events enabled by default is misleading. Userspace should explicitly enable events they want to receive. Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com> --- drivers/iio/adc/hi8435.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c index ef5c286c8e67..0331739f235c 100644 --- a/drivers/iio/adc/hi8435.c +++ b/drivers/iio/adc/hi8435.c @@ -506,8 +506,6 @@ static int hi8435_probe(struct spi_device *spi) idev->channels = hi8435_channels; idev->num_channels = ARRAY_SIZE(hi8435_channels); - /* unmask all events */ - priv->event_scan_mask = ~(0); /* * There is a restriction in the chip - the hysteresis can not be odd. * If the hysteresis is set to odd value then chip gets into lock state -- 2.11.0
[PATCH/RFC] iio: hi8435: do not enable all events by default
Having all events enabled by default is misleading. Userspace should explicitly enable events they want to receive. Signed-off-by: Nikita Yushchenko --- drivers/iio/adc/hi8435.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c index ef5c286c8e67..0331739f235c 100644 --- a/drivers/iio/adc/hi8435.c +++ b/drivers/iio/adc/hi8435.c @@ -506,8 +506,6 @@ static int hi8435_probe(struct spi_device *spi) idev->channels = hi8435_channels; idev->num_channels = ARRAY_SIZE(hi8435_channels); - /* unmask all events */ - priv->event_scan_mask = ~(0); /* * There is a restriction in the chip - the hysteresis can not be odd. * If the hysteresis is set to odd value then chip gets into lock state -- 2.11.0
[PATCH] iio: hi8435: remote ampersands from hi8435_info definition
C syntax allows apersands when initializing structures fields with
function pointers, but in Linux sources ampersands are normally not used
in thix context.
Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com>
---
drivers/iio/adc/hi8435.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
index 28fc699f00f6..ef5c286c8e67 100644
--- a/drivers/iio/adc/hi8435.c
+++ b/drivers/iio/adc/hi8435.c
@@ -414,11 +414,11 @@ static const struct iio_chan_spec hi8435_channels[] = {
static const struct iio_info hi8435_info = {
.driver_module = THIS_MODULE,
.read_raw = hi8435_read_raw,
- .read_event_config = _read_event_config,
+ .read_event_config = hi8435_read_event_config,
.write_event_config = hi8435_write_event_config,
- .read_event_value = _read_event_value,
- .write_event_value = _write_event_value,
- .debugfs_reg_access = _debugfs_reg_access,
+ .read_event_value = hi8435_read_event_value,
+ .write_event_value = hi8435_write_event_value,
+ .debugfs_reg_access = hi8435_debugfs_reg_access,
};
static void hi8435_iio_push_event(struct iio_dev *idev, unsigned int val)
--
2.11.0
[PATCH] iio: hi8435: remote ampersands from hi8435_info definition
C syntax allows apersands when initializing structures fields with
function pointers, but in Linux sources ampersands are normally not used
in thix context.
Signed-off-by: Nikita Yushchenko
---
drivers/iio/adc/hi8435.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
index 28fc699f00f6..ef5c286c8e67 100644
--- a/drivers/iio/adc/hi8435.c
+++ b/drivers/iio/adc/hi8435.c
@@ -414,11 +414,11 @@ static const struct iio_chan_spec hi8435_channels[] = {
static const struct iio_info hi8435_info = {
.driver_module = THIS_MODULE,
.read_raw = hi8435_read_raw,
- .read_event_config = _read_event_config,
+ .read_event_config = hi8435_read_event_config,
.write_event_config = hi8435_write_event_config,
- .read_event_value = _read_event_value,
- .write_event_value = _write_event_value,
- .debugfs_reg_access = _debugfs_reg_access,
+ .read_event_value = hi8435_read_event_value,
+ .write_event_value = hi8435_write_event_value,
+ .debugfs_reg_access = hi8435_debugfs_reg_access,
};
static void hi8435_iio_push_event(struct iio_dev *idev, unsigned int val)
--
2.11.0
[PATCH] iio: hi8435: fix race between event enable and event generation
Add locking to avoid interference between reading/processing current
sence in event enable and event generation paths.
Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com>
---
drivers/iio/adc/hi8435.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
index ab59969b7c49..28fc699f00f6 100644
--- a/drivers/iio/adc/hi8435.c
+++ b/drivers/iio/adc/hi8435.c
@@ -144,6 +144,8 @@ static int hi8435_write_event_config(struct iio_dev *idev,
int ret;
u32 tmp;
+ mutex_lock(>lock);
+
if (state) {
ret = hi8435_readl(priv, HI8435_SO31_0_REG, );
if (ret < 0)
@@ -157,6 +159,8 @@ static int hi8435_write_event_config(struct iio_dev *idev,
} else
priv->event_scan_mask &= ~BIT(chan->channel);
+ mutex_unlock(>lock);
+
return 0;
}
@@ -449,12 +453,16 @@ static irqreturn_t hi8435_trigger_handler(int irq, void
*private)
u32 val;
int ret;
+ mutex_lock(>lock);
+
ret = hi8435_readl(priv, HI8435_SO31_0_REG, );
if (ret < 0)
goto err_read;
hi8435_iio_push_event(idev, val);
+ mutex_unlock(>lock);
+
err_read:
iio_trigger_notify_done(idev->trig);
--
2.11.0
[PATCH] iio: hi8435: fix race between event enable and event generation
Add locking to avoid interference between reading/processing current
sence in event enable and event generation paths.
Signed-off-by: Nikita Yushchenko
---
drivers/iio/adc/hi8435.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
index ab59969b7c49..28fc699f00f6 100644
--- a/drivers/iio/adc/hi8435.c
+++ b/drivers/iio/adc/hi8435.c
@@ -144,6 +144,8 @@ static int hi8435_write_event_config(struct iio_dev *idev,
int ret;
u32 tmp;
+ mutex_lock(>lock);
+
if (state) {
ret = hi8435_readl(priv, HI8435_SO31_0_REG, );
if (ret < 0)
@@ -157,6 +159,8 @@ static int hi8435_write_event_config(struct iio_dev *idev,
} else
priv->event_scan_mask &= ~BIT(chan->channel);
+ mutex_unlock(>lock);
+
return 0;
}
@@ -449,12 +453,16 @@ static irqreturn_t hi8435_trigger_handler(int irq, void
*private)
u32 val;
int ret;
+ mutex_lock(>lock);
+
ret = hi8435_readl(priv, HI8435_SO31_0_REG, );
if (ret < 0)
goto err_read;
hi8435_iio_push_event(idev, val);
+ mutex_unlock(>lock);
+
err_read:
iio_trigger_notify_done(idev->trig);
--
2.11.0
Re: [PATCH 2/4] iio: hi8435: avoid garbage event at first enable
>> +int ret;
>> +u32 tmp;
>> +
>> +if (state) {
>> +ret = hi8435_readl(priv, HI8435_SO31_0_REG, );
>> +if (ret < 0)
>> +return ret;
>> +if (tmp & BIT(chan->channel))
>> +priv->event_prev_val |= BIT(chan->channel);
>> +else
>> +priv->event_prev_val &= ~BIT(chan->channel);
>> -priv->event_scan_mask &= ~BIT(chan->channel);
>> -if (state)
>> priv->event_scan_mask |= BIT(chan->channel);
>> +} else
>> +priv->event_scan_mask &= ~BIT(chan->channel);
>>
>
> This will race with hi8435_iio_push_event for priv->event_scan_mask.
I was under impression that mutual-exclusion is provided by core. Now I
see it is not. Will submit a fix soon.
> Since you adding raw access then it is reasonable to initialize
> priv->event_prev_val in hi8435_read_raw.
This will cause complex interdependency between events and raw access.
E.g. should we generate event if change was discovered while processing
raw access from user space? If we do, then we break semantics "events
generated only under trigger". If we don't, we need complex state handling.
> Then use the following sequence for your application:
> 1. read raw value
> 2. enable events
So what if:
- one application calls read raw,
- then, far later, signal changes,
- then, far later, other application enables events?
Should second application get notification of change that happened long
before in enabled events? I think no.
There is fundamental race between start-of-monitoring and
change-of-signal-near-start-of-monitoring. To avoid it, reading signal
must be either atomic with start of monitoring (which is not possible
with IIO API), or done after start of monitoring (which moves handling
of this race to user side).
Nikita
Re: [PATCH 2/4] iio: hi8435: avoid garbage event at first enable
>> +int ret;
>> +u32 tmp;
>> +
>> +if (state) {
>> +ret = hi8435_readl(priv, HI8435_SO31_0_REG, );
>> +if (ret < 0)
>> +return ret;
>> +if (tmp & BIT(chan->channel))
>> +priv->event_prev_val |= BIT(chan->channel);
>> +else
>> +priv->event_prev_val &= ~BIT(chan->channel);
>> -priv->event_scan_mask &= ~BIT(chan->channel);
>> -if (state)
>> priv->event_scan_mask |= BIT(chan->channel);
>> +} else
>> +priv->event_scan_mask &= ~BIT(chan->channel);
>>
>
> This will race with hi8435_iio_push_event for priv->event_scan_mask.
I was under impression that mutual-exclusion is provided by core. Now I
see it is not. Will submit a fix soon.
> Since you adding raw access then it is reasonable to initialize
> priv->event_prev_val in hi8435_read_raw.
This will cause complex interdependency between events and raw access.
E.g. should we generate event if change was discovered while processing
raw access from user space? If we do, then we break semantics "events
generated only under trigger". If we don't, we need complex state handling.
> Then use the following sequence for your application:
> 1. read raw value
> 2. enable events
So what if:
- one application calls read raw,
- then, far later, signal changes,
- then, far later, other application enables events?
Should second application get notification of change that happened long
before in enabled events? I think no.
There is fundamental race between start-of-monitoring and
change-of-signal-near-start-of-monitoring. To avoid it, reading signal
must be either atomic with start of monitoring (which is not possible
with IIO API), or done after start of monitoring (which moves handling
of this race to user side).
Nikita
Re: [PATCH] ARM: dts: vf610-zii-dev-rev-b: add hi8435 device
>> However, hi8435 driver historically was coded using inverted values >> passed to gpiolib calls. And there are setups in the wild with device >> trees containing GPIO_ACTIVE_HIGH that I'd prefer not breaking. >> >> To solve, I submitted a patch on hi8435 driver that changes to _raw() >> gpio calls (thus making it independent of what is written in device >> tree), and want [future] device trees not to contain explicitly written >> gpio polarity. > > So maybe add another #define, GPIO_ACTIVE_IGNORED, to make it clear > that it does not matter what value you put there, it is ignored. "Crap origin" here is that in vast majority of cases, polarity is per-chip, not per-chip-use, knowledge. And proper location for per-chip knowledge is chip's driver. Moving this knowledge to per-chip-use location in device trees only provides a source for errors, with little gain. Vladimir Barinov mentions possibility that signal can be inverted by board between gpio provider and chip's pin ... but do we have at least one practical case of this? And if we even do, it's quite uncommon, and something special should be required in device tree for these special cases and not for "normal" cases.
Re: [PATCH] ARM: dts: vf610-zii-dev-rev-b: add hi8435 device
>> However, hi8435 driver historically was coded using inverted values >> passed to gpiolib calls. And there are setups in the wild with device >> trees containing GPIO_ACTIVE_HIGH that I'd prefer not breaking. >> >> To solve, I submitted a patch on hi8435 driver that changes to _raw() >> gpio calls (thus making it independent of what is written in device >> tree), and want [future] device trees not to contain explicitly written >> gpio polarity. > > So maybe add another #define, GPIO_ACTIVE_IGNORED, to make it clear > that it does not matter what value you put there, it is ignored. "Crap origin" here is that in vast majority of cases, polarity is per-chip, not per-chip-use, knowledge. And proper location for per-chip knowledge is chip's driver. Moving this knowledge to per-chip-use location in device trees only provides a source for errors, with little gain. Vladimir Barinov mentions possibility that signal can be inverted by board between gpio provider and chip's pin ... but do we have at least one practical case of this? And if we even do, it's quite uncommon, and something special should be required in device tree for these special cases and not for "normal" cases.
Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
Hi,
Alternative solution could be - have separate write path for earlycon.
>>>
>>> It looks to me having the same issue with a separate write patch
>>> for earlycon as we still need distinguish Little or Big endian
>>> for Layerscape and IMX.
>>>
At a glance, it is dozen lines of code.
>>>
>>> Would you please show some sample code?
>>
>> Do not reuse lpuart32_console_putchar() in earlycon code.
>>
>> Have two sets of early_setup/early_write/putchar - for BE and
>> defaut-endian earlycon. And in these putchar's do not use
>> lpuart_(read|write).
>>
>
> Isn't that introducing another consistency break after fix one
> consistency break?
>
> If doing that, we then have two register read/write APIs.
> One for normal driver operation by dynamically checking lpuart_is_be
> property to distinguish the endian difference problem.
> Another is specifically implemented for only early console read/write
> and use hardcoded way to read/write register directly instead of using
> the standard API lpuart32_read/write, like follows:
> e.g.
> lpuart32_le_console_write() {
>writel();
> }
>
> lpuart32_be_console_write() {
>iowrite32be()
> }
> This also makes the driver a bit strange and ugly.
>
> It looks to me both way are trade offs and the later one seems sacrifice
> more. And i doubt if it's really necessary for probably a no real gain
> purpose as the FPGA you mentioned is a theoretical case and less
> possibility to exist.
>
> I'm still wondering how about keep using the exist way and adding more
> information in code to explain why use a global var?
I've checked other driver under drivers/tty/serial/, for examples of
similar cases.
Please look at serial8250_early_in() / serial8250_early_out() ?
These do handle different endian, via port->iotype
Another example is drivers/tty/serial/samsung.c, where
port->private_data is initialized and used.
Nikita
Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
Hi,
Alternative solution could be - have separate write path for earlycon.
>>>
>>> It looks to me having the same issue with a separate write patch
>>> for earlycon as we still need distinguish Little or Big endian
>>> for Layerscape and IMX.
>>>
At a glance, it is dozen lines of code.
>>>
>>> Would you please show some sample code?
>>
>> Do not reuse lpuart32_console_putchar() in earlycon code.
>>
>> Have two sets of early_setup/early_write/putchar - for BE and
>> defaut-endian earlycon. And in these putchar's do not use
>> lpuart_(read|write).
>>
>
> Isn't that introducing another consistency break after fix one
> consistency break?
>
> If doing that, we then have two register read/write APIs.
> One for normal driver operation by dynamically checking lpuart_is_be
> property to distinguish the endian difference problem.
> Another is specifically implemented for only early console read/write
> and use hardcoded way to read/write register directly instead of using
> the standard API lpuart32_read/write, like follows:
> e.g.
> lpuart32_le_console_write() {
>writel();
> }
>
> lpuart32_be_console_write() {
>iowrite32be()
> }
> This also makes the driver a bit strange and ugly.
>
> It looks to me both way are trade offs and the later one seems sacrifice
> more. And i doubt if it's really necessary for probably a no real gain
> purpose as the FPGA you mentioned is a theoretical case and less
> possibility to exist.
>
> I'm still wondering how about keep using the exist way and adding more
> information in code to explain why use a global var?
I've checked other driver under drivers/tty/serial/, for examples of
similar cases.
Please look at serial8250_early_in() / serial8250_early_out() ?
These do handle different endian, via port->iotype
Another example is drivers/tty/serial/samsung.c, where
port->private_data is initialized and used.
Nikita
Re: [PATCH] ARM: dts: vf610-zii-dev-rev-b: add hi8435 device
>> +hi8435@1 {
>> +compatible = "holt,hi8435";
>> +reg = <1>;
>> +spi-max-frequency = <2000>;
>> +gpios = < 3 0>;
>
> Nit: GPIO_ACTIVE_HIGH instead of 0?
Gray area here.
Chip's reset input is active LOW.
However, hi8435 driver historically was coded using inverted values
passed to gpiolib calls. And there are setups in the wild with device
trees containing GPIO_ACTIVE_HIGH that I'd prefer not breaking.
To solve, I submitted a patch on hi8435 driver that changes to _raw()
gpio calls (thus making it independent of what is written in device
tree), and want [future] device trees not to contain explicitly written
gpio polarity.
Far not ideal but better than device trees explicitly stating
GPIO_ACTIVE_HIGH while it is active low.
Nikita
Re: [PATCH] ARM: dts: vf610-zii-dev-rev-b: add hi8435 device
>> +hi8435@1 {
>> +compatible = "holt,hi8435";
>> +reg = <1>;
>> +spi-max-frequency = <2000>;
>> +gpios = < 3 0>;
>
> Nit: GPIO_ACTIVE_HIGH instead of 0?
Gray area here.
Chip's reset input is active LOW.
However, hi8435 driver historically was coded using inverted values
passed to gpiolib calls. And there are setups in the wild with device
trees containing GPIO_ACTIVE_HIGH that I'd prefer not breaking.
To solve, I submitted a patch on hi8435 driver that changes to _raw()
gpio calls (thus making it independent of what is written in device
tree), and want [future] device trees not to contain explicitly written
gpio polarity.
Far not ideal but better than device trees explicitly stating
GPIO_ACTIVE_HIGH while it is active low.
Nikita
[PATCH] spi: spi-fsl-dspi: ensure non-zero return on error path
Propagate error return from dspi_request_dma() into probe routine's
return.
Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com>
---
drivers/spi/spi-fsl-dspi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 15201645bdc4..d89127f4a46d 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -1032,7 +1032,8 @@ static int dspi_probe(struct platform_device *pdev)
goto out_master_put;
if (dspi->devtype_data->trans_mode == DSPI_DMA_MODE) {
- if (dspi_request_dma(dspi, res->start)) {
+ ret = dspi_request_dma(dspi, res->start);
+ if (ret < 0) {
dev_err(>dev, "can't get dma channels\n");
goto out_clk_put;
}
--
2.11.0
[PATCH] spi: spi-fsl-dspi: ensure non-zero return on error path
Propagate error return from dspi_request_dma() into probe routine's
return.
Signed-off-by: Nikita Yushchenko
---
drivers/spi/spi-fsl-dspi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 15201645bdc4..d89127f4a46d 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -1032,7 +1032,8 @@ static int dspi_probe(struct platform_device *pdev)
goto out_master_put;
if (dspi->devtype_data->trans_mode == DSPI_DMA_MODE) {
- if (dspi_request_dma(dspi, res->start)) {
+ ret = dspi_request_dma(dspi, res->start);
+ if (ret < 0) {
dev_err(>dev, "can't get dma channels\n");
goto out_clk_put;
}
--
2.11.0
[PATCH] ARM: dts: vf610-zii-dev-rev-b: add hi8435 device
ZII dev board rev B has a Holt Hi8435 connected to dspi2.
Add it to device tree.
ZII dev board rev C does not have that, so use rev-b dts file,
not common dtsi file.
Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com>
---
arch/arm/boot/dts/vf610-zii-dev-rev-b.dts | 19 +++
1 file changed, 19 insertions(+)
diff --git a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
index 37f95427616f..f433465c3a7c 100644
--- a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
+++ b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
@@ -348,6 +348,25 @@
};
};
+ {
+ status = "okay";
+};
+
+ {
+ bus-num = <1>;
+ pinctrl-names = "default";
+ pinctrl-0 = <_dspi2>;
+ status = "okay";
+ spi-num-chipselects = <2>;
+
+ hi8435@1 {
+ compatible = "holt,hi8435";
+ reg = <1>;
+ spi-max-frequency = <2000>;
+ gpios = < 3 0>;
+ };
+};
+
{
clock-frequency = <10>;
pinctrl-names = "default";
--
2.11.0
[PATCH] ARM: dts: vf610-zii-dev-rev-b: add hi8435 device
ZII dev board rev B has a Holt Hi8435 connected to dspi2.
Add it to device tree.
ZII dev board rev C does not have that, so use rev-b dts file,
not common dtsi file.
Signed-off-by: Nikita Yushchenko
---
arch/arm/boot/dts/vf610-zii-dev-rev-b.dts | 19 +++
1 file changed, 19 insertions(+)
diff --git a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
index 37f95427616f..f433465c3a7c 100644
--- a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
+++ b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
@@ -348,6 +348,25 @@
};
};
+ {
+ status = "okay";
+};
+
+ {
+ bus-num = <1>;
+ pinctrl-names = "default";
+ pinctrl-0 = <_dspi2>;
+ status = "okay";
+ spi-num-chipselects = <2>;
+
+ hi8435@1 {
+ compatible = "holt,hi8435";
+ reg = <1>;
+ spi-max-frequency = <2000>;
+ gpios = < 3 0>;
+ };
+};
+
{
clock-frequency = <10>;
pinctrl-names = "default";
--
2.11.0
[PATCH 2/4] iio: hi8435: avoid garbage event at first enable
Currently, driver generates events for channels if new reading differs
from previous one. This "previous value" is initialized to zero, which
results into event if value is constant-one.
Fix that by initializing "previous value" by reading at event enable
time.
This provides reliable sequence for userspace:
- enable event,
- AFTER THAT read current value,
- AFTER THAT each event will correspond to change.
Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com>
---
drivers/iio/adc/hi8435.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
index cb8e6342eddf..45a92e3e8f2b 100644
--- a/drivers/iio/adc/hi8435.c
+++ b/drivers/iio/adc/hi8435.c
@@ -141,10 +141,21 @@ static int hi8435_write_event_config(struct iio_dev *idev,
enum iio_event_direction dir, int state)
{
struct hi8435_priv *priv = iio_priv(idev);
+ int ret;
+ u32 tmp;
+
+ if (state) {
+ ret = hi8435_readl(priv, HI8435_SO31_0_REG, );
+ if (ret < 0)
+ return ret;
+ if (tmp & BIT(chan->channel))
+ priv->event_prev_val |= BIT(chan->channel);
+ else
+ priv->event_prev_val &= ~BIT(chan->channel);
- priv->event_scan_mask &= ~BIT(chan->channel);
- if (state)
priv->event_scan_mask |= BIT(chan->channel);
+ } else
+ priv->event_scan_mask &= ~BIT(chan->channel);
return 0;
}
--
2.11.0
[PATCH 2/4] iio: hi8435: avoid garbage event at first enable
Currently, driver generates events for channels if new reading differs
from previous one. This "previous value" is initialized to zero, which
results into event if value is constant-one.
Fix that by initializing "previous value" by reading at event enable
time.
This provides reliable sequence for userspace:
- enable event,
- AFTER THAT read current value,
- AFTER THAT each event will correspond to change.
Signed-off-by: Nikita Yushchenko
---
drivers/iio/adc/hi8435.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
index cb8e6342eddf..45a92e3e8f2b 100644
--- a/drivers/iio/adc/hi8435.c
+++ b/drivers/iio/adc/hi8435.c
@@ -141,10 +141,21 @@ static int hi8435_write_event_config(struct iio_dev *idev,
enum iio_event_direction dir, int state)
{
struct hi8435_priv *priv = iio_priv(idev);
+ int ret;
+ u32 tmp;
+
+ if (state) {
+ ret = hi8435_readl(priv, HI8435_SO31_0_REG, );
+ if (ret < 0)
+ return ret;
+ if (tmp & BIT(chan->channel))
+ priv->event_prev_val |= BIT(chan->channel);
+ else
+ priv->event_prev_val &= ~BIT(chan->channel);
- priv->event_scan_mask &= ~BIT(chan->channel);
- if (state)
priv->event_scan_mask |= BIT(chan->channel);
+ } else
+ priv->event_scan_mask &= ~BIT(chan->channel);
return 0;
}
--
2.11.0
[PATCH 3/4] iio: hi8435: make in_voltage_sensing_mode_available visible
Possible values of sensing_mode are encoded with strings and actual
atrings used are not obvious.
Provide a hint by enabling in_voltage_sensing_mode_available attribute.
Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com>
---
drivers/iio/adc/hi8435.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
index 45a92e3e8f2b..d09cb6ff8044 100644
--- a/drivers/iio/adc/hi8435.c
+++ b/drivers/iio/adc/hi8435.c
@@ -356,6 +356,7 @@ static const struct iio_enum hi8435_sensing_mode = {
static const struct iio_chan_spec_ext_info hi8435_ext_info[] = {
IIO_ENUM("sensing_mode", IIO_SEPARATE, _sensing_mode),
+ IIO_ENUM_AVAILABLE("sensing_mode", _sensing_mode),
{},
};
--
2.11.0
[PATCH 3/4] iio: hi8435: make in_voltage_sensing_mode_available visible
Possible values of sensing_mode are encoded with strings and actual
atrings used are not obvious.
Provide a hint by enabling in_voltage_sensing_mode_available attribute.
Signed-off-by: Nikita Yushchenko
---
drivers/iio/adc/hi8435.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
index 45a92e3e8f2b..d09cb6ff8044 100644
--- a/drivers/iio/adc/hi8435.c
+++ b/drivers/iio/adc/hi8435.c
@@ -356,6 +356,7 @@ static const struct iio_enum hi8435_sensing_mode = {
static const struct iio_chan_spec_ext_info hi8435_ext_info[] = {
IIO_ENUM("sensing_mode", IIO_SEPARATE, _sensing_mode),
+ IIO_ENUM_AVAILABLE("sensing_mode", _sensing_mode),
{},
};
--
2.11.0
[PATCH 1/4] iio: hi8435: add raw access
With current event-only driver, it is not possible for user space
application to know current senses if they don't change since
application starts.
Address that by adding raw access to channels.
Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com>
---
drivers/iio/adc/hi8435.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
index 678e8c7ea763..cb8e6342eddf 100644
--- a/drivers/iio/adc/hi8435.c
+++ b/drivers/iio/adc/hi8435.c
@@ -105,6 +105,26 @@ static int hi8435_writew(struct hi8435_priv *priv, u8 reg,
u16 val)
return spi_write(priv->spi, priv->reg_buffer, 3);
}
+static int hi8435_read_raw(struct iio_dev *idev,
+ const struct iio_chan_spec *chan,
+ int *val, int *val2, long mask)
+{
+ struct hi8435_priv *priv = iio_priv(idev);
+ u32 tmp;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = hi8435_readl(priv, HI8435_SO31_0_REG, );
+ if (ret < 0)
+ return ret;
+ *val = !!(tmp & BIT(chan->channel));
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
static int hi8435_read_event_config(struct iio_dev *idev,
const struct iio_chan_spec *chan,
enum iio_event_type type,
@@ -333,6 +353,7 @@ static const struct iio_chan_spec_ext_info
hi8435_ext_info[] = {
.type = IIO_VOLTAGE,\
.indexed = 1, \
.channel = num, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
.event_spec = hi8435_events,\
.num_event_specs = ARRAY_SIZE(hi8435_events), \
.ext_info = hi8435_ext_info,\
@@ -376,6 +397,7 @@ static const struct iio_chan_spec hi8435_channels[] = {
static const struct iio_info hi8435_info = {
.driver_module = THIS_MODULE,
+ .read_raw = hi8435_read_raw,
.read_event_config = _read_event_config,
.write_event_config = hi8435_write_event_config,
.read_event_value = _read_event_value,
--
2.11.0
[PATCH 1/4] iio: hi8435: add raw access
With current event-only driver, it is not possible for user space
application to know current senses if they don't change since
application starts.
Address that by adding raw access to channels.
Signed-off-by: Nikita Yushchenko
---
drivers/iio/adc/hi8435.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
index 678e8c7ea763..cb8e6342eddf 100644
--- a/drivers/iio/adc/hi8435.c
+++ b/drivers/iio/adc/hi8435.c
@@ -105,6 +105,26 @@ static int hi8435_writew(struct hi8435_priv *priv, u8 reg,
u16 val)
return spi_write(priv->spi, priv->reg_buffer, 3);
}
+static int hi8435_read_raw(struct iio_dev *idev,
+ const struct iio_chan_spec *chan,
+ int *val, int *val2, long mask)
+{
+ struct hi8435_priv *priv = iio_priv(idev);
+ u32 tmp;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = hi8435_readl(priv, HI8435_SO31_0_REG, );
+ if (ret < 0)
+ return ret;
+ *val = !!(tmp & BIT(chan->channel));
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
static int hi8435_read_event_config(struct iio_dev *idev,
const struct iio_chan_spec *chan,
enum iio_event_type type,
@@ -333,6 +353,7 @@ static const struct iio_chan_spec_ext_info
hi8435_ext_info[] = {
.type = IIO_VOLTAGE,\
.indexed = 1, \
.channel = num, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
.event_spec = hi8435_events,\
.num_event_specs = ARRAY_SIZE(hi8435_events), \
.ext_info = hi8435_ext_info,\
@@ -376,6 +397,7 @@ static const struct iio_chan_spec hi8435_channels[] = {
static const struct iio_info hi8435_info = {
.driver_module = THIS_MODULE,
+ .read_raw = hi8435_read_raw,
.read_event_config = _read_event_config,
.write_event_config = hi8435_write_event_config,
.read_event_value = _read_event_value,
--
2.11.0
[PATCH 4/4] iio: hi8435: cleanup reset gpio
Reset GPIO is active low.
Currently driver uses gpiod_set_value(1) to clean reset, which depends
on device tree to contain GPIO_ACTIVE_HIGH - that does not match reality.
This fixes driver to use _raw version of gpiod_set_value() to enforce
active-low semantics despite of what's written in device tree. Allowing
device tree to override that only opens possibility for errors and does
not add any value.
Additionally, use _cansleep version to make things work with i2c-gpio
and other sleeping gpio drivers.
Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com>
---
drivers/iio/adc/hi8435.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
index d09cb6ff8044..ab59969b7c49 100644
--- a/drivers/iio/adc/hi8435.c
+++ b/drivers/iio/adc/hi8435.c
@@ -476,13 +476,15 @@ static int hi8435_probe(struct spi_device *spi)
priv->spi = spi;
reset_gpio = devm_gpiod_get(>dev, NULL, GPIOD_OUT_LOW);
- if (IS_ERR(reset_gpio)) {
- /* chip s/w reset if h/w reset failed */
+ if (!IS_ERR(reset_gpio)) {
+ /* need >=100ns low pulse to reset chip */
+ gpiod_set_raw_value_cansleep(reset_gpio, 0);
+ udelay(1);
+ gpiod_set_raw_value_cansleep(reset_gpio, 1);
+ } else {
+ /* s/w reset chip if h/w reset is not available */
hi8435_writeb(priv, HI8435_CTRL_REG, HI8435_CTRL_SRST);
hi8435_writeb(priv, HI8435_CTRL_REG, 0);
- } else {
- udelay(5);
- gpiod_set_value(reset_gpio, 1);
}
spi_set_drvdata(spi, idev);
--
2.11.0
[PATCH 4/4] iio: hi8435: cleanup reset gpio
Reset GPIO is active low.
Currently driver uses gpiod_set_value(1) to clean reset, which depends
on device tree to contain GPIO_ACTIVE_HIGH - that does not match reality.
This fixes driver to use _raw version of gpiod_set_value() to enforce
active-low semantics despite of what's written in device tree. Allowing
device tree to override that only opens possibility for errors and does
not add any value.
Additionally, use _cansleep version to make things work with i2c-gpio
and other sleeping gpio drivers.
Signed-off-by: Nikita Yushchenko
---
drivers/iio/adc/hi8435.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
index d09cb6ff8044..ab59969b7c49 100644
--- a/drivers/iio/adc/hi8435.c
+++ b/drivers/iio/adc/hi8435.c
@@ -476,13 +476,15 @@ static int hi8435_probe(struct spi_device *spi)
priv->spi = spi;
reset_gpio = devm_gpiod_get(>dev, NULL, GPIOD_OUT_LOW);
- if (IS_ERR(reset_gpio)) {
- /* chip s/w reset if h/w reset failed */
+ if (!IS_ERR(reset_gpio)) {
+ /* need >=100ns low pulse to reset chip */
+ gpiod_set_raw_value_cansleep(reset_gpio, 0);
+ udelay(1);
+ gpiod_set_raw_value_cansleep(reset_gpio, 1);
+ } else {
+ /* s/w reset chip if h/w reset is not available */
hi8435_writeb(priv, HI8435_CTRL_REG, HI8435_CTRL_SRST);
hi8435_writeb(priv, HI8435_CTRL_REG, 0);
- } else {
- udelay(5);
- gpiod_set_value(reset_gpio, 1);
}
spi_set_drvdata(spi, idev);
--
2.11.0
Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
Hi My view of your statement is: - you currently assume only a few cases for this driver - builtin UART in vf610, ls1012a and imx7, - in each of these cases, all lpuart instances share same endian, thus having that in global var works for these cases, - having that in global var makes it possible for you to write less lines of code My complain is: - in Linux, we are trying to keep drivers generic, - in Linux, having less lines of code has never been sufficient to break basic data structure consistency, - having driver to keep per-device capability in global var is a clear case of breaking consistency. >>> That's for special case, normally we wouldn't do that. >> >> For me this "special case" looks like "let's break data structure >> consistency to reuse several lines of code". >> >> With code snippets you show, it looks even worse: you assign same global >> variable in several places for different uses. > > If you mean lpuart_is_be, it's not for different uses. > The purpose is the same to align the correct endian but in two places. _probe() routine called for device X alters state already in use for device Y. > >> implicitly assuming that >> it is for same device. Which can be true in your current system, but not >> elsewhere (e.g. why not having lpuart programmed into fpga)? >> > > Sorry, What issues for fpga? Connect FPGA to IMX7 based system and program LS1012a version of lpuart core into it. Have your console on system UART broken at time when driver gets registered. > >> Alternative solution could be - have separate write path for earlycon. > > It looks to me having the same issue with a separate write patch > for earlycon as we still need distinguish Little or Big endian > for Layerscape and IMX. > >> At a glance, it is dozen lines of code. > > Would you please show some sample code? Do not reuse lpuart32_console_putchar() in earlycon code. Have two sets of early_setup/early_write/putchar - for BE and defaut-endian earlycon. And in these putchar's do not use lpuart_(read|write). As far as I can see, fsl_lpuart.c already has two drivers in one - there is separate set of routines for 8bit and 32bit cases. And those routines that are common, have if blocks that separate cases. I think these drivers will be cleaner if separated. However that's completely different story.
Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
Hi My view of your statement is: - you currently assume only a few cases for this driver - builtin UART in vf610, ls1012a and imx7, - in each of these cases, all lpuart instances share same endian, thus having that in global var works for these cases, - having that in global var makes it possible for you to write less lines of code My complain is: - in Linux, we are trying to keep drivers generic, - in Linux, having less lines of code has never been sufficient to break basic data structure consistency, - having driver to keep per-device capability in global var is a clear case of breaking consistency. >>> That's for special case, normally we wouldn't do that. >> >> For me this "special case" looks like "let's break data structure >> consistency to reuse several lines of code". >> >> With code snippets you show, it looks even worse: you assign same global >> variable in several places for different uses. > > If you mean lpuart_is_be, it's not for different uses. > The purpose is the same to align the correct endian but in two places. _probe() routine called for device X alters state already in use for device Y. > >> implicitly assuming that >> it is for same device. Which can be true in your current system, but not >> elsewhere (e.g. why not having lpuart programmed into fpga)? >> > > Sorry, What issues for fpga? Connect FPGA to IMX7 based system and program LS1012a version of lpuart core into it. Have your console on system UART broken at time when driver gets registered. > >> Alternative solution could be - have separate write path for earlycon. > > It looks to me having the same issue with a separate write patch > for earlycon as we still need distinguish Little or Big endian > for Layerscape and IMX. > >> At a glance, it is dozen lines of code. > > Would you please show some sample code? Do not reuse lpuart32_console_putchar() in earlycon code. Have two sets of early_setup/early_write/putchar - for BE and defaut-endian earlycon. And in these putchar's do not use lpuart_(read|write). As far as I can see, fsl_lpuart.c already has two drivers in one - there is separate set of routines for 8bit and 32bit cases. And those routines that are common, have if blocks that separate cases. I think these drivers will be cleaner if separated. However that's completely different story.
Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
>> Code should be consistent. >> > > Yes. > >> There is no good reason to have sport->lpuart32 inside sport, but >> lpuart_is_be outside of it. Both these values describe properties of >> particular device, and thus should be in per-device structure. >> > > That's for special case, normally we wouldn't do that. For me this "special case" looks like "let's break data structure consistency to reuse several lines of code". With code snippets you show, it looks even worse: you assign same global variable in several places for different uses. implicitly assuming that it is for same device. Which can be true in your current system, but not elsewhere (e.g. why not having lpuart programmed into fpga)? Alternative solution could be - have separate write path for earlycon. At a glance, it is dozen lines of code.
Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
>> Code should be consistent. >> > > Yes. > >> There is no good reason to have sport->lpuart32 inside sport, but >> lpuart_is_be outside of it. Both these values describe properties of >> particular device, and thus should be in per-device structure. >> > > That's for special case, normally we wouldn't do that. For me this "special case" looks like "let's break data structure consistency to reuse several lines of code". With code snippets you show, it looks even worse: you assign same global variable in several places for different uses. implicitly assuming that it is for same device. Which can be true in your current system, but not elsewhere (e.g. why not having lpuart programmed into fpga)? Alternative solution could be - have separate write path for earlycon. At a glance, it is dozen lines of code.
Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
> static u32 lpuart32_read(void __iomem *addr)
> {
> - return ioread32be(addr);
> + return lpuart_is_be ? ioread32be(addr) : readl(addr);
> }
>
> static void lpuart32_write(u32 val, void __iomem *addr)
> {
> - iowrite32be(val, addr);
> + if (lpuart_is_be)
> + iowrite32be(val, addr);
> + else
> + writel(val, addr);
> }
What if this is ever executed on big endian system?
>>>
>>> Sorry, not catching the point...
>>>
>>> What issues will meet?
>>
>> Isn't writel() in host endian?
>
> On big endian systems, it is supposed to run iowrite32be.
Your code states, "force BE if lpuart_is_be, don't care otherwise".
This semantics looks questionable for code reviewer.
If driver handles endian, should't it be explicit in both cases?
And if indeed driver means handling BE explicitly, but don't caring
otherwise, maybe variable name should suggest that (i.e. "force_be")?
Although driver maintainer could think differently. I won't insist on this.
Nikita
Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
> static u32 lpuart32_read(void __iomem *addr)
> {
> - return ioread32be(addr);
> + return lpuart_is_be ? ioread32be(addr) : readl(addr);
> }
>
> static void lpuart32_write(u32 val, void __iomem *addr)
> {
> - iowrite32be(val, addr);
> + if (lpuart_is_be)
> + iowrite32be(val, addr);
> + else
> + writel(val, addr);
> }
What if this is ever executed on big endian system?
>>>
>>> Sorry, not catching the point...
>>>
>>> What issues will meet?
>>
>> Isn't writel() in host endian?
>
> On big endian systems, it is supposed to run iowrite32be.
Your code states, "force BE if lpuart_is_be, don't care otherwise".
This semantics looks questionable for code reviewer.
If driver handles endian, should't it be explicit in both cases?
And if indeed driver means handling BE explicitly, but don't caring
otherwise, maybe variable name should suggest that (i.e. "force_be")?
Although driver maintainer could think differently. I won't insist on this.
Nikita
Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
>>> @@ -2000,6 +2007,7 @@ static int lpuart_probe(struct platform_device *pdev) >>> } >>> sport->port.line = ret; >>> sport->lpuart32 = sdata->is_32; >>> + lpuart_is_be = sdata->is_be; >> >> Setting a global variable in per-device routine is quite bad design. >> > > There is a reason for that we don't want to change the exist > lpuart32_read[write] API which is widely used in driver. > Making a global lpuart_is_be is the simplest way to do it. > > Any strong blocking reason? Code should be consistent. There is no good reason to have sport->lpuart32 inside sport, but lpuart_is_be outside of it. Both these values describe properties of particular device, and thus should be in per-device structure. If that implies adding sport arg to lpuart32_(read|write), just do that.
Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
>>> @@ -2000,6 +2007,7 @@ static int lpuart_probe(struct platform_device *pdev) >>> } >>> sport->port.line = ret; >>> sport->lpuart32 = sdata->is_32; >>> + lpuart_is_be = sdata->is_be; >> >> Setting a global variable in per-device routine is quite bad design. >> > > There is a reason for that we don't want to change the exist > lpuart32_read[write] API which is widely used in driver. > Making a global lpuart_is_be is the simplest way to do it. > > Any strong blocking reason? Code should be consistent. There is no good reason to have sport->lpuart32 inside sport, but lpuart_is_be outside of it. Both these values describe properties of particular device, and thus should be in per-device structure. If that implies adding sport arg to lpuart32_(read|write), just do that.
Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
17.05.2017 06:39, Dong Aisheng wrote:
> On Tue, May 16, 2017 at 02:15:08PM +0300, Nikita Yushchenko wrote:
>>> static u32 lpuart32_read(void __iomem *addr)
>>> {
>>> - return ioread32be(addr);
>>> + return lpuart_is_be ? ioread32be(addr) : readl(addr);
>>> }
>>>
>>> static void lpuart32_write(u32 val, void __iomem *addr)
>>> {
>>> - iowrite32be(val, addr);
>>> + if (lpuart_is_be)
>>> + iowrite32be(val, addr);
>>> + else
>>> + writel(val, addr);
>>> }
>>
>> What if this is ever executed on big endian system?
>>
>
> Sorry, not catching the point...
>
> What issues will meet?
Isn't writel() in host endian?
Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
17.05.2017 06:39, Dong Aisheng wrote:
> On Tue, May 16, 2017 at 02:15:08PM +0300, Nikita Yushchenko wrote:
>>> static u32 lpuart32_read(void __iomem *addr)
>>> {
>>> - return ioread32be(addr);
>>> + return lpuart_is_be ? ioread32be(addr) : readl(addr);
>>> }
>>>
>>> static void lpuart32_write(u32 val, void __iomem *addr)
>>> {
>>> - iowrite32be(val, addr);
>>> + if (lpuart_is_be)
>>> + iowrite32be(val, addr);
>>> + else
>>> + writel(val, addr);
>>> }
>>
>> What if this is ever executed on big endian system?
>>
>
> Sorry, not catching the point...
>
> What issues will meet?
Isn't writel() in host endian?
Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
> static u32 lpuart32_read(void __iomem *addr)
> {
> - return ioread32be(addr);
> + return lpuart_is_be ? ioread32be(addr) : readl(addr);
> }
>
> static void lpuart32_write(u32 val, void __iomem *addr)
> {
> - iowrite32be(val, addr);
> + if (lpuart_is_be)
> + iowrite32be(val, addr);
> + else
> + writel(val, addr);
> }
What if this is ever executed on big endian system?
Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
> static u32 lpuart32_read(void __iomem *addr)
> {
> - return ioread32be(addr);
> + return lpuart_is_be ? ioread32be(addr) : readl(addr);
> }
>
> static void lpuart32_write(u32 val, void __iomem *addr)
> {
> - iowrite32be(val, addr);
> + if (lpuart_is_be)
> + iowrite32be(val, addr);
> + else
> + writel(val, addr);
> }
What if this is ever executed on big endian system?
Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
> @@ -2000,6 +2007,7 @@ static int lpuart_probe(struct platform_device *pdev) > } > sport->port.line = ret; > sport->lpuart32 = sdata->is_32; > + lpuart_is_be = sdata->is_be; Setting a global variable in per-device routine is quite bad design.
Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
> @@ -2000,6 +2007,7 @@ static int lpuart_probe(struct platform_device *pdev) > } > sport->port.line = ret; > sport->lpuart32 = sdata->is_32; > + lpuart_is_be = sdata->is_be; Setting a global variable in per-device routine is quite bad design.
Re: [PATCH] pinctrl: when claiming hog, skip maps not served by same device
>> +/* >> + * If pctldev is not null, we are claiming hog for it, >> + * that means, setting that is served by pctldev by itself. >> + * >> + * Thus we must skip map that is for this device but is served >> + * by other device. >> + */ >> +if (pctldev && >> +strcmp(dev_name(pctldev->dev), map->ctrl_dev_name)) >> +continue; >> >> ret = add_setting(p, pctldev, map); >> /* >> -- > > Maybe add a comment saying pctldev is NULL in the regular case > and only exists in the hog case? Isn't comment above saying exactly that?
Re: [PATCH] pinctrl: when claiming hog, skip maps not served by same device
>> +/* >> + * If pctldev is not null, we are claiming hog for it, >> + * that means, setting that is served by pctldev by itself. >> + * >> + * Thus we must skip map that is for this device but is served >> + * by other device. >> + */ >> +if (pctldev && >> +strcmp(dev_name(pctldev->dev), map->ctrl_dev_name)) >> +continue; >> >> ret = add_setting(p, pctldev, map); >> /* >> -- > > Maybe add a comment saying pctldev is NULL in the regular case > and only exists in the hog case? Isn't comment above saying exactly that?
[PATCH] pinctrl: when claiming hog, skip maps not served by same device
When pinctrl device registers, it automatically claims hogs, that is,
maps that pinctrl device serves for itself.
It is possible that in addition to SoC's pinctrl device, other pinctrl
devices get registered. E.g. some gpio expander devies are registered
as pinctrl devices. For such devices, pinctrl maps could be defined
that set up SoC's pins (e.g. interrupt pin for gpio expander). Such
a map will have target device set to gpio expander.
Here is device tree snippet that causes this scenario:
{
sx1503@20 {
compatible = "semtech,sx1503q";
pinctrl-names = "default";
pinctrl-0 = <_sx1503_20>;
...
};
};
...
{
pinctrl_sx1503_20: pinctrl-sx1503-20 {
fsl,pins = <
VF610_PAD_PTB1__GPIO_23 0x219d
>;
};
};
Such a map will have target device set to gpio expander. However is not
a hog, it is a regular map that is claimed by core before gpio expander
device is probed.
Thus when looking for hogs, it is not enough to check that map's target
device is set to pinctrl device being registered. Need also check that
map's control device is also set to the same.
Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com>
---
drivers/pinctrl/core.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 1653cbda6a82..682ebd360030 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1038,6 +1038,16 @@ static struct pinctrl *create_pinctrl(struct device *dev,
/* Map must be for this device */
if (strcmp(map->dev_name, devname))
continue;
+ /*
+* If pctldev is not null, we are claiming hog for it,
+* that means, setting that is served by pctldev by itself.
+*
+* Thus we must skip map that is for this device but is served
+* by other device.
+*/
+ if (pctldev &&
+ strcmp(dev_name(pctldev->dev), map->ctrl_dev_name))
+ continue;
ret = add_setting(p, pctldev, map);
/*
--
2.11.0
[PATCH] pinctrl: when claiming hog, skip maps not served by same device
When pinctrl device registers, it automatically claims hogs, that is,
maps that pinctrl device serves for itself.
It is possible that in addition to SoC's pinctrl device, other pinctrl
devices get registered. E.g. some gpio expander devies are registered
as pinctrl devices. For such devices, pinctrl maps could be defined
that set up SoC's pins (e.g. interrupt pin for gpio expander). Such
a map will have target device set to gpio expander.
Here is device tree snippet that causes this scenario:
{
sx1503@20 {
compatible = "semtech,sx1503q";
pinctrl-names = "default";
pinctrl-0 = <_sx1503_20>;
...
};
};
...
{
pinctrl_sx1503_20: pinctrl-sx1503-20 {
fsl,pins = <
VF610_PAD_PTB1__GPIO_23 0x219d
>;
};
};
Such a map will have target device set to gpio expander. However is not
a hog, it is a regular map that is claimed by core before gpio expander
device is probed.
Thus when looking for hogs, it is not enough to check that map's target
device is set to pinctrl device being registered. Need also check that
map's control device is also set to the same.
Signed-off-by: Nikita Yushchenko
---
drivers/pinctrl/core.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 1653cbda6a82..682ebd360030 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1038,6 +1038,16 @@ static struct pinctrl *create_pinctrl(struct device *dev,
/* Map must be for this device */
if (strcmp(map->dev_name, devname))
continue;
+ /*
+* If pctldev is not null, we are claiming hog for it,
+* that means, setting that is served by pctldev by itself.
+*
+* Thus we must skip map that is for this device but is served
+* by other device.
+*/
+ if (pctldev &&
+ strcmp(dev_name(pctldev->dev), map->ctrl_dev_name))
+ continue;
ret = add_setting(p, pctldev, map);
/*
--
2.11.0
Re: pinctrl-sx150x.c broken in 4.11
>>> Maybe create_pinctrl() could check if the pin controller device >>> for a potential hog points to the device itself and bail out >>> if that's not the case? >> >> Well that's exactly what patch from my first mail in this thread does. >> This indeed fixes my case, but I don't know if it is correct in generic >> case. > > OK, yeah I just looked it up as I was not in cc. > >> Should I submit it? Do you ack? > > Yeah please submit a proper patch. I assume you already checked > that this change only affects the pinctrl hogs only, not regular > device pins? I'd assume so as it's in create_pinctrl().. Regular device pins go via pinctrl_get() which calls create_pinctrl() with second argument set to NULL.
Re: pinctrl-sx150x.c broken in 4.11
>>> Maybe create_pinctrl() could check if the pin controller device >>> for a potential hog points to the device itself and bail out >>> if that's not the case? >> >> Well that's exactly what patch from my first mail in this thread does. >> This indeed fixes my case, but I don't know if it is correct in generic >> case. > > OK, yeah I just looked it up as I was not in cc. > >> Should I submit it? Do you ack? > > Yeah please submit a proper patch. I assume you already checked > that this change only affects the pinctrl hogs only, not regular > device pins? I'd assume so as it's in create_pinctrl().. Regular device pins go via pinctrl_get() which calls create_pinctrl() with second argument set to NULL.
Re: pinctrl-sx150x.c broken in 4.11
>>> Hmm maybe yeah. I don't quite follow the above the "pinctrl-0 property
>>> of sx150x device tree node, is misinterpreted as hog" part though.
>>
>> sx150x is i2c-gpio device. It has 16 GPIO lines that are communicated
>> with via i2c bus, and an interrupt line.
>>
>> Interrupt line is typically connected to SoC's pin.
>> This pin has to be configured.
>> This is done by providing appropriate subnode in SoC's pinmux node, with
>> information with pin configuration, and pinctrl-0 property in sx150x's
>> node with phandle to that subnode:
>>
>> ...
>> {
>> sx1503@20 {
>> compatible = "semtech,sx1503q";
>> pinctrl-names = "default";
>> pinctrl-0 = <_sx1503_20>;
>> ...
>> };
>> };
>> ...
>> {
>> pinctrl_sx1503_20: pinctrl-sx1503-20 {
>> fsl,pins = <
>> VF610_PAD_PTB1__GPIO_23 0x219d
>> >;
>> };
>> };
>>
>> This pin configuration is handled by driver core, i.e. before probe()
>> for sx150x is called, core applies pin configuration.
>>
>> However sx150x driver is currently implemented as a pinctrl driver.
>>
>> When it initializes, pinctrl searches for "hog", i.e. pin config that
>> should be applied at driver registration time.
>>
>> While doing so, core searches for any registered pinctrl_map for device
>> being register. Search loop is in create_pinctrl().
>>
>> In this case, this loop finds map that is defined above.
>>
>> This is *not* hog. This is pin setting already applied in SoC's pinmux
>> controller for sx1503 device.
>>
>> However code in create_pinctrl() tries to apply it, and use sx1503's
>> methods to do so. Which is plain wrong and errors out.
>
> Maybe create_pinctrl() could check if the pin controller device
> for a potential hog points to the device itself and bail out
> if that's not the case?
Well that's exactly what patch from my first mail in this thread does.
This indeed fixes my case, but I don't know if it is correct in generic
case.
Should I submit it? Do you ack?
Nikita
