On Wed, 2017-02-08 at 19:45 +0100, Richard Leitner wrote:
> On 02/08/2017 05:40 PM, Andy Shevchenko wrote:
> > On Wed, 2017-02-08 at 16:17 +0100, Richard Leitner wrote:
> > > On 02/08/2017 02:59 PM, Greg KH wrote:
> > > > On Wed, Feb 08, 2017 at 03:21:08PM +0200, Andy Shevchenko wrote:
> > > > > On Wed, 2017-02-08 at 09:52 +0100, Richard Leitner wrote:
> > > > > Above doesn't make much sense. Why not to use
> > > > >
> > > > > > BIT(bit)
> > > > >
> > > > > and
> > > > >
> > > > > & ~BIT(bit)
> > > > >
> > > > > in place?
> > > >
> > > > I thought we already had functions to do this for you. Don't
> > > > write
> > > > new
> > > > ones "by hand" either wya.
> > >
> > > Which functions do you mean? I only found set_bit() and
> > > clear_bit()
> > > from
> > > atomic_ops. But those operate on "unsigned long" variables. From
> > > the
> > > documentation:
> > > Native atomic bit operations are defined to operate
> > > on objects aligned to the size of an "unsigned long"
> > > C data type, and are least of that size.
> >
> > __set_bit(), __clear_bit() -- non-atomic variants, but you are
> > right,
> > that (unsigned long) exactly the point I didn't propose them.
>
> So the preferred solution is the BIT() stuff?
I think BIT() macro in place. Otherwise you'll need a temporary unsigned
long variable. If you already have one, then __*_bit() would work.
> + /* the first data byte transferred tells
> > > > > > the
> > > > > > hub how
> > > > > > many data
> > > > > > + * bytes will follow (byte count)
> > > > > > + */
> > > > >
> > > > > I'm not sure this is good formatted comment for USB subsystem.
> > > >
> > > > Looks fine to me, why do you think it is incorrect?
> >
> > I would do like
> >
> > /*
> > * The multi-line
> > * comment.
> > */
> >
> > Capital letter, period at the end, first empty line (unlike in net
> > subsystem).
>
> So what's the preferred format? Empty line at the beginning or not?
Both fine to me. It's not a real code anyway.
> > > > > > +static int usb251xb_get_ofdata(struct usb251xb *hub,
> > > > > > + struct usb251xb_data *data)
> > > > > > +{
> > > > > > + return 0;
> > > > > > +}
> > > > > > +#endif /* CONFIG_OF */
> > > > >
> > > > > I don't think it's a good idea to have those ugly #ifdef.
> > > >
> > > > How can it be removed?
> >
> > __maybe_unused for function, device_property_*() instead of
> > of_property_*() calls.
> >
> > Something like that. But if you are insisting this is *only* OF
> > available hardware or we don't care, I'll not object.
>
> In usb3503.c and usb4604.c we have that #ifdef CONFIG_OF too. IMHO
> those
> drivers should use the same solution here. But you guys are the ones
> with tons of kernel coding experience, so just say how it should be
> done :-)
I'll agree with whatever Greg wants here.
--
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
