Re: [PATCH v5 3/3] iio: acpi_als: Add trigger support

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 29 Dec 2020 19:33:39 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Dec 29, 2020 at 7:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu, 17 Dec 2020 00:38:06 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >  
> > > On Wed, Dec 16, 2020 at 11:41 PM Gwendal Grignou <gwendal@chromium.org> wrote:  
> > > >
> > > > As some firmware does not notify on illuminance changes, add a
> > > > trigger to be able to query light via software (sysfs-trigger or
> > > > hrtrigger).
> > > > Add a hardware trigger set as the default trigger to maintain backward
> > > > compatibility.
> > > >
> > > > Check iio_info reports the sensor as buffer capable:
> > > >   iio:device0: acpi-als (buffer capable)
> > > >
> > > > To test, check we can get data on demand on an Intel based chromebook:
> > > >
> > > >   IIO_DEV="iio:device0"
> > > >   echo 1 > iio_sysfs_trigger/add_trigger
> > > >   cat trigger2/name > ${IIO_DEV}/trigger/current_trigger
> > > >   for i in ${IIO_DEV}/scan_elements/*_en ${IIO_DEV}/buffer/enable ; do
> > > >     echo 1 > $i
> > > >   done
> > > >   od -x /dev/${IIO_DEV} &
> > > >   echo 1 > trigger2/trigger_now  
> > >
> > > Few nitpicks below. After addressing, take my
> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > assuming that backward compatibility has been tested as well.  
> 
> ...
> 
> > > > +       if (!iio_buffer_enabled(indio_dev) ||
> > > > +           !iio_trigger_using_own(indio_dev))  
> > >
> > > I guess it can be located on one line.
> > >
> > > I hope those functions have no side effects. In that case you may
> > > invert logic (save 2 characters)
> > >
> > >        if (iio_buffer_enabled(indio_dev) && iio_trigger_using_own(indio_dev))  
> >
> > You can but at cost of indenting the whole following block on level further.
> > I'm not that fussed, but in general that doesn't seem like a good idea to
> > save two characters here.  
> 
> I didn't get it. The proposed change, in case of no side effect, is an
> equivalent to the existing one, just 2 characters less.
> How does it affect code block indentation?

It's not the same. As stated it's the inverse condition.  Could add some brackets
and a ! or, flip the logic of the whole if condition and not return early.


Jonathan


>