[U-Boot] [PATCH] i2c: muxes: pca954x: Add support for GPIO reset line

[Simon Glass]

+Tom for question below

Hi Moritz,

On 8 September 2017 at 23:27, Moritz Fischer <moritz.fischer at ettus.com> wrote:
> Hi Simon,
>
> On Fri, Sep 8, 2017 at 9:55 PM, Simon Glass <sjg at chromium.org> wrote:
>> On 5 September 2017 at 12:24, Moritz Fischer <moritz.fischer at ettus.com> wrote:
>>> This commit adds support for GPIO reset lines matching the
>>> common linux "reset-gpios" devicetree binding.
>>>
>>> Signed-off-by: Moritz Fischer <moritz.fischer at ettus.com>
>>> Cc: Heiko Schocher <hs at denx.de>
>>> Cc: Stefan Roese <sr at denx.de>
>>> Cc: Marek Behún <marek.behun at nic.cz>
>>> Cc: Simon Glass <sjg at chromium.org>
>>> Cc: Michal Simek <monstr at monstr.eu>
>>>
>>> ---
>>>  drivers/i2c/muxes/pca954x.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 43 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/muxes/pca954x.c b/drivers/i2c/muxes/pca954x.c
>>> index 383f72f552..dd28ff057b 100644
>>> --- a/drivers/i2c/muxes/pca954x.c
>>> +++ b/drivers/i2c/muxes/pca954x.c
>>> @@ -1,5 +1,6 @@
>>>  /*
>>>   * Copyright (C) 2015 - 2016 Xilinx, Inc.
>>> + * Copyright (C) 2017 National Instruments Corp
>>>   * Written by Michal Simek
>>>   *
>>>   * SPDX-License-Identifier:    GPL-2.0+
>>> @@ -9,7 +10,10 @@
>>>  #include <dm.h>
>>>  #include <errno.h>
>>>  #include <i2c.h>
>>> -#include <asm/gpio.h>
>>> +
>>> +#if CONFIG_DM_GPIO
>>> +# include <asm-generic/gpio.h>
>>> +#endif /* CONFIG_DM_GPIO */
>>
>> Can we drop the #ifdef?
>
> Yeah, will do.
>>
>>>
>>>  DECLARE_GLOBAL_DATA_PTR;
>>>
>>> @@ -30,6 +34,9 @@ struct chip_desc {
>>>  struct pca954x_priv {
>>>         u32 addr; /* I2C mux address */
>>>         u32 width; /* I2C mux width - number of busses */
>>> +#ifdef CONFIG_DM_GPIO
>>> +       struct gpio_desc gpio_mux_reset;
>>> +#endif /* CONFIG_DM_GPIO */
>>>  };
>>>
>>>  static const struct chip_desc chips[] = {
>>> @@ -105,10 +112,45 @@ static int pca954x_ofdata_to_platdata(struct udevice *dev)
>>>         return 0;
>>>  }
>>>
>>> +static int pca954x_probe(struct udevice *dev)
>>> +{
>>> +#ifdef CONFIG_DM_GPIO
>>
>> Can we use if (IS_ENABLED(CONFIG_DM_GPIO)) ?
>
> I suppose. I was wondering. Is this in general preferable?

Well one benefit is that it reduces build-time code branching so we
can potentially detect build errors more easily (e.g. this driver
could be enabled for sandbox). We have traditionally had loads of
#ifdefs in U-Boot and IMO they make the code harder to read. Note that
using if() relies on not using -O0 in many cases but I think we gave
up on supporting -O0 some time ago since debugging of optimised code
is better these days.

Tom, what do you think?

Regards,
Simon