Re: [PATCH v2] i2c: i2c-mux-gpio: Enable this driver in ACPI land

[Evan Green]

On Thu, Oct 15, 2020 at 3:23 AM Peter Rosin <peda@xxxxxxxxxx> wrote:
>
> Hi!
>
> On 2020-10-15 03:02, Evan Green wrote:
> > Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state
> > property translates directly to a fwnode_property_*() call. The child
> > reg property translates naturally into _ADR in ACPI.
> >
> > The i2c-parent binding is a relic from the days when all direct children
> > of an i2c controller in Linux had to be i2c devices. These days that
>
> I2C controller. I2C devices.
>
> I fail to see why this "relic" has to be explicitly blamed on Linux? In the
> beginning, the bindings for all I2C controllers (sometimes implicitely,
> sometimes explicitely) specified that all child nodes had to be I2C devices.
> The *bindings* were simply not as flexible before the i2c-bus subnode was
> invented only a few years ago. So, there are arguments that the "problem"
> was in DT-land and that Linux just followed suit.

Gotcha, that makes sense. I was probably reading between the lines
incorrectly in your previous reply. I'll blame it on the bindings :)

>
> > implementation detail has been worked out, so the i2c-mux can sit
> > as a direct child of its parent controller, which is where it makes the
> > most sense from a hardware description perspective. For the ACPI
> > implementation we'll assume that's always how the i2c-mux-gpio is
> > instantiated.
>
> There is potential to match this and make i2c-parent optional for the
> DT case and require it to be a child of its parent in such cases, if
> someone has the time/energy...

I won't plan to since I don't have a device like this, but yeah I
agree this would be a fine convention for DT to start following as
well.

>
> >
> > Signed-off-by: Evan Green <evgreen@xxxxxxxxxxxx>
> > ---
> >
> > Changes in v2:
> >  - Make it compile properly when !CONFIG_ACPI (Randy)
> >  - Update commit message regarding i2c-parent (Peter)
> >
> >  drivers/i2c/muxes/i2c-mux-gpio.c | 103 ++++++++++++++++++++++---------
> >  1 file changed, 75 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> > index 4effe563e9e8d..8e4008f4a9b5d 100644
> > --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> > +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> > @@ -49,34 +49,80 @@ static int i2c_mux_gpio_deselect(struct i2c_mux_core *muxc, u32 chan)
> >       return 0;
> >  }
> >
> > -#ifdef CONFIG_OF
> > -static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
> > -                                     struct platform_device *pdev)
> > +#ifdef CONFIG_ACPI
> > +
> > +static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
> > +                                  struct fwnode_handle *fwdev,
> > +                                  unsigned int *adr)
> > +
> > +{
> > +     unsigned long long adr64;
> > +     acpi_status status;
> > +
> > +     status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwdev),
> > +                                    METHOD_NAME__ADR,
> > +                                    NULL, &adr64);
> > +
> > +     if (!ACPI_SUCCESS(status)) {
> > +             dev_err(dev, "Cannot get address");
>
> Missing trailing \n

Whoops.

>
> > +             return -EINVAL;
> > +     }
> > +
> > +     *adr = adr64;
>
> Maybe this is too pedantic? Optional, ignore if I'm just being insane...
>
>         if (*adr != adr64) {
>                 dev_err(dev, "Address out of range\n");
>                 return -EINVAL;
>         }
>

Sure, will add.

> > +     return 0;
> > +}
> > +
> > +#else
> > +
> > +static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
> > +                                  struct fwnode_handle *fwdev,
> > +                                  unsigned int *adr)
> > +{
> > +     return -EINVAL;
> > +}
> > +
> > +#endif
> > +
> > +static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
> > +                              struct platform_device *pdev)
> >  {
> > -     struct device_node *np = pdev->dev.of_node;
> > -     struct device_node *adapter_np, *child;
> > -     struct i2c_adapter *adapter;
> > +     struct device *dev = &pdev->dev;
> > +     struct device_node *np = dev->of_node;
> > +     acpi_handle dev_handle;
>
> Remove the dev_handle declaration here...(push)...
>
> > +     struct device_node *adapter_np;
> > +     struct i2c_adapter *adapter = NULL;
> > +     struct fwnode_handle *child = NULL;
>
> Why do you need these two " = NULL" here? I can't believe compilers are
> that stupid? If they are, fine. But otherwise, why single out these two
> pointers and add NULL only there and not everywhere? But NULL everywhere
> would be ugly...

The adapter NULL is there because in theory you could have
!is_acpi_node && !is_of_node (maybe in some weird compile test?), and
then you would be checking an uninitialized value.

child I could safely not initialize, so I'll change that.

>
> >       unsigned *values;
> > -     int i = 0;
> > +     int rc, i = 0;
> >
> > -     if (!np)
> > -             return -ENODEV;
> > +     if (is_of_node(dev->fwnode)) {
> > +             if (!np)
> > +                     return -ENODEV;
> >
> > -     adapter_np = of_parse_phandle(np, "i2c-parent", 0);
> > -     if (!adapter_np) {
> > -             dev_err(&pdev->dev, "Cannot parse i2c-parent\n");
> > -             return -ENODEV;
> > +             adapter_np = of_parse_phandle(np, "i2c-parent", 0);
> > +             if (!adapter_np) {
> > +                     dev_err(&pdev->dev, "Cannot parse i2c-parent\n");
>
> You should do "&pdev->dev" -> "dev" here, because I hate having
> the dev variable and then not use it. But that should perhaps be
> a preparatory patch, because I see more instances and this is an
> orthogonal change.

I see 3 other instances of it in this function. I'll create an initial
patch that introduces the dev local and uses it throughout this
function, then another patch with my main change on top.

>
> > +                     return -ENODEV;
> > +             }
> > +             adapter = of_find_i2c_adapter_by_node(adapter_np);
> > +             of_node_put(adapter_np);
> > +
> > +     } else if (is_acpi_node(dev->fwnode)) {
> > +             /*
> > +              * In ACPI land the mux should be a direct child of the i2c
> > +              * bus it muxes.
> > +              */
> > +             dev_handle = ACPI_HANDLE(dev->parent);
>
> ...(pop)...and perhaps say
>
>                 acpi_handle dev_handle = ACPI_HANDLE(dev->parent);
>
> here?

Will do.

Thanks for the prompt review.
-Evan