Hi Biju,
On Thu, May 18, 2023 at 1:37 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> ---
> 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?
> {
> 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?
> + }
> +
> + 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.
> *
> * 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.
> + &client->dev);
> }
> EXPORT_SYMBOL_GPL(i2c_new_ancillary_device);
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
