[PATCH 4/8] iio: accel: bmc150: Add support for dual-accelerometers with a DUAL250E HID

[Jonathan Cameron]

On Sat, 22 May 2021 19:44:55 +0200
Hans de Goede <hdegoede at redhat.com> wrote:

> Hi,
> 
> On 5/22/21 7:43 PM, Jonathan Cameron wrote:
> > On Fri, 21 May 2021 19:14:14 +0200
> > Hans de Goede <hdegoede at redhat.com> wrote:
> >   
> >> The Lenovo Yoga 300-11IBR has a ACPI fwnode with a HID of DUAL250E
> >> which contains I2C and IRQ resources for 2 accelerometers, 1 in the
> >> display and one in the base of the device. Add support for this.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> >> ---
> >>  drivers/iio/accel/bmc150-accel-i2c.c | 19 ++++++++++++-------
> >>  1 file changed, 12 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c
> >> index e24ce28a4660..b81e4005788e 100644
> >> --- a/drivers/iio/accel/bmc150-accel-i2c.c
> >> +++ b/drivers/iio/accel/bmc150-accel-i2c.c
> >> @@ -24,6 +24,7 @@
> >>  #ifdef CONFIG_ACPI
> >>  static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = {
> >>  	{"BOSC0200"},
> >> +	{"DUAL250E"},
> >>  	{ },
> >>  };
> >>  
> >> @@ -35,21 +36,24 @@ static void bmc150_acpi_dual_accel_probe(struct i2c_client *client)
> >>  {
> >>  	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> >>  	struct i2c_client *second_dev;
> >> +	char dev_name[16];  
> > 
> > I'm a bit in two minds about having a fixed length array for this.
> > Obviously this is always big enough (I think a bit too big), but it
> > might be a place where a future bug is introduced.  Perhaps it's worth the dance
> > of a kasprintf and kfree, to avoid that possibility?  
> 
> I would prefer to keep this as is, using malloc + free always leads
> to problems if an error-exit path shows up between the 2.
> 
> But if you've a strong preference for switching to
> kasprintf + kfree I can do that for v2.

Lets leave it as is and I get to be smug if we ever get a bug as a result
(given that way the one you suggest can't happen, so I can't be proved wrong :)

J
> 
> Regards,
> 
> Hans
> 
> 
> 
> >   
> >>  	struct i2c_board_info board_info = {
> >>  		.type = "bmc150_accel",
> >> -		/*
> >> -		 * The 2nd accel sits in the base of 2-in-1s. Note this
> >> -		 * name is static, as there should never be more then 1
> >> -		 * BOSC0200 ACPI node with 2 accelerometers in it.
> >> -		 */
> >> -		.dev_name = "BOSC0200:base",
> >> +		.dev_name = dev_name,
> >>  		.fwnode = client->dev.fwnode,
> >> -		.irq = -ENOENT,
> >>  	};
> >>  
> >>  	if (acpi_match_device_ids(adev, bmc150_acpi_dual_accel_ids))
> >>  		return;
> >>  
> >> +	/*
> >> +	 * The 2nd accel sits in the base of 2-in-1s. The suffix is static, as
> >> +	 * there should never be more then 1 ACPI node with 2 accelerometers in it.
> >> +	 */
> >> +	snprintf(dev_name, sizeof(dev_name), "%s:base", acpi_device_hid(adev));
> >> +
> >> +	board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 1);
> >> +
> >>  	second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info);
> >>  	if (!IS_ERR(second_dev))
> >>  		bmc150_set_second_device(client, second_dev);
> >> @@ -114,6 +118,7 @@ static const struct acpi_device_id bmc150_accel_acpi_match[] = {
> >>  	{"BMA222E",	bma222e},
> >>  	{"BMA0280",	bma280},
> >>  	{"BOSC0200"},
> >> +	{"DUAL250E"},
> >>  	{ },
> >>  };
> >>  MODULE_DEVICE_TABLE(acpi, bmc150_accel_acpi_match);  
> >   
>