From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754676AbbG3RBt (ORCPT ); Thu, 30 Jul 2015 13:01:49 -0400 Received: from arroyo.ext.ti.com ([192.94.94.40]:50572 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753645AbbG3RBq (ORCPT ); Thu, 30 Jul 2015 13:01:46 -0400 Message-ID: <55BA5857.9030301@ti.com> Date: Thu, 30 Jul 2015 12:01:11 -0500 From: "Andrew F. Davis" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: =?UTF-8?B?UGFsaSBSb2jDoXI=?= CC: Sebastian Reichel , Dmitry Eremin-Solenikov , David Woodhouse , Dan Murphy , , Subject: Re: [PATCH v2 3/6] power: bq27xxx_battery: Platform initialization must declare a device References: <1438112353-19704-1-git-send-email-afd@ti.com> <1438112353-19704-4-git-send-email-afd@ti.com> <201507282213.37788@pali> In-Reply-To: <201507282213.37788@pali> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/28/2015 03:13 PM, Pali Rohár wrote: > On Tuesday 28 July 2015 21:39:10 Andrew F. Davis wrote: >> When initialized as a platform device the initializer must now specify >> a device. An empty device name is no longer valid. >> >> Signed-off-by: Andrew F. Davis >> --- >> drivers/power/bq27xxx_battery.c | 9 ++++++--- >> drivers/w1/slaves/w1_bq27000.c | 1 + >> include/linux/power/bq27xxx_battery.h | 6 +++++- >> 3 files changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/power/bq27xxx_battery.c b/drivers/power/bq27xxx_battery.c >> index e72055c..0734413 100644 >> --- a/drivers/power/bq27xxx_battery.c >> +++ b/drivers/power/bq27xxx_battery.c >> @@ -91,8 +91,6 @@ struct bq27xxx_access_methods { >> int (*read)(struct bq27xxx_device_info *di, u8 reg, bool single); >> }; >> >> -enum bq27xxx_chip { BQ27000, BQ27500, BQ27425, BQ27742, BQ27510}; >> - >> struct bq27xxx_reg_cache { >> int temperature; >> int time_to_empty; >> @@ -1036,6 +1034,11 @@ static int bq27xxx_battery_platform_probe(struct platform_device *pdev) >> return -EINVAL; >> } >> >> + if (!pdata->chip) { > > Will this work? > > Is not first value of enum equal to zero? BQ27000 == 0 ? > We could set the enum to start at 1, but I'm not sure if this check is really necessary at all as we can just assume if the platform info has been supplied then the chip has been set. >> + dev_err(&pdev->dev, "no device supplied\n"); >> + return -EINVAL; >> + } >> + >> di = devm_kzalloc(&pdev->dev, sizeof(*di), GFP_KERNEL); >> if (!di) >> return -ENOMEM; >> @@ -1043,7 +1046,7 @@ static int bq27xxx_battery_platform_probe(struct platform_device *pdev) >> platform_set_drvdata(pdev, di); >> >> di->dev = &pdev->dev; >> - di->chip = BQ27000; >> + di->chip = pdata->chip; >> >> name = pdata->name ?: dev_name(&pdev->dev); >> di->bus.read = &bq27xxx_battery_platform_read; >> diff --git a/drivers/w1/slaves/w1_bq27000.c b/drivers/w1/slaves/w1_bq27000.c >> index 8480531..ba8bf3d 100644 >> --- a/drivers/w1/slaves/w1_bq27000.c >> +++ b/drivers/w1/slaves/w1_bq27000.c >> @@ -42,6 +42,7 @@ static int w1_bq27000_read(struct device *dev, unsigned int reg) >> static struct bq27xxx_platform_data bq27000_battery_info = { >> .read = w1_bq27000_read, >> .name = "bq27000-battery", >> + .chip = BQ27000, > ^ > Just small problem, but for me visible different spacing of previous lines > Fixed. >> }; >> >> static int w1_bq27000_add_slave(struct w1_slave *sl) >> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h >> index e70a93a..70d3865 100644 >> --- a/include/linux/power/bq27xxx_battery.h >> +++ b/include/linux/power/bq27xxx_battery.h >> @@ -3,7 +3,8 @@ >> >> /** >> * struct bq27xxx_plaform_data - Platform data for bq27xxx devices >> - * @name: Name of the battery. If NULL the driver will fallback to "bq27000". >> + * @name: Name of the battery. > > Now I'm trying to understand what name = NULL will do... > > It is this above code? > > name = pdata->name ?: dev_name(&pdev->dev); > > Then it looks like it just set device name. Maybe stupid question, but do we need such property? What other people think? > It doesn't look necessary to me. >> + * @chip: Chip class number of this device. >> * @read: HDQ read callback. >> * This function should provide access to the HDQ bus the battery is >> * connected to. >> @@ -11,8 +12,11 @@ >> * register to be read. The return value should either be the content of >> * the passed register or an error value. >> */ >> +enum bq27xxx_chip { BQ27000, BQ27500, BQ27425, BQ27742, BQ27510 }; >> + >> struct bq27xxx_platform_data { >> const char *name; >> + enum bq27xxx_chip chip; >> int (*read)(struct device *dev, unsigned int); >> }; >> >> >