Re: [PATCHv2 5/6] base: platform: name the device already during allocation
On Mon, Jul 14, 2014 at 07:55:43AM -0700, Greg Kroah-Hartman wrote:
> > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > > index 9e9227e..e856bc4 100644
> > > --- a/drivers/base/platform.c
> > > +++ b/drivers/base/platform.c
> > > @@ -177,11 +177,45 @@ struct platform_object {
> > > */
> > > void platform_device_put(struct platform_device *pdev)
> > > {
> > > - if (pdev)
> > > - put_device(&pdev->dev);
> > > + if (!pdev)
> > > + return;
> > > +
> > > + if (pdev->id_auto) {
> > > + ida_simple_remove(&platform_devid_ida, pdev->id);
> > > + pdev->id = PLATFORM_DEVID_AUTO;
> > > + }
> > > +
> > > + put_device(&pdev->dev);
> > > }
> > > EXPORT_SYMBOL_GPL(platform_device_put);
>
> Why would a single call to this function remove an id? That seems
> really wrong, you should be able to call get and put on a device
> numerous times, only the "last" reference should cause the device to be
> cleaned up.
>
> Shouldn't this be in the release function instead?
I'll fix this.
Thanks Greg.
--
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 5/6] base: platform: name the device already during allocation
On Mon, Jul 14, 2014 at 8:25 PM, Greg Kroah-Hartman
wrote:
> On Mon, Jul 14, 2014 at 11:49:38AM +0530, Kishon Vijay Abraham I wrote:
>> Greg,
>>
>> On Thursday 05 June 2014 06:22 PM, Heikki Krogerus wrote:
>> > This allows resources such as GPIOs and clocks, which can be
>> > matched based on the device name when requested, to be
>> > assigned even when PLATFORM_DEVID_AUTO is used.
>>
>> Any comments on this patch?
>
> I thought I rejected it in the past, don't have my email archives
> around at the moment, sorry.
>
>
>> > Signed-off-by: Heikki Krogerus
Are we planning a different approach for this ?
Since we are basing the phy-calibration patches for phy-exynos5-usbdrd
on this series
and we need to register the phy lookup table in DWC3, and get the PHYs
in xhci-plat.
The following patch in this series (which is based on this change)
help us achieve that :
[PATCHv2 6/6] usb: dwc3: host: convey the PHYs to xhci
(https://lkml.org/lkml/2014/6/5/585)
>> > Cc: Greg Kroah-Hartman
>> > ---
>> > drivers/base/platform.c | 77
>> > ++---
>> > 1 file changed, 47 insertions(+), 30 deletions(-)
>> >
>> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> > index 9e9227e..e856bc4 100644
>> > --- a/drivers/base/platform.c
>> > +++ b/drivers/base/platform.c
>> > @@ -177,11 +177,45 @@ struct platform_object {
>> > */
>> > void platform_device_put(struct platform_device *pdev)
>> > {
>> > - if (pdev)
>> > - put_device(&pdev->dev);
>> > + if (!pdev)
>> > + return;
>> > +
>> > + if (pdev->id_auto) {
>> > + ida_simple_remove(&platform_devid_ida, pdev->id);
>> > + pdev->id = PLATFORM_DEVID_AUTO;
>> > + }
>> > +
>> > + put_device(&pdev->dev);
>> > }
>> > EXPORT_SYMBOL_GPL(platform_device_put);
>
> Why would a single call to this function remove an id? That seems
> really wrong, you should be able to call get and put on a device
> numerous times, only the "last" reference should cause the device to be
> cleaned up.
>
> Shouldn't this be in the release function instead?
>
> thanks,
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 5/6] base: platform: name the device already during allocation
On Mon, Jul 14, 2014 at 11:49:38AM +0530, Kishon Vijay Abraham I wrote:
> Greg,
>
> On Thursday 05 June 2014 06:22 PM, Heikki Krogerus wrote:
> > This allows resources such as GPIOs and clocks, which can be
> > matched based on the device name when requested, to be
> > assigned even when PLATFORM_DEVID_AUTO is used.
>
> Any comments on this patch?
I thought I rejected it in the past, don't have my email archives
around at the moment, sorry.
> > Signed-off-by: Heikki Krogerus
> > Cc: Greg Kroah-Hartman
> > ---
> > drivers/base/platform.c | 77
> > ++---
> > 1 file changed, 47 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 9e9227e..e856bc4 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -177,11 +177,45 @@ struct platform_object {
> > */
> > void platform_device_put(struct platform_device *pdev)
> > {
> > - if (pdev)
> > - put_device(&pdev->dev);
> > + if (!pdev)
> > + return;
> > +
> > + if (pdev->id_auto) {
> > + ida_simple_remove(&platform_devid_ida, pdev->id);
> > + pdev->id = PLATFORM_DEVID_AUTO;
> > + }
> > +
> > + put_device(&pdev->dev);
> > }
> > EXPORT_SYMBOL_GPL(platform_device_put);
Why would a single call to this function remove an id? That seems
really wrong, you should be able to call get and put on a device
numerous times, only the "last" reference should cause the device to be
cleaned up.
Shouldn't this be in the release function instead?
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 5/6] base: platform: name the device already during allocation
Greg,
On Thursday 05 June 2014 06:22 PM, Heikki Krogerus wrote:
> This allows resources such as GPIOs and clocks, which can be
> matched based on the device name when requested, to be
> assigned even when PLATFORM_DEVID_AUTO is used.
Any comments on this patch?
Thanks
Kishon
>
> Signed-off-by: Heikki Krogerus
> Cc: Greg Kroah-Hartman
> ---
> drivers/base/platform.c | 77
> ++---
> 1 file changed, 47 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 9e9227e..e856bc4 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -177,11 +177,45 @@ struct platform_object {
> */
> void platform_device_put(struct platform_device *pdev)
> {
> - if (pdev)
> - put_device(&pdev->dev);
> + if (!pdev)
> + return;
> +
> + if (pdev->id_auto) {
> + ida_simple_remove(&platform_devid_ida, pdev->id);
> + pdev->id = PLATFORM_DEVID_AUTO;
> + }
> +
> + put_device(&pdev->dev);
> }
> EXPORT_SYMBOL_GPL(platform_device_put);
>
> +static int pdev_set_name(struct platform_device *pdev)
> +{
> + int ret;
> +
> + switch (pdev->id) {
> + default:
> + return dev_set_name(&pdev->dev, "%s.%d", pdev->name, pdev->id);
> + case PLATFORM_DEVID_NONE:
> + return dev_set_name(&pdev->dev, "%s", pdev->name);
> + case PLATFORM_DEVID_AUTO:
> + /*
> + * Automatically allocated device ID. We mark it as such so
> + * that we remember it must be freed, and we append a suffix
> + * to avoid namespace collision with explicit IDs.
> + */
> + ret = ida_simple_get(&platform_devid_ida, 0, 0, GFP_KERNEL);
> + if (ret < 0)
> + return ret;
> + pdev->id = ret;
> + pdev->id_auto = true;
> + return dev_set_name(&pdev->dev, "%s.%d.auto", pdev->name,
> + pdev->id);
> + }
> +
> + return 0;
> +}
> +
> static void platform_device_release(struct device *dev)
> {
> struct platform_object *pa = container_of(dev, struct platform_object,
> @@ -214,6 +248,10 @@ struct platform_device *platform_device_alloc(const char
> *name, int id)
> device_initialize(&pa->pdev.dev);
> pa->pdev.dev.release = platform_device_release;
> arch_setup_pdev_archdata(&pa->pdev);
> + if (pdev_set_name(&pa->pdev)) {
> + kfree(pa);
> + return NULL;
> + }
> }
>
> return pa ? &pa->pdev : NULL;
> @@ -294,28 +332,6 @@ int platform_device_add(struct platform_device *pdev)
>
> pdev->dev.bus = &platform_bus_type;
>
> - switch (pdev->id) {
> - default:
> - dev_set_name(&pdev->dev, "%s.%d", pdev->name, pdev->id);
> - break;
> - case PLATFORM_DEVID_NONE:
> - dev_set_name(&pdev->dev, "%s", pdev->name);
> - break;
> - case PLATFORM_DEVID_AUTO:
> - /*
> - * Automatically allocated device ID. We mark it as such so
> - * that we remember it must be freed, and we append a suffix
> - * to avoid namespace collision with explicit IDs.
> - */
> - ret = ida_simple_get(&platform_devid_ida, 0, 0, GFP_KERNEL);
> - if (ret < 0)
> - goto err_out;
> - pdev->id = ret;
> - pdev->id_auto = true;
> - dev_set_name(&pdev->dev, "%s.%d.auto", pdev->name, pdev->id);
> - break;
> - }
> -
> for (i = 0; i < pdev->num_resources; i++) {
> struct resource *p, *r = &pdev->resource[i];
>
> @@ -358,7 +374,6 @@ int platform_device_add(struct platform_device *pdev)
> release_resource(r);
> }
>
> - err_out:
> return ret;
> }
> EXPORT_SYMBOL_GPL(platform_device_add);
> @@ -378,11 +393,6 @@ void platform_device_del(struct platform_device *pdev)
> if (pdev) {
> device_del(&pdev->dev);
>
> - if (pdev->id_auto) {
> - ida_simple_remove(&platform_devid_ida, pdev->id);
> - pdev->id = PLATFORM_DEVID_AUTO;
> - }
> -
> for (i = 0; i < pdev->num_resources; i++) {
> struct resource *r = &pdev->resource[i];
> unsigned long type = resource_type(r);
> @@ -400,8 +410,15 @@ EXPORT_SYMBOL_GPL(platform_device_del);
> */
> int platform_device_register(struct platform_device *pdev)
> {
> + int ret;
> +
> device_initialize(&pdev->dev);
> arch_setup_pdev_archdata(pdev);
> +
> + ret = pdev_set_name(pdev);
> + if (ret)
> + return ret;
> +
> return platform_device_add(pdev);
> }
> EXPORT_SYMBOL_GPL(platform_device_register);
>
--
To unsubscribe from
