| Message ID | 1504104641-8369-1-git-send-email-vladimir.murzin@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: syscon: do not use raw "set" callback in syscon_gpio_dir_out | expand |
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 -- 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
>Четверг, 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; >> } ---
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; >>> } > > --- > -- 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
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 -- 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
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 -- 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
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 > -- 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
>Пятница, 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> ---
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; }
"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(-)