Re: [PATCH v8 7/8] platform/chrome: Introduce device tree hardware prober

[Chen-Yu Tsai]

On Mon, Oct 14, 2024 at 7:23 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> On Mon, Oct 14, 2024 at 12:56:20PM +0800, Chen-Yu Tsai wrote:
> > On Thu, Oct 10, 2024 at 11:32 PM Andy Shevchenko
> > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > > On Thu, Oct 10, 2024 at 06:29:44PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Oct 08, 2024 at 03:34:26PM +0800, Chen-Yu Tsai wrote:
>
> ...
>
> > > > > +   .cfg = &chromeos_i2c_probe_simple_trackpad_cfg,
> > > >
> > > >       .cfg = DEFINE_I2C_OF_PROBE_CFG(trackpad, i2c_of_probe_simple_ops),
> > > >
> > > > Or even
> > > >
> > > > #define DEFINE_I2C_OF_PROBE_CFG_SIMPLE(_type_)                        \
> > > >       DEFINE_I2C_OF_PROBE_CFG(type, &i2c_of_probe_simple_ops)
> >
> > I'm not inclined on using compound literals here. "simple X cfg" will
> > likely get shared between multiple |chromeos_i2c_probe_data| entries,
> > and AFAIK the toolchain can't merge them. So we would end up with one
> > compound literal per entry, even if their contents are the same.
>
> I'm not sure I follow, you are using compound literal _already_.
> How does my proposal change that?

I'm using it where it makes sense, i.e. where the embedded variable
is not going to be shared with other instances.

For the dumb probers, there's only going to be one instance per "type".

For the simple probers, the config part is still one instance per "type",
but the parameters are board and component specific. There will be
multiple instances. Hence the config part can be shared, while the
parameters likely won't be.

> > > With that also looking at the above
> > >
> > > #define DEFINE_I2C_OF_PROBE_CFG_NONE(_type_)                            \
> > >         DEFINE_I2C_OF_PROBE_CFG(type, NULL)
> >
> > For the "dumb" case it makes sense though, since it would be one instance
> > per type. But we could go further and just wrap the whole
> > |chromeos_i2c_probe_data| declaration.
>
> Maybe it's too far from now...

This is what I have:

#define DEFINE_CHROMEOS_I2C_PROBE_DATA_DUMB(_type)
                        \
       static const struct chromeos_i2c_probe_data
chromeos_i2c_probe_dumb_ ## _type = {       \
               .cfg = &(const struct i2c_of_probe_cfg) {
                        \
                       .type = #_type,
                        \
               },
                        \
};

DEFINE_CHROMEOS_I2C_PROBE_DATA_DUMB(touchscreen);


ChenYu