Re: [PATCH v2 1/3] mux: gpio: Use bitmap API instead of direct assignment
Hi Greg,
You can pick this series directly, right? I don't feel a compelling need to
add the patches to -next before you take them since they are simple enough...
And drivers/mux is otherwise quiet, as usual.
Cheers,
Peter
On 2021-03-30 21:33, Andy Shevchenko wrote:
> Assigning bitmaps like it's done in the driver might be error prone.
> Fix this by using bitmap API.
>
> Signed-off-by: Andy Shevchenko
> Acked-by: Peter Rosin
> ---
> v2: left blank line untouched (Peter)
> drivers/mux/gpio.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mux/gpio.c b/drivers/mux/gpio.c
> index 02c1f2c014e8..d1b4aa923657 100644
> --- a/drivers/mux/gpio.c
> +++ b/drivers/mux/gpio.c
> @@ -7,6 +7,7 @@
> * Author: Peter Rosin
> */
>
> +#include
> #include
> #include
> #include
> @@ -23,8 +24,9 @@ static int mux_gpio_set(struct mux_control *mux, int state)
> {
> struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
> DECLARE_BITMAP(values, BITS_PER_TYPE(state));
> + u32 value = state;
>
> - values[0] = state;
> + bitmap_from_arr32(values, , BITS_PER_TYPE(value));
>
> gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
> mux_gpio->gpios->desc,
> @@ -71,7 +73,7 @@ static int mux_gpio_probe(struct platform_device *pdev)
> return ret;
> }
> WARN_ON(pins != mux_gpio->gpios->ndescs);
> - mux_chip->mux->states = 1 << pins;
> + mux_chip->mux->states = BIT(pins);
>
> ret = device_property_read_u32(dev, "idle-state", (u32 *)_state);
> if (ret >= 0 && idle_state != MUX_IDLE_AS_IS) {
>
Re: [PATCH v1 3/3] mux: gpio: Simplify code by using dev_err_probe()
Hi!
On 2021-03-26 18:24, Andy Shevchenko wrote:
> Use already prepared dev_err_probe() introduced by the commit
> a787e5400a1c ("driver core: add device probe log helper").
> It simplifies EPROBE_DEFER handling.
>
> Signed-off-by: Andy Shevchenko
> ---
> drivers/mux/gpio.c | 8 ++--
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mux/gpio.c b/drivers/mux/gpio.c
> index e5ef9284e2b4..c3036bfffd50 100644
> --- a/drivers/mux/gpio.c
> +++ b/drivers/mux/gpio.c
> @@ -65,12 +65,8 @@ static int mux_gpio_probe(struct platform_device *pdev)
> mux_chip->ops = _gpio_ops;
>
> mux_gpio->gpios = devm_gpiod_get_array(dev, "mux", GPIOD_OUT_LOW);
> - if (IS_ERR(mux_gpio->gpios)) {
> - ret = PTR_ERR(mux_gpio->gpios);
> - if (ret != -EPROBE_DEFER)
> - dev_err(dev, "failed to get gpios\n");
> - return ret;
> - }
> + if (IS_ERR(mux_gpio->gpios))
> + return dev_err_probe(dev, PTR_ERR(mux_gpio->gpios), "failed to
> get gpios\n");
Please break this line, keeping it as one line does not significantly
increase readability. I know many people think long lines are super
nice, but I'm not sold and am stubbornly sticking to 80 cols. I'd rater
have room for one more window instead of wasting loads of screen on
mostly short lines and a long one here and there. Sorry to be a pest,
but coding-style.rst agrees with me:
"The preferred limit on the length of a single line is 80 columns."
So, with that changed,
Acked-by: Peter Rosin
Cheers,
Peter
> WARN_ON(pins != mux_gpio->gpios->ndescs);
> mux_chip->mux->states = BIT(pins);
>
>
Re: [PATCH v1 2/3] mux: gpio: Make it OF independent
Hi!
On 2021-03-26 18:24, Andy Shevchenko wrote:
> Module doesn't use OF APIs anyhow, make it OF independent by replacing
> headers and dropping useless of_match_ptr() call.
>
> Signed-off-by: Andy Shevchenko
> ---
> drivers/mux/gpio.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mux/gpio.c b/drivers/mux/gpio.c
> index 891ee0274733..e5ef9284e2b4 100644
> --- a/drivers/mux/gpio.c
> +++ b/drivers/mux/gpio.c
> @@ -10,8 +10,8 @@
> #include
> #include
> #include
> +#include
mod_devicetable.h sorts before module.h. With that changed,
Acked-by: Peter Rosin
Cheers,
Peter
> #include
> -#include
> #include
> #include
>
> @@ -97,7 +97,7 @@ static int mux_gpio_probe(struct platform_device *pdev)
> static struct platform_driver mux_gpio_driver = {
> .driver = {
> .name = "gpio-mux",
> - .of_match_table = of_match_ptr(mux_gpio_dt_ids),
> + .of_match_table = mux_gpio_dt_ids,
> },
> .probe = mux_gpio_probe,
> };
>
Re: [PATCH v1 1/3] mux: gpio: Use bitmap API instead of direct assignment
Hi!
On 2021-03-26 18:23, Andy Shevchenko wrote:
> Assigning bitmaps like it's done in the driver might be error prone.
> Fix this by using bitmap API.
A bit strongly worded perhaps, since the size of a mux chip with
anywhere near 31 inputs and 2^31 possible selections is a bit
ridiculous. Please send a photo of that HW if someone is producing
one :-)
But there's always the someone-borrows-code-factor, so I guess it's
fine as-is.
> Signed-off-by: Andy Shevchenko
> ---
> drivers/mux/gpio.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mux/gpio.c b/drivers/mux/gpio.c
> index 02c1f2c014e8..891ee0274733 100644
> --- a/drivers/mux/gpio.c
> +++ b/drivers/mux/gpio.c
> @@ -6,7 +6,7 @@
> *
> * Author: Peter Rosin
> */
> -
Nit, please keep the empty line here. With that,
Acked-by: Peter Rosin
And please send directly to Greg, that would be excellent, thanks!
Cheers,
Peter
> +#include
> #include
> #include
> #include
> @@ -23,8 +23,9 @@ static int mux_gpio_set(struct mux_control *mux, int state)
> {
> struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
> DECLARE_BITMAP(values, BITS_PER_TYPE(state));
> + u32 value = state;
>
> - values[0] = state;
> + bitmap_from_arr32(values, , BITS_PER_TYPE(value));
>
> gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
> mux_gpio->gpios->desc,
> @@ -71,7 +72,7 @@ static int mux_gpio_probe(struct platform_device *pdev)
> return ret;
> }
> WARN_ON(pins != mux_gpio->gpios->ndescs);
> - mux_chip->mux->states = 1 << pins;
> + mux_chip->mux->states = BIT(pins);
>
> ret = device_property_read_u32(dev, "idle-state", (u32 *)_state);
> if (ret >= 0 && idle_state != MUX_IDLE_AS_IS) {
>
Re: [RESEND PATCH v3 1/2] i2c: i2c-mux-gpio: Factor out pdev->dev in _probe_dt()
On 2020-11-19 00:40, Evan Green wrote: > Factor out >dev into a local variable in preparation for > the ACPI enablement of this function, which will utilize the variable > more. > > Signed-off-by: Evan Green > --- > > Changes in v3: > - Introduced minor >dev to dev refactor (Peter) > > drivers/i2c/muxes/i2c-mux-gpio.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) Acked-by: Peter Rosin (this patch doesn't make much sense without 2/2) Cheers, Peter
Re: [RESEND PATCH v3 2/2] i2c: i2c-mux-gpio: Enable this driver in ACPI land
On 2021-01-14 20:53, Wolfram Sang wrote: >> Can this be accepted as-is, or should I resend? > > Peter, can you have a look here as well? I have no issues, but I was waiting for a response from Andy here. I have little knowledge of ACPI, and have previously made ignorant mistakes in that area. I would greatly appreciate Andy following through with his line of thinking... So, if we ignore Andys review comments, then: Acked-by: Peter Rosin Cheers, Peter
Re: [PATCH 4/7] dt-bindings: ti-serdes-mux: Add defines for AM64 SoC
Hi! On 2020-12-24 12:42, Kishon Vijay Abraham I wrote: > AM64 has a single lane SERDES which can be configured to be used > with either PCIe or USB. Define the possilbe values for the SERDES > function in AM64 SoC here. > > Signed-off-by: Kishon Vijay Abraham I > --- > include/dt-bindings/mux/ti-serdes.h | 4 > 1 file changed, 4 insertions(+) > > diff --git a/include/dt-bindings/mux/ti-serdes.h > b/include/dt-bindings/mux/ti-serdes.h > index 9047ec6bd3cf..68e0f76deed1 100644 > --- a/include/dt-bindings/mux/ti-serdes.h > +++ b/include/dt-bindings/mux/ti-serdes.h > @@ -90,4 +90,8 @@ > #define J7200_SERDES0_LANE3_USB 0x2 > #define J7200_SERDES0_LANE3_IP4_UNUSED 0x3 > > +/* AM64 */ In case you end up keeping these defines, despite the comment by Rob... Nitpick, the J721E and J7200 sections have a blank line here, between the header comment and the actual defines. But mehh... Acked-by: Peter Rosin Cheers, Peter > +#define AM64_SERDES0_LANE0_PCIE0 0x0 > +#define AM64_SERDES0_LANE0_USB 0x1 > + > #endif /* _DT_BINDINGS_MUX_TI_SERDES */ >
Re: [PATCH] drm/fb-helper: Add missed unlocks in setcmap_legacy()
Hi!
On 2020-12-03 15:42, Chuhong Yuan wrote:
> setcmap_legacy() does not call drm_modeset_unlock_all() in some exits,
> add the missed unlocks with goto to fix it.
>
> Fixes: 964c60063bff ("drm/fb-helper: separate the fb_setcmap helper into
> atomic and legacy paths")
> Signed-off-by: Chuhong Yuan
Yup, my patch fumbled the locking. Sorry, and thanks for cleaning up my mess!
Acked-by: Peter Rosin
(Spelled that as Ached-by at first, what does that mean??)
Cheers,
Peter
> ---
> drivers/gpu/drm/drm_fb_helper.c | 15 ++-
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 1543d9d10970..8033467db4be 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -923,11 +923,15 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct
> fb_info *info)
> drm_modeset_lock_all(fb_helper->dev);
> drm_client_for_each_modeset(modeset, _helper->client) {
> crtc = modeset->crtc;
> - if (!crtc->funcs->gamma_set || !crtc->gamma_size)
> - return -EINVAL;
> + if (!crtc->funcs->gamma_set || !crtc->gamma_size) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> - if (cmap->start + cmap->len > crtc->gamma_size)
> - return -EINVAL;
> + if (cmap->start + cmap->len > crtc->gamma_size) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> r = crtc->gamma_store;
> g = r + crtc->gamma_size;
> @@ -940,8 +944,9 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct
> fb_info *info)
> ret = crtc->funcs->gamma_set(crtc, r, g, b,
>crtc->gamma_size, NULL);
> if (ret)
> - return ret;
> + goto out;
> }
> +out:
> drm_modeset_unlock_all(fb_helper->dev);
>
> return ret;
>
Re: [PATCH v2] i2c: i2c-mux-gpio: Enable this driver in ACPI land
Hi!
On 2020-10-15 03:02, Evan Green wrote:
> Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state
> property translates directly to a fwnode_property_*() call. The child
> reg property translates naturally into _ADR in ACPI.
>
> The i2c-parent binding is a relic from the days when all direct children
> of an i2c controller in Linux had to be i2c devices. These days that
I2C controller. I2C devices.
I fail to see why this "relic" has to be explicitly blamed on Linux? In the
beginning, the bindings for all I2C controllers (sometimes implicitely,
sometimes explicitely) specified that all child nodes had to be I2C devices.
The *bindings* were simply not as flexible before the i2c-bus subnode was
invented only a few years ago. So, there are arguments that the "problem"
was in DT-land and that Linux just followed suit.
> implementation detail has been worked out, so the i2c-mux can sit
> as a direct child of its parent controller, which is where it makes the
> most sense from a hardware description perspective. For the ACPI
> implementation we'll assume that's always how the i2c-mux-gpio is
> instantiated.
There is potential to match this and make i2c-parent optional for the
DT case and require it to be a child of its parent in such cases, if
someone has the time/energy...
>
> Signed-off-by: Evan Green
> ---
>
> Changes in v2:
> - Make it compile properly when !CONFIG_ACPI (Randy)
> - Update commit message regarding i2c-parent (Peter)
>
> drivers/i2c/muxes/i2c-mux-gpio.c | 103 ++-
> 1 file changed, 75 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c
> b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 4effe563e9e8d..8e4008f4a9b5d 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -49,34 +49,80 @@ static int i2c_mux_gpio_deselect(struct i2c_mux_core
> *muxc, u32 chan)
> return 0;
> }
>
> -#ifdef CONFIG_OF
> -static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
> - struct platform_device *pdev)
> +#ifdef CONFIG_ACPI
> +
> +static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
> + struct fwnode_handle *fwdev,
> + unsigned int *adr)
> +
> +{
> + unsigned long long adr64;
> + acpi_status status;
> +
> + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwdev),
> +METHOD_NAME__ADR,
> +NULL, );
> +
> + if (!ACPI_SUCCESS(status)) {
> + dev_err(dev, "Cannot get address");
Missing trailing \n
> + return -EINVAL;
> + }
> +
> + *adr = adr64;
Maybe this is too pedantic? Optional, ignore if I'm just being insane...
if (*adr != adr64) {
dev_err(dev, "Address out of range\n");
return -EINVAL;
}
> + return 0;
> +}
> +
> +#else
> +
> +static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
> + struct fwnode_handle *fwdev,
> + unsigned int *adr)
> +{
> + return -EINVAL;
> +}
> +
> +#endif
> +
> +static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
> + struct platform_device *pdev)
> {
> - struct device_node *np = pdev->dev.of_node;
> - struct device_node *adapter_np, *child;
> - struct i2c_adapter *adapter;
> + struct device *dev = >dev;
> + struct device_node *np = dev->of_node;
> + acpi_handle dev_handle;
Remove the dev_handle declaration here...(push)...
> + struct device_node *adapter_np;
> + struct i2c_adapter *adapter = NULL;
> + struct fwnode_handle *child = NULL;
Why do you need these two " = NULL" here? I can't believe compilers are
that stupid? If they are, fine. But otherwise, why single out these two
pointers and add NULL only there and not everywhere? But NULL everywhere
would be ugly...
> unsigned *values;
> - int i = 0;
> + int rc, i = 0;
>
> - if (!np)
> - return -ENODEV;
> + if (is_of_node(dev->fwnode)) {
> + if (!np)
> + return -ENODEV;
>
> - adapter_np = of_parse_phandle(np, "i2c-parent", 0);
> - if (!adapter_np) {
> - dev_err(>dev, "Cannot parse i2c-parent\n");
> - return -ENODEV;
> + adapter_np = of_parse_phandle(np, "i2c-parent", 0);
> + if (!adapter_np) {
> + dev_err(>dev, "Cannot parse i2c-parent\n");
You should do ">dev" -> "dev" here, because I hate having
the dev variable and then not use it. But that should perhaps be
a preparatory patch, because I see more instances and this is an
orthogonal change.
> + return -ENODEV;
> + }
> + adapter = of_find_i2c_adapter_by_node(adapter_np);
> + of_node_put(adapter_np);
> +
> + } else if
Re: using one gpio for multiple dht22 sensors
On 2020-10-14 11:12, Peter Rosin wrote: > Hi Rasnus, Rasmus. *Blush* Cheers, Peter
Re: using one gpio for multiple dht22 sensors
Hi Rasnus, On 2020-10-13 23:34, Rasmus Villemoes wrote: > Hi Peter > > Since you're the author of io-channel-mux.txt, gpio-mux.txt and > mux-controller.txt, I hope you don't mind me asking some perhaps silly > questions. Right, I ended up being the maintainer for a bunch of code I needed to do things "the right way", i.e. without resorting to gpio-manipulation and other breaches of abstractions from user-space... > I'm going to hook up a bunch of dht22 humidity (and temperature) sensors > [1] (drivers/iio/humidity/dht11.c), but partly due to limited number of > available gpios, I'm also going to use a 74hc4051 multiplexer [2], so > that all the dht22's actually sit on the same gpio. > > It's pretty obvious how the multiplexer is to be described in > device-tree (I'm probably going to send a patch to add support for an > optional "enable-gpio", as on the 74hc4051, so that MUX_IDLE_DISCONNECT > gets supported). > > It also seems like io-channel-mux should somehow magically apply to all > kinds of iio devices, but it can't be that simple. And if it is, I can't > figure out how to write the DT. So: The io-channel-mux is for the situation where you have only one iio-device, and where you can control its environment with a mux. I.e. it is not about how the device communicates with the host. You then get one new "virtual" iio-device for every (valid) state of the mux, and when you access those iio-devices, the mux is configured to select the correct input/output for the iio-device in question. At least, it should be possible for output devices as well, but I guess you kind of get into trouble with the output signal not persisting when the mux changes state, but that is a problem for the HW people ;-). I originally used it for a single adc device where the mux simply selected what to measure. > - do I need to teach the dht11.c driver to be mux-aware? > - currently, a dht11/dht22 shows up in sysfs with just two files, > in_humidityrelative_input and in_temp_input. Now, should I end up with > one device and, say, 2*8 files (so that it seems like one sensor with > eight input channels), or should it show up as eight different devices? > If the latter, I guess the device tree would end up with the same "gpios > = " spec for all eight - is that even allowed? It's not 100% clear to me how this is connected, but I'm guessing that you have the "DATA-signal" pin of the dht22s connected to the Y pins of the 4051, and that Z pin of the 4051 is connected to some gpio, so that you are able to communicate with the various dht22s by controlling the mux. This calls for a mux on the single source of communication from the host to the various real devices that share this single source of communication. In other words, more like an i2c-mux than an io-channel-mux. I.e., what you need is "the other kind" of mux/gpio driver, i.e. a driver that "fans out" a single gpio so that other drivers can treat the pins on the other side of the mux as gpios. I guess you'd have to sort out irq-routing through this new mux-gpio driver since the dht22 driver uses irqs to read input, and there are probably various other interesting problems as well. Maybe the dgt22 driver needs to be adjusted depending on how these problems are handled? E.g. maybe it needs to disable IRQs when it is not expecting communication or something. I'm not an expert on the intricacies of the gpio subsystem... Does that help? Cheers, Peter > If you can show how the DT is supposed to describe this situation, I'll > try to fill out the missing pieces on the driver side. > > [I could also just not describe the multiplexer at all and control > everything about it by toggling gpios from userspace, and just have a > single dht22 in DT, but I prefer doing this "the right way" if it's > feasible.] > > Thanks, > Rasmus > > Just FTR: > > [1] > https://www.electroschematics.com/wp-content/uploads/2015/02/DHT22-datasheet.pdf > [2] https://assets.nexperia.com/documents/data-sheet/74HC_HCT4051.pdf >
Re: [PATCH] i2c: i2c-mux-gpio: Enable this driver in ACPI land
Hi!
On 2020-10-10 00:43, Evan Green wrote:
> Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state
> property translates directly to a fwnode_property_*() call. The child
> reg property translates naturally into _ADR in ACPI.
>
> The i2c-parent is a little trickier, since of's phandle definition
> suggests the i2c mux could live in a completely different part of
> the tree than its upstream i2c controller. For now in ACPI,
This is so since the I2C gpio-mux predates the "i2c-bus" sub-node of
I2C controllers. At that time *all* sub-nodes of I2C controllers
represented I2C client device, IIUC. With that knowledge, you could
perhaps rephrase the above?
Also, a nit below.
> just assume that the i2c-mux-gpio device will always be a direct
> child of the i2c controller. If the additional flexibility of
> defining muxes in wildly different locations from their parent
> controllers is required, it can be added later.
>
> Signed-off-by: Evan Green
> ---
>
> drivers/i2c/muxes/i2c-mux-gpio.c | 77 +---
> 1 file changed, 50 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c
> b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 4effe563e9e8d..f195e95e8a037 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -49,34 +49,46 @@ static int i2c_mux_gpio_deselect(struct i2c_mux_core
> *muxc, u32 chan)
> return 0;
> }
>
> -#ifdef CONFIG_OF
> -static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
> - struct platform_device *pdev)
> +static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
> + struct platform_device *pdev)
> {
> - struct device_node *np = pdev->dev.of_node;
> - struct device_node *adapter_np, *child;
> - struct i2c_adapter *adapter;
> + struct device *dev = >dev;
> + struct device_node *np = dev->of_node;
> + acpi_handle dev_handle;
> + struct device_node *adapter_np;
> + struct i2c_adapter *adapter = NULL;
> + struct fwnode_handle *child = NULL;
> unsigned *values;
> int i = 0;
>
> - if (!np)
> - return -ENODEV;
> + if (is_of_node(dev->fwnode)) {
> + if (!np)
> + return -ENODEV;
>
> - adapter_np = of_parse_phandle(np, "i2c-parent", 0);
> - if (!adapter_np) {
> - dev_err(>dev, "Cannot parse i2c-parent\n");
> - return -ENODEV;
> + adapter_np = of_parse_phandle(np, "i2c-parent", 0);
> + if (!adapter_np) {
> + dev_err(>dev, "Cannot parse i2c-parent\n");
> + return -ENODEV;
> + }
> + adapter = of_find_i2c_adapter_by_node(adapter_np);
> + of_node_put(adapter_np);
> +
> + } else if (is_acpi_node(dev->fwnode)) {
> + /*
> + * In ACPI land the mux should be a direct child of the i2c
> + * bus it muxes.
> + */
> + dev_handle = ACPI_HANDLE(dev->parent);
> + adapter = i2c_acpi_find_adapter_by_handle(dev_handle);
> }
> - adapter = of_find_i2c_adapter_by_node(adapter_np);
> - of_node_put(adapter_np);
> +
> if (!adapter)
> return -EPROBE_DEFER;
>
> mux->data.parent = i2c_adapter_id(adapter);
> put_device(>dev);
>
> - mux->data.n_values = of_get_child_count(np);
> -
> + mux->data.n_values = device_get_child_node_count(dev);
> values = devm_kcalloc(>dev,
> mux->data.n_values, sizeof(*mux->data.values),
> GFP_KERNEL);
> @@ -85,24 +97,35 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
> return -ENOMEM;
> }
>
> - for_each_child_of_node(np, child) {
> - of_property_read_u32(child, "reg", values + i);
> + device_for_each_child_node(dev, child) {
> + if (is_of_node(child)) {
> + fwnode_property_read_u32(child, "reg", values + i);
> +
> + } else if (is_acpi_node(child)) {
> + unsigned long long adr;
> + acpi_status status;
> +
> + status =
> acpi_evaluate_integer(ACPI_HANDLE_FWNODE(child),
> +METHOD_NAME__ADR,
> +NULL, );
> + if (ACPI_SUCCESS(status)) {
> + *(values + i) = adr;
I would write that as
values[i] = adr;
Cheers,
Peter
> +
> + } else {
> + dev_err(dev, "Cannot get address");
> + return -EINVAL;
> + }
> + }
> +
> i++;
> }
> mux->data.values = values;
>
> - if (of_property_read_u32(np, "idle-state", >data.idle))
> + if
Re: [PATCH v4 1/6] dt-bindings: ti-serdes-mux: Add defines for J7200 SoC
Hi! On 2020-09-21 16:39, Roger Quadros wrote: > There are 4 lanes in each J7200 SERDES. Each SERDES lane mux can > select upto 4 different IPs. Define all the possible functions. > > Cc: Peter Rosin > Signed-off-by: Roger Quadros Acked-by: Peter Rosin Thanks for taking care of this! Cheers, Peter > --- > include/dt-bindings/mux/ti-serdes.h | 22 ++ > 1 file changed, 22 insertions(+) > > diff --git a/include/dt-bindings/mux/ti-serdes.h > b/include/dt-bindings/mux/ti-serdes.h > index 146d0685a925..9047ec6bd3cf 100644 > --- a/include/dt-bindings/mux/ti-serdes.h > +++ b/include/dt-bindings/mux/ti-serdes.h > @@ -68,4 +68,26 @@ > #define J721E_SERDES4_LANE3_QSGMII_LANE8 0x2 > #define J721E_SERDES4_LANE3_IP4_UNUSED 0x3 > > +/* J7200 */ > + > +#define J7200_SERDES0_LANE0_QSGMII_LANE3 0x0 > +#define J7200_SERDES0_LANE0_PCIE1_LANE0 0x1 > +#define J7200_SERDES0_LANE0_IP3_UNUSED 0x2 > +#define J7200_SERDES0_LANE0_IP4_UNUSED 0x3 > + > +#define J7200_SERDES0_LANE1_QSGMII_LANE4 0x0 > +#define J7200_SERDES0_LANE1_PCIE1_LANE1 0x1 > +#define J7200_SERDES0_LANE1_IP3_UNUSED 0x2 > +#define J7200_SERDES0_LANE1_IP4_UNUSED 0x3 > + > +#define J7200_SERDES0_LANE2_QSGMII_LANE1 0x0 > +#define J7200_SERDES0_LANE2_PCIE1_LANE2 0x1 > +#define J7200_SERDES0_LANE2_IP3_UNUSED 0x2 > +#define J7200_SERDES0_LANE2_IP4_UNUSED 0x3 > + > +#define J7200_SERDES0_LANE3_QSGMII_LANE2 0x0 > +#define J7200_SERDES0_LANE3_PCIE1_LANE3 0x1 > +#define J7200_SERDES0_LANE3_USB 0x2 > +#define J7200_SERDES0_LANE3_IP4_UNUSED 0x3 > + > #endif /* _DT_BINDINGS_MUX_TI_SERDES */ >
Re: [PATCH] arm64: dts: ti: k3-j721e: Rename mux header and update macro names
> - > -#define SERDES4_LANE3_EDP_LANE3 0x0 > -#define SERDES4_LANE3_QSGMII_LANE8 0x2 > - > -#endif /* _DT_BINDINGS_J721E_WIZ */ > diff --git a/include/dt-bindings/mux/ti-serdes.h > b/include/dt-bindings/mux/ti-serdes.h > new file mode 100644 > index ..3e1f2d243e4a > --- /dev/null > +++ b/include/dt-bindings/mux/ti-serdes.h > @@ -0,0 +1,55 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * This header provides constants for SERDES MUX for TI SoCs > + */ > + > +#ifndef _DT_BINDINGS_TI_SERDES_MUX > +#define _DT_BINDINGS_TI_SERDES_MUX I would have spelled this _DT_BINDINGS_MUX_TI_SERDES. But as stated, it doesn't really matter to me. > + > +/* J721E */ > + > +#define J721E_SERDES0_LANE0_QSGMII_LANE1 0x0 > +#define J721E_SERDES0_LANE0_PCIE0_LANE0 0x1 > +#define J721E_SERDES0_LANE0_USB3_0_SWAP 0x2 > + > +#define J721E_SERDES0_LANE1_QSGMII_LANE2 0x0 > +#define J721E_SERDES0_LANE1_PCIE0_LANE1 0x1 > +#define J721E_SERDES0_LANE1_USB3_0 0x2 > + > +#define J721E_SERDES1_LANE0_QSGMII_LANE3 0x0 > +#define J721E_SERDES1_LANE0_PCIE1_LANE0 0x1 > +#define J721E_SERDES1_LANE0_USB3_1_SWAP 0x2 > +#define J721E_SERDES1_LANE0_SGMII_LANE0 0x3 > + > +#define J721E_SERDES1_LANE1_QSGMII_LANE4 0x0 > +#define J721E_SERDES1_LANE1_PCIE1_LANE1 0x1 > +#define J721E_SERDES1_LANE1_USB3_1 0x2 > +#define J721E_SERDES1_LANE1_SGMII_LANE1 0x3 > + > +#define J721E_SERDES2_LANE0_PCIE2_LANE0 0x1 > +#define J721E_SERDES2_LANE0_SGMII_LANE0 0x3 > +#define J721E_SERDES2_LANE0_USB3_1_SWAP 0x2 > + > +#define J721E_SERDES2_LANE1_PCIE2_LANE1 0x1 > +#define J721E_SERDES2_LANE1_USB3_1 0x2 > +#define J721E_SERDES2_LANE1_SGMII_LANE1 0x3 > + > +#define J721E_SERDES3_LANE0_PCIE3_LANE0 0x1 > +#define J721E_SERDES3_LANE0_USB3_0_SWAP 0x2 > + > +#define J721E_SERDES3_LANE1_PCIE3_LANE1 0x1 > +#define J721E_SERDES3_LANE1_USB3_0 0x2 > + > +#define J721E_SERDES4_LANE0_EDP_LANE00x0 > +#define J721E_SERDES4_LANE0_QSGMII_LANE5 0x2 > + > +#define J721E_SERDES4_LANE1_EDP_LANE10x0 > +#define J721E_SERDES4_LANE1_QSGMII_LANE6 0x2 > + > +#define J721E_SERDES4_LANE2_EDP_LANE20x0 > +#define J721E_SERDES4_LANE2_QSGMII_LANE7 0x2 > + > +#define J721E_SERDES4_LANE3_EDP_LANE30x0 > +#define J721E_SERDES4_LANE3_QSGMII_LANE8 0x2 > + > +#endif /* _DT_BINDINGS_TI_SERDES_MUX */ > The J7200-series listed *all* possible mux values, some with names like BLA_BLA_UNUSED. Why is that different here? Some dt-files using the J7200 then ended up using these "unused" entries (for idle-states) so maybe thoes values are useful here as well? The choice of using _UNUSED for entries that end up being used can of course also be debated :-) If it is ill-adviced to use the values not listed above, that's another matter of course... I don't know the answer to the above, and will not be impacted in the least, I'm just throwing out questions. So, either way: Acked-by: Peter Rosin Cheers, Peter
Re: [PATCH v3 1/6] dt-bindings: mux-j7200-wiz: Add lane function defines
On 2020-09-17 14:27, Nishanth Menon wrote:
> On 11:45-20200917, Peter Rosin wrote:
> [...]
>>
>>>> Should not the defines start with J7200_WIZ? SERDES0 seems like a too
>>>> generic prefix, at least to me.
>>>
>>> Thanks, good point. I am not sure if WIZ should even be used.. It is
>>> a TI internal prefix for various serdes solutions, but I agree that
>>> SERDES0 is too generic a terminology. That said, we should cleanup
>>> include/dt-bindings/mux/mux-j721e-wiz.h as well, prior to introducing
>>> j7200 changes.
>>
>> Right. As maintainer for the directory in question, I should have
>> been on Cc for that series too, but it appears I wasn't. Hence, I
>
> yes, you should have been. The following commit introduced it.
>
> commit b766e3b0d5f6 ("arm64: dts: ti: k3-j721e-main: Add system controller
> node and SERDES lane mux")
>
>> didn't notice that file until now when I went looking for it. Why
>> wasn't I on Cc?
>
> Got through the SoC tree - an oversight on our part[1] and should'nt have,
> Apologies on the bad call.
>
> I would like to propose the following:
> a) The header should be renamed to be something more human friendly.
> b) The header should be renamed to be something TI specific and NOT per
> TI SoC.
> c) The macros need renaming to be less generic as it stands right now.
>
>
> If you ack the changes, I am guessing that the changes will impact dts
> a lot and would rather take the cleanups through SoC tree to maintain
> bisectability? OR I can pick on an immutable tag from you with just the
> header file change and pick on the dts - but I doubt that would be
> bisectable. Just worried that I have picked a bunch of cleanups already
> on the dts for 5.10, and would like to avoid a merge conflict.
[Our mails crossed.]
I do not have a tree and dt-patches should normally not go *through* me.
But I'd still like to see what's happening.
I did not realize this was going to cause a *lot* of churn in the dt
files. How bad can it be when the file is new in this cycle? And is it
worth it? But it seems you all see problems with the current naming and
in that case it must surely be better to fix it early?
And if it's a lot, maybe it needs to be more than one patch? I get the
feeling this will need to be taken care of by someone other than me,
because I'm just the maintainer of a very small subsystem and I don't
normally have to deal with "big" issues involving several trees. I would
be a bottleneck.
Cheers,
Peter
Re: [PATCH v3 1/6] dt-bindings: mux-j7200-wiz: Add lane function defines
Hi! On 2020-09-17 14:00, Roger Quadros wrote: > Hi Peter & Nishanth, > > On 16/09/2020 18:45, Nishanth Menon wrote: >> On 06:52-20200916, Peter Rosin wrote: >>> Hi, >>> >>> Sorry for the delay. >>> >>> On 2020-09-15 13:20, Roger Quadros wrote: >>>> Each SERDES lane mux can select upto 4 different IPs. >>>> There are 4 lanes in each J7200 SERDES. Define all >>>> the possible functions in this file. >>>> >>>> Cc: Peter Rosin >>>> Signed-off-by: Roger Quadros >>>> --- >>>> include/dt-bindings/mux/mux-j7200-wiz.h | 29 + >>>> 1 file changed, 29 insertions(+) >>>> create mode 100644 include/dt-bindings/mux/mux-j7200-wiz.h >>>> >>>> diff --git a/include/dt-bindings/mux/mux-j7200-wiz.h >>>> b/include/dt-bindings/mux/mux-j7200-wiz.h >>>> new file mode 100644 >>>> index ..b091b1185a36 >>>> --- /dev/null >>>> +++ b/include/dt-bindings/mux/mux-j7200-wiz.h >>>> @@ -0,0 +1,29 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0 */ >>>> +/* >>>> + * This header provides constants for J7200 WIZ. >>>> + */ >>>> + >>>> +#ifndef _DT_BINDINGS_J7200_WIZ >>>> +#define _DT_BINDINGS_J7200_WIZ >>>> + >>>> +#define SERDES0_LANE0_QSGMII_LANE30x0 >>>> +#define SERDES0_LANE0_PCIE1_LANE0 0x1 >>>> +#define SERDES0_LANE0_IP3_UNUSED 0x2 >>>> +#define SERDES0_LANE0_IP4_UNUSED 0x3 >>>> + >>>> +#define SERDES0_LANE1_QSGMII_LANE40x0 >>>> +#define SERDES0_LANE1_PCIE1_LANE1 0x1 >>>> +#define SERDES0_LANE1_IP3_UNUSED 0x2 >>>> +#define SERDES0_LANE1_IP4_UNUSED 0x3 >>>> + >>>> +#define SERDES0_LANE2_QSGMII_LANE10x0 >>>> +#define SERDES0_LANE2_PCIE1_LANE2 0x1 >>>> +#define SERDES0_LANE2_IP3_UNUSED 0x2 >>>> +#define SERDES0_LANE2_IP4_UNUSED 0x3 >>>> + >>>> +#define SERDES0_LANE3_QSGMII_LANE20x0 >>>> +#define SERDES0_LANE3_PCIE1_LANE3 0x1 >>>> +#define SERDES0_LANE3_USB 0x2 >>>> +#define SERDES0_LANE3_IP4_UNUSED 0x3 >>>> + >>>> +#endif /* _DT_BINDINGS_J7200_WIZ */ >>> >>> Should not the defines start with J7200_WIZ? SERDES0 seems like a too >>> generic prefix, at least to me. >> >> Thanks, good point. I am not sure if WIZ should even be used.. It is >> a TI internal prefix for various serdes solutions, but I agree that >> SERDES0 is too generic a terminology. That said, we should cleanup >> include/dt-bindings/mux/mux-j721e-wiz.h as well, prior to introducing >> j7200 changes. >> > > I'm planning to put all TI SERDES definitions in one header file > "ti-serdes-mux.h" > and add SOC specific prefixes to the macros. > > This will mean some churn in the existing DT files. (only 2 so far) > > Are you guys OK if I do the change in one patch to avoid a broken build in > between. > You guys can then decide whose tree it goes through. > > The new SoC addition will be separate of course. We should get these changes done before 5.9 is released. Not breaking the build for each intermediate step is always a priority. Also, renaming mux-j721e-wiz.h to ti-serdes-mux.h and renaming the macros could be seen as orthogonal, and it is certainly possible to do that as two patches without breaking the build in between. It would just need changes on both sides of the interface in both patches. But I wouldn't worry about separating this into two patches, just do a rename patch and be done with it. Then follow up with additions for j7200. However, now that we are renaming things anyway, do we really need "mux" in the name of the file itself? I personally find .../dt-dbindings/mux/ti-serdes.h descriptive enough. Cheers, Peter
Re: [PATCH v3 1/6] dt-bindings: mux-j7200-wiz: Add lane function defines
Hi! On 2020-09-16 17:45, Nishanth Menon wrote: > On 06:52-20200916, Peter Rosin wrote: >> Hi, >> >> Sorry for the delay. >> >> On 2020-09-15 13:20, Roger Quadros wrote: >>> Each SERDES lane mux can select upto 4 different IPs. >>> There are 4 lanes in each J7200 SERDES. Define all >>> the possible functions in this file. >>> >>> Cc: Peter Rosin >>> Signed-off-by: Roger Quadros >>> --- *snip* >> Should not the defines start with J7200_WIZ? SERDES0 seems like a too >> generic prefix, at least to me. > > Thanks, good point. I am not sure if WIZ should even be used.. It is > a TI internal prefix for various serdes solutions, but I agree that > SERDES0 is too generic a terminology. That said, we should cleanup > include/dt-bindings/mux/mux-j721e-wiz.h as well, prior to introducing > j7200 changes. Right. As maintainer for the directory in question, I should have been on Cc for that series too, but it appears I wasn't. Hence, I didn't notice that file until now when I went looking for it. Why wasn't I on Cc? Cheers, Peter
Re: [PATCH v3 1/6] dt-bindings: mux-j7200-wiz: Add lane function defines
Hi, Sorry for the delay. On 2020-09-15 13:20, Roger Quadros wrote: > Each SERDES lane mux can select upto 4 different IPs. > There are 4 lanes in each J7200 SERDES. Define all > the possible functions in this file. > > Cc: Peter Rosin > Signed-off-by: Roger Quadros > --- > include/dt-bindings/mux/mux-j7200-wiz.h | 29 + > 1 file changed, 29 insertions(+) > create mode 100644 include/dt-bindings/mux/mux-j7200-wiz.h > > diff --git a/include/dt-bindings/mux/mux-j7200-wiz.h > b/include/dt-bindings/mux/mux-j7200-wiz.h > new file mode 100644 > index ..b091b1185a36 > --- /dev/null > +++ b/include/dt-bindings/mux/mux-j7200-wiz.h > @@ -0,0 +1,29 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * This header provides constants for J7200 WIZ. > + */ > + > +#ifndef _DT_BINDINGS_J7200_WIZ > +#define _DT_BINDINGS_J7200_WIZ > + > +#define SERDES0_LANE0_QSGMII_LANE3 0x0 > +#define SERDES0_LANE0_PCIE1_LANE00x1 > +#define SERDES0_LANE0_IP3_UNUSED 0x2 > +#define SERDES0_LANE0_IP4_UNUSED 0x3 > + > +#define SERDES0_LANE1_QSGMII_LANE4 0x0 > +#define SERDES0_LANE1_PCIE1_LANE10x1 > +#define SERDES0_LANE1_IP3_UNUSED 0x2 > +#define SERDES0_LANE1_IP4_UNUSED 0x3 > + > +#define SERDES0_LANE2_QSGMII_LANE1 0x0 > +#define SERDES0_LANE2_PCIE1_LANE20x1 > +#define SERDES0_LANE2_IP3_UNUSED 0x2 > +#define SERDES0_LANE2_IP4_UNUSED 0x3 > + > +#define SERDES0_LANE3_QSGMII_LANE2 0x0 > +#define SERDES0_LANE3_PCIE1_LANE30x1 > +#define SERDES0_LANE3_USB0x2 > +#define SERDES0_LANE3_IP4_UNUSED 0x3 > + > +#endif /* _DT_BINDINGS_J7200_WIZ */ Should not the defines start with J7200_WIZ? SERDES0 seems like a too generic prefix, at least to me. Cheers, Peter
Re: [PATCH v3 08/18] iio: adc: stm32: Simplify with dev_err_probe()
Hi!
On 2020-09-09 21:57, Krzysztof Kozlowski wrote:
> On Wed, 9 Sep 2020 at 20:36, Jonathan Cameron wrote:
>>
>> On Sat, 29 Aug 2020 08:47:16 +0200
>> Krzysztof Kozlowski wrote:
>>
>>> Common pattern of handling deferred probe can be simplified with
>>> dev_err_probe(). Less code and also it prints the error value.
>>>
>>> Signed-off-by: Krzysztof Kozlowski
>>> Reviewed-by: Andy Shevchenko
>>>
>> I don't have the thread to hand, but this tripped a warning next
>> and the patch was dropped as a result. See below.
>
> Thanks for letting me know. If you mean the warning caused by:
> https://lore.kernel.org/lkml/20200909073716.ga560...@kroah.com/
> then the driver-core patch was dropped, not the iio one:
> https://lore.kernel.org/linux-next/20200909074130.gb561...@kroah.com/T/#t
>
> So we are good here :)
No, we are definitely not good. See below. That means "See below", and
not "Please take a guess at what is being talking about".
> Best regards,
> Krzysztof
>
>> Jonathan
>>> ---
>>>
>>> Changes since v2:
>>> 1. Wrap dev_err_probe() lines at 80 character
>>>
>>> Changes since v1:
>>> 1. Convert to devm_clk_get_optional
>>> 2. Update also stm32-dfsdm-core and stm32-dac-core.
>>> 3. Wrap around 100 characters (accepted by checkpatch).
>>> ---
>>> drivers/iio/adc/stm32-adc-core.c | 75 ++
>>> drivers/iio/adc/stm32-adc.c| 10 ++--
>>> drivers/iio/adc/stm32-dfsdm-adc.c | 10 ++--
>>> drivers/iio/adc/stm32-dfsdm-core.c | 9 ++--
>>> drivers/iio/dac/stm32-dac-core.c | 5 +-
>>> 5 files changed, 35 insertions(+), 74 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/stm32-adc-core.c
>>> b/drivers/iio/adc/stm32-adc-core.c
>>> index 0e2068ec068b..3f27b4817a42 100644
>>> --- a/drivers/iio/adc/stm32-adc-core.c
>>> +++ b/drivers/iio/adc/stm32-adc-core.c
>>> @@ -582,11 +582,9 @@ static int stm32_adc_core_switches_probe(struct device
>>> *dev,
>>> priv->syscfg = syscon_regmap_lookup_by_phandle(np, "st,syscfg");
>>> if (IS_ERR(priv->syscfg)) {
>>> ret = PTR_ERR(priv->syscfg);
>>> - if (ret != -ENODEV) {
>>> - if (ret != -EPROBE_DEFER)
>>> - dev_err(dev, "Can't probe syscfg: %d\n", ret);
>>> - return ret;
>>> - }
>>> + if (ret != -ENODEV)
>>> + return dev_err_probe(dev, ret, "Can't probe
>>> syscfg\n");
>>> +
>>> priv->syscfg = NULL;
>>> }
>>>
>>> @@ -596,12 +594,9 @@ static int stm32_adc_core_switches_probe(struct device
>>> *dev,
>>> priv->booster = devm_regulator_get_optional(dev, "booster");
>>> if (IS_ERR(priv->booster)) {
>>> ret = PTR_ERR(priv->booster);
>>> - if (ret != -ENODEV) {
>>> - if (ret != -EPROBE_DEFER)
>>> - dev_err(dev, "can't get booster %d\n",
>>> - ret);
>>> - return ret;
>>> - }
>>> + if (ret != -ENODEV)
>>> + dev_err_probe(dev, ret, "can't get
>>> booster\n");
>>
>> This tripped a warning and got the patch dropped because we no longer
>> return on error.
As Jonathan already said, we no longer return in this hunk. I.e., you have
clobbered the error path.
Cheers,
Peter
Re: [PATCH 8/9] i2c: mux: gpmux: Simplify with dev_err_probe()
On 2020-09-02 17:06, Krzysztof Kozlowski wrote: > Common pattern of handling deferred probe can be simplified with > dev_err_probe(). Less code and the error value gets printed. > > Signed-off-by: Krzysztof Kozlowski Acked-by: Peter Rosin Cheers, Peter
Re: [PATCH 9/9] i2c: mux: reg: Simplify with dev_err_probe()
On 2020-09-02 17:06, Krzysztof Kozlowski wrote: > Common pattern of handling deferred probe can be simplified with > dev_err_probe(). Less code and the error value gets printed. > > Signed-off-by: Krzysztof Kozlowski Acked-by: Peter Rosin Cheers, Peter
Re: [PATCH v3 18/18] iio: multiplexer: iio-mux: Simplify with dev_err_probe()
On 2020-08-29 08:47, Krzysztof Kozlowski wrote: > Common pattern of handling deferred probe can be simplified with > dev_err_probe(). Less code and also it prints the error value. > > Signed-off-by: Krzysztof Kozlowski > Reviewed-by: Andy Shevchenko Acked-by: Peter Rosin Cheers, Peter
Re: [PATCH v3 12/18] iio: dac: dpot-dac: Simplify with dev_err_probe()
On 2020-08-29 08:47, Krzysztof Kozlowski wrote: > Common pattern of handling deferred probe can be simplified with > dev_err_probe(). Less code and also it prints the error value. > > Signed-off-by: Krzysztof Kozlowski > Reviewed-by: Andy Shevchenko Acked-by: Peter Rosin Cheers, Peter
Re: [PATCH v3 09/18] iio: afe: iio-rescale: Simplify with dev_err_probe()
On 2020-08-29 08:47, Krzysztof Kozlowski wrote: > Common pattern of handling deferred probe can be simplified with > dev_err_probe(). Less code and also it prints the error value. > > Signed-off-by: Krzysztof Kozlowski > Reviewed-by: Andy Shevchenko Acked-by: Peter Rosin Cheers, Peter
Re: [PATCH v3 03/18] iio: adc: envelope-detector: Simplify with dev_err_probe()
On 2020-08-29 08:47, Krzysztof Kozlowski wrote: > Common pattern of handling deferred probe can be simplified with > dev_err_probe(). Less code and also it prints the error value. > > Signed-off-by: Krzysztof Kozlowski > Reviewed-by: Andy Shevchenko Thanks for the re-spin. Acked-by: Peter Rosin Cheers, Peter
Re: [PATCH v2 09/18] iio: afe: iio-rescale: Simplify with dev_err_probe()
On 2020-08-28 09:03, Krzysztof Kozlowski wrote: > On Fri, 28 Aug 2020 at 08:58, Peter Rosin wrote: >>>> I'm not a huge fan of adding *one* odd line breaking the 80 column >>>> recommendation to any file. I like to be able to fit multiple >>>> windows side by side in a meaningful way. Also, I don't like having >>>> a shitload of emptiness on my screen, which is what happens when some >>>> lines are longer and you want to see it all. I strongly believe that >>>> the 80 column rule/recommendation is still as valid as it ever was. >>>> It's just hard to read longish lines; there's a reason newspapers >>>> columns are quite narrow... >>>> >>>> Same comment for the envelope-detector (3/18). >>>> >>>> You will probably never look at these files again, but *I* might have >>>> to revisit them for one reason or another, and these long lines will >>>> annoy me when that happens. >>> >>> Initially I posted it with 80-characters wrap. Then I received a comment >>> - better to stick to the new 100, as checkpatch accepts it. >>> >>> Now you write, better to go back to 80. >>> >>> Maybe then someone else will write to me, better to go to 100. >>> >>> And another person will reply, no, coding style still mentions 80, so >>> keep it at 80. >>> >>> Sure guys, please first decide which one you prefer, then I will wrap it >>> accordingly. :) >>> >>> Otherwise I will just jump from one to another depending on one person's >>> personal preference. >>> >>> If there is no consensus among discussing people, I find this 100 line >>> more readable, already got review, checkpatch accepts it so if subsystem >>> maintainer likes it, I prefer to leave it like this. >> >> I'm not impressed by that argument. For the files I have mentioned, it >> does not matter very much to me if you and some random person think that >> 100 columns might *slightly* improve readability. >> >> Quoting coding-style >> >> Statements longer than 80 columns should be broken into sensible chunks, >> unless exceeding 80 columns significantly increases readability and does >> not hide information. >> >> Notice that word? *significantly* > > Notice also checkpatch change... How is that relevant? checkpatch has *never* had the final say and its heuristics can never be perfect. Meanwhile, coding style is talking about exactly the case under discussion, and agrees with me perfectly. > First of all, I don't have a preference over wrapping here. As I said, > I sent v1 with 80 and got a response to change it to 100. You want me > basically to bounce from A to B to A to B. > >> Why do I even have to speak up about this? WTF? > > Because we all share here our ideas... > >> For the patches that touch files that I originally wrote [1], my >> preference should be clear by now. > > I understood your preference. There is nothing unclear here. Other > person had different preference. I told you my arguments that it is > not reasonable to jump A->B->A->B just because each person has a > different view. At the end it's the subsystem maintainer's decision as > he wants to keep his subsystem clean. Yeah, I bet he is thrilled about it. Cheers, Peter
Re: [PATCH v2 09/18] iio: afe: iio-rescale: Simplify with dev_err_probe()
On 2020-08-28 10:25, Andy Shevchenko wrote: > On Fri, Aug 28, 2020 at 12:46 AM Peter Rosin wrote: >> On 2020-08-27 21:26, Krzysztof Kozlowski wrote: > > ... > >> I'm not a huge fan of adding *one* odd line breaking the 80 column >> recommendation to any file. I like to be able to fit multiple >> windows side by side in a meaningful way. Also, I don't like having >> a shitload of emptiness on my screen, which is what happens when some >> lines are longer and you want to see it all. I strongly believe that >> the 80 column rule/recommendation is still as valid as it ever was. >> It's just hard to read longish lines; there's a reason newspapers >> columns are quite narrow... > > Why not to introduce 66 (or so, like TeX recommends)? Or even less? Because 80 is what happens to what is recommended in coding style? > I consider any comparison to news or natural language text is silly. > Programming language is different in many aspects, including helpful > scripts and utilities to browse the source code. So, by all means, scratch that part. You still have a problem getting around the coding style recommendation. Cheers, Peter
Re: [PATCH v2 09/18] iio: afe: iio-rescale: Simplify with dev_err_probe()
On 2020-08-28 08:24, Krzysztof Kozlowski wrote:
> On Thu, Aug 27, 2020 at 11:46:40PM +0200, Peter Rosin wrote:
>> On 2020-08-27 21:26, Krzysztof Kozlowski wrote:
>>> Common pattern of handling deferred probe can be simplified with
>>> dev_err_probe(). Less code and also it prints the error value.
>>>
>>> Signed-off-by: Krzysztof Kozlowski
>>>
>>> ---
>>>
>>> Changes since v1:
>>> 1. Wrap dev_err_probe() lines at 100 character
>>> ---
>>> drivers/iio/afe/iio-rescale.c | 7 ++-
>>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
>>> index 69c0f277ada0..8cd9645c50e8 100644
>>> --- a/drivers/iio/afe/iio-rescale.c
>>> +++ b/drivers/iio/afe/iio-rescale.c
>>> @@ -276,11 +276,8 @@ static int rescale_probe(struct platform_device *pdev)
>>> int ret;
>>>
>>> source = devm_iio_channel_get(dev, NULL);
>>> - if (IS_ERR(source)) {
>>> - if (PTR_ERR(source) != -EPROBE_DEFER)
>>> - dev_err(dev, "failed to get source channel\n");
>>> - return PTR_ERR(source);
>>> - }
>>> + if (IS_ERR(source))
>>> + return dev_err_probe(dev, PTR_ERR(source), "failed to get
>>> source channel\n");
>>
>> Hi!
>>
>> Sorry to be a pain...but...
>>
>> I'm not a huge fan of adding *one* odd line breaking the 80 column
>> recommendation to any file. I like to be able to fit multiple
>> windows side by side in a meaningful way. Also, I don't like having
>> a shitload of emptiness on my screen, which is what happens when some
>> lines are longer and you want to see it all. I strongly believe that
>> the 80 column rule/recommendation is still as valid as it ever was.
>> It's just hard to read longish lines; there's a reason newspapers
>> columns are quite narrow...
>>
>> Same comment for the envelope-detector (3/18).
>>
>> You will probably never look at these files again, but *I* might have
>> to revisit them for one reason or another, and these long lines will
>> annoy me when that happens.
>
> Initially I posted it with 80-characters wrap. Then I received a comment
> - better to stick to the new 100, as checkpatch accepts it.
>
> Now you write, better to go back to 80.
>
> Maybe then someone else will write to me, better to go to 100.
>
> And another person will reply, no, coding style still mentions 80, so
> keep it at 80.
>
> Sure guys, please first decide which one you prefer, then I will wrap it
> accordingly. :)
>
> Otherwise I will just jump from one to another depending on one person's
> personal preference.
>
> If there is no consensus among discussing people, I find this 100 line
> more readable, already got review, checkpatch accepts it so if subsystem
> maintainer likes it, I prefer to leave it like this.
I'm not impressed by that argument. For the files I have mentioned, it
does not matter very much to me if you and some random person think that
100 columns might *slightly* improve readability.
Quoting coding-style
Statements longer than 80 columns should be broken into sensible chunks,
unless exceeding 80 columns significantly increases readability and does
not hide information.
Notice that word? *significantly*
Why do I even have to speak up about this? WTF?
For the patches that touch files that I originally wrote [1], my
preference should be clear by now.
Cheers,
Peter
[1]
drivers/iio/adc/envelope-detector.c
drivers/iio/afe/iio-rescale.c
drivers/iio/dac/dpot-dac.c
drivers/iio/multiplexer/iio-mux.c
>> You did wrap the lines for dpot-dac (12/18) - which is excellent - but
>> that makes me curious as to what the difference is?
>
> Didn't fit into limit of 100.
Re: [PATCH v2 18/18] iio: magnetometer: iio-mux: Simplify with dev_err_probe()
Hi!
Wrong subject. Made me overlook it on the first pass...
On 2020-08-27 21:26, Krzysztof Kozlowski wrote:
> Common pattern of handling deferred probe can be simplified with
> dev_err_probe(). Less code and also it prints the error value.
>
> Signed-off-by: Krzysztof Kozlowski
> ---
> drivers/iio/multiplexer/iio-mux.c | 7 ++-
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/multiplexer/iio-mux.c
> b/drivers/iio/multiplexer/iio-mux.c
> index 6910218fdb00..d219d4a86657 100644
> --- a/drivers/iio/multiplexer/iio-mux.c
> +++ b/drivers/iio/multiplexer/iio-mux.c
> @@ -354,11 +354,8 @@ static int mux_probe(struct platform_device *pdev)
> return -ENODEV;
>
> parent = devm_iio_channel_get(dev, "parent");
> - if (IS_ERR(parent)) {
> - if (PTR_ERR(parent) != -EPROBE_DEFER)
> - dev_err(dev, "failed to get parent channel\n");
> - return PTR_ERR(parent);
> - }
> + if (IS_ERR(parent))
> + return dev_err_probe(dev, PTR_ERR(parent), "failed to get
> parent channel\n");
As per the comment for 9/18, I'm not a fan of the long line here...
Cheers,
Peter
>
> sizeof_ext_info = iio_get_channel_ext_info_count(parent);
> if (sizeof_ext_info) {
>
Re: [PATCH v2 09/18] iio: afe: iio-rescale: Simplify with dev_err_probe()
On 2020-08-27 21:26, Krzysztof Kozlowski wrote:
> Common pattern of handling deferred probe can be simplified with
> dev_err_probe(). Less code and also it prints the error value.
>
> Signed-off-by: Krzysztof Kozlowski
>
> ---
>
> Changes since v1:
> 1. Wrap dev_err_probe() lines at 100 character
> ---
> drivers/iio/afe/iio-rescale.c | 7 ++-
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 69c0f277ada0..8cd9645c50e8 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -276,11 +276,8 @@ static int rescale_probe(struct platform_device *pdev)
> int ret;
>
> source = devm_iio_channel_get(dev, NULL);
> - if (IS_ERR(source)) {
> - if (PTR_ERR(source) != -EPROBE_DEFER)
> - dev_err(dev, "failed to get source channel\n");
> - return PTR_ERR(source);
> - }
> + if (IS_ERR(source))
> + return dev_err_probe(dev, PTR_ERR(source), "failed to get
> source channel\n");
Hi!
Sorry to be a pain...but...
I'm not a huge fan of adding *one* odd line breaking the 80 column
recommendation to any file. I like to be able to fit multiple
windows side by side in a meaningful way. Also, I don't like having
a shitload of emptiness on my screen, which is what happens when some
lines are longer and you want to see it all. I strongly believe that
the 80 column rule/recommendation is still as valid as it ever was.
It's just hard to read longish lines; there's a reason newspapers
columns are quite narrow...
Same comment for the envelope-detector (3/18).
You will probably never look at these files again, but *I* might have
to revisit them for one reason or another, and these long lines will
annoy me when that happens.
You did wrap the lines for dpot-dac (12/18) - which is excellent - but
that makes me curious as to what the difference is?
Cheers,
Peter
>
> sizeof_ext_info = iio_get_channel_ext_info_count(source);
> if (sizeof_ext_info) {
>
Re: [PATCH] iio: dpot-dac: fix code comment in dpot_dac_read_raw()
On 2020-08-26 16:17, Gustavo A. R. Silva wrote: >> And just to be explicit, this fix is for 5.9. >> >> Acked-by: Peter Rosin >> > > If you don't mind I can add this to my tree for 5.9-rc4 > and send it directly to Linus. Fine by me, Jonathan might think differently but I can't find a reason why. Just about nothing is happening in that file and the risk for conflicts is negligible. Cheers, Peter
Re: [PATCH] iio: dpot-dac: fix code comment in dpot_dac_read_raw()
Hi! On 2020-08-26 02:08, Gustavo A. R. Silva wrote: > After the replacement of the /* fall through */ comment with the > fallthrough pseudo-keyword macro, the natural reading of a code > comment was broken. > > Fix the natural reading of such a comment and make it intelligible. > > Reported-by: Peter Rosin > Signed-off-by: Gustavo A. R. Silva Excellent, thanks for the quick turnaround! And just to be explicit, this fix is for 5.9. Acked-by: Peter Rosin Cheers, Peter
Re: [GIT PULL] fallthrough pseudo-keyword macro conversions for 5.9-rc3
On 2020-08-23 22:48, Gustavo A. R. Silva wrote: > Hi Linus, > > Please, pull the following tree-wide patch that replaces tons (2484) of > /* fall through */ comments, and its variants, with the new pseudo-keyword > macro fallthrough[1]. Also, remove unnecessary fall-through markings when > it is the case. Hi. This is the second time [1] you have messed up the reading pleasure of drivers/iio/dac/dpot-dac.c with mindless fall through conversions. The comments in that file no longer make sense. With more context: case IIO_VAL_INT: /* * Convert integer scale to fractional scale by * setting the denominator (val2) to one, and... */ *val2 = 1; ret = IIO_VAL_FRACTIONAL; - /* fall through */ + fallthrough; case IIO_VAL_FRACTIONAL: *val *= regulator_get_voltage(dac->vref) / 1000; *val2 *= dac->max_ohms; break; } See how the comment ending with "and..." is no longer continued? I disliked the last change to this file [2] that (after pressure) moved "and" from the original "...and fall through." comment away from where it belonged (instead of just removing it) exactly because it was less clear that the "fall through" was part of the natural comment reading. But this latest patch just destroys the commentary completely. I guess I should have stood my ground the last time with my /* ...and fall through. Say it again for GCC. */ /* fall through */ suggestion, so that it would now have become /* ...and fall through. Say it again for GCC. */ fallthrough; with this latest change. Because that makes sense. If it was someone else, it would be understandable, but since it was you - again - I expect a patch fixing this. Either as I suggested above or some other way. Comments are important. Cheers, Peter [1] https://lore.kernel.org/lkml/20181008173528.ga31...@embeddedor.com/ [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/iio/dac/dpot-dac.c?id=c65a0d84ee9c
Re: include/linux/bits.h:25:21: error: first argument to '__builtin_choose_expr' not a constant
On 2020-08-11 06:27, Andrew Morton wrote: > On Sat, 8 Aug 2020 08:03:38 +0800 kernel test robot wrote: > >> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git >> master >> head: 30185b69a2d533c4ba6ca926b8390ce7de495e29 >> commit: 295bcca84916cb5079140a89fccb472bb8d1f6e2 linux/bits.h: add compile >> time sanity check of GENMASK inputs >> date: 4 months ago >> config: s390-randconfig-r001-20200808 (attached as .config) >> compiler: s390-linux-gcc (GCC) 9.3.0 >> reproduce (this is a W=1 build): >> wget >> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O >> ~/bin/make.cross >> chmod +x ~/bin/make.cross >> git checkout 295bcca84916cb5079140a89fccb472bb8d1f6e2 >> # save the attached .config to linux build tree >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross >> ARCH=s390 >> >> If you fix the issue, kindly add following tag as appropriate >> Reported-by: kernel test robot >> >> All errors (new ones prefixed by >>): >> >>drivers/mux/mmio.c: In function 'mux_mmio_probe': >>drivers/mux/mmio.c:76:20: error: storage size of 'field' isn't known >> 76 | struct reg_field field; >> |^ >>In file included from include/linux/bits.h:23, >> from include/linux/bitops.h:5, >> from drivers/mux/mmio.c:8: include/linux/bits.h:25:21: error: first argument to '__builtin_choose_expr' not a constant > > I assume the first error is the cause of the second? > > struct reg_field is only defined if CONFIG_REGMAP, and that is unset in > this .config. Perhaps drivers/mux/mmio.c should depend on > CONFIG_REGMAP? (cc Peter). > > Thanks for the CC, and yes, that sounds about right. If it shouldn't just "select REGMAP" instead? I'm not sure when one should "select" and when one should "depends on"? Cheers, Peter
Re: [PATCH 1/2] iio: afe: rescale: Add support for converting scale avail table
Hi!
On 2020-02-10 23:54, Paul Cercueil wrote:
> When the IIO channel has a scale_available attribute, we want the values
> contained to be properly converted the same way the scale value is.
>
> Signed-off-by: Paul Cercueil
> ---
> drivers/iio/afe/iio-rescale.c | 125 --
> 1 file changed, 103 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index e9ceee66d1e7..95802d9ee25e 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -31,14 +31,45 @@ struct rescale {
> struct iio_chan_spec_ext_info *ext_info;
> s32 numerator;
> s32 denominator;
> + int scale_type, scale_len;
> + int *scale_data;
> };
>
> +static int rescale_convert(struct rescale *rescale, int type,
> +const int val, const int val2,
> +int *val_out, int *val2_out)
> +{
> + unsigned long long tmp;
> +
> + switch (type) {
> + case IIO_VAL_FRACTIONAL:
> + *val_out = val * rescale->numerator;
> + *val2_out = val2 * rescale->denominator;
> + return type;
> + case IIO_VAL_INT:
> + *val_out = val * rescale->numerator;
> + if (rescale->denominator == 1)
> + return type;
> + *val2_out = rescale->denominator;
> + return IIO_VAL_FRACTIONAL;
> + case IIO_VAL_FRACTIONAL_LOG2:
> + tmp = val * 10LL;
> + do_div(tmp, rescale->denominator);
> + tmp *= rescale->numerator;
> + do_div(tmp, 10LL);
> + *val_out = tmp;
> + *val2_out = val2;
> + return type;
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> static int rescale_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int *val, int *val2, long mask)
> {
> struct rescale *rescale = iio_priv(indio_dev);
> - unsigned long long tmp;
> int ret;
>
> switch (mask) {
> @@ -47,27 +78,7 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
>
> case IIO_CHAN_INFO_SCALE:
> ret = iio_read_channel_scale(rescale->source, val, val2);
> - switch (ret) {
> - case IIO_VAL_FRACTIONAL:
> - *val *= rescale->numerator;
> - *val2 *= rescale->denominator;
> - return ret;
> - case IIO_VAL_INT:
> - *val *= rescale->numerator;
> - if (rescale->denominator == 1)
> - return ret;
> - *val2 = rescale->denominator;
> - return IIO_VAL_FRACTIONAL;
> - case IIO_VAL_FRACTIONAL_LOG2:
> - tmp = *val * 10LL;
> - do_div(tmp, rescale->denominator);
> - tmp *= rescale->numerator;
> - do_div(tmp, 10LL);
> - *val = tmp;
> - return ret;
> - default:
> - return -EOPNOTSUPP;
> - }
> + return rescale_convert(rescale, ret, *val, *val2, val, val2);
> default:
> return -EINVAL;
> }
> @@ -85,6 +96,14 @@ static int rescale_read_avail(struct iio_dev *indio_dev,
> *type = IIO_VAL_INT;
> return iio_read_avail_channel_raw(rescale->source,
> vals, length);
> + case IIO_CHAN_INFO_SCALE:
> + if (rescale->scale_len) {
> + *type = rescale->scale_type;
> + *length = rescale->scale_len;
> + *vals = rescale->scale_data;
> + return IIO_AVAIL_LIST;
> + }
> + /* fall-through */
> default:
> return -EINVAL;
> }
> @@ -119,11 +138,65 @@ static ssize_t rescale_write_ext_info(struct iio_dev
> *indio_dev,
> buf, len);
> }
>
> +static int rescale_init_scale_avail(struct device *dev, struct rescale
> *rescale)
> +{
> + const int *scale_raw;
> + unsigned int i;
> + int ret;
> +
> + ret = iio_read_avail_channel_attribute(rescale->source, _raw,
> +>scale_type,
> +>scale_len,
> +IIO_CHAN_INFO_SCALE);
> + if (ret)
> + return ret;
> +
> + if (rescale->scale_type == IIO_VAL_INT && rescale->denominator > 1)
> + rescale->scale_len *= 2;
> +
> + rescale->scale_data = devm_kzalloc(dev,
> +sizeof(int) * rescale->scale_len,
> +GFP_KERNEL);
> + if (!rescale->scale_len)
> +
Re: [PATCH 2/2] iio: afe: rescale: Implement write_raw
Hi!
Sorry for the delay. Vacation...
On 2020-07-26 14:41, Jonathan Cameron wrote:
> On Tue, 21 Jul 2020 01:16:55 +0200
> Paul Cercueil wrote:
>
>> Hi Jonathan,
>>
>> Le sam. 15 févr. 2020 à 18:32, Jonathan Cameron a
>> écrit :
>>> On Mon, 10 Feb 2020 19:54:38 -0300
>>> Paul Cercueil wrote:
>>>
Implement write_raw by converting the value if writing the scale, or
just calling the managed channel driver's write_raw otherwise.
Signed-off-by: Paul Cercueil
---
drivers/iio/afe/iio-rescale.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/iio/afe/iio-rescale.c
b/drivers/iio/afe/iio-rescale.c
index 95802d9ee25e..a48f6af9316d 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -35,6 +35,27 @@ struct rescale {
int *scale_data;
};
+static int rescale_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct rescale *rescale = iio_priv(indio_dev);
+ unsigned long long tmp;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ tmp = val * 10LL;
+ do_div(tmp, rescale->numerator);
+ tmp *= rescale->denominator;
+ do_div(tmp, 10LL);
+ return iio_write_channel_attribute(rescale->source, tmp, 0,
+IIO_CHAN_INFO_SCALE);
>>>
>>> Why is val2 always 0? Won't that only work if the backend device
>>> has integer scales?
>>
>> Sorry, somehow I didn't see your answer.
>>
>> Indeed, this will only work if the backend device has integer scales,
>> but what should I do? Just pass 'val2' instead of 0? Will the value be
>> correct if I only apply the scale ratio to 'val'?
>
> I think you'll need to include it through the calculation. Given you
> premultiply by 10LL it should be easy enough to do.
> Then for the final do_div you can easily work out the val2 part.
>
> I'm not sure we currently have an inkern interface to get the type
> of the channel attribute value though. You may need to add one.
Right, I didn't originally add scaled writing as
1. I don't need it.
2. It's a rats nest, IIRC some drivers are picky in what they take and
you would need some kind of matrix of how to best handle the different
conversion cases. I just didn't want to go there, and this patch
feels far too simple to be adequate. But then again, maybe my memory is
poorer that I thought...
Cheers,
Peter
Re: [PATCH v3 4/8] mux: sparx5: Add Sparx5 SPI mux driver
Hi!
On 2020-07-02 12:13, Lars Povlsen wrote:
> The Sparx5 mux driver may be used to control selecting between two
> alternate SPI bus segments connected to the SPI controller
> (spi-dw-mmio).
>
> Signed-off-by: Lars Povlsen
> ---
> drivers/mux/Makefile | 2 +
> drivers/mux/sparx5-spi.c | 138 +++
> 2 files changed, 140 insertions(+)
> create mode 100644 drivers/mux/sparx5-spi.c
>
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> index 6e9fa47daf566..18c3ae3582ece 100644
> --- a/drivers/mux/Makefile
> +++ b/drivers/mux/Makefile
> @@ -8,9 +8,11 @@ mux-adg792a-objs := adg792a.o
> mux-adgs1408-objs:= adgs1408.o
> mux-gpio-objs:= gpio.o
> mux-mmio-objs:= mmio.o
> +mux-sparx5-objs := sparx5-spi.o
>
> obj-$(CONFIG_MULTIPLEXER)+= mux-core.o
> obj-$(CONFIG_MUX_ADG792A)+= mux-adg792a.o
> obj-$(CONFIG_MUX_ADGS1408) += mux-adgs1408.o
> obj-$(CONFIG_MUX_GPIO) += mux-gpio.o
> obj-$(CONFIG_MUX_MMIO) += mux-mmio.o
> +obj-$(CONFIG_SPI_DW_MMIO)+= mux-sparx5.o
> diff --git a/drivers/mux/sparx5-spi.c b/drivers/mux/sparx5-spi.c
> new file mode 100644
> index 0..5fe9025b96a5e
> --- /dev/null
> +++ b/drivers/mux/sparx5-spi.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Sparx5 SPI MUX driver
> + *
> + * Copyright (c) 2019 Microsemi Corporation
> + *
> + * Author: Lars Povlsen
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define MSCC_IF_SI_OWNER_SISL0
> +#define MSCC_IF_SI_OWNER_SIBM1
> +#define MSCC_IF_SI_OWNER_SIMC2
> +
> +#define SPARX5_CPU_SYSTEM_CTRL_GENERAL_CTRL 0x88
> +#define SPARX5_IF_SI_OWNER GENMASK(7, 6)
> +#define SPARX5_IF_SI2_OWNER GENMASK(5, 4)
> +
> +#define SPARX5_MAX_CS16
> +
> +struct mux_sparx5 {
> + struct regmap *syscon;
> + u8 bus[SPARX5_MAX_CS];
> + int cur_bus;
Surplus unused variable?
> +};
> +
> +/*
> + * Set the owner of the SPI interfaces
> + */
> +static void mux_sparx5_set_owner(struct regmap *syscon,
> + u8 owner, u8 owner2)
> +{
> + u32 val, msk;
> +
> + val = FIELD_PREP(SPARX5_IF_SI_OWNER, owner) |
> + FIELD_PREP(SPARX5_IF_SI2_OWNER, owner2);
> + msk = SPARX5_IF_SI_OWNER | SPARX5_IF_SI2_OWNER;
> + regmap_update_bits(syscon,
> +SPARX5_CPU_SYSTEM_CTRL_GENERAL_CTRL,
> +msk, val);
> +}
> +
> +static void mux_sparx5_set_cs_owner(struct mux_sparx5 *mux_sparx5,
> + u8 cs, u8 owner)
> +{
> + u8 other = (owner == MSCC_IF_SI_OWNER_SIBM ?
> + MSCC_IF_SI_OWNER_SIMC : MSCC_IF_SI_OWNER_SIBM);
Empty line missing here.
> + if (mux_sparx5->bus[cs])
> + /* SPI2 */
> + mux_sparx5_set_owner(mux_sparx5->syscon, other, owner);
> + else
> + /* SPI1 */
> + mux_sparx5_set_owner(mux_sparx5->syscon, owner, other);
> +}
This smells like there are only two states for this mux control, and that
the whole point of this driver is to make the exact numbering selectable.
I don't see the point of that. To me, it looks like the pre-existing
mmio-mux should be able to work. Something like this? Untested of course,
and I might easily have misunderstood something...
mux: mux-controller {
compatible = "mmio-mux"
#mux-control-cells = <1>;
/* SI_OWNER and SI2_OWNER in GENERAL_CTRL */
mux-reg-masks = <0x88 0xf0>;
};
spi0: spi@600104000 {
compatible = "microchip,sparx5-spi";
spi@0 {
compatible = "spi-mux";
mux-controls = < 0>;
reg = <0>;
/* SI_OWNER = SIMC, SI2_OWNER = SIBM ---> mux value 9 */
spi-flash@9 {
compatible = "jedec,spi-nor";
reg = <9>;
};
};
spi@e {
compatible = "spi-mux";
mux-controls = < 0>;
reg = <14>;
/* SI_OWNER = SIBM, SI2_OWNER = SIMC ---> mux value 6 */
spi-flash@6 {
compatible = "spi-nand";
reg = <6>;
};
};
};
> +
> +static int mux_sparx5_set(struct mux_control *mux, int state)
> +{
> + struct mux_sparx5 *mux_sparx5 = mux_chip_priv(mux->chip);
> +
> + mux_sparx5_set_cs_owner(mux_sparx5, state, MSCC_IF_SI_OWNER_SIMC);
> +
> + return 0;
> +}
> +
> +static const struct mux_control_ops mux_sparx5_ops = {
> + .set = mux_sparx5_set,
> +};
> +
> +static const struct of_device_id mux_sparx5_dt_ids[] = {
> + { .compatible = "microchip,sparx5-spi-mux", },
> + {
Re: [PATCH 1/2] i2c: mux: pca9541: Change to correct bus control commands
On 2020-06-02 14:12, Quentin Strydom wrote:
> Change current bus commands to match the pca9541a datasheet
> (see table 12 on page 14 of
> https://www.nxp.com/docs/en/data-sheet/PCA9541A.pdf). Also
> where entries are marked as no change the current control
> command is repeated as the current master reading the
> control register has control of the bus and bus is on.
>
> Signed-off-by: Quentin Strydom
> ---
> drivers/i2c/muxes/i2c-mux-pca9541.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c
> b/drivers/i2c/muxes/i2c-mux-pca9541.c
> index 6a39ada..50808fa 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
> @@ -211,7 +211,7 @@ static void pca9541_release_bus(struct i2c_client *client)
>
> /* Control commands per PCA9541 datasheet */
> static const u8 pca9541_control[16] = {
> - 4, 0, 1, 5, 4, 4, 5, 5, 0, 0, 1, 1, 0, 4, 5, 1
> + 4, 4, 5, 5, 4, 4, 5, 7, 8, 0, 1, 11, 0, 0, 1, 1
> };
>
> /*
I found all your mails from git send-email in my spam folder. They probably
lack some headers that have become increasingly important... [Don't ask me
for further details.]
I do not have the HW to test this. I'm only going by the datasheet.
But yes, pca9541_control[1] and [2] indeed seem exchanged with [13] and [14].
However, pca9541_control[5], [7], [8], and [11] are never used AFAICT.
Trying to write 7, 8 and 11 also attempts to write various read-only bits
and makes no sense. So, I'd skip those changes.
All that said, I'm a bit skeptic as to why this has worked at all if this
is incorrect. I would like to see a more detailed failure description that
could explain why this change is indeed "it".
Cheers,
Peter
[PATCH] mux: adgs1408: Add mod_devicetable.h and remove of_match_ptr
From: Andy Shevchenko
Enables probing via the ACPI PRP0001 route but more is mostly about
removing examples of this that might get copied into new drivers.
Also fixes
drivers/mux/adgs1408.c:112:34: warning: unused variable 'adgs1408_of_match
as has been reported recently.
Fixes: e9e40543ad5b ("spi: Add generic SPI multiplexer")
Reported-by: kbuild test robot
Signed-off-by: Andy Shevchenko
Signed-off-by: Peter Rosin
---
drivers/mux/adgs1408.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Hi Greg,
Here's a single mux patch. It's been in -next for a couple of days, and
it all looks straight-forward to me. Please pass on to Linus whenever
convenient.
Cheers,
Peter
diff --git a/drivers/mux/adgs1408.c b/drivers/mux/adgs1408.c
index 89096f10f4c4..12466b06692c 100644
--- a/drivers/mux/adgs1408.c
+++ b/drivers/mux/adgs1408.c
@@ -6,9 +6,9 @@
*/
#include
+#include
#include
#include
-#include
#include
#include
@@ -59,7 +59,7 @@ static int adgs1408_probe(struct spi_device *spi)
s32 idle_state;
int ret;
- chip_id = (enum adgs1408_chip_id)of_device_get_match_data(dev);
+ chip_id = (enum adgs1408_chip_id)device_get_match_data(dev);
if (!chip_id)
chip_id = spi_get_device_id(spi)->driver_data;
@@ -119,7 +119,7 @@ MODULE_DEVICE_TABLE(of, adgs1408_of_match);
static struct spi_driver adgs1408_driver = {
.driver = {
.name = "adgs1408",
- .of_match_table = of_match_ptr(adgs1408_of_match),
+ .of_match_table = adgs1408_of_match,
},
.probe = adgs1408_probe,
.id_table = adgs1408_spi_id,
--
2.20.1
Re: [PATCH v1] mux: adgs1408: Add mod_devicetable.h and remove of_match_ptr
On 2020-05-20 14:01, Andy Shevchenko wrote:
> Enables probing via the ACPI PRP0001 route but more is mostly about
> removing examples of this that might get copied into new drivers.
>
> Also fixes
> drivers/mux/adgs1408.c:112:34: warning: unused variable 'adgs1408_of_match
> as has been reported recently.
>
> Fixes: e9e40543ad5b ("spi: Add generic SPI multiplexer")
> Reported-by: kbuild test robot
> Signed-off-by: Andy Shevchenko
Looks ok to me, so it is added to linux-next. I'll let it soak there for
a couple of days and then intend to pass it on to Greg.
Cheers,
Peter
> ---
> drivers/mux/adgs1408.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mux/adgs1408.c b/drivers/mux/adgs1408.c
> index 89096f10f4c4..12466b06692c 100644
> --- a/drivers/mux/adgs1408.c
> +++ b/drivers/mux/adgs1408.c
> @@ -6,9 +6,9 @@
> */
>
> #include
> +#include
> #include
> #include
> -#include
> #include
> #include
>
> @@ -59,7 +59,7 @@ static int adgs1408_probe(struct spi_device *spi)
> s32 idle_state;
> int ret;
>
> - chip_id = (enum adgs1408_chip_id)of_device_get_match_data(dev);
> + chip_id = (enum adgs1408_chip_id)device_get_match_data(dev);
> if (!chip_id)
> chip_id = spi_get_device_id(spi)->driver_data;
>
> @@ -119,7 +119,7 @@ MODULE_DEVICE_TABLE(of, adgs1408_of_match);
> static struct spi_driver adgs1408_driver = {
> .driver = {
> .name = "adgs1408",
> - .of_match_table = of_match_ptr(adgs1408_of_match),
> + .of_match_table = adgs1408_of_match,
> },
> .probe = adgs1408_probe,
> .id_table = adgs1408_spi_id,
[PATCH] spi: mux: repair mux usage
It is not valid to cache/short out selection of the mux.
mux_control_select() only locks the mux until mux_control_deselect()
is called. mux_control_deselect() may put the mux in some low power
state or some other user of the mux might select it for other purposes.
These things are probably not happening in the original setting where
this driver was developed, but it is said to be a generic SPI mux.
Also, the mux framework will short out the actual low level muxing
operation when/if that is possible.
Fixes: e9e40543ad5b ("spi: Add generic SPI multiplexer")
Signed-off-by: Peter Rosin
---
drivers/spi/spi-mux.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/spi/spi-mux.c b/drivers/spi/spi-mux.c
index 4f94c9127fc1..cc9ef371db14 100644
--- a/drivers/spi/spi-mux.c
+++ b/drivers/spi/spi-mux.c
@@ -51,6 +51,10 @@ static int spi_mux_select(struct spi_device *spi)
struct spi_mux_priv *priv = spi_controller_get_devdata(spi->controller);
int ret;
+ ret = mux_control_select(priv->mux, spi->chip_select);
+ if (ret)
+ return ret;
+
if (priv->current_cs == spi->chip_select)
return 0;
@@ -62,10 +66,6 @@ static int spi_mux_select(struct spi_device *spi)
priv->spi->mode = spi->mode;
priv->spi->bits_per_word = spi->bits_per_word;
- ret = mux_control_select(priv->mux, spi->chip_select);
- if (ret)
- return ret;
-
priv->current_cs = spi->chip_select;
return 0;
--
2.20.1
Re: [PATCH v1] mux: adgs1408: Add mod_devicetable.h and remove of_match_ptr
On 2020-05-22 14:52, Andy Shevchenko wrote: > On Wed, May 20, 2020 at 03:01:22PM +0300, Andy Shevchenko wrote: >> Enables probing via the ACPI PRP0001 route but more is mostly about >> removing examples of this that might get copied into new drivers. >> >> Also fixes >> drivers/mux/adgs1408.c:112:34: warning: unused variable 'adgs1408_of_match >> as has been reported recently. > > Maybe Mark or Greg can take this? > > I can resend Cc'ing you. I'll have a look after the (long) weekend. Cheers, Peter
Re: [PATCH] i2c: mux: demux-pinctrl: Fix an error handling path in 'i2c_demux_pinctrl_probe()'
On 2020-05-12 15:08, Wolfram Sang wrote:
> On Wed, May 06, 2020 at 09:21:00PM +0200, Christophe JAILLET wrote:
>> A call to 'i2c_demux_deactivate_master()' is missing in the error handling
>> path, as already done in the remove function.
>>
>> Fixes: 50a5ba876908 ("i2c: mux: demux-pinctrl: add driver")
>> Signed-off-by: Christophe JAILLET
>
> Applied to for-current, thanks! Peter, I hope you are okay with me
> applying patches for my strange driver directly.
Absolutely, you have always "owned" that one. I prefer to stay out :-)
Cheers,
Peter
Re: [PATCH] i2c: mux: Replace zero-length array with flexible-array
On 2020-05-07 20:53, Gustavo A. R. Silva wrote:
> The current codebase makes use of the zero-length array language
> extension to the C90 standard, but the preferred mechanism to declare
> variable-length types such as these ones is a flexible array member[1][2],
> introduced in C99:
>
> struct foo {
> int stuff;
> struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on.
>
> Also, notice that, dynamic memory allocations won't be affected by
> this change:
>
> "Flexible array members have incomplete type, and so the sizeof operator
> may not be applied. As a quirk of the original implementation of
> zero-length arrays, sizeof evaluates to zero."[1]
>
> sizeof(flexible-array-member) triggers a warning because flexible array
> members have incomplete type[1]. There are some instances of code in
> which the sizeof operator is being incorrectly/erroneously applied to
> zero-length arrays and the result is zero. Such instances may be hiding
> some bugs. So, this work (flexible-array member conversions) will also
> help to get completely rid of those sorts of issues.
>
> This issue was found with the help of Coccinelle.
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://github.com/KSPP/linux/issues/21
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>
> Signed-off-by: Gustavo A. R. Silva
Cheers,
Peter
Re: [PATCH v5 0/9] i2c: add support for filters
On 2019-10-21 16:05, Wolfram Sang wrote: > On Mon, Oct 07, 2019 at 07:53:21AM +, eugen.hris...@microchip.com wrote: >> >> >> On 11.09.2019 11:24, Eugen Hristev - M18282 wrote: >>> From: Eugen Hristev >>> >>> Hello, >>> >>> This series adds support for analog and digital filters for i2c controllers >>> >>> This series is based on the series: >>> [PATCH v2 0/9] i2c: at91: filters support for at91 SoCs >>> and later >>> [PATCH v4 0/9] i2c: add support for filters >>> and enhanced to add the bindings for all controllers plus an extra bindings >>> for the width of the spikes in nanoseconds (digital filters) and cut-off >>> frequency (analog filters) >>> >>> First, bindings are created for >>> 'i2c-analog-filter' >>> 'i2c-digital-filter' >>> 'i2c-digital-filter-width-ns' >>> 'i2c-analog-filter-cutoff-frequency' >>> >>> The support is added in the i2c core to retrieve filter width/cutoff >>> frequency >>> and add it to the timings structure. >>> Next, the at91 driver is enhanced for supporting digital filter, advanced >>> digital filter (with selectable spike width) and the analog filter. >>> >>> Finally the device tree for two boards are modified to make use of the >>> new properties. >>> >>> This series is the result of the comments on the ML in the direction >>> requested: to make the bindings globally available for i2c drivers. >>> >>> Changes in v5: >>> - renamed i2c-filter-width-ns to i2c-digital-filter-width-ns as this >>> is applicable only to digital filter >>> - created new binding i2c-digital-filter-width-ns for analog filters. >> >> Hello Wolfram and Peter, >> >> Are you happy with the changes in this version? I haven't heard from you >> since this latest update. >> I am interested to know if anymore changes are required or maybe we can >> move further with this support. > > So, I had a look now and I am happy. I will give Peter one more day to > comment, otherwise I'll apply it tomorrow. I had another read-through and only found one nit which is in a separate message. You can add Reviewed-by: Peter Rosin for the whole series. Cheers, Peter > Thanks for your patience and keeping at it! >
Re: [PATCH v5 3/9] i2c: add support for filters optional properties
On 2019-09-11 10:24, eugen.hris...@microchip.com wrote:
> From: Eugen Hristev
>
> i2c-digital-filter-width-ns:
> This optional timing property specifies the width of the spikes on the i2c
> lines (in ns) that can be filtered out by built-in digital filters which are
> embedded in some i2c controllers.
> i2c-analog-filter-cutoff-frequency:
> This optional timing property specifies the cutoff frequency of a low-pass
> analog filter built-in i2c controllers. This low pass filter is used to filter
> out high frequency noise on the i2c lines. Specified in Hz.
> Include these properties in the timings structure and read them as integers.
>
> Signed-off-by: Eugen Hristev
> ---
> drivers/i2c/i2c-core-base.c | 6 ++
> include/linux/i2c.h | 6 ++
> 2 files changed, 12 insertions(+)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 9c440fa..c9fcb16 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1658,6 +1658,12 @@ void i2c_parse_fw_timings(struct device *dev, struct
> i2c_timings *t, bool use_de
> t->sda_fall_ns = t->scl_fall_ns;
>
> device_property_read_u32(dev, "i2c-sda-hold-time-ns", >sda_hold_ns);
> +
> + device_property_read_u32(dev, "i2c-digital-filter-width-ns",
> + >digital_filter_width_ns);
> +
> + device_property_read_u32(dev, "i2c-analog-filter-cutoff-frequency",
> + >analog_filter_cutoff_freq_hz);
> }
> EXPORT_SYMBOL_GPL(i2c_parse_fw_timings);
>
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index fa5552c..26ce143 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -575,6 +575,10 @@ struct i2c_lock_operations {
> * @scl_int_delay_ns: time IP core additionally needs to setup SCL in ns
> * @sda_fall_ns: time SDA signal takes to fall in ns; t(f) in the I2C
> specification
> * @sda_hold_ns: time IP core additionally needs to hold SDA in ns
> + * @digital_filter_width_ns: width in ns of spikes on i2c lines that the IP
> core
> + *digital filter can filter out
> + * @analog_filter_cutoff_freq_hz: threshold frequency for the low pass IP
> core
> + analog filter
The indentation is a little bit excessive and also off. Other comments in the
file just uses a single tab after the asterisk in this scenario. Also, the last
of the new lines is missing that leading asterisk.
Cheers,
Peter
> */
> struct i2c_timings {
> u32 bus_freq_hz;
> @@ -583,6 +587,8 @@ struct i2c_timings {
> u32 scl_int_delay_ns;
> u32 sda_fall_ns;
> u32 sda_hold_ns;
> + u32 digital_filter_width_ns;
> + u32 analog_filter_cutoff_freq_hz;
> };
>
> /**
>
Re: [v4,2/2] i2c: mux: pca954x: support property idle-state
On 2019-10-21 10:00, Biwen Li wrote:
> This supports property idle-state
>
You should expand this a little bit to explain that idle-state, if present,
overrides i2c-mux-idle-disconnect. You could also mention your use case
where you need to avoid disconnects on probe/resume.
> Signed-off-by: Biwen Li
> ---
> Change in v4:
> - rename function
> pca954x_calculate_chan -> pca954x_regval
>
> Change in v3:
> - update subject and description
> - add a helper function pca954x_calculate_chan()
>
> Change in v2:
> - update subject and description
> - add property idle-state
>
> drivers/i2c/muxes/i2c-mux-pca954x.c | 59 ++---
> 1 file changed, 36 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c
> b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 923aa3a5a3dc..e566c4cd8ba5 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -86,7 +86,7 @@ struct pca954x {
>
> u8 last_chan; /* last register value */
> /* MUX_IDLE_AS_IS, MUX_IDLE_DISCONNECT or >= 0 for channel */
> - s8 idle_state;
> + s32 idle_state;
>
> struct i2c_client *client;
>
> @@ -229,20 +229,23 @@ static int pca954x_reg_write(struct i2c_adapter *adap,
> I2C_SMBUS_BYTE, );
> }
>
> +static u8 pca954x_regval(struct pca954x *data, u8 chan)
> +{
> + /* we make switches look like muxes, not sure how to be smarter */
I know you are just moving the comment around, but please fix the sentence
to start with a capital letter and end with a period. Sorry I didn't
catch this in v3.
> + if (data->chip->muxtype == pca954x_ismux)
> + return chan | data->chip->enable;
> + else
> + return 1 << chan;
> +}
> +
> static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
> {
> struct pca954x *data = i2c_mux_priv(muxc);
> struct i2c_client *client = data->client;
> - const struct chip_desc *chip = data->chip;
> u8 regval;
> int ret = 0;
>
> - /* we make switches look like muxes, not sure how to be smarter */
> - if (chip->muxtype == pca954x_ismux)
> - regval = chan | chip->enable;
> - else
> - regval = 1 << chan;
> -
> + regval = pca954x_regval(data, (u8)(chan & 0xff));
Both a mask and a cast to do what the compiler should be doing all by itself?
If you need to kill a warning, or something, please do just one or them. But
personally I prefer the short, sweet and uncluttered:
regval = pca954x_regval(data, chan);
> /* Only select the channel if its different from the last channel */
> if (data->last_chan != regval) {
> ret = pca954x_reg_write(muxc->parent, client, regval);
> @@ -256,7 +259,7 @@ static int pca954x_deselect_mux(struct i2c_mux_core
> *muxc, u32 chan)
> {
> struct pca954x *data = i2c_mux_priv(muxc);
> struct i2c_client *client = data->client;
> - s8 idle_state;
> + s32 idle_state;
>
> idle_state = READ_ONCE(data->idle_state);
> if (idle_state >= 0)
> @@ -402,6 +405,17 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
> i2c_mux_del_adapters(muxc);
> }
>
> +static int pca954x_init(struct i2c_client *client, struct pca954x *data)
> +{
> + if (data->idle_state >= 0) {
> + data->last_chan = pca954x_regval(data, (u8)(data->idle_state &
> 0xff));
Dito.
> + } else {
> + /* Disconnect multiplexer */
> + data->last_chan = 0;
> + }
> + return i2c_smbus_write_byte(client, data->last_chan);
Here's another thing I missed in the earlier iterations. If i2c_smbus_write_byte
fails here, I think you should set data->last_chan to zero. For the call from
probe it obviously doesn't matter much, but I think the call during resume is
better off with such extra precaution in place.
Cheers,
Peter
> +}
> +
> /*
> * I2C init/probing/exit functions
> */
> @@ -411,7 +425,6 @@ static int pca954x_probe(struct i2c_client *client,
> struct i2c_adapter *adap = client->adapter;
> struct device *dev = >dev;
> struct device_node *np = dev->of_node;
> - bool idle_disconnect_dt;
> struct gpio_desc *gpio;
> struct i2c_mux_core *muxc;
> struct pca954x *data;
> @@ -462,23 +475,24 @@ static int pca954x_probe(struct i2c_client *client,
> }
> }
>
> - /* Write the mux register at addr to verify
> + data->idle_state = MUX_IDLE_AS_IS;
> + if (of_property_read_u32(np, "idle-state", >idle_state)) {
> + if (np && of_property_read_bool(np, "i2c-mux-idle-disconnect"))
> + data->idle_state = MUX_IDLE_DISCONNECT;
> + }
> +
> + /*
> + * Write the mux register at addr to verify
>* that the mux is in fact present. This also
> - * initializes the mux to disconnected state.
> + * initializes the mux to a
Re: [v3,2/2] i2c: mux: pca954x: support property idle-state
On 2019-10-16 06:09, Biwen Li wrote:
> This supports property idle-state
>
> Signed-off-by: Biwen Li
> ---
> Change in v3:
> - update subject and description
> - add a helper function pca954x_calculate_chan()
>
> Change in v2:
> - update subject and description
> - add property idle-state
>
> drivers/i2c/muxes/i2c-mux-pca954x.c | 64 ++---
> 1 file changed, 39 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c
> b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 923aa3a5a3dc..8777d429269c 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -86,7 +86,7 @@ struct pca954x {
>
> u8 last_chan; /* last register value */
> /* MUX_IDLE_AS_IS, MUX_IDLE_DISCONNECT or >= 0 for channel */
> - s8 idle_state;
> + s32 idle_state;
>
> struct i2c_client *client;
>
> @@ -229,22 +229,25 @@ static int pca954x_reg_write(struct i2c_adapter *adap,
> I2C_SMBUS_BYTE, );
> }
>
> +static int pca954x_calculate_chan(struct pca954x *data, u32 chan)
Should return u8, and "chan" is not what is calculated. Perhaps name
the function pca954x_regval?
(Yes, last_chan is also clearly a bad name, and I suspect you may have
based this name on it, but changing that is a separate patch.)
> +{
> + /* we make switches look like muxes, not sure how to be smarter */
> + if (data->chip->muxtype == pca954x_ismux)
> + return chan | data->chip->enable;
> + else
> + return 1 << chan;
> +}
> +
> static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
> {
> struct pca954x *data = i2c_mux_priv(muxc);
> struct i2c_client *client = data->client;
> - const struct chip_desc *chip = data->chip;
> u8 regval;
> int ret = 0;
>
> - /* we make switches look like muxes, not sure how to be smarter */
> - if (chip->muxtype == pca954x_ismux)
> - regval = chan | chip->enable;
> - else
> - regval = 1 << chan;
> -
> + regval = pca954x_calculate_chan(data, chan);
I think I would have kept the empty line here. Not important...
> /* Only select the channel if its different from the last channel */
> - if (data->last_chan != regval) {
> + if ((data->last_chan & 0xff) != regval) {
The changes on this line are not needed (last_chan and regval are
both u8) and just clutters up the code.
> ret = pca954x_reg_write(muxc->parent, client, regval);
> data->last_chan = ret < 0 ? 0 : regval;
> }
> @@ -256,7 +259,7 @@ static int pca954x_deselect_mux(struct i2c_mux_core
> *muxc, u32 chan)
> {
> struct pca954x *data = i2c_mux_priv(muxc);
> struct i2c_client *client = data->client;
> - s8 idle_state;
> + s32 idle_state;
>
> idle_state = READ_ONCE(data->idle_state);
> if (idle_state >= 0)
> @@ -402,6 +405,23 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
> i2c_mux_del_adapters(muxc);
> }
>
> +static int pca954x_init(struct i2c_client *client, struct pca954x *data)
> +{
> + /*
> + * Write the mux register at addr to verify
> + * that the mux is in fact present. This also
> + * initializes the mux to a channel
> + * or disconnected state.
> + */
Again, this comment belongs in pca954x_probe before the call to this function.
It does not apply (at least not the first sentence) when pca954x_init is called
from pca954x_resume.
Hmmm, it could be argued that specifying MUX_IDLE_AS_IS should not trigger a
disconnect on init (since the mux is always idle at init) and that some other
method should be used to determine if the chip is present. The difference is
that with the idle-state property you can explicitly request MUX_IDLE_AS_IS,
while the old code only had some default behavior if i2c-mux-idle-disconnect
was not present.
The easy way out of this is to, in the binding, document the situation and
say that "idle-state = ;" is not supported but that similar
functionality can be obtained by leaving out both the i2c-mux-idle-disconnect
and idle-state properties.
> + if (data->idle_state >= 0) {
> + data->last_chan = pca954x_calculate_chan(data,
> data->idle_state);
> + } else {
> + /* Disconnect multiplexer */
> + data->last_chan = 0;
> + }
> + return i2c_smbus_write_byte(client, data->last_chan);
> +}
> +
> /*
> * I2C init/probing/exit functions
> */
> @@ -411,7 +431,6 @@ static int pca954x_probe(struct i2c_client *client,
> struct i2c_adapter *adap = client->adapter;
> struct device *dev = >dev;
> struct device_node *np = dev->of_node;
> - bool idle_disconnect_dt;
> struct gpio_desc *gpio;
> struct i2c_mux_core *muxc;
> struct pca954x *data;
> @@ -462,22 +481,18 @@ static int pca954x_probe(struct i2c_client *client,
> }
> }
>
Re: [v3,1/2] dt-bindings: i2c: support property idle-state
On 2019-10-16 06:09, Biwen Li wrote: > This supports property idle-state > > Signed-off-by: Biwen Li > --- > Change in v3: > - update subject and description > - add some information for property idle-state > > Change in v2: > - update subject and description > - add property idle-state > > Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > index 30ac6a60f041..7abda506b828 100644 > --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > @@ -25,6 +25,8 @@ Required Properties: > Optional Properties: > >- reset-gpios: Reference to the GPIO connected to the reset input. > + - idle-state: if present, overrides i2c-mux-idle-disconnect, > +Please refer to Documentation/devicetree/bindings/mux/mux-controller.txt >- i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect > all > children in idle state. This is necessary for example, if there are > several > multiplexers on the bus and the devices behind them use same I2C > addresses. > Rob, should i2c-mux-idle-disconnect perhaps be deprecated here? Is that appropriate? idle-state provides a super-set of what i2c-mux-idle-disconnect provides. Cheers, Peter
Re: [v2,1/2] dt-bindings: i2c: add property idle-state
On 2019-10-15 06:48, Biwen Li wrote: > This adds property idle-state > > Signed-off-by: Biwen Li > --- > Change in v2: > - update subject and description > - add property idle-state > > Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > index 30ac6a60f041..2c7875d338fb 100644 > --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > @@ -34,6 +34,7 @@ Optional Properties: > - first cell is the pin number > - second cell is used to specify flags. > See also > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt > + - idle-state: Please refer to > Documentation/devicetree/bindings/mux/mux-controller.txt > > Example: > > As per my comments on the code, you should mention that idle-state, if present, overrides i2c-mux-idle-disconnect. I also think you should keep idle-state and i2c-mux-idle-disconnect right next to each other. Cheers, Peter
Re: [v2,2/2] i2c: mux: pca954x: add property idle-state
On 2019-10-15 06:48, Biwen Li wrote:
> This adds property idle-state
>
> Signed-off-by: Biwen Li
> ---
> Change in v2:
> - update subject and description
> - add property idle-state
>
> drivers/i2c/muxes/i2c-mux-pca954x.c | 47 ++---
> 1 file changed, 30 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c
> b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 923aa3a5a3dc..8ec586342b92 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -86,7 +86,7 @@ struct pca954x {
>
> u8 last_chan; /* last register value */
> /* MUX_IDLE_AS_IS, MUX_IDLE_DISCONNECT or >= 0 for channel */
> - s8 idle_state;
> + s32 idle_state;
>
> struct i2c_client *client;
>
> @@ -256,7 +256,7 @@ static int pca954x_deselect_mux(struct i2c_mux_core
> *muxc, u32 chan)
> {
> struct pca954x *data = i2c_mux_priv(muxc);
> struct i2c_client *client = data->client;
> - s8 idle_state;
> + s32 idle_state;
>
> idle_state = READ_ONCE(data->idle_state);
> if (idle_state >= 0)
> @@ -402,6 +402,25 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc)
> i2c_mux_del_adapters(muxc);
> }
>
> +static int pca954x_init(struct i2c_client *client, struct pca954x *data)
> +{
> + /*
> + * Write the mux register at addr to verify
> + * that the mux is in fact present. This also
> + * initializes the mux to disconnected state.
> + */
This comment belongs in pca954x_probe, before the call to this function.
However, the comment may now be be wrong since the mux is not always
initialized to the disconnected state.
> + if (data->idle_state >= 0) {
> + /* Always enable multiplexer */
While I understand that it is important for your case that the mux is
always enabled, this is just a side effect of having a specific
idle-state. I suggest that you remove this comment.
> + if (data->chip->muxtype == pca954x_ismux)
> + data->last_chan = data->idle_state | data->chip->enable;
> + else
> + data->last_chan = 1 << data->idle_state;
The meat of this "if" is still duplicated, I was suggesting a helper
that only did the regval calculation so that the new helper could
also be used from pca954x_select_chan.
> + } else {
> + /* Disconnect multiplexer */
> + data->last_chan = 0; /* force the first selection */
These two comments should be combined.
> + }
> + return i2c_smbus_write_byte(client, data->last_chan);
> +}
> /*
> * I2C init/probing/exit functions
> */
> @@ -411,7 +430,6 @@ static int pca954x_probe(struct i2c_client *client,
> struct i2c_adapter *adap = client->adapter;
> struct device *dev = >dev;
> struct device_node *np = dev->of_node;
> - bool idle_disconnect_dt;
> struct gpio_desc *gpio;
> struct i2c_mux_core *muxc;
> struct pca954x *data;
> @@ -462,22 +480,18 @@ static int pca954x_probe(struct i2c_client *client,
> }
> }
>
> - /* Write the mux register at addr to verify
> - * that the mux is in fact present. This also
> - * initializes the mux to disconnected state.
> - */
> - if (i2c_smbus_write_byte(client, 0) < 0) {
> + if (of_property_read_u32(np, "idle-state", >idle_state))
> + data->idle_state = MUX_IDLE_AS_IS;
> +
> + if (of_property_read_bool(np, "i2c-mux-idle-disconnect"))
> + data->idle_state = MUX_IDLE_DISCONNECT;
I think you should ignore i2c-mux-idle-disconnect if idle-state is present.
I.e. move this "if" statement into the body of the former "if". Also, you have
broken things if np is NULL.
Cheers,
Peter
> +
> + ret = pca954x_init(client, data);
> + if (ret < 0) {
> dev_warn(dev, "probe failed\n");
> return -ENODEV;
> }
>
> - data->last_chan = 0; /* force the first selection */
> - data->idle_state = MUX_IDLE_AS_IS;
> -
> - idle_disconnect_dt = np &&
> - of_property_read_bool(np, "i2c-mux-idle-disconnect");
> - if (idle_disconnect_dt)
> - data->idle_state = MUX_IDLE_DISCONNECT;
>
> ret = pca954x_irq_setup(muxc);
> if (ret)
> @@ -531,8 +545,7 @@ static int pca954x_resume(struct device *dev)
> struct i2c_mux_core *muxc = i2c_get_clientdata(client);
> struct pca954x *data = i2c_mux_priv(muxc);
>
> - data->last_chan = 0;
> - return i2c_smbus_write_byte(client, 0);
> + return pca954x_init(client, data);
> }
> #endif
>
>
Re: [PATCH 2/2] i2c: mux: pca954x: replace property i2c-mux-idle-disconnect
On 2019-10-14 13:25, Biwen Li wrote:
> This replaces property i2c-mux-idle-disconnect with idle-state
>
> Signed-off-by: Biwen Li
> ---
> drivers/i2c/muxes/i2c-mux-pca954x.c | 44 -
> 1 file changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c
> b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 923aa3a5a3dc..a330929c4d67 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -86,7 +86,7 @@ struct pca954x {
>
> u8 last_chan; /* last register value */
> /* MUX_IDLE_AS_IS, MUX_IDLE_DISCONNECT or >= 0 for channel */
> - s8 idle_state;
> + s32 idle_state;
>
> struct i2c_client *client;
>
> @@ -256,7 +256,7 @@ static int pca954x_deselect_mux(struct i2c_mux_core
> *muxc, u32 chan)
> {
> struct pca954x *data = i2c_mux_priv(muxc);
> struct i2c_client *client = data->client;
> - s8 idle_state;
> + s32 idle_state;
>
> idle_state = READ_ONCE(data->idle_state);
> if (idle_state >= 0)
> @@ -411,7 +411,6 @@ static int pca954x_probe(struct i2c_client *client,
> struct i2c_adapter *adap = client->adapter;
> struct device *dev = >dev;
> struct device_node *np = dev->of_node;
> - bool idle_disconnect_dt;
> struct gpio_desc *gpio;
> struct i2c_mux_core *muxc;
> struct pca954x *data;
> @@ -462,22 +461,31 @@ static int pca954x_probe(struct i2c_client *client,
> }
> }
>
> + if (of_property_read_u32(np, "idle-state", >idle_state))
> + data->idle_state = MUX_IDLE_AS_IS;
> +
> /* Write the mux register at addr to verify
>* that the mux is in fact present. This also
>* initializes the mux to disconnected state.
>*/
> - if (i2c_smbus_write_byte(client, 0) < 0) {
> + if (data->idle_state >= 0){
Space before {
> + data->last_chan = data->idle_state;
data->last_chan should have the actual register value, i.e. (untested)
if (data->chip->muxtype == pca954x_ismux)
data->last_chan = data->idle_state | data->chip->enable;
else
data->last_chan = 1 << data->idle_state;
ret = i2c_smbus_write_byte(client, data->last_chan);
But since this regval calculation is now needed in three places, it should
be moved to a helper function.
> + /* Always enable multiplexer */
> + ret = i2c_smbus_write_byte(client, data->last_chan |
> + (data->chip->muxtype == pca954x_ismux ?
> + data->chip->enable : 0));
> + }
> + else{
Space before {
Naturally, you have the exact same issues in the pca954x_resume hunk, below.
> + data->last_chan = 0; /* force the first selection
> */
> + /* Disconnect multiplexer */
> + ret = i2c_smbus_write_byte(client, data->last_chan);
> + }
> +
> + if (ret < 0) {
> dev_warn(dev, "probe failed\n");
> return -ENODEV;
> }
>
> - data->last_chan = 0; /* force the first selection */
> - data->idle_state = MUX_IDLE_AS_IS;
> -
> - idle_disconnect_dt = np &&
> - of_property_read_bool(np, "i2c-mux-idle-disconnect");
> - if (idle_disconnect_dt)
> - data->idle_state = MUX_IDLE_DISCONNECT;
In case the idle-state property is missing, you need to fall back to the
old behavior and handle i2c-mux-idle-disconnect as before.
Cheers,
Peter
>
> ret = pca954x_irq_setup(muxc);
> if (ret)
> @@ -531,8 +539,18 @@ static int pca954x_resume(struct device *dev)
> struct i2c_mux_core *muxc = i2c_get_clientdata(client);
> struct pca954x *data = i2c_mux_priv(muxc);
>
> - data->last_chan = 0;
> - return i2c_smbus_write_byte(client, 0);
> + if (data->idle_state >= 0){
> + data->last_chan = data->idle_state;
> + /* Always enable multiplexer */
> + return i2c_smbus_write_byte(client, data->last_chan |
> + (data->chip->muxtype == pca954x_ismux ?
> + data->chip->enable : 0));
> + }
> + else{
> + data->last_chan = 0;
> + /* Disconnect multiplexer */
> + return i2c_smbus_write_byte(client, data->last_chan);
> + }
> }
> #endif
>
>
Re: [PATCH 1/2] dt-bindings: i2c: replace property i2c-mux-idle-disconnect
On 2019-10-14 13:25, Biwen Li wrote: > This replaces property i2c-mux-idle-disconnect with idle-state > > Signed-off-by: Biwen Li > --- > Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > index 30ac6a60f041..f2db517b1635 100644 > --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > @@ -25,9 +25,7 @@ Required Properties: > Optional Properties: > >- reset-gpios: Reference to the GPIO connected to the reset input. > - - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect > all > -children in idle state. This is necessary for example, if there are > several > -multiplexers on the bus and the devices behind them use same I2C > addresses. > + - idle-state: Please refer to > Documentation/devicetree/bindings/mux/mux-controller.txt >- interrupts: Interrupt mapping for IRQ. >- interrupt-controller: Marks the device node as an interrupt controller. >- #interrupt-cells : Should be two. > You can't just remove i2c-mux-idle-disconnect. It needs to remain, and the driver needs to maintain support for this in case a new kernel is running with an old devicetree. Cheers, Peter
Re: [EXT] Re: [v2,2/2] dt-bindings: i2c-mux-pca954x: Add optional property i2c-mux-never-disable
On 2019-10-14 06:16, Biwen Li wrote: >> >>> >>> On Mon, Sep 30, 2019 at 11:25:03AM +0800, Biwen Li wrote: The patch adds an optional property i2c-mux-never-disable Signed-off-by: Biwen Li --- Change in v2: - update documentation Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt index 30ac6a60f041..71b73d0fdb62 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt @@ -34,6 +34,7 @@ Optional Properties: - first cell is the pin number - second cell is used to specify flags. See also Documentation/devicetree/bindings/interrupt-controller/interrupts.tx t + - i2c-mux-never-disable: always forces mux to be enabled. >>> >>> Either needs to have a vendor prefix or be documented as a common >>> property. > I choose to be documented as a common property. Can we please just drop the never-disable approach and focus on idle-state instead? >>> >>> IIRC, we already have a property for mux default state which seems >>> like that would cover this unless you need to leave it in different states. >> Okay, you are right, thank you so much. I will try it in v3. > Do you mean that the property is i2c-mux-idle-disconnect in > Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt? > If so, the property i2c-mux-idle-disconnect is not good for me. > Because condition of the property i2c-mux-idle-disconnect is in idle > state(sometimes). > But I need always enable i2c multiplexer in whatever state(anytime), so I add > a common property i2c-mux-never-disable. No, I do not think any new property is needed. AFAICT, idle-state fits perfectly, and I will not consider this i2c-mux-never-disable approach until some compelling reason is presented why idle-state is not appropriate. You promised to take a stab at it, and until I hear back on that, this series is on hold. As indicated here [1]. You need to patch the driver to look at the idle-state property instead of inventing a new (and less flexible) property. If you implement idle-state for this driver and set the idle-state to some channel in the dts, the mux will never disconnect. Problem solved. Perhaps not your first solution, but it does solve your problem and may actually be useful for other purposes than your broken hardware. And it is consistent across other i2c-muxes. I see no downside. Cheers, Peter [1] https://lore.kernel.org/lkml/07d85748-0721-39d4-d2be-13eb16b0f...@axentia.se/
Re: [EXT] Re: [PATCH] i2c: pca954x: Add property to skip disabling PCA954x MUX device
On 2019-09-30 04:43, Biwen Li wrote: >> >> On 2019-09-29 12:36, Biwen Li wrote: >>> On some Layerscape boards like LS2085ARDB and LS2088ARDB, input >>> pull-up resistors on PCA954x MUX device are missing on board, So, if >>> MUX are disabled after powered-on, input lines will float leading to >>> incorrect functionality. SDA and SCL are not "inputs". They are part of a bus and are both bidirectional signals. Speaking of input signals in an I2C context is ambiguous. >> >> Hi! >> >> Are you saying that the parent bus of the mux is relying on some pull-ups >> inside >> the mux? > Yes, as follows: > > VCC > > --- > > |--- > || > > \\ > > /10K resister / 10k resister > > \\ > > || > || >I2C1_SCL I2C1_SCL > | -- > |SCL | > |SCL | >I2C1_SDA | PCA9547 |I2C1_SDA | | > PCA9547 | > |SDA | > |---|SDA | > >-- > --wrong design(need software fix as above or hardware fix)-- > --proper design-- Ok, got it (but the "picture" didn't help). >> >>> Hence, PCA954x MUX device should never be turned-off after power-on. >>> >>> Add property to skip disabling PCA954x MUX device if device tree >>> contains "i2c-mux-never-disable" >>> for PCA954x device node. >>> >>> Errata ID: E-00013 on board LS2085ARDB and LS2088ARDB (Board revision >>> found on Rev.B, Rev.C and Rev.D) >> >> I think you should follow the example of the i2c-mux-gpio driver and >> implement >> the idle-state property instead. >> >> That is a lot more consistent, assuming it solves the problem at hand? > Got it, thanks, I will try it. I'll wait for that patch then, instead of spending more energy on the never-disable approach. Cheers, Peter
Re: [PATCH] i2c: pca954x: Add property to skip disabling PCA954x MUX device
On 2019-09-29 12:36, Biwen Li wrote:
> On some Layerscape boards like LS2085ARDB and LS2088ARDB,
> input pull-up resistors on PCA954x MUX device are missing on board,
> So, if MUX are disabled after powered-on, input lines will float
> leading to incorrect functionality.
Hi!
Are you saying that the parent bus of the mux is relying on some
pull-ups inside the mux?
> Hence, PCA954x MUX device should never be turned-off after
> power-on.
>
> Add property to skip disabling PCA954x MUX device
> if device tree contains "i2c-mux-never-disable"
> for PCA954x device node.
>
> Errata ID: E-00013 on board LS2085ARDB and LS2088ARDB
> (Board revision found on Rev.B, Rev.C and Rev.D)
I think you should follow the example of the i2c-mux-gpio driver
and implement the idle-state property instead.
That is a lot more consistent, assuming it solves the problem
at hand?
>
> Signed-off-by: Biwen Li
> ---
> drivers/i2c/muxes/i2c-mux-pca954x.c | 33 +
> 1 file changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c
> b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 923aa3a5a3dc..ea8aca54d572 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -93,6 +93,7 @@ struct pca954x {
> struct irq_domain *irq;
> unsigned int irq_mask;
> raw_spinlock_t lock;
> + u8 disable_mux; /* do not disable mux if val not 0 */
Awful number of negations there. The name is also backwards given that
a non-zero value means that the mux should *not* be disabled. I would
have reused the name from the binding.
bool never_disable;
A bit less confusing...
> };
>
> /* Provide specs for the PCA954x types we know about */
> @@ -258,6 +259,11 @@ static int pca954x_deselect_mux(struct i2c_mux_core
> *muxc, u32 chan)
> struct i2c_client *client = data->client;
> s8 idle_state;
>
> + if (data->disable_mux != 0) {
Please drop " != 0" and use the variable as a truth value. More
instances below...
> + data->last_chan = data->chip->nchans;
> + return pca954x_reg_write(muxc->parent, client,
> data->disable_mux);
> + }
> +
> idle_state = READ_ONCE(data->idle_state);
> if (idle_state >= 0)
> /* Set the mux back to a predetermined channel */
> @@ -462,16 +468,32 @@ static int pca954x_probe(struct i2c_client *client,
> }
> }
>
> + /* Errata ID E-00013 on board LS2088ARDB and LS2088ARDB:
> + * The point here is that you must not disable a mux if there
> + * are no pullups on the input or you mess up the I2C. This
> + * needs to be put into the DTS really as the kernel cannot
> + * know this otherwise.
> + */
> +
> + data->disable_mux = np &&
> + of_property_read_bool(np, "i2c-mux-never-disable") &&
i2c-mux-never-disable needs to be documented. However see above for my
point that you should do it like the i2c-mux-gpio driver. Any usage
of idle-state still needs to be documented for pca954x binding.
> + data->chip->muxtype == pca954x_ismux ?
> + data->chip->enable : 0;
Why do you not allow never-disable for switches?
Cheers,
Peter
> +
> /* Write the mux register at addr to verify
>* that the mux is in fact present. This also
>* initializes the mux to disconnected state.
>*/
> - if (i2c_smbus_write_byte(client, 0) < 0) {
> + if (i2c_smbus_write_byte(client, data->disable_mux) < 0) {
> dev_warn(dev, "probe failed\n");
> return -ENODEV;
> }
>
> - data->last_chan = 0; /* force the first selection */
> + if (data->disable_mux != 0)
> + data->last_chan = data->chip->nchans;
> + else
> + data->last_chan = 0; /* force the first selection
> */
> +
> data->idle_state = MUX_IDLE_AS_IS;
>
> idle_disconnect_dt = np &&
> @@ -531,8 +553,11 @@ static int pca954x_resume(struct device *dev)
> struct i2c_mux_core *muxc = i2c_get_clientdata(client);
> struct pca954x *data = i2c_mux_priv(muxc);
>
> - data->last_chan = 0;
> - return i2c_smbus_write_byte(client, 0);
> + if (data->disable_mux != 0)
> + data->last_chan = data->chip->nchans;
> + else
> + data->last_chan = 0;
> + return i2c_smbus_write_byte(client, data->disable_mux);
> }
> #endif
>
>
Re: [PATCH] dt-bindings: at24: convert the binding document to yaml
On 2019-09-23 20:34, Bartosz Golaszewski wrote: > pon., 23 wrz 2019 o 20:30 Peter Rosin napisał(a): >> >> which is no longer allowed. That might be a problem? The previous binding >> also allows less e.g. >> >> compatible = "atmel,24c00", "renesas,24mac402"; >> > > Right, but I'm not really sure how to express fallbacks in yaml. Any hint? Absolutely no idea what-so-ever. Sorry... Cheers, Peter
Re: [PATCH] dt-bindings: at24: convert the binding document to yaml
On 2019-09-23 19:52, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski
>
> Convert the binding document for at24 EEPROMs from txt to yaml. The
> compatible property uses a regex pattern to address all the possible
> combinations of "vendor,model" strings.
>
> Signed-off-by: Bartosz Golaszewski
> ---
> .../devicetree/bindings/eeprom/at24.txt | 90 +--
> .../devicetree/bindings/eeprom/at24.yaml | 107 ++
> MAINTAINERS | 2 +-
> 3 files changed, 109 insertions(+), 90 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/eeprom/at24.yaml
>
> diff --git a/Documentation/devicetree/bindings/eeprom/at24.txt
> b/Documentation/devicetree/bindings/eeprom/at24.txt
> index 22aead844d0f..c94acbb8cb0c 100644
> --- a/Documentation/devicetree/bindings/eeprom/at24.txt
> +++ b/Documentation/devicetree/bindings/eeprom/at24.txt
> @@ -1,89 +1 @@
> -EEPROMs (I2C)
> -
> -Required properties:
> -
> - - compatible: Must be a "," pair. The following
>
> -values are supported (assuming "atmel" as manufacturer):
> -
> -"atmel,24c00",
> -"atmel,24c01",
> -"atmel,24cs01",
> -"atmel,24c02",
> -"atmel,24cs02",
> -"atmel,24mac402",
> -"atmel,24mac602",
> -"atmel,spd",
> -"atmel,24c04",
> -"atmel,24cs04",
> -"atmel,24c08",
> -"atmel,24cs08",
> -"atmel,24c16",
> -"atmel,24cs16",
> -"atmel,24c32",
> -"atmel,24cs32",
> -"atmel,24c64",
> -"atmel,24cs64",
> -"atmel,24c128",
> -"atmel,24c256",
> -"atmel,24c512",
> -"atmel,24c1024",
> -"atmel,24c2048",
> -
> -If is not "atmel", then a fallback must be
> used
> -with the same and "atmel" as manufacturer.
> -
> -Example:
> -compatible = "microchip,24c128", "atmel,24c128";
> -
> -Supported manufacturers are:
> -
> -"catalyst",
> -"microchip",
> -"nxp",
> -"ramtron",
> -"renesas",
> -"rohm",
> -"st",
> -
> -Some vendors use different model names for chips which are
> just
> -variants of the above. Known such exceptions are listed
> below:
> -
> -"nxp,se97b" - the fallback is "atmel,24c02",
> -"renesas,r1ex24002" - the fallback is "atmel,24c02"
> -"renesas,r1ex24016" - the fallback is "atmel,24c16"
> -"renesas,r1ex24128" - the fallback is "atmel,24c128"
> -"rohm,br24t01" - the fallback is "atmel,24c01"
> -
> - - reg: The I2C address of the EEPROM.
> -
> -Optional properties:
> -
> - - pagesize: The length of the pagesize for writing. Please consult the
> - manual of your device, that value varies a lot. A wrong value
> - may result in data loss! If not specified, a safety value of
> - '1' is used which will be very slow.
> -
> - - read-only: This parameterless property disables writes to the eeprom.
> -
> - - size: Total eeprom size in bytes.
> -
> - - no-read-rollover: This parameterless property indicates that the
> - multi-address eeprom does not automatically roll over
> - reads to the next slave address. Please consult the
> - manual of your device.
> -
> - - wp-gpios: GPIO to which the write-protect pin of the chip is connected.
> -
> - - address-width: number of address bits (one of 8, 16).
> -
> - - num-addresses: total number of i2c slave addresses this device takes
> -
> -Example:
> -
> -eeprom@52 {
> - compatible = "atmel,24c32";
> - reg = <0x52>;
> - pagesize = <32>;
> - wp-gpios = < 3 0>;
> - num-addresses = <8>;
> -};
> +This file has been moved to at24.yaml.
> diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml
> b/Documentation/devicetree/bindings/eeprom/at24.yaml
> new file mode 100644
> index ..28c8b068c8a1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
> @@ -0,0 +1,107 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright 2019 BayLibre SAS
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/eeprom/at24.yaml#;
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#;
> +
> +title: I2C EEPROMs compatible with Atmel's AT24
> +
> +maintainers:
> + - Bartosz Golaszewski
> +
> +properties:
> + compatible:
> +additionalItems: true
> +maxItems: 2
> +pattern:
> "^(atmel|catalyst|microchip|nxp|ramtron|renesas|rohm|st),24(c|cs|mac)[0-9]+$"
> +oneOf:
> + - const: nxp,se97b
> + -
Re: [PATCH] of: restore old handling of cells_name=NULL in of_*_phandle_with_args()
On 2019-09-18 08:38, Uwe Kleine-König wrote:
> From: Uwe Kleine-König
>
> Before commit e42ee61017f5 ("of: Let of_for_each_phandle fallback to
> non-negative cell_count") the iterator functions calling
> of_for_each_phandle assumed a cell count of 0 if cells_name was NULL.
> This corner case was missed when implementing the fallback logic in
> e42ee61017f5 and resulted in an endless loop.
>
> Restore the old behaviour of of_count_phandle_with_args() and
> of_parse_phandle_with_args() and add a check to
> of_phandle_iterator_init() to prevent a similar failure as a safety
> precaution. of_parse_phandle_with_args_map() doesn't need a similar fix
> as cells_name isn't NULL there.
>
> Affected drivers are:
> - drivers/base/power/domain.c
> - drivers/base/power/domain.c
> - drivers/clk/ti/clk-dra7-atl.c
> - drivers/hwmon/ibmpowernv.c
> - drivers/i2c/muxes/i2c-demux-pinctrl.c
> - drivers/iommu/mtk_iommu.c
> - drivers/net/ethernet/freescale/fman/mac.c
> - drivers/opp/of.c
> - drivers/perf/arm_dsu_pmu.c
> - drivers/regulator/of_regulator.c
> - drivers/remoteproc/imx_rproc.c
> - drivers/soc/rockchip/pm_domains.c
> - sound/soc/fsl/imx-audmix.c
> - sound/soc/fsl/imx-audmix.c
> - sound/soc/meson/axg-card.c
> - sound/soc/samsung/tm2_wm5110.c
> - sound/soc/samsung/tm2_wm5110.c
>
> Thanks to Geert Uytterhoeven for reporting the issue, Peter Rosin for
> helping pinpoint the actual problem and the testers for confirming this
> fix.
>
> Fixes: e42ee61017f5 ("of: Let of_for_each_phandle fallback to non-negative
> cell_count")
> Tested-by: Marek Szyprowski
> Tested-by: Geert Uytterhoeven
> Signed-off-by: Uwe Kleine-König
> ---
> Hello,
>
> compared to the untested patch I sent yesterday I also fixed
> of_parse_phandle_with_args which has three users that pass
> cells_name=NULL. (i.e. drivers/clk/ti/clk-dra7-atl.c,
> sound/soc/fsl/imx-audmix.c, sound/soc/samsung/tm2_wm5110.c) I didn't
> look closely, but maybe these could be converted to use of_parse_phandle
> as there are no arguments to be processed with no cells_name?!
>
> Best regards
> Uwe
>
> drivers/of/base.c | 30 --
> 1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 2f25d2dfecfa..25ee07c0a3cd 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1286,6 +1286,13 @@ int of_phandle_iterator_init(struct
> of_phandle_iterator *it,
>
> memset(it, 0, sizeof(*it));
>
> + /*
> + * one of cell_count or cells_name must be provided to determine the
> + * argument length.
> + */
> + if (cell_count < 0 && !cells_name)
> + return -EINVAL;
> +
> list = of_get_property(np, list_name, );
> if (!list)
> return -ENOENT;
> @@ -1512,10 +1519,17 @@ int of_parse_phandle_with_args(const struct
> device_node *np, const char *list_na
> const char *cells_name, int index,
> struct of_phandle_args *out_args)
> {
> + int cell_count = -1;
> +
> if (index < 0)
> return -EINVAL;
> - return __of_parse_phandle_with_args(np, list_name, cells_name, -1,
> - index, out_args);
> +
> + /* If cells_name if NULL we assume a cell count of 0 */
> + if (!cells_name)
> + cell_count = 0;
> +
> + return __of_parse_phandle_with_args(np, list_name, cells_name,
> + cell_count, index, out_args);
> }
> EXPORT_SYMBOL(of_parse_phandle_with_args);
>
> @@ -1765,6 +1779,18 @@ int of_count_phandle_with_args(const struct
> device_node *np, const char *list_na
> struct of_phandle_iterator it;
> int rc, cur_index = 0;
>
> + /* If cells_name is NULL we assume a cell count of 0 */
> + if (cells_name == NULL) {
A couple of nits.
I don't know if there are other considerations, but in the previous two
hunks you use !cells_name instead of comparing explicitly with NULL.
Personally, I find the shorter form more readable, and in the name of
consistency bla bla...
Also, the comment explaining this NULL-check didn't really make sense
to me until I realized that knowing the cell count to be zero makes
counting trivial. Something along those lines should perhaps be in the
comment?
But as I said, these are nits. Feel free to ignore.
Cheers,
Peter
> + const __be32 *list;
> + int size;
> +
> + list = of_get_property(np, list_name, );
> + if (!list)
> + return -ENOENT;
> +
> + return size / sizeof(*list);
> + }
> +
> rc = of_phandle_iterator_init(, np, list_name, cells_name, -1);
> if (rc)
> return rc;
>
Re: [PATCH v1 2/2] of: Let of_for_each_phandle fallback to non-negative cell_count
On 2019-09-17 12:13, Uwe Kleine-König wrote:
> Hello Geert,
>
> On Tue, Sep 17, 2019 at 11:40:25AM +0200, Geert Uytterhoeven wrote:
>> Hi Rob, Uwe,
>>
>> On Fri, Sep 13, 2019 at 11:58 PM Rob Herring wrote:
>>> On Sat, 24 Aug 2019 15:28:46 +0200, =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?=
>>> wrote:
Referencing device tree nodes from a property allows to pass arguments.
This is for example used for referencing gpios. This looks as follows:
gpio_ctrl: gpio-controller {
#gpio-cells = <2>
...
}
someothernode {
gpios = <_ctrl 5 0 _ctrl 3 0>;
...
}
To know the number of arguments this must be either fixed, or the
referenced node is checked for a $cells_name (here: "#gpio-cells")
property and with this information the start of the second reference can
be determined.
Currently regulators are referenced with no additional arguments. To
allow some optional arguments without having to change all referenced
nodes this change introduces a way to specify a default cell_count. So
when a phandle is parsed we check for the $cells_name property and use
it as before if present. If it is not present we fall back to
cells_count if non-negative and only fail if cells_count is smaller than
zero.
Signed-off-by: Uwe Kleine-König
>>
>> This is now commit e42ee61017f58cd9 ("of: Let of_for_each_phandle fallback
>> to non-negative cell_count") in robh/for-next, which causes a lock-up when
>> booting a shmobile_defconfig kernel on r8a7791/koelsch:
>>
>> rcu: INFO: rcu_sched self-detected stall on CPU
>> rcu: 0-: (2099 ticks this GP) idle=6fe/1/0x4002
>> softirq=29/29 fqs=1050
>> (t=2100 jiffies g=-1131 q=0)
>> NMI backtrace for cpu 0
>> CPU: 0 PID: 1 Comm: swapper/0 Not tainted
>> 5.3.0-rc2-shmobile-00050-ge42ee61017f58cd9 #376
>> Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
>> [] (unwind_backtrace) from [] (show_stack+0x10/0x14)
>> [] (show_stack) from [] (dump_stack+0x7c/0x9c)
>> [] (dump_stack) from [] (nmi_cpu_backtrace+0xa0/0xb8)
>> [] (nmi_cpu_backtrace) from []
>> (nmi_trigger_cpumask_backtrace+0x84/0x114)
>> [] (nmi_trigger_cpumask_backtrace) from []
>> (rcu_dump_cpu_stacks+0xac/0xc8)
>> [] (rcu_dump_cpu_stacks) from []
>> (rcu_sched_clock_irq+0x2ac/0x6b4)
>> [] (rcu_sched_clock_irq) from []
>> (update_process_times+0x30/0x5c)
>> [] (update_process_times) from []
>> (tick_nohz_handler+0xcc/0x120)
>> [] (tick_nohz_handler) from []
>> (arch_timer_handler_virt+0x28/0x30)
>> [] (arch_timer_handler_virt) from []
>> (handle_percpu_devid_irq+0xe8/0x21c)
>> [] (handle_percpu_devid_irq) from []
>> (generic_handle_irq+0x18/0x28)
>> [] (generic_handle_irq) from []
>> (__handle_domain_irq+0xa0/0xb4)
>> [] (__handle_domain_irq) from []
>> (gic_handle_irq+0x58/0x90)
>> [] (gic_handle_irq) from [] (__irq_svc+0x6c/0x90)
>> Exception stack(0xeb08dd30 to 0xeb08dd78)
>> dd20: c0cc7514 2013 0005 3b27
>> dd40: eb7c4020 c0cc750c 0051 0051 2013 c0c66b08 eb1cdc00 0018
>> dd60: eb08dd80 c05c1a38 c0756c00 2013
>> [] (__irq_svc) from []
>> (_raw_spin_unlock_irqrestore+0x1c/0x20)
>> [] (_raw_spin_unlock_irqrestore) from []
>> (of_find_node_by_phandle+0xcc/0xf0)
>> [] (of_find_node_by_phandle) from []
>> (of_phandle_iterator_next+0x68/0x178)
>> [] (of_phandle_iterator_next) from []
>> (of_count_phandle_with_args+0x5c/0x7c)
>> [] (of_count_phandle_with_args) from []
>> (i2c_demux_pinctrl_probe+0x24/0x1fc)
>> [] (i2c_demux_pinctrl_probe) from []
>> (platform_drv_probe+0x48/0x94)
>> [] (platform_drv_probe) from []
>> (really_probe+0x1f0/0x2b8)
>> [] (really_probe) from []
>> (driver_probe_device+0x140/0x158)
>> [] (driver_probe_device) from []
>> (device_driver_attach+0x44/0x5c)
>> [] (device_driver_attach) from []
>> (__driver_attach+0xac/0xb4)
>> [] (__driver_attach) from [] (bus_for_each_dev+0x64/0xa0)
>> [] (bus_for_each_dev) from []
>> (bus_add_driver+0x148/0x1a8)
>> [] (bus_add_driver) from [] (driver_register+0xac/0xf0)
>> [] (driver_register) from [] (do_one_initcall+0xa8/0x1d4)
>> [] (do_one_initcall) from []
>> (kernel_init_freeable+0x26c/0x2c8)
>> [] (kernel_init_freeable) from [] (kernel_init+0x8/0x10c)
>> [] (kernel_init) from [] (ret_from_fork+0x14/0x2c)
>> Exception stack(0xeb08dfb0 to 0xeb08dff8)
>> dfa0:
>> dfc0:
>> dfe0: 0013
>>
>> Presumably it loops forever, due to a conversion of -1 to unsigned
>> somewhere?
>
> Hmm, I fail to see the culprit. i2c_demux_pinctrl_probe calls
> of_count_phandle_with_args with cells_name=NULL. With that I don't see
> how my patch changes anything as the only change is
Re: [PATCH v3 1/1] i2c: qcom-geni: Provide an option to disable DMA processing
On 2019-09-05 15:49, Wolfram Sang wrote:
> Hi Lee,
>
> I understand you are in a hurry, but please double check before
> sending...
Linus indicated that an rc8 is coming up, which should provide an extra week.
https://lwn.net/Articles/798152/
> On Thu, Sep 05, 2019 at 11:22:47AM +0100, Lee Jones wrote:
>> We have a production-level laptop (Lenovo Yoga C630) which is exhibiting
>> a rather horrific bug. When I2C HID devices are being scanned for at
>> boot-time the QCom Geni based I2C (Serial Engine) attempts to use DMA.
>> When it does, the laptop reboots and the user never sees the OS.
>>
>> The beautiful thing about this approach is that, *if* the Geni SE DMA
>> ever starts working, we can remove the C code and any old properties
>> left in older DTs just become NOOP. Older kernels with newer DTs (less
>> of a priority) *still* will not work - but they do not work now anyway.
>
> ... becasue this paragraph doesn't fit anymore. Needs to be reworded.
>
>>
>> Fixes: 8bc529b25354 ("soc: qcom: geni: Add support for ACPI")
>
> As said in the other thread, I don't get it, but this is not a show
> stopper for me.
WAG: because ACPI made some driver load at all, and when it
did it something started happening which crashed some machines.
Cheers,
Peter
Re: [RFC,v2 2/6] i2c: add I2C Address Translator (ATR) support
Hi! [ Sorry about my absence. I've been meaning to comment on this series for a long time, but work and family keep interfering... ] On 2019-09-03 09:31, Luca Ceresoli wrote: > Hi Jacopo, > > thanks for your feedback. > > On 01/09/19 16:31, jacopo mondi wrote: >> Hi Luca, >>thanks for keep pushing this series! I hope we can use part of this >> for the (long time) on-going GMSL work... >> >> I hope you will be patient enough to provide (another :) overview >> of this work during the BoF Wolfram has organized at LPC for the next >> week. > > Sure! > >> In the meantime I would have some comments after having a read at the >> series and trying to apply its concept to GMSL >> >> On Tue, Jul 23, 2019 at 10:37:19PM +0200, Luca Ceresoli wrote: >>> An ATR is a device that looks similar to an i2c-mux: it has an I2C >>> slave "upstream" port and N master "downstream" ports, and forwards >>> transactions from upstream to the appropriate downstream port. But is >>> is different in that the forwarded transaction has a different slave >>> address. The address used on the upstream bus is called the "alias" >>> and is (potentially) different from the physical slave address of the >>> downstream chip. >>> >>> Add a helper file (just like i2c-mux.c for a mux or switch) to allow >>> implementing ATR features in a device driver. The helper takes care or >>> adapter creation/destruction and translates addresses at each transaction. >>> >>> Signed-off-by: Luca Ceresoli >>> >>> --- >>> >>> Changes RFCv1 -> RFCv2: >>> >>> RFCv1 was implemented inside i2c-mux.c and added yet more complexity >>> there. RFCv2 creates a new file on its own, i2c-atr.c. Since many ATR >>> features are not in a MUX and vice versa, the overlapping is low. This was >>> almost a complete rewrite, but for the records here are the main >>> differences from the old implementation: >> >> I'm not an i2c expert, but this looks very similar to me to an >> augmented i2c-mux with the following additional features: >> - automatic selection of the i2c address to use for the children >> behind the mux >> - automatic translation of the addresses the logical aliases to >> the actual physical addresses of the slaves (with the help of the >> deserializer address translation feature in this case). > > A notable difference in the hardware is that a mux needs an explicit > procedure to select a port. That's why the select() op is mandatory for > muxes. The ATR, at least in the DS90UB9xx case, selects the port > automatically based on the slave address. So I added an optional > select() op in the atr, but I suspect it's useless for "real" ATRs. > > More differences derive from how Linux implements muxes. The i2c-mux > code has several flags for different locking schemes, and it has a It's two locking schemes if you count them carefully, so several is a bit of an exaggeration. But agreed, two is more than I prefer. > fairly complex DT parsing scheme. These are mostly a historical burden. > Adding the ATR features to i2c-mux.c was very tricky and error-prone due > to all of this code, that's why I have moved ATR to its own file in RFCv2. Anyway, the locking in i2c-mux may be complex, but it does solve real problems. The way I read this series, these problems are not dealt with and therefore the ATR code will not function in all surroundings. Some things to think about: - What happens when you put a mux behind an ATR? - What happens when you put an ATR behind a mux? - What happens when you put an ATR between two muxes? - Does it make a difference if the mux is parent-locked or mux-locked? - What happens if client drivers lock the adapter in order to silence the bus for some reason or to keep two xfers together or something, and then do unlocked xfers? - Can you put an ATR behind another ATR? etc I'm not saying that these things must be handled, and maybe they are handled already, and maybe some of the combinations are not valid at all. But the possibilities and limitations should be understood. And preferably documented. My gut feeling (without spending much time on it) is that ATR as implemented in this series behave approximately like mux-locked muxes, meaning that it is not obviously safe to put a parent-locked mux behind an ATR and making it impossible for client devices behind an ATR to force xfers to happen back-to-back from the root adapter. And finally, I feel that it is not wise to introduce a third locking scheme in the I2C topology. It's bad enough with two. Why not make the ATR locking exactly match the mux-locked scheme to reduce the number of cases one needs to consider? But maybe that's just me? Cheers, Peter
Re: [PATCH v4 2/9] dt-bindings: i2c: add bindings for i2c analog and digital filter
On 2019-09-02 16:15, eugen.hris...@microchip.com wrote: > > > On 02.09.2019 13:49, Peter Rosin wrote: > >> On 2019-09-02 12:12, eugen.hris...@microchip.com wrote: >>> From: Eugen Hristev >>> >>> Some i2c controllers have a built-in digital or analog filter. >>> This is specifically required depending on the hardware PCB/board. >>> Some controllers also allow specifying the maximum width of the >>> spikes that can be filtered. The width length can be specified in >>> nanoseconds. >>> >>> Signed-off-by: Eugen Hristev >>> --- >>> Documentation/devicetree/bindings/i2c/i2c.txt | 11 +++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt >>> b/Documentation/devicetree/bindings/i2c/i2c.txt >>> index 44efafd..8dbff67 100644 >>> --- a/Documentation/devicetree/bindings/i2c/i2c.txt >>> +++ b/Documentation/devicetree/bindings/i2c/i2c.txt >>> @@ -55,6 +55,17 @@ wants to support one of the below features, it should >>> adapt the bindings below. >>> Number of nanoseconds the SDA signal takes to fall; t(f) in the I2C >>> specification. >>> >>> +- i2c-analog-filter >>> + Enable analog filter for i2c lines. >>> + >>> +- i2c-digital-filter >>> + Enable digital filter for i2c lines. >>> + >>> +- i2c-filter-width-ns >>> + Width of spikes which can be filtered by either digital or analog >>> + filters (i2c-analog-filtr or i2c-digital-filtr). This width is specified >> >> filtr -> filter (two instances) >> >> What if you want/need to have different bandwidth for the digital and analog >> filters? After all, this is a generic binding... > > Hi Peter, > > For our needs, this is enough: the purpose of the filters is to avoid > noise on the lines, the noise is as big as it is for the digital and for > the analog filters, since we use an absolute measurement for them. So I > do not know how useful it would be to make a difference. I think my gripe is that the description also seems non-generic. Analog filters never (ok, usually, but I have a hard time seeing how a simple analog filter can) work in terms of some "width of spikes". That phrasing seems like something inherent to trivial digital filters. For analog filters, specifying the bandwidth or cut-off frequency seems much more appropriate. And bandwidth would work equally well for digital filters, methinks. I also think it should be mentioned explicitly that this binding is for LP filters. I don't think anything else would be useful, but better safe than sorry... Hmm, would it be good or bad to specify the bandwidth relative to the current maximum bus speed? Cheers, Peter > Wolfram, what do you think ? > > Eugen > > >> >> Cheers, >> Peter >> >>> + in nanoseconds. >>> + >>> - interrupts >>> interrupts used by the device. >>> >>> >> >> ___ >> linux-arm-kernel mailing list >> linux-arm-ker...@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> >>
Re: [PATCH v4 2/9] dt-bindings: i2c: add bindings for i2c analog and digital filter
On 2019-09-02 12:12, eugen.hris...@microchip.com wrote: > From: Eugen Hristev > > Some i2c controllers have a built-in digital or analog filter. > This is specifically required depending on the hardware PCB/board. > Some controllers also allow specifying the maximum width of the > spikes that can be filtered. The width length can be specified in nanoseconds. > > Signed-off-by: Eugen Hristev > --- > Documentation/devicetree/bindings/i2c/i2c.txt | 11 +++ > 1 file changed, 11 insertions(+) > > diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt > b/Documentation/devicetree/bindings/i2c/i2c.txt > index 44efafd..8dbff67 100644 > --- a/Documentation/devicetree/bindings/i2c/i2c.txt > +++ b/Documentation/devicetree/bindings/i2c/i2c.txt > @@ -55,6 +55,17 @@ wants to support one of the below features, it should > adapt the bindings below. > Number of nanoseconds the SDA signal takes to fall; t(f) in the I2C > specification. > > +- i2c-analog-filter > + Enable analog filter for i2c lines. > + > +- i2c-digital-filter > + Enable digital filter for i2c lines. > + > +- i2c-filter-width-ns > + Width of spikes which can be filtered by either digital or analog > + filters (i2c-analog-filtr or i2c-digital-filtr). This width is specified filtr -> filter (two instances) What if you want/need to have different bandwidth for the digital and analog filters? After all, this is a generic binding... Cheers, Peter > + in nanoseconds. > + > - interrupts > interrupts used by the device. > >
Re: [PATCH v4 1/9] dt-bindings: i2c: at91: add new compatible
On 2019-09-02 12:11, eugen.hris...@microchip.com wrote: > From: Eugen Hristev > > Add compatible for new Microchip SoC, sam9x60 > > Reviewed-by: Rob Herring > Signed-off-by: Eugen Hristev > --- > Documentation/devicetree/bindings/i2c/i2c-at91.txt | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-at91.txt > b/Documentation/devicetree/bindings/i2c/i2c-at91.txt > index b7cec17..2210f43 100644 > --- a/Documentation/devicetree/bindings/i2c/i2c-at91.txt > +++ b/Documentation/devicetree/bindings/i2c/i2c-at91.txt > @@ -3,7 +3,8 @@ I2C for Atmel platforms > Required properties : > - compatible : Must be "atmel,at91rm9200-i2c", "atmel,at91sam9261-i2c", > "atmel,at91sam9260-i2c", "atmel,at91sam9g20-i2c", > "atmel,at91sam9g10-i2c", > - "atmel,at91sam9x5-i2c", "atmel,sama5d4-i2c" or "atmel,sama5d2-i2c" > + "atmel,at91sam9x5-i2c", "atmel,sama5d4-i2c", "atmel,sama5d2-i2c" or > + "microchip,sam9x60-i2c" IIUC, this list should ideally be reformatted with one compatible per line. Side note; unfortunate naming with SAM9x60, when there is a preexisting 9260 fitting the "wildcard" (AFAICT, it's not a wildcard in this case, but it sure looks like one). Cheers, Peter > - reg: physical base address of the controller and length of memory mapped > region. > - interrupts: interrupt number to the cpu. >
[PATCH 0/2] Add possibility to specify the number of displayed logos
Hi! The first patch fixes the fact that there are two items numbered "4" in the list of fbcon options. This bug is a teenager... The second patch extends that list with a new option that allows the user to display any number of logos (that fits on the screen). I need it to limit the display to only one logo instead of one for each CPU core. Cheers, Peter Peter Rosin (2): fbdev: fix numbering of fbcon options fbdev: fbmem: allow overriding the number of bootup logos Documentation/fb/fbcon.rst | 13 + drivers/video/fbdev/core/fbcon.c | 7 +++ drivers/video/fbdev/core/fbmem.c | 5 - include/linux/fb.h | 1 + 4 files changed, 21 insertions(+), 5 deletions(-) -- 2.11.0
[PATCH 1/2] fbdev: fix numbering of fbcon options
Three shall be the number thou shalt count, and the number of the
counting shall be three. Four shalt thou not count...
One! Two! Five!
Fixes: efb985f6b265 ("[PATCH] fbcon: Console Rotation - Add framebuffer console
documentation")
Signed-off-by: Peter Rosin
---
Documentation/fb/fbcon.rst | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/fb/fbcon.rst b/Documentation/fb/fbcon.rst
index ebca41785abe..65ba40255137 100644
--- a/Documentation/fb/fbcon.rst
+++ b/Documentation/fb/fbcon.rst
@@ -127,7 +127,7 @@ C. Boot options
is typically located on the same video card. Thus, the consoles that
are controlled by the VGA console will be garbled.
-4. fbcon=rotate:
+5. fbcon=rotate:
This option changes the orientation angle of the console display. The
value 'n' accepts the following:
@@ -152,21 +152,21 @@ C. Boot options
Actually, the underlying fb driver is totally ignorant of console
rotation.
-5. fbcon=margin:
+6. fbcon=margin:
This option specifies the color of the margins. The margins are the
leftover area at the right and the bottom of the screen that are not
used by text. By default, this area will be black. The 'color' value
is an integer number that depends on the framebuffer driver being used.
-6. fbcon=nodefer
+7. fbcon=nodefer
If the kernel is compiled with deferred fbcon takeover support, normally
the framebuffer contents, left in place by the firmware/bootloader, will
be preserved until there actually is some text is output to the console.
This option causes fbcon to bind immediately to the fbdev device.
-7. fbcon=logo-pos:
+8. fbcon=logo-pos:
The only possible 'location' is 'center' (without quotes), and when
given, the bootup logo is moved from the default top-left corner
--
2.11.0
[PATCH 2/2] fbdev: fbmem: allow overriding the number of bootup logos
Probably most useful if you only want one logo regardless of how many
CPU cores you have.
Signed-off-by: Peter Rosin
---
Documentation/fb/fbcon.rst | 5 +
drivers/video/fbdev/core/fbcon.c | 7 +++
drivers/video/fbdev/core/fbmem.c | 5 -
include/linux/fb.h | 1 +
4 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/Documentation/fb/fbcon.rst b/Documentation/fb/fbcon.rst
index 65ba40255137..9f0b399d8d4e 100644
--- a/Documentation/fb/fbcon.rst
+++ b/Documentation/fb/fbcon.rst
@@ -174,6 +174,11 @@ C. Boot options
displayed due to multiple CPUs, the collected line of logos is moved
as a whole.
+9. fbcon=logo-count:
+
+ The value 'n' overrides the number of bootup logos. Zero gives the
+ default, which is the number of online cpus.
+
C. Attaching, Detaching and Unloading
Before going on to how to attach, detach and unload the framebuffer console, an
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index c9235a2f42f8..be4bc5540aad 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -536,6 +536,13 @@ static int __init fb_console_setup(char *this_opt)
fb_center_logo = true;
continue;
}
+
+ if (!strncmp(options, "logo-count:", 11)) {
+ options += 11;
+ if (*options)
+ fb_logo_count = simple_strtoul(options,
, 0);
+ continue;
+ }
}
return 1;
}
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 64dd732021d8..4c57d522b72e 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -56,6 +56,9 @@ EXPORT_SYMBOL(num_registered_fb);
bool fb_center_logo __read_mostly;
EXPORT_SYMBOL(fb_center_logo);
+unsigned int fb_logo_count __read_mostly;
+EXPORT_SYMBOL(fb_logo_count);
+
static struct fb_info *get_fb_info(unsigned int idx)
{
struct fb_info *fb_info;
@@ -689,7 +692,7 @@ int fb_show_logo(struct fb_info *info, int rotate)
int y;
y = fb_show_logo_line(info, rotate, fb_logo.logo, 0,
- num_online_cpus());
+ fb_logo_count ?: num_online_cpus());
y = fb_show_extra_logos(info, y, rotate);
return y;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 303771264644..5f2b05406262 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -630,6 +630,7 @@ extern int fb_new_modelist(struct fb_info *info);
extern struct fb_info *registered_fb[FB_MAX];
extern int num_registered_fb;
extern bool fb_center_logo;
+extern unsigned int fb_logo_count;
extern struct class *fb_class;
#define for_each_registered_fb(i) \
--
2.11.0
Re: [PATCH v2 3/9] dt-bindings: i2c: at91: add binding for enable-dig-filtr
On 2019-06-25 10:04, eugen.hris...@microchip.com wrote:
> From: Eugen Hristev
>
> Add binding specificatoin for digital filter inside the i2c controller
>
> Signed-off-by: Eugen Hristev
> ---
> Documentation/devicetree/bindings/i2c/i2c-at91.txt | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-at91.txt
> b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
> index 2210f43..8268595 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-at91.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
> @@ -20,6 +20,9 @@ Optional properties:
>capable I2C controllers.
> - i2c-sda-hold-time-ns: TWD hold time, only available for "atmel,sama5d4-i2c"
>and "atmel,sama5d2-i2c".
> +- enable-dig-filt: Enable the built-in digital filter on the i2c lines,
> + specifically required depending on the hardware PCB/board and if the
> + version of the controller includes it.
> - Child nodes conforming to i2c bus binding
>
> Examples :
> @@ -56,6 +59,7 @@ i2c0: i2c@f8034600 {
> clocks = <>;
> atmel,fifo-size = <16>;
> i2c-sda-hold-time-ns = <336>;
> + enable-dig-filt;
This spelling does not match the spelling in the subject. But please see
my comment on patch 6/9...
Cheers,
Peter
>
> wm8731: wm8731@1a {
> compatible = "wm8731";
>
Re: [PATCH v2 6/9] dt-bindings: i2c: at91: add binding for enable-ana-filt
On 2019-06-25 10:05, eugen.hris...@microchip.com wrote:
> From: Eugen Hristev
>
> Add binding specification for analogic filter inside the i2c controller
s/analogic/the analog/
> Signed-off-by: Eugen Hristev
> ---
> Documentation/devicetree/bindings/i2c/i2c-at91.txt | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-at91.txt
> b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
> index 8268595..20d334c 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-at91.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
> @@ -23,6 +23,9 @@ Optional properties:
> - enable-dig-filt: Enable the built-in digital filter on the i2c lines,
>specifically required depending on the hardware PCB/board and if the
>version of the controller includes it.
> +- enable-ana-filt: Enable the built-in analogic filter on the i2c lines,
> + specifically required depending on the hardware PCB/board and if the
> + version of the controller includes it.
> - Child nodes conforming to i2c bus binding
>
> Examples :
> @@ -60,6 +63,7 @@ i2c0: i2c@f8034600 {
> atmel,fifo-size = <16>;
> i2c-sda-hold-time-ns = <336>;
> enable-dig-filt;
> + enable-ana-filt;
Perhaps
microchip,digital-filter;
microchip,analog-filter;
?
Cheers,
Peter
>
> wm8731: wm8731@1a {
> compatible = "wm8731";
>
Re: [PATCH v2 9/9] ARM: dts: at91: sama5d4_xplained: add analogic filter for i2c
On 2019-06-25 10:05, eugen.hris...@microchip.com wrote:
> From: Eugen Hristev
>
> Add property for digital filter for i2c0 node sama5d4_xplained
This does not match the below hunk.
Cheers,
Peter
> Signed-off-by: Eugen Hristev
> ---
> arch/arm/boot/dts/at91-sama5d4_xplained.dts | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/boot/dts/at91-sama5d4_xplained.dts
> b/arch/arm/boot/dts/at91-sama5d4_xplained.dts
> index fdfc37d..06068dc 100644
> --- a/arch/arm/boot/dts/at91-sama5d4_xplained.dts
> +++ b/arch/arm/boot/dts/at91-sama5d4_xplained.dts
> @@ -49,6 +49,7 @@
> };
>
> i2c0: i2c@f8014000 {
> + enable-ana-filt;
> status = "okay";
> };
>
>
Re: [PATCH v2 13/28] drivers: Introduce class_find_device_by_of_node() helper
On 2019-06-24 10:34, Suzuki K Poulose wrote: > Hi Peter, > > On 22/06/2019 06:25, Peter Rosin wrote: >> On 2019-06-14 19:54, Suzuki K Poulose wrote: >>> Add a wrapper to class_find_device() to search for a device >>> by the of_node pointer, reusing the generic match function. >>> Also convert the existing users to make use of the new helper. >>> >>> Cc: Alan Tull >>> Cc: Moritz Fischer >>> Cc: linux-f...@vger.kernel.org >>> Cc: Peter Rosin >>> Cc: Mark Brown >>> Cc: Florian Fainelli >>> Cc: Heiner Kallweit >>> Cc: "David S. Miller" >>> Cc: Andrew Lunn >>> Cc: Liam Girdwood >>> Cc: Greg Kroah-Hartman >>> Cc: "Rafael J. Wysocki" >>> Cc: Jiri Slaby >>> Acked-by: Mark Brown >>> Reviewed-by: Andrew Lunn >>> Reviewed-by: Peter Rosin >> >> Whoooa! I reviewed only the drivers/mux/core.c changes when this was done >> in a series of much smaller patches. This tag makes it seem as if I have >> reviewed the whole thing, which I had not done when you added this tag out >> of the blue. > > Apologies for the surprise. The patch was simply squashed with the change that > introduced the "helper" to better aid the reviewers, based on suggestions on > the > list. I kept your tags, only because there were no changes, but some > additional > context on the core driver. You could e.g. have written: ... [For the drivers/mux/core.c part] Reviewed-by: Peter Rosin ... >> >> Now, this stuff is trivial and by now I have looked at the other files >> and it all seems simple enough. So, you can keep the tag, but it is NOT >> ok to handle tags like you have done here. > > Sure, I will keep that in mind. Great! Cheers, Peter
Re: [PATCH v2 13/28] drivers: Introduce class_find_device_by_of_node() helper
On 2019-06-14 19:54, Suzuki K Poulose wrote: > Add a wrapper to class_find_device() to search for a device > by the of_node pointer, reusing the generic match function. > Also convert the existing users to make use of the new helper. > > Cc: Alan Tull > Cc: Moritz Fischer > Cc: linux-f...@vger.kernel.org > Cc: Peter Rosin > Cc: Mark Brown > Cc: Florian Fainelli > Cc: Heiner Kallweit > Cc: "David S. Miller" > Cc: Andrew Lunn > Cc: Liam Girdwood > Cc: Greg Kroah-Hartman > Cc: "Rafael J. Wysocki" > Cc: Jiri Slaby > Acked-by: Mark Brown > Reviewed-by: Andrew Lunn > Reviewed-by: Peter Rosin Whoooa! I reviewed only the drivers/mux/core.c changes when this was done in a series of much smaller patches. This tag makes it seem as if I have reviewed the whole thing, which I had not done when you added this tag out of the blue. Now, this stuff is trivial and by now I have looked at the other files and it all seems simple enough. So, you can keep the tag, but it is NOT ok to handle tags like you have done here. Cheers, Peter > Signed-off-by: Suzuki K Poulose > --- > drivers/fpga/fpga-bridge.c | 8 +--- > drivers/fpga/fpga-mgr.c | 8 +--- > drivers/mux/core.c | 7 +-- > drivers/net/phy/mdio_bus.c | 9 + > drivers/regulator/of_regulator.c | 7 +-- > drivers/spi/spi.c| 11 ++- > include/linux/device.h | 12 > 7 files changed, 19 insertions(+), 43 deletions(-)
Re: [PATCH V2] i2c: tegra: disable irq in tegra_i2c_xfer_msg
On 2019-06-18 10:42, Bitan Biswas wrote:
> tegra_i2c_xfer_msg initiates the I2C transfer in DMA
> or PIO mode. It involves steps that need FIFO register
> access, DMA API calls like dma_sync_single_for_device, etc.
> Tegra I2C ISR has calls to tegra_i2c_empty_rx_fifo in PIO mode
> and in DMA/PIO mode writes different I2C registers including
> I2C interrupt status. ISR cannot start processing
> before the preparation step at tegra_i2c_xfer_msg is complete.
> Hence, a synchronization between ISR and tegra_i2c_xfer_msg
> is in place today using spinlock.
>
> Spinlock busy waits and can add avoidable delays.
>
> In this patch needed synchronization is achieved by disabling
> I2C interrupt during preparation step and enabling interrupt
> once preparation is over and spinlock is no longer needed.
>
> Signed-off-by: Bitan Biswas
> ---
> drivers/i2c/busses/i2c-tegra.c | 17 -
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 6fb545e..ccc7fae 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -240,7 +240,6 @@ struct tegra_i2c_hw_feature {
> * @bus_clk_rate: current I2C bus clock rate
> * @clk_divisor_non_hs_mode: clock divider for non-high-speed modes
> * @is_multimaster_mode: track if I2C controller is in multi-master mode
> - * @xfer_lock: lock to serialize transfer submission and processing
> * @tx_dma_chan: DMA transmit channel
> * @rx_dma_chan: DMA receive channel
> * @dma_phys: handle to DMA resources
> @@ -270,8 +269,6 @@ struct tegra_i2c_dev {
> u32 bus_clk_rate;
> u16 clk_divisor_non_hs_mode;
> bool is_multimaster_mode;
> - /* xfer_lock: lock to serialize transfer submission and processing */
> - spinlock_t xfer_lock;
> struct dma_chan *tx_dma_chan;
> struct dma_chan *rx_dma_chan;
> dma_addr_t dma_phys;
> @@ -835,7 +832,6 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>
> status = i2c_readl(i2c_dev, I2C_INT_STATUS);
>
> - spin_lock(_dev->xfer_lock);
> if (status == 0) {
> dev_warn(i2c_dev->dev, "irq status 0 %08x %08x %08x\n",
>i2c_readl(i2c_dev, I2C_PACKET_TRANSFER_STATUS),
> @@ -935,7 +931,6 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>
> complete(_dev->msg_complete);
> done:
> - spin_unlock(_dev->xfer_lock);
> return IRQ_HANDLED;
> }
>
> @@ -1054,7 +1049,6 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev
> *i2c_dev,
> u32 packet_header;
> u32 int_mask;
> unsigned long time_left;
> - unsigned long flags;
> size_t xfer_size;
> u32 *buffer = NULL;
> int err = 0;
> @@ -1085,7 +1079,10 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev
> *i2c_dev,
>*/
> xfer_time += DIV_ROUND_CLOSEST(((xfer_size * 9) + 2) * MSEC_PER_SEC,
> i2c_dev->bus_clk_rate);
> - spin_lock_irqsave(_dev->xfer_lock, flags);
> + if (!i2c_dev->irq_disabled) {
> + disable_irq_nosync(i2c_dev->irq);
Why do you use ..._nosync in this context? Do you not care about exactly when
the irq is actually disabled? Maybe you must use _nosync, I don't know, but I
get the feeling that you just copied the code from the ISR. Anyway, the
question is if this is still adequate synchronization? Everything might very
well be safe and in order, but this is not clear to me from neither the code,
the (nonexisting) comments nor the patch description.
I'm no IRQ expert, I'm just asking questions...
Cheers,
Peter
> + i2c_dev->irq_disabled = true;
> + }
>
> int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
> tegra_i2c_unmask_irq(i2c_dev, int_mask);
> @@ -1180,7 +1177,10 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev
> *i2c_dev,
> i2c_readl(i2c_dev, I2C_INT_MASK));
>
> unlock:
> - spin_unlock_irqrestore(_dev->xfer_lock, flags);
> + if (i2c_dev->irq_disabled) {
> + i2c_dev->irq_disabled = false;
> + enable_irq(i2c_dev->irq);
> + }
>
> if (dma) {
> if (err)
> @@ -1576,7 +1576,6 @@ static int tegra_i2c_probe(struct platform_device *pdev)
> I2C_PACKET_HEADER_SIZE;
> init_completion(_dev->msg_complete);
> init_completion(_dev->dma_complete);
> - spin_lock_init(_dev->xfer_lock);
>
> if (!i2c_dev->hw->has_single_clk_source) {
> fast_clk = devm_clk_get(>dev, "fast-clk");
>
Re: [PATCH v4 4/5] phy: ti: Add a new SERDES driver for TI's AM654x SoC
Hi,
Sorry for the late reply, $-work interfered...
On 2019-06-13 12:21, Kishon Vijay Abraham I wrote:
> Hi,
>
> On 13/06/19 1:22 PM, Peter Rosin wrote:
>> Hi,
>>
>> On 2019-06-13 06:57, Kishon Vijay Abraham I wrote:
>>> Hi Peter,
>>>
>>> On 13/06/19 4:20 AM, Peter Rosin wrote:
>>>> Hi!
>>>>
>>>> [I know this has already been merged upstream, but I only just
>>>> now noticed the code and went to the archives to find the
>>>> originating mail. I hope I managed to set in-reply-to correctly...]
>>>>
>>>> The mux handling is problematic and does not follow the rules.
>>>> It needs to be fixed, or you may face deadlocks. See below.
>>>>
>>>> On 2019-04-05 11:08, Kishon Vijay Abraham I wrote:
>>>>> Add a new SERDES driver for TI's AM654x SoC which configures
>>>>> the SERDES only for PCIe. Support fo USB3 will be added later.
>>>>>
>>>>> SERDES in am654x has three input clocks (left input, externel reference
>>>>> clock and right input) and two output clocks (left output and right
>>>>> output) in addition to a PLL mux clock which the SERDES uses for Clock
>>>>> Multiplier Unit (CMU refclock).
>>>>>
>>>>> The PLL mux clock can select from one of the three input clocks.
>>>>> The right output can select between left input and external reference
>>>>> clock while the left output can select between the right input and
>>>>> external reference clock.
>>>>>
>>>>> The driver has support to select PLL mux and left/right output mux as
>>>>> specified in device tree.
>>>>>
>>>>> [rog...@ti.com: Fix boot lockup caused by accessing a structure member
>>>>> (hw->init) allocated in stack of probe() and accessed in get_parent]
>>>>> [rog...@ti.com: Fix "Failed to find the parent" warnings]
>>>>> Signed-off-by: Roger Quadros
>>>>> Signed-off-by: Kishon Vijay Abraham I
>>
>> *snip*
>>
>>>>> +static void serdes_am654_release(struct phy *x)
>>>>> +{
>>>>> + struct serdes_am654 *phy = phy_get_drvdata(x);
>>>>> +
>>>>> + phy->type = PHY_NONE;
>>>>> + phy->busy = false;
>>>>> + mux_control_deselect(phy->control);
>>>>
>>>> Here you unconditionally deselect the mux, and that seems
>>>> dangerous. Are you *sure* that ->release may not be called
>>>> without a successful xlate call?
>>>
>>> Yeah, without a successful xlate(), the consumer will never get a reference
>>> to
>>> the PHY and the ->release() is invoked only from phy_put() which needs a
>>> reference to the PHY.
>>
>> Yes, I thought it might be ok, but good that you can confirm it.
>>
>>>> I'm not 100% sure of that, but I have not looked at the phy
>>>> code before today, so it may very well be the case that this
>>>> is safe...
>>>>
>>>>> +}
>>>>> +
>>>>> +struct phy *serdes_am654_xlate(struct device *dev, struct of_phandle_args
>>>>> + *args)
>>>>> +{
>>>>> + struct serdes_am654 *am654_phy;
>>>>> + struct phy *phy;
>>>>> + int ret;
>>>>> +
>>>>> + phy = of_phy_simple_xlate(dev, args);
>>>>> + if (IS_ERR(phy))
>>>>> + return phy;
>>>>> +
>>>>> + am654_phy = phy_get_drvdata(phy);
>>>>> + if (am654_phy->busy)
>>>>> + return ERR_PTR(-EBUSY);
>>>>> +
>>>>> + ret = mux_control_select(am654_phy->control, args->args[1]);
>>>>> + if (ret) {
>>>>> + dev_err(dev, "Failed to select SERDES Lane Function\n");
>>>>> + return ERR_PTR(ret);
>>>>> + }
>>>>
>>>> *However*
>>>>
>>>> Here you select the mux as the last action, good, but, a mux must
>>>> be handled with that same care as a locking primitive, i.e.
>>>> successful selects must be perfectly balanced with deselects. I
>>>> see no guarantee of that here, since there are other failures
>>>> possible after the xlate call. So, being last in the function
>>>> does not really help. If I read the code correctly, the
>>>> phy core m
Re: [PATCH] i2c: mux-gpio: Use "mux" con_id to find channel GPIOs
On 2019-06-16 01:24, Serge Semin wrote:
> On Sat, Jun 15, 2019 at 02:43:09PM +0300, Andy Shevchenko wrote:
>> On Sat, Jun 15, 2019 at 12:51 AM Serge Semin wrote:
>>>
>>> Recent patch - ("i2c: mux/i801: Switch to use descriptor passing")
>>> altered the i2c-mux-gpio driver to use the GPIO-descriptor
>>> based interface to find and request the GPIOs then being utilized
>>> to select and deselect the channels of GPIO-driven i2c-muxes. Even
>>> though the proposed modification was correct for the platform_data-based
>>> systems, it was invalid for the OF-based ones and caused the kernel
>>> to crash at the driver probe procedure. There were two problems with
>>> that modification. First of all the gpiod_count() and gpiod_get_index()
>>> were called with NULL con_id.
>>
>> I always thought that this means "count me all GPIO's for this device
>> despite their names" and "get me GPIO by index despite it's name".
>> What's went wrong?
>>
>
> No. It's wrong as far as I can see for kernels 4.4, 4.9 and the modern
> 5.2.0-rcX. dt_gpio_count()/of_find_gpio()will always try to count/request
> either "-gpio(s)" or "gpio(s)" GPIOs in the device of-node. While
> platform_gpio_count()/gpiod_find() will take into account GPIOs with matching
> 's even if it is NULL.
Right, this is my reading to. For the OF case, gpiod_count calls dt_gpio_count
which has no way to find gpios unless they are explicitly named. And NULL
simply means unnamed (which is not the case here). The function simply does
not do any trawling for gpios it has not been told about.
Linus, care to squash this incremental into your patch and resend with proper
credit added?
Cheers,
Peter
Re: [PATCH v2 2/3] i2c-mux-gpio: Unpin the platform-specific GPIOs request code
On 2019-06-14 18:31, Serge Semin wrote: > Hello Peter, > > On Sun, Jun 09, 2019 at 09:34:54PM +0000, Peter Rosin wrote: >> On 2019-04-26 01:20, Serge Semin wrote: >>> The GPIOs request loop can be safely moved to a separate function. >>> First of all it shall improve the code readability. Secondly the >>> initialization loop at this point is used for both of- and >>> platform_data-based initialization paths, but it will be changed in >>> the next patch, so by isolating the code we'll simplify the future >>> work. >> >> This patch is just preparatory for patch 3/3, as I see it. And since >> I'm not really fond of the end result after patch 3/3, I'm going to >> sum up my issues here, instead of trying do it piecemeal in the two >> patches. >> >> Linus and Jean, for your convenience, link to this patch series [1]. >> >> While I agree with the goal (to use the more flexible gpiod functions >> to get at the gpio descriptors), the cost is too high when the init >> code for platform and OF is basically completely separated. I much >> prefer the approach taken by Linus [2], which instead converts the >> platform interface and its single user to use gpio descriptors instead >> of the legacy gpio interface. The i2c-mux-gpio code then has the >> potential to take a unified approach to the given gpio descriptors, >> wherever they are originating from, which is much nicer than the >> code-fork in this series. >> >> I also think it is pretty pointless to first split the code into >> platform and OF paths, just so that the next patch (from Linus) can >> unify the two paths again. I'd like to skip the intermediate step. >> >> So, I'm hoping for the following to happen. >> 1. Sergey sends a revised patch for patch 1/3. >> 2. I put the patch on the for-next branch. >> 3. Linus rebases his patch on top of that (while thinking about >>the questions raised by Sergey). >> 4. Sergey tests the result, I and Jean review it, then possibly >>go back to 3. >> 5. I put the patch on the for-next branch. >> >> Is that ok? Or is someone insisting that we take a detour? >> > > The series was intended to add the gpiod support to the i2c-mux-gpio driver > (see the cover letter of the series). So the last patch is the most valuable > one. Without it the whole series is nothing but a small readability > improvement. > So it is pointless to merge the first patch only. Agreed on all points, except perhaps for the "refuse" part below and that the readability improvement of patch 1/3 is perhaps not all that pointless. > Anyway since you refuse to add the last patch and the first patch is actually > pointless without the rest of the series, and I would have to spend my time to > resubmit the v3 of the first patch anyway, it was much easier to test the > current version of the Linus' patch and make it working for OF-based > platforms. > Additionally the Linus' patch also reaches the main goal of this patchset. I'm very pleased that you do not feel totally put off, and are willing to help even if we end up storing your series in /dev/null. Kudos! > I don't know what would be the appropriate way to send the updated version of > the Linus' patch. So I just attached the v4 of it to this email. Shall I > better > send it in reply to the Linus' patch series? I get the impression that you have already done the work? In that case, how I would proceed would depend on how big the difference is. If it's just a few one-liners here and there, I think I would make a detailed review comment so that it is easy for Linus to incorporate the needed changes. If it's anything even remotely complex I would post an incremental patch. Of course, the former does not exclude the latter, but I do think an incremental patch is better than a repost. Thanks again! Cheers, Peter
Re: [PATCH v4 4/5] phy: ti: Add a new SERDES driver for TI's AM654x SoC
Hi!
[I know this has already been merged upstream, but I only just
now noticed the code and went to the archives to find the
originating mail. I hope I managed to set in-reply-to correctly...]
The mux handling is problematic and does not follow the rules.
It needs to be fixed, or you may face deadlocks. See below.
On 2019-04-05 11:08, Kishon Vijay Abraham I wrote:
> Add a new SERDES driver for TI's AM654x SoC which configures
> the SERDES only for PCIe. Support fo USB3 will be added later.
>
> SERDES in am654x has three input clocks (left input, externel reference
> clock and right input) and two output clocks (left output and right
> output) in addition to a PLL mux clock which the SERDES uses for Clock
> Multiplier Unit (CMU refclock).
>
> The PLL mux clock can select from one of the three input clocks.
> The right output can select between left input and external reference
> clock while the left output can select between the right input and
> external reference clock.
>
> The driver has support to select PLL mux and left/right output mux as
> specified in device tree.
>
> [rog...@ti.com: Fix boot lockup caused by accessing a structure member
> (hw->init) allocated in stack of probe() and accessed in get_parent]
> [rog...@ti.com: Fix "Failed to find the parent" warnings]
> Signed-off-by: Roger Quadros
> Signed-off-by: Kishon Vijay Abraham I
> ---
> drivers/phy/ti/Kconfig| 12 +
> drivers/phy/ti/Makefile | 1 +
> drivers/phy/ti/phy-am654-serdes.c | 624 ++
> 3 files changed, 637 insertions(+)
> create mode 100644 drivers/phy/ti/phy-am654-serdes.c
>
> diff --git a/drivers/phy/ti/Kconfig b/drivers/phy/ti/Kconfig
> index 7cdc35f8c862..6931c87235b9 100644
> --- a/drivers/phy/ti/Kconfig
> +++ b/drivers/phy/ti/Kconfig
> @@ -20,6 +20,18 @@ config PHY_DM816X_USB
> help
> Enable this for dm816x USB to work.
>
> +config PHY_AM654_SERDES
> + tristate "TI AM654 SERDES support"
> + depends on OF && ARCH_K3 || COMPILE_TEST
> + depends on COMMON_CLK
> + select GENERIC_PHY
> + select MULTIPLEXER
> + select REGMAP_MMIO
> + select MUX_MMIO
> + help
> + This option enables support for TI AM654 SerDes PHY used for
> + PCIe.
> +
> config OMAP_CONTROL_PHY
> tristate "OMAP CONTROL PHY Driver"
> depends on ARCH_OMAP2PLUS || COMPILE_TEST
> diff --git a/drivers/phy/ti/Makefile b/drivers/phy/ti/Makefile
> index 368de8e1548d..601bbd88f35e 100644
> --- a/drivers/phy/ti/Makefile
> +++ b/drivers/phy/ti/Makefile
> @@ -6,5 +6,6 @@ obj-$(CONFIG_OMAP_USB2) +=
> phy-omap-usb2.o
> obj-$(CONFIG_TI_PIPE3) += phy-ti-pipe3.o
> obj-$(CONFIG_PHY_TUSB1210) += phy-tusb1210.o
> obj-$(CONFIG_TWL4030_USB)+= phy-twl4030-usb.o
> +obj-$(CONFIG_PHY_AM654_SERDES) += phy-am654-serdes.o
> obj-$(CONFIG_PHY_TI_GMII_SEL)+= phy-gmii-sel.o
> obj-$(CONFIG_PHY_TI_KEYSTONE_SERDES) += phy-keystone-serdes.o
> diff --git a/drivers/phy/ti/phy-am654-serdes.c
> b/drivers/phy/ti/phy-am654-serdes.c
> new file mode 100644
> index ..4817c67a
> --- /dev/null
> +++ b/drivers/phy/ti/phy-am654-serdes.c
> @@ -0,0 +1,624 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * PCIe SERDES driver for AM654x SoC
> + *
> + * Copyright (C) 2018 - 2019 Texas Instruments Incorporated -
> http://www.ti.com/
> + * Author: Kishon Vijay Abraham I
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define CMU_R07C 0x7c
> +
> +#define COMLANE_R138 0xb38
> +#define VERSION 0x70
> +
> +#define COMLANE_R190 0xb90
> +
> +#define COMLANE_R194 0xb94
> +
> +#define SERDES_CTRL 0x1fd0
> +
> +#define WIZ_LANEXCTL_STS 0x1fe0
> +#define TX0_DISABLE_STATE0x4
> +#define TX0_SLEEP_STATE 0x5
> +#define TX0_SNOOZE_STATE 0x6
> +#define TX0_ENABLE_STATE 0x7
> +
> +#define RX0_DISABLE_STATE0x4
> +#define RX0_SLEEP_STATE 0x5
> +#define RX0_SNOOZE_STATE 0x6
> +#define RX0_ENABLE_STATE 0x7
> +
> +#define WIZ_PLL_CTRL 0x1ff4
> +#define PLL_DISABLE_STATE0x4
> +#define PLL_SLEEP_STATE 0x5
> +#define PLL_SNOOZE_STATE 0x6
> +#define PLL_ENABLE_STATE 0x7
> +
> +#define PLL_LOCK_TIME10 /* in microseconds */
> +#define SLEEP_TIME 100 /* in microseconds */
> +
> +#define LANE_USB30x0
> +#define LANE_PCIE0_LANE0 0x1
> +
> +#define LANE_PCIE1_LANE0 0x0
> +#define LANE_PCIE0_LANE1 0x1
> +
> +#define SERDES_NUM_CLOCKS3
> +
> +struct serdes_am654_clk_mux {
> + struct clk_hw hw;
> + struct regmap *regmap;
> + unsigned intreg;
> + int *table;
> + u32 mask;
> + u8 shift;
> +
Re: [PATCH v4 4/5] phy: ti: Add a new SERDES driver for TI's AM654x SoC
Hi,
On 2019-06-13 06:57, Kishon Vijay Abraham I wrote:
> Hi Peter,
>
> On 13/06/19 4:20 AM, Peter Rosin wrote:
>> Hi!
>>
>> [I know this has already been merged upstream, but I only just
>> now noticed the code and went to the archives to find the
>> originating mail. I hope I managed to set in-reply-to correctly...]
>>
>> The mux handling is problematic and does not follow the rules.
>> It needs to be fixed, or you may face deadlocks. See below.
>>
>> On 2019-04-05 11:08, Kishon Vijay Abraham I wrote:
>>> Add a new SERDES driver for TI's AM654x SoC which configures
>>> the SERDES only for PCIe. Support fo USB3 will be added later.
>>>
>>> SERDES in am654x has three input clocks (left input, externel reference
>>> clock and right input) and two output clocks (left output and right
>>> output) in addition to a PLL mux clock which the SERDES uses for Clock
>>> Multiplier Unit (CMU refclock).
>>>
>>> The PLL mux clock can select from one of the three input clocks.
>>> The right output can select between left input and external reference
>>> clock while the left output can select between the right input and
>>> external reference clock.
>>>
>>> The driver has support to select PLL mux and left/right output mux as
>>> specified in device tree.
>>>
>>> [rog...@ti.com: Fix boot lockup caused by accessing a structure member
>>> (hw->init) allocated in stack of probe() and accessed in get_parent]
>>> [rog...@ti.com: Fix "Failed to find the parent" warnings]
>>> Signed-off-by: Roger Quadros
>>> Signed-off-by: Kishon Vijay Abraham I
*snip*
>>> +static void serdes_am654_release(struct phy *x)
>>> +{
>>> + struct serdes_am654 *phy = phy_get_drvdata(x);
>>> +
>>> + phy->type = PHY_NONE;
>>> + phy->busy = false;
>>> + mux_control_deselect(phy->control);
>>
>> Here you unconditionally deselect the mux, and that seems
>> dangerous. Are you *sure* that ->release may not be called
>> without a successful xlate call?
>
> Yeah, without a successful xlate(), the consumer will never get a reference to
> the PHY and the ->release() is invoked only from phy_put() which needs a
> reference to the PHY.
Yes, I thought it might be ok, but good that you can confirm it.
>> I'm not 100% sure of that, but I have not looked at the phy
>> code before today, so it may very well be the case that this
>> is safe...
>>
>>> +}
>>> +
>>> +struct phy *serdes_am654_xlate(struct device *dev, struct of_phandle_args
>>> +*args)
>>> +{
>>> + struct serdes_am654 *am654_phy;
>>> + struct phy *phy;
>>> + int ret;
>>> +
>>> + phy = of_phy_simple_xlate(dev, args);
>>> + if (IS_ERR(phy))
>>> + return phy;
>>> +
>>> + am654_phy = phy_get_drvdata(phy);
>>> + if (am654_phy->busy)
>>> + return ERR_PTR(-EBUSY);
>>> +
>>> + ret = mux_control_select(am654_phy->control, args->args[1]);
>>> + if (ret) {
>>> + dev_err(dev, "Failed to select SERDES Lane Function\n");
>>> + return ERR_PTR(ret);
>>> + }
>>
>> *However*
>>
>> Here you select the mux as the last action, good, but, a mux must
>> be handled with that same care as a locking primitive, i.e.
>> successful selects must be perfectly balanced with deselects. I
>> see no guarantee of that here, since there are other failures
>> possible after the xlate call. So, being last in the function
>> does not really help. If I read the code correctly, the
>> phy core may fail if try_module_get fails in phy_get(). If that
>> ever happens, a successful call to mux_control_select is simply
>> forgotten, and the mux will be locked indefinitely.
>
> Good catch. While adding ->release() ops which is only invoked from phy_put,
> perhaps this was missed. Ideally it should be invoked from other places where
> there is a failure after phy_get.
>>
>> am654_phy->busy will also be set indefinitely, so you will get
>> -EBUSY and not a hard deadlock. At least here, but if the now
>> locked mux control happens to also control some other muxes
>> (probably unlikely, but if), then their consumers will potentially
>> deadlock hard. But that's just after a cursory reading, so I may
>> completely miss something...
>
> The ->busy here should prevent two consumers trying to cont
[RESEND PATCH 0/2] mux: a couple of patches for 5.3-rc1
Hi Greg, (For Greg, this is not a resend, since I apparently forgot to include him last time [1]. My mistake, sorry about that) A small addition to the mmio mux so that it can handle non-syscon regmaps. The bindings patch should probably have had Robs tag, but after a bit of back and forth I got the impression that it wasn't really needed, since it's basically just a file rename plus addition of a compatible [2]. The patches have been in -next for a week or so. But, if I misunderstood or if you have a tag to spare Rob, now is the time. :-) Cheers, Peter [1] https://lore.kernel.org/lkml/20190430195226.8965-1-p...@axentia.se/ [2] https://marc.info/?l=devicetree=155121843503590 "That would have saved me reviewing the whole thing again..." Pankaj Bansal (2): dt-bindings: add register based devices' mux controller DT bindings mux: mmio: add generic regmap bitfield-based multiplexer Documentation/devicetree/bindings/mux/mmio-mux.txt | 60 -- Documentation/devicetree/bindings/mux/reg-mux.txt | 129 + drivers/mux/Kconfig| 12 +- drivers/mux/mmio.c | 6 +- 4 files changed, 140 insertions(+), 67 deletions(-) delete mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt create mode 100644 Documentation/devicetree/bindings/mux/reg-mux.txt -- 2.11.0
[RESEND PATCH 2/2] mux: mmio: add generic regmap bitfield-based multiplexer
From: Pankaj Bansal
Generic register bitfield-based multiplexer that controls the multiplexer
producer defined under a parent node.
The driver corresponding to parent node provides register read/write
capabilities.
Signed-off-by: Pankaj Bansal
Signed-off-by: Peter Rosin
---
drivers/mux/Kconfig | 12 ++--
drivers/mux/mmio.c | 6 +-
2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
index 7659d6c5f718..e5c571fd232c 100644
--- a/drivers/mux/Kconfig
+++ b/drivers/mux/Kconfig
@@ -46,14 +46,14 @@ config MUX_GPIO
be called mux-gpio.
config MUX_MMIO
- tristate "MMIO register bitfield-controlled Multiplexer"
- depends on (OF && MFD_SYSCON) || COMPILE_TEST
+ tristate "MMIO/Regmap register bitfield-controlled Multiplexer"
+ depends on OF || COMPILE_TEST
help
- MMIO register bitfield-controlled Multiplexer controller.
+ MMIO/Regmap register bitfield-controlled Multiplexer controller.
- The driver builds multiplexer controllers for bitfields in a syscon
- register. For N bit wide bitfields, there will be 2^N possible
- multiplexer states.
+ The driver builds multiplexer controllers for bitfields in either
+ a syscon register or a driver regmap register. For N bit wide
+ bitfields, there will be 2^N possible multiplexer states.
To compile the driver as a module, choose M here: the module will
be called mux-mmio.
diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c
index 935ac44aa209..44a7a0e885b8 100644
--- a/drivers/mux/mmio.c
+++ b/drivers/mux/mmio.c
@@ -28,6 +28,7 @@ static const struct mux_control_ops mux_mmio_ops = {
static const struct of_device_id mux_mmio_dt_ids[] = {
{ .compatible = "mmio-mux", },
+ { .compatible = "reg-mux", },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, mux_mmio_dt_ids);
@@ -43,7 +44,10 @@ static int mux_mmio_probe(struct platform_device *pdev)
int ret;
int i;
- regmap = syscon_node_to_regmap(np->parent);
+ if (of_device_is_compatible(np, "mmio-mux"))
+ regmap = syscon_node_to_regmap(np->parent);
+ else
+ regmap = dev_get_regmap(dev->parent, NULL) ?: ERR_PTR(-ENODEV);
if (IS_ERR(regmap)) {
ret = PTR_ERR(regmap);
dev_err(dev, "failed to get regmap: %d\n", ret);
--
2.11.0
[RESEND PATCH 1/2] dt-bindings: add register based devices' mux controller DT bindings
From: Pankaj Bansal
This adds device tree binding documentation for generic register based
multiplexer controlled by a bitfields in a parent device's register range.
since MMIO mux is a special case of generic register based mux, the
MMIO mux bindings have been subsumed in these bindings.
Signed-off-by: Pankaj Bansal
Signed-off-by: Peter Rosin
---
Documentation/devicetree/bindings/mux/mmio-mux.txt | 60 --
Documentation/devicetree/bindings/mux/reg-mux.txt | 129 +
2 files changed, 129 insertions(+), 60 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
create mode 100644 Documentation/devicetree/bindings/mux/reg-mux.txt
diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt
b/Documentation/devicetree/bindings/mux/mmio-mux.txt
deleted file mode 100644
index a9bfb4d8b6ac..
--- a/Documentation/devicetree/bindings/mux/mmio-mux.txt
+++ /dev/null
@@ -1,60 +0,0 @@
-MMIO register bitfield-based multiplexer controller bindings
-
-Define register bitfields to be used to control multiplexers. The parent
-device tree node must be a syscon node to provide register access.
-
-Required properties:
-- compatible : "mmio-mux"
-- #mux-control-cells : <1>
-- mux-reg-masks : an array of register offset and pre-shifted bitfield mask
- pairs, each describing a single mux control.
-* Standard mux-controller bindings as decribed in mux-controller.txt
-
-Optional properties:
-- idle-states : if present, the state the muxes will have when idle. The
- special state MUX_IDLE_AS_IS is the default.
-
-The multiplexer state of each multiplexer is defined as the value of the
-bitfield described by the corresponding register offset and bitfield mask pair
-in the mux-reg-masks array, accessed through the parent syscon.
-
-Example:
-
- syscon {
- compatible = "syscon";
-
- mux: mux-controller {
- compatible = "mmio-mux";
- #mux-control-cells = <1>;
-
- mux-reg-masks = <0x3 0x30>, /* 0: reg 0x3, bits 5:4 */
- <0x3 0x40>, /* 1: reg 0x3, bit 6 */
- idle-states = , <0>;
- };
- };
-
- video-mux {
- compatible = "video-mux";
- mux-controls = < 0>;
-
- ports {
- /* inputs 0..3 */
- port@0 {
- reg = <0>;
- };
- port@1 {
- reg = <1>;
- };
- port@2 {
- reg = <2>;
- };
- port@3 {
- reg = <3>;
- };
-
- /* output */
- port@4 {
- reg = <4>;
- };
- };
- };
diff --git a/Documentation/devicetree/bindings/mux/reg-mux.txt
b/Documentation/devicetree/bindings/mux/reg-mux.txt
new file mode 100644
index ..4afd7ba73d60
--- /dev/null
+++ b/Documentation/devicetree/bindings/mux/reg-mux.txt
@@ -0,0 +1,129 @@
+Generic register bitfield-based multiplexer controller bindings
+
+Define register bitfields to be used to control multiplexers. The parent
+device tree node must be a device node to provide register r/w access.
+
+Required properties:
+- compatible : should be one of
+ "reg-mux" : if parent device of mux controller is not syscon device
+ "mmio-mux" : if parent device of mux controller is syscon device
+- #mux-control-cells : <1>
+- mux-reg-masks : an array of register offset and pre-shifted bitfield mask
+ pairs, each describing a single mux control.
+* Standard mux-controller bindings as decribed in mux-controller.txt
+
+Optional properties:
+- idle-states : if present, the state the muxes will have when idle. The
+ special state MUX_IDLE_AS_IS is the default.
+
+The multiplexer state of each multiplexer is defined as the value of the
+bitfield described by the corresponding register offset and bitfield mask
+pair in the mux-reg-masks array.
+
+Example 1:
+The parent device of mux controller is not a syscon device.
+
+ {
+ fpga@66 { // fpga connected to i2c
+ compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c",
+"simple-mfd";
+ reg = <0x66>;
+
+ mux: mux-controller {
+ compatible = "reg-mux";
+ #mux-control-cells = <1>;
+ mux-reg-masks = <0x54 0xf8>, /* 0: reg 0x54, bits 7:3 */
+
Re: [PATCH v1 3/3] dt-bindings: iio: afe: Add hwmon example
On 2019-06-11 11:56, Mylène Josserand wrote:
> With the support of CHAN_INFO_PROCESSED in voltage-divider,
> it is possible to read the processed values directly from iio's
> sysfs entries or by using iio-hwmon. Add an example for this last
> use case.
As I wrote in response to the cover letter, I think iio-hwmon
could "consume" voltage dividers just fine before adding the
processed channel, and while more examples might be good,
there is really no specific relation between iio-hwmon and
voltage dividers. Adding iio-hwmon examples to each and every
iio binding seems pointless. So, I see little reason to add
such examples here.
But if everyone else wants it, don't let me stand in the way...
Cheers,
Peter
> Signed-off-by: Mylène Josserand
> ---
> .../bindings/iio/afe/voltage-divider.txt | 24
> ++
> 1 file changed, 24 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
> b/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
> index b452a8406107..f7e1c7cb2744 100644
> --- a/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
> +++ b/Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
> @@ -51,3 +51,27 @@ sysv {
> spi-max-frequency = <100>;
> };
> };
> +
> +It is also possible to retrieve the processed values using hwmon node:
> +
> +div0: div0 {
> + compatible = "voltage-divider";
> + io-channels = < 0>; /* Channel 0 of the ADC */
> + output-ohms = <47>; /* R2 */
> + full-ohms = <73>; /* R1 (26) + R2 (47) */
> + #io-channel-cells = <1>;
> +};
> +
> +div1: div1 {
> + compatible = "voltage-divider";
> + io-channels = < 1>; /* Channel 1 of the ADC */
> + output-ohms = <47>; /* R2 */
> + full-ohms = <115>; /* R1 (68) + R2 (47) */
> + #io-channel-cells = <1>;
> +};
> +
> +iio-hwmon {
> + compatible = "iio-hwmon";
> + io-channels = < 0>, < 0>;
> + io-channel-names = "3v3", "usb";
> +};
>
Re: [PATCH v1 1/3] iio: afe: rescale: Move scale conversion to new function
On 2019-06-11 11:56, Mylène Josserand wrote:
> To prepare the support of processed value, create a function
> to convert the scale according to the voltage-divider node
> used in the device-tree.
>
> Signed-off-by: Mylène Josserand
> ---
> drivers/iio/afe/iio-rescale.c | 54
> +--
> 1 file changed, 31 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index e9ceee66d1e7..3e689d6eb501 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -33,12 +33,41 @@ struct rescale {
> s32 denominator;
> };
>
> +static int rescale_convert_scale(struct rescale *rescale, int *val, int
> *val2)
> +{
> + unsigned long long tmp;
> + int ret;
> +
> + ret = iio_read_channel_scale(rescale->source, val, val2);
> + switch (ret) {
> + case IIO_VAL_FRACTIONAL:
> + *val *= rescale->numerator;
> + *val2 *= rescale->denominator;
> + return ret;
> + case IIO_VAL_INT:
> + *val *= rescale->numerator;
> + if (rescale->denominator == 1)
> + return ret;
> + *val2 = rescale->denominator;
> + return IIO_VAL_FRACTIONAL;
> + case IIO_VAL_FRACTIONAL_LOG2:
> + tmp = *val * 10LL;
> + do_div(tmp, rescale->denominator);
> + tmp *= rescale->numerator;
> + do_div(tmp, 10LL);
> + *val = tmp;
> +
This blank line is in conflict with the style of the surrounding code.
Cheers,
Peter
> + return ret;
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> static int rescale_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int *val, int *val2, long mask)
> {
> struct rescale *rescale = iio_priv(indio_dev);
> - unsigned long long tmp;
> int ret;
>
> switch (mask) {
> @@ -46,28 +75,7 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
> return iio_read_channel_raw(rescale->source, val);
>
> case IIO_CHAN_INFO_SCALE:
> - ret = iio_read_channel_scale(rescale->source, val, val2);
> - switch (ret) {
> - case IIO_VAL_FRACTIONAL:
> - *val *= rescale->numerator;
> - *val2 *= rescale->denominator;
> - return ret;
> - case IIO_VAL_INT:
> - *val *= rescale->numerator;
> - if (rescale->denominator == 1)
> - return ret;
> - *val2 = rescale->denominator;
> - return IIO_VAL_FRACTIONAL;
> - case IIO_VAL_FRACTIONAL_LOG2:
> - tmp = *val * 10LL;
> - do_div(tmp, rescale->denominator);
> - tmp *= rescale->numerator;
> - do_div(tmp, 10LL);
> - *val = tmp;
> - return ret;
> - default:
> - return -EOPNOTSUPP;
> - }
> + return rescale_convert_scale(rescale, val, val2);
> default:
> return -EINVAL;
> }
>
Re: [PATCH v1 0/3] iio: afe: rescale: Add INFO_PROCESSED support
On 2019-06-11 11:56, Mylène Josserand wrote: > Hello everyone, > > You will find a small series that add the support of processed values > for iio-rescale driver. > Thanks to that, it is possible to read processed values in sysfs instead > of getting only raw and scale values. > > Here is an example for a 3v3 voltage reading: > # cat /sys/bus/iio/devices/iio\:device1/in_voltage0_scale > 3.791015625 > # cat /sys/bus/iio/devices/iio\:device1/in_voltage0_raw > 879 > # cat /sys/bus/iio/devices/iio\:device1/in_voltage0_input > 3332 > > It is also possible to read directly the processed values using iio-hwmon > driver (see example in patch03): > > # cat /sys/class/hwmon/hwmon0/in1_input > 3328 > > I seperated my series in 3 patches: >- Patch01: Move the scale conversion into a function to prepare the >support of IIO_CHAN_INFO_PROCESSED. >- Patch02: Add the support of IIO_CHAN_INFO_PROCESSED. >- Patch03: Add an example of the use of hwmon and voltage-divider nodes >in device-tree. > > If you have any feedbacks on it, I will be pleased to read them! The last patch about hwmon has nothing to do with this series, and should be possible as-is without any code changes. No? AFAICT, iio_hwmon calls iio_read_channel_processed, which calls iio_convert_raw_to_processed_unlocked in case IIO_CHAN_INFO_PROCESSES is not handled by the driver. Is that not working? There is also libiio in userspace that provides the scale as a double and makes the conversion to a processed value trivial, so the series is really mostly about the convenience of having a human directly seeing the processed value in sysfs. Right? If that is deemed valuable, then I think it should be fixed in a central location, and not individually for each and every driver. Anyway, not my call, just stating my opinion, but those are the reasons for me not adding a processed channel from the very beginning. Cheers, Peter > Best regards, > Mylène > > Mylène Josserand (3): > iio: afe: rescale: Move scale conversion to new function > iio: afe: rescale: Add support of CHAN_INFO_PROCESSED > dt-bindings: iio: afe: Add hwmon example > > .../bindings/iio/afe/voltage-divider.txt | 24 ++ > drivers/iio/afe/iio-rescale.c | 96 > -- > 2 files changed, 96 insertions(+), 24 deletions(-) >
Re: [PATCH] i2c: mux: pinctrl: use flexible-array member and struct_size() helper
On 2019-06-03 16:53, Gustavo A. R. Silva wrote: > Update the code to use a flexible array member instead of a pointer in > structure i2c_mux_pinctrl and use the struct_size() helper. > > Also, make use of the struct_size() helper instead of an open-coded > version in order to avoid any potential type mistakes, in particular > in the context in which this code is being used. > > So, replace the following form: > > sizeof(*mux) + num_names * sizeof(*mux->states) > > with: > > struct_size(mux, states, num_names) > > This code was detected with the help of Coccinelle. > > Signed-off-by: Gustavo A. R. Silva Thanks, patch applied to i2c-mux/for-next Cheers, Peter
Re: [PATCH] i2c: mux: Use struct_size() in devm_kzalloc()
On 2019-05-29 18:20, Gustavo A. R. Silva wrote:
> One of the more common cases of allocation size calculations is finding
> the size of a structure that has a zero-sized array at the end, along
> with memory for some number of elements for that array. For example:
>
> struct foo {
> int stuff;
> struct boo entry[];
> };
>
> instance = devm_kzalloc(dev, sizeof(struct foo) + count * sizeof(struct boo),
> GFP_KERNEL);
>
> Instead of leaving these open-coded and prone to type mistakes, we can
> now use the new struct_size() helper:
>
> instance = devm_kzalloc(dev, struct_size(instance, entry, count), GFP_KERNEL);
>
> This code was detected with the help of Coccinelle.
Thanks, patch applied to i2c-mux/for-next
Cheers,
Peter
Re: [PATCH v2 2/3] i2c-mux-gpio: Unpin the platform-specific GPIOs request code
On 2019-04-26 01:20, Serge Semin wrote:
> The GPIOs request loop can be safely moved to a separate function.
> First of all it shall improve the code readability. Secondly the
> initialization loop at this point is used for both of- and
> platform_data-based initialization paths, but it will be changed in
> the next patch, so by isolating the code we'll simplify the future
> work.
This patch is just preparatory for patch 3/3, as I see it. And since
I'm not really fond of the end result after patch 3/3, I'm going to
sum up my issues here, instead of trying do it piecemeal in the two
patches.
Linus and Jean, for your convenience, link to this patch series [1].
While I agree with the goal (to use the more flexible gpiod functions
to get at the gpio descriptors), the cost is too high when the init
code for platform and OF is basically completely separated. I much
prefer the approach taken by Linus [2], which instead converts the
platform interface and its single user to use gpio descriptors instead
of the legacy gpio interface. The i2c-mux-gpio code then has the
potential to take a unified approach to the given gpio descriptors,
wherever they are originating from, which is much nicer than the
code-fork in this series.
I also think it is pretty pointless to first split the code into
platform and OF paths, just so that the next patch (from Linus) can
unify the two paths again. I'd like to skip the intermediate step.
So, I'm hoping for the following to happen.
1. Sergey sends a revised patch for patch 1/3.
2. I put the patch on the for-next branch.
3. Linus rebases his patch on top of that (while thinking about
the questions raised by Sergey).
4. Sergey tests the result, I and Jean review it, then possibly
go back to 3.
5. I put the patch on the for-next branch.
Is that ok? Or is someone insisting that we take a detour?
Cheers,
Peter
[1] https://patchwork.ozlabs.org/cover/1091119/ (and show related)
[2] https://patchwork.ozlabs.org/patch/1109521/
> Signed-off-by: Serge Semin
>
> ---
> Changelog v2
> - Create a dedicated initial_state field in the "gpiomux" structure to
> keep an initial channel selector state.
> ---
> drivers/i2c/muxes/i2c-mux-gpio.c | 113 +++
> 1 file changed, 68 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c
> b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 54158b825acd..e10f72706b99 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -20,7 +20,8 @@
>
> struct gpiomux {
> struct i2c_mux_gpio_platform_data data;
> - unsigned gpio_base;
> + unsigned int gpio_base;
> + unsigned int initial_state;
> struct gpio_desc **gpios;
> };
>
> @@ -162,13 +163,68 @@ static int i2c_mux_gpio_probe_plat(struct gpiomux *mux,
> return 0;
> }
>
> +static int i2c_mux_gpio_request_plat(struct gpiomux *mux,
> + struct platform_device *pdev)
> +{
> + struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
> + struct gpio_desc *gpio_desc;
> + struct i2c_adapter *root;
> + struct device *gpio_dev;
> + int i, ret;
> +
> + root = i2c_root_adapter(>parent->dev);
> +
> + for (i = 0; i < mux->data.n_gpios; i++) {
> + ret = gpio_request(mux->gpio_base + mux->data.gpios[i],
> +"i2c-mux-gpio");
> + if (ret) {
> + dev_err(>dev, "Failed to request GPIO %d\n",
> + mux->data.gpios[i]);
> + goto err_request_gpio;
> + }
> +
> + ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
> + mux->initial_state & (1 << i));
> + if (ret) {
> + dev_err(>dev,
> + "Failed to set direction of GPIO %d to
> output\n",
> + mux->data.gpios[i]);
> + i++;/* gpio_request above succeeded, so must free */
> + goto err_request_gpio;
> + }
> +
> + gpio_desc = gpio_to_desc(mux->gpio_base + mux->data.gpios[i]);
> + mux->gpios[i] = gpio_desc;
> +
> + if (!muxc->mux_locked)
> + continue;
> +
> + gpio_dev = _desc->gdev->dev;
> + muxc->mux_locked = i2c_root_adapter(gpio_dev) == root;
> + }
> +
> + return 0;
> +
> +err_request_gpio:
> + for (; i > 0; i--)
> + gpio_free(mux->gpio_base + mux->data.gpios[i - 1]);
> +
> + return ret;
> +}
> +
> +static void i2c_mux_gpio_free(struct gpiomux *mux)
> +{
> + int i;
> +
> + for (i = 0; i < mux->data.n_gpios; i++)
> + gpiod_free(mux->gpios[i]);
> +}
> +
> static int i2c_mux_gpio_probe(struct platform_device *pdev)
> {
> struct i2c_mux_core *muxc;
> struct gpiomux *mux;
> struct i2c_adapter *parent;
> - struct
Re: [PATCH v2 1/3] i2c-mux-gpio: Unpin a platform-based device initialization
Thanks for your patches, and sorry for the slow review...
On 2019-04-26 01:20, Serge Semin wrote:
> We can unpin a code specific for i2c-mux-gpio device declared
Unpin? I think the common phrase is "factor out"? That unpin is also
present in the subject. BTW, I prefer the subject to start with
[PATCH ...] i2c: mux: gpio: factor out...
> as platform device. In this case the platform data just needs to be
> copied to the private storage and if GPIO chip pointer is referring to
> a valid GPIO chip descriptor save it' base number for further GPIOs
> request and initialization. The rest of the code is common for both
> platform and OF-based setups.
>
> It's also pointless and might be even errors prone to proceed with
> further initialization if OF kernel config is disabled and plat-based
> initialization isn't defined. Just return an error in this case.
Hmm, there are a couple of other language issues, how about:
Subject: i2c: mux: gpio: factor out platform-based device init
We can factor out the probe code specific for i2c-mux-gpio when used as
a platform device. In this case the platform data just needs to be
copied to the private storage except if the GPIO chip pointer is
referring to a valid GPIO chip descriptor, in which case we save its
base number for further GPIO requests and init. The rest of the code
is common for both platform and OF-based setups.
It's also pointless and might even be error prone to proceed with
further initialization if neither OF nor platform-based parameters
are given. Just error out in this case.
> Signed-off-by: Serge Semin
>
> ---
> Changelog v2
> - Return an error if OF kconfig is disabled while dt-based GPIOs probe
> is called.
> ---
> drivers/i2c/muxes/i2c-mux-gpio.c | 69 ++--
> 1 file changed, 38 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c
> b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 13882a2a4f60..54158b825acd 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -132,48 +132,55 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
> static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
> struct platform_device *pdev)
> {
> - return 0;
> + return -EINVAL;
This is unrelated and should be a separate patch, as is almost always the
case when there is an "also" like you have in the commit message.
> }
> #endif
>
> +static int i2c_mux_gpio_probe_plat(struct gpiomux *mux,
> + struct platform_device *pdev)
I think you should spell out platform, and please align the arguments
vertically.
> +{
> + struct i2c_mux_gpio_platform_data *data = dev_get_platdata(>dev);
> + struct gpio_chip *gpio;
> +
> + /*
> + * If a GPIO chip name is provided, the GPIO pin numbers provided are
> + * relative to its base GPIO number. Otherwise they are absolute.
> + */
> + if (data->gpio_chip) {
> + gpio = gpiochip_find(data->gpio_chip,
> + match_gpio_chip_by_label);
> + if (!gpio)
> + return -EPROBE_DEFER;
> +
> + mux->gpio_base = gpio->base;
> + } else {
> + mux->gpio_base = 0;
This else-branch is pointless. I realize that you are just moving
code around, but mux->gpio_base is already zero here. Could be
simplified in a followup commit, I suppose.
Cheers,
Peter
> + }
> +
> + memcpy(>data, data, sizeof(mux->data));
> +
> + return 0;
> +}
> +
> static int i2c_mux_gpio_probe(struct platform_device *pdev)
> {
> struct i2c_mux_core *muxc;
> struct gpiomux *mux;
> struct i2c_adapter *parent;
> struct i2c_adapter *root;
> - unsigned initial_state, gpio_base;
> + unsigned initial_state;
> int i, ret;
>
> mux = devm_kzalloc(>dev, sizeof(*mux), GFP_KERNEL);
> if (!mux)
> return -ENOMEM;
>
> - if (!dev_get_platdata(>dev)) {
> + if (!dev_get_platdata(>dev))
> ret = i2c_mux_gpio_probe_dt(mux, pdev);
> - if (ret < 0)
> - return ret;
> - } else {
> - memcpy(>data, dev_get_platdata(>dev),
> - sizeof(mux->data));
> - }
> -
> - /*
> - * If a GPIO chip name is provided, the GPIO pin numbers provided are
> - * relative to its base GPIO number. Otherwise they are absolute.
> - */
> - if (mux->data.gpio_chip) {
> - struct gpio_chip *gpio;
> -
> - gpio = gpiochip_find(mux->data.gpio_chip,
> - match_gpio_chip_by_label);
> - if (!gpio)
> - return -EPROBE_DEFER;
> -
> - gpio_base = gpio->base;
> - } else {
> - gpio_base = 0;
> - }
> + else
> + ret = i2c_mux_gpio_probe_plat(mux, pdev);
> + if (ret)
> + return ret;
>
> parent =
Re: [PATCH V4] drivers: i2c: tegra: fix checkpatch defects
On 2019-06-06 09:35, Bitan Biswas wrote: > Fix checkpatch.pl warning(s)/error(s)/check(s) in i2c-tegra.c > > Remove redundant BUG_ON calls or replace with WARN_ON_ONCE > as needed. Replace BUG() with error handling code. > Define I2C_ERR_UNEXPECTED_STATUS for error handling. > > Signed-off-by: Bitan Biswas > --- > drivers/i2c/busses/i2c-tegra.c | 67 > +++--- > 1 file changed, 37 insertions(+), 30 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index 76b7926..55a5d87 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -78,6 +78,7 @@ > #define I2C_ERR_NO_ACK 0x01 > #define I2C_ERR_ARBITRATION_LOST 0x02 > #define I2C_ERR_UNKNOWN_INTERRUPT0x04 > +#define I2C_ERR_UNEXPECTED_STATUS 0x08 Use tabs like the the surrounding code. And perhaps convert all these flags to use the BIT() macro? > > #define PACKET_HEADER0_HEADER_SIZE_SHIFT 28 > #define PACKET_HEADER0_PACKET_ID_SHIFT 16 > @@ -112,7 +113,7 @@ > #define I2C_CLKEN_OVERRIDE 0x090 > #define I2C_MST_CORE_CLKEN_OVR BIT(0) > > -#define I2C_CONFIG_LOAD_TIMEOUT 100 > +#define I2C_CONFIG_LOAD_TMOUT100 Similar to xfer_tm already mentioned by Dmitry; just keep it as ..._TIMEOUT and ignore checkpatch on this issue. Or juggle the code in some other way to pacify checkpatch. E.g. abbreviate CONFIG instead? Or something. CONF is way easier to read than TMOUT IMHO... Cheers, Peter
Re: [RFC PATCH 39/57] drivers: mux: Use class_find_device_by_of_node helper
On 2019-06-03 18:45, Suzuki K Poulose wrote:
> Hi Peter,
> Sorry about that. The routine is a wrapper to class_find_device()
> which uses a generic routine to match the of_node, instead of the
> driver specific of_dev_node_match(). The series adds such wrappers for
> {bus/drivers/class}_find_device(). Unfortunately I didn't add
> individual driver maintainers to the patches, which add those wrappers.
> For the moment, please find the link below for the patch :
>
> https://lkml.kernel.org/r/1559577023-558-29-git-send-email-suzuki.poul...@arm.com
>
>
> I will try to address it in the next revision.
For the record, that patch references some other new function
"device_match_of_node" for which I do not have a definition. But with
the above link, I was able to find it without too much effort.
All looks ok to me, so, if you fix that blank line thing,
Reviewed-by: Peter Rosin
Cheers,
Peter
Re: [RFC PATCH 39/57] drivers: mux: Use class_find_device_by_of_node helper
Hi!
This all sounds like nice changes. First a couple of nitpicks:
From the cover letter, included here to spare most of the others...
> subsystems. This series is an attempt to consolidate the and cleanup
s/the and/and/
On 2019-06-03 17:50, Suzuki K Poulose wrote:
> Use the generic helper to find a device matching the of_node.
>
> Cc: Peter Rosin
> Signed-off-by: Suzuki K Poulose
> ---
> drivers/mux/core.c | 8 +---
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
> index d1271c1..3591e40 100644
> --- a/drivers/mux/core.c
> +++ b/drivers/mux/core.c
> @@ -405,18 +405,12 @@ int mux_control_deselect(struct mux_control *mux)
> }
> EXPORT_SYMBOL_GPL(mux_control_deselect);
>
> -static int of_dev_node_match(struct device *dev, const void *data)
> -{
> - return dev->of_node == data;
> -}
> -
> /* Note this function returns a reference to the mux_chip dev. */
> static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
> {
> struct device *dev;
>
> - dev = class_find_device(_class, NULL, np, of_dev_node_match);
> -
Nitpick #2. Please leave the blank line where it belongs.
However, how can I review this if I do not get to see the patch that
adds the class_find_device_by_of_node function? Please provide a
little bit more context!
Cheers,
Peter
> + dev = class_find_device_by_of_node(_class, NULL, np);
> return dev ? to_mux_chip(dev) : NULL;
> }
>
>
Re: [PATCH v2 0/3] i2c-mux-gpio: Split plat- and dt-specific code up
On 2019-05-07 11:02, Serge Semin wrote: > Hello folks, > > Any updates on this patchset status? I haven't got any comment on v2, but > instead a notification about the status change was sent to me: > >> * linux-i2c: [v2,1/3] i2c-mux-gpio: Unpin a platform-based device >> initialization >> - http://patchwork.ozlabs.org/patch/1091120/ >> - for: Linux I2C development >>was: New >>now: Superseded >> >> * linux-i2c: [v2,2/3] i2c-mux-gpio: Unpin the platform-specific GPIOs >> request code >> - http://patchwork.ozlabs.org/patch/1091122/ >> - for: Linux I2C development >>was: New >>now: Superseded >> >> * linux-i2c: [v2,3/3] i2c-mux-gpio: Create of-based GPIOs request method >> - http://patchwork.ozlabs.org/patch/1091121/ >> - for: Linux I2C development >>was: New >>now: Superseded > > I may misunderstand something, but how come the v2 patchset switched to be > superseded > while it is the last patchset version I've sent? That was my mistake. Patchwork got confused when v2 was sent as a reply to something in the v1 tree, and marked all 8 patches as "v2". Then I in turn got confused by that, and changed status on the wrong set. Sorry! So, thanks for the heads up, it should be fixed now. As for comments on the patches, I'm personally buried in work and others may have the merge window to focus on... Cheers, Peter
[PATCH 3/3] lib/test_string: add some testcases for strchr and strnchr
Make sure that the trailing NUL is considered part of the string and can
be found.
Signed-off-by: Peter Rosin
---
lib/test_string.c | 77 +++
1 file changed, 77 insertions(+)
diff --git a/lib/test_string.c b/lib/test_string.c
index 98a787e7a1fd..613fd5cc9872 100644
--- a/lib/test_string.c
+++ b/lib/test_string.c
@@ -111,6 +111,73 @@ static __init int memset64_selftest(void)
return 0;
}
+static __init int strchr_selftest(void)
+{
+ const char *test_string = "abcdefghijkl";
+ const char *empty_string = "";
+ char *result;
+ int i;
+
+ for (i = 0; i < strlen(test_string) + 1; i++) {
+ result = strchr(test_string, test_string[i]);
+ if (result - test_string != i)
+ return i + 'a';
+ }
+
+ result = strchr(empty_string, '\0');
+ if (result != empty_string)
+ return 0x101;
+
+ result = strchr(empty_string, 'a');
+ if (result)
+ return 0x102;
+
+ result = strchr(test_string, 'z');
+ if (result)
+ return 0x103;
+
+ return 0;
+}
+
+static __init int strnchr_selftest(void)
+{
+ const char *test_string = "abcdefghijkl";
+ const char *empty_string = "";
+ char *result;
+ int i, j;
+
+ for (i = 0; i < strlen(test_string) + 1; i++) {
+ for (j = 0; j < strlen(test_string) + 2; j++) {
+ result = strnchr(test_string, j, test_string[i]);
+ if (j <= i) {
+ if (!result)
+ continue;
+ return ((i + 'a') << 8) | j;
+ }
+ if (result - test_string != i)
+ return ((i + 'a') << 8) | j;
+ }
+ }
+
+ result = strnchr(empty_string, 0, '\0');
+ if (result)
+ return 0x10001;
+
+ result = strnchr(empty_string, 1, '\0');
+ if (result != empty_string)
+ return 0x10002;
+
+ result = strnchr(empty_string, 1, 'a');
+ if (result)
+ return 0x10003;
+
+ result = strnchr(NULL, 0, '\0');
+ if (result)
+ return 0x10004;
+
+ return 0;
+}
+
static __init int string_selftest_init(void)
{
int test, subtest;
@@ -130,6 +197,16 @@ static __init int string_selftest_init(void)
if (subtest)
goto fail;
+ test = 4;
+ subtest = strchr_selftest();
+ if (subtest)
+ goto fail;
+
+ test = 5;
+ subtest = strnchr_selftest();
+ if (subtest)
+ goto fail;
+
pr_info("String selftests succeeded\n");
return 0;
fail:
--
2.11.0
[PATCH 2/3] lib/test_string: avoid masking memset16/32/64 failures
If a memsetXX implementation is completely broken and fails in the first
iteration, when i, j, and k are all zero, the failure is masked as zero
is returned. Failing in the first iteration is perhaps the most likely
failure, so this makes the tests pretty much useless. Avoid the situation
by always setting a random unused bit in the result on failure.
Fixes: 03270c13c5ff ("lib/string.c: add testcases for memset16/32/64")
Signed-off-by: Peter Rosin
---
lib/test_string.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/lib/test_string.c b/lib/test_string.c
index 0fcdb82dca86..98a787e7a1fd 100644
--- a/lib/test_string.c
+++ b/lib/test_string.c
@@ -35,7 +35,7 @@ static __init int memset16_selftest(void)
fail:
kfree(p);
if (i < 256)
- return (i << 24) | (j << 16) | k;
+ return (i << 24) | (j << 16) | k | 0x8000;
return 0;
}
@@ -71,7 +71,7 @@ static __init int memset32_selftest(void)
fail:
kfree(p);
if (i < 256)
- return (i << 24) | (j << 16) | k;
+ return (i << 24) | (j << 16) | k | 0x8000;
return 0;
}
@@ -107,7 +107,7 @@ static __init int memset64_selftest(void)
fail:
kfree(p);
if (i < 256)
- return (i << 24) | (j << 16) | k;
+ return (i << 24) | (j << 16) | k | 0x8000;
return 0;
}
--
2.11.0
[PATCH RESEND 3/3] lib/test_string: add some testcases for strchr and strnchr
Make sure that the trailing NUL is considered part of the string and can
be found.
Signed-off-by: Peter Rosin
---
lib/test_string.c | 77 +++
1 file changed, 77 insertions(+)
diff --git a/lib/test_string.c b/lib/test_string.c
index 98a787e7a1fd..613fd5cc9872 100644
--- a/lib/test_string.c
+++ b/lib/test_string.c
@@ -111,6 +111,73 @@ static __init int memset64_selftest(void)
return 0;
}
+static __init int strchr_selftest(void)
+{
+ const char *test_string = "abcdefghijkl";
+ const char *empty_string = "";
+ char *result;
+ int i;
+
+ for (i = 0; i < strlen(test_string) + 1; i++) {
+ result = strchr(test_string, test_string[i]);
+ if (result - test_string != i)
+ return i + 'a';
+ }
+
+ result = strchr(empty_string, '\0');
+ if (result != empty_string)
+ return 0x101;
+
+ result = strchr(empty_string, 'a');
+ if (result)
+ return 0x102;
+
+ result = strchr(test_string, 'z');
+ if (result)
+ return 0x103;
+
+ return 0;
+}
+
+static __init int strnchr_selftest(void)
+{
+ const char *test_string = "abcdefghijkl";
+ const char *empty_string = "";
+ char *result;
+ int i, j;
+
+ for (i = 0; i < strlen(test_string) + 1; i++) {
+ for (j = 0; j < strlen(test_string) + 2; j++) {
+ result = strnchr(test_string, j, test_string[i]);
+ if (j <= i) {
+ if (!result)
+ continue;
+ return ((i + 'a') << 8) | j;
+ }
+ if (result - test_string != i)
+ return ((i + 'a') << 8) | j;
+ }
+ }
+
+ result = strnchr(empty_string, 0, '\0');
+ if (result)
+ return 0x10001;
+
+ result = strnchr(empty_string, 1, '\0');
+ if (result != empty_string)
+ return 0x10002;
+
+ result = strnchr(empty_string, 1, 'a');
+ if (result)
+ return 0x10003;
+
+ result = strnchr(NULL, 0, '\0');
+ if (result)
+ return 0x10004;
+
+ return 0;
+}
+
static __init int string_selftest_init(void)
{
int test, subtest;
@@ -130,6 +197,16 @@ static __init int string_selftest_init(void)
if (subtest)
goto fail;
+ test = 4;
+ subtest = strchr_selftest();
+ if (subtest)
+ goto fail;
+
+ test = 5;
+ subtest = strnchr_selftest();
+ if (subtest)
+ goto fail;
+
pr_info("String selftests succeeded\n");
return 0;
fail:
--
2.11.0
[PATCH RESEND 2/3] lib/test_string: avoid masking memset16/32/64 failures
If a memsetXX implementation is completely broken and fails in the first
iteration, when i, j, and k are all zero, the failure is masked as zero
is returned. Failing in the first iteration is perhaps the most likely
failure, so this makes the tests pretty much useless. Avoid the situation
by always setting a random unused bit in the result on failure.
Fixes: 03270c13c5ff ("lib/string.c: add testcases for memset16/32/64")
Signed-off-by: Peter Rosin
---
lib/test_string.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/lib/test_string.c b/lib/test_string.c
index 0fcdb82dca86..98a787e7a1fd 100644
--- a/lib/test_string.c
+++ b/lib/test_string.c
@@ -35,7 +35,7 @@ static __init int memset16_selftest(void)
fail:
kfree(p);
if (i < 256)
- return (i << 24) | (j << 16) | k;
+ return (i << 24) | (j << 16) | k | 0x8000;
return 0;
}
@@ -71,7 +71,7 @@ static __init int memset32_selftest(void)
fail:
kfree(p);
if (i < 256)
- return (i << 24) | (j << 16) | k;
+ return (i << 24) | (j << 16) | k | 0x8000;
return 0;
}
@@ -107,7 +107,7 @@ static __init int memset64_selftest(void)
fail:
kfree(p);
if (i < 256)
- return (i << 24) | (j << 16) | k;
+ return (i << 24) | (j << 16) | k | 0x8000;
return 0;
}
--
2.11.0
[PATCH RESEND 1/3] lib/string: allow searching for NUL with strnchr
strchr considers the terminating NUL to be part of the string, and NUL
can thus be searched for with that function. For consistency, do the
same with strnchr.
Signed-off-by: Peter Rosin
---
lib/string.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/lib/string.c b/lib/string.c
index 3ab861c1a857..9d64d7ab401a 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -367,6 +367,9 @@ EXPORT_SYMBOL(strncmp);
* strchr - Find the first occurrence of a character in a string
* @s: The string to be searched
* @c: The character to search for
+ *
+ * Note that the %NUL-terminator is considered part of the string, and can
+ * be searched for.
*/
char *strchr(const char *s, int c)
{
@@ -420,12 +423,18 @@ EXPORT_SYMBOL(strrchr);
* @s: The string to be searched
* @count: The number of characters to be searched
* @c: The character to search for
+ *
+ * Note that the %NUL-terminator is considered part of the string, and can
+ * be searched for.
*/
char *strnchr(const char *s, size_t count, int c)
{
- for (; count-- && *s != '\0'; ++s)
+ while (count--) {
if (*s == (char)c)
return (char *)s;
+ if (*s++ == '\0')
+ break;
+ }
return NULL;
}
EXPORT_SYMBOL(strnchr);
--
2.11.0
[PATCH RESEND 0/3] lib/string: search for NUL with strchr/strnchr
[With better address for Matthew?] Hi! I noticed an inconsistency where strchr and strnchr do not behave the same with respect to the trailing NUL. strchr is standardised and the kernel function conforms, and the kernel relies on the behavior. So, naturally strchr stays as-is and strnchr is what I change. While writing a few tests to verify that my new strnchr loop was sane, I noticed that the tests for memset16/32/64 had a problem. Since it's all about the lib/string.c file I made a short series of it all... But where to send it? get_maintainer suggests no victim, so I'm aiming at those that signed-off on the memset16/32/64 bug... Cheers, Peter Peter Rosin (3): lib/string: allow searching for NUL with strnchr lib/test_string: avoid masking memset16/32/64 failures lib/test_string: add some testcases for strchr and strnchr lib/string.c | 11 +++- lib/test_string.c | 83 +-- 2 files changed, 90 insertions(+), 4 deletions(-) -- 2.11.0
[PATCH 0/3] lib/string: search for NUL with strchr/strnchr
Hi! I noticed an inconsistency where strchr and strnchr do not behave the same with respect to the trailing NUL. strchr is standardised and the kernel function conforms, and the kernel relies on the behavior. So, naturally strchr stays as-is and strnchr is what I change. While writing a few tests to verify that my new strnchr loop was sane, I noticed that the tests for memset16/32/64 had a problem. Since it's all about the lib/string.c file I made a short series of it all... But where to send it? get_maintainer suggests no victim, so I'm aiming at those that signed-off on the memset16/32/64 bug... Cheers, Peter Peter Rosin (3): lib/string: allow searching for NUL with strnchr lib/test_string: avoid masking memset16/32/64 failures lib/test_string: add some testcases for strchr and strnchr lib/string.c | 11 +++- lib/test_string.c | 83 +-- 2 files changed, 90 insertions(+), 4 deletions(-) -- 2.11.0
