[PATCH] gpio: syscon: do not use raw "set" callback in syscon_gpio_dir_out

[PATCH] gpio: syscon: do not use raw "set" callback in syscon_gpio_dir_out

[Vladimir Murzin]

"set" callback is optional and can be NULL, instead use chip->set
which always points at proper callback function.

Fixes 2c341d62eb4b ("gpio: syscon: add soc specific callback to assign output value")

Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
---
 drivers/gpio/gpio-syscon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c
index 537cec7..cf88a0b 100644
--- a/drivers/gpio/gpio-syscon.c
+++ b/drivers/gpio/gpio-syscon.c
@@ -122,7 +122,7 @@ static int syscon_gpio_dir_out(struct gpio_chip *chip, unsigned offset, int val)
 				   BIT(offs % SYSCON_REG_BITS));
 	}
 
-	priv->data->set(chip, offset, val);
+	chip->set(chip, offset, val);
 
 	return 0;
 }
-- 
1.9.1

Re: [PATCH] gpio: syscon: do not use raw "set" callback in syscon_gpio_dir_out

[Linus Walleij]

I really need Alexander Shiyan to look at this patch.

The way i percieve it, .set is NULL if the chip does not
support output.

We should print the right error messages and bail out
if the user is anyway trying to set a line like that.

Yours,
Linus Walleij

On Wed, Aug 30, 2017 at 4:50 PM, Vladimir Murzin
<vladimir.murzin@arm.com> wrote:

> "set" callback is optional and can be NULL, instead use chip->set
> which always points at proper callback function.
>
> Fixes 2c341d62eb4b ("gpio: syscon: add soc specific callback to assign output value")
>
> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> ---
>  drivers/gpio/gpio-syscon.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c
> index 537cec7..cf88a0b 100644
> --- a/drivers/gpio/gpio-syscon.c
> +++ b/drivers/gpio/gpio-syscon.c
> @@ -122,7 +122,7 @@ static int syscon_gpio_dir_out(struct gpio_chip *chip, unsigned offset, int val)
>                                    BIT(offs % SYSCON_REG_BITS));
>         }
>
> -       priv->data->set(chip, offset, val);
> +       chip->set(chip, offset, val);
>
>         return 0;
>  }
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] gpio: syscon: do not use raw "set" callback in syscon_gpio_dir_out

[Alexander Shiyan]

>Четверг, 21 сентября 2017, 14:23 +03:00 от Linus Walleij <linus.walleij@linaro.org>:
>
>I really need Alexander Shiyan to look at this patch.
>
>The way i percieve it, .set is NULL if the chip does not
>support output.
>
>We should print the right error messages and bail out
>if the user is anyway trying to set a line like that.

Hello.

Using "chip->set", instead of "priv->data->set", is more proper way on my opinion.
However, if the driver is not configured for output, the any errors should not occur in any case.

>On Wed, Aug 30, 2017 at 4:50 PM, Vladimir Murzin
>< vladimir.murzin@arm.com > wrote:
>
>> "set" callback is optional and can be NULL, instead use chip->set
>> which always points at proper callback function.
>>
>> Fixes 2c341d62eb4b ("gpio: syscon: add soc specific callback to assign output value")
>>
>> Signed-off-by: Vladimir Murzin < vladimir.murzin@arm.com >
>> ---
>>  drivers/gpio/gpio-syscon.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c
>> index 537cec7..cf88a0b 100644
>> --- a/drivers/gpio/gpio-syscon.c
>> +++ b/drivers/gpio/gpio-syscon.c
>> @@ -122,7 +122,7 @@ static int syscon_gpio_dir_out(struct gpio_chip *chip, unsigned offset, int val)
>>                                    BIT(offs % SYSCON_REG_BITS));
>>         }
>>
>> -       priv->data->set(chip, offset, val);
>> +       chip->set(chip, offset, val);
>>
>>         return 0;
>>  }

---

Re: [PATCH] gpio: syscon: do not use raw "set" callback in syscon_gpio_dir_out

[Vladimir Murzin]

On 21/09/17 12:56, Alexander Shiyan wrote:
>> Четверг, 21 сентября 2017, 14:23 +03:00 от Linus Walleij <linus.walleij@linaro.org>:
>>
>> I really need Alexander Shiyan to look at this patch.
>>
>> The way i percieve it, .set is NULL if the chip does not
>> support output.
>>
>> We should print the right error messages and bail out
>> if the user is anyway trying to set a line like that.
> 
> Hello.
> 
> Using "chip->set", instead of "priv->data->set", is more proper way on my opinion.
> However, if the driver is not configured for output, the any errors should not occur in any case.

So what is conclusion on this? I agree that there is nothing broken atm, but I faced
the issues when I tried to use gpio-syscon to fit into my case which is very
similar to those pseudo-GPIOs in drivers/mfd/vexpress-sysreg.c

Thanks
Vladimir

> 
>> On Wed, Aug 30, 2017 at 4:50 PM, Vladimir Murzin
>> < vladimir.murzin@arm.com > wrote:
>>
>>> "set" callback is optional and can be NULL, instead use chip->set
>>> which always points at proper callback function.
>>>
>>> Fixes 2c341d62eb4b ("gpio: syscon: add soc specific callback to assign output value")
>>>
>>> Signed-off-by: Vladimir Murzin < vladimir.murzin@arm.com >
>>> ---
>>>  drivers/gpio/gpio-syscon.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c
>>> index 537cec7..cf88a0b 100644
>>> --- a/drivers/gpio/gpio-syscon.c
>>> +++ b/drivers/gpio/gpio-syscon.c
>>> @@ -122,7 +122,7 @@ static int syscon_gpio_dir_out(struct gpio_chip *chip, unsigned offset, int val)
>>>                                    BIT(offs % SYSCON_REG_BITS));
>>>         }
>>>
>>> -       priv->data->set(chip, offset, val);
>>> +       chip->set(chip, offset, val);
>>>
>>>         return 0;
>>>  }
> 
> ---
> 

Re: [PATCH] gpio: syscon: do not use raw "set" callback in syscon_gpio_dir_out

[Linus Walleij]

On Thu, Sep 21, 2017 at 1:56 PM, Alexander Shiyan <shc_work@mail.ru> wrote:
>>Четверг, 21 сентября 2017, 14:23 +03:00 от Linus Walleij <linus.walleij@linaro.org>:
>>
>>I really need Alexander Shiyan to look at this patch.
>>
>>The way i percieve it, .set is NULL if the chip does not
>>support output.
>>
>>We should print the right error messages and bail out
>>if the user is anyway trying to set a line like that.
>
> Hello.
>
> Using "chip->set", instead of "priv->data->set", is more proper way on my opinion.
> However, if the driver is not configured for output, the any errors should not occur in any case.

Is that an Acked-by?

Yours,
Linus Walleij

Re: [PATCH] gpio: syscon: do not use raw "set" callback in syscon_gpio_dir_out

[Linus Walleij]

On Fri, Sep 22, 2017 at 12:23 PM, Vladimir Murzin
<vladimir.murzin@arm.com> wrote:

> I tried to use gpio-syscon to fit into my case which is very
> similar to those pseudo-GPIOs in drivers/mfd/vexpress-sysreg.c

I do not like what that driver is doing and it should not be
taken as inspiration.

I have several times slammed down on people trying to shoehorn
things that are not GPIO into the GPIO subsystem just out of
convenience.

The question to ask is always: is this bit/line/pin really
"general purpose input/output"?

Not "how can I quickly code up something that makes this
thing work?"

See for example drivers/leds/leds-syscon.c
That was my response to someone trying to first use
syscon-gpio on a register and then gpio-leds on top of
that, hilarious layers of indirection!

If the use case is MMC card detect, we need card detection
using syscon directly in the MMC subsystem, not hacks
like what the Vexpress MFD driver is doing.

I have the Versatile Express in my office and one day I will
fix up this thing.

Yours,
Linus Walleij

Re: [PATCH] gpio: syscon: do not use raw "set" callback in syscon_gpio_dir_out

[Vladimir Murzin]

On 22/09/17 14:47, Linus Walleij wrote:
> On Fri, Sep 22, 2017 at 12:23 PM, Vladimir Murzin
> <vladimir.murzin@arm.com> wrote:
> 
>> I tried to use gpio-syscon to fit into my case which is very
>> similar to those pseudo-GPIOs in drivers/mfd/vexpress-sysreg.c
> 
> I do not like what that driver is doing and it should not be
> taken as inspiration.
> 
> I have several times slammed down on people trying to shoehorn
> things that are not GPIO into the GPIO subsystem just out of
> convenience.
> 

It is why I submitted only this patch, if you think there is no
issue I'm fine :)

> The question to ask is always: is this bit/line/pin really
> "general purpose input/output"?

In my case definitely it is not. These are some bits which 
control simple CLCD panel [1]. The only reason I've looked
into gpio-syscon is that I saw you forced keystone bits in
there with that "it is not general purpose" reason, but
from your response it seems it is not the right place for
such stuff.

> 
> Not "how can I quickly code up something that makes this
> thing work?"
> 
> See for example drivers/leds/leds-syscon.c
> That was my response to someone trying to first use
> syscon-gpio on a register and then gpio-leds on top of
> that, hilarious layers of indirection!
> 
> If the use case is MMC card detect, we need card detection
> using syscon directly in the MMC subsystem, not hacks
> like what the Vexpress MFD driver is doing.

No, it is not MMC card detect.

> 
> I have the Versatile Express in my office and one day I will
> fix up this thing.

Understood.

[1] http://infocenter.arm.com/help/topic/com.arm.doc.dai0399c/index.html#arm_toc19 (FPGAIO->MISC)

Cheers
Vladimir

> 
> Yours,
> Linus Walleij
> 

Re: [PATCH] gpio: syscon: do not use raw "set" callback in syscon_gpio_dir_out

[Alexander Shiyan]

>Пятница, 22 сентября 2017, 16:42 +03:00 от Linus Walleij <linus.walleij@linaro.org>:
>
>On Thu, Sep 21, 2017 at 1:56 PM, Alexander Shiyan < shc_work@mail.ru > wrote:
>>>Четверг, 21 сентября 2017, 14:23 +03:00 от Linus Walleij < linus.walleij@linaro.org >:
>>>
>>>I really need Alexander Shiyan to look at this patch.
>>>
>>>The way i percieve it, .set is NULL if the chip does not
>>>support output.
>>>
>>>We should print the right error messages and bail out
>>>if the user is anyway trying to set a line like that.
>>
>> Hello.
>>
>> Using "chip->set", instead of "priv->data->set", is more proper way on my opinion.
>> However, if the driver is not configured for output, the any errors should not occur in any case.
>
>Is that an Acked-by?
Yes, if this need.

Acked-by: Alexander Shiyan <shc_work@mail.ru>
---