RE: [PATCH v4 01/11] i2c: Enhance i2c_new_ancillary_device API

From: Biju Das <biju.das.jz@bp.renesas.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: "Andrzej Hajda" <andrzej.hajda@intel.com>,
	"Neil Armstrong" <neil.armstrong@linaro.org>,
	"Robert Foss" <rfoss@kernel.org>,
	"David Airlie" <airlied@gmail.com>,
	"Daniel Vetter" <daniel@ffwll.ch>,
	"Wolfram Sang" <wsa@kernel.org>,
	"Kieran Bingham" <kieran.bingham@ideasonboard.com>,
	"Mauro Carvalho Chehab" <mchehab@kernel.org>,
	"Hans Verkuil" <hverkuil-cisco@xs4all.nl>,
	"Alessandro Zummo" <a.zummo@towertech.it>,
	"Alexandre Belloni" <alexandre.belloni@bootlin.com>,
	"Laurent Pinchart" <Laurent.pinchart@ideasonboard.com>,
	"Jonas Karlman" <jonas@kwiboo.se>,
	"Jernej Skrabec" <jernej.skrabec@gmail.com>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Corey Minyard" <cminyard@mvista.com>,
	"Marek Behún" <kabel@kernel.org>,
	"Jiasheng Jiang" <jiasheng@iscas.ac.cn>,
	"Antonio Borneo" <antonio.borneo@foss.st.com>,
	"Abhinav Kumar" <quic_abhinavk@quicinc.com>,
	"Ahmad Fatoum" <a.fatoum@pengutronix.de>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
	"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
	"Geert Uytterhoeven" <geert+renesas@glider.be>,
	"Fabrizio Castro" <fabrizio.castro.jz@renesas.com>,
	"linux-renesas-soc@vger.kernel.org"
	<linux-renesas-soc@vger.kernel.org>
Subject: RE: [PATCH v4 01/11] i2c: Enhance i2c_new_ancillary_device API
Date: Fri, 19 May 2023 15:22:06 +0000	[thread overview]
Message-ID: <OS0PR01MB5922CFC8A55898BDD9FCD911867C9@OS0PR01MB5922.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAMuHMdVHZqwW8e9zCZgx2mTBP_Tzcuswo04fxw-DCo__NDFS1w@mail.gmail.com>

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v4 01/11] i2c: Enhance i2c_new_ancillary_device API
> 
> Hi Biju,
> 
> On Thu, May 18, 2023 at 1:37 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > Renesas PMIC RAA215300 exposes two separate i2c devices, one for the
> > main device and another for rtc device.
> >
> > Enhance i2c_new_ancillary_device() to instantiate a real device.
> > (eg: Instantiate rtc device from PMIC driver)
> >
> > Added helper function __i2c_new_dummy_device to share the code between
> > i2c_new_dummy_device and i2c_new_ancillary_device().
> >
> > Also added helper function __i2c_new_client_device() to pass parent
> > dev parameter, so that the ancillary device can assign its parent
> > during creation.
> >
> > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v3->v4:
> >  * Dropped Rb tag from Geert as there are new changes.
> >  * Introduced __i2c_new_dummy_device() to share the code between
> >    i2c_new_dummy_device and i2c_new_ancillary_device().
> >  * Introduced __i2c_new_client_device() to pass parent dev
> >    parameter, so that the ancillary device can assign its parent
> during
> >    creation.
> 
> Thanks for the update!
> 
> LGTM, a few minor comments below.
> 
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -893,24 +893,10 @@ int i2c_dev_irq_from_resources(const struct
> resource *resources,
> >         return 0;
> >  }
> >
> > -/**
> > - * i2c_new_client_device - instantiate an i2c device
> > - * @adap: the adapter managing the device
> > - * @info: describes one I2C device; bus_num is ignored
> > - * Context: can sleep
> > - *
> > - * Create an i2c device. Binding is handled through driver model
> > - * probe()/remove() methods.  A driver may be bound to this device
> > when we
> > - * return from this function, or any later moment (e.g. maybe
> > hotplugging will
> > - * load the driver module).  This call is not appropriate for use by
> > mainboard
> > - * initialization logic, which usually runs during an arch_initcall()
> > long
> > - * before any i2c_adapter could exist.
> > - *
> > - * This returns the new i2c client, which may be saved for later use
> > with
> > - * i2c_unregister_device(); or an ERR_PTR to describe the error.
> > - */
> > -struct i2c_client *
> > -i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info
> > const *info)
> > +static struct i2c_client *
> > +__i2c_new_client_device(struct i2c_adapter *adap,
> > +                       struct i2c_board_info const *info,
> > +                       struct device *dev)
> 
> struct device *parent?

OK. Will use parent below as well(__i2c_new_dummy_device())

> 
> >  {
> >         struct i2c_client       *client;
> >         int                     status;
> 
> > @@ -1054,6 +1062,25 @@ static struct i2c_driver dummy_driver = {
> >         .id_table       = dummy_id,
> >  };
> >
> > +static struct i2c_client *__i2c_new_dummy_device(struct i2c_adapter
> *adapter,
> > +                                                u16 address, const
> char *name,
> > +                                                struct device *dev) {
> > +       struct i2c_board_info info = {
> > +               I2C_BOARD_INFO("dummy", address),
> > +       };
> > +
> > +       if (name) {
> > +               ssize_t ret = strscpy(info.type, name,
> > + sizeof(info.type));
> > +
> > +               if (ret < 0)
> > +                       return ERR_PTR(dev_err_probe(&adapter->dev,
> ret,
> > +                                                    "Invalid device
> > + name\n"));
> 
> %s too long?

Ok, will add %s
			return ERR_PTR(dev_err_probe(&adapter->dev, ret,
						     "Invalid device name: %s\n",
						     name));
> 
> > +       }
> > +
> > +       return __i2c_new_client_device(adapter, &info, dev); }
> > +
> >  /**
> >   * i2c_new_dummy_device - return a new i2c device bound to a dummy
> driver
> >   * @adapter: the adapter managing the device
> 
> > @@ -1122,15 +1145,17 @@ EXPORT_SYMBOL_GPL(devm_i2c_new_dummy_device);
> >   * @client: Handle to the primary client
> >   * @name: Handle to specify which secondary address to get
> >   * @default_addr: Used as a fallback if no secondary address was
> > specified
> > + * @aux_device_name: Ancillary device name
> >   * Context: can sleep
> >   *
> >   * I2C clients can be composed of multiple I2C slaves bound together
> in a single
> >   * component. The I2C client driver then binds to the master I2C
> > slave and needs
> > - * to create I2C dummy clients to communicate with all the other
> slaves.
> > + * to create I2C ancillary clients to communicate with all the other
> slaves.
> >   *
> > - * This function creates and returns an I2C dummy client whose I2C
> > address is
> > - * retrieved from the platform firmware based on the given slave
> > name. If no
> > - * address is specified by the firmware default_addr is used.
> > + * This function creates and returns an I2C ancillary client whose
> > + I2C address
> > + * is retrieved from the platform firmware based on the given slave
> > + name. If no
> > + * address is specified by the firmware default_addr is used. If no
> > + aux_device_
> > + * name is specified by the firmware, it will create an I2C dummy
> client.
> 
> Please add something like:
> 
>     The ancillary's device parent will be set to the primary device.

OK, will add.

> 
> >   *
> >   * On DT-based platforms the address is retrieved from the "reg"
> property entry
> >   * cell whose "reg-names" value matches the slave name.
> 
> > @@ -1153,7 +1179,9 @@ struct i2c_client
> *i2c_new_ancillary_device(struct i2c_client *client,
> >         }
> >
> >         dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n",
> name, addr);
> > -       return i2c_new_dummy_device(client->adapter, addr);
> > +       return __i2c_new_dummy_device(client->adapter, addr,
> > +                                     aux_device_name ?
> > + aux_device_name : NULL,
> 
> You can just pass aux_device_name.

Agreed.

Cheers,
Biju