On Tue, Aug 28, 2012 at 7:59 PM, Marek Vasut <marek.vasut@xxxxxxxxx> wrote:
> Dear Bryan Wu,
>
>> Hi Marek,
>>
>> I think your patch is right here. I was confused by the
>> devm_pinctrl_get_select_default() which returns a pointer to pinctrl
>> instant.
>> But there is no code use that pinctrl. From the documentation and some
>> examples in drivers/, it helps to set proper pin configuration for
>> GPIO usage which depends on the DT setting and pinctrl maps. So we
>> need setup such pinctrl maps in the right place, otherwise this API
>> call will return error.
>>
>> Looks like we only use *pinctrl as a return value of
>> devm_pinctrl_get_select_default() instead of a real handler for
>> further pinctrl operation.
>>
>> I'm OK with this patch now and will apply it to my for-next branch.
>
> Please wait with that, as if there's no pinctrl in the DT file, it'll also error
> out. This will cause breakage, I'll send a V2.
>
Right, it will return error without those setting in DT file. But I
found there are many users of leds-gpio driver in kernel, so probably
adding configuration into DT will take some time, how about just
popping out a WARNING message and not return error right now. After
most of the users add proper DT setting, we can change it back to
return error.
Actually I don't find any such setting in imx53 related DT files, but
devm_pinctrl_get_select_default() is used widely in imx drivers like
i2c, spi, uart and etc. Did I miss something, Shawn?
Thanks,
-Bryan
>> Add Stephen to this loop, since he introduce this API.
>>
>> Thanks,
>> -Bryan
>>
>> On Mon, Aug 27, 2012 at 6:05 PM, Marek Vasut <marek.vasut@xxxxxxxxx> wrote:
>> > Dear Bryan Wu,
>> >
>> > [...]
>> >
>> >> >> Maybe I don't understand how to use this 'pinctrl' in this patch,
>> >> >> since I didn't see any code to use that.
>> >> >> Could you please post code to configure the pins in gpio-leds driver?
>> >> >
>> >> > It's not code that uses this, it's a DT property. But then, if the DT
>> >> > property is not present, this fails ... so this patch is wrong. The
>> >> > pinctrl is used by simply specifying the "pinctrl-names" and
>> >> > "pinctrl-0" properties in the DT file.
>> >>
>> >> I know this. What I don't understand of this patch is "how to use
>> >> 'struct pinctrl *pinctrl;' which you get from DT tree"?
>> >> So if there is no such property in DT, it will return an error, I
>> >> don't think this is necessary.
>> >
>> > Indeed, so the error should be converted into warning, correct?
>> >
>> >> And if there is a property in DT, the *pinctrl will point to some
>> >> instant. but there is no code to use this instant to control any pins
>> >> in leds-gpio driver.
>> >
>> > The way I understand it is that the devm_pinctrl_get_select_default()
>> > call configures the pin multiplexing, therefore further operation with
>> > the device is possible. In this case, the pins are multiplexed as GPIO.
>> >
>> >> And why we need this to control pins in leds-gpio
>> >> driver, since we can use gpio generic API to do that?
>> >
>> > You mean the hog gpios? I guess that's indeed possible, but I expected
>> > putting this under the leds-gpio would be more correct or maybe more
>> > systematic. Am I wrong?
>> >
>> >> Thanks,
>> >> -Bryan
>> >>
>> >> > But I wonder how to go about this.
>> >> >
>> >> >> -Bryan
>> >> >>
>> >> >> >> > Signed-off-by: Marek Vasut <marex@xxxxxxx>
>> >> >> >> > Cc: Bryan Wu <bryan.wu@xxxxxxxxxxxxx>
>> >> >> >> > Cc: Richard Purdie <rpurdie@xxxxxxxxx>
>> >> >> >> > Cc: Grant Likely <grant.likely@xxxxxxxxxxxx>
>> >> >> >> > Cc: Rob Herring <rob.herring@xxxxxxxxxxx>
>> >> >> >> > Cc: linux-leds@xxxxxxxxxxxxxxx
>> >> >> >> > ---
>> >> >> >> >
>> >> >> >> > drivers/leds/leds-gpio.c | 6 ++++++
>> >> >> >> > 1 file changed, 6 insertions(+)
>> >> >> >> >
>> >> >> >> > diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
>> >> >> >> > index cde85ba..877d69e 100644
>> >> >> >> > --- a/drivers/leds/leds-gpio.c
>> >> >> >> > +++ b/drivers/leds/leds-gpio.c
>> >> >> >> > @@ -20,6 +20,7 @@
>> >> >> >> >
>> >> >> >> > #include <linux/slab.h>
>> >> >> >> > #include <linux/workqueue.h>
>> >> >> >> > #include <linux/module.h>
>> >> >> >> >
>> >> >> >> > +#include <linux/pinctrl/consumer.h>
>> >> >> >> >
>> >> >> >> > struct gpio_led_data {
>> >> >> >> >
>> >> >> >> > struct led_classdev cdev;
>> >> >> >> >
>> >> >> >> > @@ -234,8 +235,13 @@ static int __devinit gpio_led_probe(struct
>> >> >> >> > platform_device *pdev)
>> >> >> >> >
>> >> >> >> > {
>> >> >> >> >
>> >> >> >> > struct gpio_led_platform_data *pdata =
>> >> >> >> > pdev->dev.platform_data; struct gpio_leds_priv *priv;
>> >> >> >> >
>> >> >> >> > + struct pinctrl *pinctrl;
>> >> >> >> >
>> >> >> >> > int i, ret = 0;
>> >> >> >> >
>> >> >> >> > + pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
>> >> >> >> > + if (IS_ERR(pinctrl))
>> >> >> >> > + return PTR_ERR(pinctrl);
>> >> >> >> > +
>> >> >> >> >
>> >> >> >> > if (pdata && pdata->num_leds) {
>> >> >> >> >
>> >> >> >> > priv = devm_kzalloc(&pdev->dev,
>> >> >> >> >
>> >> >> >> > sizeof_gpio_leds_priv(pdata->num
>> >> >> >> > _le ds) ,
>> >> >> >> >
>> >> >> >> > --
>> >> >> >> > 1.7.10.4
>> >> >> >
>> >> >> > Best regards,
>> >> >> > Marek Vasut
>> >> >
>> >> > Best regards,
>> >> > Marek Vasut
> --
> To unsubscribe from this list: send the line "unsubscribe linux-leds" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Bryan Wu <bryan.wu@xxxxxxxxxxxxx>
Kernel Developer +86.186-168-78255 Mobile
Canonical Ltd. www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
