Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1611500 > unrolled thread

Re: [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver Linus Walleij <linus.walleij@linaro.org> - 2017-03-29 04:10 +0200
    [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver "Han, Nandor (GE Healthcare)" <nandor.han@ge.com> - 2017-04-05 15:30 +0200
      Re: [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver Linus Walleij <linus.walleij@linaro.org> - 2017-04-07 12:10 +0200
        [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver "Han, Nandor (GE Healthcare)" <nandor.han@ge.com> - 2017-04-08 09:40 +0200

#1611500 — Re: [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver

On Mon, Mar 27, 2017 at 8:23 AM, Nandor Han <nandor.han@ge.com> wrote:

> This is a simple driver that provides a /sys/class/gpio
> interface for controlling and configuring the GPIO lines.

Use the gpio tools in tools/gpio, use the characcter device.
Do not use sysfs. Change this to reference the tools.

> It does not provide support for chip select or interrupts.
>
> Signed-off-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Semi Malinen <semi.malinen@ge.com>
(...)
> +exar   Exar Corporation

Send this as a separate patch to the DT bindings maintainer
(Rob Herring.)

> +static int xra1403_get_byte(struct xra1403 *xra, unsigned int addr)
> +{
> +       return spi_w8r8(xra->spi, XRA_READ | (addr << 1));
> +}
> +
> +static int xra1403_get_bit(struct xra1403 *xra, unsigned int addr,
> +                          unsigned int bit)
> +{
> +       int ret;
> +
> +       ret = xra1403_get_byte(xra, addr + (bit > 7));
> +       if (ret < 0)
> +               return ret;
> +
> +       return !!(ret & BIT(bit % 8));
> +}

This looks like it can use regmap-spi right off, do you agree?

git grep devm_regmap_init_spi
should give you some examples of how to use it.

If it's not off-the shelf regmap drivers like
drivers/iio/pressure/mpl115_spi.c
give examples of how to make more elaborate custom
SPI transfers with regmap.

> +static int xra1403_set_bit(struct xra1403 *xra, unsigned int addr,
> +                          unsigned int bit, int value)
> +{
> +       int ret;
> +       u8 mask;
> +       u8 tx[2];
> +
> +       addr += bit > 7;
> +
> +       mutex_lock(&xra->lock);
> +
> +       ret = xra1403_get_byte(xra, addr);
> +       if (ret < 0)
> +               goto out_unlock;
> +
> +       mask = BIT(bit % 8);
> +       if (value)
> +               value = ret | mask;
> +       else
> +               value = ret & ~mask;
> +
> +       if (value != ret) {
> +               tx[0] = addr << 1;
> +               tx[1] = value;
> +               ret = spi_write(xra->spi, tx, sizeof(tx));
> +       } else {
> +               ret = 0;
> +       }
> +
> +out_unlock:
> +       mutex_unlock(&xra->lock);
> +
> +       return ret;
> +}

Classical mask-and-set implementation right?
With regmap this becomes simply regmap_update_bits(map, addr, mask, set)

> +static int xra1403_probe(struct spi_device *spi)
> +{
> +       struct xra1403 *xra;
> +       struct gpio_desc *reset_gpio;
> +
> +       xra = devm_kzalloc(&spi->dev, sizeof(*xra), GFP_KERNEL);
> +       if (!xra)
> +               return -ENOMEM;
> +
> +       /* bring the chip out of reset */
> +       reset_gpio = gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
> +       if (IS_ERR(reset_gpio))
> +               dev_warn(&spi->dev, "could not get reset-gpios\n");
> +       else if (reset_gpio)
> +               gpiod_put(reset_gpio);

I don't think you should put it, other than in the remove()
function and in that case you need to have it in the
state container.

> +       mutex_init(&xra->lock);
> +
> +       xra->chip.direction_input = xra1403_direction_input;
> +       xra->chip.direction_output = xra1403_direction_output;

Please implement .get_direction(). This is very nice to have.

> +static int xra1403_remove(struct spi_device *spi)
> +{
> +       struct xra1403 *xra = spi_get_drvdata(spi);
> +
> +       gpiochip_remove(&xra->chip);

Use devm_gpiochip_add_data() and this remove is not
needed at all.

> +static int __init xra1403_init(void)
> +{
> +       return spi_register_driver(&xra1403_driver);
> +}
> +
> +/*
> + * register after spi postcore initcall and before
> + * subsys initcalls that may rely on these GPIOs
> + */
> +subsys_initcall(xra1403_init);
> +
> +static void __exit xra1403_exit(void)
> +{
> +       spi_unregister_driver(&xra1403_driver);
> +}
> +module_exit(xra1403_exit);

This seems like tricksy. Just module_spi_driver()
should be fine don't you think?

Yours,
Linus Walleij

[toc] | [next] | [standalone]

#1616921

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: 29 March 2017 05:07
> To: Han, Nandor (GE Healthcare) <nandor.han@ge.com>
> Cc: Alexandre Courbot <gnurou@gmail.com>; Rob Herring <robh+dt@kernel.org>; Mark Rutland
> <mark.rutland@arm.com>; linux-gpio@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> Malinen, Semi (GE Healthcare) <semi.malinen@ge.com>
> Subject: EXT: Re: [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver
> 
> On Mon, Mar 27, 2017 at 8:23 AM, Nandor Han <nandor.han@ge.com> wrote:
> 
> > This is a simple driver that provides a /sys/class/gpio
> > interface for controlling and configuring the GPIO lines.
> 
> Use the gpio tools in tools/gpio, use the characcter device.
> Do not use sysfs. Change this to reference the tools.
> 
> > It does not provide support for chip select or interrupts.
> >
> > Signed-off-by: Nandor Han <nandor.han@ge.com>
> > Signed-off-by: Semi Malinen <semi.malinen@ge.com>
> (...)
> > +exar   Exar Corporation
> 
> Send this as a separate patch to the DT bindings maintainer
> (Rob Herring.)
> 

OK. I will create a separate patch with this one. 
I guess is not an issue to send all the patches to Rob as well.

<snip>

> > +
> > +       ret = xra1403_get_byte(xra, addr + (bit > 7));
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       return !!(ret & BIT(bit % 8));
> > +}
> 
> This looks like it can use regmap-spi right off, do you agree?
> 

Yes. Using regmap-spi will definitely improve the code readability and reduce boilerplate.
Done.

> git grep devm_regmap_init_spi
> should give you some examples of how to use it.
> 
> If it's not off-the shelf regmap drivers like
> drivers/iio/pressure/mpl115_spi.c
> give examples of how to make more elaborate custom
> SPI transfers with regmap.
> 

Thanks, I did check other drivers as examples. 
Not that I needed for this driver, but ...mpl115_spi.c doesn't seem to
use regmap (checked on next-20170327)

<snip>

> > +
> > +       if (value != ret) {
> > +               tx[0] = addr << 1;
> > +               tx[1] = value;
> > +               ret = spi_write(xra->spi, tx, sizeof(tx));
> > +       } else {
> > +               ret = 0;
> > +       }
> > +
> > +out_unlock:
> > +       mutex_unlock(&xra->lock);
> > +
> > +       return ret;
> > +}
> 
> Classical mask-and-set implementation right?
> With regmap this becomes simply regmap_update_bits(map, addr, mask, set)
> 

True. :)

<snip>

> > +       /* bring the chip out of reset */
> > +       reset_gpio = gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
> > +       if (IS_ERR(reset_gpio))
> > +               dev_warn(&spi->dev, "could not get reset-gpios\n");
> > +       else if (reset_gpio)
> > +               gpiod_put(reset_gpio);
> 
> I don't think you should put it, other than in the remove()
> function and in that case you need to have it in the
> state container.

Can you please be more explicit here.

Currently I'm trying to bring the device out from reset in case reset GPIO is provided.
I don't see how this could be done in remove()  :)

> 
> > +       mutex_init(&xra->lock);
> > +
> > +       xra->chip.direction_input = xra1403_direction_input;
> > +       xra->chip.direction_output = xra1403_direction_output;
> 
> Please implement .get_direction(). This is very nice to have.
> 

Done

> > +static int xra1403_remove(struct spi_device *spi)
> > +{
> > +       struct xra1403 *xra = spi_get_drvdata(spi);
> > +
> > +       gpiochip_remove(&xra->chip);
> 
> Use devm_gpiochip_add_data() and this remove is not
> needed at all.
> 

True. Done

<snip>

> > +subsys_initcall(xra1403_init);
> > +
> > +static void __exit xra1403_exit(void)
> > +{
> > +       spi_unregister_driver(&xra1403_driver);
> > +}
> > +module_exit(xra1403_exit);
> 
> This seems like tricksy. Just module_spi_driver()
> should be fine don't you think?

Yeah. TBH I don't have a strong reason why module_spi_driver init level shouldn't be enough. 
Done.

Regards,
    Nandor

[toc] | [prev] | [next] | [standalone]

#1618651

On Wed, Apr 5, 2017 at 3:24 PM, Han, Nandor (GE Healthcare)
<nandor.han@ge.com> wrote:
> [Me]
>> > +       /* bring the chip out of reset */
>> > +       reset_gpio = gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
>> > +       if (IS_ERR(reset_gpio))
>> > +               dev_warn(&spi->dev, "could not get reset-gpios\n");
>> > +       else if (reset_gpio)
>> > +               gpiod_put(reset_gpio);
>>
>> I don't think you should put it, other than in the remove()
>> function and in that case you need to have it in the
>> state container.
>
> Can you please be more explicit here.
>
> Currently I'm trying to bring the device out from reset in case reset GPIO is provided.
> I don't see how this could be done in remove()  :)

If you issue gpiod_put() you release the GPIO hande so something else
can go in and grab the GPIO and assert the reset.

This is not what you want to make possible: you want to hold this gpiod handle
as long as the driver is running. devm_gpiod_get_optional() will do the
trick if you don't want to put the handle under explicit control.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1619239

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: 07 April 2017 13:07
> To: Han, Nandor (GE Healthcare) <nandor.han@ge.com>
> Cc: Alexandre Courbot <gnurou@gmail.com>; Rob Herring <robh+dt@kernel.org>; Mark Rutland
> <mark.rutland@arm.com>; linux-gpio@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> Malinen, Semi (GE Healthcare) <semi.malinen@ge.com>
> Subject: EXT: Re: [PATCH 1/3] gpio - Add EXAR XRA1403 SPI GPIO expander driver
> 
> On Wed, Apr 5, 2017 at 3:24 PM, Han, Nandor (GE Healthcare)
> <nandor.han@ge.com> wrote:
> > [Me]
> >> > +       /* bring the chip out of reset */
> >> > +       reset_gpio = gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
> >> > +       if (IS_ERR(reset_gpio))
> >> > +               dev_warn(&spi->dev, "could not get reset-gpios\n");
> >> > +       else if (reset_gpio)
> >> > +               gpiod_put(reset_gpio);
> >>
> >> I don't think you should put it, other than in the remove()
> >> function and in that case you need to have it in the
> >> state container.
> >
> > Can you please be more explicit here.
> >
> > Currently I'm trying to bring the device out from reset in case reset GPIO is provided.
> > I don't see how this could be done in remove()  :)
> 
> If you issue gpiod_put() you release the GPIO hande so something else
> can go in and grab the GPIO and assert the reset.
> 
> This is not what you want to make possible: you want to hold this gpiod handle
> as long as the driver is running. devm_gpiod_get_optional() will do the
> trick if you don't want to put the handle under explicit control.
> 

That was my first intention to release the reset line in case somebody else wants to control it. I did it
like that because usually reset line controls multiple devices and probably some upper layer wants to
control that. 

After your comment I did some analysing and I will follow your advice and change the reset line handling. 
Once the GPIO is provided to the driver the driver will own it and bring out the device from reset. In case not
provided the reset line is somebody else responsibility. 

This way we are able to cover multiple use-cases. 

Thanks Linus,
Nandy

> Yours,
> Linus Walleij

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web