Hi,
On Wed, Sep 24, 2014 at 10:44:23PM +0200, Robert Jarzmik wrote:
> > On Wed, Sep 24, 2014 at 10:14:53PM +0200, Robert Jarzmik wrote:
> > 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.
> So be it, I'll send it right after this mail.
>
> >
> >> >> - 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.
> This is no patch, there is not a single line in the patch for that. This is just
> a statement, I can remove it if you want.
nah, that's fine then.
> >
> >> >> 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.
>
> You still don't get my point I'm afraid, sic... It's not about "using" the flag,
> it's about "setting" the flag from the driver. Or said differently the assertion
> is "there is not gpiod_*() function which toggles the FLAG_ACTIVE_LOW bit id
> gpio_desc->flags".
>
> I'll wait for Linus on that topic.
might be better, if I'm wrong, I'm wrong; but I'd expect gpiod_* to also
support legacy board-file-based booting so people can convert to DT in
smaller steps.
--
balbi
Attachment:
signature.asc
Description: Digital signature
