Hi,
On Wed, Sep 24, 2014 at 10:14:53PM +0200, Robert Jarzmik wrote:
> Felipe Balbi <balbi@xxxxxx> writes:
>
> > On Wed, Sep 24, 2014 at 09:41:12PM +0200, Robert Jarzmik wrote:
> >> For this preparation, a preliminary cleanup is done :
> >> - convert the probing of pxa27x_udc to gpio_desc.
> >> The conversion is partial because :
> >> - the platform data still provides a gpio number, not a gpio desc
> >> - the "invert" attribute is lost, hence a loss in the translation
> >
> > I asked for gpio to gpiod conversion to be a separate patch. It'll make
> > things a lot easier to review.
>
> What does patch 1/2 do then ? There are 2 lines for udc_command (1 in .h and
> 1.c), and all the remaining is the gpiod conversion.
>
> Is it difficult for you to review a "51 lines changed" patch you asked for ? Do
> you want me to ask other people to help you ?
if you reply like this again I'll start ignoring your patches. Settle
down, I'm trying to help you get your patches merged.
With that said:
patch 1 should *ONLY* convert gpio_* to gpiod_*. That turns the patch
into a clear cleanup where people don't need to spend too much time
reviewing details.
> >> - convert the mach info, and store the udc_command in the pxa_udc
> >> control structure
this is patch 2.
> >> - loose the udc_is_connected() in mach info
> >> This was not really used, as mioa701 machine doesn't need it,
> >> balloon3 doesn't really use it, and most importantly the current
> >> driver never uses it.
and this is patch 3.
> >> The drawback with the gpio_desc conversion is that the "inverted" gpio
> >> attribute is lost, as no gpiod_*() function exists to set the
> >
> > it's not lost, it's handled for you by the gpio library. Look at how
> > GPIO_ACTIVE_LOW is used.
> It is so, the above assertion "no gpiod_*() function exists" is
> false. Therefore, which is the right function in the gpio library accessible to
> a driver ?
look at the implementation of gpiod_set_value:
1271 void gpiod_set_value(struct gpio_desc *desc, int value)
1272 {
1273 if (!desc)
1274 return;
1275 /* Should be using gpio_set_value_cansleep() */
1276 WARN_ON(desc->chip->can_sleep);
1277 if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
1278 value = !value;
1279 _gpiod_set_raw_value(desc, value);
1280 }
1281 EXPORT_SYMBOL_GPL(gpiod_set_value);
do you see how it handles our "inverted" flag internally. It seems like
what you're missing is a helper to set FLAG_ACTIVE_LOW when !OF &&
!ACPI. If that's really not part of the framework yet, propose a patch
as that would help many others convert to gpio descs and, later, DT.
--
balbi
Attachment:
signature.asc
Description: Digital signature
