mbox series

[v3,0/2] Support accelerometers for veyron_minnie

Message ID 20190624225312.131745-1-gwendal@chromium.org (mailing list archive)
Headers show
Series Support accelerometers for veyron_minnie | expand

Message

Gwendal Grignou June 24, 2019, 10:53 p.m. UTC
veyron_minnie - ASUS Chromebook Flip C100PA - embedded controller
controls two accelerometers, one in the lid, one in the base.
However, the EC firmware does not follow the new interface that
cros_ec_accel driver use.
Extend the legacy driver used on glimmer - Lenovo ThinkPad 11e
Chromebook - to veyron_minnie.
veyron_minnie being ARM based, issue command over the I2C bus to the EC
instead of relying on the shared registers over LPC.

Gwendal Grignou (2):
  iio: cros_ec: Add sign vector in core for backward compatibility
  iio: cros_ec: Extend legacy support to ARM device

Changes in v3:
- Fix commit message, add reviewed-by for first patch.

Changes in v2:
- Readd empty line to reduce amount of change in patch.
- Remove Keywords used by ChromeOS commit queue.

 drivers/iio/accel/Kconfig                     |   4 +-
 drivers/iio/accel/cros_ec_accel_legacy.c      | 350 ++++--------------
 .../cros_ec_sensors/cros_ec_sensors_core.c    |   4 +
 .../linux/iio/common/cros_ec_sensors_core.h   |   1 +
 4 files changed, 84 insertions(+), 275 deletions(-)

Comments

Enric Balletbo i Serra June 25, 2019, 5:09 p.m. UTC | #1
Hi,

On 25/6/19 0:53, Gwendal Grignou wrote:
> veyron_minnie - ASUS Chromebook Flip C100PA - embedded controller
> controls two accelerometers, one in the lid, one in the base.
> However, the EC firmware does not follow the new interface that
> cros_ec_accel driver use.
> Extend the legacy driver used on glimmer - Lenovo ThinkPad 11e
> Chromebook - to veyron_minnie.
> veyron_minnie being ARM based, issue command over the I2C bus to the EC
> instead of relying on the shared registers over LPC.
> 
> Gwendal Grignou (2):
>   iio: cros_ec: Add sign vector in core for backward compatibility
>   iio: cros_ec: Extend legacy support to ARM device
> 
> Changes in v3:
> - Fix commit message, add reviewed-by for first patch.
> 
> Changes in v2:
> - Readd empty line to reduce amount of change in patch.
> - Remove Keywords used by ChromeOS commit queue.
> 
>  drivers/iio/accel/Kconfig                     |   4 +-
>  drivers/iio/accel/cros_ec_accel_legacy.c      | 350 ++++--------------
>  .../cros_ec_sensors/cros_ec_sensors_core.c    |   4 +
>  .../linux/iio/common/cros_ec_sensors_core.h   |   1 +
>  4 files changed, 84 insertions(+), 275 deletions(-)
> 

Just a side note that I think that this patch depends on [1] to have the legacy
sensors working on veyron minnie.

For the full series:

Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>


[1] https://lkml.org/lkml/2019/6/18/268
Jonathan Cameron June 26, 2019, 7:21 p.m. UTC | #2
On Tue, 25 Jun 2019 19:09:50 +0200
Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote:

> Hi,
> 
> On 25/6/19 0:53, Gwendal Grignou wrote:
> > veyron_minnie - ASUS Chromebook Flip C100PA - embedded controller
> > controls two accelerometers, one in the lid, one in the base.
> > However, the EC firmware does not follow the new interface that
> > cros_ec_accel driver use.
> > Extend the legacy driver used on glimmer - Lenovo ThinkPad 11e
> > Chromebook - to veyron_minnie.
> > veyron_minnie being ARM based, issue command over the I2C bus to the EC
> > instead of relying on the shared registers over LPC.
> > 
> > Gwendal Grignou (2):
> >   iio: cros_ec: Add sign vector in core for backward compatibility
> >   iio: cros_ec: Extend legacy support to ARM device
> > 
> > Changes in v3:
> > - Fix commit message, add reviewed-by for first patch.
> > 
> > Changes in v2:
> > - Readd empty line to reduce amount of change in patch.
> > - Remove Keywords used by ChromeOS commit queue.
> > 
> >  drivers/iio/accel/Kconfig                     |   4 +-
> >  drivers/iio/accel/cros_ec_accel_legacy.c      | 350 ++++--------------
> >  .../cros_ec_sensors/cros_ec_sensors_core.c    |   4 +
> >  .../linux/iio/common/cros_ec_sensors_core.h   |   1 +
> >  4 files changed, 84 insertions(+), 275 deletions(-)
> >   
> 
> Just a side note that I think that this patch depends on [1] to have the legacy
> sensors working on veyron minnie.
> 
> For the full series:
> 
> Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Ah. I was assuming you mean a runtime dependency.  Whereas applying this
on it's own is giving me a build problem:

 drivers/iio/accel/cros_ec_accel_legacy.c: In function ‘cros_ec_accel_legacy_read_cmd’:
drivers/iio/accel/cros_ec_accel_legacy.c:50:16: error: ‘struct <anonymous>’ has no member named ‘max_sensor_count’
   50 |  st->param.dump.max_sensor_count = CROS_EC_SENSOR_LEGACY_NUM;
      |                ^
make[3]: *** [scripts/Makefile.build:285: drivers/iio/accel/cros_ec_accel_legacy.o] Error 1
make[2]: *** [scripts/Makefile.build:489: drivers/iio/accel] Error 2
make[1]: *** [scripts/Makefile.build:489: drivers/iio] Error 2

Which I'll assume is related to that other set.

I'm happy for these to go another route to sit on top of that series.
If not I'll pick them up for the next cycle via IIO.

Should they go another route,

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> for both of them.

> 
> 
> [1] https://lkml.org/lkml/2019/6/18/268
Enric Balletbo i Serra June 26, 2019, 8:40 p.m. UTC | #3
Hi Jonathan,

On 26/6/19 21:21, Jonathan Cameron wrote:
> On Tue, 25 Jun 2019 19:09:50 +0200
> Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote:
> 
>> Hi,
>>
>> On 25/6/19 0:53, Gwendal Grignou wrote:
>>> veyron_minnie - ASUS Chromebook Flip C100PA - embedded controller
>>> controls two accelerometers, one in the lid, one in the base.
>>> However, the EC firmware does not follow the new interface that
>>> cros_ec_accel driver use.
>>> Extend the legacy driver used on glimmer - Lenovo ThinkPad 11e
>>> Chromebook - to veyron_minnie.
>>> veyron_minnie being ARM based, issue command over the I2C bus to the EC
>>> instead of relying on the shared registers over LPC.
>>>
>>> Gwendal Grignou (2):
>>>   iio: cros_ec: Add sign vector in core for backward compatibility
>>>   iio: cros_ec: Extend legacy support to ARM device
>>>
>>> Changes in v3:
>>> - Fix commit message, add reviewed-by for first patch.
>>>
>>> Changes in v2:
>>> - Readd empty line to reduce amount of change in patch.
>>> - Remove Keywords used by ChromeOS commit queue.
>>>
>>>  drivers/iio/accel/Kconfig                     |   4 +-
>>>  drivers/iio/accel/cros_ec_accel_legacy.c      | 350 ++++--------------
>>>  .../cros_ec_sensors/cros_ec_sensors_core.c    |   4 +
>>>  .../linux/iio/common/cros_ec_sensors_core.h   |   1 +
>>>  4 files changed, 84 insertions(+), 275 deletions(-)
>>>   
>>
>> Just a side note that I think that this patch depends on [1] to have the legacy
>> sensors working on veyron minnie.
>>
>> For the full series:
>>
>> Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Ah. I was assuming you mean a runtime dependency.  Whereas applying this
> on it's own is giving me a build problem:
> 
>  drivers/iio/accel/cros_ec_accel_legacy.c: In function ‘cros_ec_accel_legacy_read_cmd’:
> drivers/iio/accel/cros_ec_accel_legacy.c:50:16: error: ‘struct <anonymous>’ has no member named ‘max_sensor_count’

Right, this also build depends on the immutable branch (ib-mfd-cros-v5.3).


>    50 |  st->param.dump.max_sensor_count = CROS_EC_SENSOR_LEGACY_NUM;
>       |                ^
> make[3]: *** [scripts/Makefile.build:285: drivers/iio/accel/cros_ec_accel_legacy.o] Error 1
> make[2]: *** [scripts/Makefile.build:489: drivers/iio/accel] Error 2
> make[1]: *** [scripts/Makefile.build:489: drivers/iio] Error 2
> 
> Which I'll assume is related to that other set.
> 
> I'm happy for these to go another route to sit on top of that series.
> If not I'll pick them up for the next cycle via IIO.
> 
> Should they go another route,
> 
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> for both of them.

I'd be happy to pick this patch and add to the chrome-platform tree, ideally
once we manage to get [1] ready for land.

Thanks,
~ Enric

> 
>>
>>
>> [1] https://lkml.org/lkml/2019/6/18/268
>
Jonathan Cameron June 26, 2019, 8:58 p.m. UTC | #4
On Wed, 26 Jun 2019 22:40:00 +0200
Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote:

> Hi Jonathan,
> 
> On 26/6/19 21:21, Jonathan Cameron wrote:
> > On Tue, 25 Jun 2019 19:09:50 +0200
> > Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote:
> >   
> >> Hi,
> >>
> >> On 25/6/19 0:53, Gwendal Grignou wrote:  
> >>> veyron_minnie - ASUS Chromebook Flip C100PA - embedded controller
> >>> controls two accelerometers, one in the lid, one in the base.
> >>> However, the EC firmware does not follow the new interface that
> >>> cros_ec_accel driver use.
> >>> Extend the legacy driver used on glimmer - Lenovo ThinkPad 11e
> >>> Chromebook - to veyron_minnie.
> >>> veyron_minnie being ARM based, issue command over the I2C bus to the EC
> >>> instead of relying on the shared registers over LPC.
> >>>
> >>> Gwendal Grignou (2):
> >>>   iio: cros_ec: Add sign vector in core for backward compatibility
> >>>   iio: cros_ec: Extend legacy support to ARM device
> >>>
> >>> Changes in v3:
> >>> - Fix commit message, add reviewed-by for first patch.
> >>>
> >>> Changes in v2:
> >>> - Readd empty line to reduce amount of change in patch.
> >>> - Remove Keywords used by ChromeOS commit queue.
> >>>
> >>>  drivers/iio/accel/Kconfig                     |   4 +-
> >>>  drivers/iio/accel/cros_ec_accel_legacy.c      | 350 ++++--------------
> >>>  .../cros_ec_sensors/cros_ec_sensors_core.c    |   4 +
> >>>  .../linux/iio/common/cros_ec_sensors_core.h   |   1 +
> >>>  4 files changed, 84 insertions(+), 275 deletions(-)
> >>>     
> >>
> >> Just a side note that I think that this patch depends on [1] to have the legacy
> >> sensors working on veyron minnie.
> >>
> >> For the full series:
> >>
> >> Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>  
> > Ah. I was assuming you mean a runtime dependency.  Whereas applying this
> > on it's own is giving me a build problem:
> > 
> >  drivers/iio/accel/cros_ec_accel_legacy.c: In function ‘cros_ec_accel_legacy_read_cmd’:
> > drivers/iio/accel/cros_ec_accel_legacy.c:50:16: error: ‘struct <anonymous>’ has no member named ‘max_sensor_count’  
> 
> Right, this also build depends on the immutable branch (ib-mfd-cros-v5.3).
Ah. I'd forgotten about that set.

> 
> 
> >    50 |  st->param.dump.max_sensor_count = CROS_EC_SENSOR_LEGACY_NUM;
> >       |                ^
> > make[3]: *** [scripts/Makefile.build:285: drivers/iio/accel/cros_ec_accel_legacy.o] Error 1
> > make[2]: *** [scripts/Makefile.build:489: drivers/iio/accel] Error 2
> > make[1]: *** [scripts/Makefile.build:489: drivers/iio] Error 2
> > 
> > Which I'll assume is related to that other set.
> > 
> > I'm happy for these to go another route to sit on top of that series.
> > If not I'll pick them up for the next cycle via IIO.
> > 
> > Should they go another route,
> > 
> > Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> for both of them.  
> 
> I'd be happy to pick this patch and add to the chrome-platform tree, ideally
> once we manage to get [1] ready for land.
Let's go with that.  If you change your mind, let me know.

Thanks,

Jonathan

> 
> Thanks,
> ~ Enric
> 
> >   
> >>
> >>
> >> [1] https://lkml.org/lkml/2019/6/18/268  
> >